Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231006115147.18559-1-brgl@bgdev.pl> <ZSAIUVAQ6ifi8LTL@smile.fi.intel.com>
 <CAMRc=MdrLSPCEsQ6OEgRX-7Wh7ka+Rczja=QjY-srozj3cz68w@mail.gmail.com> <ZSEMnqAynnrfBxX1@smile.fi.intel.com>
In-Reply-To: <ZSEMnqAynnrfBxX1@smile.fi.intel.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Mon, 9 Oct 2023 20:28:21 +0200
Message-ID: <CAMRc=MfFEBSeJ78NO7XeuzAMJ0KezEPAYWsWnFXXaRyQPAf3dA@mail.gmail.com>
Subject: Re: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct gpio_chip
To:     Andy Shevchenko <andy@kernel.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Dipen Patel <dipenp@nvidia.com>,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Oct 7, 2023 at 9:45=E2=80=AFAM Andy Shevchenko <andy@kernel.org> wr=
ote:
>
> On Fri, Oct 06, 2023 at 09:07:54PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Oct 6, 2023 at 3:15=E2=80=AFPM 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 t=
o
> > > > 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 devic=
e by
> > > > fwnode unless it was explicitly assigned to the chip in the provide=
r
> > > > 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 descri=
ptions
> of the GPIO controller, when it's 1:1 to the banks and when it's one devi=
ce
> 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 wo=
rk in
> > > > all cases.
> > >
> > > I don't think this is a good change. We paper over the real issue whe=
re
> > > 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 be=
en
> > > 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 understan=
d
> > > the lifetime of the respective object.
> > >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>