Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Kent Gibson <warthog618@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Shuah Khan <shuah@kernel.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip
Date: Tue, 30 Nov 2021 22:04:27 +0100	[thread overview]
Message-ID: <CAMRc=Me=Oq_V=+p-AFPcyDjBs-+4Ug3k0AWK9fdEEet2JD3eFw@mail.gmail.com> (raw)
In-Reply-To: <YaaQp2rq7N71dm1l@smile.fi.intel.com>

On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > > Software nodes allow us to represent hierarchies for device components
> > > > that don't have their struct device representation yet - for instance:
> > > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> > >
> > > core .. core ?!
> > >
> > > > however doesn't offer any way of passing this information from the
> > > > drivers.
> > > >
> > > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > > by the driver and used to pass device properties for child nodes.
> > > >
> > > > This is similar to how we handle device-tree sub-nodes with
> > > > CONFIG_OF_GPIO enabled.
> > >
> > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > examples?
> > >
> > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > need to solve this way?
> > >
> > > ...
> > >
> > > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > > +     if (gc->of_node && gc->fwnode) {
> > > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > > +                    __func__);
> > > > +             return -EINVAL;
> > > > +     }
> > > > +#endif /* CONFIG_OF_GPIO */
> > >
> > > I don't like this. It seems like a hack right now.
> > >
> > > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > > rather than doing this? (I believe in most of the drivers we can drop
> > > completely the of_node assignment).
> > >
> >
> > Yes, it's definitely a good idea but I would be careful with just
> > dropping the of_node assignments as callbacks may depend on them
> > later.
>
> GPIO library does it for us among these lines:
>
>         struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
>
>         of_gpio_dev_init(gc, gdev); <<< HERE!
>         acpi_gpio_dev_init(gc, gdev);
>
>         /*
>          * Assign fwnode depending on the result of the previous calls,
>          * if none of them succeed, assign it to the parent's one.
>          */
>         gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>

Except that it doesn't and I noticed that when working on the
subsequent patch. The child gpiochipX devices all had the parent's
fwnode assigned as their primary fwnode and no secondary fwnode.

Note that this driver doesn't use neither OF nor ACPI in which case
gdev->dev has no fwnode and the parent's one is used. This patch
addresses it. If you have a better idea, let me know.

Bart

>
> > Also it's not just about the gpio_chip of_node assignment -
> > drivers also use a bunch of OF APIs all around the place. I would
> > prefer that it be done one by one and every modified driver be tested.
>
> That's why we want to eliminate dev->fwnode explicit dereference as a first
> step (see dev_fwnode() / device_set_node() APIs).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>