Re: [PATCH v2 2/2] leds: Switch to use fwnode instead of be stuck with OF one

Jacek Anaszewski
2019-08-23T18:14:56Z

Hi Andy,

Thank you for the patch. I have one issue below.

On 8/22/19 5:19 PM, Andy Shevchenko wrote:
> There is no need to be stuck with OF node when we may use agnostic
> firmware node instead.
>
> It allows users to get property if needed independently of provider.
>
> Note, some OF parts are left because %pfw [1] is in progress.
>
> [1]: https://lore.kernel.org/patchwork/cover/1054863/
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/leds/led-class.c | 4 ++--
> drivers/leds/led-core.c | 9 +++------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index d231240c2047..052c7571e188 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -14,7 +14,7 @@
> #include <linux/leds.h>
> #include <linux/list.h>
> #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/timer.h>
> @@ -277,7 +277,7 @@ int led_classdev_register_ext(struct device *parent,
> return PTR_ERR(led_cdev->dev);
> }
> if (init_data && init_data->fwnode)
> - led_cdev->dev->of_node = to_of_node(init_data->fwnode);
> + led_cdev->dev->fwnode = init_data->fwnode;
>
> if (ret)
> dev_warn(parent, "Led %s renamed to %s due to name collision",
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index f0c1c403f678..f1f718dbe0f8 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -324,14 +324,11 @@ EXPORT_SYMBOL_GPL(led_update_brightness);
>
> u32 *led_get_default_pattern(struct led_classdev *led_cdev, unsigned int *size)
> {
> - struct device_node *np = dev_of_node(led_cdev->dev);
> + struct fwnode_handle *fwnode = led_cdev->dev->fwnode;
> u32 *pattern;
> int count;
>
> - if (!np)
> - return NULL;

Why aren't you checking fwnode for being not NULL?
It is not guaranteed to be always initialized.

> -
> - count = of_property_count_u32_elems(np, "led-pattern");
> + count = fwnode_property_count_u32(fwnode, "led-pattern");
> if (count < 0)
> return NULL;
>
> @@ -339,7 +336,7 @@ u32 *led_get_default_pattern(struct led_classdev *led_cdev, unsigned int *size)
> if (!pattern)
> return NULL;
>
> - if (of_property_read_u32_array(np, "led-pattern", pattern, count)) {
> + if (fwnode_property_read_u32_array(fwnode, "led-pattern", pattern, count)) {
> kfree(pattern);
> return NULL;
> }
>

--
Best regards,
Jacek Anaszewski