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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Oct 7, 2023 at 1:14 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Fri, Oct 6, 2023 at 1:51 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> > 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. 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.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> because we want the code to work (rough consensus and running code)
>

> The core of the crux is that we have
> information duplication with a reference to the fwnode in two
> places. One in gdev->dev and one in gc->fwnode.

No, we don't. We have plenty of drivers that have gc->fwnode == NULL,
which means that it is shared with the parent device.


...

> A gdev is created for each gpio_chip so in my naive brain we could
> get rid of gc->fwnode and only have the one inside gdev->dev?
> +/- some helpful getters/setters if need be.
>
> Or what am I thinking wrong here?

That would work I think. But I'm definitely against this change. It is
the way to nowhere. We should really be quite strict about fwnode and
do NOT assign the gc one behind the provider's back. If something is
not working in this scenario, that should be fixed and not with a hack
like this.

-- 
With Best Regards,
Andy Shevchenko