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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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