diff mbox series

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

Message ID 20211130154127.12272-3-brgl@bgdev.pl
State Superseded
Headers show
Series gpio-sim: configfs-based GPIO simulator | expand

Commit Message

Bartosz Golaszewski Nov. 30, 2021, 3:41 p.m. UTC
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
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.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c      | 15 ++++++++++++++-
 include/linux/gpio/driver.h |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Nov. 30, 2021, 4:19 p.m. UTC | #1
On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:

...

> 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?

Have you seen these:
	drivers/gpio/gpio-dwapb.c
	drivers/mfd/intel_quark_i2c_gpio.c
?

GPIO driver has a main (controller level) node along with children on per bank
basis. Currently it works with the provided approach (see second driver).
Andy Shevchenko Nov. 30, 2021, 4:55 p.m. UTC | #2
On Tue, Nov 30, 2021 at 06:19:12PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> 
> ...
> 
> > 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?
> 
> Have you seen these:
> 	drivers/gpio/gpio-dwapb.c
> 	drivers/mfd/intel_quark_i2c_gpio.c
> ?
> 
> GPIO driver has a main (controller level) node along with children on per bank
> basis. Currently it works with the provided approach (see second driver).

Btw, may be helpful to debug swnodes application
https://lore.kernel.org/lkml/20210327222012.54103-3-andriy.shevchenko@linux.intel.com/#t
Bartosz Golaszewski Nov. 30, 2021, 6:32 p.m. UTC | #3
On Tue, Nov 30, 2021 at 5:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > 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?
>
> Have you seen these:
>         drivers/gpio/gpio-dwapb.c
>         drivers/mfd/intel_quark_i2c_gpio.c
> ?
>
> GPIO driver has a main (controller level) node along with children on per bank
> basis. Currently it works with the provided approach (see second driver).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Yep, I know dwapd. What happens in probe is that each bank device is
created using the properties from the associated child fwnode but the
parent device's fwnode is actually assigned as the gpiochip's fwnode.
This is logically wrong and OF doesn't do it - it assigns the child
of_node to the child device if gpio_chip->of_node is assigned in the
driver. I'm not sure if ACPI does this.

Non-OF drivers don't have a way to do this and this patch enables it.

I want to add it mostly because gpio-sim can then use the software
node to identify the device in the configfs by that software node but
IMO this is logically correct too.

Bart
Bartosz Golaszewski Nov. 30, 2021, 8:25 p.m. UTC | #4
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. 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.

Bart
Andy Shevchenko Nov. 30, 2021, 8:31 p.m. UTC | #5
On Tue, Nov 30, 2021 at 07:32:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 5:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > 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?
> >
> > Have you seen these:
> >         drivers/gpio/gpio-dwapb.c
> >         drivers/mfd/intel_quark_i2c_gpio.c
> > ?
> >
> > GPIO driver has a main (controller level) node along with children on per bank
> > basis. Currently it works with the provided approach (see second driver).

> Yep, I know dwapd. What happens in probe is that each bank device is
> created using the properties from the associated child fwnode but the
> parent device's fwnode is actually assigned as the gpiochip's fwnode.

