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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

@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@xxxxxxxxx> 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