[PATCH v5 0/2] ledtrig-tty: add additional tty state evaluation

[PATCH v5 0/2] ledtrig-tty: add additional tty state evaluation

[Florian Eckert]

Changes in v5:
==============
- Update commit message as request by greg k-h, to make the commit message
  more generic and not focusing on my use case [1]. Thanks for that.
- Removing PATCH v4 1/3 from previous set. This has been already applied to
  tty-testing [2] by greg k-h.
- As requested by greq k-h. I have also made the following changes to
  PATCH v4 3/3 [3].
  * Add a comment to the enum that this is already used for bit evaluation and
    sysfs read and write.
  * Renaming the variable 'unsigned long mode' to 'unsigned long ttytrigger'
    in the ledtrig_tty_data structure to make it clearer that the selected
    triggers are stored there.
  * Using sysfs_emit() function to dump the requestd ttytrigger to userland.
  * Also using the kstrtobool() function to write the selected ttytrigger
    via the sysfs. This values are booleans.
- I also removed the function ledtrig_tty_evaluate() from my last patchset.
  PATCH v4 3/3 [3]. The new API tty_get_tiocm() function is only called once
  now and checked for each ttytrigger bit. Previously this function was
  called for each bit, which is not necessary.

Links:
[1] https://lore.kernel.org/linux-leds/2023102115-stock-scrambled-f7d5@gregkh/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-testing&id=838eb763c3e939a8de8d4c55a17ddcce737685c1
[3] https://lore.kernel.org/linux-leds/20231019112809.881730-4-fe@dev.tdt.de/

Changes in v4:
==============
v4: https://lore.kernel.org/linux-leds/20231019112809.881730-1-fe@dev.tdt.de/
- Merging patch 3/4 into patch number 4/4 from previous series, because
  it fixes a problem that does not exist upstream. This was a note from
  the build robot regarding my change that I added with previous series.
  This change was never upstream and therefore this is not relevant.
- Update the commit message of patch 1/3 of this series, that this commit
  also changes the 'ndashes' to simple dashes. There were no changes, so
  I add the 'Reviewed-by' that the commit received before.
- With this patchset version I have reworked my implementation for the
  evaluation of the additional line state, so that this changes becomes
  smaller. As basis I have used the staged commits from Christian Marangi
  that makes this changes to the netdev trigger. This has already been
  applied to 'for-leds-next-next' by Lee Jones. I adapted this to the
  tty trigger.
  Convert device attr to macro:
  https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=509412749002f4bac4c29f2012fff90c08d8afca
  Unify sysfs and state handling:
  https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=0fd93ac8582627bee9a3c824489f302dff722881

Changes in v3:
==============
v3: https://lore.kernel.org/linux-leds/20231016071332.597654-1-fe@dev.tdt.de/
- Add missing 'kernel test robot' information to the commit message.
- Additional information added to the commit message

Changes in v2:
==============
v2: https://lore.kernel.org/linux-leds/20230928132632.200263-1-fe@dev.tdt.de/
- rename new function from tty_get_mget() to tty_get_tiocm() as
  requested by 'Jiri Slaby'.
- As suggested by 'Jiri Slaby', fixed tabs in function documentation
  throughout the file '/drivers/tty/tty_io.c' in a separate commit.
- Move the variable definition to the top in function
  'ledtrig_tty_work()'.
  This was reported by the 'kernel test robot' after my change in v1.
- Also set the 'max_brightness' to 'blink_brightness' if no
  'blink_brightness' was set. This fixes a problem at startup when the
  brightness is still set to 0 and only 'line_*' is evaluated. I looked
  in the netdev trigger and that's exactly how it's done there.

Changes in v1:
==============
v1: https://lore.kernel.org/linux-leds/20230926093607.59536-1-fe@dev.tdt.de/
This is a follow-up patchset, based on the mailing list discussion from
March 2023 based on the old patchset v8 [1]. I have changed, the LED
trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König.
Links:
[1] https://lore.kernel.org/linux-leds/20230306094113.273988-1-fe@dev.tdt.de/

Florian Eckert (2):
  tty: add new helper function tty_get_tiocm
  leds: ledtrig-tty: add new line mode evaluation

 .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
 drivers/leds/trigger/ledtrig-tty.c            | 187 ++++++++++++++++--
 drivers/tty/tty_io.c                          |  28 ++-
 include/linux/tty.h                           |   1 +
 4 files changed, 253 insertions(+), 17 deletions(-)

