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 Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote:
> Hi,
> 
> On 03-03-17 15:57, Andy Shevchenko wrote:
> > 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:

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

Any comment / remark on this?

> > > 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);
> 
> I would think the _label variants would not allow a connection_id at
> all.

Ah, okay.

> > 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);
> 
> Right, which is why the _label variants would not allow a
> connection_id
> at all using an _label variants implies connection_id == NULL.
> 
> And using a variant with connection_id argument implies label
>   = connection_id.
> 
> > 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).
> 
> Again, gpios would have a connection_id OR a label, so in
> _DSD case only a connection_id.
> 
> > 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.
> 
> This problem will exist independent of which solution we choose.

Yes.

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

> I believe that this will work fine even in the long run, better then

See above about bluetooth case.

> forcible adding fake _DSD tables everywhere, see above.

Why are they fake?

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