[PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

On some x86 tablets we cannot directly access the GPIOs as they are
claimed by the ACPI tables, so check it the i2c client is not being
power-managed by ACPI before trying to get the power pin GPIO.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Check acpi_bus_power_manageable() instead of trying to directly
 control the acpi power level ourselves
---
 drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 404830a..2fbcd7f 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -31,6 +31,7 @@
 #include <linux/irq.h>
 #include <linux/regulator/consumer.h>
 
+#include <acpi/acpi_bus.h>
 #include <asm/unaligned.h>
 
 #define SILEAD_TS_NAME		"silead_ts"
@@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	/* Power GPIO pin */
-	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
-	if (IS_ERR(data->gpio_power)) {
-		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
-			dev_err(dev, "Shutdown GPIO request failed\n");
-		return PTR_ERR(data->gpio_power);
+	/*
+	 * If device power is not managed by ACPI, get the power_gpio
+	 * and manage it ourselves.
+	 */
+#ifdef CONFIG_ACPI
+	if (!acpi_bus_power_manageable(ACPI_HANDLE(dev)))
+#endif
+	{
+		data->gpio_power = devm_gpiod_get_optional(dev, "power",
+							   GPIOD_OUT_LOW);
+		if (IS_ERR(data->gpio_power)) {
+			if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
+				dev_err(dev, "Power GPIO request failed\n");
+			return PTR_ERR(data->gpio_power);
+		}
 	}
 
 	error = silead_ts_setup(client);
-- 
2.9.3

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Dmitry Torokhov]

On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
> On some x86 tablets we cannot directly access the GPIOs as they are
> claimed by the ACPI tables, so check it the i2c client is not being
> power-managed by ACPI before trying to get the power pin GPIO.

Why do we even get this GPIO if driver is not supposed to be using it?
I'd much rather gpio provider hid it from the driver instead of every
driver having this check.

Thanks.

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Check acpi_bus_power_manageable() instead of trying to directly
>  control the acpi power level ourselves
> ---
>  drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> index 404830a..2fbcd7f 100644
> --- a/drivers/input/touchscreen/silead.c
> +++ b/drivers/input/touchscreen/silead.c
> @@ -31,6 +31,7 @@
>  #include <linux/irq.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <acpi/acpi_bus.h>
>  #include <asm/unaligned.h>
>  
>  #define SILEAD_TS_NAME		"silead_ts"
> @@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client,
>  	if (error)
>  		return error;
>  
> -	/* Power GPIO pin */
> -	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
> -	if (IS_ERR(data->gpio_power)) {
> -		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
> -			dev_err(dev, "Shutdown GPIO request failed\n");
> -		return PTR_ERR(data->gpio_power);
> +	/*
> +	 * If device power is not managed by ACPI, get the power_gpio
> +	 * and manage it ourselves.
> +	 */
> +#ifdef CONFIG_ACPI
> +	if (!acpi_bus_power_manageable(ACPI_HANDLE(dev)))
> +#endif
> +	{
> +		data->gpio_power = devm_gpiod_get_optional(dev, "power",
> +							   GPIOD_OUT_LOW);
> +		if (IS_ERR(data->gpio_power)) {
> +			if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
> +				dev_err(dev, "Power GPIO request failed\n");
> +			return PTR_ERR(data->gpio_power);
> +		}
>  	}
>  
>  	error = silead_ts_setup(client);
> -- 
> 2.9.3
> 

-- 
Dmitry

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 22-01-17 23:20, Dmitry Torokhov wrote:
> On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
>> On some x86 tablets we cannot directly access the GPIOs as they are
>> claimed by the ACPI tables, so check it the i2c client is not being
>> power-managed by ACPI before trying to get the power pin GPIO.
>
> Why do we even get this GPIO if driver is not supposed to be using it?
> I'd much rather gpio provider hid it from the driver instead of every
> driver having this check.

The problem is that the gpio subsys does not really know about ACPI
managed GPIOs the way this works is that the firmware sets a special
"reserved for firmware use" bit in the gpio control register and
directly bit-bangs the gpio control register when it wants to toggle
the gpio. So there is no awareness of these gpios being reserved
(as gpios) at the ACPI level AFAICT.

The hardware specific low-level gpio chip driver checks this bit
when we request the gpio and returns -EBUSY.

Regards,

Hans






>
> Thanks.
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Check acpi_bus_power_manageable() instead of trying to directly
>>  control the acpi power level ourselves
>> ---
>>  drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------
>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
>> index 404830a..2fbcd7f 100644
>> --- a/drivers/input/touchscreen/silead.c
>> +++ b/drivers/input/touchscreen/silead.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/regulator/consumer.h>
>>
>> +#include <acpi/acpi_bus.h>
>>  #include <asm/unaligned.h>
>>
>>  #define SILEAD_TS_NAME		"silead_ts"
>> @@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client,
>>  	if (error)
>>  		return error;
>>
>> -	/* Power GPIO pin */
>> -	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
>> -	if (IS_ERR(data->gpio_power)) {
>> -		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
>> -			dev_err(dev, "Shutdown GPIO request failed\n");
>> -		return PTR_ERR(data->gpio_power);
>> +	/*
>> +	 * If device power is not managed by ACPI, get the power_gpio
>> +	 * and manage it ourselves.
>> +	 */
>> +#ifdef CONFIG_ACPI
>> +	if (!acpi_bus_power_manageable(ACPI_HANDLE(dev)))
>> +#endif
>> +	{
>> +		data->gpio_power = devm_gpiod_get_optional(dev, "power",
>> +							   GPIOD_OUT_LOW);
>> +		if (IS_ERR(data->gpio_power)) {
>> +			if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
>> +				dev_err(dev, "Power GPIO request failed\n");
>> +			return PTR_ERR(data->gpio_power);
>> +		}
>>  	}
>>
>>  	error = silead_ts_setup(client);
>> --
>> 2.9.3
>>
>

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Dmitry Torokhov]

On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 22-01-17 23:20, Dmitry Torokhov wrote:
> >On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
> >>On some x86 tablets we cannot directly access the GPIOs as they are
> >>claimed by the ACPI tables, so check it the i2c client is not being
> >>power-managed by ACPI before trying to get the power pin GPIO.
> >
> >Why do we even get this GPIO if driver is not supposed to be using it?
> >I'd much rather gpio provider hid it from the driver instead of every
> >driver having this check.
> 
> The problem is that the gpio subsys does not really know about ACPI
> managed GPIOs the way this works is that the firmware sets a special
> "reserved for firmware use" bit in the gpio control register and
> directly bit-bangs the gpio control register when it wants to toggle
> the gpio. So there is no awareness of these gpios being reserved
> (as gpios) at the ACPI level AFAICT.
> 
> The hardware specific low-level gpio chip driver checks this bit
> when we request the gpio and returns -EBUSY.

I'd say that, if GPIOs are reserved for firmware use, and kernel should
not or can not touch them, then they should not be visible, if not to
the gpio core, then to consumers for sure.

Let's add Mika and Linus.

Thanks.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> >
> >Thanks.
> >
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>Changes in v2:
> >>-Check acpi_bus_power_manageable() instead of trying to directly
> >> control the acpi power level ourselves
> >>---
> >> drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------
> >> 1 file changed, 16 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> >>index 404830a..2fbcd7f 100644
> >>--- a/drivers/input/touchscreen/silead.c
> >>+++ b/drivers/input/touchscreen/silead.c
> >>@@ -31,6 +31,7 @@
> >> #include <linux/irq.h>
> >> #include <linux/regulator/consumer.h>
> >>
> >>+#include <acpi/acpi_bus.h>
> >> #include <asm/unaligned.h>
> >>
> >> #define SILEAD_TS_NAME		"silead_ts"
> >>@@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client,
> >> 	if (error)
> >> 		return error;
> >>
> >>-	/* Power GPIO pin */
> >>-	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
> >>-	if (IS_ERR(data->gpio_power)) {
> >>-		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
> >>-			dev_err(dev, "Shutdown GPIO request failed\n");
> >>-		return PTR_ERR(data->gpio_power);
> >>+	/*
> >>+	 * If device power is not managed by ACPI, get the power_gpio
> >>+	 * and manage it ourselves.
> >>+	 */
> >>+#ifdef CONFIG_ACPI
> >>+	if (!acpi_bus_power_manageable(ACPI_HANDLE(dev)))
> >>+#endif
> >>+	{
> >>+		data->gpio_power = devm_gpiod_get_optional(dev, "power",
> >>+							   GPIOD_OUT_LOW);
> >>+		if (IS_ERR(data->gpio_power)) {
> >>+			if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
> >>+				dev_err(dev, "Power GPIO request failed\n");
> >>+			return PTR_ERR(data->gpio_power);
> >>+		}
> >> 	}
> >>
> >> 	error = silead_ts_setup(client);
> >>--
> >>2.9.3
> >>
> >

-- 
Dmitry

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 22-01-17 23:20, Dmitry Torokhov wrote:
> > >On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
> > >>On some x86 tablets we cannot directly access the GPIOs as they are
> > >>claimed by the ACPI tables, so check it the i2c client is not being
> > >>power-managed by ACPI before trying to get the power pin GPIO.
> > >
> > >Why do we even get this GPIO if driver is not supposed to be using it?
> > >I'd much rather gpio provider hid it from the driver instead of every
> > >driver having this check.
> > 
> > The problem is that the gpio subsys does not really know about ACPI
> > managed GPIOs the way this works is that the firmware sets a special
> > "reserved for firmware use" bit in the gpio control register and
> > directly bit-bangs the gpio control register when it wants to toggle
> > the gpio. So there is no awareness of these gpios being reserved
> > (as gpios) at the ACPI level AFAICT.
> > 
> > The hardware specific low-level gpio chip driver checks this bit
> > when we request the gpio and returns -EBUSY.
> 
> I'd say that, if GPIOs are reserved for firmware use, and kernel should
> not or can not touch them, then they should not be visible, if not to
> the gpio core, then to consumers for sure.
> 
> Let's add Mika and Linus.

It is not always possible for the GPIO driver to find out if a certain
GPIO is reserved for the firmware use or not. I don't think we have any
API in gpiolib that allows excluding certain GPIOs though.

What we do for example with the ACPI OpRegion GPIOs is that gpiolib
reserves those automatically thus preventing any consumer from using
those.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-02-17 11:41, Mika Westerberg wrote:
> On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote:
>> On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 22-01-17 23:20, Dmitry Torokhov wrote:
>>>> On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
>>>>> On some x86 tablets we cannot directly access the GPIOs as they are
>>>>> claimed by the ACPI tables, so check it the i2c client is not being
>>>>> power-managed by ACPI before trying to get the power pin GPIO.
>>>>
>>>> Why do we even get this GPIO if driver is not supposed to be using it?
>>>> I'd much rather gpio provider hid it from the driver instead of every
>>>> driver having this check.
>>>
>>> The problem is that the gpio subsys does not really know about ACPI
>>> managed GPIOs the way this works is that the firmware sets a special
>>> "reserved for firmware use" bit in the gpio control register and
>>> directly bit-bangs the gpio control register when it wants to toggle
>>> the gpio. So there is no awareness of these gpios being reserved
>>> (as gpios) at the ACPI level AFAICT.
>>>
>>> The hardware specific low-level gpio chip driver checks this bit
>>> when we request the gpio and returns -EBUSY.
>>
>> I'd say that, if GPIOs are reserved for firmware use, and kernel should
>> not or can not touch them, then they should not be visible, if not to
>> the gpio core, then to consumers for sure.
>>
>> Let's add Mika and Linus.
>
> It is not always possible for the GPIO driver to find out if a certain
> GPIO is reserved for the firmware use or not. I don't think we have any
> API in gpiolib that allows excluding certain GPIOs though.
>
> What we do for example with the ACPI OpRegion GPIOs is that gpiolib
> reserves those automatically thus preventing any consumer from using
> those.

Right, but that would result in a -EBUSY error from gpiod_get, so
iow gpiod_get_optional would still fail and not just return NULL as it
would if the requested gpio does not exist?

IOW even if that was happening here, we would still need to handle
either -EBUSY and treat it as gpiod_get_optional returning NULL, or
we need the acpi_bus_power_manageable check the patch from this
thread is adding, right ?

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Feb 02, 2017 at 12:57:05PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-02-17 11:41, Mika Westerberg wrote:
> > On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 22-01-17 23:20, Dmitry Torokhov wrote:
> > > > > On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote:
> > > > > > On some x86 tablets we cannot directly access the GPIOs as they are
> > > > > > claimed by the ACPI tables, so check it the i2c client is not being
> > > > > > power-managed by ACPI before trying to get the power pin GPIO.
> > > > > 
> > > > > Why do we even get this GPIO if driver is not supposed to be using it?
> > > > > I'd much rather gpio provider hid it from the driver instead of every
> > > > > driver having this check.
> > > > 
> > > > The problem is that the gpio subsys does not really know about ACPI
> > > > managed GPIOs the way this works is that the firmware sets a special
> > > > "reserved for firmware use" bit in the gpio control register and
> > > > directly bit-bangs the gpio control register when it wants to toggle
> > > > the gpio. So there is no awareness of these gpios being reserved
> > > > (as gpios) at the ACPI level AFAICT.
> > > > 
> > > > The hardware specific low-level gpio chip driver checks this bit
> > > > when we request the gpio and returns -EBUSY.
> > > 
> > > I'd say that, if GPIOs are reserved for firmware use, and kernel should
> > > not or can not touch them, then they should not be visible, if not to
> > > the gpio core, then to consumers for sure.
> > > 
> > > Let's add Mika and Linus.
> > 
> > It is not always possible for the GPIO driver to find out if a certain
> > GPIO is reserved for the firmware use or not. I don't think we have any
> > API in gpiolib that allows excluding certain GPIOs though.
> > 
> > What we do for example with the ACPI OpRegion GPIOs is that gpiolib
> > reserves those automatically thus preventing any consumer from using
> > those.
> 
> Right, but that would result in a -EBUSY error from gpiod_get, so
> iow gpiod_get_optional would still fail and not just return NULL as it
> would if the requested gpio does not exist?

Yes, that's my understanding as well.

> IOW even if that was happening here, we would still need to handle
> either -EBUSY and treat it as gpiod_get_optional returning NULL, or
> we need the acpi_bus_power_manageable check the patch from this
> thread is adding, right ?

I do not have a copy of the patch in this thread but sounds like
something that might work.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
> I do not have a copy of the patch in this thread but sounds like
> something that might work.

Actually, I seem have a copy of that patch.

So you are saying that the device has a power GPIO in ACPI _CRS but it
should not be used for some reason?

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-02-17 13:32, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
>> I do not have a copy of the patch in this thread but sounds like
>> something that might work.
>
> Actually, I seem have a copy of that patch.
>
> So you are saying that the device has a power GPIO in ACPI _CRS but it
> should not be used for some reason?

Yes, it should not be used as it is controlled by the _PS0 and _PS3
functions. It is reserved for firmware use by setting a special
bit indicating this in the gpio pin's control register in the intel
cherrytrail gpio controller. So arguably it should not be in _CRS
at all, since the OS is supposed to not touch it, yet it is there:

             Device (TCS4)
             {
                 Name (_ADR, Zero)  // _ADR: Address
                 Name (_HID, "MSSL1680")  // _HID: Hardware ID
                 Name (_CID, "PNP1680")  // _CID: Compatible ID
                 Name (_S0W, Zero)  // _S0W: S0 Device Wake State
                 Name (_DDN, "MSSL1680")  // _DDN: DOS Device Name

<snip>

                 Method (_PS3, 0, Serialized)  // _PS3: Power State 3
                 {
                     If (^^^^GPO1.AVBL == One)
                     {
                         ^^^^GPO1.TCTL = One
                     }

                     Sleep (0x78)
                 }

                 Method (_PS0, 0, Serialized)  // _PS0: Power State 0
                 {
                     If (^^^^GPO1.AVBL == One)
                     {
                         ^^^^GPO1.TCTL = One
                     }

                     Sleep (0x05)
                     If (^^^^GPO1.AVBL == One)
                     {
                         ^^^^GPO1.TCTL = Zero
                     }

                     Sleep (0x1E)
                     If (^^^^GPO1.AVBL == One)
                     {
                         ^^^^GPO1.TCTL = Zero
                     }

                     Sleep (0x78)
                 }

                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
                 {
                     Name (WBUF, ResourceTemplate ()
                     {
                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
                             0x00, ResourceConsumer, , Exclusive,
                             )
                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                             )
                             {   // Pin list
                                 0x0019
                             }
                         GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000,
                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                             )
                             {   // Pin list
                                 0x0013
                             }
                     })
                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
                 }

The setting of the special bit in the gpio control register leads to
drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
returning -EBUSY, which in return makes gpiod_get_optional
return -EBUSY for this pin, rather then NULL (as we would like).

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-02-17 13:32, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
> > > I do not have a copy of the patch in this thread but sounds like
> > > something that might work.
> > 
> > Actually, I seem have a copy of that patch.
> > 
> > So you are saying that the device has a power GPIO in ACPI _CRS but it
> > should not be used for some reason?
> 
>                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
>                 {
>                     Name (WBUF, ResourceTemplate ()
>                     {
>                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
>                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>                             0x00, ResourceConsumer, , Exclusive,
>                             )
>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>                             )
>                             {   // Pin list
>                                 0x0019
>                             }
>                         GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000,
>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>                             )
>                             {   // Pin list
>                                 0x0013
>                             }
>                     })
>                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
>                 }
> 
> The setting of the special bit in the gpio control register leads to
> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
> returning -EBUSY, which in return makes gpiod_get_optional
> return -EBUSY for this pin, rather then NULL (as we would like).

Actually what is wrong here is that your gpiod_get(dev, "power") falls
back to use plain indexes and returns the first GPIO even though it
should not as the driver specifically requests GPIO with name "power"
and there is no _DSD.

Andy (Cc'd) has a patch that tries to make the fallback mechanism more
stricter which should in theory fix the problem as well. The patch
series is here:

https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-02-17 14:12, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-02-17 13:32, Mika Westerberg wrote:
>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
>>>> I do not have a copy of the patch in this thread but sounds like
>>>> something that might work.
>>>
>>> Actually, I seem have a copy of that patch.
>>>
>>> So you are saying that the device has a power GPIO in ACPI _CRS but it
>>> should not be used for some reason?
>>
>>                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
>>                 {
>>                     Name (WBUF, ResourceTemplate ()
>>                     {
>>                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
>>                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>>                             0x00, ResourceConsumer, , Exclusive,
>>                             )
>>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>                             )
>>                             {   // Pin list
>>                                 0x0019
>>                             }
>>                         GpioInt (Edge, ActiveHigh, Shared,// PullDefault, 0x0000,
>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>                             )
>>                             {   // Pin list
>>                                 0x0013
>>                             }
>>                     })
>>                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
>>                 }
>>
>> The setting of the special bit in the gpio control register leads to
>> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
>> returning -EBUSY, which in return makes gpiod_get_optional
>> return -EBUSY for this pin, rather then NULL (as we would like).
>
> Actually what is wrong here is that your gpiod_get(dev, "power") falls
> back to use plain indexes and returns the first GPIO even though it
> should not as the driver specifically requests GPIO with name "power"
> and there is no _DSD.

There is no clear "binding" for this device in ACPI, so the fallback
is actually used as a feature by the Silead driver (more or less),
the use of "power" as name here is for the ARM + devicetree usage
of the driver really, so IOW the series:

>
> Andy (Cc'd) has a patch that tries to make the fallback mechanism more
> stricter which should in theory fix the problem as well. The patch
> series is here:
>
> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master

You are referring to might fix this, but then we may need to add an
attempt to get the gpio by index for some boards which do not control
it themselves from _PS#.

I can give the linked series a try, but I would still like a fallback
plan if we indeed encounter boards where we need to fallback to
getting the gpio by index. Once we do that we're back to having the
same problem as then we would do the same fallback on boards where
the pin is reserved for _PS# usage, and end up with an -EBUSY error
again. I guess we could ignore -EBUSY in the fallback path, or only
do the fallback if acpi_bus_power_manageable() returns false.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote:
> > Actually what is wrong here is that your gpiod_get(dev, "power") falls
> > back to use plain indexes and returns the first GPIO even though it
> > should not as the driver specifically requests GPIO with name "power"
> > and there is no _DSD.
> 
> There is no clear "binding" for this device in ACPI, so the fallback
> is actually used as a feature by the Silead driver (more or less),
> the use of "power" as name here is for the ARM + devicetree usage
> of the driver really, so IOW the series:
> 
> > 
> > Andy (Cc'd) has a patch that tries to make the fallback mechanism more
> > stricter which should in theory fix the problem as well. The patch
> > series is here:
> > 
> > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master
> 
> You are referring to might fix this, but then we may need to add an
> attempt to get the gpio by index for some boards which do not control
> it themselves from _PS#.
> 
> I can give the linked series a try, but I would still like a fallback
> plan if we indeed encounter boards where we need to fallback to
> getting the gpio by index. Once we do that we're back to having the
> same problem as then we would do the same fallback on boards where
> the pin is reserved for _PS# usage, and end up with an -EBUSY error
> again. I guess we could ignore -EBUSY in the fallback path, or only
> do the fallback if acpi_bus_power_manageable() returns false.

I don't think using acpi_bus_power_manageable() is a proper way to fix
this.

This can be fixed without the fallback so that for the boards you know
need to handle the GPIO themselves (based on the _HID for example), they
will call acpi_dev_add_driver_gpios() passing the mapping for "power".
The other boards then just don't get the GPIO.

We really want to get rid of the whole fallback mess.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-02-17 14:44, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote:
>>> Actually what is wrong here is that your gpiod_get(dev, "power") falls
>>> back to use plain indexes and returns the first GPIO even though it
>>> should not as the driver specifically requests GPIO with name "power"
>>> and there is no _DSD.
>>
>> There is no clear "binding" for this device in ACPI, so the fallback
>> is actually used as a feature by the Silead driver (more or less),
>> the use of "power" as name here is for the ARM + devicetree usage
>> of the driver really, so IOW the series:
>>
>>>
>>> Andy (Cc'd) has a patch that tries to make the fallback mechanism more
>>> stricter which should in theory fix the problem as well. The patch
>>> series is here:
>>>
>>> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master
>>
>> You are referring to might fix this, but then we may need to add an
>> attempt to get the gpio by index for some boards which do not control
>> it themselves from _PS#.
>>
>> I can give the linked series a try, but I would still like a fallback
>> plan if we indeed encounter boards where we need to fallback to
>> getting the gpio by index. Once we do that we're back to having the
>> same problem as then we would do the same fallback on boards where
>> the pin is reserved for _PS# usage, and end up with an -EBUSY error
>> again. I guess we could ignore -EBUSY in the fallback path, or only
>> do the fallback if acpi_bus_power_manageable() returns false.
>
> I don't think using acpi_bus_power_manageable() is a proper way to fix
> this.

Ok.

> This can be fixed without the fallback so that for the boards you know
> need to handle the GPIO themselves (based on the _HID for example),

The _HID is the same everywhere, these boards dstd's are mostly a copy
and paste fest. So this would to be DMI based.

> they
> will call acpi_dev_add_driver_gpios() passing the mapping for "power".
> The other boards then just don't get the GPIO.

The list of boards needing this might be huge. Anyways we will figure
this out as we encounter boards needing some form of fallback to
getting gpios based on index. If you think using acpi_bus_power_manageable()
is a bad way to detect that no gpio is needed then we may just end up ignoring
the -EBUSY return from gpiod_get_by_index

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Feb 02, 2017 at 02:55:57PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-02-17 14:44, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote:
> > > > Actually what is wrong here is that your gpiod_get(dev, "power") falls
> > > > back to use plain indexes and returns the first GPIO even though it
> > > > should not as the driver specifically requests GPIO with name "power"
> > > > and there is no _DSD.
> > > 
> > > There is no clear "binding" for this device in ACPI, so the fallback
> > > is actually used as a feature by the Silead driver (more or less),
> > > the use of "power" as name here is for the ARM + devicetree usage
> > > of the driver really, so IOW the series:
> > > 
> > > > 
> > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more
> > > > stricter which should in theory fix the problem as well. The patch
> > > > series is here:
> > > > 
> > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master
> > > 
> > > You are referring to might fix this, but then we may need to add an
> > > attempt to get the gpio by index for some boards which do not control
> > > it themselves from _PS#.
> > > 
> > > I can give the linked series a try, but I would still like a fallback
> > > plan if we indeed encounter boards where we need to fallback to
> > > getting the gpio by index. Once we do that we're back to having the
> > > same problem as then we would do the same fallback on boards where
> > > the pin is reserved for _PS# usage, and end up with an -EBUSY error
> > > again. I guess we could ignore -EBUSY in the fallback path, or only
> > > do the fallback if acpi_bus_power_manageable() returns false.
> > 
> > I don't think using acpi_bus_power_manageable() is a proper way to fix
> > this.
> 
> Ok.
> 
> > This can be fixed without the fallback so that for the boards you know
> > need to handle the GPIO themselves (based on the _HID for example),
> 
> The _HID is the same everywhere, these boards dstd's are mostly a copy
> and paste fest. So this would to be DMI based.

:(

I suppose the Windows driver just works everywhere, right?

> > they
> > will call acpi_dev_add_driver_gpios() passing the mapping for "power".
> > The other boards then just don't get the GPIO.
> 
> The list of boards needing this might be huge. Anyways we will figure
> this out as we encounter boards needing some form of fallback to
> getting gpios based on index. If you think using acpi_bus_power_manageable()
> is a bad way to detect that no gpio is needed then we may just end up ignoring
> the -EBUSY return from gpiod_get_by_index

Well, that is certainly better than using acpi_bus_power_manageable() if
nothing else can be done here.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Gregor Riepl]

> I suppose the Windows driver just works everywhere, right?

Not really, no.
AFAIK every vendor builds their own driver using a special configuration 
utility from Silead.
All the device-specific data, including panel parameters and calibration data, 
is compiled into this driver.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Linus Walleij]

On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com> wrote:

>> I suppose the Windows driver just works everywhere, right?
>
> Not really, no.
> AFAIK every vendor builds their own driver using a special configuration
> utility from Silead.
> All the device-specific data, including panel parameters and calibration
> data, is compiled into this driver.

Isn't that just defeating the whole purpose of ACPI (or any other
hardware description)?

Isn't the idea to describe all this in ACPI tables, and that is what the
vendor should be doing rather than compiling in hardcoded things
into drivers?

I just see this as another sign that the ACPI "ecosystem" is not
really working because vendors choose to arbitrarily bypass it like
this.

Is it too hard for them to use ACPI or is ACPI lacking the right
parameters to tweak or what is the real problem?

Yours,
Linus Walleij

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 14-03-17 11:21, Linus Walleij wrote:
> On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com> wrote:
>
>>> I suppose the Windows driver just works everywhere, right?
>>
>> Not really, no.
>> AFAIK every vendor builds their own driver using a special configuration
>> utility from Silead.
>> All the device-specific data, including panel parameters and calibration
>> data, is compiled into this driver.
>
> Isn't that just defeating the whole purpose of ACPI (or any other
> hardware description)?
>
> Isn't the idea to describe all this in ACPI tables, and that is what the
> vendor should be doing rather than compiling in hardcoded things
> into drivers?
>
> I just see this as another sign that the ACPI "ecosystem" is not
> really working because vendors choose to arbitrarily bypass it like
> this.
>
> Is it too hard for them to use ACPI or is ACPI lacking the right
> parameters to tweak or what is the real problem?

It is certainly possible to do this in a more sane manner using
ACPI, if I were to guess what the real problem is it is a combination
of time-to-marker + getting what you pay for (pay peanuts ...).

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Tue, 2017-03-14 at 11:21 +0100, Linus Walleij wrote:
> On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com>
> wrote:
> 
> > > I suppose the Windows driver just works everywhere, right?
> > 
> > Not really, no.
> > AFAIK every vendor builds their own driver using a special
> > configuration
> > utility from Silead.
> > All the device-specific data, including panel parameters and
> > calibration
> > data, is compiled into this driver.
> 
> Isn't that just defeating the whole purpose of ACPI (or any other
> hardware description)?
> 
> Isn't the idea to describe all this in ACPI tables, and that is what
> the
> vendor should be doing rather than compiling in hardcoded things
> into drivers?

> I just see this as another sign that the ACPI "ecosystem" is not
> really working because vendors choose to arbitrarily bypass it like
> this.
> 
> Is it too hard for them to use ACPI or is ACPI lacking the right
> parameters to tweak or what is the real problem?

With _DSD is possible to enhance ACPI tables to cover almost everything,
but there is another issue(s):

a) we still have to cope with old firmwares,

b) Windows is doing things differently, and

c) some vendors are abusing ACPI (existing!) standards.

Here is a dilemma: make user's life miserable with hardware they bought,
or make developers' life miserable to support all that. And the winner
is... vendor which is abusing whatever it wants to!

One recent example I found is a Kontron xBTI x86 board (Linux
compatible!) where they abused ACPI by creating a table that even iasl
can't decode (not a major issue, the table name is just non-standard,
but nevertheless).

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Gregor Riepl]

> Isn't that just defeating the whole purpose of ACPI (or any other
> hardware description)?
> 
> Isn't the idea to describe all this in ACPI tables, and that is what the
> vendor should be doing rather than compiling in hardcoded things
> into drivers?
> 
> I just see this as another sign that the ACPI "ecosystem" is not
> really working because vendors choose to arbitrarily bypass it like
> this.

I hear you...

If you want my opinion - in this case it wasn't just a chip manufacturer that
didn't understand or care about the ecosystem, it was also OEMs that didn't
give a shit and Intel that failed to grasp the problem and counteract.

But there's no point in arguing about it, it is how it is and we have to deal
with the fallout.

What we can do is provide better guidance for inexperienced Chinese
manufacturers and lobby Intel so they take more responsibility for their
platforms. A single person can't do much, but I do think that a vital open
source project like the Linux kernel has some leverage.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-02-17 14:12, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-02-17 13:32, Mika Westerberg wrote:
>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
>>>> I do not have a copy of the patch in this thread but sounds like
>>>> something that might work.
>>>
>>> Actually, I seem have a copy of that patch.
>>>
>>> So you are saying that the device has a power GPIO in ACPI _CRS but it
>>> should not be used for some reason?
>>
>>                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
>>                 {
>>                     Name (WBUF, ResourceTemplate ()
>>                     {
>>                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
>>                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>>                             0x00, ResourceConsumer, , Exclusive,
>>                             )
>>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>                             )
>>                             {   // Pin list
>>                                 0x0019
>>                             }
>>                         GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000,
>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>                             )
>>                             {   // Pin list
>>                                 0x0013
>>                             }
>>                     })
>>                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
>>                 }
>>
>> The setting of the special bit in the gpio control register leads to
>> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
>> returning -EBUSY, which in return makes gpiod_get_optional
>> return -EBUSY for this pin, rather then NULL (as we would like).
>
> Actually what is wrong here is that your gpiod_get(dev, "power") falls
> back to use plain indexes and returns the first GPIO even though it
> should not as the driver specifically requests GPIO with name "power"
> and there is no _DSD.
>
> Andy (Cc'd) has a patch that tries to make the fallback mechanism more
> stricter which should in theory fix the problem as well. The patch
> series is here:
>
> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master

Ok, that patches fixes the issues I was seeing with the silead driver
on my cube iwork8 air cherrytrail tablet.

As discussed we might need some extra handling to fallback to actually
get a gpio by index on some tablets which don't control the gpio from
ACPI AML code, but lets cross that bridge when we get there.

Regards,

Hans


>

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Fri, Feb 10, 2017 at 12:52:41PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-02-17 14:12, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 02-02-17 13:32, Mika Westerberg wrote:
> > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
> > > > > I do not have a copy of the patch in this thread but sounds like
> > > > > something that might work.
> > > > 
> > > > Actually, I seem have a copy of that patch.
> > > > 
> > > > So you are saying that the device has a power GPIO in ACPI _CRS but it
> > > > should not be used for some reason?
> > > 
> > >                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
> > >                 {
> > >                     Name (WBUF, ResourceTemplate ()
> > >                     {
> > >                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
> > >                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
> > >                             0x00, ResourceConsumer, , Exclusive,
> > >                             )
> > >                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
> > >                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> > >                             )
> > >                             {   // Pin list
> > >                                 0x0019
> > >                             }
> > >                         GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000,
> > >                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> > >                             )
> > >                             {   // Pin list
> > >                                 0x0013
> > >                             }
> > >                     })
> > >                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
> > >                 }
> > > 
> > > The setting of the special bit in the gpio control register leads to
> > > drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
> > > returning -EBUSY, which in return makes gpiod_get_optional
> > > return -EBUSY for this pin, rather then NULL (as we would like).
> > 
> > Actually what is wrong here is that your gpiod_get(dev, "power") falls
> > back to use plain indexes and returns the first GPIO even though it
> > should not as the driver specifically requests GPIO with name "power"
> > and there is no _DSD.
> > 
> > Andy (Cc'd) has a patch that tries to make the fallback mechanism more
> > stricter which should in theory fix the problem as well. The patch
> > series is here:
> > 
> > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master
> 
> Ok, that patches fixes the issues I was seeing with the silead driver
> on my cube iwork8 air cherrytrail tablet.

OK, good to hear. I suppose Andy can start cleaning up that branch and
send them for review then ;-)

> As discussed we might need some extra handling to fallback to actually
> get a gpio by index on some tablets which don't control the gpio from
> ACPI AML code, but lets cross that bridge when we get there.

Agreed.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 10-02-17 12:52, Hans de Goede wrote:
> Hi,
>
> On 02-02-17 14:12, Mika Westerberg wrote:
>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 02-02-17 13:32, Mika Westerberg wrote:
>>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote:
>>>>> I do not have a copy of the patch in this thread but sounds like
>>>>> something that might work.
>>>>
>>>> Actually, I seem have a copy of that patch.
>>>>
>>>> So you are saying that the device has a power GPIO in ACPI _CRS but it
>>>> should not be used for some reason?
>>>
>>>                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
>>>                 {
>>>                     Name (WBUF, ResourceTemplate ()
>>>                     {
>>>                         I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80,
>>>                             AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>>>                             0x00, ResourceConsumer, , Exclusive,
>>>                             )
>>>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri
>>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>>                             )
>>>                             {   // Pin list
>>>                                 0x0019
>>>                             }
>>>                         GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000,
>>>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>>                             )
>>>                             {   // Pin list
>>>                                 0x0013
>>>                             }
>>>                     })
>>>                     Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
>>>                 }
>>>
>>> The setting of the special bit in the gpio control register leads to
>>> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable()
>>> returning -EBUSY, which in return makes gpiod_get_optional
>>> return -EBUSY for this pin, rather then NULL (as we would like).
>>
>> Actually what is wrong here is that your gpiod_get(dev, "power") falls
>> back to use plain indexes and returns the first GPIO even though it
>> should not as the driver specifically requests GPIO with name "power"
>> and there is no _DSD.
>>
>> Andy (Cc'd) has a patch that tries to make the fallback mechanism more
>> stricter which should in theory fix the problem as well. The patch
>> series is here:
>>
>> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master
>
> Ok, that patches fixes the issues I was seeing with the silead driver
> on my cube iwork8 air cherrytrail tablet.

But unfortunately it causes regressions for drivers which actually use
gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which
does:

         data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
                                                 INT3496_GPIO_USB_ID,
                                                 GPIOD_IN);

Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess
this driver can be fixed by replacing "id" with NULL, but the name
gets used in things like /sys/kernel/debug/gpio and is actually
useful there, so it looks like that patch from Andy needs some
work so as to not see getting by index as an undesirable fallback
while the driver is actually doing a request gpio by index.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> Hi,
> 
> On 10-02-17 12:52, Hans de Goede wrote:
> > Hi,
> > 
> > On 02-02-17 14:12, Mika Westerberg wrote:
> > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 02-02-17 13:32, Mika Westerberg wrote:
> > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg
> > > > > wrote:
> > > > > > I do not have a copy of the patch in this thread but sounds
> > > > > > like
> > > > > > something that might work.
> > > > > 
> > > > > Actually, I seem have a copy of that patch.
> > > > > 
> > > > > So you are saying that the device has a power GPIO in ACPI
> > > > > _CRS but it
> > > > > should not be used for some reason?
> > > > 
> > > >                 Method (_CRS, 0, NotSerialized)  // _CRS:
> > > > Current Resource Setti
> > > >                 {
> > > >                     Name (WBUF, ResourceTemplate ()
> > > >                     {
> > > >                         I2cSerialBusV2 (0x0040,
> > > > ControllerInitiated, 0x00061A80,
> > > >                             AddressingMode7Bit,
> > > > "\\_SB.PCI0.I2C6",
> > > >                             0x00, ResourceConsumer, , Exclusive,
> > > >                             )
> > > >                         GpioIo (Exclusive, PullDefault, 0x0000,
> > > > 0x0000, IoRestri
> > > >                             "\\_SB.GPO1", 0x00,
> > > > ResourceConsumer, ,
> > > >                             )
> > > >                             {   // Pin list
> > > >                                 0x0019
> > > >                             }
> > > >                         GpioInt (Edge, ActiveHigh, Shared,
> > > > PullDefault, 0x0000,
> > > >                             "\\_SB.GPO1", 0x00,
> > > > ResourceConsumer, ,
> > > >                             )
> > > >                             {   // Pin list
> > > >                                 0x0013
> > > >                             }
> > > >                     })
> > > >                     Return (WBUF) /*
> > > > \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
> > > >                 }
> > > > 
> > > > The setting of the special bit in the gpio control register
> > > > leads to
> > > > drivers/pinctrl/intel/pinctrl-cherryview.c
> > > > chv_gpio_request_enable()
> > > > returning -EBUSY, which in return makes gpiod_get_optional
> > > > return -EBUSY for this pin, rather then NULL (as we would like).
> > > 
> > > Actually what is wrong here is that your gpiod_get(dev, "power")
> > > falls
> > > back to use plain indexes and returns the first GPIO even though
> > > it
> > > should not as the driver specifically requests GPIO with name
> > > "power"
> > > and there is no _DSD.
> > > 
> > > Andy (Cc'd) has a patch that tries to make the fallback mechanism
> > > more
> > > stricter which should in theory fix the problem as well. The patch
> > > series is here:
> > > 
> > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1
> > > 43070a301d8b8883c349?at=master
> > 
> > Ok, that patches fixes the issues I was seeing with the silead
> > driver
> > on my cube iwork8 air cherrytrail tablet.
> 
> But unfortunately it causes regressions for drivers which actually use
> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which
> does:
> 
>          data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
>                                                  INT3496_GPIO_USB_ID,
>                                                  GPIOD_IN);
> 
> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess
> this driver can be fixed by replacing "id" with NULL, but the name
> gets used in things like /sys/kernel/debug/gpio and is actually
> useful there, so it looks like that patch from Andy needs some
> work so as to not see getting by index as an undesirable fallback
> while the driver is actually doing a request gpio by index.

Yes, this part is missed. We would like to introduce a flag for that to
encourage drivers to fix this mess by adding compatible lookup table.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> Hi,
> 
> On 10-02-17 12:52, Hans de Goede wrote:
> > Hi,
> > 
> > On 02-02-17 14:12, Mika Westerberg wrote:
> > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 02-02-17 13:32, Mika Westerberg wrote:
> > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg
> > > > > wrote:
> > > > > > I do not have a copy of the patch in this thread but sounds
> > > > > > like
> > > > > > something that might work.
> > > > > 
> > > > > Actually, I seem have a copy of that patch.
> > > > > 
> > > > > So you are saying that the device has a power GPIO in ACPI
> > > > > _CRS but it
> > > > > should not be used for some reason?
> > > > 
> > > >                 Method (_CRS, 0, NotSerialized)  // _CRS:
> > > > Current Resource Setti
> > > >                 {
> > > >                     Name (WBUF, ResourceTemplate ()
> > > >                     {
> > > >                         I2cSerialBusV2 (0x0040,
> > > > ControllerInitiated, 0x00061A80,
> > > >                             AddressingMode7Bit,
> > > > "\\_SB.PCI0.I2C6",
> > > >                             0x00, ResourceConsumer, , Exclusive,
> > > >                             )
> > > >                         GpioIo (Exclusive, PullDefault, 0x0000,
> > > > 0x0000, IoRestri
> > > >                             "\\_SB.GPO1", 0x00,
> > > > ResourceConsumer, ,
> > > >                             )
> > > >                             {   // Pin list
> > > >                                 0x0019
> > > >                             }
> > > >                         GpioInt (Edge, ActiveHigh, Shared,
> > > > PullDefault, 0x0000,
> > > >                             "\\_SB.GPO1", 0x00,
> > > > ResourceConsumer, ,
> > > >                             )
> > > >                             {   // Pin list
> > > >                                 0x0013
> > > >                             }
> > > >                     })
> > > >                     Return (WBUF) /*
> > > > \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
> > > >                 }
> > > > 
> > > > The setting of the special bit in the gpio control register
> > > > leads to
> > > > drivers/pinctrl/intel/pinctrl-cherryview.c
> > > > chv_gpio_request_enable()
> > > > returning -EBUSY, which in return makes gpiod_get_optional
> > > > return -EBUSY for this pin, rather then NULL (as we would like).
> > > 
> > > Actually what is wrong here is that your gpiod_get(dev, "power")
> > > falls
> > > back to use plain indexes and returns the first GPIO even though
> > > it
> > > should not as the driver specifically requests GPIO with name
> > > "power"
> > > and there is no _DSD.
> > > 
> > > Andy (Cc'd) has a patch that tries to make the fallback mechanism
> > > more
> > > stricter which should in theory fix the problem as well. The patch
> > > series is here:
> > > 
> > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1
> > > 43070a301d8b8883c349?at=master
> > 
> > Ok, that patches fixes the issues I was seeing with the silead
> > driver
> > on my cube iwork8 air cherrytrail tablet.
> 
> But unfortunately it causes regressions for drivers which actually use
> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which
> does:
> 
>          data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
>                                                  INT3496_GPIO_USB_ID,
>                                                  GPIOD_IN);
> 
> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess
> this driver can be fixed by replacing "id" with NULL, but the name
> gets used in things like /sys/kernel/debug/gpio and is actually
> useful there, so it looks like that patch from Andy needs some
> work so as to not see getting by index as an undesirable fallback
> while the driver is actually doing a request gpio by index.

Hans, I have just pushed most recent stuff into my branch. Would you
have a chance test it? It has extcon patches embedded.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi Andy,

On 22-02-17 16:52, Andy Shevchenko wrote:
> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 10-02-17 12:52, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 02-02-17 14:12, Mika Westerberg wrote:
>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 02-02-17 13:32, Mika Westerberg wrote:
>>>>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg
>>>>>> wrote:
>>>>>>> I do not have a copy of the patch in this thread but sounds
>>>>>>> like
>>>>>>> something that might work.
>>>>>>
>>>>>> Actually, I seem have a copy of that patch.
>>>>>>
>>>>>> So you are saying that the device has a power GPIO in ACPI
>>>>>> _CRS but it
>>>>>> should not be used for some reason?
>>>>>
>>>>>                 Method (_CRS, 0, NotSerialized)  // _CRS:
>>>>> Current Resource Setti
>>>>>                 {
>>>>>                     Name (WBUF, ResourceTemplate ()
>>>>>                     {
>>>>>                         I2cSerialBusV2 (0x0040,
>>>>> ControllerInitiated, 0x00061A80,
>>>>>                             AddressingMode7Bit,
>>>>> "\\_SB.PCI0.I2C6",
>>>>>                             0x00, ResourceConsumer, , Exclusive,
>>>>>                             )
>>>>>                         GpioIo (Exclusive, PullDefault, 0x0000,
>>>>> 0x0000, IoRestri
>>>>>                             "\\_SB.GPO1", 0x00,
>>>>> ResourceConsumer, ,
>>>>>                             )
>>>>>                             {   // Pin list
>>>>>                                 0x0019
>>>>>                             }
>>>>>                         GpioInt (Edge, ActiveHigh, Shared,
>>>>> PullDefault, 0x0000,
>>>>>                             "\\_SB.GPO1", 0x00,
>>>>> ResourceConsumer, ,
>>>>>                             )
>>>>>                             {   // Pin list
>>>>>                                 0x0013
>>>>>                             }
>>>>>                     })
>>>>>                     Return (WBUF) /*
>>>>> \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */
>>>>>                 }
>>>>>
>>>>> The setting of the special bit in the gpio control register
>>>>> leads to
>>>>> drivers/pinctrl/intel/pinctrl-cherryview.c
>>>>> chv_gpio_request_enable()
>>>>> returning -EBUSY, which in return makes gpiod_get_optional
>>>>> return -EBUSY for this pin, rather then NULL (as we would like).
>>>>
>>>> Actually what is wrong here is that your gpiod_get(dev, "power")
>>>> falls
>>>> back to use plain indexes and returns the first GPIO even though
>>>> it
>>>> should not as the driver specifically requests GPIO with name
>>>> "power"
>>>> and there is no _DSD.
>>>>
>>>> Andy (Cc'd) has a patch that tries to make the fallback mechanism
>>>> more
>>>> stricter which should in theory fix the problem as well. The patch
>>>> series is here:
>>>>
>>>> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1
>>>> 43070a301d8b8883c349?at=master
>>>
>>> Ok, that patches fixes the issues I was seeing with the silead
>>> driver
>>> on my cube iwork8 air cherrytrail tablet.
>>
>> But unfortunately it causes regressions for drivers which actually use
>> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which
>> does:
>>
>>          data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
>>                                                  INT3496_GPIO_USB_ID,
>>                                                  GPIOD_IN);
>>
>> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess
>> this driver can be fixed by replacing "id" with NULL, but the name
>> gets used in things like /sys/kernel/debug/gpio and is actually
>> useful there, so it looks like that patch from Andy needs some
>> work so as to not see getting by index as an undesirable fallback
>> while the driver is actually doing a request gpio by index.
>
> Hans, I have just pushed most recent stuff into my branch. Would you
> have a chance test it? It has extcon patches embedded.

First of all thank you for working on this.

Before I spend time on testing this I must say that I've the feeling
these patches are going in the wrong direction.

I would expect you to modify gpiod_get_index to internally inside
the gpiolib code pass a flag which makes it clear that the name is
just a hint and that it should fallback to the index (*), as it is
doing before your patches to clean things up. That way we avoid
needing to fixup the drivers and add with IMHO is unnecessary
boilerplate to them, in both the extcon-intel-int3496.c and
soc_button_array.c cases we really just want to get a gpio by
index and the name is just there to make debugging easier.

Also if you look at the ACPI 6.0 or later spec. then there is
a new "generic button device" defined there and I've patches to
soc_button_array.c pending to add support for that:

https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e6030dfcaddb555d0e2d
https://github.com/jwrdegoede/linux-sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5

The ACPI spec clearly defines the _DSD (device specific data)
for these devices with a ACPI0011 _HID as containing an index
into the ACPI resources table for the device, since your patches
make it impossible to directly get a gpio by index (if one still
want the gpio to have a sensible name) that means I now need
to create an acpi_gpio_mapping table on the fly for this.

TL;DR: this approach seems like a lot of extra work / churn and
boilerplate code in drivers for no gain.

Can't we please just simply keep the fallback as-is when a driver
calls gpiod_get_index rather then gpiod_get ? That seems like a
lot simpler and cleaner solution to me.

Regards,

Hans



*) Or maybe even a flag that it is the index which should be looked at
and not the name, but that may break some existing users

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
> On 22-02-17 16:52, Andy Shevchenko wrote:
> > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> > > On 10-02-17 12:52, Hans de Goede wrote:
> > > > On 02-02-17 14:12, Mika Westerberg wrote:
> > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
> > > > > > On 02-02-17 13:32, Mika Westerberg wrote:
> > > > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg
> > > > > > > wrote:

> > > > Ok, that patches fixes the issues I was seeing with the silead
> > > > driver
> > > > on my cube iwork8 air cherrytrail tablet.
> > > 
> > > But unfortunately it causes regressions for drivers which actually
> > > use
> > > gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c,
> > > which
> > > does:
> > > 
> > >          data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
> > >                                                  INT3496_GPIO_USB_
> > > ID,
> > >                                                  GPIOD_IN);
> > > 
> > > Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I
> > > guess
> > > this driver can be fixed by replacing "id" with NULL, but the name
> > > gets used in things like /sys/kernel/debug/gpio and is actually
> > > useful there, so it looks like that patch from Andy needs some
> > > work so as to not see getting by index as an undesirable fallback
> > > while the driver is actually doing a request gpio by index.
> > 
> > Hans, I have just pushed most recent stuff into my branch. Would you
> > have a chance test it? It has extcon patches embedded.
> 
> First of all thank you for working on this.
> 
> Before I spend time on testing this I must say that I've the feeling
> these patches are going in the wrong direction.
> 
> I would expect you to modify gpiod_get_index to internally inside
> the gpiolib code pass a flag which makes it clear that the name is
> just a hint and that it should fallback to the index (*), as it is
> doing before your patches to clean things up. That way we avoid
> needing to fixup the drivers and add with IMHO is unnecessary
> boilerplate to them, in both the extcon-intel-int3496.c and
> soc_button_array.c cases we really just want to get a gpio by
> index and the name is just there to make debugging easier.

Unfortunately the flag solution was discussed as a *temporary* one to
makes someone's eye hurt when see the flag. Real solution is exactly
what I'm doing right now.

(Side note: when I started implementing flag option, I realized that it
even uglier than I thought, that's why I decide to go for fixing users
first)

The problem is that previously ACPI has no mean of mapping between
indexes and names, and connection ID has being abused for ACPI case.

Basically it means you can put *anything* as connection ID right now.
And this is bad, very bad idea!

Now, since we have _DSD and specification for mapping GPIO resources to
names (connection IDs!) we should *not* allow drivers to put anything
they want there.

It means that any driver that is supposed to be used on ACPI-based
platfroms with *or* without _DSD should provide a mapping table for the
latter case.

Other solution is to extend GPIO API to have almost all same set of
calls with an additional field "label" as it was recently done for
fwnode_get_gpiod_child() (whatever its name nowadays). I don't think
this is best (though allows less intrusion to the existing drivers) way
because (see above) an heavy abuse in the kernel of connection ID
meaning for ACPI-enabled platforms.

> Also if you look at the ACPI 6.0 or later spec. then there is
> a new "generic button device" defined there and I've patches to
> soc_button_array.c pending to add support for that:
> 
> https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e
> 6030dfcaddb555d0e2d
> https://github.com/jwrdegoede/linux-
> sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5

Side note.
The one patch is okay, the main one needs a comprehensive review (at
least two points just came to my mind: a) it should be done in generic
ACPICA / Linux ACPI glue code, b) it should use UUID library in kernel).

> The ACPI spec clearly defines the _DSD (device specific data)
> for these devices with a ACPI0011 _HID as containing an index
> into the ACPI resources table for the device, since your patches
> make it impossible to directly get a gpio by index (if one still
> want the gpio to have a sensible name) that means I now need
> to create an acpi_gpio_mapping table on the fly for this.

Since the GPIO API doesn't provide an additional "label" field when you
get it. Otherwise the problem that you never know what you get by index.

Regarding to how to create this, I think, as I already said above, it
would be internal stuff to Linux ACPI glue layer and perhaps GPIO ACPI
library.

> TL;DR: this approach seems like a lot of extra work / churn and
> boilerplate code in drivers for no gain.

Yes, because of current *abuse* of connection ID field in ACPI case.

> Can't we please just simply keep the fallback as-is when a driver
> calls gpiod_get_index rather then gpiod_get ? That seems like a
> lot simpler and cleaner solution to me.

No. We can't.

This is explained by documentation addon in:

https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0db1df
4182673826646c

(with flag removed approach)
https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf17093
dacccd30e349cc

and in commit message

https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3a139
0075613daee56f

> *) Or maybe even a flag that it is the index which should be looked at
> and not the name, but that may break some existing users

Mika, Linus, I would really appreciate your input to the topic:
opinions, critique, ideas, etc.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 02-03-17 12:38, Andy Shevchenko wrote:
> On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
>> On 22-02-17 16:52, Andy Shevchenko wrote:
>>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
>>>> On 10-02-17 12:52, Hans de Goede wrote:
>>>>> On 02-02-17 14:12, Mika Westerberg wrote:
>>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote:
>>>>>>> On 02-02-17 13:32, Mika Westerberg wrote:
>>>>>>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg
>>>>>>>> wrote:
>
>>>>> Ok, that patches fixes the issues I was seeing with the silead
>>>>> driver
>>>>> on my cube iwork8 air cherrytrail tablet.
>>>>
>>>> But unfortunately it causes regressions for drivers which actually
>>>> use
>>>> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c,
>>>> which
>>>> does:
>>>>
>>>>          data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
>>>>                                                  INT3496_GPIO_USB_
>>>> ID,
>>>>                                                  GPIOD_IN);
>>>>
>>>> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I
>>>> guess
>>>> this driver can be fixed by replacing "id" with NULL, but the name
>>>> gets used in things like /sys/kernel/debug/gpio and is actually
>>>> useful there, so it looks like that patch from Andy needs some
>>>> work so as to not see getting by index as an undesirable fallback
>>>> while the driver is actually doing a request gpio by index.
>>>
>>> Hans, I have just pushed most recent stuff into my branch. Would you
>>> have a chance test it? It has extcon patches embedded.
>>
>> First of all thank you for working on this.
>>
>> Before I spend time on testing this I must say that I've the feeling
>> these patches are going in the wrong direction.
>>
>> I would expect you to modify gpiod_get_index to internally inside
>> the gpiolib code pass a flag which makes it clear that the name is
>> just a hint and that it should fallback to the index (*), as it is
>> doing before your patches to clean things up. That way we avoid
>> needing to fixup the drivers and add with IMHO is unnecessary
>> boilerplate to them, in both the extcon-intel-int3496.c and
>> soc_button_array.c cases we really just want to get a gpio by
>> index and the name is just there to make debugging easier.
>
> Unfortunately the flag solution was discussed as a *temporary* one to
> makes someone's eye hurt when see the flag. Real solution is exactly
> what I'm doing right now.
>
> (Side note: when I started implementing flag option, I realized that it
> even uglier than I thought, that's why I decide to go for fixing users
> first)

Ok.

> The problem is that previously ACPI has no mean of mapping between
> indexes and names, and connection ID has being abused for ACPI case.
>
> Basically it means you can put *anything* as connection ID right now.
> And this is bad, very bad idea!

Ok.

> Now, since we have _DSD and specification for mapping GPIO resources to
> names (connection IDs!) we should *not* allow drivers to put anything
> they want there.
>
> It means that any driver that is supposed to be used on ACPI-based
> platfroms with *or* without _DSD should provide a mapping table for the
> latter case.
>
> Other solution is to extend GPIO API to have almost all same set of
> calls with an additional field "label" as it was recently done for
> fwnode_get_gpiod_child() (whatever its name nowadays). I don't think
> this is best (though allows less intrusion to the existing drivers) way
> because (see above) an heavy abuse in the kernel of connection ID
> meaning for ACPI-enabled platforms.

Hmm, I actually like the label vs connection-id distinction there
are many ACPI device "bindings" where we simply get an index into
the ACPI resource table for the device as only way to get the right
gpio. Forcing the addition of a connection-id table to all those
drivers not only is needless churn / boilerplate, but also gives the
false impression that we actually have more info (a valid connection
id) then we really have.

So my vote goes to adding a label field, and passing NULL as
connection id in these drivers, rather then adding a fake connection-id
table.

>> Also if you look at the ACPI 6.0 or later spec. then there is
>> a new "generic button device" defined there and I've patches to
>> soc_button_array.c pending to add support for that:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e
>> 6030dfcaddb555d0e2d
>> https://github.com/jwrdegoede/linux-
>> sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5
>
> Side note.
> The one patch is okay, the main one needs a comprehensive review (at
> least two points just came to my mind: a) it should be done in generic
> ACPICA / Linux ACPI glue code, b) it should use UUID library in kernel).
>
>> The ACPI spec clearly defines the _DSD (device specific data)
>> for these devices with a ACPI0011 _HID as containing an index
>> into the ACPI resources table for the device, since your patches
>> make it impossible to directly get a gpio by index (if one still
>> want the gpio to have a sensible name) that means I now need
>> to create an acpi_gpio_mapping table on the fly for this.
>
> Since the GPIO API doesn't provide an additional "label" field when you
> get it. Otherwise the problem that you never know what you get by index.
>
> Regarding to how to create this, I think, as I already said above, it
> would be internal stuff to Linux ACPI glue layer and perhaps GPIO ACPI
> library.
>
>> TL;DR: this approach seems like a lot of extra work / churn and
>> boilerplate code in drivers for no gain.
>
> Yes, because of current *abuse* of connection ID field in ACPI case.
>
>> Can't we please just simply keep the fallback as-is when a driver
>> calls gpiod_get_index rather then gpiod_get ? That seems like a
>> lot simpler and cleaner solution to me.
>
> No. We can't.
>
> This is explained by documentation addon in:
>
> https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0db1df
> 4182673826646c
>
> (with flag removed approach)
> https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf17093
> dacccd30e349cc
>
> and in commit message
>
> https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3a139
> 0075613daee56f
>
>> *) Or maybe even a flag that it is the index which should be looked at
>> and not the name, but that may break some existing users
>
> Mika, Linus, I would really appreciate your input to the topic:
> opinions, critique, ideas, etc.

So my opinion on this is that I prefer the add a label field idea over
the everything must have either a connection-id in ACPI or a
connection-id-table in the driver.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-03-17 12:38, Andy Shevchenko wrote:
> > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
> > > On 22-02-17 16:52, Andy Shevchenko wrote:
> > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> > > > > On 10-02-17 12:52, Hans de Goede wrote:
> > > > > > On 02-02-17 14:12, Mika Westerberg wrote:
> > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede
> > > > > > > wrote:

> > Now, since we have _DSD and specification for mapping GPIO resources
> > to
> > names (connection IDs!) we should *not* allow drivers to put
> > anything
> > they want there.
> > 
> > It means that any driver that is supposed to be used on ACPI-based
> > platfroms with *or* without _DSD should provide a mapping table for
> > the
> > latter case.
> > 
> > Other solution is to extend GPIO API to have almost all same set of
> > calls with an additional field "label" as it was recently done for
> > fwnode_get_gpiod_child() (whatever its name nowadays). I don't think
> > this is best (though allows less intrusion to the existing drivers)
> > way
> > because (see above) an heavy abuse in the kernel of connection ID
> > meaning for ACPI-enabled platforms.
> 
> Hmm, I actually like the label vs connection-id distinction there
> are many ACPI device "bindings" where we simply get an index into
> the ACPI resource table for the device as only way to get the right
> gpio. 

Yeah, and we will be screwed if the index matrix is changed per device
ID / board.

> Forcing the addition of a connection-id table to all those
> drivers not only is needless churn / boilerplate, but also gives the
> false impression that we actually have more info (a valid connection
> id) then we really have.

Again, we have no idea what exactly we get if we call
gpiod_get_index(..., NULL, index); in case of ACPI. It would work *if
and only if* we assume that all versions of the same device on all
possible platforms will have *very same* mapping.

I have heard it's already not true. Check the commit 89ab37b489d1
("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").

> So my vote goes to adding a label field, and passing NULL as
> connection id in these drivers, rather then adding a fake connection-
> id
> table.

There are also few concerns:

1) it would be often passed the same string as connection ID and label
(okay, meaning we need like two functions per each in current API,
something like gpiod_get_*label(dev, con_id, ..., label);
and gpiod_get_label_nocid(dev, ..., label); besides the former ones);

2) using labels different to connection ID sounds not okay for debugging
purposes (I dunno why we have this done for fwnode_get_gpiod_child() in
the first place);

3) additionally different labels will not play good in _DSD enabled
case, since we must use connection ID there (we believe firmware until
otherwise is proven).

4) if some firmwares have different indexes for the same device we will
need to have device ID (PCI ID, ... or alike) to _CRS index mapping
anyway in the code.

> > > TL;DR: this approach seems like a lot of extra work / churn and
> > > boilerplate code in drivers for no gain.
> > 
> > Yes, because of current *abuse* of connection ID field in ACPI case.
> > 
> > > Can't we please just simply keep the fallback as-is when a driver
> > > calls gpiod_get_index rather then gpiod_get ? That seems like a
> > > lot simpler and cleaner solution to me.
> > 
> > No. We can't.
> > 
> > This is explained by documentation addon in:
> > 
> > https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0d
> > b1df
> > 4182673826646c
> > 
> > (with flag removed approach)
> > https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf1
> > 7093
> > dacccd30e349cc
> > 
> > and in commit message
> > 
> > https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3
> > a139
> > 0075613daee56f
> > 
> > > *) Or maybe even a flag that it is the index which should be
> > > looked at
> > > and not the name, but that may break some existing users
> > 
> > Mika, Linus, I would really appreciate your input to the topic:
> > opinions, critique, ideas, etc.
> 
> So my opinion on this is that I prefer the add a label field idea over
> the everything must have either a connection-id in ACPI or a
> connection-id-table in the driver.

As a ultimate workaround it would work, in long-term prospective it's
not a solution.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 03-03-17 15:57, Andy Shevchenko wrote:
> On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-03-17 12:38, Andy Shevchenko wrote:
>>> On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
>>>> On 22-02-17 16:52, Andy Shevchenko wrote:
>>>>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
>>>>>> On 10-02-17 12:52, Hans de Goede wrote:
>>>>>>> On 02-02-17 14:12, Mika Westerberg wrote:
>>>>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede
>>>>>>>> wrote:
>
>>> Now, since we have _DSD and specification for mapping GPIO resources
>>> to
>>> names (connection IDs!) we should *not* allow drivers to put
>>> anything
>>> they want there.
>>>
>>> It means that any driver that is supposed to be used on ACPI-based
>>> platfroms with *or* without _DSD should provide a mapping table for
>>> the
>>> latter case.
>>>
>>> Other solution is to extend GPIO API to have almost all same set of
>>> calls with an additional field "label" as it was recently done for
>>> fwnode_get_gpiod_child() (whatever its name nowadays). I don't think
>>> this is best (though allows less intrusion to the existing drivers)
>>> way
>>> because (see above) an heavy abuse in the kernel of connection ID
>>> meaning for ACPI-enabled platforms.
>>
>> Hmm, I actually like the label vs connection-id distinction there
>> are many ACPI device "bindings" where we simply get an index into
>> the ACPI resource table for the device as only way to get the right
>> gpio.
>
> Yeah, and we will be screwed if the index matrix is changed per device
> ID / board.
>
>> Forcing the addition of a connection-id table to all those
>> drivers not only is needless churn / boilerplate, but also gives the
>> false impression that we actually have more info (a valid connection
>> id) then we really have.
>
> Again, we have no idea what exactly we get if we call
> gpiod_get_index(..., NULL, index); in case of ACPI. It would work *if
> and only if* we assume that all versions of the same device on all
> possible platforms will have *very same* mapping.
>
> I have heard it's already not true. Check the commit 89ab37b489d1
> ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").
>
>> So my vote goes to adding a label field, and passing NULL as
>> connection id in these drivers, rather then adding a fake connection-
>> id
>> table.
>
> There are also few concerns:
>
> 1) it would be often passed the same string as connection ID and label
> (okay, meaning we need like two functions per each in current API,
> something like gpiod_get_*label(dev, con_id, ..., label);
> and gpiod_get_label_nocid(dev, ..., label); besides the former ones);

I would think the _label variants would not allow a connection_id at all.

> 2) using labels different to connection ID sounds not okay for debugging
> purposes (I dunno why we have this done for fwnode_get_gpiod_child() in
> the first place);

Right, which is why the _label variants would not allow a connection_id
at all using an _label variants implies connection_id == NULL.

And using a variant with connection_id argument implies label
  = connection_id.

> 3) additionally different labels will not play good in _DSD enabled
> case, since we must use connection ID there (we believe firmware until
> otherwise is proven).

Again, gpios would have a connection_id OR a label, so in
_DSD case only a connection_id.

> 4) if some firmwares have different indexes for the same device we will
> need to have device ID (PCI ID, ... or alike) to _CRS index mapping
> anyway in the code.

This problem will exist independent of which solution we choose.

>>>> TL;DR: this approach seems like a lot of extra work / churn and
>>>> boilerplate code in drivers for no gain.
>>>
>>> Yes, because of current *abuse* of connection ID field in ACPI case.
>>>
>>>> Can't we please just simply keep the fallback as-is when a driver
>>>> calls gpiod_get_index rather then gpiod_get ? That seems like a
>>>> lot simpler and cleaner solution to me.
>>>
>>> No. We can't.
>>>
>>> This is explained by documentation addon in:
>>>
>>> https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0d
>>> b1df
>>> 4182673826646c
>>>
>>> (with flag removed approach)
>>> https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf1
>>> 7093
>>> dacccd30e349cc
>>>
>>> and in commit message
>>>
>>> https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3
>>> a139
>>> 0075613daee56f
>>>
>>>> *) Or maybe even a flag that it is the index which should be
>>>> looked at
>>>> and not the name, but that may break some existing users
>>>
>>> Mika, Linus, I would really appreciate your input to the topic:
>>> opinions, critique, ideas, etc.
>>
>> So my opinion on this is that I prefer the add a label field idea over
>> the everything must have either a connection-id in ACPI or a
>> connection-id-table in the driver.
>
> As a ultimate workaround it would work, in long-term prospective it's
> not a solution.

I believe that this will work fine even in the long run, better then
forcible adding fake _DSD tables everywhere, see above.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote:
> Hi,
> 
> On 03-03-17 15:57, Andy Shevchenko wrote:
> > On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 02-03-17 12:38, Andy Shevchenko wrote:
> > > > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
> > > > > On 22-02-17 16:52, Andy Shevchenko wrote:
> > > > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> > > > > > > On 10-02-17 12:52, Hans de Goede wrote:
> > > > > > > > On 02-02-17 14:12, Mika Westerberg wrote:
> > > > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de
> > > > > > > > > Goede
> > > > > > > > > wrote:

> > > Forcing the addition of a connection-id table to all those
> > > drivers not only is needless churn / boilerplate, but also gives
> > > the
> > > false impression that we actually have more info (a valid
> > > connection
> > > id) then we really have.
> > 
> > Again, we have no idea what exactly we get if we call
> > gpiod_get_index(..., NULL, index); in case of ACPI. It would work
> > *if
> > and only if* we assume that all versions of the same device on all
> > possible platforms will have *very same* mapping.
> > 
> > I have heard it's already not true. Check the commit 89ab37b489d1
> > ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").

Any comment / remark on this?

> > > So my vote goes to adding a label field, and passing NULL as
> > > connection id in these drivers, rather then adding a fake
> > > connection-
> > > id
> > > table.
> > 
> > There are also few concerns:
> > 
> > 1) it would be often passed the same string as connection ID and
> > label
> > (okay, meaning we need like two functions per each in current API,
> > something like gpiod_get_*label(dev, con_id, ..., label);
> > and gpiod_get_label_nocid(dev, ..., label); besides the former
> > ones);
> 
> I would think the _label variants would not allow a connection_id at
> all.

Ah, okay.

> > 2) using labels different to connection ID sounds not okay for
> > debugging
> > purposes (I dunno why we have this done for fwnode_get_gpiod_child()
> > in
> > the first place);
> 
> Right, which is why the _label variants would not allow a
> connection_id
> at all using an _label variants implies connection_id == NULL.
> 
> And using a variant with connection_id argument implies label
>   = connection_id.
> 
> > 3) additionally different labels will not play good in _DSD enabled
> > case, since we must use connection ID there (we believe firmware
> > until
> > otherwise is proven).
> 
> Again, gpios would have a connection_id OR a label, so in
> _DSD case only a connection_id.
> 
> > 4) if some firmwares have different indexes for the same device we
> > will
> > need to have device ID (PCI ID, ... or alike) to _CRS index mapping
> > anyway in the code.
> 
> This problem will exist independent of which solution we choose.

Yes.

> > > > 
> > > > Mika, Linus, I would really appreciate your input to the topic:
> > > > opinions, critique, ideas, etc.
> > > 
> > > So my opinion on this is that I prefer the add a label field idea
> > > over
> > > the everything must have either a connection-id in ACPI or a
> > > connection-id-table in the driver.
> > 
> > As a ultimate workaround it would work, in long-term prospective
> > it's
> > not a solution.

> I believe that this will work fine even in the long run, better then

See above about bluetooth case.

> forcible adding fake _DSD tables everywhere, see above.

Why are they fake?

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 03-03-17 16:23, Andy Shevchenko wrote:
> On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03-03-17 15:57, Andy Shevchenko wrote:
>>> On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 02-03-17 12:38, Andy Shevchenko wrote:
>>>>> On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
>>>>>> On 22-02-17 16:52, Andy Shevchenko wrote:
>>>>>>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
>>>>>>>> On 10-02-17 12:52, Hans de Goede wrote:
>>>>>>>>> On 02-02-17 14:12, Mika Westerberg wrote:
>>>>>>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de
>>>>>>>>>> Goede
>>>>>>>>>> wrote:
>
>>>> Forcing the addition of a connection-id table to all those
>>>> drivers not only is needless churn / boilerplate, but also gives
>>>> the
>>>> false impression that we actually have more info (a valid
>>>> connection
>>>> id) then we really have.
>>>
>>> Again, we have no idea what exactly we get if we call
>>> gpiod_get_index(..., NULL, index); in case of ACPI. It would work
>>> *if
>>> and only if* we assume that all versions of the same device on all
>>> possible platforms will have *very same* mapping.
>>>
>>> I have heard it's already not true. Check the commit 89ab37b489d1
>>> ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").
>
> Any comment / remark on this?

Looking at that commit, the indxex order is consistent for a single
device HID/CID but at some point in time the decided to change
the order for newer HIDs which sucks from a driver pov, but is
less bad then it could have been (they could have different orders
for the same HID and we would need to fallback to dmi quirks).

But I fail to see how this is really relevant for this discussion.

>>>> So my vote goes to adding a label field, and passing NULL as
>>>> connection id in these drivers, rather then adding a fake
>>>> connection-
>>>> id
>>>> table.
>>>
>>> There are also few concerns:
>>>
>>> 1) it would be often passed the same string as connection ID and
>>> label
>>> (okay, meaning we need like two functions per each in current API,
>>> something like gpiod_get_*label(dev, con_id, ..., label);
>>> and gpiod_get_label_nocid(dev, ..., label); besides the former
>>> ones);
>>
>> I would think the _label variants would not allow a connection_id at
>> all.
>
> Ah, okay.
>
>>> 2) using labels different to connection ID sounds not okay for
>>> debugging
>>> purposes (I dunno why we have this done for fwnode_get_gpiod_child()
>>> in
>>> the first place);
>>
>> Right, which is why the _label variants would not allow a
>> connection_id
>> at all using an _label variants implies connection_id == NULL.
>>
>> And using a variant with connection_id argument implies label
>>   = connection_id.
>>
>>> 3) additionally different labels will not play good in _DSD enabled
>>> case, since we must use connection ID there (we believe firmware
>>> until
>>> otherwise is proven).
>>
>> Again, gpios would have a connection_id OR a label, so in
>> _DSD case only a connection_id.
>>
>>> 4) if some firmwares have different indexes for the same device we
>>> will
>>> need to have device ID (PCI ID, ... or alike) to _CRS index mapping
>>> anyway in the code.
>>
>> This problem will exist independent of which solution we choose.
>
> Yes.
>
>>>>>
>>>>> Mika, Linus, I would really appreciate your input to the topic:
>>>>> opinions, critique, ideas, etc.
>>>>
>>>> So my opinion on this is that I prefer the add a label field idea
>>>> over
>>>> the everything must have either a connection-id in ACPI or a
>>>> connection-id-table in the driver.
>>>
>>> As a ultimate workaround it would work, in long-term prospective
>>> it's
>>> not a solution.
>
>> I believe that this will work fine even in the long run, better then
>
> See above about bluetooth case.
>
>> forcible adding fake _DSD tables everywhere, see above.
>
> Why are they fake?

Because they are not coming from the firmware, as said I believe it
is better to clearly differentiate between the no-connection-id (so we
use indexes) and the firmware provides connection-ids cases.

I believe this is better for 2 reasons:

1) Cleaner (and less) code for drivers which need to use indexes
2) It is easier to debug things if it is clear at all levels if we
are dealing with indexes or connection ids

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Mon, 2017-03-06 at 10:31 +0100, Hans de Goede wrote:
> Hi,
> 
> On 03-03-17 16:23, Andy Shevchenko wrote:
> > On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 03-03-17 15:57, Andy Shevchenko wrote:
> > > > On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 02-03-17 12:38, Andy Shevchenko wrote:
> > > > > > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:
> > > > > > > On 22-02-17 16:52, Andy Shevchenko wrote:
> > > > > > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:
> > > > > > > > > On 10-02-17 12:52, Hans de Goede wrote:
> > > > > > > > > > On 02-02-17 14:12, Mika Westerberg wrote:
> > > > > > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de
> > > > > > > > > > > Goede
> > > > > > > > > > > wrote:
> > > > > Forcing the addition of a connection-id table to all those
> > > > > drivers not only is needless churn / boilerplate, but also
> > > > > gives
> > > > > the
> > > > > false impression that we actually have more info (a valid
> > > > > connection
> > > > > id) then we really have.
> > > > 
> > > > Again, we have no idea what exactly we get if we call
> > > > gpiod_get_index(..., NULL, index); in case of ACPI. It would
> > > > work
> > > > *if
> > > > and only if* we assume that all versions of the same device on
> > > > all
> > > > possible platforms will have *very same* mapping.
> > > > 
> > > > I have heard it's already not true. Check the commit
> > > > 89ab37b489d1
> > > > ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").
> > 
> > Any comment / remark on this?
> 
> Looking at that commit, the indxex order is consistent for a single
> device HID/CID but at some point in time the decided to change
> the order for newer HIDs which sucks from a driver pov, but is
> less bad then it could have been (they could have different orders
> for the same HID and we would need to fallback to dmi quirks).
> 

> But I fail to see how this is really relevant for this discussion.

My point is that the chaos with connection ID vs. label will not be
simple suppressed by adding a parameter to API. In some cases it
wouldn't be enough.

> > > > 2) using labels different to connection ID sounds not okay for
> > > > debugging
> > > > purposes (I dunno why we have this done for
> > > > fwnode_get_gpiod_child()
> > > > in
> > > > the first place);
> > > 
> > > Right, which is why the _label variants would not allow a
> > > connection_id
> > > at all using an _label variants implies connection_id == NULL.
> > > 
> > > And using a variant with connection_id argument implies label
> > >   = connection_id.
> > > 
> > > > 3) additionally different labels will not play good in _DSD
> > > > enabled
> > > > case, since we must use connection ID there (we believe firmware
> > > > until
> > > > otherwise is proven).
> > > 
> > > Again, gpios would have a connection_id OR a label, so in
> > > _DSD case only a connection_id.
> > > 
> > > > 4) if some firmwares have different indexes for the same device
> > > > we
> > > > will
> > > > need to have device ID (PCI ID, ... or alike) to _CRS index
> > > > mapping
> > > > anyway in the code.
> > > 
> > > This problem will exist independent of which solution we choose.
> > 
> > Yes.
> > 
> > > > > > 
> > > > > > Mika, Linus, I would really appreciate your input to the
> > > > > > topic:
> > > > > > opinions, critique, ideas, etc.
> > > > > 
> > > > > So my opinion on this is that I prefer the add a label field
> > > > > idea
> > > > > over
> > > > > the everything must have either a connection-id in ACPI or a
> > > > > connection-id-table in the driver.
> > > > 
> > > > As a ultimate workaround it would work, in long-term prospective
> > > > it's
> > > > not a solution.
> > > I believe that this will work fine even in the long run, better
> > > then
> > 
> > See above about bluetooth case.
> > 
> > > forcible adding fake _DSD tables everywhere, see above.
> > 
> > Why are they fake?
> 
> Because they are not coming from the firmware,

They actually are. The problem here is that older firmwares and ACPI
specification lack of naming GPIO resources. So, this information is
complimentary to the existing GPIO resources. It's not fake.

>  as said I believe it
> is better to clearly differentiate between the no-connection-id (so we
> use indexes) and the firmware provides connection-ids cases.

Indexes without label is error prone approach. Sorry, I'm not going to
vote for them. This is explained in the patches I have: we never know
what we get by index. The mapping makes it robust.

It was clearly my mistake to even think in this direction.

> I believe this is better for 2 reasons:
> 
> 1) Cleaner (and less) code for drivers which need to use indexes

Yes and no. Cleaner, indeed, but less code does not always mean less
error prone, more robust, etc.

> 2) It is easier to debug things if it is clear at all levels if we
> are dealing with indexes or connection ids

And my point that adding labels along with connection IDs is a hiding of
a huge abuse of at least ACPI case! It will not get rid of the chaos we
have. And it will make API more confusing.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 07-03-17 12:51, Andy Shevchenko wrote:
> On Mon, 2017-03-06 at 10:31 +0100, Hans de Goede wrote:

<mega snip>

>>>> forcible adding fake _DSD tables everywhere, see above.
>>>
>>> Why are they fake?
>>
>> Because they are not coming from the firmware,
>
> They actually are. The problem here is that older firmwares and ACPI
> specification lack of naming GPIO resources. So, this information is
> complimentary to the existing GPIO resources. It's not fake.
>
>>  as said I believe it
>> is better to clearly differentiate between the no-connection-id (so we
>> use indexes) and the firmware provides connection-ids cases.
>
> Indexes without label is error prone approach. Sorry, I'm not going to
> vote for them. This is explained in the patches I have: we never know
> what we get by index. The mapping makes it robust.
>
> It was clearly my mistake to even think in this direction.
>
>> I believe this is better for 2 reasons:
>>
>> 1) Cleaner (and less) code for drivers which need to use indexes
>
> Yes and no. Cleaner, indeed, but less code does not always mean less
> error prone, more robust, etc.
>
>> 2) It is easier to debug things if it is clear at all levels if we
>> are dealing with indexes or connection ids
>
> And my point that adding labels along with connection IDs is a hiding of
> a huge abuse of at least ACPI case! It will not get rid of the chaos we
> have. And it will make API more confusing.

Ok, since it seems clear that I'm not going to be able to change your
mind on this, I will give your patches a try and see if they fix the
silead ts problems.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 07-03-17 14:55, Hans de Goede wrote:
> Hi,

<more snip>

> Ok, since it seems clear that I'm not going to be able to change your
> mind on this, I will give your patches a try and see if they fix the
> silead ts problems.

So I've cherry picked all the gpio related patches from your topic/uart/rpm
branch into my wip branch and then ran some tests. I did not get around
to actually test if the fix the silead issue (I believe they will) as I
started testing on a cht device and looking if soc_button_array still
works with your patches applied.

Unfortunately it no longer works, there are 2 problems:

1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
also replace:

	desc = gpiod_get_index(dev, info->name, info->acpi_index, GPIOD_ASIS);

with:

	desc = gpiod_get(dev, info->name, GPIOD_ASIS);

At which point we can also drop the acpi_index field from the buttoninfo struct
altogether.