-- 
2.30.2

[PATCH v5 1/2] tty: add new helper function tty_get_tiocm

[Florian Eckert]

There is no in-kernel function to get the status register of a tty device
like the TIOCMGET ioctl returns to userspace. Create a new function,
tty_get_tiocm(), to obtain the status register that other portions of the
kernel can call if they need this information, and move the existing
internal tty_tiocmget() function to use this interface.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3299a5d50727..a12f63854ac4 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2494,6 +2494,24 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	return retval;
 }
 
+/**
+ * tty_get_tiocm - get tiocm status register
+ * @tty: tty device
+ *
+ * Obtain the modem status bits from the tty driver if the feature
+ * is supported.
+ */
+int tty_get_tiocm(struct tty_struct *tty)
+{
+	int retval = -ENOTTY;
+
+	if (tty->ops->tiocmget)
+		retval = tty->ops->tiocmget(tty);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(tty_get_tiocm);
+
 /**
  * tty_tiocmget - get modem status
  * @tty: tty device
@@ -2506,14 +2524,12 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
  */
 static int tty_tiocmget(struct tty_struct *tty, int __user *p)
 {
-	int retval = -ENOTTY;
+	int retval;
 
-	if (tty->ops->tiocmget) {
-		retval = tty->ops->tiocmget(tty);
+	retval = tty_get_tiocm(tty);
+	if (retval >= 0)
+		retval = put_user(retval, p);
 
-		if (retval >= 0)
-			retval = put_user(retval, p);
-	}
 	return retval;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index f002d0f25db7..8e4d0b3b12b7 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -421,6 +421,7 @@ int tty_unthrottle_safe(struct tty_struct *tty);
 int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 int tty_get_icount(struct tty_struct *tty,
 		struct serial_icounter_struct *icount);
+int tty_get_tiocm(struct tty_struct *tty);
 int is_current_pgrp_orphaned(void);
 void tty_hangup(struct tty_struct *tty);
 void tty_vhangup(struct tty_struct *tty);
-- 
2.30.2

Re: [PATCH v5 1/2] tty: add new helper function tty_get_tiocm

[Greg KH]

On Mon, Oct 23, 2023 at 11:42:04AM +0200, Florian Eckert wrote:
> There is no in-kernel function to get the status register of a tty device
> like the TIOCMGET ioctl returns to userspace. Create a new function,
> tty_get_tiocm(), to obtain the status register that other portions of the
> kernel can call if they need this information, and move the existing
> internal tty_tiocmget() function to use this interface.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Florian Eckert]

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

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
index 2bf6b24e781b..08127b1a4602 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -4,3 +4,57 @@ KernelVersion:	5.10
 Contact:	linux-leds@vger.kernel.org
 Description:
 		Specifies the tty device name of the triggering tty
+
+What:		/sys/class/leds/<led>/rx
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		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.
+
+What:		/sys/class/leds/<led>/tx
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		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.
+
+car rng
+What:		/sys/class/leds/<led>/line_cts
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to accept data from the DTE (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.
+
+What:		/sys/class/leds/<led>/line_dsr
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to receive and send data (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.
+
+What:		/sys/class/leds/<led>/line_car
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DTE is receiving a carrier from the DCE (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).
+
+What:		/sys/class/leds/<led>/line_cts
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE has detected an incoming ring signal on the telephone
+		line (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).
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 8ae0d2d284af..5c8aea1791cf 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -16,6 +16,28 @@ struct ledtrig_tty_data {
 	const char *ttyname;
 	struct tty_struct *tty;
 	int rx, tx;
+	unsigned long ttytrigger;
+};
+
+/* Indicates which state the LED should now display */
+enum led_trigger_tty_state {
+	TTY_LED_BLINK,
+	TTY_LED_ENABLE,
+	TTY_LED_DISABLE,
+};
+
+/* This enum is used to read and write the ttytrigger selection via the
+ * sysfs entry and also to evaluate the TIOCM_* bits.
+ */
+enum led_trigger_tty_bits {
+	TRIGGER_TTY_RX = 0,
+	TRIGGER_TTY_TX,
+	TRIGGER_TTY_CTS,
+	TRIGGER_TTY_DSR,
+	TRIGGER_TTY_CAR,
+	TRIGGER_TTY_RNG,
+	/* Keep last */
+	__TRIGGER_TTY_MAX,
 };
 
 static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
@@ -78,13 +100,94 @@ static ssize_t ttyname_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(ttyname);
 
+static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
+	enum led_trigger_tty_bits attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int trigger;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+	case TRIGGER_TTY_TX:
+	case TRIGGER_TTY_CTS:
+	case TRIGGER_TTY_DSR:
+	case TRIGGER_TTY_CAR:
+	case TRIGGER_TTY_RNG:
+		trigger = attr;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", test_bit(trigger, &trigger_data->ttytrigger));
+}
+
+static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
+	size_t size, enum led_trigger_tty_bits attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	bool enable;
+	int trigger;
+	int ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+	case TRIGGER_TTY_TX:
+	case TRIGGER_TTY_CTS:
+	case TRIGGER_TTY_DSR:
+	case TRIGGER_TTY_CAR:
+	case TRIGGER_TTY_RNG:
+		trigger = attr;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (enable)
+		set_bit(trigger, &trigger_data->ttytrigger);
+	else
+		clear_bit(trigger, &trigger_data->ttytrigger);
+
+	return size;
+}
+
+#define DEFINE_TTY_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+		struct device_attribute *attr, char *buf) \
+	{ \
+		return ledtrig_tty_attr_show(dev, buf, trigger); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+		struct device_attribute *attr, const char *buf, size_t size) \
+	{ \
+		return ledtrig_tty_attr_store(dev, buf, size, trigger); \
+	} \
+	static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
+DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
+DEFINE_TTY_TRIGGER(line_cts, TRIGGER_TTY_CTS);
+DEFINE_TTY_TRIGGER(line_dsr, TRIGGER_TTY_DSR);
+DEFINE_TTY_TRIGGER(line_car, TRIGGER_TTY_CAR);
+DEFINE_TTY_TRIGGER(line_rng, TRIGGER_TTY_RNG);
+
 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)) {
+			trigger_data->tx = icount.tx;
+			state = TTY_LED_BLINK;
+		}
+
+		if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
+		    (icount.rx != trigger_data->rx)) {
+			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;
+
+	switch (state) {
+	case TTY_LED_BLINK:
 		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
 				      &interval, 0);
-
-		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;
 	}
 
 out:
@@ -141,6 +296,12 @@ static void ledtrig_tty_work(struct work_struct *work)
 
 static struct attribute *ledtrig_tty_attrs[] = {
 	&dev_attr_ttyname.attr,
+	&dev_attr_rx.attr,
+	&dev_attr_tx.attr,
+	&dev_attr_line_cts.attr,
+	&dev_attr_line_dsr.attr,
+	&dev_attr_line_car.attr,
+	&dev_attr_line_rng.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ledtrig_tty);
@@ -153,6 +314,10 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
+	/* Enable default rx/tx LED blink */
+	set_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger);
+	set_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger);
+
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
-- 
2.30.2

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

[Greg KH]

On Mon, Oct 23, 2023 at 11:42:05AM +0200, Florian Eckert wrote:
> 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

nit, one too many spaces in this line.

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

These definitions should be above where you use them.

> 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

"entries"

> 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(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> index 2bf6b24e781b..08127b1a4602 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -4,3 +4,57 @@ KernelVersion:	5.10
>  Contact:	linux-leds@vger.kernel.org
>  Description:
>  		Specifies the tty device name of the triggering tty
> +
> +What:		/sys/class/leds/<led>/rx
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		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.
> +
> +What:		/sys/class/leds/<led>/tx
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		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.

Were these existing files not documented already?  If not, they should
be a separate patch we can take now.

> +
> +car rng

What is that line for?

> +What:		/sys/class/leds/<led>/line_cts

Why are these all "line_" now?  What is wrong with just "cts" and "dsr"
and the like?  That keeps them in sync with the "rx" and "tx" files,
right?

> +Date:		October 2023

October ends in a few days, I think this needs some more work.

> +KernelVersion:	6.7

And trees should probably be closed now, this looks like 6.8 stuff.

> +Description:
> +		DCE is ready to accept data from the DTE (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.
> +
> +What:		/sys/class/leds/<led>/line_dsr
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DCE is ready to receive and send data (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.
> +
> +What:		/sys/class/leds/<led>/line_car
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DTE is receiving a carrier from the DCE (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).
> +
> +What:		/sys/class/leds/<led>/line_cts
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DCE has detected an incoming ring signal on the telephone
> +		line (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).
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 8ae0d2d284af..5c8aea1791cf 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -16,6 +16,28 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	unsigned long ttytrigger;

Please explain why "unsigned long" is needed here.

> +};
> +
> +/* Indicates which state the LED should now display */
> +enum led_trigger_tty_state {
> +	TTY_LED_BLINK,
> +	TTY_LED_ENABLE,
> +	TTY_LED_DISABLE,
> +};

Shouldn't we need these states for rx/tx already?

> +
> +/* This enum is used to read and write the ttytrigger selection via the
> + * sysfs entry and also to evaluate the TIOCM_* bits.
> + */
> +enum led_trigger_tty_bits {
> +	TRIGGER_TTY_RX = 0,
> +	TRIGGER_TTY_TX,
> +	TRIGGER_TTY_CTS,
> +	TRIGGER_TTY_DSR,
> +	TRIGGER_TTY_CAR,
> +	TRIGGER_TTY_RNG,

These are bit values (more on that below), so please explicitly set them
to a value.

> +	/* Keep last */
> +	__TRIGGER_TTY_MAX,

You never use this, so it is not needed at all.

>  };
>  
>  static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> @@ -78,13 +100,94 @@ static ssize_t ttyname_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(ttyname);
>  
> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
> +	enum led_trigger_tty_bits attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int trigger;
> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +	case TRIGGER_TTY_TX:
> +	case TRIGGER_TTY_CTS:
> +	case TRIGGER_TTY_DSR:
> +	case TRIGGER_TTY_CAR:
> +	case TRIGGER_TTY_RNG:
> +		trigger = attr;
> +		break;
> +	default:
> +		return -EINVAL;

How can default ever happen?

> +	}
> +
> +	return sysfs_emit(buf, "%u\n", test_bit(trigger, &trigger_data->ttytrigger));
> +}
> +
> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
> +	size_t size, enum led_trigger_tty_bits attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	bool enable;
> +	int trigger;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret)
> +		return ret;
> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +	case TRIGGER_TTY_TX:
> +	case TRIGGER_TTY_CTS:
> +	case TRIGGER_TTY_DSR:
> +	case TRIGGER_TTY_CAR:
> +	case TRIGGER_TTY_RNG:
> +		trigger = attr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (enable)
> +		set_bit(trigger, &trigger_data->ttytrigger);
> +	else
> +		clear_bit(trigger, &trigger_data->ttytrigger);
> +
> +	return size;
> +}
> +
> +#define DEFINE_TTY_TRIGGER(trigger_name, trigger) \
> +	static ssize_t trigger_name##_show(struct device *dev, \
> +		struct device_attribute *attr, char *buf) \
> +	{ \
> +		return ledtrig_tty_attr_show(dev, buf, trigger); \
> +	} \
> +	static ssize_t trigger_name##_store(struct device *dev, \
> +		struct device_attribute *attr, const char *buf, size_t size) \
> +	{ \
> +		return ledtrig_tty_attr_store(dev, buf, size, trigger); \
> +	} \
> +	static DEVICE_ATTR_RW(trigger_name)
> +
> +DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
> +DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);

