Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Linus Walleij]

On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:

>> That's correct. However using con_id to pass this results in different
>> behavior across DT and ACPI. A better way is to export the labeling
>> function so consumers can set meaningful labels themselves.
>
> But this code is the consumer of those GPIOs. IF the parameter to
> devm_gpiod_get_index() isn't intended to be used, why does it exist?

Kerneldoc says:

/**
 * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
 * @dev:        GPIO consumer, can be NULL for system-global GPIOs
 * @con_id:     function within the GPIO consumer
 * @idx:        index of the GPIO to obtain in the consumer
 *

Basically it is just exposing the fact that of_find_gpio() and
acpi_find_gpio() both take a con_id as argument.

If we drill into this, we find that it is used to conjure the
arbitrary string before the gpios in the DT case, like:

foo-gpios = <...>;

As in tegra30-beaver.dts...

    sdhci@78000000 {
            status = "okay";
            cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
            wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
            power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
            bus-width = <4>;
    };

Instead of passing the GPIOs as index 0,1,2 they are named
and I do admit this has a nice "things are under control" aspect
to it.

In the ACPI case the con_id is not used for anything.

So it is basically there to satisfy the habit in some device
tree bindings to name gpio arrays instead of just passing gpios = <...>;
(The latter should be encouraged going forward.)

As DT is ABI we need to keep this forever, and driver may need to
look for foo-gpios=<> and gpios=<> alike for backward compatibility
if we'd change it, sweet isn't it? :-)

I don't quite like this, as it is adding stupid nonsens arguments for
ACPI GPIO producers (which only take an index AFAICT), but it is a
first sacrifice on the altar of trying to mask the differences between DT
and ACPI probe paths about which I have mixed feelings.

Yours,
LInus Walleij

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Stephen Warren]

On 02/25/2014 02:13 AM, Linus Walleij wrote:
> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
> 
>>> That's correct. However using con_id to pass this results in different
>>> behavior across DT and ACPI. A better way is to export the labeling
>>> function so consumers can set meaningful labels themselves.
...
> As in tegra30-beaver.dts...
> 
>     sdhci@78000000 {
>             status = "okay";
>             cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
>             wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
>             power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
>             bus-width = <4>;
>     };
> 
> Instead of passing the GPIOs as index 0,1,2 they are named
> and I do admit this has a nice "things are under control" aspect
> to it.
> 
> In the ACPI case the con_id is not used for anything.
> 
> So it is basically there to satisfy the habit in some device
> tree bindings to name gpio arrays instead of just passing gpios = <...>;
> (The latter should be encouraged going forward.)

Do you really want to switch from named GPIO lookups to index-based GPIO
lookups? Index-based lookups make it much harder to extend the DT
binding in a backwards-compatible fashion, especially in the face of
optional GPIOs (of which all of CD, WP, power are).

If we switch to a single gpios property, I'd assert we should still do
named-based lookups using a parallel gpio-names property, just like most
(all?) other resource types now support. If we do that, we'll still need
the name parameter.

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Linus Walleij]

On Wed, Feb 26, 2014 at 1:35 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/25/2014 02:13 AM, Linus Walleij wrote:

>> So it is basically there to satisfy the habit in some device
>> tree bindings to name gpio arrays instead of just passing gpios = <...>;
>> (The latter should be encouraged going forward.)
>
> Do you really want to switch from named GPIO lookups to index-based GPIO
> lookups?

I don't know what I want. I only know that ACPI and DT people
are starting to step on each others feet.

> Index-based lookups make it much harder to extend the DT
> binding in a backwards-compatible fashion, especially in the face of
> optional GPIOs (of which all of CD, WP, power are).

You're probably right. At the same time it makes the world complex
for the ambition to use the same interface for DT and ACPI alike.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Gross, Mark]

Please know that no one should not consider me an authority on ACPI at this time.  But, I have some comments / context / thoughts below.

Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client.  Folks can use mgross@linux.intel.com to avoid outlook-isms from me in the future.

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Tuesday, February 25, 2014 1:14 AM
> To: Stephen Warren; Alexandre Courbot; Grant Likely;
> devicetree@vger.kernel.org
> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
> 
> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
> <swarren@wwwdotorg.org> wrote:
> > On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
> 
> >> That's correct. However using con_id to pass this results in
> >> different behavior across DT and ACPI. A better way is to export the
> >> labeling function so consumers can set meaningful labels themselves.
> >
> > But this code is the consumer of those GPIOs. IF the parameter to
> > devm_gpiod_get_index() isn't intended to be used, why does it exist?
> 
> Kerneldoc says:
> 
> /**
>  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
>  * @dev:        GPIO consumer, can be NULL for system-global GPIOs
>  * @con_id:     function within the GPIO consumer
>  * @idx:        index of the GPIO to obtain in the consumer
>  *
> 
> Basically it is just exposing the fact that of_find_gpio() and
> acpi_find_gpio() both take a con_id as argument.
> 
> If we drill into this, we find that it is used to conjure the arbitrary string
> before the gpios in the DT case, like:
> 
> foo-gpios = <...>;
> 
> As in tegra30-beaver.dts...
> 
>     sdhci@78000000 {
>             status = "okay";
>             cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
>             wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
>             power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
>             bus-width = <4>;
>     };
> 
> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
> this has a nice "things are under control" aspect to it.
[Gross, Mark] FWIW I don't think this is as "under control" as you do.  Those names in the above sdhci example are derived from a specific SDHCI tegra spec sheet or schematic.  Those names likely come from the data sheet for the controller.  I would like SDHCI kernel drivers to work pretty much the same for all compatible controllers.  The set of compatible controllers have spec sheets that use different naming conventions for their pins and thus a name space pollution of the SDHCI enumeration ABI will be a problem.

> 
> In the ACPI case the con_id is not used for anything.
> 
> So it is basically there to satisfy the habit in some device tree bindings to
> name gpio arrays instead of just passing gpios = <...>; (The latter should be
> encouraged going forward.)
[Gross, Mark] This is in fact just an idiom of the ACPI ABI inherited from writing linux drivers that work on systems with BIOS/FW developed for MS-Windows.  For the devices I work on we have a number of driver teams filing bugs against the BIOS/FW to add named GPIO objects to device name spaces such that they can directly query for the GPIO their driver needs and avoid assuming the 3rd GPIO is the special one they need for whatever...

So if you have the luxury of being able to influence (file bugs against or write) the platform enumeration ABI then with ACPI you can have a named gpio today.  Note: there is an expectation that the _PRP feature to go into the next version of ACPI and that should enable a trivial 1-1 mapping (when _prp is used by the ACPI platform) between the different platform enumeration API's (DT and ACPI).

Still, from a platform enumeration ABI point of view I see the arbitrariness of indexes not much worse than the arbitrary naming of pins off of spec sheets or schematics.  Given that the authors of those specs/layouts come up with a new naming schemes every time they log into their workstations (especially for the schematics).  I do admit names are a little better but still have the scaling issue WRT namespaces and arbitrary naming by HW engineers over time.

> 
> As DT is ABI we need to keep this forever, and driver may need to look for
> foo-gpios=<> and gpios=<> alike for backward compatibility if we'd change it,
> sweet isn't it? :-)
> 
> I don't quite like this, as it is adding stupid nonsens arguments for ACPI GPIO
> producers (which only take an index AFAICT), but it is a first sacrifice on the
> altar of trying to mask the differences between DT and ACPI probe paths
> about which I have mixed feelings.
[Gross, Mark] I don't have mixed feelings.  I want to see converged probe paths that can handle both ACPI and DT platform ABI's seamlessly so we can reuse drivers across architectures effectively.  I think the _PRP will help with that even though its use assumes you have influence over the platform FW and this will not help with supporting of legacy BIOS's will burden us with gpio index assumptions.

--mark

> 
> Yours,
> LInus Walleij

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Stephen Warren]

On 02/27/2014 10:38 AM, Gross, Mark wrote:
> Please know that no one should not consider me an authority on ACPI at this time.  But, I have some comments / context / thoughts below.
> 
> Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client.  Folks can use mgross@linux.intel.com to avoid outlook-isms from me in the future.
> 
>> -----Original Message-----
>> From: Linus Walleij [mailto:linus.walleij@linaro.org]
>> Sent: Tuesday, February 25, 2014 1:14 AM
>> To: Stephen Warren; Alexandre Courbot; Grant Likely;
>> devicetree@vger.kernel.org
>> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
>> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
>> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
>>
>> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
>> <swarren@wwwdotorg.org> wrote:
>>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
>>
>>>> That's correct. However using con_id to pass this results in
>>>> different behavior across DT and ACPI. A better way is to export the
>>>> labeling function so consumers can set meaningful labels themselves.
>>>
>>> But this code is the consumer of those GPIOs. IF the parameter to
>>> devm_gpiod_get_index() isn't intended to be used, why does it exist?
>>
>> Kerneldoc says:
>>
>> /**
>>  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
>>  * @dev:        GPIO consumer, can be NULL for system-global GPIOs
>>  * @con_id:     function within the GPIO consumer
>>  * @idx:        index of the GPIO to obtain in the consumer
>>  *
>>
>> Basically it is just exposing the fact that of_find_gpio() and
>> acpi_find_gpio() both take a con_id as argument.
>>
>> If we drill into this, we find that it is used to conjure the arbitrary string
>> before the gpios in the DT case, like:
>>
>> foo-gpios = <...>;
>>
>> As in tegra30-beaver.dts...
>>
>>     sdhci@78000000 {
>>             status = "okay";
>>             cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
>>             wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
>>             power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
>>             bus-width = <4>;
>>     };
>>
>> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
>> this has a nice "things are under control" aspect to it.
>
> [Gross, Mark] FWIW I don't think this is as "under control" as you do.  Those
> names in the above sdhci example are derived from a specific SDHCI
tegra spec
> sheet or schematic.  Those names likely come from the data sheet for
> the controller.

The names of the properties are fixed and defined by the DT binding for
the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they
will be the same in every DT file that uses that Tegra SDHCI compatible
value (the compatible property isn't show above, because the above
fragment is a board.dts file, and the compatible value gets inherited
from the soc.dtsi file). There won't be any variation at all,
irrespective of what signal names exist in a particular board schematic.

If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI
controller, I would hope it would use the exact same names for the GPIO
signals.

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[mark gross]

On Thu, Feb 27, 2014 at 10:47:58AM -0700, Stephen Warren wrote:
> On 02/27/2014 10:38 AM, Gross, Mark wrote:
> > Please know that no one should not consider me an authority on ACPI at this
> > time.  But, I have some comments / context / thoughts below.
> > 
> > Also I apologize in advance for any email formatting issues caused by
> > replying to this via my work exchange account / outlook client.  Folks can
> > use mgross@linux.intel.com to avoid outlook-isms from me in the future.
> > 
> >> -----Original Message-----
> >> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> >> Sent: Tuesday, February 25, 2014 1:14 AM
> >> To: Stephen Warren; Alexandre Courbot; Grant Likely;
> >> devicetree@vger.kernel.org
> >> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
> >> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
> >> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
> >>
> >> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
> >> <swarren@wwwdotorg.org> wrote:
> >>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
> >>
> >>>> That's correct. However using con_id to pass this results in
> >>>> different behavior across DT and ACPI. A better way is to export the
> >>>> labeling function so consumers can set meaningful labels themselves.
> >>>
> >>> But this code is the consumer of those GPIOs. IF the parameter to
> >>> devm_gpiod_get_index() isn't intended to be used, why does it exist?
> >>
> >> Kerneldoc says:
> >>
> >> /**
> >>  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> >>  * @dev:        GPIO consumer, can be NULL for system-global GPIOs
> >>  * @con_id:     function within the GPIO consumer
> >>  * @idx:        index of the GPIO to obtain in the consumer
> >>  *
> >>
> >> Basically it is just exposing the fact that of_find_gpio() and
> >> acpi_find_gpio() both take a con_id as argument.
> >>
> >> If we drill into this, we find that it is used to conjure the arbitrary string
> >> before the gpios in the DT case, like:
> >>
> >> foo-gpios = <...>;
> >>
> >> As in tegra30-beaver.dts...
> >>
> >>     sdhci@78000000 {
> >>             status = "okay";
> >>             cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
> >>             wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
> >>             power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
> >>             bus-width = <4>;
> >>     };
> >>
> >> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
> >> this has a nice "things are under control" aspect to it.
> >
> > [Gross, Mark] FWIW I don't think this is as "under control" as you do.  Those
> > names in the above sdhci example are derived from a specific SDHCI
> tegra spec
> > sheet or schematic.  Those names likely come from the data sheet for
> > the controller.
> 
> The names of the properties are fixed and defined by the DT binding for
> the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they
> will be the same in every DT file that uses that Tegra SDHCI compatible
> value (the compatible property isn't show above, because the above
> fragment is a board.dts file, and the compatible value gets inherited
> from the soc.dtsi file). There won't be any variation at all,
> irrespective of what signal names exist in a particular board schematic.
> 
> If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI
> controller, I would hope it would use the exact same names for the GPIO
> signals.

me to!
--mark

Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names

[Linus Walleij]

On Fri, Feb 28, 2014 at 1:38 AM, Gross, Mark <mark.gross@intel.com> wrote:

> So if you have the luxury of being able to influence (file bugs against or write)
> the platform enumeration ABI then with ACPI you can have a named gpio today.
> Note: there is an expectation that the _PRP feature to go into the next version
> of ACPI and that should enable a trivial 1-1 mapping (when _prp is used by the
> ACPI platform) between the different platform enumeration API's (DT and ACPI).

Oh I wasn't aware. This makes things better, and gives con_id a clear usecase
in the ACPI probe path as well.

[Stephen]
>> If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI
>> controller, I would hope it would use the exact same names for the GPIO
>> signals.
>
> me to!

Amen to that.

Yours,
Linus Walleij