Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings

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

On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.

...

> @@ -57,6 +60,9 @@ struct lm3533_als {
>  
>  	atomic_t zone;
>  	struct mutex thresh_mutex;
> +
> +	bool pwm_mode;
> +	u32 r_select;
>  };

Have you run `pahole`? Does it agree with the layout you made here?

...

> -	als->irq = lm3533->irq;
> +	als->irq = platform_get_irq_optional(pdev, 0);

> +

Redundant blank line.

> +	if (als->irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

What about other error codes when IRQ is found by can't be retrieved for some
reasons? IIRC we check against ENXIO in similar cases

	als->irq = platform_get_irq_optional(pdev, 0);
	if (als->irq == -ENXIO)
		als->irq = 0;
	if (als->irq < 0)
		return als->irq;

...

> +	led->pwm = 0;

Isn't it 0 by zalloc ?

> +	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);

...

>  #define LM3533_BOOST_FREQ_MASK		0x01
>  #define LM3533_BOOST_FREQ_SHIFT		0
> +#define LM3533_BOOST_FREQ_MIN		500000
> +#define LM3533_BOOST_FREQ_MAX		1000000

HZ_PER_KHZ  (since you included units.h)?

...

> +	nchilds = device_get_child_node_count(dev);
> +	if (!nchilds || nchilds > LM3533_CELLS_MAX) {
> +		dev_err(dev, "num of child nodes is not supported\n");
> +		return -ENODEV;

Why not dev_err_probe() here and elsewhere? It looks inconsistent with this
patch.

>  	}

...

> +	device_for_each_child_node_scoped(lm3533->dev, child) {

> +		if (!fwnode_device_is_available(child))
> +			continue;

Do we need this check?

...

> +				dev_err(dev, "invalid LED node %s\n",
> +					fwnode_get_name(child));

%pfw

...

> +	ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);

No way. You should use .dev_groups.

> +	if (ret) {
> +		dev_err(dev, "failed to create sysfs attributes\n");
>  		goto err_unregister;
>  	}

...

Can you think on how to split this change to smaller steps? I believe it's
possible.

-- 
With Best Regards,
Andy Shevchenko