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

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

On Tue, Feb 18, 2025 at 03:27:00PM +0200, Svyatoslav Ryhel wrote:
> Remove platform data and fully relay on OF and device tree
> parsing and binding devices.

Thanks for following the advice, but the problem with this change as it does
too much at once. It should be split to a few simpler ones.
On top of that, this removes MFD participation from the driver but leaves it
under MFD realm. Moreover, looking briefly at the code it looks like it open
codes the parts of MFD. The latter needs a very goo justification which commit
message is missing.

...

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

No comma for the terminator entry. I think I already pointed that out earlier.

> +};

...

> +	device_property_read_string(&pdev->dev, "linux,default-trigger",
> +				    &led->cdev.default_trigger);

One prerequisite patch you probably want is an introduction of

	struct device *dev = &pdev->dev;

in the respective ->probe() implementations. This, in particular, makes the
above lines shorter and fit one line.

...

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

As per above.

> +};

...

> +		if (!strcmp(comatible, "ti,lm3533-als"))
> +			lm3533->have_als = 1;

If you end up having this, it's not the best what we can do. OF ID tables have
a driver_data field exactly for the cases like this.

...

> +		if (!strcmp(comatible, "ti,lm3533-backlight"))
> +			lm3533->have_backlights = 1;

Ditto.

...

> +		if (!strcmp(comatible, "ti,lm3533-leds"))
> +			lm3533->have_leds = 1;

Ditto.

...

> +		ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
> +				    1 << (2 * id + 1), 1 << (2 * id + 1));

BIT() and better to use a temporary variable for this calculation.

> +		if (ret)
> +			return ret;

...

> +		ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> +				    id | id << 1, BIT(0) | BIT(1));

		mask = GENMASK();
		..., id ? mask : 0, mask);

> +		if (ret)
> +			return ret;
> +	}

...

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


With the advice from above:

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


>  	if (IS_ERR(bd)) {
>  		dev_err(&pdev->dev, "failed to register backlight device\n");
>  		return PTR_ERR(bd);

Consider another prerequisite patch (which should come before the firstly
proposed one):

	struct device *dev = &pdev->dev; // yes, this can go in this change
	...

	if (IS_ERR(bd))
		return dev_err_probe(dev, PTR_ERR(bd), "failed to register backlight device\n");

...

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

As per above.

> +};

-- 
With Best Regards,
Andy Shevchenko