Hi,

On 03-03-17 16:23, Andy Shevchenko wrote:

On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote:

quoted

Hi,

On 03-03-17 15:57, Andy Shevchenko wrote:

quoted

On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote:

quoted

Hi,

On 02-03-17 12:38, Andy Shevchenko wrote:

quoted

On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote:

quoted

On 22-02-17 16:52, Andy Shevchenko wrote:

quoted

On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote:

quoted

On 10-02-17 12:52, Hans de Goede wrote:

quoted

On 02-02-17 14:12, Mika Westerberg wrote:

quoted

On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de
Goede
wrote:

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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

See above about bluetooth case.

quoted

forcible adding fake _DSD tables everywhere, see above.

Why are they fake?

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

I believe this is better for 2 reasons:

1) Cleaner (and less) code for drivers which need to use indexes
2) It is easier to debug things if it is clear at all levels if we
are dealing with indexes or connection ids

Regards,

Hans