Re: [PATCH v3 04/15] ACPI: Document ACPI device specific properties

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Saturday, October 04, 2014 12:59:59 PM Arnd Bergmann wrote:
> On Saturday 04 October 2014 02:13:23 Rafael J. Wysocki wrote:
> > > Because people get the format wrong regardless of documentation. The
> > > format:
> > > 
> > > Package () {
> > >       Package () { ^ref1, data, data },
> > >       Package () { ^ref2, data },
> > >       Package () { ^ref3, data, data, data },
> > > }
> > > 
> > > Is superior to the format:
> > > 
> > > Package () { ^ref1, data, data, ^ref2, data, ^ref3, data, data, data }
> > > 
> > > Because in the former you have delimiters that can be used to verify
> > > each tuple. Imagine someone misses a data element for one of these
> > > tuples. In the former layout you can detect this easily while in the
> > > latter you cannot.
> > 
> > I agree with this particular thing (although other people seem to have
> > problems with too many package nesting levels) but I'm not sure what that
> > has to do with the example given above (let me quote again):
> > 
> > > Putting everything to a single package results this:
> > > 
> > >       Package () { "pwms", Package () {"led-red", ^PWM0, 0, 10, "led-green", ^PWM0, 1, 10 }}
> > > 
> > > But I think the below looks better:
> > > 
> > >       Package () { "pwms", Package () {^PWM0, 0, 10, ^PWM0, 1, 10 }}
> > >       Package () { "pwm-names", Package () {"led-red", "led-green"}}
> > > 
> > > and it is trivial to match with the corresponding DT fragment.
> > 
> > that I was commenting.  Both cases contains the
> > 
> >         Package () { ^ref1, data, data, ^ref2, data, ^ref3, data, data, data }
> > 
> > format that you don't like, don't they?
> > 
> 
> There are two independent issues:
> 
> a) avoiding the need for "pwm-names" by embedding the name in the
>    "pwms" property
> 
> b) avoiding the need for "#pwm-cells" by having explicit separators between
>    entries in a "pwms" property.
> 
> It would be possible to do one but not the other.

OK

So aside from theorethical considerations it's good to look at what the
current code is doing in my opinion.

In the current patchset, acpi_dev_get_property_reference() is only used
in one place, which is acpi_get_gpiod_by_index().  That, in turn, is only
used by acpi_find_gpio() and by dev_get_named_gpiod_from_child() (and its
devm variant).  acpi_find_gpio() passes 0 as index (which means it only
cares about the first reference in the list) and the _get_named_gpiod_from_child()
thigs are used by leds-gpio and gpio-keys-polled.  Each of them also uses
0 as the index (so they only care about the first reference in the list as well).

Thus, arguably, acpi_dev_get_property_reference() in its current form is
sufficient for what we need at the moment.  There simply is no need to make
it more complicated right now and we can extend it in the future if need be.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.