Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Randy Dunlap]

Hi,

On 9/9/20 9:25 AM, Marek Behún wrote:
> Many an ethernet PHY (and other chips) supports various HW control modes
> for LEDs connected directly to them.
> 
> This patch adds a generic API for registering such LEDs when described
> in device tree. This API also exposes generic way to select between
> these hardware control modes.
> 
> This API registers a new private LED trigger called dev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> are supported by the device for given LED can be get/set via hw_mode
> sysfs file.
> 
> Signed-off-by: Marek Behún <marek.behun@xxxxxx>
> ---
>  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
>  drivers/leds/Kconfig                          |  10 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-hw-controlled.c             | 227 ++++++++++++++++++
>  include/linux/leds-hw-controlled.h            |  74 ++++++
>  5 files changed, 320 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
>  create mode 100644 drivers/leds/leds-hw-controlled.c
>  create mode 100644 include/linux/leds-hw-controlled.h
> 

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df24eae4..5e47ab21aafb4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>  
>  	  See Documentation/ABI/testing/sysfs-class-led for details.
>  
> +config LEDS_HW_CONTROLLED
> +	bool "API for LEDs that can be controlled by hardware"
> +	depends on LEDS_CLASS

	depends on OF || COMPILE_TEST
?

> +	select LEDS_TRIGGERS
> +	help
> +	  This option enables support for a generic API via which other drivers
> +	  can register LEDs that can be put into hardware controlled mode, eg.

	                                                                   e.g.

> +	  a LED connected to an ethernet PHY can be configured to blink on

	  an LED

> +	  network activity.
> +
>  comment "LED drivers"
>  
>  config LEDS_88PM860X


> diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h
> new file mode 100644
> index 0000000000000..2c9b8a06def18
> --- /dev/null
> +++ b/include/linux/leds-hw-controlled.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> + *
> + * Copyright (C) 2020 Marek Behun <marek.behun@xxxxxx>
> + */
> +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
> +#define _LINUX_LEDS_HW_CONTROLLED_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +
> +struct hw_controlled_led {
> +	struct led_classdev cdev;
> +	const struct hw_controlled_led_ops *ops;
> +	struct mutex lock;
> +
> +	/* these members are filled in by OF if OF is enabled */
> +	int addr;
> +	bool active_low;
> +	bool tristate;
> +
> +	/* also filled in by OF, but changed by led_set_hw_mode operation */
> +	const char *hw_mode;
> +
> +	void *priv;
> +};
> +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev)
> +
> +/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW
> + *
> + * All the following operations must be implemented:
> + * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct).
> + *            This should also change led->cdev.max_brightness, if the value differs from default,
> + *            which is 1.
> + * @led_brightness_set: Sets brightness.
> + * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
> + * @led_set_hw_mode: Sets HW control mode to value specified by given name.
> + * @led_get_hw_mode: Returns current HW control mode name.
> + */

Convert that struct info to kernel-doc?

> +struct hw_controlled_led_ops {
> +	int (*led_init)(struct device *dev, struct hw_controlled_led *led);
> +	int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led,
> +				  enum led_brightness brightness);
> +	const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> +					void **iter);
> +	int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> +			       const char *mode);
> +	const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led);
> +};
> +

> +
> +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */

thanks.
-- 
~Randy