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 <[email protected]>
---
This is on top of latest linux-pm/device-properties.
drivers/gpio/gpiolib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fbf717a56b0a..58659dbe702a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1522,7 +1522,7 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
suffixes[i]);
}
- desc = acpi_get_gpiod_by_index(adev, propname, 0, &info);
+ desc = acpi_get_gpiod_by_index(adev, propname, idx, &info);
if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
break;
}
--
2.1.1
On 10/28/14 4:15, 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 <[email protected]>
Nice, that was a gap that had been gnawing at me. Thanks Mika :-)
Acked-by: Darren Hart <[email protected]>
--
Darren Hart
Intel Open Source Technology Center
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 <[email protected]>
Cool. :-)
Any objections anyone?
> ---
> This is on top of latest linux-pm/device-properties.
>
> drivers/gpio/gpiolib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index fbf717a56b0a..58659dbe702a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1522,7 +1522,7 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
> suffixes[i]);
> }
>
> - desc = acpi_get_gpiod_by_index(adev, propname, 0, &info);
> + desc = acpi_get_gpiod_by_index(adev, propname, idx, &info);
> if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
> break;
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 10/29/2014 06:59 AM, Rafael J. Wysocki 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 <[email protected]>
>
> Cool. :-)
>
> Any objections anyone?
Looks good to me!
Acked-by: Alexandre Courbot <[email protected]>
Since this looks like a bug fix, shouldn't this be squashed into the
relevant patch of the device-properties set?
On Wed, Oct 29, 2014 at 04:41:47PM +0900, Alexandre Courbot wrote:
> On 10/29/2014 06:59 AM, Rafael J. Wysocki 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 <[email protected]>
> >
> >Cool. :-)
> >
> >Any objections anyone?
>
> Looks good to me!
>
> Acked-by: Alexandre Courbot <[email protected]>
Thanks!
> Since this looks like a bug fix, shouldn't this be squashed into the
> relevant patch of the device-properties set?
I agree.
On Wednesday, October 29, 2014 10:54:29 AM Mika Westerberg wrote:
> On Wed, Oct 29, 2014 at 04:41:47PM +0900, Alexandre Courbot wrote:
> > On 10/29/2014 06:59 AM, Rafael J. Wysocki 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 <[email protected]>
> > >
> > >Cool. :-)
> > >
> > >Any objections anyone?
> >
> > Looks good to me!
> >
> > Acked-by: Alexandre Courbot <[email protected]>
>
> Thanks!
>
> > Since this looks like a bug fix, shouldn't this be squashed into the
> > relevant patch of the device-properties set?
>
> I agree.
Well, except that the set is in my linux-next branch now and I very much
prefer to do fixes on top of it.
Besides, I'm not sure if that even matters for the current series.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wednesday, October 29, 2014 03:42:23 PM Rafael J. Wysocki wrote:
> On Wednesday, October 29, 2014 10:54:29 AM Mika Westerberg wrote:
> > On Wed, Oct 29, 2014 at 04:41:47PM +0900, Alexandre Courbot wrote:
> > > On 10/29/2014 06:59 AM, Rafael J. Wysocki 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 <[email protected]>
> > > >
> > > >Cool. :-)
> > > >
> > > >Any objections anyone?
> > >
> > > Looks good to me!
> > >
> > > Acked-by: Alexandre Courbot <[email protected]>
> >
> > Thanks!
> >
> > > Since this looks like a bug fix, shouldn't this be squashed into the
> > > relevant patch of the device-properties set?
> >
> > I agree.
>
> Well, except that the set is in my linux-next branch now and I very much
> prefer to do fixes on top of it.
>
> Besides, I'm not sure if that even matters for the current series.
Never mind. I need to rebase the series anyway because of a bug in the
second patch, so I'll fold the $subject one into patch [5/12].
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, Oct 29, 2014 at 03:51:59PM +0100, Rafael J. Wysocki wrote:
> Never mind. I need to rebase the series anyway because of a bug in the
> second patch, so I'll fold the $subject one into patch [5/12].
Thank you.
On Tue, 28 Oct 2014 22:59:57 +0100
, "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
>
> 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.
g.
On 11/1/14 4:11, Grant Likely wrote:
> On Tue, 28 Oct 2014 22:59:57 +0100
> , "Rafael J. Wysocki" <[email protected]>
> 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 <[email protected]>
>>
>> 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
2.1 Data Format Definition
...
" a Package consisting entirely of Integer, String, or Reference objects
(and
specifically not containing a nested Package)."
That said, I am not fond of the many properties mixed in as a single
Package. We discussed this at some length while this was being proposed,
and this was deemed the lesser evil.
--
Darren Hart
Intel Open Source Technology Center
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" <[email protected]>
> > 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 <[email protected]>
> >>
> >> 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?
Rafael
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" <[email protected]>
> > > 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 <[email protected]>
> > >>
> > >> 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
On Mon, Nov 3, 2014 at 9:52 PM, Rafael J. Wysocki <[email protected]> wrote:
> 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" <[email protected]>
>> > > 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 <[email protected]>
>> > >>
>> > >> 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.
Okay, this make sense. I'm okay with this approach, and I would
recommend making that the only valid method for parsing in
acpi_dev_get_property_reference(). Get rid of the *size_prop argument
so that it always behaves the same way and users aren't tempted to do
something clever.
>
> 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.
Probably not. With a pure list it isn't implicitly referencing data in
another node. In the ref+args pattern the length of each tuple can
vary based on which node it references, but on a simple list the
parsing is going to be a lot simpler.
g.
On Tuesday, November 04, 2014 02:48:40 PM Grant Likely wrote:
> On Mon, Nov 3, 2014 at 9:52 PM, Rafael J. Wysocki <[email protected]> wrote:
> > 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" <[email protected]>
> >> > > 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 <[email protected]>
> >> > >>
> >> > >> 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.
>
> Okay, this make sense. I'm okay with this approach, and I would
> recommend making that the only valid method for parsing in
> acpi_dev_get_property_reference(). Get rid of the *size_prop argument
> so that it always behaves the same way and users aren't tempted to do
> something clever.
OK, I'll send a followup patch to remove the size_prop arg from
acpi_dev_get_property_reference().
> > 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.
>
> Probably not. With a pure list it isn't implicitly referencing data in
> another node. In the ref+args pattern the length of each tuple can
> vary based on which node it references, but on a simple list the
> parsing is going to be a lot simpler.
OK
Rafael
On Tuesday, November 04, 2014 05:06:40 PM Rafael J. Wysocki wrote:
> On Tuesday, November 04, 2014 02:48:40 PM Grant Likely wrote:
> > On Mon, Nov 3, 2014 at 9:52 PM, Rafael J. Wysocki <[email protected]> wrote:
> > > 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" <[email protected]>
> > >> > > 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 <[email protected]>
> > >> > >>
> > >> > >> 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.
> >
> > Okay, this make sense. I'm okay with this approach, and I would
> > recommend making that the only valid method for parsing in
> > acpi_dev_get_property_reference(). Get rid of the *size_prop argument
> > so that it always behaves the same way and users aren't tempted to do
> > something clever.
>
> OK, I'll send a followup patch to remove the size_prop arg from
> acpi_dev_get_property_reference().
This:
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
The size_prop argument of the recently added function
acpi_dev_get_property_reference() is not used by the only current
caller of that function and is very unlikely to be used at any time
going forward.
Namely, for a property whose value is a list of items each containing
a references to a device object possibly accompanied by some integers,
the number of items in the list can always be computed as the number
of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
Thus it should never be necessary to provide an additional "cells"
property with a value equal to the number of items in that list.
For this reason, drop the size_prop argument from
acpi_dev_get_property_reference() and update its caller accordingly.
Link: http://marc.info/?l=linux-kernel&m=141511255610556&w=2
Suggested-by: Grant Likely <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
On top of
http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties
---
drivers/acpi/property.c | 62 +++++++++++---------------------------------
drivers/gpio/gpiolib-acpi.c | 2 -
include/linux/acpi.h | 4 +-
3 files changed, 19 insertions(+), 49 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -273,25 +273,21 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_property_
* acpi_dev_get_property_reference - returns handle to the referenced object
* @adev: ACPI device to get property
* @name: Name of the property
- * @size_prop: Name of the "size" property in referenced object
* @index: Index of the reference to return
* @args: Location to store the returned reference with optional arguments
*
* Find property with @name, verifify that it is a package containing at least
* one object reference and if so, store the ACPI device object pointer to the
- * target object in @args->adev.
+ * target object in @args->adev. If the reference includes arguments, store
+ * them in the @args->args[] array.
*
- * If the reference includes arguments (@size_prop is not %NULL) follow the
- * reference and check whether or not there is an integer property @size_prop
- * under the target object and if so, whether or not its value matches the
- * number of arguments that follow the reference. If there's more than one
- * reference in the property value package, @index is used to select the one to
- * return.
+ * If there's more than one reference in the property value package, @index is
+ * used to select the one to return.
*
* Return: %0 on success, negative error code on failure.
*/
-int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
- const char *size_prop, size_t index,
+int acpi_dev_get_property_reference(struct acpi_device *adev,
+ const char *name, size_t index,
struct acpi_reference_args *args)
{
const union acpi_object *element, *end;
@@ -308,7 +304,7 @@ int acpi_dev_get_property_reference(stru
* return that reference then.
*/
if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) {
- if (size_prop || index)
+ if (index)
return -EINVAL;
ret = acpi_bus_get_device(obj->reference.handle, &device);
@@ -348,42 +344,16 @@ int acpi_dev_get_property_reference(stru
element++;
nargs = 0;
- if (size_prop) {
- const union acpi_object *prop;
-
- /*
- * Find out how many arguments the refenced object
- * expects by reading its size_prop property.
- */
- ret = acpi_dev_get_property(device, size_prop,
- ACPI_TYPE_INTEGER, &prop);
- if (ret)
- return ret;
-
- nargs = prop->integer.value;
- if (nargs > MAX_ACPI_REFERENCE_ARGS
- || element + nargs > end)
+ /* assume following integer elements are all args */
+ for (i = 0; element + i < end; i++) {
+ int type = element[i].type;
+
+ if (type == ACPI_TYPE_INTEGER)
+ nargs++;
+ else if (type == ACPI_TYPE_LOCAL_REFERENCE)
+ break;
+ else
return -EPROTO;
-
- /*
- * Skip to the start of the arguments and verify
- * that they all are in fact integers.
- */
- for (i = 0; i < nargs; i++)
- if (element[i].type != ACPI_TYPE_INTEGER)
- return -EPROTO;
- } else {
- /* assume following integer elements are all args */
- for (i = 0; element + i < end; i++) {
- int type = element[i].type;
-
- if (type == ACPI_TYPE_INTEGER)
- nargs++;
- else if (type == ACPI_TYPE_LOCAL_REFERENCE)
- break;
- else
- return -EPROTO;
- }
}
if (idx++ == index) {
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -718,8 +718,8 @@ int acpi_dev_get_property(struct acpi_de
int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
acpi_object_type type,
const union acpi_object **obj);
-int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
- const char *cells_name, size_t index,
+int acpi_dev_get_property_reference(struct acpi_device *adev,
+ const char *name, size_t index,
struct acpi_reference_args *args);
int acpi_dev_prop_get(struct acpi_device *adev, const char *propname,
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -405,7 +405,7 @@ struct gpio_desc *acpi_get_gpiod_by_inde
dev_dbg(&adev->dev, "GPIO: looking up %s\n", propname);
memset(&args, 0, sizeof(args));
- ret = acpi_dev_get_property_reference(adev, propname, NULL,
+ ret = acpi_dev_get_property_reference(adev, propname,
index, &args);
if (ret) {
bool found = acpi_get_driver_gpio_data(adev, propname,
On Tue, Nov 4, 2014 at 10:54 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Tuesday, November 04, 2014 05:06:40 PM Rafael J. Wysocki wrote:
>> On Tuesday, November 04, 2014 02:48:40 PM Grant Likely wrote:
>> > On Mon, Nov 3, 2014 at 9:52 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > > 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" <[email protected]>
>> > >> > > 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 <[email protected]>
>> > >> > >>
>> > >> > >> 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.
>> >
>> > Okay, this make sense. I'm okay with this approach, and I would
>> > recommend making that the only valid method for parsing in
>> > acpi_dev_get_property_reference(). Get rid of the *size_prop argument
>> > so that it always behaves the same way and users aren't tempted to do
>> > something clever.
>>
>> OK, I'll send a followup patch to remove the size_prop arg from
>> acpi_dev_get_property_reference().
>
> This:
>
> ---
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
>
> The size_prop argument of the recently added function
> acpi_dev_get_property_reference() is not used by the only current
> caller of that function and is very unlikely to be used at any time
> going forward.
>
> Namely, for a property whose value is a list of items each containing
> a references to a device object possibly accompanied by some integers,
> the number of items in the list can always be computed as the number
> of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
> Thus it should never be necessary to provide an additional "cells"
> property with a value equal to the number of items in that list.
>
> For this reason, drop the size_prop argument from
> acpi_dev_get_property_reference() and update its caller accordingly.
Beautiful.
Acked-by: Grant Likely <[email protected]>
>
> Link: http://marc.info/?l=linux-kernel&m=141511255610556&w=2
> Suggested-by: Grant Likely <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> On top of
>
> http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties
>
> ---
> drivers/acpi/property.c | 62 +++++++++++---------------------------------
> drivers/gpio/gpiolib-acpi.c | 2 -
> include/linux/acpi.h | 4 +-
> 3 files changed, 19 insertions(+), 49 deletions(-)
>
> Index: linux-pm/drivers/acpi/property.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/property.c
> +++ linux-pm/drivers/acpi/property.c
> @@ -273,25 +273,21 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_property_
> * acpi_dev_get_property_reference - returns handle to the referenced object
> * @adev: ACPI device to get property
> * @name: Name of the property
> - * @size_prop: Name of the "size" property in referenced object
> * @index: Index of the reference to return
> * @args: Location to store the returned reference with optional arguments
> *
> * Find property with @name, verifify that it is a package containing at least
> * one object reference and if so, store the ACPI device object pointer to the
> - * target object in @args->adev.
> + * target object in @args->adev. If the reference includes arguments, store
> + * them in the @args->args[] array.
> *
> - * If the reference includes arguments (@size_prop is not %NULL) follow the
> - * reference and check whether or not there is an integer property @size_prop
> - * under the target object and if so, whether or not its value matches the
> - * number of arguments that follow the reference. If there's more than one
> - * reference in the property value package, @index is used to select the one to
> - * return.
> + * If there's more than one reference in the property value package, @index is
> + * used to select the one to return.
> *
> * Return: %0 on success, negative error code on failure.
> */
> -int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
> - const char *size_prop, size_t index,
> +int acpi_dev_get_property_reference(struct acpi_device *adev,
> + const char *name, size_t index,
> struct acpi_reference_args *args)
> {
> const union acpi_object *element, *end;
> @@ -308,7 +304,7 @@ int acpi_dev_get_property_reference(stru
> * return that reference then.
> */
> if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) {
> - if (size_prop || index)
> + if (index)
> return -EINVAL;
>
> ret = acpi_bus_get_device(obj->reference.handle, &device);
> @@ -348,42 +344,16 @@ int acpi_dev_get_property_reference(stru
> element++;
> nargs = 0;
>
> - if (size_prop) {
> - const union acpi_object *prop;
> -
> - /*
> - * Find out how many arguments the refenced object
> - * expects by reading its size_prop property.
> - */
> - ret = acpi_dev_get_property(device, size_prop,
> - ACPI_TYPE_INTEGER, &prop);
> - if (ret)
> - return ret;
> -
> - nargs = prop->integer.value;
> - if (nargs > MAX_ACPI_REFERENCE_ARGS
> - || element + nargs > end)
> + /* assume following integer elements are all args */
> + for (i = 0; element + i < end; i++) {
> + int type = element[i].type;
> +
> + if (type == ACPI_TYPE_INTEGER)
> + nargs++;
> + else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> + break;
> + else
> return -EPROTO;
> -
> - /*
> - * Skip to the start of the arguments and verify
> - * that they all are in fact integers.
> - */
> - for (i = 0; i < nargs; i++)
> - if (element[i].type != ACPI_TYPE_INTEGER)
> - return -EPROTO;
> - } else {
> - /* assume following integer elements are all args */
> - for (i = 0; element + i < end; i++) {
> - int type = element[i].type;
> -
> - if (type == ACPI_TYPE_INTEGER)
> - nargs++;
> - else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> - break;
> - else
> - return -EPROTO;
> - }
> }
>
> if (idx++ == index) {
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -718,8 +718,8 @@ int acpi_dev_get_property(struct acpi_de
> int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
> acpi_object_type type,
> const union acpi_object **obj);
> -int acpi_dev_get_property_reference(struct acpi_device *adev, const char *name,
> - const char *cells_name, size_t index,
> +int acpi_dev_get_property_reference(struct acpi_device *adev,
> + const char *name, size_t index,
> struct acpi_reference_args *args);
>
> int acpi_dev_prop_get(struct acpi_device *adev, const char *propname,
> Index: linux-pm/drivers/gpio/gpiolib-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
> +++ linux-pm/drivers/gpio/gpiolib-acpi.c
> @@ -405,7 +405,7 @@ struct gpio_desc *acpi_get_gpiod_by_inde
> dev_dbg(&adev->dev, "GPIO: looking up %s\n", propname);
>
> memset(&args, 0, sizeof(args));
> - ret = acpi_dev_get_property_reference(adev, propname, NULL,
> + ret = acpi_dev_get_property_reference(adev, propname,
> index, &args);
> if (ret) {
> bool found = acpi_get_driver_gpio_data(adev, propname,
>
On Tue, Nov 04, 2014 at 11:54:41PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
>
> The size_prop argument of the recently added function
> acpi_dev_get_property_reference() is not used by the only current
> caller of that function and is very unlikely to be used at any time
> going forward.
>
> Namely, for a property whose value is a list of items each containing
> a references to a device object possibly accompanied by some integers,
> the number of items in the list can always be computed as the number
> of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
> Thus it should never be necessary to provide an additional "cells"
> property with a value equal to the number of items in that list.
>
> For this reason, drop the size_prop argument from
> acpi_dev_get_property_reference() and update its caller accordingly.
>
> Link: http://marc.info/?l=linux-kernel&m=141511255610556&w=2
> Suggested-by: Grant Likely <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Just pulled your latest bleeding-edge (including this patch) and it
still works fine with both Minnowboards. Feel free to add my:
Acked-by: Mika Westerberg <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
On 11/4/14 14:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
>
> The size_prop argument of the recently added function
> acpi_dev_get_property_reference() is not used by the only current
> caller of that function and is very unlikely to be used at any time
> going forward.
>
> Namely, for a property whose value is a list of items each containing
> a references to a device object possibly accompanied by some integers,
> the number of items in the list can always be computed as the number
> of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
> Thus it should never be necessary to provide an additional "cells"
> property with a value equal to the number of items in that list.
In this case, do we never expect a property to contain more than one
ACPI_TYPE_LOCAL_REFERENCE?
Package () { "foobar",
Package () {
"PCI0.FOO", "PCI0.BAR", 0, 1, 0,
"PCI0.FOO", "PCI0.BAR2", 0, 1, 1
}
}
This seems like it could be useful for connecting various types of
devices together, but I confess not to have a specific exmaple in mind.
It does concern me to limit the data format in this way.
I suppose should such a case become necessary, we can deal with the
issue then - and still avoid having a potential abuse point in the API
from the start.
--
Darren Hart
Intel Open Source Technology Center
On Tuesday, November 04, 2014 03:42:38 PM Darren Hart wrote:
>
> On 11/4/14 14:54, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> > Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
> >
> > The size_prop argument of the recently added function
> > acpi_dev_get_property_reference() is not used by the only current
> > caller of that function and is very unlikely to be used at any time
> > going forward.
> >
> > Namely, for a property whose value is a list of items each containing
> > a references to a device object possibly accompanied by some integers,
> > the number of items in the list can always be computed as the number
> > of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
> > Thus it should never be necessary to provide an additional "cells"
> > property with a value equal to the number of items in that list.
>
> In this case, do we never expect a property to contain more than one
> ACPI_TYPE_LOCAL_REFERENCE?
>
> Package () { "foobar",
> Package () {
> "PCI0.FOO", "PCI0.BAR", 0, 1, 0,
> "PCI0.FOO", "PCI0.BAR2", 0, 1, 1
> }
> }
>
> This seems like it could be useful for connecting various types of
> devices together, but I confess not to have a specific exmaple in mind.
> It does concern me to limit the data format in this way.
We don't support this even with size_prop, so it doesn't seem to be relevant here.
Now, if we were to support this, I'd rather not use acpi_dev_get_property_reference()
for that, but add a new function specifically for it. Moreover, I would extend the
format definition then so that we could do
Package () {
"foobar", Package () {
Package () {"PCI0.FOO", "PCI0.BAR", 0, 1, 0},
Package () {"PCI0.FOO", "PCI0.BAR2", 0, 1, 1}
}
}
in which case adding a special "size" property could be avoided.
That said, I have no idea why it might be necessary. One reference in a property
value means that we're connecting the current node (the owner of the _DSD
containing that property) with some other node in the namespace. Two references
in there would mean that the current node is to be connected with *two* other
nodes in the namespace at the same time. That raises some questions that I'd
rather not consider in detail here, unless you insist. ;-)
> I suppose should such a case become necessary, we can deal with the
> issue then - and still avoid having a potential abuse point in the API
> from the start.
What we have today is sufficient for all of the cases we've considered so far.
If we find a case where it is not sufficient, we'll need to consider extending
the data format as well as the API.
Rafael
On Wednesday, November 05, 2014 11:16:22 AM Mika Westerberg wrote:
> On Tue, Nov 04, 2014 at 11:54:41PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> > Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
> >
> > The size_prop argument of the recently added function
> > acpi_dev_get_property_reference() is not used by the only current
> > caller of that function and is very unlikely to be used at any time
> > going forward.
> >
> > Namely, for a property whose value is a list of items each containing
> > a references to a device object possibly accompanied by some integers,
> > the number of items in the list can always be computed as the number
> > of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
> > Thus it should never be necessary to provide an additional "cells"
> > property with a value equal to the number of items in that list.
> >
> > For this reason, drop the size_prop argument from
> > acpi_dev_get_property_reference() and update its caller accordingly.
> >
> > Link: http://marc.info/?l=linux-kernel&m=141511255610556&w=2
> > Suggested-by: Grant Likely <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> Just pulled your latest bleeding-edge (including this patch) and it
> still works fine with both Minnowboards. Feel free to add my:
>
> Acked-by: Mika Westerberg <[email protected]>
> Tested-by: Mika Westerberg <[email protected]>
Thanks!
On 11/5/14 12:59, Rafael J. Wysocki wrote:
> On Tuesday, November 04, 2014 03:42:38 PM Darren Hart wrote:
>>
>> On 11/4/14 14:54, Rafael J. Wysocki wrote:
>>
>>> From: Rafael J. Wysocki <[email protected]>
>>> Subject: ACPI / property: Drop size_prop from acpi_dev_get_property_reference()
>>>
>>> The size_prop argument of the recently added function
>>> acpi_dev_get_property_reference() is not used by the only current
>>> caller of that function and is very unlikely to be used at any time
>>> going forward.
>>>
>>> Namely, for a property whose value is a list of items each containing
>>> a references to a device object possibly accompanied by some integers,
>>> the number of items in the list can always be computed as the number
>>> of elements of type ACPI_TYPE_LOCAL_REFERENCE in the property package.
>>> Thus it should never be necessary to provide an additional "cells"
>>> property with a value equal to the number of items in that list.
>>
>> In this case, do we never expect a property to contain more than one
>> ACPI_TYPE_LOCAL_REFERENCE?
>>
>> Package () { "foobar",
>> Package () {
>> "PCI0.FOO", "PCI0.BAR", 0, 1, 0,
>> "PCI0.FOO", "PCI0.BAR2", 0, 1, 1
>> }
>> }
>>
>> This seems like it could be useful for connecting various types of
>> devices together, but I confess not to have a specific exmaple in mind.
>> It does concern me to limit the data format in this way.
>
> We don't support this even with size_prop, so it doesn't seem to be relevant here.
>
> Now, if we were to support this, I'd rather not use acpi_dev_get_property_reference()
> for that, but add a new function specifically for it. Moreover, I would extend the
> format definition then so that we could do
>
> Package () {
> "foobar", Package () {
> Package () {"PCI0.FOO", "PCI0.BAR", 0, 1, 0},
> Package () {"PCI0.FOO", "PCI0.BAR2", 0, 1, 1}
> }
> }
>
> in which case adding a special "size" property could be avoided.
>
> That said, I have no idea why it might be necessary. One reference in a property
> value means that we're connecting the current node (the owner of the _DSD
> containing that property) with some other node in the namespace. Two references
> in there would mean that the current node is to be connected with *two* other
> nodes in the namespace at the same time. That raises some questions that I'd
> rather not consider in detail here, unless you insist. ;-)
>
>> I suppose should such a case become necessary, we can deal with the
>> issue then - and still avoid having a potential abuse point in the API
>> from the start.
>
> What we have today is sufficient for all of the cases we've considered so far.
> If we find a case where it is not sufficient, we'll need to consider extending
> the data format as well as the API.
>
> Rafael
Agreed on all points. Thanks Rafael.
--
Darren Hart
Intel Open Source Technology Center