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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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