The first device is the physical device of the GPIO controller,
the

   ,-> fwnode of the physical device
   |
  GPIO  -> portA -> gpiodev A (child fwnode A)
       `-> portB -> gpiodev B (child fwnode B)
           ...

> This is logically wrong

I don't see how, it represents hardware as is.

> and OF doesn't do it - it assigns the child
> of_node to the child device if gpio_chip->of_node is assigned in the
> driver.

And this is exactly what happens.

> I'm not sure if ACPI does this.

Depending on the device description. In the case of dwapb on Galileo platform
the ACPI just misses that, that's why board files (see second driver).

> Non-OF drivers don't have a way to do this and this patch enables it.

You meant non-FW drivers, right? How come? What am I missing?

I don't see the issue here. When user wants to have GPIO device, first we
instantiate it with its own fwnode (in your case swnode), followed by
additional (child) swnode per bank.

> I want to add it mostly because gpio-sim can then use the software
> node to identify the device in the configfs by that software node but
> IMO this is logically correct too.

I also think so.
Andy Shevchenko Nov. 30, 2021, 8:59 p.m. UTC | #6
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;


> 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).
Bartosz Golaszewski Nov. 30, 2021, 9:04 p.m. UTC | #7
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
>
>
Bartosz Golaszewski Dec. 1, 2021, 1:11 p.m. UTC | #8
On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> 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

Let me maybe rephrase the problem: currently, for GPIO devices
instantiating multiple banks created outside of the OF or ACPI
frameworks (e.g. instantiated manually and configured using a
hierarchy of software nodes with a single parent swnode and a number
of child swnodes representing the children), it is impossible to
assign firmware nodes other than the one representing the top GPIO
device to the gpiochip child devices.

In fact if we want to drop the OF APIs entirely from gpiolib - this
would be the right first step as for gpio-sim it actually replaces the
gc->of_node = some_of_node; assignment that OF-based drivers do for
sub-nodes defining banks and it does work with device-tree (I verified
that too) thanks to the fwnode abstraction layer.

Linus: Do you have anything against this change?

Bart
Bartosz Golaszewski Dec. 1, 2021, 1:53 p.m. UTC | #9
On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > 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
> >
> > Let me maybe rephrase the problem: currently, for GPIO devices
> > instantiating multiple banks created outside of the OF or ACPI
> > frameworks (e.g. instantiated manually and configured using a
> > hierarchy of software nodes with a single parent swnode and a number
> > of child swnodes representing the children), it is impossible to
> > assign firmware nodes other than the one representing the top GPIO
> > device to the gpiochip child devices.
> >
> > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > would be the right first step as for gpio-sim it actually replaces the
> > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > sub-nodes defining banks and it does work with device-tree (I verified
> > that too) thanks to the fwnode abstraction layer.
>
> I still don't see how you set up hierarchy of primary/secondary fwnodes.
>
> And I don't like this change. It seems it band-aids some issue with fwnode
> usage. What the easiest way to reproduce the issue with your series applied
> (without this change)?
>

Drop this patch and drop the line where the fwnode is assigned in
gpio-sim.c. Then probe the device and print the addresses of the
parent and child swnodes. See how they are the same and don't match
the swnode hierarchy we created. You can then apply this patch and see
how it becomes correct.

Bart
Andy Shevchenko Dec. 1, 2021, 2:28 p.m. UTC | #10
On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > instantiating multiple banks created outside of the OF or ACPI
> > > frameworks (e.g. instantiated manually and configured using a
> > > hierarchy of software nodes with a single parent swnode and a number
> > > of child swnodes representing the children), it is impossible to
> > > assign firmware nodes other than the one representing the top GPIO
> > > device to the gpiochip child devices.
> > >
> > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > would be the right first step as for gpio-sim it actually replaces the
> > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > sub-nodes defining banks and it does work with device-tree (I verified
> > > that too) thanks to the fwnode abstraction layer.
> >
> > I still don't see how you set up hierarchy of primary/secondary fwnodes.
> >
> > And I don't like this change. It seems it band-aids some issue with fwnode
> > usage. What the easiest way to reproduce the issue with your series applied
> > (without this change)?
> 
> Drop this patch and drop the line where the fwnode is assigned in
> gpio-sim.c. Then probe the device and print the addresses of the
> parent and child swnodes. See how they are the same and don't match
> the swnode hierarchy we created. You can then apply this patch and see
> how it becomes correct.

Thanks. I will give a spin.

Note, it seems I have to revert your older code first...
Andy Shevchenko Dec. 1, 2021, 2:33 p.m. UTC | #11
On Wed, Dec 01, 2021 at 04:28:19PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> ...
> 
> > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > instantiating multiple banks created outside of the OF or ACPI
> > > > frameworks (e.g. instantiated manually and configured using a
> > > > hierarchy of software nodes with a single parent swnode and a number
> > > > of child swnodes representing the children), it is impossible to
> > > > assign firmware nodes other than the one representing the top GPIO
> > > > device to the gpiochip child devices.
> > > >
> > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > would be the right first step as for gpio-sim it actually replaces the
> > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > that too) thanks to the fwnode abstraction layer.
> > >
> > > I still don't see how you set up hierarchy of primary/secondary fwnodes.
> > >
> > > And I don't like this change. It seems it band-aids some issue with fwnode
> > > usage. What the easiest way to reproduce the issue with your series applied
> > > (without this change)?
> > 
> > Drop this patch and drop the line where the fwnode is assigned in
> > gpio-sim.c. Then probe the device and print the addresses of the
> > parent and child swnodes. See how they are the same and don't match
> > the swnode hierarchy we created. You can then apply this patch and see
> > how it becomes correct.
> 
> Thanks. I will give a spin.
> 
> Note, it seems I have to revert your older code first...

Okay, I have to postpone because simple revert doesn't work for me.
Can you clean up the next, please and I can use it starting from tomorrow?


$ git tag --contains 5065e08e4ef3
DONT-USE-next-20211105
next-20211101
next-20211102
next-20211103
next-20211104
next-20211105
next-20211106
next-20211108
next-20211109
next-20211110
next-20211111
next-20211112
next-20211115
next-20211116
next-20211117
next-20211118
next-20211123
next-20211124
next-20211125
next-20211126
next-20211129
next-20211130
next-20211201
Andy Shevchenko Dec. 1, 2021, 2:54 p.m. UTC | #12
On Wed, Dec 01, 2021 at 03:36:29PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 1, 2021 at 3:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Dec 01, 2021 at 04:28:19PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > Drop this patch and drop the line where the fwnode is assigned in
> > > > gpio-sim.c. Then probe the device and print the addresses of the
> > > > parent and child swnodes. See how they are the same and don't match
> > > > the swnode hierarchy we created. You can then apply this patch and see
> > > > how it becomes correct.
> > >
> > > Thanks. I will give a spin.
> > >
> > > Note, it seems I have to revert your older code first...
> >
> > Okay, I have to postpone because simple revert doesn't work for me.
> > Can you clean up the next, please and I can use it starting from tomorrow?
> >
> >
> > $ git tag --contains 5065e08e4ef3
> > DONT-USE-next-20211105
> > next-20211101
> > next-20211102
> > next-20211103
> > next-20211104
> > next-20211105
> > next-20211106
> > next-20211108
> > next-20211109
> > next-20211110
> > next-20211111
> > next-20211112
> > next-20211115
> > next-20211116
> > next-20211117
> > next-20211118
> > next-20211123
> > next-20211124
> > next-20211125
> > next-20211126
> > next-20211129
> > next-20211130
> > next-20211201

> None of this is in next, please use:
> https://github.com/brgl/linux/tree/topic/gpio-sim-v12 if you want a
> branch.

It the old version that prevents me to apply (my branches based on next).

> I just thought you were going to simply apply these patches.

That's correct, but see above.

Anyway, I'm already switched tasks for today. Maybe I will have some time
later on.
Andy Shevchenko Dec. 2, 2021, 10:57 a.m. UTC | #13
On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> Let me maybe rephrase the problem: currently, for GPIO devices
> instantiating multiple banks created outside of the OF or ACPI
> frameworks (e.g. instantiated manually and configured using a
> hierarchy of software nodes with a single parent swnode and a number
> of child swnodes representing the children), it is impossible to
> assign firmware nodes other than the one representing the top GPIO
> device to the gpiochip child devices.
> 
> In fact if we want to drop the OF APIs entirely from gpiolib - this
> would be the right first step as for gpio-sim it actually replaces the
> gc->of_node = some_of_node; assignment that OF-based drivers do for
> sub-nodes defining banks and it does work with device-tree (I verified
> that too) thanks to the fwnode abstraction layer.

In exchange of acknowledgements I confirm that I understood the issue
you are describing. What I still don't like is this band-aid:ish approach.
What we really need is to replace of_node by fwnode in GPIO library once
for all. But it can be done later after your simulation series (or before,
i.o.w. independently), hence I propose to update TODO and do it separately.
Bartosz Golaszewski Dec. 2, 2021, 11:24 a.m. UTC | #14
On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> ...
>
> > Let me maybe rephrase the problem: currently, for GPIO devices
> > instantiating multiple banks created outside of the OF or ACPI
> > frameworks (e.g. instantiated manually and configured using a
> > hierarchy of software nodes with a single parent swnode and a number
> > of child swnodes representing the children), it is impossible to
> > assign firmware nodes other than the one representing the top GPIO
> > device to the gpiochip child devices.
> >
> > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > would be the right first step as for gpio-sim it actually replaces the
> > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > sub-nodes defining banks and it does work with device-tree (I verified
> > that too) thanks to the fwnode abstraction layer.
>
> In exchange of acknowledgements I confirm that I understood the issue
> you are describing. What I still don't like is this band-aid:ish approach.
> What we really need is to replace of_node by fwnode in GPIO library once
> for all. But it can be done later after your simulation series (or before,
> i.o.w. independently), hence I propose to update TODO and do it separately.
>

But this is what we already do for OF. How would the core gpiolib know
how the firmware nodes represent the banks? It's the driver's job to
tell the framework which node corresponds with what. If anything, we
should start replacing of_nodes with fwnodes in drivers and eventually
we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
the fwnode pointer I added as the driver still needs to assign it
itself.

Again: I may be missing something here but I've been going through
this on and on and can't figure out any other way. Looking at
gpiolib-acpi.c I don't see it correctly assigning fwnodes to
sub-devices either but I don't have any HW to test it.

As for this series: I can't really drop this patch as gpio-sim relies
on swnodes being correctly associated with gpio_chips to identify the
gpiodevs from configfs callbacks.

Bart
Andy Shevchenko Dec. 2, 2021, 11:35 a.m. UTC | #15
On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > ...
> >
> > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > instantiating multiple banks created outside of the OF or ACPI
> > > frameworks (e.g. instantiated manually and configured using a
> > > hierarchy of software nodes with a single parent swnode and a number
> > > of child swnodes representing the children), it is impossible to
> > > assign firmware nodes other than the one representing the top GPIO
> > > device to the gpiochip child devices.
> > >
> > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > would be the right first step as for gpio-sim it actually replaces the
> > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > sub-nodes defining banks and it does work with device-tree (I verified
> > > that too) thanks to the fwnode abstraction layer.
> >
> > In exchange of acknowledgements I confirm that I understood the issue
> > you are describing. What I still don't like is this band-aid:ish approach.
> > What we really need is to replace of_node by fwnode in GPIO library once
> > for all. But it can be done later after your simulation series (or before,
> > i.o.w. independently), hence I propose to update TODO and do it separately.
> >
> 
> But this is what we already do for OF. How would the core gpiolib know
> how the firmware nodes represent the banks? It's the driver's job to
> tell the framework which node corresponds with what. If anything, we
> should start replacing of_nodes with fwnodes in drivers and eventually
> we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> the fwnode pointer I added as the driver still needs to assign it
> itself.
> 
> Again: I may be missing something here but I've been going through
> this on and on and can't figure out any other way. Looking at
> gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> sub-devices either but I don't have any HW to test it.
> 
> As for this series: I can't really drop this patch as gpio-sim relies
> on swnodes being correctly associated with gpio_chips to identify the
> gpiodevs from configfs callbacks.

Then we need to replace of_node by fwnode as a first step. I have looked
briefly into the list of drivers that may have been cleaned up and it doesn't
look too long.
Andy Shevchenko Dec. 2, 2021, 11:37 a.m. UTC | #16
On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > ...
> > >
> > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > instantiating multiple banks created outside of the OF or ACPI
> > > > frameworks (e.g. instantiated manually and configured using a
> > > > hierarchy of software nodes with a single parent swnode and a number
> > > > of child swnodes representing the children), it is impossible to
> > > > assign firmware nodes other than the one representing the top GPIO
> > > > device to the gpiochip child devices.
> > > >
> > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > would be the right first step as for gpio-sim it actually replaces the
> > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > that too) thanks to the fwnode abstraction layer.
> > >
> > > In exchange of acknowledgements I confirm that I understood the issue
> > > you are describing. What I still don't like is this band-aid:ish approach.
> > > What we really need is to replace of_node by fwnode in GPIO library once
> > > for all. But it can be done later after your simulation series (or before,
> > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > >
> > 
> > But this is what we already do for OF. How would the core gpiolib know
> > how the firmware nodes represent the banks? It's the driver's job to
> > tell the framework which node corresponds with what. If anything, we
> > should start replacing of_nodes with fwnodes in drivers and eventually
> > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > the fwnode pointer I added as the driver still needs to assign it
> > itself.
> > 
> > Again: I may be missing something here but I've been going through
> > this on and on and can't figure out any other way. Looking at
> > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > sub-devices either but I don't have any HW to test it.
> > 
> > As for this series: I can't really drop this patch as gpio-sim relies
> > on swnodes being correctly associated with gpio_chips to identify the
> > gpiodevs from configfs callbacks.
> 
> Then we need to replace of_node by fwnode as a first step. I have looked
> briefly into the list of drivers that may have been cleaned up and it doesn't
> look too long.

Let me kick this off by sending couple of patches.
Bartosz Golaszewski Dec. 2, 2021, 1:06 p.m. UTC | #17
On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > ...
> > > >
> > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > of child swnodes representing the children), it is impossible to
> > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > device to the gpiochip child devices.
> > > > >
> > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > that too) thanks to the fwnode abstraction layer.
> > > >
> > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > for all. But it can be done later after your simulation series (or before,
> > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > >
> > >
> > > But this is what we already do for OF. How would the core gpiolib know
> > > how the firmware nodes represent the banks? It's the driver's job to
> > > tell the framework which node corresponds with what. If anything, we
> > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > the fwnode pointer I added as the driver still needs to assign it
> > > itself.
> > >
> > > Again: I may be missing something here but I've been going through
> > > this on and on and can't figure out any other way. Looking at
> > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > sub-devices either but I don't have any HW to test it.
> > >
> > > As for this series: I can't really drop this patch as gpio-sim relies
> > > on swnodes being correctly associated with gpio_chips to identify the
> > > gpiodevs from configfs callbacks.
> >
> > Then we need to replace of_node by fwnode as a first step. I have looked
> > briefly into the list of drivers that may have been cleaned up and it doesn't
> > look too long.
>
> Let me kick this off by sending couple of patches.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Are you fine with merging this in the meantime to get gpio-sim into mainline?

Bart
Andy Shevchenko Dec. 2, 2021, 1:44 p.m. UTC | #18
On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > of child swnodes representing the children), it is impossible to
> > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > device to the gpiochip child devices.
> > > > > >
> > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > that too) thanks to the fwnode abstraction layer.
> > > > >
> > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > for all. But it can be done later after your simulation series (or before,
> > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > >
> > > >
> > > > But this is what we already do for OF. How would the core gpiolib know
> > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > tell the framework which node corresponds with what. If anything, we
> > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > the fwnode pointer I added as the driver still needs to assign it
> > > > itself.
> > > >
> > > > Again: I may be missing something here but I've been going through
> > > > this on and on and can't figure out any other way. Looking at
> > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > sub-devices either but I don't have any HW to test it.
> > > >
> > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > gpiodevs from configfs callbacks.
> > >
> > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > look too long.
> >
> > Let me kick this off by sending couple of patches.
> 
> Are you fine with merging this in the meantime to get gpio-sim into mainline?

gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
etc) but not this patch.
Bartosz Golaszewski Dec. 2, 2021, 1:52 p.m. UTC | #19
On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > device to the gpiochip child devices.
> > > > > > >
> > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > >
> > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > >
> > > > >
> > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > tell the framework which node corresponds with what. If anything, we
> > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > itself.
> > > > >
> > > > > Again: I may be missing something here but I've been going through
> > > > > this on and on and can't figure out any other way. Looking at
> > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > sub-devices either but I don't have any HW to test it.
> > > > >
> > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > gpiodevs from configfs callbacks.
> > > >
> > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > look too long.
> > >
> > > Let me kick this off by sending couple of patches.
> >
> > Are you fine with merging this in the meantime to get gpio-sim into mainline?
>
> gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> etc) but not this patch.
>

There's no way around it though AFAIK. First - the 'gpio-line-names'
property will not work for banks. 'ngpios' will only work because we
read it manually in probe() to figure out the number of sysfs groups.
And also configfs callbacks will not be able to associate bank devices
with configfs groups. I would really like to hear an alternative -
even if it's just an idea and not actual implementation.

I'm really curious to see how you'll remove the of_node pointer and
not introduce the corresponding fwnode pointer actually.

Bart
Andy Shevchenko Dec. 2, 2021, 3:40 p.m. UTC | #20
On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > device to the gpiochip child devices.
> > > > > > > >
> > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > >
> > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > >
> > > > > >
> > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > itself.
> > > > > >
> > > > > > Again: I may be missing something here but I've been going through
> > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > sub-devices either but I don't have any HW to test it.
> > > > > >
> > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > gpiodevs from configfs callbacks.
> > > > >
> > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > look too long.
> > > >
> > > > Let me kick this off by sending couple of patches.
> > >
> > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> >
> > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > etc) but not this patch.
> >
> 
> There's no way around it though AFAIK. First - the 'gpio-line-names'
> property will not work for banks. 'ngpios' will only work because we
> read it manually in probe() to figure out the number of sysfs groups.
> And also configfs callbacks will not be able to associate bank devices
> with configfs groups. I would really like to hear an alternative -
> even if it's just an idea and not actual implementation.
> 
> I'm really curious to see how you'll remove the of_node pointer and
> not introduce the corresponding fwnode pointer actually.

Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
having of_node and fwnode at the same time in the struct gpio_chip.

Yes, we may modify this patch to work without that ugly ifdeffery and with both
in the structure, but I don't think it's a good solution.

Now clearly we have to clean up of_node first.
Bartosz Golaszewski Dec. 2, 2021, 5 p.m. UTC | #21
On Thu, Dec 2, 2021 at 4:41 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > >
> > > > > > > > ...
> > > > > > > >
> > > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > > device to the gpiochip child devices.
> > > > > > > > >
> > > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > > >
> > > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > > >
> > > > > > >
> > > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > > itself.
> > > > > > >
> > > > > > > Again: I may be missing something here but I've been going through
> > > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > > sub-devices either but I don't have any HW to test it.
> > > > > > >
> > > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > > gpiodevs from configfs callbacks.
> > > > > >
> > > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > > look too long.
> > > > >
> > > > > Let me kick this off by sending couple of patches.
> > > >
> > > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> > >
> > > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > > etc) but not this patch.
> > >
> >
> > There's no way around it though AFAIK. First - the 'gpio-line-names'
> > property will not work for banks. 'ngpios' will only work because we
> > read it manually in probe() to figure out the number of sysfs groups.
> > And also configfs callbacks will not be able to associate bank devices
> > with configfs groups. I would really like to hear an alternative -
> > even if it's just an idea and not actual implementation.
> >
> > I'm really curious to see how you'll remove the of_node pointer and
> > not introduce the corresponding fwnode pointer actually.
>
> Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
> having of_node and fwnode at the same time in the struct gpio_chip.
>
> Yes, we may modify this patch to work without that ugly ifdeffery and with both
> in the structure, but I don't think it's a good solution.
>

It may not be the best solution but we can't simply convert all the
drivers to fwnode and pray they work. I would like every converted
driver to be well tested because there can be some issues lurking in
the fwnode <-> of_node conversion. That will take time.

Meanwhile, this would block gpio-sim for months again. I don't believe
this patch is wrong as it fixes a real issue and as you said: fwnode
will most likely stay in gpio_chip.

IMO we should introduce fwnode, convert gpiolib and drivers to using
it gradually, remove of_node once there are no more users.

Bart

> Now clearly we have to clean up of_node first.
>
Andy Shevchenko Dec. 2, 2021, 5:29 p.m. UTC | #22
On Thu, Dec 02, 2021 at 06:00:05PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 4:41 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > > > device to the gpiochip child devices.
> > > > > > > > > >
> > > > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > > > >
> > > > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > > > >
> > > > > > > >
> > > > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > > > itself.
> > > > > > > >
> > > > > > > > Again: I may be missing something here but I've been going through
> > > > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > > > sub-devices either but I don't have any HW to test it.
> > > > > > > >
> > > > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > > > gpiodevs from configfs callbacks.
> > > > > > >
> > > > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > > > look too long.
> > > > > >
> > > > > > Let me kick this off by sending couple of patches.
> > > > >
> > > > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> > > >
> > > > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > > > etc) but not this patch.
> > > >
> > >
> > > There's no way around it though AFAIK. First - the 'gpio-line-names'
> > > property will not work for banks. 'ngpios' will only work because we
> > > read it manually in probe() to figure out the number of sysfs groups.
> > > And also configfs callbacks will not be able to associate bank devices
> > > with configfs groups. I would really like to hear an alternative -
> > > even if it's just an idea and not actual implementation.
> > >
> > > I'm really curious to see how you'll remove the of_node pointer and
> > > not introduce the corresponding fwnode pointer actually.
> >
> > Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
> > having of_node and fwnode at the same time in the struct gpio_chip.
> >
> > Yes, we may modify this patch to work without that ugly ifdeffery and with both
> > in the structure, but I don't think it's a good solution.
> >
> 
> It may not be the best solution but we can't simply convert all the
> drivers to fwnode and pray they work. I would like every converted
> driver to be well tested because there can be some issues lurking in
> the fwnode <-> of_node conversion. That will take time.

> Meanwhile, this would block gpio-sim for months again. I don't believe
> this patch is wrong as it fixes a real issue and as you said: fwnode
> will most likely stay in gpio_chip.o

It doesn't strictly speaking "fix". But it allows to get things right.

> IMO we should introduce fwnode, convert gpiolib and drivers to using
> it gradually, remove of_node once there are no more users.

I may accept the change after some amendments done:
 - get rid of ifdeffery (remove that block completely)
 - add TODO entry
 - add "deprecated" keyword to of_node
 - ...I hope I haven't miss anything else...

> > Now clearly we have to clean up of_node first.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 22b98a590a88..2a877b4ccdee 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -593,13 +593,26 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *lock_key,
 			       struct lock_class_key *request_key)
 {
-	struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
+	struct fwnode_handle *fwnode = NULL;
 	unsigned long	flags;
 	int		ret = 0;
 	unsigned	i;
 	int		base = gc->base;
 	struct gpio_device *gdev;
 
+#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 */
+
+	if (gc->fwnode)
+		fwnode = gc->fwnode;
+	else if (gc->parent)
+		fwnode = dev_fwnode(gc->parent);
+
 	/*
 	 * First: allocate and populate the internal stat container, and
 	 * set up the struct device.
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a673a359e20b..b0728c8ad90c 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -289,6 +289,7 @@  struct gpio_irq_chip {
  *	number or the name of the SoC IP-block implementing it.
  * @gpiodev: the internal state holder, opaque struct
  * @parent: optional parent device providing the GPIOs
+ * @fwnode: optional fwnode providing this controller's properties
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @request: optional hook for chip-specific activation, such as
  *	enabling module power and clock; may sleep
@@ -377,6 +378,7 @@  struct gpio_chip {
 	const char		*label;
 	struct gpio_device	*gpiodev;
 	struct device		*parent;
+	struct fwnode_handle	*fwnode;
 	struct module		*owner;
 
 	int			(*request)(struct gpio_chip *gc,