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(-)
--
2.30.2
Has complained about the following construct:
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 <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Florian Eckert <[email protected]>
---
drivers/leds/trigger/ledtrig-tty.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 8ae0d2d284af..1c6fadf0b856 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -82,6 +82,7 @@ static void ledtrig_tty_work(struct work_struct *work)
{
struct ledtrig_tty_data *trigger_data =
container_of(work, struct ledtrig_tty_data, dwork.work);
+ unsigned long interval = LEDTRIG_TTY_INTERVAL;
struct serial_icounter_struct icount;
int ret;
@@ -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;
-
led_blink_set_oneshot(trigger_data->led_cdev, &interval,
&interval, 0);
--
2.30.2
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 <[email protected]>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Signed-off-by: Florian Eckert <[email protected]>
> ---
> @@ -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 2023-10-16 10:46, [email protected] 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.
>> 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 <[email protected]>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/[email protected]/
>> Signed-off-by: Florian Eckert <[email protected]>
>> ---
>> @@ -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/[email protected]/
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.
[1]
https://lore.kernel.org/linux-leds/[email protected]/
[2]
https://lore.kernel.org/linux-leds/[email protected]/
Florian
On 16. 10. 23, 11:12, Florian Eckert wrote:
>
>
> On 2023-10-16 10:46, [email protected] 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 <[email protected]>
>>> Closes:
>>> https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>> Signed-off-by: Florian Eckert <[email protected]>
>>> ---
>>> @@ -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,
--
js
suse labs
Florian Eckert wrote on 2023-10-16 11:12:
> On 2023-10-16 10:46, [email protected] 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 <[email protected]>
>>> Closes:
>>> https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>> Signed-off-by: Florian Eckert <[email protected]>
>>> ---
>>> @@ -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/[email protected]/
> 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/[email protected]/
> [2]
> https://lore.kernel.org/linux-leds/[email protected]/
>
>
> Florian
Maarten