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
>From 26f2b4ef3c867c7a7c9a17738219a1f7cc656bb7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
