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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

-- 
With Best Regards,
Andy Shevchenko