| Message ID | 20230123134617.265382-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand |
On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus 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. ... > - * Copyright (C) 2014, Intel Corporation > + * Copyright (C) 2014--2023, Intel Corporation Isn't one dash enough? $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l 37 $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l 15064 > * 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> > + * Sakari Ailus <sakari.ailus@linux.intel.com> Seems wrong indentation in comparison to the others. > */ ... > +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; Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()... > + } else if (is_acpi_data_node(fwnode)) { > + scope = to_acpi_data_node(fwnode)->handle; ...but not for this. > + } else { > + pr_err("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; > +}
Hi Andy, On Mon, Jan 23, 2023 at 04:51:33PM +0200, Andy Shevchenko wrote: > On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus 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. > > ... > > > - * Copyright (C) 2014, Intel Corporation > > + * Copyright (C) 2014--2023, Intel Corporation > > Isn't one dash enough? > > $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l > 37 > > $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l > 15064 This is a range, not hyphenation. There's no different character in the ASCII character set for the former, commonly two regular dashes are used. There probably would be a correct Unicode character though. > > > > * 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> > > + * Sakari Ailus <sakari.ailus@linux.intel.com> > > Seems wrong indentation in comparison to the others. Tabs are preferred for intendation. I can change all the lines to use tab. How about that? > > > */ > > ... > > > +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; > > Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()... > > > + } else if (is_acpi_data_node(fwnode)) { > > > + scope = to_acpi_data_node(fwnode)->handle; > > ...but not for this. I'd either prefer to keep them as-is, as it's easy to see what's being done there, or add a new macro --- or a function to do this. Say, acpi_fwnode_acpi_handle(), as this is clearly ACPI specific and to differentiate between ACPI handles and fwnode handles. ACPI_HANDLE_FWNODE()'s name suggests it would do something else than it does, if you consider the current fwnode API.
On Mon, Jan 23, 2023 at 05:53:59PM +0200, Sakari Ailus wrote: > On Mon, Jan 23, 2023 at 04:51:33PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus wrote: ... > > > - * Copyright (C) 2014, Intel Corporation > > > + * Copyright (C) 2014--2023, Intel Corporation > > > > Isn't one dash enough? > > > > $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l > > 37 > > > > $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l > > 15064 > > This is a range, not hyphenation. There's no different character in the > ASCII character set for the former, commonly two regular dashes are used. > There probably would be a correct Unicode character though. Fine, but it's not even close to be called "a common use" as I showed by running `git grep`. > > > * 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> > > > + * Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Seems wrong indentation in comparison to the others. > > Tabs are preferred for intendation. I can change all the lines to use tab. Dunno, not a maintainer. I just pointed to inconsistency in the comment lines. > > > */ ... > > > +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; > > > > Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()... > > > > > + } else if (is_acpi_data_node(fwnode)) { > > > > > + scope = to_acpi_data_node(fwnode)->handle; > > > > ...but not for this. > > I'd either prefer to keep them as-is, as it's easy to see what's being done > there, or add a new macro --- or a function to do this. Say, > acpi_fwnode_acpi_handle(), as this is clearly ACPI specific and to > differentiate between ACPI handles and fwnode handles. Since it's an ACPI glue layer code, I'm not insisting on changes. Just pointed out that we have a helper function for one of the cases. > ACPI_HANDLE_FWNODE()'s name suggests it would do something else than it > does, if you consider the current fwnode API.
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index b8d9eb9a433ed..ae1f8259e76a5 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> + * 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_err("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 | 106 ++++++++++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 14 deletions(-)