| Message ID | 20230329100951.1522322-3-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand |
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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> This one looks good to me, so formally Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> and please add this tag if you resubmit. Alternatively, I can just queue it up, because IMV this is an improvement regardless of the rest of the series. > --- > 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> > + * 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) > { > u32 nargs = 0, i; > > @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > * Find the referred data extension node under the > * referred device node. > */ > - for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > - (*element)++) { > - const char *child_name = (*element)->string.pointer; > - > - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name); > - if (!ref_fwnode) > - return -EINVAL; > + if (subnode_string) { > + for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > + (*element)++) { > + const char *child_name = (*element)->string.pointer; > + > + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, > + child_name); > + if (!ref_fwnode) > + return -EINVAL; > + } > } > > /* > @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > for (i = 0; (*element) + i < end && i < num_args; i++) { > acpi_object_type type = (*element)[i].type; > > - if (type == ACPI_TYPE_LOCAL_REFERENCE) > + if (type == ACPI_TYPE_LOCAL_REFERENCE || > + (!subnode_string && type == ACPI_TYPE_STRING)) > break; > > if (type == ACPI_TYPE_INTEGER) > @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > return 0; > } > > +static struct fwnode_handle * > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring) > +{ > + acpi_handle scope, handle; > + struct acpi_data_node *dn; > + struct acpi_device *device; > + acpi_status status; > + > + if (is_acpi_device_node(fwnode)) { > + scope = to_acpi_device_node(fwnode)->handle; > + } else if (is_acpi_data_node(fwnode)) { > + scope = to_acpi_data_node(fwnode)->handle; > + } else { > + pr_debug("bad node type for node %pfw\n", fwnode); > + return ERR_PTR(-EINVAL); > + } > + > + status = acpi_get_handle(scope, refstring, &handle); > + if (ACPI_FAILURE(status)) { > + acpi_handle_debug(scope, "can't get handle for %s", refstring); > + return ERR_PTR(-EINVAL); > + } > + > + device = acpi_fetch_acpi_dev(handle); > + if (device) > + return acpi_fwnode_handle(device); > + > + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag, > + (void **)&dn, NULL); > + if (ACPI_FAILURE(status) || !dn) { > + acpi_handle_debug(handle, "can't find subnode"); > + return ERR_PTR(-EINVAL); > + } > + > + return &dn->fwnode; > +} > + > /** > * __acpi_node_get_property_reference - returns handle to the referenced object > * @fwnode: Firmware node to get the property from > @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > const union acpi_object *element, *end; > const union acpi_object *obj; > const struct acpi_device_data *data; > + struct fwnode_handle *ref_fwnode; > struct acpi_device *device; > int ret, idx = 0; > > @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > args->fwnode = acpi_fwnode_handle(device); > args->nargs = 0; > + return 0; > + case ACPI_TYPE_STRING: > + if (index) > + return -ENOENT; > + > + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer); > + if (IS_ERR(ref_fwnode)) > + return PTR_ERR(ref_fwnode); > + > + args->fwnode = ref_fwnode; > + args->nargs = 0; > + > return 0; > case ACPI_TYPE_PACKAGE: > /* > * If it is not a single reference, then it is a package of > - * references followed by number of ints as follows: > + * references, followed by number of ints as follows: > * > * Package () { REF, INT, REF, INT, INT } > * > - * The index argument is then used to determine which reference > - * the caller wants (along with the arguments). > + * Here, REF may be either a local reference or a string. The > + * index argument is then used to determine which reference the > + * caller wants (along with the arguments). > */ > break; > default: > @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > ret = acpi_get_ref_args(idx == index ? args : NULL, > acpi_fwnode_handle(device), > - &element, end, num_args); > + &element, end, num_args, true); > + if (ret < 0) > + return ret; > + > + if (idx == index) > + return 0; > + > + break; > + case ACPI_TYPE_STRING: > + ref_fwnode = > + acpi_parse_string_ref(fwnode, > + element->string.pointer); > + if (IS_ERR(ref_fwnode)) > + return PTR_ERR(ref_fwnode); > + > + element++; > + > + ret = acpi_get_ref_args(idx == index ? args : NULL, > + ref_fwnode, &element, end, > + num_args, false); > if (ret < 0) > return ret; > > -- > 2.30.2 >
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. > + * 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. > { > u32 nargs = 0, i; > > @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > * Find the referred data extension node under the > * referred device node. > */ > - for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > - (*element)++) { > - const char *child_name = (*element)->string.pointer; > - > - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name); > - if (!ref_fwnode) > - return -EINVAL; > + if (subnode_string) { > + for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > + (*element)++) { > + const char *child_name = (*element)->string.pointer; > + > + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, > + child_name); > + if (!ref_fwnode) > + return -EINVAL; > + } > } > > /* > @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > for (i = 0; (*element) + i < end && i < num_args; i++) { > acpi_object_type type = (*element)[i].type; > > - if (type == ACPI_TYPE_LOCAL_REFERENCE) > + if (type == ACPI_TYPE_LOCAL_REFERENCE || > + (!subnode_string && type == ACPI_TYPE_STRING)) > break; > > if (type == ACPI_TYPE_INTEGER) > @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > return 0; > } > > +static struct fwnode_handle * > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring) > +{ > + acpi_handle scope, handle; > + struct acpi_data_node *dn; > + struct acpi_device *device; > + acpi_status status; > + > + if (is_acpi_device_node(fwnode)) { > + scope = to_acpi_device_node(fwnode)->handle; > + } else if (is_acpi_data_node(fwnode)) { > + scope = to_acpi_data_node(fwnode)->handle; > + } else { > + pr_debug("bad node type for node %pfw\n", fwnode); > + return ERR_PTR(-EINVAL); > + } > + > + status = acpi_get_handle(scope, refstring, &handle); > + if (ACPI_FAILURE(status)) { > + acpi_handle_debug(scope, "can't get handle for %s", refstring); > + return ERR_PTR(-EINVAL); > + } > + > + device = acpi_fetch_acpi_dev(handle); > + if (device) > + return acpi_fwnode_handle(device); > + > + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag, > + (void **)&dn, NULL); > + if (ACPI_FAILURE(status) || !dn) { > + acpi_handle_debug(handle, "can't find subnode"); > + return ERR_PTR(-EINVAL); > + } > + > + return &dn->fwnode; So on failure this function always returns the same error code. Can it return NULL instead which can be translated into an error code by the caller? > +} > + > /** > * __acpi_node_get_property_reference - returns handle to the referenced object > * @fwnode: Firmware node to get the property from > @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > const union acpi_object *element, *end; > const union acpi_object *obj; > const struct acpi_device_data *data; > + struct fwnode_handle *ref_fwnode; > struct acpi_device *device; > int ret, idx = 0; > > @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > args->fwnode = acpi_fwnode_handle(device); > args->nargs = 0; > + return 0; > + case ACPI_TYPE_STRING: > + if (index) > + return -ENOENT; > + > + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer); > + if (IS_ERR(ref_fwnode)) > + return PTR_ERR(ref_fwnode); > + > + args->fwnode = ref_fwnode; > + args->nargs = 0; > + > return 0; > case ACPI_TYPE_PACKAGE: > /* > * If it is not a single reference, then it is a package of > - * references followed by number of ints as follows: > + * references, followed by number of ints as follows: > * > * Package () { REF, INT, REF, INT, INT } > * > - * The index argument is then used to determine which reference > - * the caller wants (along with the arguments). > + * Here, REF may be either a local reference or a string. The > + * index argument is then used to determine which reference the > + * caller wants (along with the arguments). > */ > break; > default: > @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > ret = acpi_get_ref_args(idx == index ? args : NULL, > acpi_fwnode_handle(device), > - &element, end, num_args); > + &element, end, num_args, true); > + if (ret < 0) > + return ret; > + > + if (idx == index) > + return 0; > + > + break; > + case ACPI_TYPE_STRING: > + ref_fwnode = > + acpi_parse_string_ref(fwnode, > + element->string.pointer); > + if (IS_ERR(ref_fwnode)) > + return PTR_ERR(ref_fwnode); > + > + element++; > + > + ret = acpi_get_ref_args(idx == index ? args : NULL, > + ref_fwnode, &element, end, > + num_args, false); > if (ret < 0) > return ret; > > --
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. > > > { > > u32 nargs = 0, i; > > > > @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > > * Find the referred data extension node under the > > * referred device node. > > */ > > - for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > > - (*element)++) { > > - const char *child_name = (*element)->string.pointer; > > - > > - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name); > > - if (!ref_fwnode) > > - return -EINVAL; > > + if (subnode_string) { > > + for (; *element < end && (*element)->type == ACPI_TYPE_STRING; > > + (*element)++) { > > + const char *child_name = (*element)->string.pointer; > > + > > + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, > > + child_name); > > + if (!ref_fwnode) > > + return -EINVAL; > > + } > > } > > > > /* > > @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > > for (i = 0; (*element) + i < end && i < num_args; i++) { > > acpi_object_type type = (*element)[i].type; > > > > - if (type == ACPI_TYPE_LOCAL_REFERENCE) > > + if (type == ACPI_TYPE_LOCAL_REFERENCE || > > + (!subnode_string && type == ACPI_TYPE_STRING)) > > break; > > > > if (type == ACPI_TYPE_INTEGER) > > @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, > > return 0; > > } > > > > +static struct fwnode_handle * > > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring) > > +{ > > + acpi_handle scope, handle; > > + struct acpi_data_node *dn; > > + struct acpi_device *device; > > + acpi_status status; > > + > > + if (is_acpi_device_node(fwnode)) { > > + scope = to_acpi_device_node(fwnode)->handle; > > + } else if (is_acpi_data_node(fwnode)) { > > + scope = to_acpi_data_node(fwnode)->handle; > > + } else { > > + pr_debug("bad node type for node %pfw\n", fwnode); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + status = acpi_get_handle(scope, refstring, &handle); > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_debug(scope, "can't get handle for %s", refstring); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + device = acpi_fetch_acpi_dev(handle); > > + if (device) > > + return acpi_fwnode_handle(device); > > + > > + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag, > > + (void **)&dn, NULL); > > + if (ACPI_FAILURE(status) || !dn) { > > + acpi_handle_debug(handle, "can't find subnode"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + return &dn->fwnode; > > So on failure this function always returns the same error code. Can > it return NULL instead which can be translated into an error code by > the caller? Sure, makes sense. > > > +} > > + > > /** > > * __acpi_node_get_property_reference - returns handle to the referenced object > > * @fwnode: Firmware node to get the property from > > @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > const union acpi_object *element, *end; > > const union acpi_object *obj; > > const struct acpi_device_data *data; > > + struct fwnode_handle *ref_fwnode; > > struct acpi_device *device; > > int ret, idx = 0; > > > > @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > > > args->fwnode = acpi_fwnode_handle(device); > > args->nargs = 0; > > + return 0; > > + case ACPI_TYPE_STRING: > > + if (index) > > + return -ENOENT; > > + > > + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer); > > + if (IS_ERR(ref_fwnode)) > > + return PTR_ERR(ref_fwnode); > > + > > + args->fwnode = ref_fwnode; > > + args->nargs = 0; > > + > > return 0; > > case ACPI_TYPE_PACKAGE: > > /* > > * If it is not a single reference, then it is a package of > > - * references followed by number of ints as follows: > > + * references, followed by number of ints as follows: > > * > > * Package () { REF, INT, REF, INT, INT } > > * > > - * The index argument is then used to determine which reference > > - * the caller wants (along with the arguments). > > + * Here, REF may be either a local reference or a string. The > > + * index argument is then used to determine which reference the > > + * caller wants (along with the arguments). > > */ > > break; > > default: > > @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > > > ret = acpi_get_ref_args(idx == index ? args : NULL, > > acpi_fwnode_handle(device), > > - &element, end, num_args); > > + &element, end, num_args, true); > > + if (ret < 0) > > + return ret; > > + > > + if (idx == index) > > + return 0; > > + > > + break; > > + case ACPI_TYPE_STRING: > > + ref_fwnode = > > + acpi_parse_string_ref(fwnode, > > + element->string.pointer); > > + if (IS_ERR(ref_fwnode)) > > + return PTR_ERR(ref_fwnode); > > + > > + element++; > > + > > + ret = acpi_get_ref_args(idx == index ? args : NULL, > > + ref_fwnode, &element, end, > > + num_args, false); > > if (ret < 0) > > return ret; > > > > --
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.
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>.
Hi Sakari, On Mon, May 22, 2023 at 6:28 PM Sakari Ailus <sakari.ailus@linux.intel.com> 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@linux.intel.com> 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@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. 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.
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@linux.intel.com> 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@linux.intel.com> 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@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. > > 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.
Hi Sakari, On Mon, May 22, 2023 at 10:35 PM Sakari Ailus <sakari.ailus@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@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. > > > > 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).
Hi Rafael, On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Mon, May 22, 2023 at 10:35 PM Sakari Ailus > <sakari.ailus@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@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. > > > > > > 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? Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR it was intended to use this in DisCo for Imaging but after review (in a rather liberal sense of the term) it was decided to use string-only references, as in this patch. I'm not aware of anyone needing this. They've been there for about five years but I'd guess someone would complain if it stops working for them. > > 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). Works for me.
Hi Sakari, On Tue, May 23, 2023 at 1:43 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote: > > On Mon, May 22, 2023 at 10:35 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > 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@linux.intel.com> wrote: [cut] > > > > > > > > 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? > > Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR > it was intended to use this in DisCo for Imaging but after review (in a > rather liberal sense of the term) it was decided to use string-only > references, as in this patch. It looks like this is sort of a placeholder then. > I'm not aware of anyone needing this. They've been there for about five > years but I'd guess someone would complain if it stops working for them. It is also quite straightforward to restore if need be. > > > > 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). > > Works for me. OK, I'll make this change then.
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> + * 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) { u32 nargs = 0, i; @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, * Find the referred data extension node under the * referred device node. */ - for (; *element < end && (*element)->type == ACPI_TYPE_STRING; - (*element)++) { - const char *child_name = (*element)->string.pointer; - - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name); - if (!ref_fwnode) - return -EINVAL; + if (subnode_string) { + for (; *element < end && (*element)->type == ACPI_TYPE_STRING; + (*element)++) { + const char *child_name = (*element)->string.pointer; + + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, + child_name); + if (!ref_fwnode) + return -EINVAL; + } } /* @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, for (i = 0; (*element) + i < end && i < num_args; i++) { acpi_object_type type = (*element)[i].type; - if (type == ACPI_TYPE_LOCAL_REFERENCE) + if (type == ACPI_TYPE_LOCAL_REFERENCE || + (!subnode_string && type == ACPI_TYPE_STRING)) break; if (type == ACPI_TYPE_INTEGER) @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args, return 0; } +static struct fwnode_handle * +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring) +{ + acpi_handle scope, handle; + struct acpi_data_node *dn; + struct acpi_device *device; + acpi_status status; + + if (is_acpi_device_node(fwnode)) { + scope = to_acpi_device_node(fwnode)->handle; + } else if (is_acpi_data_node(fwnode)) { + scope = to_acpi_data_node(fwnode)->handle; + } else { + pr_debug("bad node type for node %pfw\n", fwnode); + return ERR_PTR(-EINVAL); + } + + status = acpi_get_handle(scope, refstring, &handle); + if (ACPI_FAILURE(status)) { + acpi_handle_debug(scope, "can't get handle for %s", refstring); + return ERR_PTR(-EINVAL); + } + + device = acpi_fetch_acpi_dev(handle); + if (device) + return acpi_fwnode_handle(device); + + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag, + (void **)&dn, NULL); + if (ACPI_FAILURE(status) || !dn) { + acpi_handle_debug(handle, "can't find subnode"); + return ERR_PTR(-EINVAL); + } + + return &dn->fwnode; +} + /** * __acpi_node_get_property_reference - returns handle to the referenced object * @fwnode: Firmware node to get the property from @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, const union acpi_object *element, *end; const union acpi_object *obj; const struct acpi_device_data *data; + struct fwnode_handle *ref_fwnode; struct acpi_device *device; int ret, idx = 0; @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, args->fwnode = acpi_fwnode_handle(device); args->nargs = 0; + return 0; + case ACPI_TYPE_STRING: + if (index) + return -ENOENT; + + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer); + if (IS_ERR(ref_fwnode)) + return PTR_ERR(ref_fwnode); + + args->fwnode = ref_fwnode; + args->nargs = 0; + return 0; case ACPI_TYPE_PACKAGE: /* * If it is not a single reference, then it is a package of - * references followed by number of ints as follows: + * references, followed by number of ints as follows: * * Package () { REF, INT, REF, INT, INT } * - * The index argument is then used to determine which reference - * the caller wants (along with the arguments). + * Here, REF may be either a local reference or a string. The + * index argument is then used to determine which reference the + * caller wants (along with the arguments). */ break; default: @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, ret = acpi_get_ref_args(idx == index ? args : NULL, acpi_fwnode_handle(device), - &element, end, num_args); + &element, end, num_args, true); + if (ret < 0) + return ret; + + if (idx == index) + return 0; + + break; + case ACPI_TYPE_STRING: + ref_fwnode = + acpi_parse_string_ref(fwnode, + element->string.pointer); + if (IS_ERR(ref_fwnode)) + return PTR_ERR(ref_fwnode); + + element++; + + ret = acpi_get_ref_args(idx == index ? args : NULL, + ref_fwnode, &element, end, + num_args, false); if (ret < 0) return ret;
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. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------ 1 file changed, 94 insertions(+), 16 deletions(-)