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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPVz0n2WwAOb1UU7J7aDTdhXXCaAZkCpYjW_nc_CBRgkGWdEOw@mail.gmail.com>
Date: Wed, 19 Feb 2025 17:13:15 +0200
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Pavel Machek <pavel@....cz>, 
	Daniel Thompson <danielt@...nel.org>, Jingoo Han <jingoohan1@...il.com>, Helge Deller <deller@....de>, 
	Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-leds@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mfd: lm3533: convert to use OF

ср, 19 лют. 2025 р. о 17:07 Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> пише:
>
> On Wed, Feb 19, 2025 at 04:36:38PM +0200, Svyatoslav Ryhel wrote:
> > ср, 19 лют. 2025 р. о 16:27 Andy Shevchenko
> > <andriy.shevchenko@...ux.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.
>

It does not. I have tried. If mfd cell binding is missing, driver will
complain and fail.

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

Let this driver rot for now, I might return to it. At some point