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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux