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