| Message ID | 20231023094205.2706812-1-fe@dev.tdt.de |
|---|---|
| Headers | show |
| Series | ledtrig-tty: add additional tty state evaluation | expand |
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
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/
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
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
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(-)