[v5,2/2] leds: ledtrig-tty: add new line mode evaluation

Until now, the LED blinks when data is sent via the tty (rx/tx).
The serial tty interface also supports additional input signals, that can
also be evaluated within this trigger. This change is adding the following
additional input sources, which could be controlled
via the '/sys/class/<leds>/' sysfs interface.

- line_cts:
  DCE is ready to accept data from the DTE (CTS = Clear To  Send). If the
  line state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate CTS.
  If set to 1, the LED will evaluate CTS.

- line_dsr:
  DCE is ready to receive and send data (DSR = Data Set Ready). If the line
  state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate DSR.
  If set to 1, the LED will evaluate DSR.

- line_car:
  DTE is receiving a carrier from the DCE (DCD = Data Carrier Detect). If
  the line state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate CAR (DCD).
  If set to 1, the LED will evaluate CAR (DCD).

- line_rng:
  DCE has detected an incoming ring signal on the telephone line
  (RI = Ring Indicator). If the line state is detected, the LED is
  switched on.
  If set to 0 (default), the LED will not evaluate RNG (RI).
  If set to 1, the LED will evaluate RNG (RI).

Explanation:
DCE = Data Communication Equipment (Modem)
DTE = Data Terminal Equipment (Computer)

In addition to the new line_* entries in sysfs, the indication for the
direction of the transmitted data is independently controllable via the
new rx and tx sysfs entrie now too. These are on by default. Thus the
trigger behaves as before this change.

- rx:
  Signal reception (rx) of data on the named tty device.
  If set to 0, the LED will not blink on reception.
  If set to 1 (default), the LED will blink on reception.

- tx:
  Signal transmission (tx) of data on the named tty device.
  If set to 0, the LED will not blink on transmission.
  If set to 1 (default), the LED will blink on transmission.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
 drivers/leds/trigger/ledtrig-tty.c            | 187 ++++++++++++++++--
 2 files changed, 230 insertions(+), 11 deletions(-)

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

On 2023-10-28 12:43, m.brock@vanmierlo.com wrote:
> 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 ?

Yes that would be nicer name. thanks

> [...]
> 
>>  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?

I would say this is correct. At first I check if the tx path should be 
evaluated
and if this is correct I check if there was a tx transmission during the 
last run.

>> +			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?

I would say this is correct. At first I check if the rx path should be 
evaluated
and if this is correct I check if there was a rx transmission during the 
last run.

>> +			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?

In my setup my brightness in the sysfs path of the LED ist set to '0'.
Even though the tty trigger was configured correctly the LED was not
turned on. If I set max_brightness in this path the LED works correctly.
I would check this a gain if this is still needed.

>> +
>> +	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

Thanks for the hint. I will change this.

> Shouldn't the led return to the line controlled steady state?

Sorry I do not understand your question.

> Set an invert variable to true if state was TTY_LED_ENABLE before it 
> got set
> to TTY_LED_BLINK

No matter whether the LED is on or off beforehand. I understand that the
LED is always on for the first half of the period and off for the rest 
of
the period. I think that is correct and I don't need to make a 
distinction
via invert here. I hope I have understood your comment correctly here.

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

The work is twice as long as of the interval. So the variable
LEDTRIG_TTY_INTERVAL = 50 and the work is scheduled LEDTRIG_TTY_INTERVAL 
* 2.
But that was also before my change.

>> -
>> -		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

Thank you for your feedback. I must say, however, that I am currently in
the process of preparing v6, which will implement the comments and
change requests from 'greg k-h' [1]. The big change here in v6 is, that 
I have
switched to completion and split the change in more reviewable commits.
I will see if your comments can also be incorporated into the new 
approach.

---

Florian

[1] 
https://lore.kernel.org/linux-leds/2023102341-jogger-matching-dded@gregkh/