I think that "extcon: int3496: Add GPIO ACPI mapping table" will need
a similar change (I haven't tested it yet).

2) acpi_gpio_count() does not seem to work right in combination with the
new patches. It returns -ENOENT rather then the number of gpios specified
in the table passed to devm_acpi_dev_add_driver_gpios. It seems to only
check for gpios actually in the acpi-properties without looking at
adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for
that and disallows fallback to counting the gpios in the _CRS causing
acpi_gpio_count() to not find any gpios. I believe the right fix for
this is to make acpi_gpio_count() also count the number of entries
in the adev->driver_gpios table.

For now I've just removed the acpi_gpio_count() check from soc_button_array,
with that removed and 1) fixed soc_button_array does work.

I will try to do some more testing later today, but all my cht work is
a side project and I first need to finish some stuff for my actual
main $dayjob project.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> Hi,
> 
> On 07-03-17 14:55, Hans de Goede wrote:
> > Hi,
> 
> <more snip>

Thanks for looking into this!
My comments below.

> > Ok, since it seems clear that I'm not going to be able to change
> > your
> > mind on this, I will give your patches a try and see if they fix the
> > silead ts problems.
> 
> So I've cherry picked all the gpio related patches from your
> topic/uart/rpm
> branch into my wip branch and then ran some tests. 

Some of them are WIP, so, they might break something as well.

> I did not get around
> to actually test if the fix the silead issue (I believe they will) as
> I
> started testing on a cht device and looking if soc_button_array still
> works with your patches applied.
> 
> Unfortunately it no longer works, there are 2 problems:
> 
> 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
> also replace:
> 
> 	desc = gpiod_get_index(dev, info->name, info->acpi_index,
> GPIOD_ASIS);
> 
> with:
> 
> 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
> 
> At which point we can also drop the acpi_index field from the
> buttoninfo struct
> altogether.
> 

I was thinking about passing NULL as connection ID there as it's done
for surface3 button array driver. In current soc-button-array we have
file name passed for all of the pins, which is slightly informative.

> I think that "extcon: int3496: Add GPIO ACPI mapping table" will need
> a similar change (I haven't tested it yet).

The mapping table converts Linux index, which you pass via
gpiod_get_index(), and _CRS index pair (resource, index in a list).

If it doesn't work that way, there is another bug then.

> 2) acpi_gpio_count() does not seem to work right in combination with
> the
> new patches. It returns -ENOENT rather then the number of gpios
> specified
> in the table passed to devm_acpi_dev_add_driver_gpios. It seems to
> only
> check for gpios actually in the acpi-properties without looking at
> adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for
> that and disallows fallback to counting the gpios in the _CRS causing
> acpi_gpio_count() to not find any gpios. I believe the right fix for
> this is to make acpi_gpio_count() also count the number of entries
> in the adev->driver_gpios table.
> 

Thanks for catching this, it sounds indeed as a bug.

> For now I've just removed the acpi_gpio_count() check from
> soc_button_array,
> with that removed and 1) fixed soc_button_array does work.
> 
> I will try to do some more testing later today, but all my cht work is
> a side project and I first need to finish some stuff for my actual
> main $dayjob project.

Understood.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 08-03-17 11:30, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 07-03-17 14:55, Hans de Goede wrote:
>>> Hi,
>>
>> <more snip>
>
> Thanks for looking into this!
> My comments below.
>
>>> Ok, since it seems clear that I'm not going to be able to change
>>> your
>>> mind on this, I will give your patches a try and see if they fix the
>>> silead ts problems.
>>
>> So I've cherry picked all the gpio related patches from your
>> topic/uart/rpm
>> branch into my wip branch and then ran some tests.
>
> Some of them are WIP, so, they might break something as well.

Understood.

>> I did not get around
>> to actually test if the fix the silead issue (I believe they will) as
>> I
>> started testing on a cht device and looking if soc_button_array still
>> works with your patches applied.
>>
>> Unfortunately it no longer works, there are 2 problems:
>>
>> 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
>> also replace:
>>
>> 	desc = gpiod_get_index(dev, info->name, info->acpi_index,
>> GPIOD_ASIS);
>>
>> with:
>>
>> 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
>>
>> At which point we can also drop the acpi_index field from the
>> buttoninfo struct
>> altogether.
>>
>
> I was thinking about passing NULL as connection ID there as it's done
> for surface3 button array driver. In current soc-button-array we have
> file name passed for all of the pins, which is slightly informative.

No currently the button name, so "power", "volume-up", "volume-down"
gets passed. The only place where the file-name used to get passed
is in the gpiod_count call, but you've replaced it with NULL there.

But passing NULL and removing the need for a mapping table is fine with me.

>> I think that "extcon: int3496: Add GPIO ACPI mapping table" will need
>> a similar change (I haven't tested it yet).
>
> The mapping table converts Linux index, which you pass via
> gpiod_get_index(), and _CRS index pair (resource, index in a list).
>
> If it doesn't work that way, there is another bug then.

Hmm, so maybe this:

static const struct acpi_gpio_params power_gpios = { 0, 0, false };
static const struct acpi_gpio_params home_gpios = { 1, 0, false };
static const struct acpi_gpio_params volume_up_gpios = { 2, 0, false };
static const struct acpi_gpio_params volume_down_gpios = { 3, 0, false };
static const struct acpi_gpio_params rotation_lock_gpios = { 4, 0, false };

Needs to be like this then? :

static const struct acpi_gpio_params power_gpios = { 0, 0, false };
static const struct acpi_gpio_params home_gpios = { 1, 1, false };
static const struct acpi_gpio_params volume_up_gpios = { 2, 2, false };
static const struct acpi_gpio_params volume_down_gpios = { 3, 3, false };
static const struct acpi_gpio_params rotation_lock_gpios = { 4, 4, false };

In order for gpiod_get_index() to work ? Note that if we are going
to use the mapping table I believe we really should just use
gpiod_get (instead of gpiod_get_index) as the map also provides a
name so the index is not necessary (I've tested that using
gpiod_get() works fine with your current code).

>> 2) acpi_gpio_count() does not seem to work right in combination with
>> the
>> new patches. It returns -ENOENT rather then the number of gpios
>> specified
>> in the table passed to devm_acpi_dev_add_driver_gpios. It seems to
>> only
>> check for gpios actually in the acpi-properties without looking at
>> adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for
>> that and disallows fallback to counting the gpios in the _CRS causing
>> acpi_gpio_count() to not find any gpios. I believe the right fix for
>> this is to make acpi_gpio_count() also count the number of entries
>> in the adev->driver_gpios table.
>>
>
> Thanks for catching this, it sounds indeed as a bug.
>
>> For now I've just removed the acpi_gpio_count() check from
>> soc_button_array,
>> with that removed and 1) fixed soc_button_array does work.
>>
>> I will try to do some more testing later today, but all my cht work is
>> a side project and I first need to finish some stuff for my actual
>> main $dayjob project.
>
> Understood.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> On 08-03-17 11:30, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > On 07-03-17 14:55, Hans de Goede wrote:


> > > I did not get around
> > > to actually test if the fix the silead issue (I believe they will)
> > > as
> > > I
> > > started testing on a cht device and looking if soc_button_array
> > > still
> > > works with your patches applied.
> > > 
> > > Unfortunately it no longer works, there are 2 problems:
> > > 
> > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
> > > also replace:
> > > 
> > > 	desc = gpiod_get_index(dev, info->name, info->acpi_index,
> > > GPIOD_ASIS);
> > > 
> > > with:
> > > 
> > > 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
> > > 
> > > At which point we can also drop the acpi_index field from the
> > > buttoninfo struct
> > > altogether.
> > > 
> > 
> > I was thinking about passing NULL as connection ID there as it's
> > done
> > for surface3 button array driver. In current soc-button-array we
> > have
> > file name passed for all of the pins, which is slightly informative.
> 
> No currently the button name, so "power", "volume-up", "volume-down"
> gets passed.

Because of my WIP patches? They have FIXME: in commit message where I'm
in doubt on the way to go. 

>  The only place where the file-name used to get passed
> is in the gpiod_count call, but you've replaced it with NULL there.

Yes.


> But passing NULL and removing the need for a mapping table is fine
> with me.

NULL sounds to me a bit clearer in this case, since the original name of
connection IDs with underscores, not dashes.

> > > I think that "extcon: int3496: Add GPIO ACPI mapping table" will
> > > need
> > > a similar change (I haven't tested it yet).
> > 
> > The mapping table converts Linux index, which you pass via
> > gpiod_get_index(), and _CRS index pair (resource, index in a list).
> > 
> > If it doesn't work that way, there is another bug then.
> 
> Hmm, so maybe this:
> 
> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
> static const struct acpi_gpio_params home_gpios = { 1, 0, false };

> Needs to be like this then? :
> 
> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
> static const struct acpi_gpio_params home_gpios = { 1, 1, false };

Obviously not.

Just really small pseudo ASL to consider:

_CRS:

GpioIo(...)  { pin #5 }
GpioIo(...)  { pin #3, pin #4, pin #2 }
GpioIo(...)  { pin #15 }

In Linux (for example) [index, connection ID]:

index 0  "reset"  (pin #2)
index 1  "func1"  (pin #4)
index 2  "func2"  (pin #3)
index 3  "enable" (pin #5)
index 4  "ready"  (pin #15)

Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):

index 0  pin #2    to 1,2
index 1  pin #4    to 1,1
index 2  pin #3    to 1,0
index 3  pin #5    to 0,0
index 4  pin #15   to 2,0

> In order for gpiod_get_index() to work ? Note that if we are going
> to use the mapping table I believe we really should just use
> gpiod_get (instead of gpiod_get_index) as the map also provides a
> name so the index is not necessary (I've tested that using
> gpiod_get() works fine with your current code).

See above, I think it makes picture clearer to understand the problems
we have.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> > On 08-03-17 11:30, Andy Shevchenko wrote:
> > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > > On 07-03-17 14:55, Hans de Goede wrote:

> > > > Unfortunately it no longer works, there are 2 problems:
> > > > 
> > > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table"
> > > > should
> > > > also replace:
> > > > 
> > > > 	desc = gpiod_get_index(dev, info->name, info-
> > > > >acpi_index,
> > > > GPIOD_ASIS);
> > > > 
> > > > with:
> > > > 
> > > > 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
> > > > 
> > > > At which point we can also drop the acpi_index field from the
> > > > buttoninfo struct
> > > > altogether.

> Obviously not.

However this statement still true, you seems to be right about indexes.
What a horrible mess :-)

Below corrections to my initial view.

> Just really small pseudo ASL to consider:
> 
> _CRS:
> 
> GpioIo(...)  { pin #5 }
> GpioIo(...)  { pin #3, pin #4, pin #2 }
> GpioIo(...)  { pin #15 }
> 
> In Linux (for example) [index, connection ID]:
> 

> index 0  "reset"  (pin #2)
> index 1  "func1"  (pin #4)
> index 2  "func2"  (pin #3)

An this is completely reversed, should be

index 2  "reset"  (pin #2)
index 1  "func1"  (pin #4)
index 0  "func2"  (pin #3)

> index 3  "enable" (pin #5)
> index 4  "ready"  (pin #15)

Both above should have indexes 0 on Linux side!

> Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):
> 
> index 0  pin #2    to 1,2
> index 1  pin #4    to 1,1
> index 2  pin #3    to 1,0
> index 3  pin #5    to 0,0
> index 4  pin #15   to 2,0

Ditto.

So, basically with GPIO ACPI mapping table we have to replace indexes in
most cases to 0, which effectively means drop of _index() variant of
gpiod_get() calls. And here you are right.

Sorry for my broken picture.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 08-03-17 18:01, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote:
>> On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
>>> On 08-03-17 11:30, Andy Shevchenko wrote:
>>>> On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
>>>>> On 07-03-17 14:55, Hans de Goede wrote:
>
>>>>> Unfortunately it no longer works, there are 2 problems:
>>>>>
>>>>> 1) "Input: soc_button_array - Add GPIO ACPI mapping table"
>>>>> should
>>>>> also replace:
>>>>>
>>>>> 	desc = gpiod_get_index(dev, info->name, info-
>>>>>> acpi_index,
>>>>> GPIOD_ASIS);
>>>>>
>>>>> with:
>>>>>
>>>>> 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
>>>>>
>>>>> At which point we can also drop the acpi_index field from the
>>>>> buttoninfo struct
>>>>> altogether.
>
>> Obviously not.
>
> However this statement still true, you seems to be right about indexes.
> What a horrible mess :-)
>
> Below corrections to my initial view.
>
>> Just really small pseudo ASL to consider:
>>
>> _CRS:
>>
>> GpioIo(...)  { pin #5 }
>> GpioIo(...)  { pin #3, pin #4, pin #2 }
>> GpioIo(...)  { pin #15 }
>>
>> In Linux (for example) [index, connection ID]:
>>
>
>> index 0  "reset"  (pin #2)
>> index 1  "func1"  (pin #4)
>> index 2  "func2"  (pin #3)
>
> An this is completely reversed, should be
>
> index 2  "reset"  (pin #2)
> index 1  "func1"  (pin #4)
> index 0  "func2"  (pin #3)
>
>> index 3  "enable" (pin #5)
>> index 4  "ready"  (pin #15)
>
> Both above should have indexes 0 on Linux side!
>
>> Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):
>>
>> index 0  pin #2    to 1,2
>> index 1  pin #4    to 1,1
>> index 2  pin #3    to 1,0
>> index 3  pin #5    to 0,0
>> index 4  pin #15   to 2,0
>
> Ditto.
>
> So, basically with GPIO ACPI mapping table we have to replace indexes in
> most cases to 0, which effectively means drop of _index() variant of
> gpiod_get() calls. And here you are right.

Ok, that explains why switching to gpiod_get() fixed things for me.

But I do believe that just not using a GPIO ACPI mapping table
at all for the soc_button_array driver, combined with using
gpiod_get_index() with a NULL con-id as you suggested is the best
solution for the soc_button_array driver.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Wed, 2017-03-08 at 19:01 +0200, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> > > On 08-03-17 11:30, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > > > On 07-03-17 14:55, Hans de Goede wrote:

> Below corrections to my initial view.
> 
> > Just really small pseudo ASL to consider:
> > 
> > _CRS:
> > 
> > GpioIo(...)  { pin #5 }
> > GpioIo(...)  { pin #3, pin #4, pin #2 }
> > GpioIo(...)  { pin #15 }
> > 
> > In Linux (for example) [index, connection ID]:
> > 
> > index 0  "reset"  (pin #2)
> > index 1  "func1"  (pin #4)
> > index 2  "func2"  (pin #3)
> 

> An this is completely reversed, should be
> 
> index 2  "reset"  (pin #2)
> index 1  "func1"  (pin #4)
> index 0  "func2"  (pin #3)

zOMG, since we have *different* connection IDs, they are all 0!!!

I'm really messing up the things in my brain :-(

> 
> > index 3  "enable" (pin #5)
> > index 4  "ready"  (pin #15)
> 
> Both above should have indexes 0 on Linux side!
> 
> > Mapping Linux <-> _CRS (either from _DSD or hard coded mapping
> > table):
> > 
> > index 0  pin #2    to 1,2
> > index 1  pin #4    to 1,1
> > index 2  pin #3    to 1,0
> > index 3  pin #5    to 0,0
> > index 4  pin #15   to 2,0
> 
> Ditto.

Ditto.

-- 8< -- 8< --

Alter scheme to see the different indexes on Linux side is

In Linux (for example) [index, connection ID]:

index 2  "reset"  (pin #2)
index 1  "reset"  (pin #4)
index 0  "reset"  (pin #3)

Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):

index 2  pin #2    to 1,2
index 1  pin #4    to 1,1
index 0  pin #3    to 1,0

-- 8< -- 8< --

> Sorry for my broken picture.

Sorry again.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]

Hi,

On 08-03-17 12:46, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
>> On 08-03-17 11:30, Andy Shevchenko wrote:
>>> On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
>>>> On 07-03-17 14:55, Hans de Goede wrote:
>
>
>>>> I did not get around
>>>> to actually test if the fix the silead issue (I believe they will)
>>>> as
>>>> I
>>>> started testing on a cht device and looking if soc_button_array
>>>> still
>>>> works with your patches applied.
>>>>
>>>> Unfortunately it no longer works, there are 2 problems:
>>>>
>>>> 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
>>>> also replace:
>>>>
>>>> 	desc = gpiod_get_index(dev, info->name, info->acpi_index,
>>>> GPIOD_ASIS);
>>>>
>>>> with:
>>>>
>>>> 	desc = gpiod_get(dev, info->name, GPIOD_ASIS);
>>>>
>>>> At which point we can also drop the acpi_index field from the
>>>> buttoninfo struct
>>>> altogether.
>>>>
>>>
>>> I was thinking about passing NULL as connection ID there as it's
>>> done
>>> for surface3 button array driver. In current soc-button-array we
>>> have
>>> file name passed for all of the pins, which is slightly informative.
>>
>> No currently the button name, so "power", "volume-up", "volume-down"
>> gets passed.
>
> Because of my WIP patches? They have FIXME: in commit message where I'm
> in doubt on the way to go.

Ah yes that is changed in your WIP patches I did not notice before.

>>  The only place where the file-name used to get passed
>> is in the gpiod_count call, but you've replaced it with NULL there.
>
> Yes.
>
>
>> But passing NULL and removing the need for a mapping table is fine
>> with me.
>
> NULL sounds to me a bit clearer in this case, since the original name of
> connection IDs with underscores, not dashes.

Ok, so pass NULL and then drop the patch to add the mapping table,
because with a NULL con-id that won't be necessary right ?

I've just given this a spin (patch to pass NULl attached), your
patch to add the GPIO ACPI mapping table dropped and this works well
I agree just passing NULL as con-id is the better solution for
soc_button_array.

>>>> I think that "extcon: int3496: Add GPIO ACPI mapping table" will
>>>> need
>>>> a similar change (I haven't tested it yet).

So I assume you want to do the same (pass NULL as con-id to
gpiod_get_index()) for the extcon-in3496 driver or do you want
to keep the GPIO ACPI mapping table there?

>>>
>>> The mapping table converts Linux index, which you pass via
>>> gpiod_get_index(), and _CRS index pair (resource, index in a list).
>>>
>>> If it doesn't work that way, there is another bug then.
>>
>> Hmm, so maybe this:
>>
>> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
>> static const struct acpi_gpio_params home_gpios = { 1, 0, false };
>
>> Needs to be like this then? :
>>
>> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
>> static const struct acpi_gpio_params home_gpios = { 1, 1, false };
>
> Obviously not.
>
> Just really small pseudo ASL to consider:
>
> _CRS:
>
> GpioIo(...)  { pin #5 }
> GpioIo(...)  { pin #3, pin #4, pin #2 }
> GpioIo(...)  { pin #15 }
>
> In Linux (for example) [index, connection ID]:
>
> index 0  "reset"  (pin #2)
> index 1  "func1"  (pin #4)
> index 2  "func2"  (pin #3)
> index 3  "enable" (pin #5)
> index 4  "ready"  (pin #15)
>
> Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):
>
> index 0  pin #2    to 1,2
> index 1  pin #4    to 1,1
> index 2  pin #3    to 1,0
> index 3  pin #5    to 0,0
> index 4  pin #15   to 2,0
>
>> In order for gpiod_get_index() to work ? Note that if we are going
>> to use the mapping table I believe we really should just use
>> gpiod_get (instead of gpiod_get_index) as the map also provides a
>> name so the index is not necessary (I've tested that using
>> gpiod_get() works fine with your current code).
>
> See above, I think it makes picture clearer to understand the problems
> we have.

Yes it does, now I understand why there are 2 indexes in
struct acpi_gpio_params.

Regards,

Hans


[-- Attachment #2: 0001-Input-soc_button_array-use-NULL-for-GPIO-connection-.patch --]
[-- Type: text/x-patch, Size: 1222 bytes --]

>From 26f2b4ef3c867c7a7c9a17738219a1f7cc656bb7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 8 Mar 2017 18:00:08 +0100
Subject: [PATCH v3] Input: soc_button_array: use NULL for GPIO connection ID

The gpiolib-acpi code is becoming more strict and connection-IDs
may only be used with devices which have a _DSD with matching IDs
in there. Since the soc_button_array ACPI binding is pure index
based pass in NULL as connection-ID to avoid the more strict cheks
resulting in gpiod_get_infex not returning any gpios.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/misc/soc_button_array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 8d1c5f4..c9c492e 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -48,7 +48,7 @@ static int soc_button_lookup_gpio(struct device *dev, int acpi_index)
 	struct gpio_desc *desc;
 	int gpio;
 
-	desc = gpiod_get_index(dev, KBUILD_MODNAME, acpi_index, GPIOD_ASIS);
+	desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-- 
2.9.3

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Wed, 2017-03-08 at 18:05 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-03-17 12:46, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> > > On 08-03-17 11:30, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > > > On 07-03-17 14:55, Hans de Goede wrote:

> > NULL sounds to me a bit clearer in this case, since the original
> > name of
> > connection IDs with underscores, not dashes.
> 
> Ok, so pass NULL and then drop the patch to add the mapping table,
> because with a NULL con-id that won't be necessary right ?
> 
> I've just given this a spin (patch to pass NULl attached), your
> patch to add the GPIO ACPI mapping table dropped and this works well
> I agree just passing NULL as con-id is the better solution for
> soc_button_array.

Yeah. The attached patch you sent is fine by me.

> > > > > I think that "extcon: int3496: Add GPIO ACPI mapping table"
> > > > > will
> > > > > need
> > > > > a similar change (I haven't tested it yet).
> 
> So I assume you want to do the same (pass NULL as con-id to
> gpiod_get_index()) for the extcon-in3496 driver or do you want
> to keep the GPIO ACPI mapping table there?

A slightly preferable table variant (needs to be fixed I guess) because
initial one used to have different labels.

Though if you would like to move to NULL, I wouldn't object.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 08-03-17 19:25, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 18:05 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 08-03-17 12:46, Andy Shevchenko wrote:
>>> On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
>>>> On 08-03-17 11:30, Andy Shevchenko wrote:
>>>>> On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
>>>>>> On 07-03-17 14:55, Hans de Goede wrote:
>
>>> NULL sounds to me a bit clearer in this case, since the original
>>> name of
>>> connection IDs with underscores, not dashes.
>>
>> Ok, so pass NULL and then drop the patch to add the mapping table,
>> because with a NULL con-id that won't be necessary right ?
>>
>> I've just given this a spin (patch to pass NULl attached), your
>> patch to add the GPIO ACPI mapping table dropped and this works well
>> I agree just passing NULL as con-id is the better solution for
>> soc_button_array.
>
> Yeah. The attached patch you sent is fine by me.

Ok, I've slighty modified it to also change the KBUILD_MODNAME
passed to gpiod_count to NULL, I know that your:
"Input: soc_button_array - Propagate error from gpiod_count()"
patch: https://bitbucket.org/andy-shev/linux/commits/13b5b3e1b178fbc9b2ecaa915715f3bb8a024f88

Also does that, but that depends on the rest of your gpiolib changes,
where as this patch can be merged right now, to prepare things
for your gpiolib changes landing. So I'm going to send the
patch with the extra s/KBUILD_MODNAME/NULL/ to Dmitry for
merging into input/next. You may want to rebase your
"Input: soc_button_array - Propagate error from gpiod_count()"
patch on top, that also will make it cleaner as now it
no longer needs to do the s/KBUILD_MODNAME/NULL/.

>>>>>> I think that "extcon: int3496: Add GPIO ACPI mapping table"
>>>>>> will
>>>>>> need
>>>>>> a similar change (I haven't tested it yet).
>>
>> So I assume you want to do the same (pass NULL as con-id to
>> gpiod_get_index()) for the extcon-in3496 driver or do you want
>> to keep the GPIO ACPI mapping table there?
>
> A slightly preferable table variant (needs to be fixed I guess) because
> initial one used to have different labels.

Ok, I will test with the acpi-mapping table then and if necessary
create a fixup patch for it and send that to you.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-03-17 19:25, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 18:05 +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 08-03-17 12:46, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> > > > > On 08-03-17 11:30, Andy Shevchenko wrote:
> > > > > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > > > > On 07-03-17 14:55, Hans de Goede wrote:

> > Yeah. The attached patch you sent is fine by me.
> 
> Ok, I've slighty modified it to also change the KBUILD_MODNAME
> passed to gpiod_count to NULL, I know that your:
> "Input: soc_button_array - Propagate error from gpiod_count()"
> patch: https://bitbucket.org/andy-shev/linux/commits/13b5b3e1b178fbc9b
> 2ecaa915715f3bb8a024f88
> 
> Also does that, but that depends on the rest of your gpiolib changes,
> where as this patch can be merged right now, to prepare things
> for your gpiolib changes landing. So I'm going to send the
> patch with the extra s/KBUILD_MODNAME/NULL/ to Dmitry for
> merging into input/next. You may want to rebase your
> "Input: soc_button_array - Propagate error from gpiod_count()"
> patch on top, that also will make it cleaner as now it
> no longer needs to do the s/KBUILD_MODNAME/NULL/.

Go ahead!

I pushed few hours ago updated version of the branch where I applied
that your previous patch and reverted mine regarding to soc-button-array 
and surface3_button drivers.

I will rebase my stuff on your new patch.

> > > > > > > I think that "extcon: int3496: Add GPIO ACPI mapping
> > > > > > > table"
> > > > > > > will
> > > > > > > need
> > > > > > > a similar change (I haven't tested it yet).
> > > 
> > > So I assume you want to do the same (pass NULL as con-id to
> > > gpiod_get_index()) for the extcon-in3496 driver or do you want
> > > to keep the GPIO ACPI mapping table there?
> > 
> > A slightly preferable table variant (needs to be fixed I guess)
> > because
> > initial one used to have different labels.
> 
> Ok, I will test with the acpi-mapping table then and if necessary
> create a fixup patch for it and send that to you.

Sounds like a plan!

Since extcon patches are landed already mainstream it might make sense
to send it as usual to all maintainers.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 09-03-17 15:03, Andy Shevchenko wrote:
> On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 08-03-17 19:25, Andy Shevchenko wrote:

<snip soc_button_array stuff>

>>>>>>>> I think that "extcon: int3496: Add GPIO ACPI mapping
>>>>>>>> table"
>>>>>>>> will
>>>>>>>> need
>>>>>>>> a similar change (I haven't tested it yet).
>>>>
>>>> So I assume you want to do the same (pass NULL as con-id to
>>>> gpiod_get_index()) for the extcon-in3496 driver or do you want
>>>> to keep the GPIO ACPI mapping table there?
>>>
>>> A slightly preferable table variant (needs to be fixed I guess)
>>> because
>>> initial one used to have different labels.
>>
>> Ok, I will test with the acpi-mapping table then and if necessary
>> create a fixup patch for it and send that to you.
>
> Sounds like a plan!
>
> Since extcon patches are landed already mainstream it might make sense
> to send it as usual to all maintainers.

Ack, so to be clear we should use gpiod_get not gpiod_get_index with
the acpi mapping table, right ? The reason I'm asking is that my
test devices only have the id pin which has index 0 so in my
experience with soc_button_array it will work with both
(the button with index 0 would even work with gpiod_get_index).

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Thu, 2017-03-09 at 15:45 +0100, Hans de Goede wrote:

> > Sounds like a plan!
> > 
> > Since extcon patches are landed already mainstream it might make
> > sense
> > to send it as usual to all maintainers.
> 
> Ack, so to be clear we should use gpiod_get not gpiod_get_index with
> the acpi mapping table, right ? The reason I'm asking is that my
> test devices only have the id pin which has index 0 so in my
> experience with soc_button_array it will work with both
> (the button with index 0 would even work with gpiod_get_index).

TL;DR -- right.


So, now simple and clean example:

_CRS:

GpioIo(...)  { pin #5 }
GpioIo(...)  { pin #3, pin #4, pin #2 }
GpioIo(...)  { pin #15 }

If we assume each line represents one function (connection ID):
"func0"
"func1"
"func2"

we would see that index is needed only when we would like to get access
to pin #4 or pin #2 of "func1".

Was:

gpiod_get_index(..., NULL, <index_in_CRS>);

where index is 0,1, or 2 *with second index assumed 0*!

Now, what we actually is doing we mapping connection ID to the first
index and can use index to access mentioned above pins:

gpiod_get_index(..., "<funcX>", <secondary_index_in_CRS>);

For example, for pin #2 or #4
gpiod_get_index(..., "func1", 2); // pin #2
gpiod_get_index(..., "func1", 1); // pin #4

Thus,
gpiod_get_index(..., "func1", 0); // pin #3


Or just for the first (virtual) column:

gpiod_get(..., "<funcX>");

where pin #5, #3 or #15 is accessible.

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 09-03-17 16:03, Andy Shevchenko wrote:
> On Thu, 2017-03-09 at 15:45 +0100, Hans de Goede wrote:
>
>>> Sounds like a plan!
>>>
>>> Since extcon patches are landed already mainstream it might make
>>> sense
>>> to send it as usual to all maintainers.
>>
>> Ack, so to be clear we should use gpiod_get not gpiod_get_index with
>> the acpi mapping table, right ? The reason I'm asking is that my
>> test devices only have the id pin which has index 0 so in my
>> experience with soc_button_array it will work with both
>> (the button with index 0 would even work with gpiod_get_index).
>
> TL;DR -- right.
>
>
> So, now simple and clean example:
>
> _CRS:
>
> GpioIo(...)  { pin #5 }
> GpioIo(...)  { pin #3, pin #4, pin #2 }
> GpioIo(...)  { pin #15 }
>
> If we assume each line represents one function (connection ID):
> "func0"
> "func1"
> "func2"
>
> we would see that index is needed only when we would like to get access
> to pin #4 or pin #2 of "func1".
>
> Was:
>
> gpiod_get_index(..., NULL, <index_in_CRS>);
>
> where index is 0,1, or 2 *with second index assumed 0*!
>
> Now, what we actually is doing we mapping connection ID to the first
> index and can use index to access mentioned above pins:
>
> gpiod_get_index(..., "<funcX>", <secondary_index_in_CRS>);
>
> For example, for pin #2 or #4
> gpiod_get_index(..., "func1", 2); // pin #2
> gpiod_get_index(..., "func1", 1); // pin #4
>
> Thus,
> gpiod_get_index(..., "func1", 0); // pin #3
>
>
> Or just for the first (virtual) column:
>
> gpiod_get(..., "<funcX>");
>
> where pin #5, #3 or #15 is accessible.

Ah, I understand so the meaning of index changes and
we should always pass 0 for resources where there
is one gpio per resource. Yes that explains why I
needed to switch to plain gpiod_get for the
soc_button_array to work and the same goes
for extcon-int3496 as that also has one gpio
per resource.

Ok I will test with that fixed and get back to you.

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 09-03-17 15:45, Hans de Goede wrote:
> Hi,
>
> On 09-03-17 15:03, Andy Shevchenko wrote:
>> On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 08-03-17 19:25, Andy Shevchenko wrote:
>
> <snip soc_button_array stuff>
>
>>>>>>>>> I think that "extcon: int3496: Add GPIO ACPI mapping
>>>>>>>>> table"
>>>>>>>>> will
>>>>>>>>> need
>>>>>>>>> a similar change (I haven't tested it yet).
>>>>>
>>>>> So I assume you want to do the same (pass NULL as con-id to
>>>>> gpiod_get_index()) for the extcon-in3496 driver or do you want
>>>>> to keep the GPIO ACPI mapping table there?
>>>>
>>>> A slightly preferable table variant (needs to be fixed I guess)
>>>> because
>>>> initial one used to have different labels.
>>>
>>> Ok, I will test with the acpi-mapping table then and if necessary
>>> create a fixup patch for it and send that to you.
>>
>> Sounds like a plan!
>>
>> Since extcon patches are landed already mainstream it might make sense
>> to send it as usual to all maintainers.
>
> Ack, so to be clear we should use gpiod_get not gpiod_get_index with
> the acpi mapping table, right ? The reason I'm asking is that my
> test devices only have the id pin which has index 0 so in my
> experience with soc_button_array it will work with both
> (the button with index 0 would even work with gpiod_get_index).

Ok, so there is one problem with the extcon-intel-int3496.c
together with your gpio patches:

[ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
[ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3 for IRQ
[ 2382.484429] genirq: Failed to request resources for INT3496:00 (irq 174) on irqchip chv-gpio
[ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB ID GPIO: -22
[ 2382.500359] intel-int3496: probe of INT3496:00 failed with error -22

This can be fixed / worked around by doing this:


--- a/drivers/extcon/extcon-intel-int3496.c
+++ b/drivers/extcon/extcon-intel-int3496.c
@@ -113,6 +113,7 @@ static int int3496_probe(struct platform_device *pdev)
                 dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
                 return ret;
         }
+       gpiod_direction_input(data->gpio_usb_id);

         data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
         if (data->usb_id_irq < 0) {

But the gpio is requested as:

         data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);

Note the GPIOD_IN I think the new behavior is caused by:

https://bitbucket.org/andy-shev/linux/commits/ba458eb945879a66eac75de69a4e7312c4f44cc7

Of which I'm not sure it is a good idea, as the dsdt of my
cube iwork8 air shows:

         Device (OTG2)
         {
             Name (_HID, "INT3496")  // _HID: Hardware ID
             Name (_CID, "INT3496")  // _CID: Compatible ID

Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
{
     Name (ABUF, ResourceTemplate ()
     {
         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
             )
             {   // Pin list
                 0x0003
             }
         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
             "\\_SB.GPO3", 0x00, ResourceConsumer, ,
             )
             {   // Pin list
                 0x002E
             }
     })
     Return (ABUF) /* \_SB_.PCI0.OTG2._CRS.ABUF */
}

}

The dstd-s out there in the wild do not always get this right.

With my workaround from above the extcon-intel-int3496 driver
does work properly again, so I see 2 options here:

1) Let drivers workaround known broken IoRestriction in dstd-s
by using gpiod_direction_...

2) Drop the patch which makes gpiod_get honor the IoRestrictions

I've also tested the silead driver and with the new more strict
gpio code it works as it should as-is (as expected).

Regards,

Hans

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Dmitry Torokhov]

On Thu, Mar 09, 2017 at 07:48:06PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 09-03-17 15:45, Hans de Goede wrote:
> >Hi,
> >
> >On 09-03-17 15:03, Andy Shevchenko wrote:
> >>On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
> >>>Hi,
> >>>
> >>>On 08-03-17 19:25, Andy Shevchenko wrote:
> >
> ><snip soc_button_array stuff>
> >
> >>>>>>>>>I think that "extcon: int3496: Add GPIO ACPI mapping
> >>>>>>>>>table"
> >>>>>>>>>will
> >>>>>>>>>need
> >>>>>>>>>a similar change (I haven't tested it yet).
> >>>>>
> >>>>>So I assume you want to do the same (pass NULL as con-id to
> >>>>>gpiod_get_index()) for the extcon-in3496 driver or do you want
> >>>>>to keep the GPIO ACPI mapping table there?
> >>>>
> >>>>A slightly preferable table variant (needs to be fixed I guess)
> >>>>because
> >>>>initial one used to have different labels.
> >>>
> >>>Ok, I will test with the acpi-mapping table then and if necessary
> >>>create a fixup patch for it and send that to you.
> >>
> >>Sounds like a plan!
> >>
> >>Since extcon patches are landed already mainstream it might make sense
> >>to send it as usual to all maintainers.
> >
> >Ack, so to be clear we should use gpiod_get not gpiod_get_index with
> >the acpi mapping table, right ? The reason I'm asking is that my
> >test devices only have the id pin which has index 0 so in my
> >experience with soc_button_array it will work with both
> >(the button with index 0 would even work with gpiod_get_index).
> 
> Ok, so there is one problem with the extcon-intel-int3496.c
> together with your gpio patches:
> 
> [ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
> [ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3 for IRQ
> [ 2382.484429] genirq: Failed to request resources for INT3496:00 (irq 174) on irqchip chv-gpio
> [ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB ID GPIO: -22
> [ 2382.500359] intel-int3496: probe of INT3496:00 failed with error -22
> 
> This can be fixed / worked around by doing this:
> 
> 
> --- a/drivers/extcon/extcon-intel-int3496.c
> +++ b/drivers/extcon/extcon-intel-int3496.c
> @@ -113,6 +113,7 @@ static int int3496_probe(struct platform_device *pdev)
>                 dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
>                 return ret;
>         }
> +       gpiod_direction_input(data->gpio_usb_id);
> 
>         data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
>         if (data->usb_id_irq < 0) {
> 
> But the gpio is requested as:
> 
>         data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);


If devm_gpiod_get(dev, "id", GPIOD_IN) fails but gpiod_direction_input()
works it means there is a bug in gpiod_direction_input().

> 
> Note the GPIOD_IN I think the new behavior is caused by:
> 
> https://bitbucket.org/andy-shev/linux/commits/ba458eb945879a66eac75de69a4e7312c4f44cc7
> 
> Of which I'm not sure it is a good idea, as the dsdt of my
> cube iwork8 air shows:
> 
>         Device (OTG2)
>         {
>             Name (_HID, "INT3496")  // _HID: Hardware ID
>             Name (_CID, "INT3496")  // _CID: Compatible ID
> 
> Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> {
>     Name (ABUF, ResourceTemplate ()
>     {
>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>             )
>             {   // Pin list
>                 0x0003
>             }
>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>             "\\_SB.GPO3", 0x00, ResourceConsumer, ,
>             )
>             {   // Pin list
>                 0x002E
>             }
>     })
>     Return (ABUF) /* \_SB_.PCI0.OTG2._CRS.ABUF */
> }
> 
> }
> 
> The dstd-s out there in the wild do not always get this right.
> 
> With my workaround from above the extcon-intel-int3496 driver
> does work properly again, so I see 2 options here:
> 
> 1) Let drivers workaround known broken IoRestriction in dstd-s
> by using gpiod_direction_...
> 
> 2) Drop the patch which makes gpiod_get honor the IoRestrictions
> 
> I've also tested the silead driver and with the new more strict
> gpio code it works as it should as-is (as expected).

It looks like IoRestriction enforcement should be optional and it is
another thing to be tweaked in drivers/platform/x86/whatever.c

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Mika Westerberg]

On Thu, Mar 09, 2017 at 01:32:00PM -0800, Dmitry Torokhov wrote:
> It looks like IoRestriction enforcement should be optional and it is
> another thing to be tweaked in drivers/platform/x86/whatever.c

If the firmware says the pin is only to be used as output, I don't think
it is really good idea to make that kind of enforcement optional.

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Thu, 2017-03-09 at 19:48 +0100, Hans de Goede wrote:
> Hi,
> 
> On 09-03-17 15:45, Hans de Goede wrote:
> > Hi,
> > 
> > On 09-03-17 15:03, Andy Shevchenko wrote:
> > > On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:

> Ok, so there is one problem with the extcon-intel-int3496.c
> together with your gpio patches:
> 
> [ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq:
> tried to flag a GPIO set as output for IRQ
> [ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3
> for IRQ
> [ 2382.484429] genirq: Failed to request resources for INT3496:00 (irq
> 174) on irqchip chv-gpio
> [ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB ID
> GPIO: -22
> [ 2382.500359] intel-int3496: probe of INT3496:00 failed with error
> -22

By the way did not you see a warning from gpiolib-acpi.c that flags are
overridden by firmware?

> 
> This can be fixed / worked around by doing this:
> 
> 
> --- a/drivers/extcon/extcon-intel-int3496.c
> +++ b/drivers/extcon/extcon-intel-int3496.c
> @@ -113,6 +113,7 @@ static int int3496_probe(struct platform_device
> *pdev)
>                  dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
>                  return ret;
>          }

> +       gpiod_direction_input(data->gpio_usb_id);

(1)

> 
>          data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
>          if (data->usb_id_irq < 0) {
> 
> But the gpio is requested as:
> 
>          data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);
> 
> Note the GPIOD_IN I think the new behavior is caused by:
> 
> https://bitbucket.org/andy-shev/linux/commits/ba458eb945879a66eac75de6
> 9a4e7312c4f44cc7
> 
> Of which I'm not sure it is a good idea, as the dsdt of my
> cube iwork8 air shows:
> 
>          Device (OTG2)
>          {
>              Name (_HID, "INT3496")  // _HID: Hardware ID
>              Name (_CID, "INT3496")  // _CID: Compatible ID
> 
> Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> {
>      Name (ABUF, ResourceTemplate ()
>      {

>          GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
> IoRestrictionOutputOnly,
>              "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>              )
>              {   // Pin list
>                  0x0003
>              }

This is definitely bug in firmware.

>          GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
> IoRestrictionOutputOnly,
>              "\\_SB.GPO3", 0x00, ResourceConsumer, ,
>              )
>              {   // Pin list
>                  0x002E
>              }
>      })
>      Return (ABUF) /* \_SB_.PCI0.OTG2._CRS.ABUF */
> }
> 
> }
> 
> The dstd-s out there in the wild do not always get this right.

Yeah and here is the dilemma of making life easier to the user and
provoke vendors to abuse standards, or say "no".

In this particular case it is even worse since driver is in upstream and
it's working. (2)

> 
> With my workaround from above the extcon-intel-int3496 driver
> does work properly again, so I see 2 options here:
> 
> 1) Let drivers workaround known broken IoRestriction in dstd-s
> by using gpiod_direction_...

I see two options here (at least):
a) override DSDT completely (or in the future perhaps partially);
b) use workaround in the driver.

Taking into consideration (2) I would go with option b) and warn LOUDLY
in (1) that firmware has a bug which we aware of.

> 2) Drop the patch which makes gpiod_get honor the IoRestrictions

I would go 1).

> I've also tested the silead driver and with the new more strict
> gpio code it works as it should as-is (as expected).

Thanks!

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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Andy Shevchenko]

On Fri, 2017-03-10 at 13:33 +0200, Andy Shevchenko wrote:
> On Thu, 2017-03-09 at 19:48 +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 09-03-17 15:45, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 09-03-17 15:03, Andy Shevchenko wrote:
> > > > On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
> > Ok, so there is one problem with the extcon-intel-int3496.c
> > together with your gpio patches:
> > 
> > [ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq:
> > tried to flag a GPIO set as output for IRQ
> > [ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3
> > for IRQ
> > [ 2382.484429] genirq: Failed to request resources for INT3496:00
> > (irq
> > 174) on irqchip chv-gpio
> > [ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB
> > ID
> > GPIO: -22
> > [ 2382.500359] intel-int3496: probe of INT3496:00 failed with error
> > -22
> 
> By the way did not you see a warning from gpiolib-acpi.c that flags
> are
> overridden by firmware?

Yeah, it's dev_dbg(). Perhaps needs to be changed to dev_warn()?


> > --- a/drivers/extcon/extcon-intel-int3496.c
> > +++ b/drivers/extcon/extcon-intel-int3496.c

Just a reminder that we need
--- a/drivers/extcon/extcon-intel-int3496.c
+++ b/drivers/extcon/extcon-intel-int3496.c
@@ -107,9 +107,7 @@ static int int3496_probe(struct platform_device
*pdev)
        data->dev = dev;
        INIT_DELAYED_WORK(&data->work, int3496_do_usb_id);

-       data->gpio_usb_id = devm_gpiod_get_index(dev, "id",
-                                               INT3496_GPIO_USB_ID,
-                                               GPIOD_IN);
+       data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);
        if (IS_ERR(data->gpio_usb_id)) {
                ret = PTR_ERR(data->gpio_usb_id);
                dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
@@ -122,15 +120,11 @@ static int int3496_probe(struct platform_device
*pdev)
                return data->usb_id_irq;
        }

-       data->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus",
-                                                INT3496_GPIO_VBUS_EN,
-                                                GPIOD_ASIS);
+       data->gpio_vbus_en = devm_gpiod_get(dev, "vbus", GPIOD_ASIS);
        if (IS_ERR(data->gpio_vbus_en))
                dev_info(dev, "can't request VBUS EN GPIO\n");

-       data->gpio_usb_mux = devm_gpiod_get_index(dev, "mux",
-                                                INT3496_GPIO_USB_MUX,
-                                                GPIOD_ASIS);
+       data->gpio_usb_mux = devm_gpiod_get(dev, "mux", GPIOD_ASIS);
        if (IS_ERR(data->gpio_usb_mux))
                dev_info(dev, "can't request USB MUX GPIO\n");


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

Re: [PATCH v2] Input: silead - Do not try to directly access the GPIO when using ACPI pm

[Hans de Goede]

Hi,

On 03/10/2017 12:33 PM, Andy Shevchenko wrote:
> On Thu, 2017-03-09 at 19:48 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 09-03-17 15:45, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 09-03-17 15:03, Andy Shevchenko wrote:
>>>> On Thu, 2017-03-09 at 14:57 +0100, Hans de Goede wrote:
>
>> Ok, so there is one problem with the extcon-intel-int3496.c
>> together with your gpio patches:
>>
>> [ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq:
>> tried to flag a GPIO set as output for IRQ
>> [ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3
>> for IRQ
>> [ 2382.484429] genirq: Failed to request resources for INT3496:00 (irq
>> 174) on irqchip chv-gpio
>> [ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB ID
>> GPIO: -22
>> [ 2382.500359] intel-int3496: probe of INT3496:00 failed with error
>> -22
>
> By the way did not you see a warning from gpiolib-acpi.c that flags are
> overridden by firmware?

Nope (as you figured out already in your next reply).
>
>>
>> This can be fixed / worked around by doing this:
>>
>>
>> --- a/drivers/extcon/extcon-intel-int3496.c
>> +++ b/drivers/extcon/extcon-intel-int3496.c
>> @@ -113,6 +113,7 @@ static int int3496_probe(struct platform_device
>> *pdev)
>>                  dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
>>                  return ret;
>>          }
>
>> +       gpiod_direction_input(data->gpio_usb_id);
>
> (1)
>
>>
>>          data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
>>          if (data->usb_id_irq < 0) {
>>
>> But the gpio is requested as:
>>
>>          data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);
>>
>> Note the GPIOD_IN I think the new behavior is caused by:
>>
>> https://bitbucket.org/andy-shev/linux/commits/ba458eb945879a66eac75de6
>> 9a4e7312c4f44cc7
>>
>> Of which I'm not sure it is a good idea, as the dsdt of my
>> cube iwork8 air shows:
>>
>>          Device (OTG2)
>>          {
>>              Name (_HID, "INT3496")  // _HID: Hardware ID
>>              Name (_CID, "INT3496")  // _CID: Compatible ID
>>
>> Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>> {
>>      Name (ABUF, ResourceTemplate ()
>>      {
>
>>          GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> IoRestrictionOutputOnly,
>>              "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>>              )
>>              {   // Pin list
>>                  0x0003
>>              }
>
> This is definitely bug in firmware.

Yes, agreed, but it is wat it is and we will have to deal with it.

>>          GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> IoRestrictionOutputOnly,
>>              "\\_SB.GPO3", 0x00, ResourceConsumer, ,
>>              )
>>              {   // Pin list
>>                  0x002E
>>              }
>>      })
>>      Return (ABUF) /* \_SB_.PCI0.OTG2._CRS.ABUF */
>> }
>>
>> }
>>
>> The dstd-s out there in the wild do not always get this right.
>
> Yeah and here is the dilemma of making life easier to the user and
> provoke vendors to abuse standards, or say "no".
>
> In this particular case it is even worse since driver is in upstream and
> it's working. (2)
>
>>
>> With my workaround from above the extcon-intel-int3496 driver
>> does work properly again, so I see 2 options here:
>>
>> 1) Let drivers workaround known broken IoRestriction in dstd-s
>> by using gpiod_direction_...
>
> I see two options here (at least):
> a) override DSDT completely (or in the future perhaps partially);
> b) use workaround in the driver.
>
> Taking into consideration (2) I would go with option b) and warn LOUDLY
> in (1) that firmware has a bug which we aware of.
>
>> 2) Drop the patch which makes gpiod_get honor the IoRestrictions
>
> I would go 1).

Ok, I will send a patch for this, as well as switching from get_index to normal
get upstream.

Regards,

Hans