linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Jisheng Zhang <Jisheng.Zhang@synaptics.com>,
	Kever Yang <kever.yang@rock-chips.com>,
	Android Kernel Team <kernel-team@android.com>,
	linux-gpio@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default
Date: Wed, 20 Jan 2021 16:48:31 +0100	[thread overview]
Message-ID: <YAhQzxaHCffpPSdQ@kroah.com> (raw)
In-Reply-To: <01f733ab81959e4cf847cbf1d521ad9d@kernel.org>

On Wed, Jan 20, 2021 at 03:39:30PM +0000, Marc Zyngier wrote:
> On 2021-01-18 20:38, Saravana Kannan wrote:
> > On Sun, Jan 17, 2021 at 4:02 AM Marc Zyngier <maz@kernel.org> wrote:
> > > 
> > > Hi Saravana,
> > > 
> > > Thanks for posting this, much appreciated.
> > > 
> > > On Sat, 16 Jan 2021 01:14:11 +0000,
> > > Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > There are multiple instances of GPIO devictree nodes of the form:
> > > >
> > > > foo {
> > > >       compatible = "acme,foo";
> > > >       ...
> > > >
> > > >       gpio0: gpio0@xxxxxxxx {
> > > >               compatible = "acme,bar";
> > > >               ...
> > > >               gpio-controller;
> > > >       };
> > > >
> > > >       gpio1: gpio1@xxxxxxxx {
> > > >               compatible = "acme,bar";
> > > >               ...
> > > >               gpio-controller;
> > > >       };
> > > >
> > > >       ...
> > > > }
> > > >
> > > > bazz {
> > > >       my-gpios = <&gpio0 ...>;
> > > > }
> > > >
> > > > Case 1: The driver for "foo" populates struct device for these gpio*
> > > > nodes and then probes them using a driver that binds with "acme,bar".
> > > > This lines up with how DT nodes with the "compatible" property are
> > > > generally converted to struct devices and then registered with driver
> > > > core to probe them. This also allows the gpio* devices to hook into all
> > > > the driver core capabilities like runtime PM, probe deferral,
> > > > suspend/resume ordering, device links, etc.
> > > >
> > > > Case 2: The driver for "foo" doesn't populate its child device nodes
> > > > with "compatible" property and instead just loops through its child
> > > > nodes and directly registers the GPIOs with gpiolib without ever
> > > > populating a struct device or binding a driver to it.
> > > 
> > > That's not quite an accurate description. The gpiolib subsystem does
> > > create a struct device. It doesn't register a driver though, which is
> > > what causes the issue with fr_devlink (more on that below).
> > 
> > The devices created by gpiolib care are created for case 1 and case 2.
> > They are just devices gpiolib uses to represent a virtual software
> > device to hook different attributes to and expose them in sysfs. I'm
> > not talking about those devices here. The devices I'm referring to are
> > devices that represent the actual HW IP -- so what I'm saying is
> > accurate.
> 
> Please read what you wrote : "without ever populating a struct device".
> I stand by the "not quite accurate".
> 
> > 
> > > >
> > > > Drivers that follow the case 2 cause problems with fw_devlink=on.  This
> > > > is because fw_devlink will prevent bazz from probing until there's a
> > > > struct device that has gpio0 as its fwnode (because bazz lists gpio0 as
> > > > a GPIO supplier). Once the struct device is available, fw_devlink will
> > > > create a device link between with gpio0 as the supplier and bazz as the
> > > > consumer. After this point, the device link will prevent bazz from
> > > > probing until its supplier (the gpio0 device) has bound to a driver.
> > > > Once the supplier is bound to a driver, the probe of bazz is triggered
> > > > automatically.
> > > >
> > > > Finding and refactoring all the instances of drivers that follow case 2
> > > > will cause a lot of code churn and it not something that can be done in
> > > > one shot. Examples of such instances are [1] [2].
> > > >
> > > > This patch works around this problem and avoids all the code churn by
> > > > simply creating a stub driver to bind to the gpio_device. Since the
> > > > gpio_device already points to the GPIO device tree node, this allows all
> > > > the consumers to continue probing when the driver follows case 2.
> > > >
> > > > If/when all the old drivers are refactored, we can revert this
> > > > patch.
> > > 
> > > My personal gripe with this approach is that it is an abrupt change in
> > > the way DT and device model are mapped onto each other.
> > > 
> > > As far as I know (and someone please correct me if I am wrong), there
> > > is zero expectation that a device/subdevice/random IP block described
> > > by a node with a "compatible" property will end-up being managed by a
> > > driver that is bound to that particular node.
> > > 
> > > The node/subnode division is a good way to express some HW boundaries,
> > > but doesn't say anything about the way this should be handled in the
> > > kernel. Assuming that everything containing a "compatible" string will
> > > eventually be bound to a driver is IMO pretty limiting.
> > 
> > The default behavior of of_platform_populate() is to create a struct
> > device for every node with "compatible" property. That's how top level
> > devices (or children of simple-bus devices) are populated. IIRC,
> > that's what a lot of other busses do too. So, if anything, not having
> > a struct device for nodes with "compatible" property is an exception.
> 
> Again, that's not a description of the reality. The case we are talking
> about here does have a struct device. The misconception you keep
> repeating is that binding it to a driver is the only valid approach.
> 
> > 
> > Honestly, if one has a driver that supports a HW IP, I don't see any
> > reason it should operate outside of the device-driver model supported
> > by driver core. The driver code is there for a reason -- to solve all
> > the common problems faced by drivers.
> 
> News flash: this isn't the case. Most of the code I deal with cannot
> be represented as a driver, because it is needed way earlier
> than the device model.
> 
> > Operating outside of it just
> > causes reinventing the wheel, things like playing chicken with
> > initcalls to make sure drivers initialize their device in the right
> > order, not working with deferred probe, etc. For example, the rockchip
> > driver in your device (the one that follows case 2) tries to get some
> > clocks. But if that fails with -EPROBE_DEFER, the driver has no way
> > for it to recover and just doesn't register the GPIO anymore.
> > 
> > Obviously exceptions are allowed for devices that are needed before
> > the driver core even comes up -- like early, clocks, irqs and timers
> > for the kernel/scheduler to kick off.
> 
> There you go. Exceptions. Tons of. The trouble is, you are rewriting
> the rules of the game. Except that you are about 10 year late to
> the party. Forcing your changes on everyone by saying that was
> perfectly valid a month ago is now illegal doesn't really fly.
> 
> Anyway, I said what I had to say. If platforms break with this
> change, I'll expect it to be disabled in 5.12.

