Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Jun 23, 2020 at 4:49 AM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> On Sat, Jun 20, 2020 at 01:13:56PM +0300, Andy Shevchenko wrote:
> > On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> > > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> > > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> > > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:

...

> > Linus replied to three messages out of 7, seems you read only the first one.

> Actually I've read all of them and from what I could see, he didn't like a code
> setting gpio-base regardless whether it has been created before your patchset
> or your explanation why it was necessary.

Yes, and then he realized what happened in the past. TBH I also do not
like this kind of thing, but we may not get rid of it because we will
get a regression.
The compromise (which is used in several cases of my knowledge, but I
believe there are more) is to use internal property. This keeps layers
separate and individual drivers nicer and neater.

...

> > > My point is that no mater how you pass the gpio-base value it will always be a
> > > quirk. The same thing is with the irq_shared flag. Both of them are specific to
> > > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would
> > > create a GPIO device working well with the Quark-specific DW APB GPIO block.
> >
> > Precisely!
> >
> > > > > > We, by applying this series, make (keep) layers independent: board
> > > > > > code vs. driver code. Mixing them more is the opposite to what I
> > > > > > propose.
> > > > > >
> > > > > > WRT property.
> > > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > > > > > being both converted to linux,gpio-base to explicitly show people that
> > > > > > this is internal stuff that must not be present in firmware nodes.
> > > > >
> > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the
> > > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> > > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> > > > > the driver.
> > > >
> > >
> > > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.
> > >
> > > As I said above having the gpio-base value set is the platform-specific thing in
> > > any case. So no mater whether you pass it via the software_node/properties or
> > > the private platform-data structure, the DW APB GPIO driver will have to handle
> > > those parameters in some special way, which is quirk-prone since normal platforms
> > > don't have such peculiar requirements.
> >
>
> > Seems you are proposing layering violation without seeing it.
> > Let me explain the design of the drivers in Linux kernel.
> >
> > There are basically few possible concepts how drivers are handling
> > quirks (because we know that most of the hardware support is a set of
> > quirks here and there):
> > 1) platform provides a board files and via platform data structures
> > supplies quirks to the certain driver
> > 2) platform provides a firmware node (ACPI, DT, ...) and unified
> > driver handles it thru the fwnode interface
> > 3) driver is split to be a library (or core part) + glue drivers full of quirks
> > 4) driver has embedded quirks and supplies them based on IDs (PCI ID,
> > ACPI ID, compatible string, ID table, etc)
> > 5) ...missed something? ...
> >
> > What I'm proposing is turn 1 to 2 for Quark case, what you are
> > proposing is absent on the picture, i.e.:
> > x) platform provides a board file (intel_quark_i2c_gpio.c) and the
> > driver has embedded quirks based on ID.
> >
> > This is not what we want, definitely.
>
> From the list you provided it's still not obvious why what I suggested wasn't
> good, because it's perfectly fine to have both ACPI/DT-based firmware node
> (entry 2) and quirks based on IDs (entry 4), which plenty of the kernel
> drivers do.

And I'm not talking about combinations of 2+4, what I'm talking about
is 1+4 which *is* layering violation and a bad idea to start with.

> The difference between the most of those drivers and what I
> suggested is that by bonding a device with a driver they provide
> !device!-quirks, but not the !platform!-quirks. While the platform quirks
> and platform properties are normally provided by means of the platform data,
> firmware nodes, glue layers, etc. Moving some of the platform-quirks to a
> driver you called "layering violation". Well, I've never met that definition
> in the kernel before

> (have you just come up with it?),

Nope, I heard it many times during reviews.

> but at least I see
> what you meant.

> Anyway there are GPIO-drivers which still use the device IDs to get the
> platform quirks

Maybe you missed that we have more players here than simple dwapb-gpio?
How many of them are part of MFD (being used as MFD cells)?

> or get the GPIO-base from an of node alias (gpio-zynq.c,
> gpio-vf610.c, gpio-zx.c, gpio-mxc.c, gpio-mxs.c). There are even some,
> which either use a static variable to redistribute the GPIO-base between
> all available GPIO-chips (gpio-brcmstb.c, gpio-sta2x11.c, gpio-omap.c)
> or set a fixed GPIO-based value (like gpio-xlp.c, gpio-iop.c, gpio-ep93xx.c,
> gpio-vx855.c, gpio-cs5535.c, gpio-sch311x.c, gpio-loongson.c, gpio-loongson1.c,
> gpio-ath79.c, gpio-octeon.c),

So,  the question is above.

> or even get the GPIO-base value from some
> hardware register (gpio-merrifield.c, gpio-intel-mid.c).

These examples are not good, they are not part of MFD and the latter
one actually should be a glue driver to gpio-pxa.c.

> I am pretty sure
> the examples of having the locally-defined platform quirks and the concepts
> 1, 2, 3 and 4 at some extent utilized in a single driver can be found in another
> subsystems too.

I'm pretty sure layering violation can be found in many places in the
kernel. It doesn't mean we have to take bad or different examples as
suitable.

> I am not saying, that the approaches utilized in those drivers are ideal.
> Those are just examples, that the platform specifics can be reflected in
> the corresponding drivers and the so called "layering violation" is allowed
> at some circumstances. Linus, correct me if I am wrong.

It depends how a certain driver is being used. In our case we don't
need to spread board code over the kernel, we may be smarter than
that!

> Getting back to this patchset. As I see it, the main problem here is connected
> with two parameters:
> - GPIO-base. You suggest to update the gpio-dwapb.c driver so one would
>   support a firmware property like "gpio-base". It's not good, since the
>   property will be implicitly supported by OF API as well and nothing will
>   prevent a user from using it. Even though you said that we won't advertise
>   that property, some user may try to define it in dts anyway, which can be
>   easily missed on review.

No, if we don't advertise that and if we add "linux," prefix (see DWC3
for example) to be explicit what this is used for and why.

> - IRQ-shared. As I said before it's not good to replace the irq_shared flag
>   with the to_of_node() macro. Because having to_of_node() returned an
>   of-node doesn't mean the IRQs can't be shared.

This is a valid point, but I would ask you, as a more familiar guy
with the OF/DT system,  what suggestion would be better?

> As I see it the convenience provided by your patchset in relation to the
> GPIO-base and IRQ-shared properties doesn't overcome the problems denoted
> above. IMO it would be better either to move the GPIO-base and the IRQ-shared
> parameters definition to the gpio-dwapb.c driver despite of the so called
> "layering violation" or just leave them in the MFD driver. Linus, please join
> the discussion. Do you have any better idea of what to do with these
> properties?

Moving them to gpio-dwapb is a silly move. Better to do nothing if you
insist, but I consider that is non-constructive.

--
With Best Regards,
Andy Shevchenko