Florian Eckert wrote on 2023-10-30 09:15:
>>> +	/* 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?
> 
> I would say this is correct. At first I check if the tx path should be 
> evaluated
> and if this is correct I check if there was a tx transmission during
> the last run.

No, you check if the *RX* path should be evaluated! On the bright side: 
this is
fixed in the new patch set.

>>> +			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?
> 
> I would say this is correct. At first I check if the rx path should be 
> evaluated
> and if this is correct I check if there was a rx transmission during
> the last run.

Same difference.

>>> +			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?
> 
> In my setup my brightness in the sysfs path of the LED ist set to '0'.
> Even though the tty trigger was configured correctly the LED was not
> turned on. If I set max_brightness in this path the LED works 
> correctly.
> I would check this a gain if this is still needed.

I see you've dropped this from the new patch set. Thank you.

>> Shouldn't the led return to the line controlled steady state?
> 
> Sorry I do not understand your question.
> 
>> Set an invert variable to true if state was TTY_LED_ENABLE before it 
>> got set
>> to TTY_LED_BLINK
> 
> No matter whether the LED is on or off beforehand. I understand that 
> the
> LED is always on for the first half of the period and off for the rest 
> of
> the period. I think that is correct and I don't need to make a 
> distinction
> via invert here. I hope I have understood your comment correctly here.
> 
>> How do interval and the frequency of ledtrig_tty_work() relate?
> 
> The work is twice as long as of the interval. So the variable
> LEDTRIG_TTY_INTERVAL = 50 and the work is scheduled 
> LEDTRIG_TTY_INTERVAL * 2.
> But that was also before my change.

This explains why you don't necessarily need to invert the blink.
If E.g. both CTS and TX are configured I would expect to see the led 
turn on
once CTS actives and then blink off when something is transmitted. After 
that
I expect to see the led still on because CTS is still active.

Now only because the work interval is 2*LEDTRIG_TTY_INTERVAL and the 
blink
uses an interval of LEDTRIG_TTY_INTERVAL for both on and off the user 
doesn't
notice any difference except maybe a bit of delay of the blink.

If either the work schedule was larger than 2*LEDTRIG_TTY_INTERVAL or 
the on
interval would differ from the off interval the behaviour would differ
noticably.

This is why I recommend to use an invert variable that is set to true 
when
the previous state was TTY_LED_ENABLE.

Maarten

On 2023-11-04 14:59, m.brock@vanmierlo.com wrote:
> Florian Eckert wrote on 2023-10-30 09:15:
> 
>>> Shouldn't the led return to the line controlled steady state?
>> 
>> Sorry I do not understand your question.
>> 
>>> Set an invert variable to true if state was TTY_LED_ENABLE before it 
>>> got set
>>> to TTY_LED_BLINK
>> 
>> No matter whether the LED is on or off beforehand. I understand that 
>> the
>> LED is always on for the first half of the period and off for the rest 
>> of
>> the period. I think that is correct and I don't need to make a 
>> distinction
>> via invert here. I hope I have understood your comment correctly here.
>> 
>>> How do interval and the frequency of ledtrig_tty_work() relate?
>> 
>> The work is twice as long as of the interval. So the variable
>> LEDTRIG_TTY_INTERVAL = 50 and the work is scheduled 
>> LEDTRIG_TTY_INTERVAL * 2.
>> But that was also before my change.
> 
> This explains why you don't necessarily need to invert the blink.
> If E.g. both CTS and TX are configured I would expect to see the led 
> turn on
> once CTS actives and then blink off when something is transmitted. 
> After that
> I expect to see the led still on because CTS is still active.

The evaluation starts again with the next iteration of the work.
And if no data was transferred but CTS was set, the LED is enabled again
but does not flash.

> Now only because the work interval is 2*LEDTRIG_TTY_INTERVAL and the 
> blink
> uses an interval of LEDTRIG_TTY_INTERVAL for both on and off the user 
> doesn't
> notice any difference except maybe a bit of delay of the blink.

That is correct

> If either the work schedule was larger than 2*LEDTRIG_TTY_INTERVAL or 
> the on
> interval would differ from the off interval the behaviour would differ
> noticably.
> 
> This is why I recommend to use an invert variable that is set to true 
> when
> the previous state was TTY_LED_ENABLE.

In the next patch round, I will save the state of the LED and evaluate 
whether
I need to invert the LED if the state of the LED has been set to blink.

> Maarten

Thanks for your feedback

--
Florian