Re: [PATCH leds v1 06/10] leds: pm8058: use struct led_init_data when registering

[Bjorn Andersson]

On Wed 16 Sep 18:16 CDT 2020, Marek Beh?n wrote:

> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
> 
> Signed-off-by: Marek Behún <marek.behun@xxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
>  drivers/leds/leds-pm8058.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
> index 7869ccdf70ce6..f6190e4af60fe 100644
> --- a/drivers/leds/leds-pm8058.c
> +++ b/drivers/leds/leds-pm8058.c
> @@ -87,36 +87,37 @@ static enum led_brightness pm8058_led_get(struct led_classdev *cled)
>  
>  static int pm8058_led_probe(struct platform_device *pdev)
>  {
> +	struct led_init_data init_data = {};
> +	struct device *dev = &pdev->dev;
> +	enum led_brightness maxbright;
> +	struct device_node *np;
>  	struct pm8058_led *led;
> -	struct device_node *np = pdev->dev.of_node;
> -	int ret;
>  	struct regmap *map;
>  	const char *state;
> -	enum led_brightness maxbright;
> +	int ret;
>  
> -	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);

The pdev->dev -> dev and of_node changes are reasonable, but shouldn't
be part of this patch. It simply makes it hard to reason about he actual
change.

Please respin this with only the introduction of led_init_data.

Thanks,
Bjorn

>  	if (!led)
>  		return -ENOMEM;
>  
> -	led->ledtype = (u32)(unsigned long)of_device_get_match_data(&pdev->dev);
> +	led->ledtype = (u32)(unsigned long)device_get_match_data(dev);
>  
> -	map = dev_get_regmap(pdev->dev.parent, NULL);
> +	map = dev_get_regmap(dev->parent, NULL);
>  	if (!map) {
> -		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
> +		dev_err(dev, "Parent regmap unavailable.\n");
>  		return -ENXIO;
>  	}
>  	led->map = map;
>  
> +	np = dev_of_node(dev);
> +
>  	ret = of_property_read_u32(np, "reg", &led->reg);
>  	if (ret) {
> -		dev_err(&pdev->dev, "no register offset specified\n");
> +		dev_err(dev, "no register offset specified\n");
>  		return -EINVAL;
>  	}
>  
>  	/* Use label else node name */
> -	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
> -	led->cdev.default_trigger =
> -		of_get_property(np, "linux,default-trigger", NULL);
>  	led->cdev.brightness_set = pm8058_led_set;
>  	led->cdev.brightness_get = pm8058_led_get;
>  	if (led->ledtype == PM8058_LED_TYPE_COMMON)
> @@ -142,14 +143,13 @@ static int pm8058_led_probe(struct platform_device *pdev)
>  	    led->ledtype == PM8058_LED_TYPE_FLASH)
>  		led->cdev.flags	= LED_CORE_SUSPENDRESUME;
>  
> -	ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "unable to register led \"%s\"\n",
> -			led->cdev.name);
> -		return ret;
> -	}
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> +	if (ret)
> +		dev_err(dev, "Failed to register LED for node %pOF\n", np);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct of_device_id pm8058_leds_id_table[] = {
> @@ -173,7 +173,7 @@ static struct platform_driver pm8058_led_driver = {
>  	.probe		= pm8058_led_probe,
>  	.driver		= {
>  		.name	= "pm8058-leds",
> -		.of_match_table = pm8058_leds_id_table,
> +		.of_match_table = of_match_ptr(pm8058_leds_id_table),
>  	},
>  };
>  module_platform_driver(pm8058_led_driver);
> -- 
> 2.26.2
>