Again, I thought we supported rx/tx already?  If so, this should be
split out into "redo the rx/tx into a new style" and then "add new
attributes".  Well maybe, more below...


> +DEFINE_TTY_TRIGGER(line_cts, TRIGGER_TTY_CTS);
> +DEFINE_TTY_TRIGGER(line_dsr, TRIGGER_TTY_DSR);
> +DEFINE_TTY_TRIGGER(line_car, TRIGGER_TTY_CAR);
> +DEFINE_TTY_TRIGGER(line_rng, TRIGGER_TTY_RNG);
> +
>  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;
> +		}

Let's ask why you are using bits at all here.  Why?  What is it helping
with?  Is it faster than a normal "bool" value?  I am guessing not
(requires mask and test instead of just test).

So let's just make these individual values in the structure, that makes
things much simpler and easier and you never have to worry about bit
field values anywhere.  And the end code will be easier to read as well
as probably faster (which matters in this codepath, right?)

> +	}
> +
> +	/* 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");

Not your fault, but this should NOT be "dev_info()" but rather,
"dev_err()", or "dev_warn()".

> +			mutex_unlock(&trigger_data->mutex);

This looks ready-made for the completion.h usage so we always make sure
to drop the mutex when done.

> +			return;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) &&
> +		    (icount.tx != trigger_data->tx)) {
> +			trigger_data->tx = icount.tx;
> +			state = TTY_LED_BLINK;
> +		}
> +
> +		if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
> +		    (icount.rx != trigger_data->rx)) {
> +			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;
> +
> +	switch (state) {
> +	case TTY_LED_BLINK:
>  		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
>  				      &interval, 0);
> -
> -		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;
>  	}
>  
>  out:
> @@ -141,6 +296,12 @@ static void ledtrig_tty_work(struct work_struct *work)
>  
>  static struct attribute *ledtrig_tty_attrs[] = {
>  	&dev_attr_ttyname.attr,
> +	&dev_attr_rx.attr,
> +	&dev_attr_tx.attr,

Again, I thought we had rx/tx already?  If not, how was that controlled
today?

thanks,

greg k-h

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

[Florian Eckert]

On 2023-10-23 12:06, Greg KH wrote:
>> ---
>>  .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
>>  drivers/leds/trigger/ledtrig-tty.c            | 187 
>> ++++++++++++++++--
>>  2 files changed, 230 insertions(+), 11 deletions(-)
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty 
>> b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
>> index 2bf6b24e781b..08127b1a4602 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
>> @@ -4,3 +4,57 @@ KernelVersion:	5.10
>>  Contact:	linux-leds@vger.kernel.org
>>  Description:
>>  		Specifies the tty device name of the triggering tty
>> +
>> +What:		/sys/class/leds/<led>/rx
>> +Date:		October 2023
>> +KernelVersion:	6.7
>> +Description:
>> +		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.
>> +
>> +What:		/sys/class/leds/<led>/tx
>> +Date:		October 2023
>> +KernelVersion:	6.7
>> +Description:
>> +		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.
> 
> Were these existing files not documented already?  If not, they should
> be a separate patch we can take now.
> 
>> +
>> +car rng
> 
> What is that line for?

Oops, my fault!

> 
>> +What:		/sys/class/leds/<led>/line_cts
> 
> Why are these all "line_" now?  What is wrong with just "cts" and "dsr"
> and the like?  That keeps them in sync with the "rx" and "tx" files,
> right?

I can change that, I just thought it makes sense to prefix the line 
state
to emphasize the meaning compared to rx and tx.

>> +Date:		October 2023
> 
> October ends in a few days, I think this needs some more work.

But then that's tricky! I do not know when the 6.8 is released. February 
2024?

>> +KernelVersion:	6.7
> 
> And trees should probably be closed now, this looks like 6.8 stuff.


Ok will change this to 6.8

>> +Description:
>> +		DCE is ready to accept data from the DTE (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.
>> +
>> +What:		/sys/class/leds/<led>/line_dsr
>> +Date:		October 2023
>> +KernelVersion:	6.7
>> +Description:
>> +		DCE is ready to receive and send data (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.
>> +
>> +What:		/sys/class/leds/<led>/line_car
>> +Date:		October 2023
>> +KernelVersion:	6.7
>> +Description:
>> +		DTE is receiving a carrier from the DCE (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).
>> +
>> +What:		/sys/class/leds/<led>/line_cts
>> +Date:		October 2023
>> +KernelVersion:	6.7
>> +Description:
>> +		DCE has detected an incoming ring signal on the telephone
>> +		line (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).
>> diff --git a/drivers/leds/trigger/ledtrig-tty.c 
>> b/drivers/leds/trigger/ledtrig-tty.c
>> index 8ae0d2d284af..5c8aea1791cf 100644
>> --- a/drivers/leds/trigger/ledtrig-tty.c
>> +++ b/drivers/leds/trigger/ledtrig-tty.c
>> @@ -16,6 +16,28 @@ struct ledtrig_tty_data {
>>  	const char *ttyname;
>>  	struct tty_struct *tty;
>>  	int rx, tx;
>> +	unsigned long ttytrigger;
> 
> Please explain why "unsigned long" is needed here.

As described by me a few lines below and wanted by you, I will change 
this to
boolean flags for rx,tx,cts,dsr,car and rng.

> 
>> +};
>> +
>> +/* Indicates which state the LED should now display */
>> +enum led_trigger_tty_state {
>> +	TTY_LED_BLINK,
>> +	TTY_LED_ENABLE,
>> +	TTY_LED_DISABLE,
>> +};
> 
> Shouldn't we need these states for rx/tx already?

Before my change, the LED has only flashed if there was a transmission.
This is now the TTY_LED_BLINK state. I could rename this to
TTY_LED_FLASH if that is more familiar.

>> +
>> +/* This enum is used to read and write the ttytrigger selection via 
>> the
>> + * sysfs entry and also to evaluate the TIOCM_* bits.
>> + */
>> +enum led_trigger_tty_bits {
>> +	TRIGGER_TTY_RX = 0,
>> +	TRIGGER_TTY_TX,
>> +	TRIGGER_TTY_CTS,
>> +	TRIGGER_TTY_DSR,
>> +	TRIGGER_TTY_CAR,
>> +	TRIGGER_TTY_RNG,
> 
> These are bit values (more on that below), so please explicitly set 
> them
> to a value.

OK

> 
>> +	/* Keep last */
>> +	__TRIGGER_TTY_MAX,
> 
> You never use this, so it is not needed at all.

I saw it in other source files, so I added it. I thought this was
common in the kernel. If this is not the case I will remove this in
the next round.

>>  };
>> 
>>  static void ledtrig_tty_restart(struct ledtrig_tty_data 
>> *trigger_data)
>> @@ -78,13 +100,94 @@ static ssize_t ttyname_store(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RW(ttyname);
>> 
>> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
>> +	enum led_trigger_tty_bits attr)
>> +{
>> +	struct ledtrig_tty_data *trigger_data = 
>> led_trigger_get_drvdata(dev);
>> +	int trigger;
>> +
>> +	switch (attr) {
>> +	case TRIGGER_TTY_RX:
>> +	case TRIGGER_TTY_TX:
>> +	case TRIGGER_TTY_CTS:
>> +	case TRIGGER_TTY_DSR:
>> +	case TRIGGER_TTY_CAR:
>> +	case TRIGGER_TTY_RNG:
>> +		trigger = attr;
>> +		break;
>> +	default:
>> +		return -EINVAL;
> 
> How can default ever happen?

If I use the DEFINE_TTY_TRIGGER then this cannot happen.
This is an artifact from when I didn't used the DEFINE_TTY_TRIGGER 
macro.
I will remove it in the next round.

>> +	}
>> +
>> +	return sysfs_emit(buf, "%u\n", test_bit(trigger, 
>> &trigger_data->ttytrigger));
>> +}
>> +
>> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char 
>> *buf,
>> +	size_t size, enum led_trigger_tty_bits attr)
>> +{
>> +	struct ledtrig_tty_data *trigger_data = 
>> led_trigger_get_drvdata(dev);
>> +	bool enable;
>> +	int trigger;
>> +	int ret;
>> +
>> +	ret = kstrtobool(buf, &enable);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (attr) {
>> +	case TRIGGER_TTY_RX:
>> +	case TRIGGER_TTY_TX:
>> +	case TRIGGER_TTY_CTS:
>> +	case TRIGGER_TTY_DSR:
>> +	case TRIGGER_TTY_CAR:
>> +	case TRIGGER_TTY_RNG:
>> +		trigger = attr;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (enable)
>> +		set_bit(trigger, &trigger_data->ttytrigger);
>> +	else
>> +		clear_bit(trigger, &trigger_data->ttytrigger);
>> +
>> +	return size;
>> +}
>> +
>> +#define DEFINE_TTY_TRIGGER(trigger_name, trigger) \
>> +	static ssize_t trigger_name##_show(struct device *dev, \
>> +		struct device_attribute *attr, char *buf) \
>> +	{ \
>> +		return ledtrig_tty_attr_show(dev, buf, trigger); \
>> +	} \
>> +	static ssize_t trigger_name##_store(struct device *dev, \
>> +		struct device_attribute *attr, const char *buf, size_t size) \
>> +	{ \
>> +		return ledtrig_tty_attr_store(dev, buf, size, trigger); \
>> +	} \
>> +	static DEVICE_ATTR_RW(trigger_name)
>> +
>> +DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
>> +DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
> 
> Again, I thought we supported rx/tx already?  If so, this should be
> split out into "redo the rx/tx into a new style" and then "add new
> attributes".  Well maybe, more below...

