Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Serge Semin <fancer.lancer@gmail.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH v1 0/6] mfd: Make use of software nodes
Date: Wed, 24 Jun 2020 15:43:35 +0300
Message-ID: <CAHp75Vd0YM=mRkUweK0TRhg9wyb2bs6-AO3m21BRE_=ZMEv8hg@mail.gmail.com> (raw)
In-Reply-To: <20200623014945.56j3hnwnz4cj2u4t@mobilestation>

On Tue, Jun 23, 2020 at 4:49 AM Serge Semin <fancer.lancer@gmail.com> 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@gmail.com> 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@gmail.com> 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

      reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-08 13:42 Andy Shevchenko
2020-06-08 13:42 ` [PATCH v1 1/6] gpio: dwapb: Replace irq_shared flag with fwnode type check Andy Shevchenko
2020-06-08 13:42 ` [PATCH v1 2/6] gpio: dwapb: Read GPIO base from snps,gpio-base property Andy Shevchenko
2020-06-10 11:26   ` Linus Walleij
2020-06-10 13:41     ` Andy Shevchenko
2020-06-08 13:42 ` [PATCH v1 3/6] mfd: core: Propagate software node group to the sub devices Andy Shevchenko
2020-06-08 19:25   ` Lee Jones
2020-06-09 12:40     ` Andy Shevchenko
2020-06-17 12:51       ` Lee Jones
2020-06-08 13:42 ` [PATCH v1 4/6] mfd: intel_quark_i2c_gpio: Convert to use software nodes Andy Shevchenko
2020-06-10 11:38   ` Linus Walleij
2020-06-10 13:43     ` Andy Shevchenko
2020-06-08 13:42 ` [PATCH v1 5/6] gpio: dwapb: Get rid of legacy platform data Andy Shevchenko
2020-06-10 11:39   ` Linus Walleij
2020-06-10 13:47     ` Andy Shevchenko
2020-06-08 13:43 ` [PATCH v1 6/6] gpio: dwapb: Define magic number for IRQ and GPIO lines Andy Shevchenko
2020-06-16 20:02 ` [PATCH v1 0/6] mfd: Make use of software nodes Serge Semin
2020-06-16 21:40   ` Andy Shevchenko
2020-06-16 22:56     ` Serge Semin
2020-06-18  8:56       ` Andy Shevchenko
2020-06-19 22:12         ` Serge Semin
2020-06-20 10:13           ` Andy Shevchenko
2020-06-23  1:49             ` Serge Semin
2020-06-24 12:43               ` Andy Shevchenko [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHp75Vd0YM=mRkUweK0TRhg9wyb2bs6-AO3m21BRE_=ZMEv8hg@mail.gmail.com' \
    --to=andy.shevchenko@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=fancer.lancer@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git