ср, 19 лют. 2025 р. о 16:27 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> пише:
>
> 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.
>
Splitting this into a set of commits would be nearly impossible,
original driver does not relay on OF, it relays on platform data.
Ripping out platform data will leave behind a broken useless driver.
So it has to be done simultaneously.
MFD part is removed since MFD cells binding is unconditional, while
the device supports any amount of children grater then one. For
example, my device uses only backlight at bank A, while all other
subdevices are not present and used. This patch switches to dynamic
bind of children.
> ...
>
> > +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.
>
This is not a scope of this patchset. Original driver uses &pdev->dev
> ...
>
> > +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.
>
This is required by core driver to handle some attributes and is here
solely not to touch those in this patch.
> ...
>
> > + 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
>
>
