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@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.
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
