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 Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:
> 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:

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

Yeah, and we will be screwed if the index matrix is changed per device
ID / board.

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

Again, we have no idea what exactly we get if we call
gpiod_get_index(..., NULL, index); in case of ACPI. It would work *if
and only if* we assume that all versions of the same device on all
possible platforms will have *very same* mapping.

I have heard it's already not true. Check the commit 89ab37b489d1
("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96").

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

There are also few concerns:

1) it would be often passed the same string as connection ID and label
(okay, meaning we need like two functions per each in current API,
something like gpiod_get_*label(dev, con_id, ..., label);
and gpiod_get_label_nocid(dev, ..., label); besides the former ones);

2) using labels different to connection ID sounds not okay for debugging
purposes (I dunno why we have this done for fwnode_get_gpiod_child() in
the first place);

3) additionally different labels will not play good in _DSD enabled
case, since we must use connection ID there (we believe firmware until
otherwise is proven).

4) if some firmwares have different indexes for the same device we will
need to have device ID (PCI ID, ... or alike) to _CRS index mapping
anyway in the code.

> > > 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/27f4d31dcf7997613dfb0d
> > b1df
> > 4182673826646c
> > 
> > (with flag removed approach)
> > https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf1
> > 7093
> > dacccd30e349cc
> > 
> > and in commit message
> > 
> > https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3
> > a139
> > 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.

As a ultimate workaround it would work, in long-term prospective it's
not a solution.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy