Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Rafael,

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@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > 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@linux.intel.com> 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@linux.intel.com>
> > > > ---
> > > >  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@linux.intel.com>
> > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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@linux.intel.com>
> > > >   */
> > > >
> > > > +#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.

You can get the spec from here:
<URL:https://www.mipi.org/mipi-disco-for-imaging-download>.

-- 
Kind regards,

Sakari Ailus