Hi Andy,
On Thu, Jan 19, 2023 at 05:44:55PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 19, 2023 at 03:03:48PM +0000, Sakari Ailus wrote:
> > On Tue, Jan 17, 2023 at 04:59:18PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 17, 2023 at 02:22:40PM +0200, Sakari Ailus wrote:
>
> ...
>
> > > > +#define GRAPH_PORT_NAME(var, num) \
> > > > + (snprintf((var), sizeof(var), "port@%u", (num)) > sizeof(var))
> > >
> > > SWNODE_GRAPH_PORT_NAME_FMT ?
> >
> > The name is not used anywhere else. I would keep it as-is.
>
> It repeats the same string literal which is the part of the firmware node graph
> representation, right? I think you can rename the above mentioned format macro
> and use it in your code. We will reduce the possible deviation and amount of
> points of error.
Ah, I thought you had suggested using a new one. Yes, I'll use the existing
macro.
>
> ...
>
> > > > + static const char mipi_port_prefix[] = "mipi-img-port-";
> > >
> > > It's harder to read in the code, please put it in place.
> >
> > There are multiple uses of it. It's better there's a single definition.
>
> Yes and without this definition one read exact value of the property without
> too much brain power, now I need to go first to remember the prefix, then
> concatenate it without typo in my brain and think about the result.
Still having them exactly the same is of utmost importance and a common
definition reliably achieves that. What the string actually is is of
secondary importance.
>
> ...
>
> > > > + port->ep_props[NEXT_PROPERTY(*ep_prop_index,
> > > > + EP_DATA_LANES)] =
> > >
> > > It's hard to read, taking into account that you split on index of the array.
> > >
> > > How much a new monitor costs for you? Maybe I can donate to make you use more
> > > than 80 from time to time? :-)
> >
> > You know newspaper pages are split into multiple columns for a reason,
> > similarly web pages with text columns very seldom span the entire page
> > width. The number of characters per line tends to be less than --- you
> > might be surprised --- 80. The reason is readability.
>
> Surprisingly to you, the newspaper and the limit is for quick reading the
> text. The code differs to the SciFi book, for example. And doesn't have
> same requirements. Code has different tokenisation which you break when
> splitting in the middle of the token. That's why one line is better than
> silly 80 characters limit. It _increases_ readability of the *code*.
I disagree. Do you know if studies have been made on the topic?
I can make some a little longer if that makes you happy (depending on other
comments, too), but I won't make the lines e.g. 200 characters long.
>
> > > > + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> > > > + port->data_lanes,
> > > > + num_lanes);
>
> Ditto for all other similar cases.
>
> ...
>
> > > > + if (!ret)
> > >
> > > Why not positive conditional?
> >
> > The success case is handled first.
>
> And in kernel we usually check for error first. Esp. taking into account that
> here you have both cases under 'if'.
The other assignments take place just before this, so it's closer to them. I
can change this though.
>
> ...
>
> > > > + if (acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer) != AE_OK) {
> > >
> > > We have ACPI_SUCCESS() / ACPI_FAILURE()
> >
> > Yes.
>
> Why not using them?
Yes, in v2.
>
> > > > + acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
> > > > + return;
> > > > + }
--
Kind regards,
Sakari Ailus
