GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Alexandre Courbot]

On Mon, Oct 20, 2014 at 11:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 20 October 2014 15:12:50 Alexandre Courbot wrote:
>> On Sat, Oct 18, 2014 at 6:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 17 October 2014 20:09:51 Arnd Bergmann wrote:
>> >> On October 17, 2014 2:16:00 PM CEST, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>> >> >From: Mika Westerberg <mika.westerberg@linux.intel.com>
>> >> >
>> >> >Some drivers need to deal with only firmware representation of its
>> >> >GPIOs. An example would be a GPIO button array driver where each button
>> >> >is described as a separate firmware node in device tree. Typically
>> >> >these
>> >> >child nodes do not have physical representation in the Linux device
>> >> >model.
>> >> >
>> >> >In order to help device drivers to handle such firmware child nodes we
>> >> >add dev[m]_get_named_gpiod_from_child() that takes a child firmware
>> >> >node pointer as its second argument (the first one is the parent device
>> >> >itself), finds the GPIO using whatever is the underlying firmware
>> >> >method, and requests the GPIO properly.
>> >>
>> >> Could we also have a wrapper around this function without a "name" argument,
>> >> using just the index?
>> >
>> > Expanding on this thought: I think we should mandate for new bindings
>> > that they use either a name and no index, or an index but not name,
>>
>> I'm afraid this could forbid some useful use-cases, namely the ones
>> where several GPIOs serve the same function (and are typically set
>> together). We had a few patch proposals to handle such GPIO groups,
>> and even though one was in pretty good shape the submitter did not
>> push it until the end. :/
>>
>> But my concern is that instead of having this:
>>
>> enable-gpio = <&gpio 0 GPIO_ACTIVE_HIGH>;
>> value-gpios = <&gpio 1 GPIO_ACTIVE_HIGH ... &gpio 8 GPIO_ACTIVE_HIGH>;
>>
>> We would force this:
>>
>> enable-gpio = <&gpio 0 GPIO_ACTIVE_HIGH>;
>> value0-gpio = <&gpio 1 GPIO_ACTIVE_HIGH>;
>> ...
>> value7-gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
>>
>> Or this:
>>
>> // First GPIO is enable, other GPIOs are value
>> gpios = <&gpio 0 GPIO_ACTIVE_HIGH &gpio 1 GPIO_ACTIVE_HIGH ... &gpio 8
>> GPIO_ACTIVE_HIGH>;
>>
>> Most bindings don't need that much sophistication, and for these we
>> should indeed make sure that they stick to using either the names or
>> index (and in a consistent manner), but closing the possibility to use
>> both together may bite us in the end.
>
> I would actually prefer the single-property case here, but I see your
> point. Could we make it a strong suggestion rather than a mandatory
> requirement for new bindings then?

Definitely, and a very strong suggestion even. Having to use both
names *and* index in GPIO properties should not be needed 99% of the
time.

>
>> > and I also think that for named gpios, we should try to converge on a
>> > common naming scheme. As discussed, we will probably want to support all
>> > the existing ways to do this even with ACPI and with the unified
>> > interface, but it doesn't have to be the obvious way.
>>
>> Personally, I like the idea that each GPIO has a function, so now that
>> ACPI fully supports this I'd suggest the policy of using names for
>> each GPIO (e.g. never use the fallback "gpios" or "gpio" property),
>> and only ressort to indexes if several GPIOs happen to serve the same
>> function. I know we haven't reached consensus about this so far, but
>> it would be nice it we could discuss this point again in the light of
>> the new ACPI capabilities and come with something to write as a
>> guideline in the GPIO documentation.
>
> We have enforced naming things for the dmaengine binding, which has
> just led to everyone calling things "rx" and "tx". My fear is that
> if we start to enforce giving a name, we'd end up with lots of
> drivers that use a "gpio-gpios" property or something silly.

Checking the bindings is also part of the review process. :) Things
like "gpio-gpios" should simply not be accepted to begin with.

This sounds like a good chance to finally land some guidelines
regarding GPIO bindings. Let's summarize the situation:
- GPIO bindings can be defined using both DT and ACPI (both interfaces
nicely abstracted by the interface introduced by this series)
- Both firmware interfaces support indexed GPIOs
- Both firmware interfaces support named GPIO properties, with an
optional index (can we absolutely take this for granted on ACPI now?)
- For DT bindings, both foo-gpio and foo-gpios are valid properties
for the GPIO "foo".

That's what we have now and need to maintain. However for new drivers
we want to come with guidelines that will hopefully make things easier
to review. Here are my thoughts on the topic:

- GPIOs have a function, and this function should be the GPIO name.
Thus now that ACPI supports named properties, all new GPIO properties
*must* have an accurate, explicit name.
- Indexes are only used if several GPIOs fulfill the same function
(like parallel data lines). This situation should be exceptional.
GPIOs not fulfilling the same function are in no way allowed to reside
under the same property.
- The DT property suffix should be "-gpio" for properties expecting
exactly one GPIO and "-gpios" for properties specifying one or more
GPIOs, to make it clear whether one should bother about the number of
GPIOs present of not.

Anything against this, especially from the ACPI side? If not I will
come with a patch to amend the GPIO documentation.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Arnd Bergmann]

On Tuesday 21 October 2014 14:14:02 Alexandre Courbot wrote:
> >
> > We have enforced naming things for the dmaengine binding, which has
> > just led to everyone calling things "rx" and "tx". My fear is that
> > if we start to enforce giving a name, we'd end up with lots of
> > drivers that use a "gpio-gpios" property or something silly.
> 
> Checking the bindings is also part of the review process.  Things
> like "gpio-gpios" should simply not be accepted to begin with.
> 
> This sounds like a good chance to finally land some guidelines
> regarding GPIO bindings. Let's summarize the situation:
> - GPIO bindings can be defined using both DT and ACPI (both interfaces
> nicely abstracted by the interface introduced by this series)
> - Both firmware interfaces support indexed GPIOs
> - Both firmware interfaces support named GPIO properties, with an
> optional index (can we absolutely take this for granted on ACPI now?)

The developers working on it have said that they definitely want to
be compatible with the existing bindings, so the answer to your question
is yes.

> - For DT bindings, both foo-gpio and foo-gpios are valid properties
> for the GPIO "foo".

I would like to see the documentation recommend one over the other for
new bindings. Most other subsystems use the plural form even for
properties that only have one entry, so I'd like to see "foo-gpios"
become the canonical form for named gpio lines. Drivers that use
existing bindings with the "foo-gpio" form (or worse, "foo-somethingelse"
can use the same internal interface as the drivers that use name plus
index. Do you see a problem using what I suggested for the combined
API:

__gpiod_get(dev, propname, index); // use property name plus index
gpiod_get(dev, index); // use "gpios" plus index
gpiod_get_named(dev, "name"); use "name-gpios" with index 0

plus all fwprop/devm/flags/of/... variations of that

	Arnd

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Tue, Oct 21, 2014 at 09:54:45AM +0200, Arnd Bergmann wrote:
> On Tuesday 21 October 2014 14:14:02 Alexandre Courbot wrote:
> > >
> > > We have enforced naming things for the dmaengine binding, which has
> > > just led to everyone calling things "rx" and "tx". My fear is that
> > > if we start to enforce giving a name, we'd end up with lots of
> > > drivers that use a "gpio-gpios" property or something silly.
> > 
> > Checking the bindings is also part of the review process.  Things
> > like "gpio-gpios" should simply not be accepted to begin with.
> > 
> > This sounds like a good chance to finally land some guidelines
> > regarding GPIO bindings. Let's summarize the situation:
> > - GPIO bindings can be defined using both DT and ACPI (both interfaces
> > nicely abstracted by the interface introduced by this series)
> > - Both firmware interfaces support indexed GPIOs
> > - Both firmware interfaces support named GPIO properties, with an
> > optional index (can we absolutely take this for granted on ACPI now?)
> 
> The developers working on it have said that they definitely want to
> be compatible with the existing bindings, so the answer to your question
> is yes.

One thing to consider still is that we have plenty of existing ACPI code
out there where _DSD is not used but instead we rely on the index. An
example would be rfkill-gpio.c driver:

        gpio = devm_gpiod_get_index(&pdev->dev, "reset", 0);
        if (!IS_ERR(gpio)) {
		...
        }

        gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
        if (!IS_ERR(gpio)) {
		...
        }

It expects that GPIOs returned from _CRS are in specific order. Since we
can't change these existing ACPI tables, we must support them somehow.

This patch series handles it so that:

  1) If we can't find given property (e.g "reset-gpios" or
     "shutdown-gpios") the index above will refer directly to the GPIO
     resource returned from _CRS.

  2) If the property is found we ignore index and take it from the
     property instead.

This has the drawback that we cannot support this:

	Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
                                                                 ^^^^^^^^^^^^^^
So the second entry in the above is not accessible using
gpiod_get_index() and the reason is that we want to support the existing
and new ACPI tables where _DSD is not being used.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Arnd Bergmann]

On Wednesday 22 October 2014 11:10:44 Mika Westerberg wrote:
> 
> It expects that GPIOs returned from _CRS are in specific order. Since we
> can't change these existing ACPI tables, we must support them somehow.
> 
> This patch series handles it so that:
> 
>   1) If we can't find given property (e.g "reset-gpios" or
>      "shutdown-gpios") the index above will refer directly to the GPIO
>      resource returned from _CRS.
> 
>   2) If the property is found we ignore index and take it from the
>      property instead.
> 
> This has the drawback that we cannot support this:
> 
>         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
>                                                                  ^^^^^^^^^^^^^^
> So the second entry in the above is not accessible using
> gpiod_get_index() and the reason is that we want to support the existing
> and new ACPI tables where _DSD is not being used.

So this is not using the DT binding but does thing slightly differently then.
In this case (supporting two incompatible bindings for DT and ACPI), I think
the only sensible driver implementation would be to know what we are asking
for and use different devm_gpiod_get_index statements based on the firmware
interface.

	Arnd

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Wed, Oct 22, 2014 at 10:33:32AM +0200, Arnd Bergmann wrote:
> On Wednesday 22 October 2014 11:10:44 Mika Westerberg wrote:
> > 
> > It expects that GPIOs returned from _CRS are in specific order. Since we
> > can't change these existing ACPI tables, we must support them somehow.
> > 
> > This patch series handles it so that:
> > 
> >   1) If we can't find given property (e.g "reset-gpios" or
> >      "shutdown-gpios") the index above will refer directly to the GPIO
> >      resource returned from _CRS.
> > 
> >   2) If the property is found we ignore index and take it from the
> >      property instead.
> > 
> > This has the drawback that we cannot support this:
> > 
> >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
> >                                                                  ^^^^^^^^^^^^^^
> > So the second entry in the above is not accessible using
> > gpiod_get_index() and the reason is that we want to support the existing
> > and new ACPI tables where _DSD is not being used.
> 
> So this is not using the DT binding but does thing slightly differently then.
> In this case (supporting two incompatible bindings for DT and ACPI), I think
> the only sensible driver implementation would be to know what we are asking
> for and use different devm_gpiod_get_index statements based on the firmware
> interface.

Yes something like that is probably needed.

Alternatively (I didn't try if this works) we could do it so that
when we see:

	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);

we check first for the property ("shutdown-gpios"), and check if it has
more than one entry in the value, like:

	Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}

and in that case return the second entry. If we find this instead:

	Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0 }}

we just ignore the index.

Last if there is no _DSD the index refers directly to the GPIO resource
in _CRS.

This would support both _DSD and non-_DSD at the same time but it makes
the implementation more complex.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Arnd Bergmann]

On Wednesday 22 October 2014 11:51:40 Mika Westerberg wrote:
> On Wed, Oct 22, 2014 at 10:33:32AM +0200, Arnd Bergmann wrote:
> > On Wednesday 22 October 2014 11:10:44 Mika Westerberg wrote:
> > > 
> > > It expects that GPIOs returned from _CRS are in specific order. Since we
> > > can't change these existing ACPI tables, we must support them somehow.
> > > 
> > > This patch series handles it so that:
> > > 
> > >   1) If we can't find given property (e.g "reset-gpios" or
> > >      "shutdown-gpios") the index above will refer directly to the GPIO
> > >      resource returned from _CRS.
> > > 
> > >   2) If the property is found we ignore index and take it from the
> > >      property instead.
> > > 
> > > This has the drawback that we cannot support this:
> > > 
> > >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
> > >                                                                  ^^^^^^^^^^^^^^
> > > So the second entry in the above is not accessible using
> > > gpiod_get_index() and the reason is that we want to support the existing
> > > and new ACPI tables where _DSD is not being used.
> > 
> > So this is not using the DT binding but does thing slightly differently then.
> > In this case (supporting two incompatible bindings for DT and ACPI), I think
> > the only sensible driver implementation would be to know what we are asking
> > for and use different devm_gpiod_get_index statements based on the firmware
> > interface.
> 
> Yes something like that is probably needed.
> 
> Alternatively (I didn't try if this works) we could do it so that
> when we see:
> 
>         gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
> 
> we check first for the property ("shutdown-gpios"), and check if it has
> more than one entry in the value, like:
> 
>         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
> 
> and in that case return the second entry. If we find this instead:
> 
>         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0 }}
> 
> we just ignore the index.
> 
> Last if there is no _DSD the index refers directly to the GPIO resource
> in _CRS.
> 
> This would support both _DSD and non-_DSD at the same time but it makes
> the implementation more complex.

I think the main problem with that approach is that it makes the common
code more error-prone in case of unintentionally broken device descriptions,
because it less often returns an error.

	Arnd

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Rafael J. Wysocki]

On Wednesday, October 22, 2014 11:28:40 AM Arnd Bergmann wrote:
> On Wednesday 22 October 2014 11:51:40 Mika Westerberg wrote:
> > On Wed, Oct 22, 2014 at 10:33:32AM +0200, Arnd Bergmann wrote:
> > > On Wednesday 22 October 2014 11:10:44 Mika Westerberg wrote:
> > > > 
> > > > It expects that GPIOs returned from _CRS are in specific order. Since we
> > > > can't change these existing ACPI tables, we must support them somehow.
> > > > 
> > > > This patch series handles it so that:
> > > > 
> > > >   1) If we can't find given property (e.g "reset-gpios" or
> > > >      "shutdown-gpios") the index above will refer directly to the GPIO
> > > >      resource returned from _CRS.
> > > > 
> > > >   2) If the property is found we ignore index and take it from the
> > > >      property instead.
> > > > 
> > > > This has the drawback that we cannot support this:
> > > > 
> > > >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
> > > >                                                                  ^^^^^^^^^^^^^^
> > > > So the second entry in the above is not accessible using
> > > > gpiod_get_index() and the reason is that we want to support the existing
> > > > and new ACPI tables where _DSD is not being used.
> > > 
> > > So this is not using the DT binding but does thing slightly differently then.
> > > In this case (supporting two incompatible bindings for DT and ACPI), I think
> > > the only sensible driver implementation would be to know what we are asking
> > > for and use different devm_gpiod_get_index statements based on the firmware
> > > interface.
> > 
> > Yes something like that is probably needed.
> > 
> > Alternatively (I didn't try if this works) we could do it so that
> > when we see:
> > 
> >         gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
> > 
> > we check first for the property ("shutdown-gpios"), and check if it has
> > more than one entry in the value, like:
> > 
> >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
> > 
> > and in that case return the second entry. If we find this instead:
> > 
> >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0 }}
> > 
> > we just ignore the index.
> > 
> > Last if there is no _DSD the index refers directly to the GPIO resource
> > in _CRS.
> > 
> > This would support both _DSD and non-_DSD at the same time but it makes
> > the implementation more complex.
> 
> I think the main problem with that approach is that it makes the common
> code more error-prone in case of unintentionally broken device descriptions,
> because it less often returns an error.

Moreover, we need to clarify what situation we're really talking about.

For one, drivers using the unified interface only will always use names for
GPIOs, because they have to assume that either a DT or ACPI w/ _DSD is present.
This is the cost of keeping those drivers firmware interface agnostic.

So it looks like we're not talking about this case here.

Now, if there's no DT or no _DSD in the ACPI tables for the given device
*and* the driver wants to use its GPIOs anyway, it has to be ACPI-aware to
some extent, because in that case the device ID it has been matched against
tells it what the meaning of the GpioIo resources in the _CRS is.

Then, the driver needs to do something like:

	if (!device_property_present(dev, "known_property_that_should_be_present")
	    && ACPI_COMPANION(dev))
		acpi_probe_gpios(dev);

and in the acpi_probe_gpios() routine there may be checks like:

	if (device_has_id(dev, "MARY0001")) {
		The first pin in the first GpioIo resource in _CRS is "fred" and
		it is active-low.
		The third pin in the second GpioIo resource in _CRS is "steve"
		and it is not active-low.
	} else if (device_has_id(dev, "JANE0002")) {
		The first pin in the second GpioIo resource in _CRS is "fred" and
		it is not active-low.
		The second pin in the first GpioIo resource in _CRS is "steve"
		and it is active-low.
	}

and so on.  Of course, there may be drivers knowing that the meaning of the
GpioIo resources in _CRS is the same for all devices handled by them, in which
case they will not need to check device IDs, but the core has now way of
knowing that.  Only the drivers have that information and the core has now
way to figure out what to do for a given specific device.

So here's a radical idea: Why don't we introduce something like

	acpi_enumerate_gpio(dev, name, GpioIo_index, pin_index, active_low)

such that after calling, say, acpi_enumerate_gpio(dev, "fred", 0, 0, true) the
driver can do something like:

	desc = get_gpiod_by_name(dev, "fred");

and it'll all work.  Then, the only part of the driver that really needs to be
ACPI-specific will be the acpi_probe_gpios() function calling acpi_enumerate_gpio()
in accordance with what the device ID is.

Thoughts?

Rafael

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Wed, Oct 22, 2014 at 04:07:08PM +0200, Rafael J. Wysocki wrote:
> Moreover, we need to clarify what situation we're really talking about.
> 
> For one, drivers using the unified interface only will always use names for
> GPIOs, because they have to assume that either a DT or ACPI w/ _DSD is present.
> This is the cost of keeping those drivers firmware interface agnostic.
> 
> So it looks like we're not talking about this case here.

We are talking about the case of rfkill-gpio.c where it definitely wants
to use properties for the GPIOs but it cannot be sure if the underlying
firmware provides _DSD or not.

> Now, if there's no DT or no _DSD in the ACPI tables for the given device
> *and* the driver wants to use its GPIOs anyway, it has to be ACPI-aware to
> some extent, because in that case the device ID it has been matched against
> tells it what the meaning of the GpioIo resources in the _CRS is.
> 
> Then, the driver needs to do something like:
> 
> 	if (!device_property_present(dev, "known_property_that_should_be_present")
> 	    && ACPI_COMPANION(dev))
> 		acpi_probe_gpios(dev);

Indeed we can use similar pattern to detect if we have _DSD present or
not.

> and in the acpi_probe_gpios() routine there may be checks like:
> 
> 	if (device_has_id(dev, "MARY0001")) {
> 		The first pin in the first GpioIo resource in _CRS is "fred" and
> 		it is active-low.
> 		The third pin in the second GpioIo resource in _CRS is "steve"
> 		and it is not active-low.
> 	} else if (device_has_id(dev, "JANE0002")) {
> 		The first pin in the second GpioIo resource in _CRS is "fred" and
> 		it is not active-low.
> 		The second pin in the first GpioIo resource in _CRS is "steve"
> 		and it is active-low.
> 	}
> 
> and so on.  Of course, there may be drivers knowing that the meaning of the
> GpioIo resources in _CRS is the same for all devices handled by them, in which
> case they will not need to check device IDs, but the core has now way of
> knowing that.  Only the drivers have that information and the core has now
> way to figure out what to do for a given specific device.
> 
> So here's a radical idea: Why don't we introduce something like
> 
> 	acpi_enumerate_gpio(dev, name, GpioIo_index, pin_index, active_low)
> 
> such that after calling, say, acpi_enumerate_gpio(dev, "fred", 0, 0, true) the
> driver can do something like:
> 
> 	desc = get_gpiod_by_name(dev, "fred");
> 
> and it'll all work.  Then, the only part of the driver that really needs to be
> ACPI-specific will be the acpi_probe_gpios() function calling acpi_enumerate_gpio()
> in accordance with what the device ID is.
> 
> Thoughts?

I think this is good idea. It solves the rfkill-gpio.c problem with just
small amount of ACPI specific code.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Rafael J. Wysocki]

On Wednesday, October 22, 2014 05:56:51 PM Mika Westerberg wrote:
> On Wed, Oct 22, 2014 at 04:07:08PM +0200, Rafael J. Wysocki wrote:
> > Moreover, we need to clarify what situation we're really talking about.
> > 
> > For one, drivers using the unified interface only will always use names for
> > GPIOs, because they have to assume that either a DT or ACPI w/ _DSD is present.
> > This is the cost of keeping those drivers firmware interface agnostic.
> > 
> > So it looks like we're not talking about this case here.
> 
> We are talking about the case of rfkill-gpio.c where it definitely wants
> to use properties for the GPIOs but it cannot be sure if the underlying
> firmware provides _DSD or not.
> 
> > Now, if there's no DT or no _DSD in the ACPI tables for the given device
> > *and* the driver wants to use its GPIOs anyway, it has to be ACPI-aware to
> > some extent, because in that case the device ID it has been matched against
> > tells it what the meaning of the GpioIo resources in the _CRS is.
> > 
> > Then, the driver needs to do something like:
> > 
> > 	if (!device_property_present(dev, "known_property_that_should_be_present")
> > 	    && ACPI_COMPANION(dev))
> > 		acpi_probe_gpios(dev);
> 
> Indeed we can use similar pattern to detect if we have _DSD present or
> not.
> 
> > and in the acpi_probe_gpios() routine there may be checks like:
> > 
> > 	if (device_has_id(dev, "MARY0001")) {
> > 		The first pin in the first GpioIo resource in _CRS is "fred" and
> > 		it is active-low.
> > 		The third pin in the second GpioIo resource in _CRS is "steve"
> > 		and it is not active-low.
> > 	} else if (device_has_id(dev, "JANE0002")) {
> > 		The first pin in the second GpioIo resource in _CRS is "fred" and
> > 		it is not active-low.
> > 		The second pin in the first GpioIo resource in _CRS is "steve"
> > 		and it is active-low.
> > 	}
> > 
> > and so on.  Of course, there may be drivers knowing that the meaning of the
> > GpioIo resources in _CRS is the same for all devices handled by them, in which
> > case they will not need to check device IDs, but the core has now way of
> > knowing that.  Only the drivers have that information and the core has now
> > way to figure out what to do for a given specific device.
> > 
> > So here's a radical idea: Why don't we introduce something like
> > 
> > 	acpi_enumerate_gpio(dev, name, GpioIo_index, pin_index, active_low)
> > 
> > such that after calling, say, acpi_enumerate_gpio(dev, "fred", 0, 0, true) the
> > driver can do something like:
> > 
> > 	desc = get_gpiod_by_name(dev, "fred");
> > 
> > and it'll all work.  Then, the only part of the driver that really needs to be
> > ACPI-specific will be the acpi_probe_gpios() function calling acpi_enumerate_gpio()
> > in accordance with what the device ID is.
> > 
> > Thoughts?
> 
> I think this is good idea. It solves the rfkill-gpio.c problem with just
> small amount of ACPI specific code.

OK, would the below make sense, then (completely untested, on top of the v6
of the device properties patchset)?

Rafael


---
 drivers/acpi/scan.c         |    2 +
 drivers/gpio/gpiolib-acpi.c |   80 ++++++++++++++++++++++++++++++++++++++++++--
 include/acpi/acpi_bus.h     |    1 
 include/linux/acpi.h        |   15 ++++++++
 4 files changed, 96 insertions(+), 2 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -355,6 +355,7 @@ struct acpi_device {
 	struct list_head node;
 	struct list_head wakeup_list;
 	struct list_head del_list;
+	struct list_head driver_gpios;
 	struct acpi_device_status status;
 	struct acpi_device_flags flags;
 	struct acpi_device_pnp pnp;
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -672,6 +672,21 @@ do {									\
 #endif
 #endif
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
+int acpi_dev_add_driver_gpio(struct acpi_device *adev, const char *propname,
+			     int entry_index, int pin_index, bool active_low);
+void acpi_dev_remove_driver_gpios(struct acpi_device *adev);
+#else
+static inline int acpi_dev_add_driver_gpio(struct acpi_device *adev,
+					   const char *propname,
+					   int entry_index, int pin_index,
+					   bool active_low)
+{
+	return -ENXIO;
+}
+static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
+#endif
+
 /* Device properties */
 
 #define MAX_ACPI_REFERENCE_ARGS	8
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -47,6 +47,16 @@ struct acpi_gpio_chip {
 	struct list_head events;
 };
 
+struct acpi_gpio_data {
+	struct list_head item;
+	char *name;
+	int entry_index;
+	int pin_index;
+	bool active_low;
+};
+
+static DEFINE_MUTEX(driver_gpio_data_lock);
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->dev)
@@ -287,6 +297,70 @@ void acpi_gpiochip_free_interrupts(struc
 	}
 }
 
+int acpi_dev_add_driver_gpio(struct acpi_device *adev, const char *propname,
+			     int entry_index, int pin_index, bool active_low)
+{
+	struct acpi_gpio_data *gd;
+
+	if (!propname)
+		return -EINVAL;
+
+	gd = kzalloc(sizeof(*gd), GFP_KERNEL);
+	if (!gd)
+		return -ENOMEM;
+
+	gd->name = kstrdup(propname, GFP_KERNEL);
+	if (!gd->name) {
+		kfree(gd);
+		return -ENOMEM;
+	}
+	gd->entry_index = entry_index;
+	gd->pin_index = pin_index;
+	gd->active_low = active_low;
+
+	mutex_lock(&driver_gpio_data_lock);
+	list_add_tail(&gd->item, &adev->driver_gpios);
+	mutex_unlock(&driver_gpio_data_lock);
+
+	return 0;
+}
+
+void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
+{
+	struct acpi_gpio_data *gd, *next;
+
+	mutex_lock(&driver_gpio_data_lock);
+	list_for_each_entry_safe(gd, next, &adev->driver_gpios, item) {
+		list_del(&gd->item);
+		kfree(gd->name);
+		kfree(gd);
+	}
+	mutex_unlock(&driver_gpio_data_lock);
+}
+
+static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
+				      const char *name,
+				      struct acpi_reference_args *args)
+{
+	struct acpi_gpio_data *gd;
+
+	mutex_lock(&driver_gpio_data_lock);
+
+	list_for_each_entry(gd, &adev->driver_gpios, item)
+		if (!strcmp(name, gd->name)) {
+			args->adev = adev;
+			args->args[0] = gd->entry_index;
+			args->args[1] = gd->pin_index;
+			args->args[2] = gd->active_low;
+			args->nargs = 3;
+			return true;
+		}
+
+	mutex_unlock(&driver_gpio_data_lock);
+
+	return false;
+}
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -372,8 +446,10 @@ struct gpio_desc *acpi_get_gpiod_by_inde
 		memset(&args, 0, sizeof(args));
 		ret = acpi_dev_get_property_reference(adev, propname, NULL,
 						      index, &args);
-		if (ret)
-			return ERR_PTR(ret);
+		if (ret) {
+			if (!acpi_get_driver_gpio_data(adev, propname, &args))
+				return ERR_PTR(ret);
+		}
 
 		/*
 		 * The property was found and resolved so need to
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1284,6 +1284,7 @@ int acpi_device_add(struct acpi_device *
 	INIT_LIST_HEAD(&device->wakeup_list);
 	INIT_LIST_HEAD(&device->physical_node_list);
 	INIT_LIST_HEAD(&device->del_list);
+	INIT_LIST_HEAD(&device->driver_gpios);
 	mutex_init(&device->physical_node_lock);
 
 	new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL);
@@ -2354,6 +2355,7 @@ void acpi_bus_trim(struct acpi_device *a
 	} else {
 		device_release_driver(&adev->dev);
 	}
+	acpi_dev_remove_driver_gpios(adev);
 	/*
 	 * Most likely, the device is going away, so put it into D3cold before
 	 * that.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Alexandre Courbot]

On Tue, Oct 21, 2014 at 4:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 21 October 2014 14:14:02 Alexandre Courbot wrote:
>> >
>> > We have enforced naming things for the dmaengine binding, which has
>> > just led to everyone calling things "rx" and "tx". My fear is that
>> > if we start to enforce giving a name, we'd end up with lots of
>> > drivers that use a "gpio-gpios" property or something silly.
>>
>> Checking the bindings is also part of the review process.  Things
>> like "gpio-gpios" should simply not be accepted to begin with.
>>
>> This sounds like a good chance to finally land some guidelines
>> regarding GPIO bindings. Let's summarize the situation:
>> - GPIO bindings can be defined using both DT and ACPI (both interfaces
>> nicely abstracted by the interface introduced by this series)
>> - Both firmware interfaces support indexed GPIOs
>> - Both firmware interfaces support named GPIO properties, with an
>> optional index (can we absolutely take this for granted on ACPI now?)
>
> The developers working on it have said that they definitely want to
> be compatible with the existing bindings, so the answer to your question
> is yes.
>
>> - For DT bindings, both foo-gpio and foo-gpios are valid properties
>> for the GPIO "foo".
>
> I would like to see the documentation recommend one over the other for
> new bindings. Most other subsystems use the plural form even for
> properties that only have one entry, so I'd like to see "foo-gpios"
> become the canonical form for named gpio lines.

Sounds good.

> Drivers that use
> existing bindings with the "foo-gpio" form (or worse, "foo-somethingelse"
> can use the same internal interface as the drivers that use name plus
> index. Do you see a problem using what I suggested for the combined
> API:
>
> __gpiod_get(dev, propname, index); // use property name plus index
> gpiod_get(dev, index); // use "gpios" plus index
> gpiod_get_named(dev, "name"); use "name-gpios" with index 0

Apart from the loosy naming practices which we sometimes see (and
which should be caught during review), do you have something against
requiring a name for all new GPIO bindings, i.e. for ensuring that all
new properties are "name-gpio" and forbidding "gpios"?

Requiring a proper name for all GPIOs makes a lot of sense IMHO, it
makes drivers easier to understand and is less error-prone than long
arrays of GPIOs. The API would then be basically what we have today:

gpiod_get(dev, name) // use "name-gpios" with index 0
gpiod_get_index(dev, name, index) // for the rare case where several
GPIOs serve the same function. Not to be used lightly.

... with stronger guidelines for the definition of new bindings, and a
big warning in the kerneldoc of gpiod_get_index().

ACPI drivers that may use tables without _DSD should then use a way to
bind GPIO names to indexes as a fallback for older hardware.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Alexandre Courbot]

On Wed, Oct 22, 2014 at 11:07 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, October 22, 2014 11:28:40 AM Arnd Bergmann wrote:
>> On Wednesday 22 October 2014 11:51:40 Mika Westerberg wrote:
>> > On Wed, Oct 22, 2014 at 10:33:32AM +0200, Arnd Bergmann wrote:
>> > > On Wednesday 22 October 2014 11:10:44 Mika Westerberg wrote:
>> > > >
>> > > > It expects that GPIOs returned from _CRS are in specific order. Since we
>> > > > can't change these existing ACPI tables, we must support them somehow.
>> > > >
>> > > > This patch series handles it so that:
>> > > >
>> > > >   1) If we can't find given property (e.g "reset-gpios" or
>> > > >      "shutdown-gpios") the index above will refer directly to the GPIO
>> > > >      resource returned from _CRS.
>> > > >
>> > > >   2) If the property is found we ignore index and take it from the
>> > > >      property instead.
>> > > >
>> > > > This has the drawback that we cannot support this:
>> > > >
>> > > >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
>> > > >                                                                  ^^^^^^^^^^^^^^
>> > > > So the second entry in the above is not accessible using
>> > > > gpiod_get_index() and the reason is that we want to support the existing
>> > > > and new ACPI tables where _DSD is not being used.
>> > >
>> > > So this is not using the DT binding but does thing slightly differently then.
>> > > In this case (supporting two incompatible bindings for DT and ACPI), I think
>> > > the only sensible driver implementation would be to know what we are asking
>> > > for and use different devm_gpiod_get_index statements based on the firmware
>> > > interface.
>> >
>> > Yes something like that is probably needed.
>> >
>> > Alternatively (I didn't try if this works) we could do it so that
>> > when we see:
>> >
>> >         gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
>> >
>> > we check first for the property ("shutdown-gpios"), and check if it has
>> > more than one entry in the value, like:
>> >
>> >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0, ^GPIO, 1, 0, 0}}
>> >
>> > and in that case return the second entry. If we find this instead:
>> >
>> >         Package () { "reset-gpios", Package () { ^GPIO, 0, 0, 0 }}
>> >
>> > we just ignore the index.
>> >
>> > Last if there is no _DSD the index refers directly to the GPIO resource
>> > in _CRS.
>> >
>> > This would support both _DSD and non-_DSD at the same time but it makes
>> > the implementation more complex.
>>
>> I think the main problem with that approach is that it makes the common
>> code more error-prone in case of unintentionally broken device descriptions,
>> because it less often returns an error.
>
> Moreover, we need to clarify what situation we're really talking about.
>
> For one, drivers using the unified interface only will always use names for
> GPIOs, because they have to assume that either a DT or ACPI w/ _DSD is present.
> This is the cost of keeping those drivers firmware interface agnostic.
>
> So it looks like we're not talking about this case here.
>
> Now, if there's no DT or no _DSD in the ACPI tables for the given device
> *and* the driver wants to use its GPIOs anyway, it has to be ACPI-aware to
> some extent, because in that case the device ID it has been matched against
> tells it what the meaning of the GpioIo resources in the _CRS is.
>
> Then, the driver needs to do something like:
>
>         if (!device_property_present(dev, "known_property_that_should_be_present")
>             && ACPI_COMPANION(dev))
>                 acpi_probe_gpios(dev);
>
> and in the acpi_probe_gpios() routine there may be checks like:
>
>         if (device_has_id(dev, "MARY0001")) {
>                 The first pin in the first GpioIo resource in _CRS is "fred" and
>                 it is active-low.
>                 The third pin in the second GpioIo resource in _CRS is "steve"
>                 and it is not active-low.
>         } else if (device_has_id(dev, "JANE0002")) {
>                 The first pin in the second GpioIo resource in _CRS is "fred" and
>                 it is not active-low.
>                 The second pin in the first GpioIo resource in _CRS is "steve"
>                 and it is active-low.
>         }
>
> and so on.  Of course, there may be drivers knowing that the meaning of the
> GpioIo resources in _CRS is the same for all devices handled by them, in which
> case they will not need to check device IDs, but the core has now way of
> knowing that.  Only the drivers have that information and the core has now
> way to figure out what to do for a given specific device.
>
> So here's a radical idea: Why don't we introduce something like
>
>         acpi_enumerate_gpio(dev, name, GpioIo_index, pin_index, active_low)
>
> such that after calling, say, acpi_enumerate_gpio(dev, "fred", 0, 0, true) the
> driver can do something like:
>
>         desc = get_gpiod_by_name(dev, "fred");
>
> and it'll all work.  Then, the only part of the driver that really needs to be
> ACPI-specific will be the acpi_probe_gpios() function calling acpi_enumerate_gpio()
> in accordance with what the device ID is.

I like this idea. It doesn't complicate the GPIO interface (i.e. no
"if you are on ACPI and no _DSD is present then gpiod_get() will
behave that way...") and does the plumbing behind the scenes.

I will also allow us to finally push the use of names instead of
indexes in the GPIO API. I'm all for it.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Arnd Bergmann]

On Thursday 23 October 2014 15:10:55 Alexandre Courbot wrote:
> >
> > Then, the driver needs to do something like:
> >
> >         if (!device_property_present(dev, "known_property_that_should_be_present")
> >             && ACPI_COMPANION(dev))
> >                 acpi_probe_gpios(dev);
> >
> > and in the acpi_probe_gpios() routine there may be checks like:
> >
> >         if (device_has_id(dev, "MARY0001")) {
> >                 The first pin in the first GpioIo resource in _CRS is "fred" and
> >                 it is active-low.
> >                 The third pin in the second GpioIo resource in _CRS is "steve"
> >                 and it is not active-low.
> >         } else if (device_has_id(dev, "JANE0002")) {
> >                 The first pin in the second GpioIo resource in _CRS is "fred" and
> >                 it is not active-low.
> >                 The second pin in the first GpioIo resource in _CRS is "steve"
> >                 and it is active-low.
> >         }
> >
> > and so on.  Of course, there may be drivers knowing that the meaning of the
> > GpioIo resources in _CRS is the same for all devices handled by them, in which
> > case they will not need to check device IDs, but the core has now way of
> > knowing that.  Only the drivers have that information and the core has now
> > way to figure out what to do for a given specific device.
> >
> > So here's a radical idea: Why don't we introduce something like
> >
> >         acpi_enumerate_gpio(dev, name, GpioIo_index, pin_index, active_low)
> >
> > such that after calling, say, acpi_enumerate_gpio(dev, "fred", 0, 0, true) the
> > driver can do something like:
> >
> >         desc = get_gpiod_by_name(dev, "fred");
> >
> > and it'll all work.  Then, the only part of the driver that really needs to be
> > ACPI-specific will be the acpi_probe_gpios() function calling acpi_enumerate_gpio()
> > in accordance with what the device ID is.
> 
> I like this idea. It doesn't complicate the GPIO interface (i.e. no
> "if you are on ACPI and no _DSD is present then gpiod_get() will
> behave that way...") and does the plumbing behind the scenes.
> 
> I will also allow us to finally push the use of names instead of
> indexes in the GPIO API. I'm all for it.

Yes, sounds good.

	Arnd

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Arnd Bergmann]

On Thursday 23 October 2014 15:02:46 Alexandre Courbot wrote:
> On Tue, Oct 21, 2014 at 4:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 21 October 2014 14:14:02 Alexandre Courbot wrote:

> > Drivers that use
> > existing bindings with the "foo-gpio" form (or worse, "foo-somethingelse"
> > can use the same internal interface as the drivers that use name plus
> > index. Do you see a problem using what I suggested for the combined
> > API:
> >
> > __gpiod_get(dev, propname, index); // use property name plus index
> > gpiod_get(dev, index); // use "gpios" plus index
> > gpiod_get_named(dev, "name"); use "name-gpios" with index 0
> 
> Apart from the loosy naming practices which we sometimes see (and
> which should be caught during review), do you have something against
> requiring a name for all new GPIO bindings, i.e. for ensuring that all
> new properties are "name-gpio" and forbidding "gpios"?

Most other subsystems don't require a name, and traditionally we only
had anonymous indexed properties for a lot of things (registers, 
interrupts, ...).

I still like the idea of using anonymous references for simple things,
but if you and Linus feel that it's better to mandate names from now on,
I won't complain.

> Requiring a proper name for all GPIOs makes a lot of sense IMHO, it
> makes drivers easier to understand and is less error-prone than long
> arrays of GPIOs. The API would then be basically what we have today:
> 
> gpiod_get(dev, name) // use "name-gpios" with index 0
> gpiod_get_index(dev, name, index) // for the rare case where several
> GPIOs serve the same function. Not to be used lightly.
> 
> ... with stronger guidelines for the definition of new bindings, and a
> big warning in the kerneldoc of gpiod_get_index().
> 
> ACPI drivers that may use tables without _DSD should then use a way to
> bind GPIO names to indexes as a fallback for older hardware.

Ok.

	Arnd

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Thu, Oct 23, 2014 at 01:21:06AM +0200, Rafael J. Wysocki wrote:
> OK, would the below make sense, then (completely untested, on top of the v6
> of the device properties patchset)?

Yes it does :-)

With the the below fix it works nicely with the modified rfkill-gpio.c
driver.

> +static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
> +				      const char *name,
> +				      struct acpi_reference_args *args)
> +{
> +	struct acpi_gpio_data *gd;
> +
> +	mutex_lock(&driver_gpio_data_lock);
> +
> +	list_for_each_entry(gd, &adev->driver_gpios, item)
> +		if (!strcmp(name, gd->name)) {
> +			args->adev = adev;
> +			args->args[0] = gd->entry_index;
> +			args->args[1] = gd->pin_index;
> +			args->args[2] = gd->active_low;
> +			args->nargs = 3;

			mutex_unlock(&driver_gpio_data_lock);

> +			return true;
> +		}
> +
> +	mutex_unlock(&driver_gpio_data_lock);
> +
> +	return false;
> +}

Also I think the two functions should be exported to modules as well.

Here are the modifications needed for rfkill-gpio.c driver. I think it
is not that bad considering that now the driver supports both ACPI _DSD
and non-_DSD at the same time.

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 0f62326c0f5e..f8754e7d81ea 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -67,6 +67,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 				  struct rfkill_gpio_data *rfkill)
 {
 	const struct acpi_device_id *id;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret;
 
 	id = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!id)
@@ -75,6 +77,20 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 	rfkill->name = dev_name(dev);
 	rfkill->type = (unsigned)id->driver_data;
 
+	/*
+	 * Add default mapping for ACPI _DSD properties now just in case
+	 * there is no _DSD for this device.
+	 */
+	ret = acpi_dev_add_driver_gpio(adev, "reset-gpios", 0, 0, false);
+	if (ret)
+		return ret;
+
+	ret = acpi_dev_add_driver_gpio(adev, "shutdown-gpios", 1, 0, false);
+	if (ret) {
+		acpi_dev_remove_driver_gpios(adev);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -110,7 +126,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 		rfkill->reset_gpio = gpio;
 	}
 
-	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
+	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
 	if (!IS_ERR(gpio)) {
 		ret = gpiod_direction_output(gpio, 0);
 		if (ret)
@@ -150,6 +166,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
 	rfkill_unregister(rfkill->rfkill_dev);
 	rfkill_destroy(rfkill->rfkill_dev);
 
+	if (ACPI_COMPANION(&pdev->dev))
+		acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
+
 	return 0;
 }
 

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Rafael J. Wysocki]

On Thursday, October 23, 2014 03:56:55 PM Mika Westerberg wrote:
> On Thu, Oct 23, 2014 at 01:21:06AM +0200, Rafael J. Wysocki wrote:
> > OK, would the below make sense, then (completely untested, on top of the v6
> > of the device properties patchset)?
> 
> Yes it does :-)
> 
> With the the below fix it works nicely with the modified rfkill-gpio.c
> driver.
> 
> > +static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
> > +				      const char *name,
> > +				      struct acpi_reference_args *args)
> > +{
> > +	struct acpi_gpio_data *gd;
> > +
> > +	mutex_lock(&driver_gpio_data_lock);
> > +
> > +	list_for_each_entry(gd, &adev->driver_gpios, item)
> > +		if (!strcmp(name, gd->name)) {
> > +			args->adev = adev;
> > +			args->args[0] = gd->entry_index;
> > +			args->args[1] = gd->pin_index;
> > +			args->args[2] = gd->active_low;
> > +			args->nargs = 3;
> 
> 			mutex_unlock(&driver_gpio_data_lock);
> 
> > +			return true;
> > +		}
> > +
> > +	mutex_unlock(&driver_gpio_data_lock);
> > +
> > +	return false;
> > +}
> 
> Also I think the two functions should be exported to modules as well.
> 
> Here are the modifications needed for rfkill-gpio.c driver. I think it
> is not that bad considering that now the driver supports both ACPI _DSD
> and non-_DSD at the same time.

OK, let's try to take that a bit farther. :-)

With the (untested) patch below applied (which is a replacement for the one
sent previously), the driver can use static tables like these:

static struct acpi_gpio_params reset_gpios = { 0, 0, false };
statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };

static struct acpi_gpio_mapping my_gpio_mapping[] = {
	{ "reset-gpios", &reset_gpios, 1 },
	{ "shutdown-gpios", &shutdown_gpios, 1 },
	NULL,
};

->

> 
> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
> index 0f62326c0f5e..f8754e7d81ea 100644
> --- a/net/rfkill/rfkill-gpio.c
> +++ b/net/rfkill/rfkill-gpio.c
> @@ -67,6 +67,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>  				  struct rfkill_gpio_data *rfkill)
>  {
>  	const struct acpi_device_id *id;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	int ret;
>  
>  	id = acpi_match_device(dev->driver->acpi_match_table, dev);
>  	if (!id)
> @@ -75,6 +77,20 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>  	rfkill->name = dev_name(dev);
>  	rfkill->type = (unsigned)id->driver_data;
>  
> +	/*
> +	 * Add default mapping for ACPI _DSD properties now just in case
> +	 * there is no _DSD for this device.
> +	 */
> +	ret = acpi_dev_add_driver_gpio(adev, "reset-gpios", 0, 0, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = acpi_dev_add_driver_gpio(adev, "shutdown-gpios", 1, 0, false);
> +	if (ret) {
> +		acpi_dev_remove_driver_gpios(adev);
> +		return ret;
> +	}

-> and then simply do

	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(dev), &my_gpio_mapping);

instead of the above.

>  	return 0;
>  }
>  
> @@ -110,7 +126,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
>  		rfkill->reset_gpio = gpio;
>  	}
>  
> -	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
> +	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
>  	if (!IS_ERR(gpio)) {
>  		ret = gpiod_direction_output(gpio, 0);
>  		if (ret)
> @@ -150,6 +166,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
>  	rfkill_unregister(rfkill->rfkill_dev);
>  	rfkill_destroy(rfkill->rfkill_dev);
>  
> +	if (ACPI_COMPANION(&pdev->dev))
> +		acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
> +

And here simply 

	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));

>  	return 0;
>  }

and then we can even cover situations in which there is one name for a list of
GPIOs.

Rafael


---
 drivers/gpio/gpiolib-acpi.c |   40 ++++++++++++++++++++++++++++++++++++++--
 include/acpi/acpi_bus.h     |    3 +++
 include/linux/acpi.h        |   30 ++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 2 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -345,6 +345,8 @@ struct acpi_device_data {
 	const union acpi_object *of_compatible;
 };
 
+struct acpi_gpio_mapping;
+
 /* Device */
 struct acpi_device {
 	int device_type;
@@ -366,6 +368,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
+	struct acpi_gpio_mapping *driver_gpios;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -672,6 +672,36 @@ do {									\
 #endif
 #endif
 
+struct acpi_gpio_params {
+	unsigned int crs_entry_index;
+	unsigned int pin_index;
+	bool active_low;
+};
+
+struct acpi_gpio_mapping {
+	const char *name;
+	struct acpi_gpio_params *data;
+	unsigned int size;
+};
+
+#if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
+int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      struct acpi_gpio_mapping *gpios);
+
+static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
+{
+	if (adev)
+		adev->driver_gpios = NULL;
+}
+#else
+static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      struct acpi_gpio_mapping *gpios)
+{
+	return -ENXIO;
+}
+static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
+#endif
+
 /* Device properties */
 
 #define MAX_ACPI_REFERENCE_ARGS	8
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -287,6 +287,38 @@ void acpi_gpiochip_free_interrupts(struc
 	}
 }
 
+int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      struct acpi_gpio_mapping *gpios)
+{
+	if (adev && gpios) {
+		adev->driver_gpios = gpios;
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_add_driver_gpios);
+
+static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
+				      const char *name, int index,
+				      struct acpi_reference_args *args)
+{
+	struct acpi_gpio_mapping *gm;
+
+	for (gm = adev->driver_gpios; gm; gm++)
+		if (!strcmp(name, gm->name) && gm->data && index < gm->size) {
+			struct acpi_gpio_params *par = gm->data + index;
+
+			args->adev = adev;
+			args->args[0] = par->crs_entry_index;
+			args->args[1] = par->pin_index;
+			args->args[2] = par->active_low;
+			args->nargs = 3;
+			return true;
+		}
+
+	return false;
+}
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -372,8 +404,12 @@ struct gpio_desc *acpi_get_gpiod_by_inde
 		memset(&args, 0, sizeof(args));
 		ret = acpi_dev_get_property_reference(adev, propname, NULL,
 						      index, &args);
-		if (ret)
-			return ERR_PTR(ret);
+		if (ret) {
+			bool found = acpi_get_driver_gpio_data(adev, propname,
+							       index, &args);
+			if (!found)
+				return ERR_PTR(ret);
+		}
 
 		/*
 		 * The property was found and resolved so need to

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Alexandre Courbot]

On Fri, Oct 24, 2014 at 6:51 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, October 23, 2014 03:56:55 PM Mika Westerberg wrote:
>> On Thu, Oct 23, 2014 at 01:21:06AM +0200, Rafael J. Wysocki wrote:
>> > OK, would the below make sense, then (completely untested, on top of the v6
>> > of the device properties patchset)?
>>
>> Yes it does :-)
>>
>> With the the below fix it works nicely with the modified rfkill-gpio.c
>> driver.
>>
>> > +static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
>> > +                                 const char *name,
>> > +                                 struct acpi_reference_args *args)
>> > +{
>> > +   struct acpi_gpio_data *gd;
>> > +
>> > +   mutex_lock(&driver_gpio_data_lock);
>> > +
>> > +   list_for_each_entry(gd, &adev->driver_gpios, item)
>> > +           if (!strcmp(name, gd->name)) {
>> > +                   args->adev = adev;
>> > +                   args->args[0] = gd->entry_index;
>> > +                   args->args[1] = gd->pin_index;
>> > +                   args->args[2] = gd->active_low;
>> > +                   args->nargs = 3;
>>
>>                       mutex_unlock(&driver_gpio_data_lock);
>>
>> > +                   return true;
>> > +           }
>> > +
>> > +   mutex_unlock(&driver_gpio_data_lock);
>> > +
>> > +   return false;
>> > +}
>>
>> Also I think the two functions should be exported to modules as well.
>>
>> Here are the modifications needed for rfkill-gpio.c driver. I think it
>> is not that bad considering that now the driver supports both ACPI _DSD
>> and non-_DSD at the same time.
>
> OK, let's try to take that a bit farther. :-)
>
> With the (untested) patch below applied (which is a replacement for the one
> sent previously), the driver can use static tables like these:
>
> static struct acpi_gpio_params reset_gpios = { 0, 0, false };
> statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
>
> static struct acpi_gpio_mapping my_gpio_mapping[] = {
>         { "reset-gpios", &reset_gpios, 1 },
>         { "shutdown-gpios", &shutdown_gpios, 1 },
>         NULL,
> };
>
> ->
>
>>
>> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
>> index 0f62326c0f5e..f8754e7d81ea 100644
>> --- a/net/rfkill/rfkill-gpio.c
>> +++ b/net/rfkill/rfkill-gpio.c
>> @@ -67,6 +67,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>>                                 struct rfkill_gpio_data *rfkill)
>>  {
>>       const struct acpi_device_id *id;
>> +     struct acpi_device *adev = ACPI_COMPANION(dev);
>> +     int ret;
>>
>>       id = acpi_match_device(dev->driver->acpi_match_table, dev);
>>       if (!id)
>> @@ -75,6 +77,20 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>>       rfkill->name = dev_name(dev);
>>       rfkill->type = (unsigned)id->driver_data;
>>
>> +     /*
>> +      * Add default mapping for ACPI _DSD properties now just in case
>> +      * there is no _DSD for this device.
>> +      */
>> +     ret = acpi_dev_add_driver_gpio(adev, "reset-gpios", 0, 0, false);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = acpi_dev_add_driver_gpio(adev, "shutdown-gpios", 1, 0, false);
>> +     if (ret) {
>> +             acpi_dev_remove_driver_gpios(adev);
>> +             return ret;
>> +     }
>
> -> and then simply do
>
>         ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(dev), &my_gpio_mapping);
>
> instead of the above.
>
>>       return 0;
>>  }
>>
>> @@ -110,7 +126,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
>>               rfkill->reset_gpio = gpio;
>>       }
>>
>> -     gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
>> +     gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
>>       if (!IS_ERR(gpio)) {
>>               ret = gpiod_direction_output(gpio, 0);
>>               if (ret)
>> @@ -150,6 +166,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
>>       rfkill_unregister(rfkill->rfkill_dev);
>>       rfkill_destroy(rfkill->rfkill_dev);
>>
>> +     if (ACPI_COMPANION(&pdev->dev))
>> +             acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
>> +
>
> And here simply
>
>         acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
>
>>       return 0;
>>  }
>
> and then we can even cover situations in which there is one name for a list of
> GPIOs.

That looks just perfect.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Thu, Oct 23, 2014 at 11:51:59PM +0200, Rafael J. Wysocki wrote:
> OK, let's try to take that a bit farther. :-)
> 
> With the (untested) patch below applied (which is a replacement for the one
> sent previously), the driver can use static tables like these:
> 
> static struct acpi_gpio_params reset_gpios = { 0, 0, false };
> statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
> 
> static struct acpi_gpio_mapping my_gpio_mapping[] = {
> 	{ "reset-gpios", &reset_gpios, 1 },
> 	{ "shutdown-gpios", &shutdown_gpios, 1 },
> 	NULL,
> };
> 

This is even better :-)

I tried this with few changes:

 - constify the mapping parameter
 - check for NULL adev->driver_gpios in acpi_get_driver_gpio_data()
 - check for gm->name instead of gm in acpi_get_driver_gpio_data()

It works just fine. For reference here are my changes to rkill-gpio.c:

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 0f62326c0f5e..ec8f5ac53520 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -63,6 +63,15 @@ static const struct rfkill_ops rfkill_gpio_ops = {
 	.set_block = rfkill_gpio_set_power,
 };
 
+static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
+static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_rfkill_default_gpios[] = {
+	{ "reset-gpios", &reset_gpios, 1 },
+	{ "shutdown-gpios", &shutdown_gpios, 1 },
+	{ },
+};
+
 static int rfkill_gpio_acpi_probe(struct device *dev,
 				  struct rfkill_gpio_data *rfkill)
 {
@@ -75,7 +84,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 	rfkill->name = dev_name(dev);
 	rfkill->type = (unsigned)id->driver_data;
 
-	return 0;
+	return acpi_dev_add_driver_gpios(ACPI_COMPANION(dev),
+					 acpi_rfkill_default_gpios);
 }
 
 static int rfkill_gpio_probe(struct platform_device *pdev)
@@ -110,7 +120,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 		rfkill->reset_gpio = gpio;
 	}
 
-	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
+	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
 	if (!IS_ERR(gpio)) {
 		ret = gpiod_direction_output(gpio, 0);
 		if (ret)
@@ -150,6 +160,8 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
 	rfkill_unregister(rfkill->rfkill_dev);
 	rfkill_destroy(rfkill->rfkill_dev);
 
+	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
+
 	return 0;
 }
 
And here are the changes on top of your patch:

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 691501b53c5e..4151cd774f2f 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -288,7 +288,7 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 }
 
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios)
+			      const struct acpi_gpio_mapping *gpios)
 {
 	if (adev && gpios) {
 		adev->driver_gpios = gpios;
@@ -302,11 +302,14 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 				      const char *name, int index,
 				      struct acpi_reference_args *args)
 {
-	struct acpi_gpio_mapping *gm;
+	const struct acpi_gpio_mapping *gm;
 
-	for (gm = adev->driver_gpios; gm; gm++)
+	if (!adev->driver_gpios)
+		return false;
+
+	for (gm = adev->driver_gpios; gm->name; gm++)
 		if (!strcmp(name, gm->name) && gm->data && index < gm->size) {
-			struct acpi_gpio_params *par = gm->data + index;
+			const struct acpi_gpio_params *par = gm->data + index;
 
 			args->adev = adev;
 			args->args[0] = par->crs_entry_index;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 2de7a2614c61..98d992062270 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -368,7 +368,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
-	struct acpi_gpio_mapping *driver_gpios;
+	const struct acpi_gpio_mapping *driver_gpios;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 26b7a639f4b9..23f9c55d0331 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -680,13 +680,13 @@ struct acpi_gpio_params {
 
 struct acpi_gpio_mapping {
 	const char *name;
-	struct acpi_gpio_params *data;
+	const struct acpi_gpio_params *data;
 	unsigned int size;
 };
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios);
+			      const struct acpi_gpio_mapping *gpios);
 
 static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
 {
@@ -695,7 +695,7 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
 }
 #else
 static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios)
+			      const struct acpi_gpio_mapping *gpios)
 {
 	return -ENXIO;
 }

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Rafael J. Wysocki]

On Friday, October 24, 2014 10:34:36 AM Mika Westerberg wrote:
> On Thu, Oct 23, 2014 at 11:51:59PM +0200, Rafael J. Wysocki wrote:
> > OK, let's try to take that a bit farther. :-)
> > 
> > With the (untested) patch below applied (which is a replacement for the one
> > sent previously), the driver can use static tables like these:
> > 
> > static struct acpi_gpio_params reset_gpios = { 0, 0, false };
> > statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
> > 
> > static struct acpi_gpio_mapping my_gpio_mapping[] = {
> > 	{ "reset-gpios", &reset_gpios, 1 },
> > 	{ "shutdown-gpios", &shutdown_gpios, 1 },
> > 	NULL,
> > };
> > 
> 
> This is even better :-)
> 
> I tried this with few changes:
> 
>  - constify the mapping parameter
>  - check for NULL adev->driver_gpios in acpi_get_driver_gpio_data()
>  - check for gm->name instead of gm in acpi_get_driver_gpio_data()
> 
> It works just fine. For reference here are my changes to rkill-gpio.c:
> 
> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
> index 0f62326c0f5e..ec8f5ac53520 100644
> --- a/net/rfkill/rfkill-gpio.c
> +++ b/net/rfkill/rfkill-gpio.c
> @@ -63,6 +63,15 @@ static const struct rfkill_ops rfkill_gpio_ops = {
>  	.set_block = rfkill_gpio_set_power,
>  };
>  
> +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_rfkill_default_gpios[] = {
> +	{ "reset-gpios", &reset_gpios, 1 },
> +	{ "shutdown-gpios", &shutdown_gpios, 1 },
> +	{ },
> +};
> +
>  static int rfkill_gpio_acpi_probe(struct device *dev,
>  				  struct rfkill_gpio_data *rfkill)
>  {
> @@ -75,7 +84,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>  	rfkill->name = dev_name(dev);
>  	rfkill->type = (unsigned)id->driver_data;
>  
> -	return 0;
> +	return acpi_dev_add_driver_gpios(ACPI_COMPANION(dev),
> +					 acpi_rfkill_default_gpios);
>  }
>  
>  static int rfkill_gpio_probe(struct platform_device *pdev)
> @@ -110,7 +120,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
>  		rfkill->reset_gpio = gpio;
>  	}
>  
> -	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
> +	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
>  	if (!IS_ERR(gpio)) {
>  		ret = gpiod_direction_output(gpio, 0);
>  		if (ret)
> @@ -150,6 +160,8 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
>  	rfkill_unregister(rfkill->rfkill_dev);
>  	rfkill_destroy(rfkill->rfkill_dev);
>  
> +	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
> +
>  	return 0;
>  }
>  
> And here are the changes on top of your patch:
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 691501b53c5e..4151cd774f2f 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -288,7 +288,7 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  }
>  
>  int acpi_dev_add_driver_gpios(struct acpi_device *adev,
> -			      struct acpi_gpio_mapping *gpios)
> +			      const struct acpi_gpio_mapping *gpios)
>  {
>  	if (adev && gpios) {
>  		adev->driver_gpios = gpios;
> @@ -302,11 +302,14 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
>  				      const char *name, int index,
>  				      struct acpi_reference_args *args)
>  {
> -	struct acpi_gpio_mapping *gm;
> +	const struct acpi_gpio_mapping *gm;
>  
> -	for (gm = adev->driver_gpios; gm; gm++)
> +	if (!adev->driver_gpios)
> +		return false;
> +
> +	for (gm = adev->driver_gpios; gm->name; gm++)
>  		if (!strcmp(name, gm->name) && gm->data && index < gm->size) {
> -			struct acpi_gpio_params *par = gm->data + index;
> +			const struct acpi_gpio_params *par = gm->data + index;
>  
>  			args->adev = adev;
>  			args->args[0] = par->crs_entry_index;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 2de7a2614c61..98d992062270 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -368,7 +368,7 @@ struct acpi_device {
>  	struct acpi_scan_handler *handler;
>  	struct acpi_hotplug_context *hp;
>  	struct acpi_driver *driver;
> -	struct acpi_gpio_mapping *driver_gpios;
> +	const struct acpi_gpio_mapping *driver_gpios;
>  	void *driver_data;
>  	struct device dev;
>  	unsigned int physical_node_count;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 26b7a639f4b9..23f9c55d0331 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -680,13 +680,13 @@ struct acpi_gpio_params {
>  
>  struct acpi_gpio_mapping {
>  	const char *name;
> -	struct acpi_gpio_params *data;
> +	const struct acpi_gpio_params *data;
>  	unsigned int size;
>  };
>  
>  #if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
>  int acpi_dev_add_driver_gpios(struct acpi_device *adev,
> -			      struct acpi_gpio_mapping *gpios);
> +			      const struct acpi_gpio_mapping *gpios);
>  
>  static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
>  {
> @@ -695,7 +695,7 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
>  }
>  #else
>  static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
> -			      struct acpi_gpio_mapping *gpios)
> +			      const struct acpi_gpio_mapping *gpios)
>  {
>  	return -ENXIO;
>  }

Thanks!

I folded your fixes into my patch and I'm going to send the result, with a
changelog, in a reply to this message.

If everyone is happy with it, I'll add it to the device properties patch
series as it depends on those.

I guess you can add a changelog to your rfkill changes and then we could
add them to that series too if there are no objections.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Provide a way for device drivers using GPIOs described by ACPI
GpioIo resources in _CRS to tell the GPIO subsystem what names
(connection IDs) to associate with specific GPIO pins defined
in there.

To do that, a driver needs to define a mapping table as a
NULL-terminated array of struct acpi_gpio_mapping objects
that each contain a name, a pointer to an array of pin data
(struct acpi_gpio_params) objects and the size of that array.

Each struct acpi_gpio_params object consists of three fields,
crs_entry_index, pin_index, active_low, representing the index of
the target GpioIo()/GpioInt() resource in _CRS starting from zero,
the index of the target pin in that resource starting from zero,
and the active-low flag for that pin, respectively.

Next, the mapping table needs to be passed as the second argument to
acpi_dev_add_driver_gpios() that will register it with the ACPI device
object pointed to by its first argument.  That object must represent
the ACPI namespace node containing the _CRS object referred to by the
GPIO mapping.  That should be done in the driver's .probe() routine.

On removal, the driver should unregister its GPIO mapping table
by calling acpi_dev_remove_driver_gpios() on the ACPI device
object where that table was previously registered.

Included are fixes from Mika Westerberg.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of the device-properties branch of the linux-pm.git tree at kernel.org.

---
 drivers/gpio/gpiolib-acpi.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 include/acpi/acpi_bus.h     |    3 +++
 include/linux/acpi.h        |   30 ++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -345,6 +345,8 @@ struct acpi_device_data {
 	const union acpi_object *of_compatible;
 };
 
+struct acpi_gpio_mapping;
+
 /* Device */
 struct acpi_device {
 	int device_type;
@@ -366,6 +368,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
+	const struct acpi_gpio_mapping *driver_gpios;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -672,6 +672,36 @@ do {									\
 #endif
 #endif
 
+struct acpi_gpio_params {
+	unsigned int crs_entry_index;
+	unsigned int pin_index;
+	bool active_low;
+};
+
+struct acpi_gpio_mapping {
+	const char *name;
+	const struct acpi_gpio_params *data;
+	unsigned int size;
+};
+
+#if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
+int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      const struct acpi_gpio_mapping *gpios);
+
+static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
+{
+	if (adev)
+		adev->driver_gpios = NULL;
+}
+#else
+static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      const struct acpi_gpio_mapping *gpios)
+{
+	return -ENXIO;
+}
+static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
+#endif
+
 /* Device properties */
 
 #define MAX_ACPI_REFERENCE_ARGS	8
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -287,6 +287,41 @@ void acpi_gpiochip_free_interrupts(struc
 	}
 }
 
+int acpi_dev_add_driver_gpios(struct acpi_device *adev,
+			      const struct acpi_gpio_mapping *gpios)
+{
+	if (adev && gpios) {
+		adev->driver_gpios = gpios;
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_add_driver_gpios);
+
+static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
+				      const char *name, int index,
+				      struct acpi_reference_args *args)
+{
+	const struct acpi_gpio_mapping *gm;
+
+	if (!adev->driver_gpios)
+		return false;
+
+	for (gm = adev->driver_gpios; gm->name; gm++)
+		if (!strcmp(name, gm->name) && gm->data && index < gm->size) {
+			const struct acpi_gpio_params *par = gm->data + index;
+
+			args->adev = adev;
+			args->args[0] = par->crs_entry_index;
+			args->args[1] = par->pin_index;
+			args->args[2] = par->active_low;
+			args->nargs = 3;
+			return true;
+		}
+
+	return false;
+}
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -372,8 +407,12 @@ struct gpio_desc *acpi_get_gpiod_by_inde
 		memset(&args, 0, sizeof(args));
 		ret = acpi_dev_get_property_reference(adev, propname, NULL,
 						      index, &args);
-		if (ret)
-			return ERR_PTR(ret);
+		if (ret) {
+			bool found = acpi_get_driver_gpio_data(adev, propname,
+							       index, &args);
+			if (!found)
+				return ERR_PTR(ret);
+		}
 
 		/*
 		 * The property was found and resolved so need to

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Sat, Oct 25, 2014 at 7:05 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Provide a way for device drivers using GPIOs described by ACPI
> GpioIo resources in _CRS to tell the GPIO subsystem what names
> (connection IDs) to associate with specific GPIO pins defined
> in there.
>
> To do that, a driver needs to define a mapping table as a
> NULL-terminated array of struct acpi_gpio_mapping objects
> that each contain a name, a pointer to an array of pin data
> (struct acpi_gpio_params) objects and the size of that array.
>
> Each struct acpi_gpio_params object consists of three fields,
> crs_entry_index, pin_index, active_low, representing the index of
> the target GpioIo()/GpioInt() resource in _CRS starting from zero,
> the index of the target pin in that resource starting from zero,
> and the active-low flag for that pin, respectively.
>
> Next, the mapping table needs to be passed as the second argument to
> acpi_dev_add_driver_gpios() that will register it with the ACPI device
> object pointed to by its first argument.  That object must represent
> the ACPI namespace node containing the _CRS object referred to by the
> GPIO mapping.  That should be done in the driver's .probe() routine.
>
> On removal, the driver should unregister its GPIO mapping table
> by calling acpi_dev_remove_driver_gpios() on the ACPI device
> object where that table was previously registered.
>
> Included are fixes from Mika Westerberg.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

As we discussed already, this is a great idea. The only thing that is
missing is a paragraph in Documentation/gpio/consumer.txt with an
explanation of the global mechanism and a simple example to illustrate
how and when this should be used.

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Sat, Oct 25, 2014 at 12:00:33AM +0200, Rafael J. Wysocki wrote:
> I folded your fixes into my patch and I'm going to send the result, with a
> changelog, in a reply to this message.

Thanks!

> If everyone is happy with it, I'll add it to the device properties patch
> series as it depends on those.
> 
> I guess you can add a changelog to your rfkill changes and then we could
> add them to that series too if there are no objections.

OK, I'll do that today.

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

On Monday, October 27, 2014 02:21:23 PM Alexandre Courbot wrote:
> On Sat, Oct 25, 2014 at 7:05 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Provide a way for device drivers using GPIOs described by ACPI
> > GpioIo resources in _CRS to tell the GPIO subsystem what names
> > (connection IDs) to associate with specific GPIO pins defined
> > in there.
> >
> > To do that, a driver needs to define a mapping table as a
> > NULL-terminated array of struct acpi_gpio_mapping objects
> > that each contain a name, a pointer to an array of pin data
> > (struct acpi_gpio_params) objects and the size of that array.
> >
> > Each struct acpi_gpio_params object consists of three fields,
> > crs_entry_index, pin_index, active_low, representing the index of
> > the target GpioIo()/GpioInt() resource in _CRS starting from zero,
> > the index of the target pin in that resource starting from zero,
> > and the active-low flag for that pin, respectively.
> >
> > Next, the mapping table needs to be passed as the second argument to
> > acpi_dev_add_driver_gpios() that will register it with the ACPI device
> > object pointed to by its first argument.  That object must represent
> > the ACPI namespace node containing the _CRS object referred to by the
> > GPIO mapping.  That should be done in the driver's .probe() routine.
> >
> > On removal, the driver should unregister its GPIO mapping table
> > by calling acpi_dev_remove_driver_gpios() on the ACPI device
> > object where that table was previously registered.
> >
> > Included are fixes from Mika Westerberg.
> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> As we discussed already, this is a great idea. The only thing that is
> missing is a paragraph in Documentation/gpio/consumer.txt with an
> explanation of the global mechanism and a simple example to illustrate
> how and when this should be used.

Yes, I'm going to provide documentation.

One question, though.  We're already adding gpio-properties.txt under
Documentation/acpi/ containing analogous information for _DSD-based
mappings.  I thought I'd add a section to that file and a short paragraph
pointing to it into consumer.txt, would that work?

Rafael

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Tue, Oct 28, 2014 at 7:34 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, October 27, 2014 02:21:23 PM Alexandre Courbot wrote:
>> On Sat, Oct 25, 2014 at 7:05 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > Provide a way for device drivers using GPIOs described by ACPI
>> > GpioIo resources in _CRS to tell the GPIO subsystem what names
>> > (connection IDs) to associate with specific GPIO pins defined
>> > in there.
>> >
>> > To do that, a driver needs to define a mapping table as a
>> > NULL-terminated array of struct acpi_gpio_mapping objects
>> > that each contain a name, a pointer to an array of pin data
>> > (struct acpi_gpio_params) objects and the size of that array.
>> >
>> > Each struct acpi_gpio_params object consists of three fields,
>> > crs_entry_index, pin_index, active_low, representing the index of
>> > the target GpioIo()/GpioInt() resource in _CRS starting from zero,
>> > the index of the target pin in that resource starting from zero,
>> > and the active-low flag for that pin, respectively.
>> >
>> > Next, the mapping table needs to be passed as the second argument to
>> > acpi_dev_add_driver_gpios() that will register it with the ACPI device
>> > object pointed to by its first argument.  That object must represent
>> > the ACPI namespace node containing the _CRS object referred to by the
>> > GPIO mapping.  That should be done in the driver's .probe() routine.
>> >
>> > On removal, the driver should unregister its GPIO mapping table
>> > by calling acpi_dev_remove_driver_gpios() on the ACPI device
>> > object where that table was previously registered.
>> >
>> > Included are fixes from Mika Westerberg.
>>
>> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> As we discussed already, this is a great idea. The only thing that is
>> missing is a paragraph in Documentation/gpio/consumer.txt with an
>> explanation of the global mechanism and a simple example to illustrate
>> how and when this should be used.
>
> Yes, I'm going to provide documentation.
>
> One question, though.  We're already adding gpio-properties.txt under
> Documentation/acpi/ containing analogous information for _DSD-based
> mappings.  I thought I'd add a section to that file and a short paragraph
> pointing to it into consumer.txt, would that work?

Definitely yes, since this is ACPI-specific. Thanks!

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

On Tuesday, October 28, 2014 01:11:50 PM Alexandre Courbot wrote:
> On Tue, Oct 28, 2014 at 7:34 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, October 27, 2014 02:21:23 PM Alexandre Courbot wrote:

[cut]

> > Yes, I'm going to provide documentation.
> >
> > One question, though.  We're already adding gpio-properties.txt under
> > Documentation/acpi/ containing analogous information for _DSD-based
> > mappings.  I thought I'd add a section to that file and a short paragraph
> > pointing to it into consumer.txt, would that work?
> 
> Definitely yes, since this is ACPI-specific. Thanks!

OK, so what about this (on top of the device-properties branch at
http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties):

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / GPIO: Document ACPI GPIO mappings API

Document the previously introduced method that can be used by device
drivers to provide the GPIO subsystem with mappings between GPIO names
(connection IDs) and GpioIo()/GpioInt() resources in _CRS.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/acpi/gpio-properties.txt |   44 +++++++++++++++++++++++++++++++++
 Documentation/gpio/consumer.txt        |   18 +++++++++++++
 2 files changed, 62 insertions(+)

Index: linux-pm/Documentation/acpi/gpio-properties.txt
===================================================================
--- linux-pm.orig/Documentation/acpi/gpio-properties.txt
+++ linux-pm/Documentation/acpi/gpio-properties.txt
@@ -50,3 +50,47 @@ it to 1 marks the GPIO as active low.
 
 In our Bluetooth example the "reset-gpio" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
+
+ACPI GPIO Mappings Provided by Drivers
+--------------------------------------
+
+There are systems in which the ACPI tables do not contain _DSD but provide _CRS
+with GpioIo()/GpioInt() resources and device drivers still need to work with
+them.
+
+In those cases ACPI device identification objects, _HID, _CID, _CLS, _SUB, _HRV,
+available to the driver can be used to identify the device and that is supposed
+to be sufficient to determine the meaning and purpose of all of the GPIO pins
+listed by the GpioIo()/GpioInt() resources returned by _CRS.  In other words,
+the driver is supposed to know what to use the GpioIo()/GpioInt() resources for
+once it has identified the device.  Having done that, it can simply assign names
+to the GPIO pins it is going to use and provide the GPIO subsystem with a
+mapping between those names and the ACPI GPIO resources corresponding to them.
+
+To do that, the driver needs to define a mapping table as a NULL-terminated
+array of struct acpi_gpio_mapping objects that each contain a name, a pointer
+to an array of pin data (struct acpi_gpio_params) objects and the size of that
+array.  Each struct acpi_gpio_params object consists of three fields,
+crs_entry_index, pin_index, active_low, representing the index of the target
+GpioIo()/GpioInt() resource in _CRS starting from zero, the index of the target
+pin in that resource starting from zero, and the active-low flag for that pin,
+respectively, in analogy with the _DSD GPIO property format specified above.
+
+For the example Bluetooth device discussed previously the data structures in
+question would look like this:
+
+static const struct acpi_gpio_params reset_gpio = { 1, 1, false };
+static const struct acpi_gpio_params shutdown_gpio = { 0, 0, false };
+
+static const struct acpi_gpio_mapping bluetooth_acpi_gpios[] = {
+  { "reset-gpio", &reset_gpio, 1 },
+  { "shutdown-gpio", &shutdown_gpio, 1 },
+  { },
+};
+
+Next, the mapping table needs to be passed as the second argument to
+acpi_dev_add_driver_gpios() that will register it with the ACPI device object
+pointed to by its first argument.  That should be done in the driver's .probe()
+routine.  On removal, the driver should unregister its GPIO mapping table by
+calling acpi_dev_remove_driver_gpios() on the ACPI device object where that
+table was previously registered.
Index: linux-pm/Documentation/gpio/consumer.txt
===================================================================
--- linux-pm.orig/Documentation/gpio/consumer.txt
+++ linux-pm/Documentation/gpio/consumer.txt
@@ -219,6 +219,24 @@ part of the IRQ interface, e.g. IRQF_TRI
 capabilities.
 
 
+GPIOs and ACPI
+==============
+
+On ACPI systems, GPIOs are described by GpioIo()/GpioInt() resources listed by
+the _CRS configuration objects of devices.  Those resources do not provide
+connection IDs (names) for GPIOs, so it is necessary to use an additional
+mechanism for this purpose.
+
+Systems compliant with ACPI 5.1 or newer may provide a _DSD configuration object
+which, among other things, may be used to provide connection IDs for specific
+GPIOs described by the GpioIo()/GpioInt() resources in _CRS.  If that is the
+case, it will be handled by the GPIO subsystem automatically.  However, if the
+_DSD is not present, the mappings between GpioIo()/GpioInt() resources and GPIO
+connection IDs need to be provided by device drivers.
+
+For details refer to Documentation/acpi/gpio-properties.txt
+
+
 Interacting With the Legacy GPIO Subsystem
 ==========================================
 Many kernel subsystems still handle GPIOs using the legacy integer-based

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Mika Westerberg]

On Thu, Oct 30, 2014 at 01:45:09AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / GPIO: Document ACPI GPIO mappings API
> 
> Document the previously introduced method that can be used by device
> drivers to provide the GPIO subsystem with mappings between GPIO names
> (connection IDs) and GpioIo()/GpioInt() resources in _CRS.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Looks really good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Linus Walleij]

On Thu, Oct 23, 2014 at 2:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 23 October 2014 15:02:46 Alexandre Courbot wrote:
>> On Tue, Oct 21, 2014 at 4:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 21 October 2014 14:14:02 Alexandre Courbot wrote:
>
>> > Drivers that use
>> > existing bindings with the "foo-gpio" form (or worse, "foo-somethingelse"
>> > can use the same internal interface as the drivers that use name plus
>> > index. Do you see a problem using what I suggested for the combined
>> > API:
>> >
>> > __gpiod_get(dev, propname, index); // use property name plus index
>> > gpiod_get(dev, index); // use "gpios" plus index
>> > gpiod_get_named(dev, "name"); use "name-gpios" with index 0
>>
>> Apart from the loosy naming practices which we sometimes see (and
>> which should be caught during review), do you have something against
>> requiring a name for all new GPIO bindings, i.e. for ensuring that all
>> new properties are "name-gpio" and forbidding "gpios"?
>
> Most other subsystems don't require a name, and traditionally we only
> had anonymous indexed properties for a lot of things (registers,
> interrupts, ...).
>
> I still like the idea of using anonymous references for simple things,
> but if you and Linus feel that it's better to mandate names from now on,
> I won't complain.

I think names are nice since it appears in <debugfs>/gpio and makes
device tree using platforms easier to debug.

Moving things into DT already made information hard to follow at
times...

In difference from other things like IRQs or DMAs, GPIOs very often
have a very specific function, does not repeat itself, and thus can
be given a suitable name.

Yours,
Linus Walleij

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Linus Walleij]

On Sat, Oct 25, 2014 at 12:05 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Provide a way for device drivers using GPIOs described by ACPI
> GpioIo resources in _CRS to tell the GPIO subsystem what names
> (connection IDs) to associate with specific GPIO pins defined
> in there.
>
> To do that, a driver needs to define a mapping table as a
> NULL-terminated array of struct acpi_gpio_mapping objects
> that each contain a name, a pointer to an array of pin data
> (struct acpi_gpio_params) objects and the size of that array.
>
> Each struct acpi_gpio_params object consists of three fields,
> crs_entry_index, pin_index, active_low, representing the index of

I prefer that you call the second thing "line_index" rather than
"pin_index". Not all GPIOs in the world are tied to physical pins.
Also it is easy to confuse stuff with the pin control terminology
if it's called "pin".

> the target GpioIo()/GpioInt() resource in _CRS starting from zero,
> the index of the target pin in that resource starting from zero,
> and the active-low flag for that pin, respectively.
>
> Next, the mapping table needs to be passed as the second argument to
> acpi_dev_add_driver_gpios() that will register it with the ACPI device
> object pointed to by its first argument.  That object must represent
> the ACPI namespace node containing the _CRS object referred to by the
> GPIO mapping.  That should be done in the driver's .probe() routine.
>
> On removal, the driver should unregister its GPIO mapping table
> by calling acpi_dev_remove_driver_gpios() on the ACPI device
> object where that table was previously registered.
>
> Included are fixes from Mika Westerberg.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

And "pin" is mentioned several times in the commit message.
I prefer that we talk about "lines".

(...)

> +struct acpi_gpio_params {
> +       unsigned int crs_entry_index;
> +       unsigned int pin_index;

So line_index;

> +       bool active_low;
> +};

With that change:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:
> On Sat, Oct 25, 2014 at 12:05 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Provide a way for device drivers using GPIOs described by ACPI
> > GpioIo resources in _CRS to tell the GPIO subsystem what names
> > (connection IDs) to associate with specific GPIO pins defined
> > in there.
> >
> > To do that, a driver needs to define a mapping table as a
> > NULL-terminated array of struct acpi_gpio_mapping objects
> > that each contain a name, a pointer to an array of pin data
> > (struct acpi_gpio_params) objects and the size of that array.
> >
> > Each struct acpi_gpio_params object consists of three fields,
> > crs_entry_index, pin_index, active_low, representing the index of
> 
> I prefer that you call the second thing "line_index" rather than
> "pin_index". Not all GPIOs in the world are tied to physical pins.
> Also it is easy to confuse stuff with the pin control terminology
> if it's called "pin".
> 
> > the target GpioIo()/GpioInt() resource in _CRS starting from zero,
> > the index of the target pin in that resource starting from zero,
> > and the active-low flag for that pin, respectively.
> >
> > Next, the mapping table needs to be passed as the second argument to
> > acpi_dev_add_driver_gpios() that will register it with the ACPI device
> > object pointed to by its first argument.  That object must represent
> > the ACPI namespace node containing the _CRS object referred to by the
> > GPIO mapping.  That should be done in the driver's .probe() routine.
> >
> > On removal, the driver should unregister its GPIO mapping table
> > by calling acpi_dev_remove_driver_gpios() on the ACPI device
> > object where that table was previously registered.
> >
> > Included are fixes from Mika Westerberg.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> And "pin" is mentioned several times in the commit message.
> I prefer that we talk about "lines".
> 
> (...)
> 
> > +struct acpi_gpio_params {
> > +       unsigned int crs_entry_index;
> > +       unsigned int pin_index;
> 
> So line_index;
> 
> > +       bool active_low;
> > +};
> 
> With that change:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

OK, made the changes and added your Reviewed-by.

One semi-related question though.  Alexandre ACKed the patch before, so
what did that mean from the GPIO subsystem's perspective?  Was the patch
approved, not approved, semi-approved?

Rafael

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Linus Walleij]

On Mon, Nov 3, 2014 at 11:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:

>> With that change:
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> OK, made the changes and added your Reviewed-by.
>
> One semi-related question though.  Alexandre ACKed the patch before, so
> what did that mean from the GPIO subsystem's perspective?  Was the patch
> approved, not approved, semi-approved?

It just means we are not always entirely aligned, it's not like
Alexandre and I have meetings on the side and decide what to
say, it's just these mails. I do trust him, I wouldn't get mad if it
had been merged just wanted some polishing when I could get
it...

Yours,
Linus Walleij

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

On Friday, November 14, 2014 09:58:41 AM Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 11:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:
> 
> >> With that change:
> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > OK, made the changes and added your Reviewed-by.
> >
> > One semi-related question though.  Alexandre ACKed the patch before, so
> > what did that mean from the GPIO subsystem's perspective?  Was the patch
> > approved, not approved, semi-approved?
> 
> It just means we are not always entirely aligned, it's not like
> Alexandre and I have meetings on the side and decide what to
> say, it's just these mails. I do trust him, I wouldn't get mad if it
> had been merged just wanted some polishing when I could get
> it...

I see, thanks!

Rafael

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Fri, Nov 14, 2014 at 5:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Nov 3, 2014 at 11:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:
>
>>> With that change:
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> OK, made the changes and added your Reviewed-by.
>>
>> One semi-related question though.  Alexandre ACKed the patch before, so
>> what did that mean from the GPIO subsystem's perspective?  Was the patch
>> approved, not approved, semi-approved?
>
> It just means we are not always entirely aligned, it's not like
> Alexandre and I have meetings on the side and decide what to
> say, it's just these mails. I do trust him, I wouldn't get mad if it
> had been merged just wanted some polishing when I could get
> it...

In case of conflicting feedback between me and Linus, it goes without
saying that Linus' opinion should prevail. I am trying to help a bit
and take some of the burden off his shoulders, but have neither his
skill nor his experience (which shows when you compare our patch
reviews). So for important stuff it is a good idea to wait until both
of us give a ack - the most likely conflict scenario being that I give
a pass overlooking something that Linus will catch.

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Rafael J. Wysocki]

On Monday, November 17, 2014 01:28:54 PM Alexandre Courbot wrote:
> On Fri, Nov 14, 2014 at 5:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Nov 3, 2014 at 11:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:
> >
> >>> With that change:
> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>
> >> OK, made the changes and added your Reviewed-by.
> >>
> >> One semi-related question though.  Alexandre ACKed the patch before, so
> >> what did that mean from the GPIO subsystem's perspective?  Was the patch
> >> approved, not approved, semi-approved?
> >
> > It just means we are not always entirely aligned, it's not like
> > Alexandre and I have meetings on the side and decide what to
> > say, it's just these mails. I do trust him, I wouldn't get mad if it
> > had been merged just wanted some polishing when I could get
> > it...
> 
> In case of conflicting feedback between me and Linus, it goes without
> saying that Linus' opinion should prevail. I am trying to help a bit
> and take some of the burden off his shoulders, but have neither his
> skill nor his experience (which shows when you compare our patch
> reviews). So for important stuff it is a good idea to wait until both
> of us give a ack - the most likely conflict scenario being that I give
> a pass overlooking something that Linus will catch.

Thanks for the clarification!

Rafael