I'm thinking we can not change the default and will probably revert this
patch "soon".

thanks,

greg k-h

  reply	other threads:[~2021-01-20 16:21 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-16  1:14 [PATCH v2] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default Saravana Kannan
2021-01-16 20:37 ` Andy Shevchenko
2021-01-17 12:02 ` Marc Zyngier
2021-01-18 20:38   ` Saravana Kannan
2021-01-20 15:39     ` Marc Zyngier
2021-01-20 15:48       ` Greg Kroah-Hartman [this message]
2021-01-20 15:58         ` Marc Zyngier
2021-01-20 17:01           ` Greg Kroah-Hartman
2021-01-21 13:00 ` Linus Walleij
2021-01-21 19:43   ` Saravana Kannan
2021-01-22 13:04     ` Linus Walleij
2021-01-22 19:35 [PATCH v5] " Saravana Kannan
     [not found] ` <CAHp75VfKiuVd7JO-0nwCuvy7tgPZScOpKX8Q4+oT+JSBR+d=ew@mail.gmail.com>
2021-01-26 18:16   ` [PATCH v2] " Saravana Kannan
2021-01-26 18:32     ` Andy Shevchenko

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=YAhQzxaHCffpPSdQ@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=Jisheng.Zhang@synaptics.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=kernel-team@android.com \
    --cc=kever.yang@rock-chips.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=saravanak@google.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).