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).
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.
> 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.
