Re: [PATCH v4] leds: trigger: Introduce a NETDEV trigger

[Jacek Anaszewski]

On 12/10/2017 08:12 PM, Ben Whitten wrote:
> Hi Jacek,
> 
> On 10 December 2017 at 18:31, Jacek Anaszewski
> <jacek.anaszewski@xxxxxxxxx> wrote:
>> Hi Ben,
>>
>> Thanks for the update. I have one doubt about comment style
>> at the top of the file. Please refer below.
>>
>> On 12/10/2017 05:24 PM, Ben Whitten wrote:
>>> This commit introduces a NETDEV trigger for named device
>>> activity. Available triggers are link, rx, and tx.
>>>
>>> Signed-off-by: Ben Whitten <ben.whitten@xxxxxxxxx>
>>>
>>> ---
>>> Changes in v4:
>>> Adopt SPDX licence header
>>> Changes in v3:
>>> Cancel the software blink prior to a oneshot re-queue
>>> Changes in v2:
>>> Sort includes and redate documentation
>>> Correct licence
>>> Remove macro and replace with generic function using enums
>>> Convert blink logic in stats work to use led_blink_oneshot
>>> Uses configured brightness instead of FULL
>>> ---
>>>  .../ABI/testing/sysfs-class-led-trigger-netdev     |  45 ++
>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>  drivers/leds/trigger/ledtrig-netdev.c              | 498 +++++++++++++++++++++
>>>  4 files changed, 551 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-netdev
>>>  create mode 100644 drivers/leds/trigger/ledtrig-netdev.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
>>> new file mode 100644
>>> index 0000000..451af6d
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
>>> @@ -0,0 +1,45 @@
>>> +What:                /sys/class/leds/<led>/device_name
>>> +Date:                Dec 2017
>>> +KernelVersion:       4.16
>>> +Contact:     linux-leds@xxxxxxxxxxxxxxx
>>> +Description:
>>> +             Specifies the network device name to monitor.
>>> +
>>> +What:                /sys/class/leds/<led>/interval
>>> +Date:                Dec 2017
>>> +KernelVersion:       4.16
>>> +Contact:     linux-leds@xxxxxxxxxxxxxxx
>>> +Description:
>>> +             Specifies the duration of the LED blink in milliseconds.
>>> +             Defaults to 50 ms.
>>> +
>>> +What:                /sys/class/leds/<led>/link
>>> +Date:                Dec 2017
>>> +KernelVersion:       4.16
>>> +Contact:     linux-leds@xxxxxxxxxxxxxxx
>>> +Description:
>>> +             Signal the link state of the named network device.
>>> +             If set to 0 (default), the LED's normal state is off.
>>> +             If set to 1, the LED's normal state reflects the link state
>>> +             of the named network device.
>>> +             Setting this value also immediately changes the LED state.
>>> +
>>> +What:                /sys/class/leds/<led>/tx
>>> +Date:                Dec 2017
>>> +KernelVersion:       4.16
>>> +Contact:     linux-leds@xxxxxxxxxxxxxxx
>>> +Description:
>>> +             Signal transmission of data on the named network device.
>>> +             If set to 0 (default), the LED will not blink on transmission.
>>> +             If set to 1, the LED will blink for the milliseconds specified
>>> +             in interval to signal transmission.
>>> +
>>> +What:                /sys/class/leds/<led>/rx
>>> +Date:                Dec 2017
>>> +KernelVersion:       4.16
>>> +Contact:     linux-leds@xxxxxxxxxxxxxxx
>>> +Description:
>>> +             Signal reception of data on the named network device.
>>> +             If set to 0 (default), the LED will not blink on reception.
>>> +             If set to 1, the LED will blink for the milliseconds specified
>>> +             in interval to signal reception.
>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>> index 3f9ddb9..4ec1853 100644
>>> --- a/drivers/leds/trigger/Kconfig
>>> +++ b/drivers/leds/trigger/Kconfig
>>> @@ -126,4 +126,11 @@ config LEDS_TRIGGER_PANIC
>>>         a different trigger.
>>>         If unsure, say Y.
>>>
>>> +config LEDS_TRIGGER_NETDEV
>>> +     tristate "LED Netdev Trigger"
>>> +     depends on NET && LEDS_TRIGGERS
>>> +     help
>>> +       This allows LEDs to be controlled by network device activity.
>>> +       If unsure, say Y.
>>> +
>>>  endif # LEDS_TRIGGERS
>>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>>> index 9f2e868..59e163d 100644
>>> --- a/drivers/leds/trigger/Makefile
>>> +++ b/drivers/leds/trigger/Makefile
>>> @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)       += ledtrig-default-on.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_CAMERA)    += ledtrig-camera.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_PANIC)     += ledtrig-panic.o
>>> +obj-$(CONFIG_LEDS_TRIGGER_NETDEV)    += ledtrig-netdev.o
>>> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
>>> new file mode 100644
>>> index 0000000..3d24573
>>> --- /dev/null
>>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>>> @@ -0,0 +1,498 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright 2017 Ben Whitten <ben.whitten@xxxxxxxxx>
>>> +// Copyright 2007 Oliver Jowett <oliver@xxxxxxxxxxxxx>
>>> +/*
>>> + * LED Kernel Netdev Trigger
>>> + *
>>> + * Toggles the LED to reflect the link and traffic state of a named net device
>>> + *
>>> + * Derived from ledtrig-timer.c which is:
>>> + *  Copyright 2005-2006 Openedhand Ltd.
>>> + *  Author: Richard Purdie <rpurdie@xxxxxxxxxxxxxx>
>>> + *
>>> + */
>>
>> This mixed comment style looks odd to me. I'd go for // style
>> on the whole span of this block of commented text.
>> Especially taking into account following Linus' statement
>> from [0]:
>>
>> "And yes, feel free to replace block comments with // while at it."
>>
> 
> Sure thing, should I apply the same to other block comments and single line
> /* */'s whilst at it, to keep the overall style? Or just this header.

Since it is not clear if this should apply also to the
struct documentation, let's confine ourselves to the header for now.

-- 
Best regards,
Jacek Anaszewski