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

Commit Message

Hans de Goede March 8, 2017, 5:05 p.m. UTC

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

Comments

Andy Shevchenko March 8, 2017, 6:25 p.m. UTC | #1

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.

Hans de Goede March 9, 2017, 1:57 p.m. UTC | #2

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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Shevchenko March 9, 2017, 2:03 p.m. UTC | #3

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.

Hans de Goede March 9, 2017, 2:45 p.m. UTC | #4

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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Shevchenko March 9, 2017, 3:03 p.m. UTC | #5

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.

Hans de Goede March 9, 2017, 3:40 p.m. UTC | #6

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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

From 26f2b4ef3c867c7a7c9a17738219a1f7cc656bb7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <[e-mail]>
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 <[e-mail]>
---
 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