вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <[email protected]> пише:
>
> 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?
>
Noted.
> ...
>
> > - als->irq = lm3533->irq;
> > + als->irq = platform_get_irq_optional(pdev, 0);
>
> > +
>
> Redundant blank line.
>
Simplifies code perception, whatever.
> > + 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
>
Then we treat it as no IRQ. Original implementation cares only if IRQ
is present or no.
> 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 ?
It is, thanks.
>
> > + 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)?
>
500 * HZ_PER_KHZ
1000 * HZ_PER_KHZ
You meant this? Sure.
> ...
>
> > + 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.
>
I must have overlooked it, thanks. WDYM elsewhere, this is the only occurance.
> > }
>
> ...
>
> > + device_for_each_child_node_scoped(lm3533->dev, child) {
>
> > + if (!fwnode_device_is_available(child))
> > + continue;
>
> Do we need this check?
>
This is nice to have if the node is disabled. If we assume that there
are no disabled nodes, I can remove it.
> ...
>
> > + dev_err(dev, "invalid LED node %s\n",
> > + fwnode_get_name(child));
>
> %pfw
>
Noted.
> ...
>
> > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
>
> No way. You should use .dev_groups.
>
I did not change how driver does this, just swapped lm3533->dev to
dev. I will set is back as it was.
> > + 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.
>
No, I am done with tinkering with this patchset. It is broken enough
and it has inflated enough.
> --
> With Best Regards,
> Andy Shevchenko
>
>
