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