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

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

On Wed, Feb 19, 2025 at 04:36:38PM +0200, Svyatoslav Ryhel wrote:
> ср, 19 лют. 2025 р. о 16:27 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.

...

> Splitting this into a set of commits would be nearly impossible,

I don't buy this.
One patch can introduce device property support.
Another one removes the old platform data interface.

So, at bare minimum there will be two patches. (Besides the advice to have
two more.)

> original driver does not relay on OF, it relays on platform data.

And?..

> Ripping out platform data will leave behind a broken useless driver.

Hmm... This cna be the case if and only if we have the user in kernel.
Is this the case?

> So it has to be done simultaneously.

Nope.

> 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.

MFD does the same. Please, take your time and get familiar with how MFD works.

...

> > > +     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

Indirectly it is. The change you are proposing tries to continue using this
construction with making needlessly longer.

...

> > > +             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.

What does this mean?

-- 
With Best Regards,
Andy Shevchenko