Re: [PATCH v2 1/8] ACPI: property: Parse data node string references in properties

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

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.

-- 
Kind regards,

Sakari Ailus