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@xxxxxxxxxxxxxxx>
> > * Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > * Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > + * Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> 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
