Re: [PATCH 09/10] leds: Support custom sysfs groups on multicolor and flash LEDs

[Rong Zhang]

Hi Armin,

Nice refactor!

On Sun, 2026-10-18 at 15:46 +0200, Armin Wolf wrote:
> Drivers registering multicolor/flash LEDs currently cannot use
> led_cdev->groups due to said field being overwritten by the
> multicolor/flash LED registration functions. Fix this by assigning
> each LED type a seperate device type ("multicolor" and "flash")
> that carries the sysfs attributes instead.
> 
> Also assign a device type for standard LEDs ("standard") for symmetry.
> 
> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> ---
>  drivers/leds/led-class-flash.c      | 108 ++++++++++++++--------------
>  drivers/leds/led-class-multicolor.c |  18 ++++-
>  drivers/leds/led-class.c            |   5 ++
>  include/linux/led-class-flash.h     |   5 --
>  4 files changed, 73 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 953265fddb7d..e023511cea1c 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> +#include "leds.h"
>  
>  #define has_flash_op(fled_cdev, op)				\
>  	(fled_cdev && fled_cdev->ops->op)
> @@ -207,42 +208,56 @@ static ssize_t flash_fault_show(struct device *dev, const struct device_attribut
>  }
>  static const DEVICE_ATTR_RO(flash_fault);
>  
> -static const struct attribute *const led_flash_strobe_attrs[] = {
> +static const struct attribute *const led_flash_attrs[] = {
>  	&dev_attr_flash_strobe.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_timeout_attrs[] = {
>  	&dev_attr_flash_timeout.attr,
>  	&dev_attr_max_flash_timeout.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_brightness_attrs[] = {
>  	&dev_attr_flash_brightness.attr,
>  	&dev_attr_max_flash_brightness.attr,
> -	NULL,
> -};
> -
> -static const struct attribute *const led_flash_fault_attrs[] = {
>  	&dev_attr_flash_fault.attr,
>  	NULL,
>  };
>  
> -static const struct attribute_group led_flash_strobe_group = {
> -	.attrs_const = led_flash_strobe_attrs,
> -};
> +static umode_t led_flash_is_visible(struct kobject *kobj, const struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> +
> +	if (attr == &dev_attr_flash_strobe.attr)
> +		return attr->mode;
> +
> +        if (attr == &dev_attr_flash_timeout.attr && fled_cdev->ops->timeout_set)
> +                return attr->mode;
>  
> -static const struct attribute_group led_flash_timeout_group = {
> -	.attrs_const = led_flash_timeout_attrs,
> +	if (attr == &dev_attr_max_flash_timeout.attr && fled_cdev->ops->timeout_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_max_flash_brightness.attr && fled_cdev->ops->flash_brightness_set)
> +		return attr->mode;
> +
> +	if (attr == &dev_attr_flash_fault.attr && fled_cdev->ops->fault_get)
> +		return attr->mode;
> +
> +        return 0;
> +}
> +
> +static const struct attribute_group led_flash_group = {
> +	.is_visible_const = led_flash_is_visible,
> +	.attrs_const = led_flash_attrs,
>  };
>  
> -static const struct attribute_group led_flash_brightness_group = {
> -	.attrs_const = led_flash_brightness_attrs,
> +static const struct attribute_group *led_flash_groups[] = {
> +	&led_flash_group,
> +	NULL,
>  };
>  
> -static const struct attribute_group led_flash_fault_group = {
> -	.attrs_const = led_flash_fault_attrs,

Hmm, a bunch of back-and-forth changes. I suppose the is_visible_const
changes can be merged into PATCH 3, right?

Thanks,
Rong

> +static const struct device_type led_flash_device_type = {
> +	.name = "flash",
> +	.groups = led_flash_groups,
>  };
>  
>  static void led_flash_resume(struct led_classdev *led_cdev)
> @@ -254,28 +269,6 @@ static void led_flash_resume(struct led_classdev *led_cdev)
>  	call_flash_op(fled_cdev, timeout_set, fled_cdev->timeout.val);
>  }
>  
> -static void led_flash_init_sysfs_groups(struct led_classdev_flash *fled_cdev)
> -{
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	const struct led_flash_ops *ops = fled_cdev->ops;
> -	const struct attribute_group **flash_groups = fled_cdev->sysfs_groups;
> -
> -	int num_sysfs_groups = 0;
> -
> -	flash_groups[num_sysfs_groups++] = &led_flash_strobe_group;
> -
> -	if (ops->flash_brightness_set)
> -		flash_groups[num_sysfs_groups++] = &led_flash_brightness_group;
> -
> -	if (ops->timeout_set)
> -		flash_groups[num_sysfs_groups++] = &led_flash_timeout_group;
> -
> -	if (ops->fault_get)
> -		flash_groups[num_sysfs_groups++] = &led_flash_fault_group;
> -
> -	led_cdev->groups = flash_groups;
> -}
> -
>  int led_classdev_flash_register_ext(struct device *parent,
>  				    struct led_classdev_flash *fled_cdev,
>  				    struct led_init_data *init_data)
> @@ -289,26 +282,29 @@ int led_classdev_flash_register_ext(struct device *parent,
>  
>  	led_cdev = &fled_cdev->led_cdev;
>  
> -	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
> -		if (!led_cdev->brightness_set_blocking)
> -			return -EINVAL;
> +	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
> +		return led_classdev_register_ext(parent, led_cdev, init_data);
>  
> -		ops = fled_cdev->ops;
> -		if (!ops || !ops->strobe_set)
> -			return -EINVAL;
> +	if (!led_cdev->brightness_set_blocking)
> +		return -EINVAL;
>  
> -		led_cdev->flash_resume = led_flash_resume;
> +	ops = fled_cdev->ops;
> +	if (!ops || !ops->strobe_set)
> +		return -EINVAL;
>  
> -		/* Select the sysfs attributes to be created for the device */
> -		led_flash_init_sysfs_groups(fled_cdev);
> -	}
> +	led_cdev->flash_resume = led_flash_resume;
>  
>  	/* Register led class device */
> -	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	ret = led_classdev_init(parent, led_cdev, init_data);
>  	if (ret < 0)
>  		return ret;
>  
> -	return 0;
> +	led_cdev->dev->type = &led_flash_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext);
>  
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index 57df0b2e785b..9b8c37f7bdf4 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/uaccess.h>
> +#include "leds.h"
>  
>  static unsigned int led_mc_get_max_intensity(struct led_classdev_mc *mcled_cdev, size_t index)
>  {
> @@ -159,11 +160,17 @@ static const struct attribute *const led_multicolor_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(led_multicolor);
>  
> +static const struct device_type led_multicolor_device_type = {
> +	.name = "multicolor",
> +	.groups = led_multicolor_groups,
> +};
> +
>  int led_classdev_multicolor_register_ext(struct device *parent,
>  				     struct led_classdev_mc *mcled_cdev,
>  				     struct led_init_data *init_data)
>  {
>  	struct led_classdev *led_cdev;
> +	int ret;
>  
>  	if (!mcled_cdev)
>  		return -EINVAL;
> @@ -176,9 +183,16 @@ int led_classdev_multicolor_register_ext(struct device *parent,
>  
>  	led_cdev = &mcled_cdev->led_cdev;
>  	led_cdev->flags |= LED_MULTI_COLOR;
> -	mcled_cdev->led_cdev.groups = led_multicolor_groups;
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_cdev->dev->type = &led_multicolor_device_type;
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
>  
> -	return led_classdev_register_ext(parent, led_cdev, init_data);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
>  
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index ace417a2388e..49b321b3d863 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -613,6 +613,10 @@ int led_classdev_add(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_add);
>  
> +static const struct device_type led_device_type = {
> +	.name = "standard",
> +};
> +
>  /**
>   * led_classdev_register_ext - register a new object of led_classdev class
>   *                             with init data.
> @@ -631,6 +635,7 @@ int led_classdev_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	led_cdev->dev->type = &led_device_type;
>  	ret = led_classdev_add(led_cdev);
>  	if (ret < 0)
>  		put_device(led_cdev->dev);
> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
> index 775a96217518..5204ef60dc4b 100644
> --- a/include/linux/led-class-flash.h
> +++ b/include/linux/led-class-flash.h
> @@ -28,8 +28,6 @@ struct led_classdev_flash;
>  #define LED_FAULT_LED_OVER_TEMPERATURE	(1 << 8)
>  #define LED_NUM_FLASH_FAULTS		9
>  
> -#define LED_FLASH_SYSFS_GROUPS_SIZE	5
> -
>  struct led_flash_ops {
>  	/* set flash brightness */
>  	int (*flash_brightness_set)(struct led_classdev_flash *fled_cdev,
> @@ -79,9 +77,6 @@ struct led_classdev_flash {
>  
>  	/* flash timeout value in microseconds along with its constraints */
>  	struct led_flash_setting duration;
> -
> -	/* LED Flash class sysfs groups */
> -	const struct attribute_group *sysfs_groups[LED_FLASH_SYSFS_GROUPS_SIZE];
>  };
>  
>  static inline struct led_classdev_flash *lcdev_to_flcdev(