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 <[email protected]>
---
Documentation/leds/ledtrig-tty.rst | 47 ++++++++++++++++++++++++++
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");
+}
+
+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;
+ 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);
trigger_data->rx = icount.rx;
trigger_data->tx = icount.tx;
@@ -137,6 +187,7 @@ static void ledtrig_tty_work(struct work_struct *work)
static struct attribute *ledtrig_tty_attrs[] = {
&dev_attr_ttyname.attr,
+ &dev_attr_dirfilter.attr,
NULL
};
ATTRIBUTE_GROUPS(ledtrig_tty);
--
2.20.1
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 <[email protected]>
> ---
> Documentation/leds/ledtrig-tty.rst | 47 ++++++++++++++++++++++++++
> 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");
sysfs_emit() please.
And you are adding new sysfs files, that requires an update to
Documentation/ABI/ please do so.
But why are you adding random new sysfs values to a class device? That
feels really wrong.
thanks,
greg k-h
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 <[email protected]>
> ---
> 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/ |
Hello Greg,
On Thu, Apr 22, 2021 at 10:05:28AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 22, 2021 at 09:47:02AM +0200, Juergen Borleis wrote:
> > +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");
>
> sysfs_emit() please.
>
> And you are adding new sysfs files, that requires an update to
> Documentation/ABI/ please do so.
I agree to these two suggestions.
> But why are you adding random new sysfs values to a class device? That
> feels really wrong.
This is quite usual for triggers and there is IMHO no way around this.
And it is also save as led_trigger_set() emits an uevent after a trigger
was activated.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi!
> 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.
Do you have actual usecase for this?
For most protocols, you get tx and rx at the same time...
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek