[PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Andy Shevchenko]

While using GPIO library API we might get into troubles in the future,
because we can't rely on label name in the driver since vendor firmware
might provide any GPIO pin there, e.g. "reset", and even mark it in _DSD
(in which case the request will fail).

To avoid inconsistency and potential issues we have two options:
a) generate GPIO ACPI mapping table and supply it via
acpi_dev_add_driver_gpios(), or
b) just pass NULL as connection ID.

The b) approach is much simpler and would work since the driver relies
on GPIO indices only. Moreover, the _CRS fallback mechanism, when
requesting GPIO, is going to be stricter, and supplying non-NULL
connection ID when neither _DSD, nor GPIO ACPI mapping is present, will
make request fail.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/mma9551.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index bf2704435629..1f53f08476f5 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -27,7 +27,6 @@
 
 #define MMA9551_DRV_NAME		"mma9551"
 #define MMA9551_IRQ_NAME		"mma9551_event"
-#define MMA9551_GPIO_NAME		"mma9551_int"
 #define MMA9551_GPIO_COUNT		4
 
 /* Tilt application (inclination in IIO terms). */
@@ -418,8 +417,7 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
 	struct device *dev = &data->client->dev;
 
 	for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
-		gpio = devm_gpiod_get_index(dev, MMA9551_GPIO_NAME, i,
-					    GPIOD_IN);
+		gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
 		if (IS_ERR(gpio)) {
 			dev_err(dev, "acpi gpio get index failed\n");
 			return PTR_ERR(gpio);
-- 
2.11.0

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Jonathan Cameron]

On Sat, 10 Jun 2017 21:58:41 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> While using GPIO library API we might get into troubles in the future,
> because we can't rely on label name in the driver since vendor firmware
> might provide any GPIO pin there, e.g. "reset", and even mark it in _DSD
> (in which case the request will fail).
> 
> To avoid inconsistency and potential issues we have two options:
> a) generate GPIO ACPI mapping table and supply it via
> acpi_dev_add_driver_gpios(), or
> b) just pass NULL as connection ID.
> 
> The b) approach is much simpler and would work since the driver relies
> on GPIO indices only. Moreover, the _CRS fallback mechanism, when
> requesting GPIO, is going to be stricter, and supplying non-NULL
> connection ID when neither _DSD, nor GPIO ACPI mapping is present, will
> make request fail.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi Andy,

A bit of googling suggests that this is one of a number of similar patches.
I'd have appreciated some cross references in here as otherwise it seems
to have come out of nowhere. 

Also, I'd be happier with an Ack from Linus Walleij on a change like
this to confirm the method (a reference to him saying it was fine
elsewhere would be fine as well!)

Anyhow I've cc'd Linus in the meantime.

Thanks,

Jonathan
> ---
>  drivers/iio/accel/mma9551.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index bf2704435629..1f53f08476f5 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -27,7 +27,6 @@
>  
>  #define MMA9551_DRV_NAME		"mma9551"
>  #define MMA9551_IRQ_NAME		"mma9551_event"
> -#define MMA9551_GPIO_NAME		"mma9551_int"
>  #define MMA9551_GPIO_COUNT		4
>  
>  /* Tilt application (inclination in IIO terms). */
> @@ -418,8 +417,7 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>  	struct device *dev = &data->client->dev;
>  
>  	for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
> -		gpio = devm_gpiod_get_index(dev, MMA9551_GPIO_NAME, i,
> -					    GPIOD_IN);
> +		gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
>  		if (IS_ERR(gpio)) {
>  			dev_err(dev, "acpi gpio get index failed\n");
>  			return PTR_ERR(gpio);

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Andy Shevchenko]

On Sun, 2017-06-11 at 11:46 +0100, Jonathan Cameron wrote:
> On Sat, 10 Jun 2017 21:58:41 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > While using GPIO library API we might get into troubles in the
> > future,
> > because we can't rely on label name in the driver since vendor
> > firmware
> > might provide any GPIO pin there, e.g. "reset", and even mark it in
> > _DSD
> > (in which case the request will fail).
> > 
> > To avoid inconsistency and potential issues we have two options:
> > a) generate GPIO ACPI mapping table and supply it via
> > acpi_dev_add_driver_gpios(), or
> > b) just pass NULL as connection ID.
> > 
> > The b) approach is much simpler and would work since the driver
> > relies
> > on GPIO indices only. Moreover, the _CRS fallback mechanism, when
> > requesting GPIO, is going to be stricter, and supplying non-NULL
> > connection ID when neither _DSD, nor GPIO ACPI mapping is present,
> > will
> > make request fail.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Hi Andy,
> 
> A bit of googling suggests that this is one of a number of similar
> patches.
> I'd have appreciated some cross references in here as otherwise it
> seems
> to have come out of nowhere. 

I think the best is to read a documentation portion patch from here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/co
mmit/?h=for-next&id=ed7fcf1ed5ea4ea01243995ae085757a77cf0f3e

> 
> Also, I'd be happier with an Ack from Linus Walleij on a change like
> this to confirm the method (a reference to him saying it was fine
> elsewhere would be fine as well!)
> 
> Anyhow I've cc'd Linus in the meantime.

Sure.

> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/accel/mma9551.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/mma9551.c
> > b/drivers/iio/accel/mma9551.c
> > index bf2704435629..1f53f08476f5 100644
> > --- a/drivers/iio/accel/mma9551.c
> > +++ b/drivers/iio/accel/mma9551.c
> > @@ -27,7 +27,6 @@
> >  
> >  #define MMA9551_DRV_NAME		"mma9551"
> >  #define MMA9551_IRQ_NAME		"mma9551_event"
> > -#define MMA9551_GPIO_NAME		"mma9551_int"
> >  #define MMA9551_GPIO_COUNT		4
> >  
> >  /* Tilt application (inclination in IIO terms). */
> > @@ -418,8 +417,7 @@ static int mma9551_gpio_probe(struct iio_dev
> > *indio_dev)
> >  	struct device *dev = &data->client->dev;
> >  
> >  	for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
> > -		gpio = devm_gpiod_get_index(dev, MMA9551_GPIO_NAME,
> > i,
> > -					    GPIOD_IN);
> > +		gpio = devm_gpiod_get_index(dev, NULL, i,
> > GPIOD_IN);
> >  		if (IS_ERR(gpio)) {
> >  			dev_err(dev, "acpi gpio get index
> > failed\n");
> >  			return PTR_ERR(gpio);
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Jonathan Cameron]

On Sun, 11 Jun 2017 17:25:54 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, 2017-06-11 at 11:46 +0100, Jonathan Cameron wrote:
> > On Sat, 10 Jun 2017 21:58:41 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> > > While using GPIO library API we might get into troubles in the
> > > future,
> > > because we can't rely on label name in the driver since vendor
> > > firmware
> > > might provide any GPIO pin there, e.g. "reset", and even mark it in
> > > _DSD
> > > (in which case the request will fail).
> > > 
> > > To avoid inconsistency and potential issues we have two options:
> > > a) generate GPIO ACPI mapping table and supply it via
> > > acpi_dev_add_driver_gpios(), or
> > > b) just pass NULL as connection ID.
> > > 
> > > The b) approach is much simpler and would work since the driver
> > > relies
> > > on GPIO indices only. Moreover, the _CRS fallback mechanism, when
> > > requesting GPIO, is going to be stricter, and supplying non-NULL
> > > connection ID when neither _DSD, nor GPIO ACPI mapping is present,
> > > will
> > > make request fail.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> > 
> > Hi Andy,
> > 
> > A bit of googling suggests that this is one of a number of similar
> > patches.
> > I'd have appreciated some cross references in here as otherwise it
> > seems
> > to have come out of nowhere.   
> 
> I think the best is to read a documentation portion patch from here:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/co
> mmit/?h=for-next&id=ed7fcf1ed5ea4ea01243995ae085757a77cf0f3e
Cool. I think I follow what is going on now.

We don't need the string and it just makes life nasty for ACPI. Fair
enough.  Applied to the togreg branch of iio.git with that link added
and pushed out as testing for the autobuilders to play with it.

Still time if Linus want's to add anything.

