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 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