Re: [PATCH] leds: trigger/tty: feature data direction

[Uwe Kleine-König]

Hello Jürgen,

On Thu, Apr 22, 2021 at 09:47:02AM +0200, Juergen Borleis wrote:
> The current implementation just signals a visible feedback on all kind of
> activity on the corresponding TTY. But sometimes it is useful to see what
> kind of activity just happens. This change adds the capability to filter
> the direction of TTY's data flow. It enables a user to forward both
> directions to separate LEDs for tx and rx on demand. Default behavior is
> still both directions.
> 
> Signed-off-by: Juergen Borleis <jbe@xxxxxxxxxxxxxx>
> ---
>  Documentation/leds/ledtrig-tty.rst | 47 ++++++++++++++++++++++++++

I think putting the change to the documentation into a separate patch is
a good idea as it explains the usage in general and not only adapts it
to the changes to the source. Other than that: Thanks for this document.

>  drivers/leds/trigger/ledtrig-tty.c | 53 +++++++++++++++++++++++++++++-
>  2 files changed, 99 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/leds/ledtrig-tty.rst
> 
> diff --git a/Documentation/leds/ledtrig-tty.rst b/Documentation/leds/ledtrig-tty.rst
> new file mode 100644
> index 00000000..6fc765c
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-tty.rst
> @@ -0,0 +1,47 @@
> +===============
> +LED TTY Trigger
> +===============
> +
> +This LED trigger flashes the LED whenever some data flows are happen on the
> +corresponding TTY device. The TTY device can be freely selected, as well as the
> +data flow direction.
> +
> +TTY trigger can be enabled and disabled from user space on led class devices,
> +that support this trigger as shown below::
> +
> +	echo tty > trigger
> +	echo none > trigger
> +
> +This trigger exports two properties, 'ttyname' and 'dirfilter'. When the
> +tty trigger is activated both properties are set to default values, which means
> +no related TTY device yet and the LED would flash on both directions.
> +
> +Selecting a corresponding trigger TTY::
> +
> +	echo ttyS0 > ttyname
> +
> +This LED will now flash on data flow in both directions of 'ttyS0'.
> +
> +Selecting a direction::
> +
> +	echo in > dirfilter
> +	echo out > dirfilter
> +	echo inout > dirfilter
> +
> +This selection will flash the LED on data flow in the selected direction.
> +
> +Example
> +=======
> +
> +With the 'dirfilter' property one can use two LEDs to give a user a separate
> +visual feedback about data flow.
> +
> +Flash on data send on one LED::
> +
> +	echo ttyS0 > ttyname
> +	echo out > dirfilter
> +
> +Flash on data receive on a second LED::
> +
> +	echo ttyS0 > ttyname
> +	echo in > dirfilter
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index f62db7e..d3bd231 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -14,6 +14,8 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	unsigned indirection:1;
> +	unsigned outdirection:1;
>  };
>  
>  static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> @@ -76,6 +78,47 @@ static ssize_t ttyname_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(ttyname);
>  
> +static ssize_t dirfilter_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +
> +	if (trigger_data->indirection)
> +		return (ssize_t)sprintf(buf, "in\n");
> +	if (trigger_data->outdirection)
> +		return (ssize_t)sprintf(buf, "out\n");
> +	return (ssize_t)sprintf(buf, "inout\n");

I would prefer to call this TX and RX to match the UART lines. This
would then allow to expand the trigger to also blink on handshaking
signals or RI. Then maybe a file per signal is sensible?

> +}
> +
> +static ssize_t dirfilter_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t size)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	ssize_t ret = size;
> +
> +	if (size > 0 && buf[size - 1] == '\n')
> +		size -= 1;
> +
> +	if (size) {
> +		if (!strncmp(buf, "in", size)) {
> +			trigger_data->indirection = 1;
> +			trigger_data->outdirection = 0;
> +			return ret;
> +		}
> +		if (!strncmp(buf, "out", size)) {
> +			trigger_data->indirection = 0;
> +			trigger_data->outdirection = 1;
> +			return ret;
> +		}
> +	}
> +
> +	trigger_data->indirection = 0;
> +	trigger_data->outdirection = 0;

It would be natural to have these two = 1 if "inout" is written.

> +	return ret;
> +}
> +static DEVICE_ATTR_RW(dirfilter);
> +
>  static void ledtrig_tty_work(struct work_struct *work)
>  {
>  	struct ledtrig_tty_data *trigger_data =
> @@ -122,7 +165,14 @@ static void ledtrig_tty_work(struct work_struct *work)
>  
>  	if (icount.rx != trigger_data->rx ||
>  	    icount.tx != trigger_data->tx) {
> -		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		if (trigger_data->indirection) {
> +			if (icount.rx != trigger_data->rx)
> +				led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		} else if (trigger_data->outdirection) {
> +			if (icount.tx != trigger_data->tx)
> +				led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		} else
> +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);

With the above suggestion, this can be simplified to:

	if ((icount.rx != trigger_data->rx && trigger_data->indirection) ||
	    (icount.tx != trigger_data->tx && trigger_data->outdirection)) {
		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
	    	
		...
		

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature