Hi Rafael,
On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> 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,
}
}
})
}
>
> 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?
>
> > You can get the spec from here:
> > <URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
>
> Sure, but it alone won't help me much with documenting this code change.
Not as such but the spec is publicly available now.
--
Kind regards,
Sakari Ailus
