Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> Add ability to fill pdata from device tree. Common stuff is
> filled from core driver and then pdata is filled per-device
> since all cells are optional.

...

>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mfd/core.h>

> +#include <linux/of.h>

Is it used? In any case, please no OF-centric APIs in a new (feature) code.

>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,

pass_of_node sounds a bit awkward.
Perhaps you thought about parse_fwnode ?

> +			       struct lm3533_als_platform_data *pdata)
> +{
> +	struct device *parent_dev = pdev->dev.parent;
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *node;
> +	const char *label;
> +	int val, ret;
> +
> +	device_for_each_child_node(parent_dev, node) {
> +		fwnode_property_read_string(node, "compatible", &label);
> +
> +		if (!strcmp(label, pdev->name)) {

This is a bit strange. Why one need to compare platform device instance (!)
name with compatible which is part of ABI. This looks really wrong approach.
Needs a very good explanation on what's going on here.

Besides that the label is usually filled by LEDS core, why do we need to handle
it in a special way?

> +			ret = fwnode_property_read_u32(node, "reg", &val);
> +			if (ret) {
> +				dev_err(dev, "reg property is missing: ret %d\n", ret);
> +				return ret;
> +			}
> +
> +			if (val == pdev->id) {

> +				dev->fwnode = node;
> +				dev->of_node = to_of_node(node);

No direct access to fwnode in struct device, please use device_set_node().

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get als device node\n");

With

	struct device *dev = &pdev->dev;

at the top of the function will help a lot in making the code neater and easier
to read.

> +		lm3533_parse_als(pdev, pdata);
>  	}

...

>  #include <linux/leds.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mutex.h>

> +#include <linux/of.h>

Cargo cult? "Proxy" header? Please follow IWYU principle.

>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>

...

> +static void lm3533_parse_led(struct platform_device *pdev,
> +			     struct lm3533_led_platform_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	int val, ret;
> +
> +	ret = device_property_read_string(dev, "default-trigger",
> +					  &pdata->default_trigger);
> +	if (ret)
> +		pdata->default_trigger = "none";

Isn't this done already in LEDS core?

> +	/* 5000 - 29800 uA (800 uA step) */
> +	ret = device_property_read_u32(dev, "max-current-microamp", &val);
> +	if (ret)
> +		val = 5000;
> +	pdata->max_current = val;
> +
> +	/* 0 - 0x3f */
> +	ret = device_property_read_u32(dev, "pwm", &val);
> +	if (ret)
> +		val = 0;
> +	pdata->pwm = val;
> +}

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,
> +			       struct lm3533_led_platform_data *pdata)

I think I already saw exactly the same piece of code. Please make sure
you have no duplications.

> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get led device node\n");
> +
> +		lm3533_parse_led(pdev, pdata);

Ditto.

>  	}

...

> -	led->cdev.name = pdata->name;
> +	led->cdev.name = dev_name(&pdev->dev);

Are you sure it's a good idea?

...

> -	if (!pdata->als)
> +	if (!pdata->num_als)
>  		return 0;
>  
> -	lm3533_als_devs[0].platform_data = pdata->als;
> -	lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> +	if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> +		pdata->num_als = ARRAY_SIZE(lm3533_als_devs);

Looks like you want

	pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
	if (!pdata->num_als)
		return 0;

instead of the above. You would need minmax.h for that.

...

> +	if (pdata->leds) {

This is strange. I would expect num_leds == 0 in this case

> +		for (i = 0; i < pdata->num_leds; ++i) {
> +			lm3533_led_devs[i].platform_data = &pdata->leds[i];
> +			lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> +		}
>  	}

...

> +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> +			       struct lm3533_platform_data *pdata)
> +{
> +	struct fwnode_handle *node;
> +	const char *label;
> +
> +	device_for_each_child_node(lm3533->dev, node) {
> +		fwnode_property_read_string(node, "compatible", &label);
> +
> +		if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> +			pdata->num_backlights++;
> +
> +		if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> +			pdata->num_leds++;
> +
> +		if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> +			pdata->num_als++;
> +	}
> +}

Oh, I don't like this approach. If you have compatible, you have driver_data
available, all this is not needed as it may be hard coded.

...

>  	if (!pdata) {

I would expect actually that legacy platform data support will be simply killed
by this patch(es). Do we have in-kernel users? If so, they can be easily
converted to use software nodes, otherwise we even don't need to care.

> -		dev_err(lm3533->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = device_property_read_u32(lm3533->dev,
> +					       "ti,boost-ovp",
> +					       &pdata->boost_ovp);
> +		if (ret)
> +			pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> +
> +		ret = device_property_read_u32(lm3533->dev,
> +					       "ti,boost-freq",
> +					       &pdata->boost_freq);
> +		if (ret)
> +			pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> +
> +		lm3533_parse_nodes(lm3533, pdata);
> +
> +		lm3533->dev->platform_data = pdata;
>  	}

...

> +static const struct of_device_id lm3533_match_table[] = {
> +	{ .compatible = "ti,lm3533" },
> +	{ },

No comma in the terminator entry.

> +};

...

> +static void lm3533_parse_backlight(struct platform_device *pdev,

pdev is not actually used, just pass struct device *dev directly.
Same comment to other functions in this change. It will make code more
bus independent and reusable.

> +				   struct lm3533_bl_platform_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	int val, ret;
> +
> +	/* 5000 - 29800 uA (800 uA step) */
> +	ret = device_property_read_u32(dev, "max-current-microamp", &val);
> +	if (ret)
> +		val = 5000;
> +	pdata->max_current = val;

> +	/* 0 - 255 */
> +	ret = device_property_read_u32(dev, "default-brightness", &val);
> +	if (ret)
> +		val = LM3533_BL_MAX_BRIGHTNESS;
> +	pdata->default_brightness = val;

Isn't handled by LEDS core?

> +	/* 0 - 0x3f */
> +	ret = device_property_read_u32(dev, "pwm", &val);
> +	if (ret)
> +		val = 0;
> +	pdata->pwm = val;
> +}

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,
> +			       struct lm3533_bl_platform_data *pdata)
> +{

3rd dup?

> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get backlight device node\n");
> +
> +		lm3533_parse_backlight(pdev, pdata);
>  	}

Ditto.

> -	bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> -					pdev->dev.parent, bl, &lm3533_bl_ops,
> -					&props);
> +	bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),

I'm not sure the dev_name() is a good idea. We usually try to rely on the
predictable outcome, something like what "%pfw" prints against a certain fwnode.

> +					    pdev->dev.parent, bl,
> +					    &lm3533_bl_ops, &props);

...

Also I feel that this change may be split to a few separate logical changes.

-- 
With Best Regards,
Andy Shevchenko