Yes, but the trigger rx/tx was already support before my change.
It could not be configured via the sysfs.

In my setup I do not want to show if data is being transferred.
I want to display the line stats.
Therefore I made it configurable to turn it off.

>> +DEFINE_TTY_TRIGGER(line_cts, TRIGGER_TTY_CTS);
>> +DEFINE_TTY_TRIGGER(line_dsr, TRIGGER_TTY_DSR);
>> +DEFINE_TTY_TRIGGER(line_car, TRIGGER_TTY_CAR);
>> +DEFINE_TTY_TRIGGER(line_rng, TRIGGER_TTY_RNG);
>> +
>>  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;
>> +		}
> 
> Let's ask why you are using bits at all here.  Why?  What is it helping
> with?  Is it faster than a normal "bool" value?  I am guessing not
> (requires mask and test instead of just test).
> 
> So let's just make these individual values in the structure, that makes
> things much simpler and easier and you never have to worry about bit
> field values anywhere.  And the end code will be easier to read as well
> as probably faster (which matters in this codepath, right?)

I had never thought of it that way. I will have a look at that.
Thanks for this.

>> +	}
>> +
>> +	/* 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");
> 
> Not your fault, but this should NOT be "dev_info()" but rather,
> "dev_err()", or "dev_warn()".

This is what I have adopted from the code before my change.
But it will change this in dev_warn.

> 
>> +			mutex_unlock(&trigger_data->mutex);
> 
> This looks ready-made for the completion.h usage so we always make sure
> to drop the mutex when done.

I'll have to take a closer look, I don't know that one.
This is what I have adopted from the code before my change.
Shouldn't that be done in a different patch set?

>> +			return;
>> +		}
>> +
>> +		if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) &&
>> +		    (icount.tx != trigger_data->tx)) {
>> +			trigger_data->tx = icount.tx;
>> +			state = TTY_LED_BLINK;
>> +		}
>> +
>> +		if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
>> +		    (icount.rx != trigger_data->rx)) {
>> +			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;
>> +
>> +	switch (state) {
>> +	case TTY_LED_BLINK:
>>  		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
>>  				      &interval, 0);
>> -
>> -		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;
>>  	}
>> 
>>  out:
>> @@ -141,6 +296,12 @@ static void ledtrig_tty_work(struct work_struct 
>> *work)
>> 
>>  static struct attribute *ledtrig_tty_attrs[] = {
>>  	&dev_attr_ttyname.attr,
>> +	&dev_attr_rx.attr,
>> +	&dev_attr_tx.attr,
> 
> Again, I thought we had rx/tx already?  If not, how was that controlled
> today?

It could not be controlled! The LED flashed when data where transferred.
This was the only function that the trigger supported.

Thanks for your review!

---
Florian

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

[Greg KH]

On Mon, Oct 23, 2023 at 02:15:55PM +0200, Florian Eckert wrote:
> > Again, I thought we had rx/tx already?  If not, how was that controlled
> > today?
> 
> It could not be controlled! The LED flashed when data where transferred.
> This was the only function that the trigger supported.

Ok, then maybe this needs to be a bit longer of a series.  One that does
the "tx/rx" feature, as that is needed today, and will be the more
complex one, and then one-per-line-setting that you want to apply.

That should make it much easier to review overall, right?

thanks,

greg k-h

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

[Florian Eckert]

On 2023-10-23 14:27, Greg KH wrote:
> On Mon, Oct 23, 2023 at 02:15:55PM +0200, Florian Eckert wrote:
>> > Again, I thought we had rx/tx already?  If not, how was that controlled
>> > today?
>> 
>> It could not be controlled! The LED flashed when data where 
>> transferred.
>> This was the only function that the trigger supported.
> 
> Ok, then maybe this needs to be a bit longer of a series.  One that 
> does
> the "tx/rx" feature, as that is needed today, and will be the more
> complex one, and then one-per-line-setting that you want to apply.
> 
> That should make it much easier to review overall, right?

Sorry for asking, but why should I split the change.
What is the added value? But if it is necessary, then I will do it.

Before my change, the trigger could not be configured.
The LED always flashed when data was transferred.
Now I can configure for which tty event the LED should flash or be 
on/off.
So that the trigger behaves the same as before (flash on rx/tx 
transmission),
I set the rx/tx bits in the function ledtrig_tty_activate() with the
following code. Nothing changes for the user of the trigger.

/* Enable default rx/tx LED blink */
set_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger);
set_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger);

