| Message ID | 20231016071332.597654-1-fe@dev.tdt.de |
|---|---|
| Headers | show |
| Series | ledtrig-tty: add additional tty state evaluation | expand |
Florian Eckert wrote on 2023-10-16 09:13: > Has complained about the following construct: Who is "Has" or who/what has complained? > drivers/leds/trigger/ledtrig-tty.c:362:3: error: a label can only be > part of a statement and a declaration is not a statement > > Hence move the variable definition to the beginning of the function. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: > https://lore.kernel.org/oe-kbuild-all/202309270440.IJB24Xap-lkp@intel.com/ > Signed-off-by: Florian Eckert <fe@dev.tdt.de> > --- > @@ -124,8 +125,6 @@ static void ledtrig_tty_work(struct work_struct > *work) > > if (icount.rx != trigger_data->rx || > icount.tx != trigger_data->tx) { > - unsigned long interval = LEDTRIG_TTY_INTERVAL; > - Is this kernel test robot broken? I see no label definition here. And this variable declaration is at the start of a new block which does not even require C99 support. > led_blink_set_oneshot(trigger_data->led_cdev, &interval, > &interval, 0); Maarten
On 16. 10. 23, 11:12, Florian Eckert wrote: > > > On 2023-10-16 10:46, m.brock@vanmierlo.com wrote: >> Florian Eckert wrote on 2023-10-16 09:13: >>> Has complained about the following construct: >> >> Who is "Has" or who/what has complained? > > The test robot who does not agree with my change in the v1 patchset. Well, you should have put subject to that sentence, so that we can understand. And not to parse "Has" as a tool/person name ;). >>> drivers/leds/trigger/ledtrig-tty.c:362:3: error: a label can only be >>> part of a statement and a declaration is not a statement >>> >>> Hence move the variable definition to the beginning of the function. >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: >>> https://lore.kernel.org/oe-kbuild-all/202309270440.IJB24Xap-lkp@intel.com/ >>> Signed-off-by: Florian Eckert <fe@dev.tdt.de> >>> --- >>> @@ -124,8 +125,6 @@ static void ledtrig_tty_work(struct work_struct >>> *work) >>> >>> if (icount.rx != trigger_data->rx || >>> icount.tx != trigger_data->tx) { >>> - unsigned long interval = LEDTRIG_TTY_INTERVAL; >>> - >> >> Is this kernel test robot broken? > > The test robot does nothing wrong. > >> I see no label definition here. case is a label. >> And this variable declaration is at the start of a new block which >> does not >> even require C99 support. Nah. The block begins after the switch. So """ switch (X) { type var; case X: } would work. Moving the def after case is no longer at the block beginning. So just wrap put the case code in a block like we are used to: """ case X: { type var; } """. regards,
Florian Eckert wrote on 2023-10-16 11:12: > On 2023-10-16 10:46, m.brock@vanmierlo.com wrote: >> Florian Eckert wrote on 2023-10-16 09:13: >>> Has complained about the following construct: >> >> Who is "Has" or who/what has complained? > > The test robot who does not agree with my change in the v1 patchset. You don't have to explain to me, just fix the comment. >>> drivers/leds/trigger/ledtrig-tty.c:362:3: error: a label can only be >>> part of a statement and a declaration is not a statement >>> >>> Hence move the variable definition to the beginning of the function. >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: >>> https://lore.kernel.org/oe-kbuild-all/202309270440.IJB24Xap-lkp@intel.com/ >>> Signed-off-by: Florian Eckert <fe@dev.tdt.de> >>> --- >>> @@ -124,8 +125,6 @@ static void ledtrig_tty_work(struct work_struct >>> *work) >>> >>> if (icount.rx != trigger_data->rx || >>> icount.tx != trigger_data->tx) { >>> - unsigned long interval = LEDTRIG_TTY_INTERVAL; >>> - >> >> Is this kernel test robot broken? > > The test robot does nothing wrong. > >> I see no label definition here. >> And this variable declaration is at the start of a new block which >> does >> not even require C99 support. > > I made change in patch set v1, that moves the definition of the > variable > `interval` into the switch case statement. > https://lore.kernel.org/linux-leds/20230926093607.59536-3-fe@dev.tdt.de/ > The robot complained about this. > > So I decided to move the definition of the variable 'interval' to > function > head to make the test robot happy in the commit. So this commit > prepares > the code for my change. > > If it is more common, I can merge this patch [1] into the next patch > [2] > of this set. Yes, please. You're fixing a problem that does not exist yet (and never will), because the patch that introduces it is not yet applied. So fix the proposed patch instead of patching the patch. > [1] > https://lore.kernel.org/linux-leds/20231016071332.597654-4-fe@dev.tdt.de/ > [2] > https://lore.kernel.org/linux-leds/20231016071332.597654-5-fe@dev.tdt.de/ > > > Florian Maarten
Changes in v3: - Add missing 'kernel test robot' information to the commit message. - Additional information added to the commit message Changes in v2: - 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. v1: This is a follow-up patchset, based on the mailing list discussion from March 2023 based on the old patchset v7 [1]. I have changed, the LED trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König. Florian Eckert (4): tty: whitespaces in descriptions corrected by replacing tabs with spaces. tty: add new helper function tty_get_tiocm trigger: ledtrig-tty: move variable definition to the top trigger: ledtrig-tty: add new line mode to triggers .../ABI/testing/sysfs-class-led-trigger-tty | 53 ++++ drivers/leds/trigger/ledtrig-tty.c | 280 +++++++++++++++++- drivers/tty/tty_io.c | 130 ++++---- include/linux/tty.h | 1 + 4 files changed, 396 insertions(+), 68 deletions(-)