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:
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);
I would think the _label variants would not allow a connection_id at all.
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.
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.
I believe that this will work fine even in the long run, better then
forcible adding fake _DSD tables everywhere, see above.
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
