Re: [PATCH] ACPI / GPIO: Pass index to acpi_get_gpiod_by_index() when using properties

[Rafael J. Wysocki]

On Monday, November 03, 2014 04:25:08 PM Rafael J. Wysocki wrote:
> On Sunday, November 02, 2014 08:49:37 PM Darren Hart wrote:
> > 
> > On 11/1/14 4:11, Grant Likely wrote:
> > > On Tue, 28 Oct 2014 22:59:57 +0100
> > > , "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > >  wrote:
> > >> On Tuesday, October 28, 2014 01:15:27 PM Mika Westerberg wrote:
> > >>> acpi_dev_add_driver_gpios() makes it possible to set up mapping between
> > >>> properties and ACPI GpioIo resources in a driver, so we can take index
> > >>> parameter in acpi_find_gpio() into use with _DSD device properties now.
> > >>>
> > >>> This index can be used to select a GPIO from a property with multiple
> > >>> GPIOs:
> > >>>
> > >>>   Package () {
> > >>>   	"data-gpios",
> > >>>   	Package () {
> > >>>   		\_SB.GPIO, 0, 0, 0,
> > >>>   		\_SB.GPIO, 1, 0, 0,
> > >>>   		\_SB.GPIO, 2, 0, 1,
> > >>>   	}
> > >>>   }
> > >>>
> > >>> In order to retrieve the last GPIO from a driver we can simply do:
> > >>>
> > >>>   desc = devm_gpiod_get_index(dev, "data", 2);
> > >>>
> > >>> and so on.
> > >>>
> > >>> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > >>
> > >> Cool. :-)
> > >>
> > >> Any objections anyone?
> > > 
> > > Actually, I do. Not in the idea, but in the implementation. The way this gets encoded:
> > > 
> > > 	Package () {
> > > 		\_SB.GPIO, 0, 0, 0,
> > > 		\_SB.GPIO, 1, 0, 0,
> > > 		\_SB.GPIO, 2, 0, 1,
> > > 	}
> > > 
> > > Means that decoding each GPIO tuple requires the length of a tuple to be
> > > fixed, or to implement a DT-like #gpio-cells. If it is fixed, then there
> > > is no way to expand the binding later. Can this be done in the following
> > > way instead?
> > > 
> > > 	Package () {
> > > 		Package () { \_SB.GPIO, 0, 0, 0 },
> > > 		Package () { \_SB.GPIO, 1, 0, 0 },
> > > 		Package () { \_SB.GPIO, 2, 0, 1 },
> > > 	}
> > > 
> > > This is one of the biggest pains in device tree. We don't have any way
> > > to group tuples so it requires looking up stuff across the tree to
> > > figure out how to parse each multi-item property.
> > > 
> > > I know that last year we talked about how bios vendors would get
> > > complicated properties wrong, but I think there is little risk in this
> > > case. If the property is encoded wrong, the driver simply won't work and
> > > it is unlikely to get shipped before being fixed.
> > 
> > This particular nesting of Packages is expressly prohibited by the
> > Device Properties UUID for the reasons you mention.
> > 
> > http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> 
> Also we don't use properties where single name is assigned to multiple GPIOs
> anywhere in the current device-properties patchset, so this is not relevant at
> the moment.
> 
> Moreover, even if we were to use them, we would need to ensure that this:
> 
> 	Package () {
> 		\_SB.GPIO, 0, 0, 0
> 	}
> 
> was equivalent to
> 
> 	Package () {
>  		Package () { \_SB.GPIO, 0, 0, 0 }
> 	}
> 
> This is not impossible to do and I suppose we could even explain that in the
> implementation guide document in a sensible way, but that would require the
> document linked above to be changed first and *then* we can think about writing
> kernel code to it.  Not the other way around, please.
> 
> So Grant, do you want us to proceed with that?

Before you reply, one more observation that seems to be relevant.

In ACPI, both this:

	Package () {
		\_SB.GPIO, 0, 0, 0,
		\_SB.GPIO, 1, 0, 0,
		\_SB.GPIO, 2, 0, 1,
	}

and this:

	Package () {
		Package () { \_SB.GPIO, 0, 0, 0 },
		Package () { \_SB.GPIO, 1, 0, 0 },
		Package () { \_SB.GPIO, 2, 0, 1 },
	}

carry the same information, because every element of a package has a type,
so there is no danger of confusing an ACPI_TYPE_LOCAL_REFERENCE with
ACPI_TYPE_INTEGER.  Thus one can easily count the number of GPIOs represented
by the first package by counting the number of reference elements in it.
The second one has more structure which in this particular case is arguably
redundant.

Of course, that's not the case for list properties where each item consists
of a bunch of integers, like

	Package () {
		Package () { 0, 0, 0 },
		Package () { 1, 0, 0 },
		Package () { 2, 0, 1 },
	}

but I'm not sure if this is relevant at all.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/