Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-usb <linux-usb@vger.kernel.org>,
	linux-pci <linux-pci@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198
Date: Wed, 6 Jan 2021 16:17:47 +0100	[thread overview]
Message-ID: <CAFBinCBCYZ3bzvvn==CFZyVh8E7TiGvW9PnqmK-Qd=y4X2HgNw@mail.gmail.com> (raw)
In-Reply-To: <CACRpkdbKQaT61w6r9Hx40Qvy+7qyLNm-fx-BpL_wdGcB=tmcqQ@mail.gmail.com>

Hi Linus,

On Tue, Jan 5, 2021 at 11:23 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> > On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> wrote:
> > [...]
> > > > As noted on the earlier patches I think this should be folded into the
> > > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> > > > gets messy, as a separate bolt-on, something like
> > > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> > > > You can use a Kconfig symbol for the GPIO portions or not.
> > > OK, I will do that if there are no objections from other developers
> > > I am intending to place the relevant code in xhci-pci-etron.c, similar
> > > to what we already have with xhci-pci-renesas.c
> >
> > I tried this and unfortunately there's a catch.
> > the nice thing about having a separate GPIO driver means that the
> > xhci-pci driver doesn't need to know about it.
>
> Since PCI devices have device-wide power management and things
> like that I think that is a really dangerous idea.
>
> What if the GPIO driver starts poking around in this PCI device
> when the main driver is also probed and has put the device
> into sleep state?
that is asking for trouble, indeed.

[...]
> > I implemented xhci-pci-etron.c and gave it a Kconfig option.
> > xhci-pci is then calling into xhci-pci-etron (through some
> > etron_xhci_pci_probe function).
>
> This sounds about right.
>
> > unfortunately this means that xhci-pci now depends on xhci-pci-etron.
> > for xhci-pci-renesas this is fine (I think) because that part of the
> > code is needed to get the xHCI controller going
> > but for xhci-pci-etron this is a different story: the GPIO controller
> > is entirely optional and only used on few devices
>
> I might be naive but should it not be the other way around?
> That xhci-pci-etron is dependent on xhci-pci? I imagine
> it would be an optional add-on.
the only way to achieve this that I can think of is to basically have
xhci-pci-etron implement it's own pci_driver and then call
xhci_pci_probe, xhci_pci_remove, etc.
but then it depends on the driver load order if the GPIO controller is exposed

what structure did you have in mind to achieve this?

> > my goal is (at some point in the future) to have the GPIO driver in OpenWrt.
> > I am not sure if they would accept a patch where xhci-pci would then
> > pull in the dependencies for that Etron controller, even though most
> > boards don't need it.
>
> Make sure the etron part is an additional module that can be
> loaded after xhci-pci.
my approach from above unfortunately would not achieve this
so if you have an idea how to achieve this (or have any other driver
in mind that I can use as reference, even if not related to
GPIO/USB/PCI then please let me know)

> OpenWrt support optional modules to be compiled per-system.
that I already found out. That's why I think that I need to get the
driver part "right" and then get the OpenWrt part done in just a few
lines of their build-system

Best regards,
Martin