| Message ID | 20210726125436.58685-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs() | expand |
+Cc Rob On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote: > For backward compatibility with some legacy devices introduce > a new (*) property gpio-base to read GPIO base. This will allow > further cleanup of the driver. > > *) Note, it's not new for GPIO library since mockup driver is > using it already. You are right but I don't think it's a good idea to advertise the pure Linux-internal property "gpio-base" to any use-case like OF and ACPI FW nodes. Especially seeing we don't have it described in the DT-bindings and noting that the mockup driver is dedicated for the GPIO tests only. What about restricting the property usage for the SW-nodes only by adding an additional check: is_software_node() here? @Linus, @Bartosz, @Rob, what do you think about that? -Sergey > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-dwapb.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index f6ae69d5d644..e3011d4e17b0 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -581,7 +581,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) > pp->ngpio = DWAPB_MAX_GPIOS; > } > > - pp->gpio_base = -1; > + if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base)) > + pp->gpio_base = -1; > > /* > * Only port A can provide interrupts in all configurations of > -- > 2.30.2 >
On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote: > On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote: > > For backward compatibility with some legacy devices introduce > > a new (*) property gpio-base to read GPIO base. This will allow > > further cleanup of the driver. Thanks for the review! My answers below. > > *) Note, it's not new for GPIO library since mockup driver is > > using it already. > > You are right but I don't think it's a good idea to advertise the > pure Linux-internal property "gpio-base" to any use-case like OF > and ACPI FW nodes. I don't want to advertise them, actually (that's why no bindings are modified). Perhaps introducing a paragraph in the GPIO documentation about this (and / or in GPIO generic bindings) that gpio-base property is solely for internal use and should't be used in actual DTs? > Especially seeing we don't have it described in the > DT-bindings and noting that the mockup driver is dedicated for the > GPIO tests only. What about restricting the property usage for the > SW-nodes only by adding an additional check: is_software_node() here? I don't think we need this. But if you think it's better this way just to avoid usage of this property outside of internal properties, I'm fine to add. Perhaps we may issue a warning and continue? (see also above) > @Linus, @Bartosz, @Rob, what do you think about that? -- With Best Regards, Andy Shevchenko
@Linus, @Bartosz, @Rob, could you join the discussion regarding the gpio-base property usage? On Mon, Aug 02, 2021 at 06:52:28PM +0300, Andy Shevchenko wrote: > On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote: > > > For backward compatibility with some legacy devices introduce > > > a new (*) property gpio-base to read GPIO base. This will allow > > > further cleanup of the driver. > > Thanks for the review! My answers below. > > > > *) Note, it's not new for GPIO library since mockup driver is > > > using it already. > > > > You are right but I don't think it's a good idea to advertise the > > pure Linux-internal property "gpio-base" to any use-case like OF > > and ACPI FW nodes. > > I don't want to advertise them, actually (that's why no bindings are > modified). Perhaps introducing a paragraph in the GPIO documentation > about this (and / or in GPIO generic bindings) that gpio-base property > is solely for internal use and should't be used in actual DTs? It might have been not that clear but by "advertising" I meant to have the property generically handled in the driver, thus permitting it being specified not only via the SW-nodes but also via the ACPI and OF firmware. (Please see my next comment for more details.) Regarding adding the gpio-base property documentation. I am pretty sure it shouldn't be mentioned neither in the DW APB GPIO bindings, nor in any other GPIO device DT-bindings because as you are right saying it is the solely Linux kernel-specific parameter and isn't supposed to be part of the device tree specification. On the other hand if it gets to be frequently used then indeed we need to somehow have it described and of course make sure it isn't used inappropriately. Thus a possible option of documenting the property would be just adding a new paragraph/file somewhere in Documentation/driver-api/gpio/ since the property name implies that it's going to be generic and permitted to be specified for all GPIO-chips. Though it's for @Linus and @Bartosz to decide after all. > > > Especially seeing we don't have it described in the > > DT-bindings and noting that the mockup driver is dedicated for the > > GPIO tests only. What about restricting the property usage for the > > SW-nodes only by adding an additional check: is_software_node() here? > > I don't think we need this. But if you think it's better this way just > to avoid usage of this property outside of internal properties, I'm > fine to add. Perhaps we may issue a warning and continue? (see also > above) In my opinion it's very required and here is why. Adding the generic gpio-base property support into the driver basically means saying: "Hey, the driver supports it, so you can add it to your firmware." Even if the property isn't described in the bindings, the platform developers will be able to use it in new DTS-files since it's much easier to add a property into a DT-file and make things working than to convert the drivers/platforms/apps to using the GPIOd API. In case if maintainers aren't that careful at review such dts may get slip into the kernel, which in its turn will de facto make the property being part of the DT specification and will need to be supported. That is we must be very careful in what properties are permitted in the driver. Thus, yes, I think we need to make sure here that the property is only used in framework of the kernel and isn't passed via inappropriate paths like DT/ACPI fw so not to get into the maintainability troubles in future. Issuing a warning but accepting the property isn't good alternative due to the same reason. Why do we need to add the DT/ACPI property support, which isn't supposed to be used like that instead of just restricting the usecases beforehand? So I vote for parsing the "gpio-base" property only if it's passed as a part of the SW-node. -Sergey > > > @Linus, @Bartosz, @Rob, what do you think about that? > > -- > With Best Regards, > Andy Shevchenko
On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote: > On Mon, Aug 02, 2021 at 06:52:28PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote: > > > > For backward compatibility with some legacy devices introduce > > > > a new (*) property gpio-base to read GPIO base. This will allow > > > > further cleanup of the driver. > > > > Thanks for the review! My answers below. > > > > > > *) Note, it's not new for GPIO library since mockup driver is > > > > using it already. > > > > > > You are right but I don't think it's a good idea to advertise the > > > pure Linux-internal property "gpio-base" to any use-case like OF > > > and ACPI FW nodes. > > > I don't want to advertise them, actually (that's why no bindings are > > modified). Perhaps introducing a paragraph in the GPIO documentation > > about this (and / or in GPIO generic bindings) that gpio-base property > > is solely for internal use and should't be used in actual DTs? > > It might have been not that clear but by "advertising" I meant to have > the property generically handled in the driver, thus permitting it > being specified not only via the SW-nodes but also via the ACPI > and OF firmware. (Please see my next comment for more details.) > > Regarding adding the gpio-base property documentation. I am pretty > sure it shouldn't be mentioned neither in the DW APB GPIO bindings, > nor in any other GPIO device DT-bindings because as you are right > saying it is the solely Linux kernel-specific parameter and isn't > supposed to be part of the device tree specification. On the other > hand if it gets to be frequently used then indeed we need to somehow > have it described and of course make sure it isn't used > inappropriately. Thus a possible option of documenting the property > would be just adding a new paragraph/file somewhere in > Documentation/driver-api/gpio/ since the property name implies that > it's going to be generic and permitted to be specified for all > GPIO-chips. Though it's for @Linus and @Bartosz to decide after all. Thanks for elaborative point. > > > Especially seeing we don't have it described in the > > > DT-bindings and noting that the mockup driver is dedicated for the > > > GPIO tests only. What about restricting the property usage for the > > > SW-nodes only by adding an additional check: is_software_node() here? > > > I don't think we need this. But if you think it's better this way just > > to avoid usage of this property outside of internal properties, I'm > > fine to add. Perhaps we may issue a warning and continue? (see also > > above) > > In my opinion it's very required and here is why. Adding the generic > gpio-base property support into the driver basically means saying: > "Hey, the driver supports it, so you can add it to your firmware." > Even if the property isn't described in the bindings, the platform > developers will be able to use it in new DTS-files since it's much > easier to add a property into a DT-file and make things working than > to convert the drivers/platforms/apps to using the GPIOd API. In case > if maintainers aren't that careful at review such dts may get slip > into the kernel, which in its turn will de facto make the property > being part of the DT specification and will need to be supported. That > is we must be very careful in what properties are permitted in the > driver. Thus, yes, I think we need to make sure here that the property > is only used in framework of the kernel and isn't passed via > inappropriate paths like DT/ACPI fw so not to get into the > maintainability troubles in future. Got it. I'll add the additional check in next version. > Issuing a warning but accepting the property isn't good alternative > due to the same reason. Why do we need to add the DT/ACPI property > support, which isn't supposed to be used like that instead of just > restricting the usecases beforehand? So I vote for parsing the > "gpio-base" property only if it's passed as a part of the SW-node. -- With Best Regards, Andy Shevchenko
On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > For backward compatibility with some legacy devices introduce > a new (*) property gpio-base to read GPIO base. This will allow > further cleanup of the driver. > > *) Note, it's not new for GPIO library since mockup driver is > using it already. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> (...) > - pp->gpio_base = -1; > + if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base)) > + pp->gpio_base = -1; This is problematic because we have repeatedly NACKed this property to be added to device trees. I don't know about fwnode policies, but in the device tree this would have to be "linux,gpio-base" and then it would be NACKed because of adding an operating-system specific thing to a OS-independent hardware description. I don't know what to do with this really, but I understand the need of it as a kernel-internal thing, however I am afraid that adding this will make it possible to add linux,gpio-base = <n> to any device tree gpio_chip as well and that encourages bad behaviour even if we don't allow a DT binding (YAML) like that. Is there a way to make a fwnode property only come from software nodes and not allowed to be used in ACPI or DT? (I guess not...) Yours, Linus Walleij
On Wed, Aug 4, 2021 at 4:44 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote: > > Thus, yes, I think we need to make sure here that the property > > is only used in framework of the kernel and isn't passed via > > inappropriate paths like DT/ACPI fw so not to get into the > > maintainability troubles in future. > > Got it. I'll add the additional check in next version. This seems reasonable for me, if you can get this done with some kind of elegance. Maybe use the "linux,gpio-base" property as mentioned so it is clear that this is a Linux-internal thing only. Yours, Linus Walleij
Hello Linus On Wed, Aug 11, 2021 at 02:40:49PM +0200, Linus Walleij wrote: > On Wed, Aug 4, 2021 at 4:44 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote: > > > Thus, yes, I think we need to make sure here that the property > > > is only used in framework of the kernel and isn't passed via > > > inappropriate paths like DT/ACPI fw so not to get into the > > > maintainability troubles in future. > > > > Got it. I'll add the additional check in next version. > > This seems reasonable for me, if you can get this done with > some kind of elegance. > There is v2 of this series has already been posted: https://lore.kernel.org/linux-gpio/20210804160019.77105-1-andriy.shevchenko@linux.intel.com/ with the denoted concern taken into account. -Sergey > Maybe use the "linux,gpio-base" property as mentioned so it is > clear that this is a Linux-internal thing only. > > Yours, > Linus Walleij
On Wed, Aug 11, 2021 at 02:37:42PM +0200, Linus Walleij wrote: > On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > For backward compatibility with some legacy devices introduce > > a new (*) property gpio-base to read GPIO base. This will allow > > further cleanup of the driver. > > > > *) Note, it's not new for GPIO library since mockup driver is > > using it already. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > (...) > > - pp->gpio_base = -1; > > + if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base)) > > + pp->gpio_base = -1; > > This is problematic because we have repeatedly NACKed this property > to be added to device trees. > > I don't know about fwnode policies, but in the device tree this would have > to be "linux,gpio-base" and then it would be NACKed because of adding > an operating-system specific thing to a OS-independent hardware > description. > > I don't know what to do with this really, but I understand the need of it > as a kernel-internal thing, however I am afraid that adding this will make > it possible to add linux,gpio-base = <n> to any device tree gpio_chip > as well and that encourages bad behaviour even if we don't allow a > DT binding (YAML) like that. > > Is there a way to make a fwnode property only come from software > nodes and not allowed to be used in ACPI or DT? (I guess not...) This has been the very same concern by Serge and we agreed on limiting this to software nodes only. And I have seen you are fine with the approach, thanks!
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index f6ae69d5d644..e3011d4e17b0 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -581,7 +581,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) pp->ngpio = DWAPB_MAX_GPIOS; } - pp->gpio_base = -1; + if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base)) + pp->gpio_base = -1; /* * Only port A can provide interrupts in all configurations of
For backward compatibility with some legacy devices introduce a new (*) property gpio-base to read GPIO base. This will allow further cleanup of the driver. *) Note, it's not new for GPIO library since mockup driver is using it already. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-dwapb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)