Re: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct gpio_chip

[Andy Shevchenko <andy@kernel.org>]

On Fri, Oct 06, 2023 at 09:07:54PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 6, 2023 at 3:15 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > struct gpio_chip is not only used to carry the information needed to
> > > set-up a GPIO device but is also used in all GPIOLIB callbacks and is
> > > passed to the matching functions of lookup helpers.
> > >
> > > In that last case, it is currently impossible to match a GPIO device by
> > > fwnode unless it was explicitly assigned to the chip in the provider
> > > code.
> >
> > That's expected behaviour.
> 
> Is it though? We now have a GPIO device that represents a piece of
> physical hardware that has an fwnode assigned and the associated GPIO
> chip (tied to that device) that has none. How is that logical? It's
> not coherent.

To me it is pretty much logical, yes. The providers decide themselves
if they want to have any specific device node for the chip or inherit
it from the physical hardware. Note, there are two types of the FW descriptions
of the GPIO controller, when it's 1:1 to the banks and when it's one device
with list of children, one per bank. Due to this differences we have
this field in the GPIO chip to begin with.


> I'm not surprised users of that code will be confused -
> like Dipen in this case.

Which case? I'm still unsure you pictured the issue here.
Where can I read about it?

> > > If the fwnode is taken from the parent device, the pointer in
> > > struct gpio_chip will remain NULL.
> >
> > > If we have a parent device but gc->fwnode was not assigned by the
> > > provider, let's assign it ourselves so that lookup by fwnode can work in
> > > all cases.
> >
> > I don't think this is a good change. We paper over the real issue where
> > we and callers need to understand what they are looking for.
> >
> > ...
> >
> > > This is something that Dipen reported with one of the tegra drivers where
> > > a GPIO lookup by fwnode does not work because the fwnode pointer in struct
> > > gpio_chip is NULL. This patch addresses this use-case.
> >
> > I am not sure I understand the problem here. All these should have been
> > addressed already, no?
> >
> > So, the GPIOLIB should use dev_fwnode(&gdev->dev) inside it, outside it
> > the GPIO drivers are free to use gc->fwnode as long as they understand
> > the lifetime of the respective object.
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

-- 
With Best Regards,
Andy Shevchenko