---
Florian

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

[Greg KH]

On Mon, Oct 23, 2023 at 02:45:55PM +0200, Florian Eckert wrote:
> 
> 
> On 2023-10-23 14:27, Greg KH wrote:
> > On Mon, Oct 23, 2023 at 02:15:55PM +0200, Florian Eckert wrote:
> > > > Again, I thought we had rx/tx already?  If not, how was that controlled
> > > > today?
> > > 
> > > It could not be controlled! The LED flashed when data where
> > > transferred.
> > > This was the only function that the trigger supported.
> > 
> > Ok, then maybe this needs to be a bit longer of a series.  One that does
> > the "tx/rx" feature, as that is needed today, and will be the more
> > complex one, and then one-per-line-setting that you want to apply.
> > 
> > That should make it much easier to review overall, right?
> 
> Sorry for asking, but why should I split the change.
> What is the added value? But if it is necessary, then I will do it.
> 
> Before my change, the trigger could not be configured.
> The LED always flashed when data was transferred.

But you could configure that, right?  on/off, correct?  And now you are
splitting this out into different "options", which are all different.

> Now I can configure for which tty event the LED should flash or be on/off.

Great.

> So that the trigger behaves the same as before (flash on rx/tx
> transmission),
> I set the rx/tx bits in the function ledtrig_tty_activate() with the
> following code. Nothing changes for the user of the trigger.
> 
> /* Enable default rx/tx LED blink */
> set_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger);
> set_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger);

