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

["Marek Behún" <marek.behun@nic.cz>]

On Wed, 9 Sep 2020 11:20:00 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> 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@nic.cz>
> > ---
> >  .../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
> ?
> 

I specifically did not add OF dependency so that this can be also used
on non-OF systems. A device driver may register such LED itself...
That's why hw_controlled_led_brightness_set symbol is exported.

Do you think I shouldn't do it?

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

This will need to be changed on multiple places, thanks.

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

This as well

> > +	  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@nic.cz>
> > + */
> > +#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?
> 

Will look into this.
Thanks.

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