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 Mon, 2017-03-06 at 10:31 +0100, Hans de Goede wrote:
> Hi,
> 
> On 03-03-17 16:23, Andy Shevchenko wrote:
> > 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?
> 
> Looking at that commit, the indxex order is consistent for a single
> device HID/CID but at some point in time the decided to change
> the order for newer HIDs which sucks from a driver pov, but is
> less bad then it could have been (they could have different orders
> for the same HID and we would need to fallback to dmi quirks).
> 

> But I fail to see how this is really relevant for this discussion.

My point is that the chaos with connection ID vs. label will not be
simple suppressed by adding a parameter to API. In some cases it
wouldn't be enough.

> > > > 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?
> 
> Because they are not coming from the firmware,

They actually are. The problem here is that older firmwares and ACPI
specification lack of naming GPIO resources. So, this information is
complimentary to the existing GPIO resources. It's not fake.

>  as said I believe it
> is better to clearly differentiate between the no-connection-id (so we
> use indexes) and the firmware provides connection-ids cases.

Indexes without label is error prone approach. Sorry, I'm not going to
vote for them. This is explained in the patches I have: we never know
what we get by index. The mapping makes it robust.

It was clearly my mistake to even think in this direction.

> I believe this is better for 2 reasons:
> 
> 1) Cleaner (and less) code for drivers which need to use indexes

Yes and no. Cleaner, indeed, but less code does not always mean less
error prone, more robust, etc.

> 2) It is easier to debug things if it is clear at all levels if we
> are dealing with indexes or connection ids

And my point that adding labels along with connection IDs is a hiding of
a huge abuse of at least ACPI case! It will not get rid of the chaos we
have. And it will make API more confusing.

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