I agree, but now you are splitting this up into a much finer grained
feature.

Anyway, just a thought, I'll defer to the LED maintainers here as to how
they want to see this, I thought it would actually be easier this way,
maybe not, your call.

thanks,

greg k-h

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

[Florian Eckert]

On 2023-10-23 14:59, Greg KH wrote:
> On Mon, Oct 23, 2023 at 02:45:55PM +0200, Florian Eckert wrote:
>> 
>> 
>> On 2023-10-23 14:27, Greg KH wrote:
>> > On Mon, Oct 23, 2023 at 02:15:55PM +0200, Florian Eckert wrote:
>> > > > Again, I thought we had rx/tx already?  If not, how was that controlled
>> > > > today?
>> > >
>> > > It could not be controlled! The LED flashed when data where
>> > > transferred.
>> > > This was the only function that the trigger supported.
>> >
>> > Ok, then maybe this needs to be a bit longer of a series.  One that does
>> > the "tx/rx" feature, as that is needed today, and will be the more
>> > complex one, and then one-per-line-setting that you want to apply.
>> >
>> > That should make it much easier to review overall, right?
>> 
>> Sorry for asking, but why should I split the change.
>> What is the added value? But if it is necessary, then I will do it.
>> 
>> Before my change, the trigger could not be configured.
>> The LED always flashed when data was transferred.
> 
> But you could configure that, right?  on/off, correct?

No, this could not be configured before. It always flashed as
soon as data went through the configured tty. It couldn't even
be switched off, only if I change the tty interface via sysfs

>> So that the trigger behaves the same as before (flash on rx/tx
>> transmission),
>> I set the rx/tx bits in the function ledtrig_tty_activate() with the
>> following code. Nothing changes for the user of the trigger.
>> 
>> /* Enable default rx/tx LED blink */
>> set_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger);
>> set_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger);
> 
> I agree, but now you are splitting this up into a much finer grained
> feature.
> 
> Anyway, just a thought, I'll defer to the LED maintainers here as to 
> how
> they want to see this, I thought it would actually be easier this way,
> maybe not, your call.

Thank you for your review. I will incorporate your comments and split 
the changes.
:+1:

---
Florian

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

[m.brock]

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

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

[Florian Eckert]

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/

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

[m.brock]

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

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

[Florian Eckert]

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