| Message ID | 20230222083335.847655-3-fe@dev.tdt.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: ledtrig-tty: add tty_led_mode xtension | expand |
On Wed, 22 Feb 2023, Florian Eckert wrote: > Add additional modes to trigger the selected LED. > The following modes are supported: > > Tx/Rx: Flash LED on data transmission (default) > CTS: DCE Ready to accept data from the DTE. > DSR: DCE is ready to receive and send data. > CAR: DCE is receiving a carrier from a remote DTE. > RNG: DCE has detected an incoming ring signal. > > The mode can be changed for example with the following command: > echo "CTS" > /sys/class/leds/<led>/mode > > This would turn on the LED, when the DTE(modem) signals the DCE that it > is ready to accept data. > > Signed-off-by: Florian Eckert <fe@dev.tdt.de> > --- > .../ABI/testing/sysfs-class-led-trigger-tty | 17 ++ > drivers/leds/trigger/ledtrig-tty.c | 145 ++++++++++++++++-- > 2 files changed, 147 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > index 2bf6b24e781b..1c28e6c61d19 100644 > --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > @@ -4,3 +4,20 @@ KernelVersion: 5.10 > Contact: linux-leds@vger.kernel.org > Description: > Specifies the tty device name of the triggering tty > + > +What: /sys/class/leds/<led>/mode > +Date: January 2023 > +KernelVersion: 6.3 > +Description: > + Specifies the operating to trigger the LED. The operating ... ? "mode"? > + The following operating modes are supported: > + > + * Tx/Rx: Flash LED on data transmission (default) > + * CTS: DCE Ready to accept data from the DTE. > + LED on if line is high. > + * DSR: DCE is ready to receive and send data. > + LED on if line is high. > + * CAR: DCE has detected a carrier from a remote DTE. > + LED on if line is high. > + * RNG: DCE has detected an incoming ring signal. > + LED on if line is high. Seeing as this is unchanging, how about you mention it once globally? > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c > index f62db7e520b5..7c4c171c8745 100644 > --- a/drivers/leds/trigger/ledtrig-tty.c > +++ b/drivers/leds/trigger/ledtrig-tty.c > @@ -7,6 +7,15 @@ > #include <linux/tty.h> > #include <uapi/linux/serial.h> > > +enum tty_led_mode { > + TTY_LED_CNT, What's CNT? Is it documented somewhere else? Ah, I see below that this is Tx/Rx. Odd name, what does it mean? Defines and Enums should be self documenting IMHO. > + TTY_LED_CTS, > + TTY_LED_DSR, > + TTY_LED_CAR, > + TTY_LED_RNG, > + __TTY_LED_LAST = TTY_LED_RNG Do you have to prepend with _'s? > +}; > + > struct ledtrig_tty_data { > struct led_classdev *led_cdev; > struct delayed_work dwork; > @@ -14,6 +23,15 @@ struct ledtrig_tty_data { > const char *ttyname; > struct tty_struct *tty; > int rx, tx; > + enum tty_led_mode mode; > +}; > + > +static const char * const mode[] = { > + [TTY_LED_CNT] = "Tx/Rx", // Trasmit Data / Receive Data C++ style comments? > + [TTY_LED_CTS] = "CTS", // CTS Clear To Send > + [TTY_LED_DSR] = "DSR", // DSR Data Set Ready > + [TTY_LED_CAR] = "CAR", // CAR Data Carrier Detect (DCD) > + [TTY_LED_RNG] = "RNG", // RNG Ring Indicator (RI) > }; > > static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data) > @@ -21,6 +39,70 @@ static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data) > schedule_delayed_work(&trigger_data->dwork, 0); > } > > +static ssize_t ledtrig_tty_mode_show(char *buf, enum tty_led_mode tty_mode) > +{ > + int len = 0; > + int i; > + > + for (i = 0; i <= __TTY_LED_LAST; i++) { > + bool hit = tty_mode == i; > + bool last = i == __TTY_LED_LAST; > + > + len += sysfs_emit_at(buf, len, "%s%s%s%s", > + hit ? "[" : "", > + mode[i], > + hit ? "]" : "", > + last ? "" : " "); > + } > + > + len += sysfs_emit_at(buf, len, "\n"); > + > + return len; > +} > + > +static ssize_t tty_led_mode_show(struct device *dev, > + struct device_attribute *attr, char *buf) This may be a personal preference, but I'd rather see alignment with the '('. > +{ > + struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev); > + enum tty_led_mode tty_mode; > + > + mutex_lock(&trigger_data->mutex); > + tty_mode = trigger_data->mode; > + mutex_unlock(&trigger_data->mutex); > + > + return ledtrig_tty_mode_show(buf, tty_mode); > +} > + > +static ssize_t tty_led_mode_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; > + enum tty_led_mode tty_mode = __TTY_LED_LAST; Nit: Can you reverse these 2 lines to make my OCD happy please? > + int i; > + > + /* Check for new line in string*/ ' ' before the '*'. > + if (size > 0 && buf[size - 1] == '\n') > + size -= 1; > + > + for (i = 0; i <= __TTY_LED_LAST; i++) > + if (strncmp(buf, mode[i], size) == 0) { > + tty_mode = i; > + break; > + } > + > + if (i > __TTY_LED_LAST) > + return -EINVAL; > + > + mutex_lock(&trigger_data->mutex); > + trigger_data->mode = tty_mode; > + mutex_unlock(&trigger_data->mutex); > + > + return ret; > +} > +static DEVICE_ATTR_RW(tty_led_mode); > + > static ssize_t ttyname_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -76,6 +158,18 @@ static ssize_t ttyname_store(struct device *dev, > } > static DEVICE_ATTR_RW(ttyname); > > +static void ledtrig_tty_flags(struct ledtrig_tty_data *trigger_data, > + unsigned int flag) This can be on a single line. Please use 100-chars throughout. > +{ > + unsigned int status; > + > + status = tty_get_mget(trigger_data->tty); > + if (status & flag) > + led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > + else > + led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > +} > + > static void ledtrig_tty_work(struct work_struct *work) > { > struct ledtrig_tty_data *trigger_data = > @@ -113,21 +207,38 @@ 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; > - } > - > - if (icount.rx != trigger_data->rx || > - icount.tx != trigger_data->tx) { > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > - > - trigger_data->rx = icount.rx; > - trigger_data->tx = icount.tx; > - } else { > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > + switch (trigger_data->mode) { > + case TTY_LED_CTS: > + ledtrig_tty_flags(trigger_data, TIOCM_CTS); > + break; > + case TTY_LED_DSR: > + ledtrig_tty_flags(trigger_data, TIOCM_DSR); > + break; > + case TTY_LED_CAR: > + ledtrig_tty_flags(trigger_data, TIOCM_CAR); > + break; > + case TTY_LED_RNG: > + ledtrig_tty_flags(trigger_data, TIOCM_RNG); > + break; > + case TTY_LED_CNT: I believe this requires a 'fall-through' statement. Documentation/process/deprecated.rst > + default: > + 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 (icount.rx != trigger_data->rx || > + icount.tx != trigger_data->tx) { One line, etc. > + led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > + > + trigger_data->rx = icount.rx; > + trigger_data->tx = icount.tx; > + } else { > + led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > + } > + break; > } > > out: > @@ -137,6 +248,7 @@ static void ledtrig_tty_work(struct work_struct *work) > > static struct attribute *ledtrig_tty_attrs[] = { > &dev_attr_ttyname.attr, > + &dev_attr_tty_led_mode.attr, > NULL > }; > ATTRIBUTE_GROUPS(ledtrig_tty); > @@ -149,6 +261,9 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev) > if (!trigger_data) > return -ENOMEM; > > + /* set default mode */ Nit: "Set" > + trigger_data->mode = TTY_LED_CNT; > + > led_set_trigger_data(led_cdev, trigger_data); > > INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work); > -- > 2.30.2 >
On 03. 03. 23, 15:11, Lee Jones wrote: > On Wed, 22 Feb 2023, Florian Eckert wrote: >> @@ -113,21 +207,38 @@ 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; >> - } >> - >> - if (icount.rx != trigger_data->rx || >> - icount.tx != trigger_data->tx) { >> - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); >> - >> - trigger_data->rx = icount.rx; >> - trigger_data->tx = icount.tx; >> - } else { >> - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); >> + switch (trigger_data->mode) { >> + case TTY_LED_CTS: >> + ledtrig_tty_flags(trigger_data, TIOCM_CTS); >> + break; >> + case TTY_LED_DSR: >> + ledtrig_tty_flags(trigger_data, TIOCM_DSR); >> + break; >> + case TTY_LED_CAR: >> + ledtrig_tty_flags(trigger_data, TIOCM_CAR); >> + break; >> + case TTY_LED_RNG: >> + ledtrig_tty_flags(trigger_data, TIOCM_RNG); >> + break; >> + case TTY_LED_CNT: > > I believe this requires a 'fall-through' statement. I don't think this is the case. Isn't fallthrough required only in cases when there is at least one statement, i.e. a block? > Documentation/process/deprecated.rst > >> + default: >> + 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; >> + } >> +
On 2023-03-06 07:57, Jiri Slaby wrote: > On 03. 03. 23, 15:11, Lee Jones wrote: >> On Wed, 22 Feb 2023, Florian Eckert wrote: >>> @@ -113,21 +207,38 @@ 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; >>> - } >>> - >>> - if (icount.rx != trigger_data->rx || >>> - icount.tx != trigger_data->tx) { >>> - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); >>> - >>> - trigger_data->rx = icount.rx; >>> - trigger_data->tx = icount.tx; >>> - } else { >>> - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); >>> + switch (trigger_data->mode) { >>> + case TTY_LED_CTS: >>> + ledtrig_tty_flags(trigger_data, TIOCM_CTS); >>> + break; >>> + case TTY_LED_DSR: >>> + ledtrig_tty_flags(trigger_data, TIOCM_DSR); >>> + break; >>> + case TTY_LED_CAR: >>> + ledtrig_tty_flags(trigger_data, TIOCM_CAR); >>> + break; >>> + case TTY_LED_RNG: >>> + ledtrig_tty_flags(trigger_data, TIOCM_RNG); >>> + break; >>> + case TTY_LED_CNT: >> >> I believe this requires a 'fall-through' statement. > > I don't think this is the case. Isn't fallthrough required only in > cases when there is at least one statement, i.e. a block? Jiri thanks for the advice I also understood that I only need the /* Fall through */ comment if I also have at least one statement. Which is not the case there. So I would say that fits. For all other things, I am in the process of fixing that and sending a v8 patchset. > >> Documentation/process/deprecated.rst >> >>> + default: >>> + 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; >>> + } >>> +
On Mon, 06 Mar 2023, Jiri Slaby wrote: > On 03. 03. 23, 15:11, Lee Jones wrote: > > On Wed, 22 Feb 2023, Florian Eckert wrote: > > > @@ -113,21 +207,38 @@ 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; > > > - } > > > - > > > - if (icount.rx != trigger_data->rx || > > > - icount.tx != trigger_data->tx) { > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > > - > > > - trigger_data->rx = icount.rx; > > > - trigger_data->tx = icount.tx; > > > - } else { > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > > + switch (trigger_data->mode) { > > > + case TTY_LED_CTS: > > > + ledtrig_tty_flags(trigger_data, TIOCM_CTS); > > > + break; > > > + case TTY_LED_DSR: > > > + ledtrig_tty_flags(trigger_data, TIOCM_DSR); > > > + break; > > > + case TTY_LED_CAR: > > > + ledtrig_tty_flags(trigger_data, TIOCM_CAR); > > > + break; > > > + case TTY_LED_RNG: > > > + ledtrig_tty_flags(trigger_data, TIOCM_RNG); > > > + break; > > > + case TTY_LED_CNT: > > > > I believe this requires a 'fall-through' statement. > > I don't think this is the case. Isn't fallthrough required only in cases > when there is at least one statement, i.e. a block? There's no mention of this caveat in the document. To my untrained eyes, the rule looks fairly explicit, starting with "All". " All switch/case blocks must end in one of: * break; * fallthrough; * continue; * goto <label>; * return [expression]; " If you're aware of something I'm not, please consider updating the doc. > > Documentation/process/deprecated.rst > > > > > + default: > > > + 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; > > > + } > > > +
On Mon, Mar 06, 2023 at 09:04:56AM +0000, Lee Jones wrote: > On Mon, 06 Mar 2023, Jiri Slaby wrote: > > > On 03. 03. 23, 15:11, Lee Jones wrote: > > > On Wed, 22 Feb 2023, Florian Eckert wrote: > > > > @@ -113,21 +207,38 @@ 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; > > > > - } > > > > - > > > > - if (icount.rx != trigger_data->rx || > > > > - icount.tx != trigger_data->tx) { > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > > > - > > > > - trigger_data->rx = icount.rx; > > > > - trigger_data->tx = icount.tx; > > > > - } else { > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > > > + switch (trigger_data->mode) { > > > > + case TTY_LED_CTS: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CTS); > > > > + break; > > > > + case TTY_LED_DSR: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_DSR); > > > > + break; > > > > + case TTY_LED_CAR: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CAR); > > > > + break; > > > > + case TTY_LED_RNG: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_RNG); > > > > + break; > > > > + case TTY_LED_CNT: > > > > > > I believe this requires a 'fall-through' statement. > > > > I don't think this is the case. Isn't fallthrough required only in cases > > when there is at least one statement, i.e. a block? > > There's no mention of this caveat in the document. > > To my untrained eyes, the rule looks fairly explicit, starting with "All". > > " > All switch/case blocks must end in one of: > > * break; > * fallthrough; > * continue; > * goto <label>; > * return [expression]; > " > > If you're aware of something I'm not, please consider updating the doc. Just to add my 0.02€: I think this case (i.e. case TTY_LED_CNT: default: ... ) doesn't need a fall-through for two reasons: a) The compilers don't warn about this construct even with the fall-through warning enabled; and b) I wouldn't call the TTY_LED_CNT line a "block", so the quoted document[1] doesn't apply. (Though I agree that there is some potential for improvement by mentioning this case. (haha)) Best regards Uwe [1] Documentation/process/deprecated.rst
On 06. 03. 23, 10:04, Lee Jones wrote: > On Mon, 06 Mar 2023, Jiri Slaby wrote: > >> On 03. 03. 23, 15:11, Lee Jones wrote: >>> On Wed, 22 Feb 2023, Florian Eckert wrote: >>>> @@ -113,21 +207,38 @@ 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; >>>> - } >>>> - >>>> - if (icount.rx != trigger_data->rx || >>>> - icount.tx != trigger_data->tx) { >>>> - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); >>>> - >>>> - trigger_data->rx = icount.rx; >>>> - trigger_data->tx = icount.tx; >>>> - } else { >>>> - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); >>>> + switch (trigger_data->mode) { >>>> + case TTY_LED_CTS: >>>> + ledtrig_tty_flags(trigger_data, TIOCM_CTS); >>>> + break; >>>> + case TTY_LED_DSR: >>>> + ledtrig_tty_flags(trigger_data, TIOCM_DSR); >>>> + break; >>>> + case TTY_LED_CAR: >>>> + ledtrig_tty_flags(trigger_data, TIOCM_CAR); >>>> + break; >>>> + case TTY_LED_RNG: >>>> + ledtrig_tty_flags(trigger_data, TIOCM_RNG); >>>> + break; >>>> + case TTY_LED_CNT: >>> >>> I believe this requires a 'fall-through' statement. >> >> I don't think this is the case. Isn't fallthrough required only in cases >> when there is at least one statement, i.e. a block? > > There's no mention of this caveat in the document. > > To my untrained eyes, the rule looks fairly explicit, starting with "All". > > " > All switch/case blocks must end in one of: > > * break; > * fallthrough; > * continue; > * goto <label>; > * return [expression]; > " > > If you're aware of something I'm not, please consider updating the doc. The magic word in the above is "block", IMO. A block is defined for me as a list of declarations and/or statements. Which is not the case in the above (i.e. in sequential "case"s). Furthermore, the gcc docs specifically say about fallthrough attribute: It can only be used in a switch statement (the compiler will issue an error otherwise), after a preceding statement and before a logically succeeding case label, or user-defined label. While "case X:" is technically a (label) statement, I don't think they were thinking of it as such here due to following "succeeding case label" in the text. So checking with the code, gcc indeed skips those (should_warn_for_implicit_fallthrough()): /* Skip all immediately following labels. */ while (!gsi_end_p (gsi) && (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL || gimple_code (gsi_stmt (gsi)) == GIMPLE_PREDICT)) gsi_next_nondebug (&gsi); Apart from that, fallthrough only makes the code harder to read: case X: case Y: case Z: default: do_something(); VS case X: fallthrough; case Y: fallthrough; case Z: fallthrough; default: do_something(); The first one is a clear win, IMO, and it's pretty clear that it falls through on purpose. And even for compiler -- it shall not produce a warning in that case. regards,
On Wed, Feb 22, 2023 at 09:33:35AM +0100, Florian Eckert wrote: > Add additional modes to trigger the selected LED. > The following modes are supported: > > Tx/Rx: Flash LED on data transmission (default) > CTS: DCE Ready to accept data from the DTE. > DSR: DCE is ready to receive and send data. > CAR: DCE is receiving a carrier from a remote DTE. > RNG: DCE has detected an incoming ring signal. > > The mode can be changed for example with the following command: > echo "CTS" > /sys/class/leds/<led>/mode > > This would turn on the LED, when the DTE(modem) signals the DCE that it > is ready to accept data. > > Signed-off-by: Florian Eckert <fe@dev.tdt.de> > --- > .../ABI/testing/sysfs-class-led-trigger-tty | 17 ++ > drivers/leds/trigger/ledtrig-tty.c | 145 ++++++++++++++++-- > 2 files changed, 147 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > index 2bf6b24e781b..1c28e6c61d19 100644 > --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > @@ -4,3 +4,20 @@ KernelVersion: 5.10 > Contact: linux-leds@vger.kernel.org > Description: > Specifies the tty device name of the triggering tty > + > +What: /sys/class/leds/<led>/mode > +Date: January 2023 > +KernelVersion: 6.3 > +Description: > + Specifies the operating to trigger the LED. > + The following operating modes are supported: > + > + * Tx/Rx: Flash LED on data transmission (default) > + * CTS: DCE Ready to accept data from the DTE. > + LED on if line is high. > + * DSR: DCE is ready to receive and send data. > + LED on if line is high. > + * CAR: DCE has detected a carrier from a remote DTE. > + LED on if line is high. > + * RNG: DCE has detected an incoming ring signal. > + LED on if line is high. Something I (still) don't like about this approach is that you cannot make the LED flash on TX only (or CAR and DSR). Something like: led=/sys/class/leds/<led>/ echo 1 > $led/TX echo 0 > $led/RX echo 1 > $led/CAR would be a more flexible and IMHO nicer interface. (Maybe with improved file names.) Best regards Uwe
On Mon, 06 Mar 2023, Jiri Slaby wrote: > On 06. 03. 23, 10:04, Lee Jones wrote: > > On Mon, 06 Mar 2023, Jiri Slaby wrote: > > > > > On 03. 03. 23, 15:11, Lee Jones wrote: > > > > On Wed, 22 Feb 2023, Florian Eckert wrote: > > > > > @@ -113,21 +207,38 @@ 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; > > > > > - } > > > > > - > > > > > - if (icount.rx != trigger_data->rx || > > > > > - icount.tx != trigger_data->tx) { > > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > > > > - > > > > > - trigger_data->rx = icount.rx; > > > > > - trigger_data->tx = icount.tx; > > > > > - } else { > > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > > > > + switch (trigger_data->mode) { > > > > > + case TTY_LED_CTS: > > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CTS); > > > > > + break; > > > > > + case TTY_LED_DSR: > > > > > + ledtrig_tty_flags(trigger_data, TIOCM_DSR); > > > > > + break; > > > > > + case TTY_LED_CAR: > > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CAR); > > > > > + break; > > > > > + case TTY_LED_RNG: > > > > > + ledtrig_tty_flags(trigger_data, TIOCM_RNG); > > > > > + break; > > > > > + case TTY_LED_CNT: > > > > > > > > I believe this requires a 'fall-through' statement. > > > > > > I don't think this is the case. Isn't fallthrough required only in cases > > > when there is at least one statement, i.e. a block? > > > > There's no mention of this caveat in the document. > > > > To my untrained eyes, the rule looks fairly explicit, starting with "All". > > > > " > > All switch/case blocks must end in one of: > > > > * break; > > * fallthrough; > > * continue; > > * goto <label>; > > * return [expression]; > > " > > > > If you're aware of something I'm not, please consider updating the doc. > > The magic word in the above is "block", IMO. A block is defined for me as a > list of declarations and/or statements. Which is not the case in the above > (i.e. in sequential "case"s). > > Furthermore, the gcc docs specifically say about fallthrough attribute: > It can only be used in a switch statement (the compiler will issue an error > otherwise), after a preceding statement and before a logically succeeding > case label, or user-defined label. > > While "case X:" is technically a (label) statement, I don't think they were > thinking of it as such here due to following "succeeding case label" in the > text. > > So checking with the code, gcc indeed skips those > (should_warn_for_implicit_fallthrough()): > /* Skip all immediately following labels. */ > while (!gsi_end_p (gsi) > && (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL > || gimple_code (gsi_stmt (gsi)) == GIMPLE_PREDICT)) > gsi_next_nondebug (&gsi); > > > Apart from that, fallthrough only makes the code harder to read: > > case X: > case Y: > case Z: > default: > do_something(); > > VS > > case X: > fallthrough; > case Y: > fallthrough; > case Z: > fallthrough; > default: > do_something(); > > The first one is a clear win, IMO, and it's pretty clear that it falls > through on purpose. And even for compiler -- it shall not produce a warning > in that case. Works for me. Thanks for the clear explanation, Jiri and Uwe. And yes Uwe, it would be good if we could make that clearer in the doc.
Hello Uwe, >> + LED on if line is high. >> + * RNG: DCE has detected an incoming ring signal. >> + LED on if line is high. > > Something I (still) don't like about this approach is that you cannot > make the LED flash on TX only (or CAR and DSR). Something like: > > led=/sys/class/leds/<led>/ > echo 1 > $led/TX > echo 0 > $led/RX > echo 1 > $led/CAR > > would be a more flexible and IMHO nicer interface. (Maybe with improved > file names.) The question is whether it makes sense to combine several states on one LED. We can add TTY_LED_RX or TTY_LED_TX to meet your requirements. The only led trigger I know that combines multiple states is ledtrig-netdev. If so, I can only imagine that we handle it the same way as with ledtrig-netdev. For the states CTS/DSR/CAR/RNG, the LED goes on or off and when data is transmitted (rx/tx), the LED flashes. I have personally have a usecase where I need to indicate whether I am getting CTS from the mode or not. If that's how we want to do it, then I can only imagine that: led=/sys/class/leds/<led>/ echo 1 > $led/rx echo 0 > $led/tx echo <CTS|DSR|CAR|RNG> > $led/tty_led_mode I think it only makes sense to always display only one mode This are "CTS|DSR|CAR|RNG". Personally, I think it complicates things because the LED shows several states. Best regards Florian
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty index 2bf6b24e781b..1c28e6c61d19 100644 --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty @@ -4,3 +4,20 @@ KernelVersion: 5.10 Contact: linux-leds@vger.kernel.org Description: Specifies the tty device name of the triggering tty + +What: /sys/class/leds/<led>/mode +Date: January 2023 +KernelVersion: 6.3 +Description: + Specifies the operating to trigger the LED. + The following operating modes are supported: + + * Tx/Rx: Flash LED on data transmission (default) + * CTS: DCE Ready to accept data from the DTE. + LED on if line is high. + * DSR: DCE is ready to receive and send data. + LED on if line is high. + * CAR: DCE has detected a carrier from a remote DTE. + LED on if line is high. + * RNG: DCE has detected an incoming ring signal. + LED on if line is high. diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c index f62db7e520b5..7c4c171c8745 100644 --- a/drivers/leds/trigger/ledtrig-tty.c +++ b/drivers/leds/trigger/ledtrig-tty.c @@ -7,6 +7,15 @@ #include <linux/tty.h> #include <uapi/linux/serial.h> +enum tty_led_mode { + TTY_LED_CNT, + TTY_LED_CTS, + TTY_LED_DSR, + TTY_LED_CAR, + TTY_LED_RNG, + __TTY_LED_LAST = TTY_LED_RNG +}; + struct ledtrig_tty_data { struct led_classdev *led_cdev; struct delayed_work dwork; @@ -14,6 +23,15 @@ struct ledtrig_tty_data { const char *ttyname; struct tty_struct *tty; int rx, tx; + enum tty_led_mode mode; +}; + +static const char * const mode[] = { + [TTY_LED_CNT] = "Tx/Rx", // Trasmit Data / Receive Data + [TTY_LED_CTS] = "CTS", // CTS Clear To Send + [TTY_LED_DSR] = "DSR", // DSR Data Set Ready + [TTY_LED_CAR] = "CAR", // CAR Data Carrier Detect (DCD) + [TTY_LED_RNG] = "RNG", // RNG Ring Indicator (RI) }; static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data) @@ -21,6 +39,70 @@ static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data) schedule_delayed_work(&trigger_data->dwork, 0); } +static ssize_t ledtrig_tty_mode_show(char *buf, enum tty_led_mode tty_mode) +{ + int len = 0; + int i; + + for (i = 0; i <= __TTY_LED_LAST; i++) { + bool hit = tty_mode == i; + bool last = i == __TTY_LED_LAST; + + len += sysfs_emit_at(buf, len, "%s%s%s%s", + hit ? "[" : "", + mode[i], + hit ? "]" : "", + last ? "" : " "); + } + + len += sysfs_emit_at(buf, len, "\n"); + + return len; +} + +static ssize_t tty_led_mode_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev); + enum tty_led_mode tty_mode; + + mutex_lock(&trigger_data->mutex); + tty_mode = trigger_data->mode; + mutex_unlock(&trigger_data->mutex); + + return ledtrig_tty_mode_show(buf, tty_mode); +} + +static ssize_t tty_led_mode_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; + enum tty_led_mode tty_mode = __TTY_LED_LAST; + int i; + + /* Check for new line in string*/ + if (size > 0 && buf[size - 1] == '\n') + size -= 1; + + for (i = 0; i <= __TTY_LED_LAST; i++) + if (strncmp(buf, mode[i], size) == 0) { + tty_mode = i; + break; + } + + if (i > __TTY_LED_LAST) + return -EINVAL; + + mutex_lock(&trigger_data->mutex); + trigger_data->mode = tty_mode; + mutex_unlock(&trigger_data->mutex); + + return ret; +} +static DEVICE_ATTR_RW(tty_led_mode); + static ssize_t ttyname_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -76,6 +158,18 @@ static ssize_t ttyname_store(struct device *dev, } static DEVICE_ATTR_RW(ttyname); +static void ledtrig_tty_flags(struct ledtrig_tty_data *trigger_data, + unsigned int flag) +{ + unsigned int status; + + status = tty_get_mget(trigger_data->tty); + if (status & flag) + led_set_brightness_sync(trigger_data->led_cdev, LED_ON); + else + led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); +} + static void ledtrig_tty_work(struct work_struct *work) { struct ledtrig_tty_data *trigger_data = @@ -113,21 +207,38 @@ 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; - } - - if (icount.rx != trigger_data->rx || - icount.tx != trigger_data->tx) { - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); - - trigger_data->rx = icount.rx; - trigger_data->tx = icount.tx; - } else { - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); + switch (trigger_data->mode) { + case TTY_LED_CTS: + ledtrig_tty_flags(trigger_data, TIOCM_CTS); + break; + case TTY_LED_DSR: + ledtrig_tty_flags(trigger_data, TIOCM_DSR); + break; + case TTY_LED_CAR: + ledtrig_tty_flags(trigger_data, TIOCM_CAR); + break; + case TTY_LED_RNG: + ledtrig_tty_flags(trigger_data, TIOCM_RNG); + break; + case TTY_LED_CNT: + default: + 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 (icount.rx != trigger_data->rx || + icount.tx != trigger_data->tx) { + led_set_brightness_sync(trigger_data->led_cdev, LED_ON); + + trigger_data->rx = icount.rx; + trigger_data->tx = icount.tx; + } else { + led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); + } + break; } out: @@ -137,6 +248,7 @@ static void ledtrig_tty_work(struct work_struct *work) static struct attribute *ledtrig_tty_attrs[] = { &dev_attr_ttyname.attr, + &dev_attr_tty_led_mode.attr, NULL }; ATTRIBUTE_GROUPS(ledtrig_tty); @@ -149,6 +261,9 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev) if (!trigger_data) return -ENOMEM; + /* set default mode */ + trigger_data->mode = TTY_LED_CNT; + led_set_trigger_data(led_cdev, trigger_data); INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
Add additional modes to trigger the selected LED. The following modes are supported: Tx/Rx: Flash LED on data transmission (default) CTS: DCE Ready to accept data from the DTE. DSR: DCE is ready to receive and send data. CAR: DCE is receiving a carrier from a remote DTE. RNG: DCE has detected an incoming ring signal. The mode can be changed for example with the following command: echo "CTS" > /sys/class/leds/<led>/mode This would turn on the LED, when the DTE(modem) signals the DCE that it is ready to accept data. Signed-off-by: Florian Eckert <fe@dev.tdt.de> --- .../ABI/testing/sysfs-class-led-trigger-tty | 17 ++ drivers/leds/trigger/ledtrig-tty.c | 145 ++++++++++++++++-- 2 files changed, 147 insertions(+), 15 deletions(-)