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

Andy Shevchenko
2023-01-23T17:24:41Z

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