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

[Armin Wolf <W_Armin@gmx.de>]

Am 19.10.26 um 13:53 schrieb 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@gmx.de>
>> ---
>>   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

If the LED subsystem maintainers are OK with this then i can gladly introduce
the is_visible_const callback already in patch 3.

Thanks,
Armin Wolf

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