Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231006115147.18559-1-brgl@bgdev.pl> <ZSAKdOXpo+xOI3sJ@smile.fi.intel.com>
In-Reply-To: <ZSAKdOXpo+xOI3sJ@smile.fi.intel.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Fri, 6 Oct 2023 21:07:49 +0200
Message-ID: <CAMRc=MeYiiWaaqRtSjRBfaWGFtZCPWCjYk+ZrX5TwicNq9MQeA@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:24=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. 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.
>
> ...
>
> > +             gc->fwnode =3D parent_fwnode;
>
> Ah, this is basically reverts my commit, the whole idea of which was to g=
o
> towards constant struct gpio_chip object that is supplied by a provider.
>

Then this idea was wrong in the first place and that goal will never
be achieved. Whether that's a correct approach is questionable but
struct gpio_chip has become so much more than a simple config
structure and - given how ubiquitous GPIO providers are throughout the
different subsystems of the kernel - it'll stay that way unless we're
ready to rebuild every GPIO provider in linux.

The best we can do now is at least make its usage safe. Meaning: it's
a structure with which providers will interact using GPIOLIB callbacks
which will in turn assure that during the execution of any function
taking struct gpio_chip as argument, it will remain alive and
protected from concurrent access.

The providers however will continue to use gpio_chip for many
purposes. One of such purposes is matching the GPIO device BY its
backing gpio_chip structure. It not having the same fwnode in this
particular case is an inconsistency rather than design IMO.

I don't see any good reason for it not having the fwnode assigned.
User calling gpio_device_find() will have to jump through hoops in
order to match the device by fwnode (include gpiolib.h and dereference
gpiodev?) but it could be very easily facilitated by just assigning it
at registration-time - just like we assign a whole bunch of other
pointers and data structures.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>