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

[Hans de Goede <hdegoede@redhat.com>]

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