Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
Date:   Sat, 28 Oct 2023 12:43:40 +0200
From:   m.brock@vanmierlo.com
To:     Florian Eckert <fe@dev.tdt.de>
Cc:     Eckert.Florian@googlemail.com, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, pavel@ucw.cz, lee@kernel.org,
        kabel@kernel.org, u.kleine-koenig@pengutronix.de,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        linux-leds@vger.kernel.org
Subject: Re: [PATCH v5 2/2] leds: ledtrig-tty: add new line mode evaluation
In-Reply-To: <20231023094205.2706812-3-fe@dev.tdt.de>
References: <20231023094205.2706812-1-fe@dev.tdt.de>
 <20231023094205.2706812-3-fe@dev.tdt.de>
Message-ID: <ddf9439a092576cd18c6e025d0b61602@vanmierlo.com>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Florian Eckert wrote on 2023-10-23 11:42:

> @@ -16,6 +16,28 @@ struct ledtrig_tty_data {
>      const char *ttyname;
>      struct tty_struct *tty;
>      int rx, tx;
> +     unsigned long ttytrigger;
> +};

ttytriggers ?

[...]

>  static void ledtrig_tty_work(struct work_struct *work)
>  {
>  	struct ledtrig_tty_data *trigger_data =
>  		container_of(work, struct ledtrig_tty_data, dwork.work);
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +	unsigned long interval = LEDTRIG_TTY_INTERVAL;
>  	struct serial_icounter_struct icount;
> +	enum led_trigger_tty_state state;
> +	int current_brightness;
> +	int status;
>  	int ret;
> 
> +	state = TTY_LED_DISABLE;
>  	mutex_lock(&trigger_data->mutex);
> 
>  	if (!trigger_data->ttyname) {
> @@ -115,22 +218,74 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
>  		trigger_data->tty = tty;
>  	}
> 
> -	ret = tty_get_icount(trigger_data->tty, &icount);
> -	if (ret) {
> -		dev_info(trigger_data->tty->dev, "Failed to get icount, stopped 
> polling\n");
> -		mutex_unlock(&trigger_data->mutex);
> -		return;
> +	status = tty_get_tiocm(trigger_data->tty);
> +	if (status > 0) {
> +		if (test_bit(TRIGGER_TTY_CTS, &trigger_data->ttytrigger)) {
> +			if (status & TIOCM_CTS)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_DSR, &trigger_data->ttytrigger)) {
> +			if (status & TIOCM_DSR)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_CAR, &trigger_data->ttytrigger)) {
> +			if (status & TIOCM_CAR)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_RNG, &trigger_data->ttytrigger)) {
> +			if (status & TIOCM_RNG)
> +				state = TTY_LED_ENABLE;
> +		}
> +	}
> +
> +	/* The rx/tx handling must come after the evaluation of TIOCM_*,
> +	 * since the display for rx/tx has priority
> +	 */
> +	if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) ||
> +	    test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger)) {
> +		ret = tty_get_icount(trigger_data->tty, &icount);
> +		if (ret) {
> +			dev_info(trigger_data->tty->dev, "Failed to get icount, stopped 
> polling\n");
> +			mutex_unlock(&trigger_data->mutex);
> +			return;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) &&
> +		    (icount.tx != trigger_data->tx)) {

You check for TRIGGER_TTY_RX and then compare icount.tx, is that 
correct?

> +			trigger_data->tx = icount.tx;
> +			state = TTY_LED_BLINK;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
> +		    (icount.rx != trigger_data->rx)) {

You check for TRIGGER_TTY_TX and then compare icount.rx, is that 
correct?

> +			trigger_data->rx = icount.rx;
> +			state = TTY_LED_BLINK;
> +		}
>  	}
> 
> -	if (icount.rx != trigger_data->rx ||
> -	    icount.tx != trigger_data->tx) {
> -		unsigned long interval = LEDTRIG_TTY_INTERVAL;
> +	current_brightness = led_cdev->brightness;
> +	if (current_brightness)
> +		led_cdev->blink_brightness = current_brightness;
> 
> +	if (!led_cdev->blink_brightness)
> +		led_cdev->blink_brightness = led_cdev->max_brightness;

Is it OK to override the chosen brightness here?

> +
> +	switch (state) {
> +	case TTY_LED_BLINK:
>  		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
>  				      &interval, 0);

Change trigger_data->led_cdev to simply led_cdev

Shouldn't the led return to the line controlled steady state?
Set an invert variable to true if state was TTY_LED_ENABLE before it got 
set
to TTY_LED_BLINK

How do interval and the frequency of ledtrig_tty_work() relate?

> -
> -		trigger_data->rx = icount.rx;
> -		trigger_data->tx = icount.tx;
> +		break;
> +	case TTY_LED_ENABLE:
> +		led_set_brightness(led_cdev, led_cdev->blink_brightness);
> +		break;
> +	case TTY_LED_DISABLE:
> +		fallthrough;
> +	default:
> +		led_set_brightness(led_cdev, LED_OFF);
> +		break;
>  	}

Maarten