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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Oct 7, 2023 at 9:45 AM Andy Shevchenko <andy@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > >
> > > > 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.
>

This is irrelevant for this discussion. The tegra driver in question
knows which fwnode it's using - the one from the parent device. It's
just that when the HTE driver tries to find the chip using either
gpiochip_find() or gpio_device_find(), it fails and I'm pretty sure
that if Dipen bisected it, it would point to commit daecca4b8433
("gpiolib: Do not alter GPIO chip fwnode member").

IMO the GPIO subsystem should take a phandle to the HTE engine it uses
for timestamping and that would allow us to not do the lookup at all
but that's a different discussion.

Anyway, I think Linus' suggestion is better than this patch.

Bart

>
> > 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
>
>