Re: [PATCH 08/10] leds: class: Split led_classdev_register_ext()

[Armin Wolf <W_Armin@gmx.de>]

Am 18.10.26 um 15:46 schrieb Armin Wolf:
> Split led_classdev_register_ext() into two seperate internal functions
> to prepare for future changes around LED class device initialization.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/leds/led-class.c | 105 +++++++++++++++++++++++++++++++--------
>   drivers/leds/leds.h      |   3 ++
>   2 files changed, 88 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 77a0fce03952..ace417a2388e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -228,9 +228,15 @@ static struct led_classdev *led_module_get(struct device *led_dev)
>   	return led_cdev;
>   }
>   
> +static void led_classdev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
>   static const struct class leds_class = {
>   	.name = "leds",
>   	.dev_groups = led_groups,
> +	.dev_release = led_classdev_release,
>   	.pm = &leds_class_dev_pm_ops,
>   };
>   
> @@ -465,16 +471,21 @@ static int led_classdev_next_name(const char *init_name, char *name,
>   }
>   
>   /**
> - * led_classdev_register_ext - register a new object of led_classdev class
> - *			       with init data.
> - *
> - * @parent: parent of LED device
> + * led_classdev_init - Initialize a new LED class device.
> + * @parent: parent of LED device.
>    * @led_cdev: the led_classdev structure for this device.
> - * @init_data: LED class device initialization data
> + * @init_data: LED class device initialization data.
> + *
> + * Initializes a new LED class device, but does not register with the
> + * subsystem yet. Useful for performing additional initialization before
> + * adding the LED class device to the subsystem using led_classdev_add().
> + * After calling this function put_device() needs to be called on the dev
> + * field inside the LED class device in order to dispose it.
> + *
> + * Return: Zero on success or a negative error code on failure.
>    */
> -int led_classdev_register_ext(struct device *parent,
> -			      struct led_classdev *led_cdev,
> -			      struct led_init_data *init_data)
> +int led_classdev_init(struct device *parent, struct led_classdev *led_cdev,
> +		      struct led_init_data *init_data)
>   {
>   	char composed_name[LED_MAX_NAME_SIZE];
>   	char final_name[LED_MAX_NAME_SIZE];
> @@ -539,23 +550,54 @@ int led_classdev_register_ext(struct device *parent,
>   	if (!led_cdev->max_brightness)
>   		led_cdev->max_brightness = LED_FULL;
>   
> -	mutex_lock(&led_cdev->led_access);
> -	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
> -						  led_cdev->groups, "%s", final_name);
> -	if (IS_ERR(led_cdev->dev)) {
> -		mutex_unlock(&led_cdev->led_access);
> -		return PTR_ERR(led_cdev->dev);
> +	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
> +	if (!led_cdev->dev)
> +		return -ENOMEM;
> +
> +	device_initialize(led_cdev->dev);
> +	led_cdev->dev->class = &leds_class;
> +	led_cdev->dev->parent = parent;
> +	led_cdev->dev->groups = led_cdev->groups;
> +	dev_set_drvdata(led_cdev->dev, led_cdev);
> +	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
> +	if (ret < 0)
> +	{
> +		put_device(led_cdev->dev);
> +		return ret;
>   	}
>   
>   	if (init_data && init_data->fwnode)
>   		device_set_node(led_cdev->dev, init_data->fwnode);

Sashiko has the following complain about this:

Because led_cdev->dev is a class device that never binds to a driver:

1. __fw_devlink_link_to_consumers(led_cdev->dev) converts consumer
fwnode_links
     on init_data->fwnode into managed device links with
led_cdev->dev as the
     supplier instead of the parent LED controller. During boot (while
     fw_devlink_drv_reg_done is false), consumers remain blocked with
     -EPROBE_DEFER until fw_devlink_drivers_done() runs, and are
never linked to
     the parent controller for runtime PM, suspend/resume, or unbind
ordering.

2. __fw_devlink_link_to_suppliers(led_cdev->dev, fwnode) creates
managed device
     links with led_cdev->dev as the consumer for any supplier
properties on the
     child LED node (such as gpios in gpio-leds or pwms in pwm-leds).
Since
     led_cdev->dev never probes a driver, those links stay in
DL_STATE_AVAILABLE
     and prevent the GPIO or PWM suppliers from reaching sync_state().

I have no experience with devicetree, so i cannot verify if this is
true, why do we call device_set_node() in the first place after having 
already registered the device?

Thanks,
Armin Wolf

>   
> -	led_update_brightness(led_cdev);
> -
>   	led_cdev->wq = leds_wq;
>   
>   	led_init_core(led_cdev);
>   
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_init);
> +
> +/**
> + * led_classdev_add - Add a new LED class device to the subsystem.
> + * @led_cdev: the led_classdev structure for this device.
> + *
> + * Adds a new LED class device to the subsystem, allowing userspace applications
> + * and/or kernel consumers to access it.
> + *
> + * Returns: Zero on success or a negative error code on failure.
> + */
> +int led_classdev_add(struct led_classdev *led_cdev)
> +{
> +	int ret;
> +
> +	guard(mutex)(&led_cdev->led_access);
> +
> +	ret = device_add(led_cdev->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_update_brightness(led_cdev);
> +
>   	/* add to the list of leds */
>   	down_write(&leds_list_lock);
>   	list_add_tail(&led_cdev->node, &leds_list);
> @@ -565,13 +607,36 @@ int led_classdev_register_ext(struct device *parent,
>   	led_trigger_set_default(led_cdev);
>   #endif
>   
> -	mutex_unlock(&led_cdev->led_access);
> -
> -	dev_dbg(parent, "Registered led device: %s\n",
> -			led_cdev->name);
> +	dev_dbg(led_cdev->dev->parent, "Registered led device: %s\n", led_cdev->name);
>   
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(led_classdev_add);
> +
> +/**
> + * led_classdev_register_ext - register a new object of led_classdev class
> + *                             with init data.
> + *
> + * @parent: parent of LED device.
> + * @led_cdev: the led_classdev structure for this device.
> + * @init_data: LED class device initialization data.
> + */
> +int led_classdev_register_ext(struct device *parent,
> +                              struct led_classdev *led_cdev,
> +                              struct led_init_data *init_data)
> +{
> +	int ret;
> +
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
> +}
>   EXPORT_SYMBOL_GPL(led_classdev_register_ext);
>   
>   /**
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 6d00e6f44126..2d977c5a3520 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -18,6 +18,9 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
>   }
>   
>   void led_init_core(struct led_classdev *led_cdev);
> +int led_classdev_init(struct device *parent, struct led_classdev *led_cdev,
> +		      struct led_init_data *init_data);
> +int led_classdev_add(struct led_classdev *led_cdev);
>   void led_stop_software_blink(struct led_classdev *led_cdev);
>   void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value);
>   void led_set_brightness_nosleep(struct led_classdev *led_cdev, unsigned int value);