Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <20231006115147.18559-1-brgl@bgdev.pl> <ZSAIUVAQ6ifi8LTL@smile.fi.intel.com>
In-Reply-To: <ZSAIUVAQ6ifi8LTL@smile.fi.intel.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Fri, 6 Oct 2023 21:07:54 +0200
Message-ID: <CAMRc=MdrLSPCEsQ6OEgRX-7Wh7ka+Rczja=QjY-srozj3cz68w@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 Fri, Oct 6, 2023 at 3:15=E2=80=AFPM Andy Shevchenko <andy@kernel.org> wr=
ote:
>
> 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. I'm not surprised users of that code will be confused -
like Dipen in this case.

Bart

> > 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 i=
n
> > 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 whe=
re
> > a GPIO lookup by fwnode does not work because the fwnode pointer in str=
uct
> > 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
>
>