Jonathan
> 
> > 
> > Also, I'd be happier with an Ack from Linus Walleij on a change like
> > this to confirm the method (a reference to him saying it was fine
> > elsewhere would be fine as well!)
> > 
> > Anyhow I've cc'd Linus in the meantime.  
> 
> Sure.
> 
> > 
> > Thanks,
> > 
> > Jonathan  
> > > ---
> > >  drivers/iio/accel/mma9551.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/accel/mma9551.c
> > > b/drivers/iio/accel/mma9551.c
> > > index bf2704435629..1f53f08476f5 100644
> > > --- a/drivers/iio/accel/mma9551.c
> > > +++ b/drivers/iio/accel/mma9551.c
> > > @@ -27,7 +27,6 @@
> > >  
> > >  #define MMA9551_DRV_NAME		"mma9551"
> > >  #define MMA9551_IRQ_NAME		"mma9551_event"
> > > -#define MMA9551_GPIO_NAME		"mma9551_int"
> > >  #define MMA9551_GPIO_COUNT		4
> > >  
> > >  /* Tilt application (inclination in IIO terms). */
> > > @@ -418,8 +417,7 @@ static int mma9551_gpio_probe(struct iio_dev
> > > *indio_dev)
> > >  	struct device *dev = &data->client->dev;
> > >  
> > >  	for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
> > > -		gpio = devm_gpiod_get_index(dev, MMA9551_GPIO_NAME,
> > > i,
> > > -					    GPIOD_IN);
> > > +		gpio = devm_gpiod_get_index(dev, NULL, i,
> > > GPIOD_IN);
> > >  		if (IS_ERR(gpio)) {
> > >  			dev_err(dev, "acpi gpio get index
> > > failed\n");
> > >  			return PTR_ERR(gpio);  
> > 
> >   
> 

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Linus Walleij]

On Sun, Jun 11, 2017 at 5:08 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

>> I think the best is to read a documentation portion patch from here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/co
>> mmit/?h=for-next&id=ed7fcf1ed5ea4ea01243995ae085757a77cf0f3e
>
> Cool. I think I follow what is going on now.
>
> We don't need the string and it just makes life nasty for ACPI. Fair
> enough.  Applied to the togreg branch of iio.git with that link added
> and pushed out as testing for the autobuilders to play with it.
>
> Still time if Linus want's to add anything.

It's fine. I have a very high level of trust about Andy's patches and I
know he's always doing his best to make ACPI play well with GPIO.
AFICT this is the best thing to do, but honestly, I see Andy and
Mika Westerberg as the true GPIO-ACPI maintainers.

Yours,
Linus Walleij

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Andy Shevchenko]

On Sun, 2017-06-11 at 19:45 +0200, Linus Walleij wrote:
> On Sun, Jun 11, 2017 at 5:08 PM, Jonathan Cameron <jic23@kernel.org>
> wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > I think the best is to read a documentation portion patch from
> > > here:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.
> > > git/co
> > > mmit/?h=for-next&id=ed7fcf1ed5ea4ea01243995ae085757a77cf0f3e
> > 
> > Cool. I think I follow what is going on now.
> > 
> > We don't need the string and it just makes life nasty for ACPI. Fair
> > enough.  Applied to the togreg branch of iio.git with that link
> > added
> > and pushed out as testing for the autobuilders to play with it.
> > 
> > Still time if Linus want's to add anything.
> 
> It's fine. I have a very high level of trust about Andy's patches and
> I
> know he's always doing his best to make ACPI play well with GPIO.
> AFICT this is the best thing to do, but honestly, I see Andy and
> Mika Westerberg as the true GPIO-ACPI maintainers.
> 

By the way, we may add ourselves (still need Mika's Ack on this) to a
separate record in MAINTEINERS. It might make your burden less heavy.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Linus Walleij]

On Sun, Jun 11, 2017 at 9:39 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sun, 2017-06-11 at 19:45 +0200, Linus Walleij wrote:
>> On Sun, Jun 11, 2017 at 5:08 PM, Jonathan Cameron <jic23@kernel.org>
>> wrote:
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > > I think the best is to read a documentation portion patch from
>> > > here:
>> > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.
>> > > git/co
>> > > mmit/?h=for-next&id=ed7fcf1ed5ea4ea01243995ae085757a77cf0f3e
>> >
>> > Cool. I think I follow what is going on now.
>> >
>> > We don't need the string and it just makes life nasty for ACPI. Fair
>> > enough.  Applied to the togreg branch of iio.git with that link
>> > added
>> > and pushed out as testing for the autobuilders to play with it.
>> >
>> > Still time if Linus want's to add anything.
>>
>> It's fine. I have a very high level of trust about Andy's patches and
>> I
>> know he's always doing his best to make ACPI play well with GPIO.
>> AFICT this is the best thing to do, but honestly, I see Andy and
>> Mika Westerberg as the true GPIO-ACPI maintainers.
>>
>
> By the way, we may add ourselves (still need Mika's Ack on this) to a
> separate record in MAINTEINERS. It might make your burden less heavy.

Thanks, appreciated. Would be super if that covers gpiolib-acpi.c.

Also maybe time that you folks volunteer form some drivers/firmware
and drivers/platform/x86 business, eh ;)

Yours,
Linus Walleij

Re: [PATCH v1] iio: accel: mma9551: use NULL for GPIO connection ID

[Mika Westerberg]

On Sun, Jun 11, 2017 at 11:37:12PM +0200, Linus Walleij wrote:
> > By the way, we may add ourselves (still need Mika's Ack on this) to a
> > separate record in MAINTEINERS. It might make your burden less heavy.
> 
> Thanks, appreciated. Would be super if that covers gpiolib-acpi.c.

Works for me, ack ;-)