Re: [PATCH 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

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

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.

...

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

...

> > > +			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*.

> > > +				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'.

...

> > > +	if (acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer) != AE_OK) {
> > 
> > We have ACPI_SUCCESS() / ACPI_FAILURE()
> 
> Yes.

Why not using them?

> > > +		acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
> > > +		return;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko