Hi Sakari,
On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Rafael,
>
> On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Add support for parsing property references using strings, besides
> > > > > > > reference objects that were previously supported. This allows also
> > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > >
> > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > >
> > > > > > > While at it, update copyright.
> > > > > >
> > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > still be made.
> > > > > >
> > > > > > First off, the above changelog is a bit terse.
> > > > > >
> > > > > > I think that it would help to provide an example of device properties
> > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > now.
> > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > --- a/drivers/acpi/property.c
> > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > @@ -2,14 +2,17 @@
> > > > > > > /*
> > > > > > > * ACPI device specific properties support.
> > > > > > > *
> > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > > * All rights reserved.
> > > > > > > *
> > > > > > > * Authors: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > > > > > - * Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > > > > > > - * Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > > > > + * Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > > > > > > + * Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > > >
> > > > > > I'm not sure if the whitespace change here is really useful.
> > > > >
> > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > for indentation.
> > > > >
> > > > > >
> > > > > > > + * Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > > > > */
> > > > > > >
> > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > +
> > > > > > > #include <linux/acpi.h>
> > > > > > > #include <linux/device.h>
> > > > > > > #include <linux/export.h>
> > > > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > > > struct fwnode_handle *ref_fwnode,
> > > > > > > const union acpi_object **element,
> > > > > > > - const union acpi_object *end, size_t num_args)
> > > > > > > + const union acpi_object *end, size_t num_args,
> > > > > > > + bool subnode_string)
> > > > > >
> > > > > > The meaning of the new argument isn't really clear. it would be good
> > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > easily.
> > > > >
> > > > > I can add comments to v9.
> > > >
> > > > If you can send me an example of ASL that will be parsed correctly
> > > > after this change, but not before, it will help a bit.
> > >
> > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > >
> > > Package () {
> > > "mipi-img-flash-leds",
> > > Package () {
> > > "\\_SB.PCI0.I2C2.LEDD.LED0",
> > > "\\_SB.PCI0.I2C2.LEDD.LED1"
> > > },
> > > },
> > >
> > > It's a property with a string reference to an ACPI non-device node,
> > > although you can refer to device nodes as well.
> >
> > This example is missing the definition of LED0 or LED1 from which it
> > would be clear that they are data nodes (or at least one of them is a
> > data node).
>
> Ok, perhaps this one could work as a complete example, with a single
> reference:
>
> Package ()
> {
> "mipi-img-flash-leds", "\\_SB.PCI0.I2C2.LEDD.LED0",
> }
>
> Device (LEDD)
> {
> Name (_DSD, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
> Package ()
> {
> Package ()
> {
> "mipi-img-flash-led-0",
> "LED0",
> }
> },
> })
> Name (LED0, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> Package ()
> {
> Package ()
> {
> "mipi-img-max-current",
> 1000000,
> }
> }
> })
> }
>
This works, thanks!
> >
> > Also I'm kind of wondering about the "reference with arguments" part
> > which seems to work differently depending on whether the reference is
> > represented by a string or by a reference object.
>
> Yes. With (device) reference objects, it is possible currently to refer to
> subnodes with the _DSD data extension child names of those nodes. This is
> not done with string references as 1) any node can already be referenced so
> there's no need to and 2) as node references are strings already, it's not
> possible to distinguish node string references from _DSD data node names.
> E.g.
>
> "\\_SB.I2C0.LED0", "LED1"
>
> ^ ACPI object name or _DSD data node name?
>
Has this behavior been documented anywhere? Or is there any
expectation to see anything like this shipping in production platform
firmware?
If any of the above isn't the case, I would be inclined to simply
remove this special case and make both the "object reference" and
"string" cases work in the same way and if someone needs to refer to a
data node, they will just need to use a string (in which case it will
be the only option).
