Re: [PATCH 01/24] leds: class: Improve LED and LED flash class registration API
From: Jacek Anaszewski
Date: Thu Nov 08 2018 - 15:47:48 EST
Hi Dan,
Thanks for the review.
On 11/08/2018 06:50 PM, Dan Murphy wrote:
> Jacek
>
> On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
[...]
>>
>> /**
>> - * of_led_classdev_register - register a new object of led_classdev class.
>> + * led_classdev_register_ext - register a new object of led_classdev class.
>
> For this should the comment indicate a different between the extended and non-extended calls?
> Like the _ext might say register a new object of led_classdev with init_data?
>
> Same comments for each addition below.
Agreed, I will amend it as you propose.
> Otherwise with Baolin's comments for 1/24
>
> Acked-by: Dan Murphy <dmurphy@xxxxxx>
>
> Dan
>
>> *
>> * @parent: parent of LED device
>> * @led_cdev: the led_classdev structure for this device.
>> - * @np: DT node describing this LED
>> + * @init_data: LED class device initialization data
>> */
>> -int of_led_classdev_register(struct device *parent, struct device_node *np,
>> - struct led_classdev *led_cdev)
>> +int led_classdev_register_ext(struct device *parent,
>> + struct led_classdev *led_cdev,
>> + struct led_init_data *init_data)
>> {
>> char name[LED_MAX_NAME_SIZE];
>> int ret;
>>
>> + if (init_data && init_data->name) {
>> + led_cdev->name = init_data->name;
>> + } else {
>> + dev_info(parent, "init_data not available");
>> + }
>> +
>> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>> if (ret < 0)
>> return ret;
>> @@ -268,7 +276,8 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>> mutex_unlock(&led_cdev->led_access);
>> return PTR_ERR(led_cdev->dev);
>> }
>> - led_cdev->dev->of_node = np;
>> + if (init_data && init_data->fwnode)
>> + led_cdev->dev->of_node = to_of_node(init_data->fwnode);
>>
>> if (ret)
>> dev_warn(parent, "Led %s renamed to %s due to name collision",
>> @@ -313,7 +322,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>>
>> return 0;
>> }
>> -EXPORT_SYMBOL_GPL(of_led_classdev_register);
>> +EXPORT_SYMBOL_GPL(led_classdev_register_ext);
>>
>> /**
>> * led_classdev_unregister - unregisters a object of led_properties class.
>> @@ -358,14 +367,15 @@ static void devm_led_classdev_release(struct device *dev, void *res)
>> }
>>
>> /**
>> - * devm_of_led_classdev_register - resource managed led_classdev_register()
>> + * devm_led_classdev_register_ext - resource managed led_classdev_register_ext()
>> *
>> * @parent: parent of LED device
>> * @led_cdev: the led_classdev structure for this device.
>> + * @init_data: LED class device initialization data
>> */
>> -int devm_of_led_classdev_register(struct device *parent,
>> - struct device_node *np,
>> - struct led_classdev *led_cdev)
>> +int devm_led_classdev_register_ext(struct device *parent,
>> + struct led_classdev *led_cdev,
>> + struct led_init_data *init_data)
>> {
>> struct led_classdev **dr;
>> int rc;
>> @@ -374,7 +384,7 @@ int devm_of_led_classdev_register(struct device *parent,
>> if (!dr)
>> return -ENOMEM;
>>
>> - rc = of_led_classdev_register(parent, np, led_cdev);
>> + rc = led_classdev_register_ext(parent, led_cdev, init_data);
>> if (rc) {
>> devres_free(dr);
>> return rc;
>> @@ -385,7 +395,7 @@ int devm_of_led_classdev_register(struct device *parent,
>>
>> return 0;
>> }
>> -EXPORT_SYMBOL_GPL(devm_of_led_classdev_register);
>> +EXPORT_SYMBOL_GPL(devm_led_classdev_register_ext);
>>
>> static int devm_led_classdev_match(struct device *dev, void *res, void *data)
>> {
>> diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
>> index 0e42624..1c82152 100644
>> --- a/drivers/leds/leds-cr0014114.c
>> +++ b/drivers/leds/leds-cr0014114.c
>> @@ -207,8 +207,7 @@ static int cr0014114_probe_dt(struct cr0014114 *priv)
>> led->ldev.max_brightness = CR_MAX_BRIGHTNESS;
>> led->ldev.brightness_set_blocking = cr0014114_set_sync;
>>
>> - ret = devm_of_led_classdev_register(priv->dev, np,
>> - &led->ldev);
>> + ret = devm_led_classdev_register(priv->dev, &led->ldev);
>> if (ret) {
>> dev_err(priv->dev,
>> "failed to register LED device %s, err %d",
>> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
>> index 32fa752..c87fbd3 100644
>> --- a/drivers/leds/leds-gpio.c
>> +++ b/drivers/leds/leds-gpio.c
>> @@ -112,7 +112,7 @@ static int create_gpio_led(const struct gpio_led *template,
>> if (ret < 0)
>> return ret;
>>
>> - return devm_of_led_classdev_register(parent, np, &led_dat->cdev);
>> + return devm_led_classdev_register(parent, &led_dat->cdev);
>> }
>>
>> struct gpio_leds_priv {
>> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
>> index 700efaa..28a73d0 100644
>> --- a/include/linux/led-class-flash.h
>> +++ b/include/linux/led-class-flash.h
>> @@ -90,15 +90,20 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
>> }
>>
>> /**
>> - * led_classdev_flash_register - register a new object of led_classdev class
>> - * with support for flash LEDs
>> + * fwnode_led_classdev_flash_register - register a new object of led_classdev
>> + * class with support for flash LEDs
>> * @parent: the flash LED to register
>> + * @fwnode: the flash LED fwnode handle
>> * @fled_cdev: the led_classdev_flash structure for this device
>> *
>> * Returns: 0 on success or negative error value on failure
>> */
>> -extern int led_classdev_flash_register(struct device *parent,
>> - struct led_classdev_flash *fled_cdev);
>> +extern int led_classdev_flash_register_ext(struct device *parent,
>> + struct led_classdev_flash *fled_cdev,
>> + struct led_init_data *init_data);
>> +
>> +#define led_classdev_flash_register(parent, fled_cdev) \
>> + led_classdev_flash_register_ext(parent, fled_cdev, NULL)
>>
>> /**
>> * led_classdev_flash_unregister - unregisters an object of led_classdev class
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 834683d..646c49a 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -20,6 +20,7 @@
>> #include <linux/spinlock.h>
>> #include <linux/timer.h>
>> #include <linux/workqueue.h>
>> +#include <uapi/linux/uleds.h>
>>
>> struct device;
>> /*
>> @@ -33,6 +34,13 @@ enum led_brightness {
>> LED_FULL = 255,
>> };
>>
>> +struct led_init_data {
>> + /* Device fwnode handle */
>> + struct fwnode_handle *fwnode;
>> + /* Requested LED class device name */
>> + char name[LED_MAX_NAME_SIZE];
>> +};
>> +
>> struct led_classdev {
>> const char *name;
>> enum led_brightness brightness;
>> @@ -73,6 +81,7 @@ struct led_classdev {
>> */
>> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>> enum led_brightness brightness);
>> +
>> /* Get LED brightness level */
>> enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>>
>> @@ -123,16 +132,16 @@ struct led_classdev {
>> struct mutex led_access;
>> };
>>
>> -extern int of_led_classdev_register(struct device *parent,
>> - struct device_node *np,
>> - struct led_classdev *led_cdev);
>> -#define led_classdev_register(parent, led_cdev) \
>> - of_led_classdev_register(parent, NULL, led_cdev)
>> -extern int devm_of_led_classdev_register(struct device *parent,
>> - struct device_node *np,
>> - struct led_classdev *led_cdev);
>> -#define devm_led_classdev_register(parent, led_cdev) \
>> - devm_of_led_classdev_register(parent, NULL, led_cdev)
>> +extern int led_classdev_register_ext(struct device *parent,
>> + struct led_classdev *led_cdev,
>> + struct led_init_data *init_data);
>> +#define led_classdev_register(parent, led_cdev) \
>> + led_classdev_register_ext(parent, led_cdev, NULL)
>> +extern int devm_led_classdev_register_ext(struct device *parent,
>> + struct led_classdev *led_cdev,
>> + struct led_init_data *init_data);
>> +#define devm_led_classdev_register(parent, led_cdev) \
>> + devm_led_classdev_register_ext(parent, led_cdev, NULL)
>> extern void led_classdev_unregister(struct led_classdev *led_cdev);
>> extern void devm_led_classdev_unregister(struct device *parent,
>> struct led_classdev *led_cdev);
>>
>
>
--
Best regards,
Jacek Anaszewski