[v1,2/4] gpio: dwapb: Read GPIO base from gpio-base property

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()
Related show

Commit Message

Andy Shevchenko July 26, 2021, 12:54 p.m.
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(-)

Comments

Serge Semin Aug. 2, 2021, 1:58 p.m. | #1
+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

>
Andy Shevchenko Aug. 2, 2021, 3:52 p.m. | #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
Serge Semin Aug. 4, 2021, 12:44 p.m. | #3
@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
Andy Shevchenko Aug. 4, 2021, 2:43 p.m. | #4
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
Linus Walleij Aug. 11, 2021, 12:37 p.m. | #5
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
Linus Walleij Aug. 11, 2021, 12:40 p.m. | #6
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
Serge Semin Aug. 11, 2021, 12:47 p.m. | #7
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
Andy Shevchenko Aug. 11, 2021, 1:11 p.m. | #8
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!

Patch

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