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

[Jacek Anaszewski]

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."

Otherwise the driver looks good to me.

[0] https://lkml.org/lkml/2017/11/2/715

-- 
Best regards,
Jacek Anaszewski