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 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:
> > Generate software nodes for information parsed from ACPI _CRS for CSI-2 as
> > well as MIPI DisCo for Imaging spec. The software nodes are compliant with
> > existing ACPI or DT definitions and are parsed by relevant drivers without
> > changes.
> 
> ...
> 
> > +#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.

> 
> ...
> 
> > +#define NEXT_PROPERTY(index, max)			   \
> > +	(WARN_ON(++(index) > ACPI_DEVICE_SWNODE_##max + 1) ?	\
> 
> '>' -- > '>=' and drop ' + 1'?

Yeah.

> 
> > +	 ACPI_DEVICE_SWNODE_##max : (index) - 1)
> 
> ...
> 
> > +	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.

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

> 
> > +				PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> > +							     port->data_lanes,
> > +							     num_lanes);
> 
> ...
> 
> 
> > +	ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > +					    "mipi-img-lane-polarities",
> > +					    u.val8, sizeof(u.val8));
> > +	if (ret > 0) {
> > +		unsigned int bytes = ret;
> > +
> > +		/* Total number of lanes here is clock lane + data lanes */
> > +		if (bytes << 3 >= 1 + num_lanes) {
> 
> bytes * BITS_PER_BYTE? Or if you want to be super precise BITS_PER_TYPE(u8).

I think of these two, BITS_PER_TYPE(u8) looks better.

> 
> > +			unsigned int i;
> > +
> > +			/* Move polarity bits to the lane polarity u32 array */
> > +			for (i = 0; i < 1 + num_lanes; i++)
> > +				port->lane_polarities[i] =
> > +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> > +					1U : 0U;
> 
> > +			port->ep_props[NEXT_PROPERTY(*ep_prop_index,
> > +						     EP_LANE_POLARITIES)] =
> 
> Index on one line?
> 
> > +				PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
> > +							     port->lane_polarities,
> > +							     1 + num_lanes);
> > +		} else {
> > +			acpi_handle_warn(acpi_device_handle(device),
> > +					 "too few lane polarity bytes (%u)\n",
> > +					 bytes);
> > +		}
> > +	}
> 
> ...
> 
> > +	unsigned int port_index = next_csi2_port_index(device->swnodes,
> > +						       port_nr);
> 
> One line easier to read.
> 
> ...
> 
> > +		if (!ret)
> 
> Why not positive conditional?

The success case is handled first.

> Also seems like {} are missing since the body is multi-line.

Multiple lines as such isn't a reason to add braces (per coding style).

> 
> > +			port->ep_props[NEXT_PROPERTY(ep_prop_index,
> > +						     EP_LINK_FREQUENCIES)] =
> 
> Index on one line?

This is more or less random.

> 
> > +				PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
> > +							     port->link_frequencies,
> > +							     num_link_freqs);
> > +		else
> > +			acpi_handle_info(acpi_device_handle(device),
> > +					 "can't get link frequencies (%d)\n",
> > +					 ret);
> 
> ...
> 
> > +	if (acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer) != AE_OK) {
> 
> We have ACPI_SUCCESS() / ACPI_FAILURE()

Yes.

> 
> > +		acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
> > +		return;
> > +	}
> 
> ...
> 
> > +	ads->nodes[ACPI_DEVICE_SWNODE_ROOT] = (struct software_node) {
> > +		.name = buffer.pointer,
> > +		.properties = ads->dev_props,
> > +	};
> 
> Aren't you provided a macro for this?

I think I added a macro for this but forgot to use it. I'll address this
(and other issues) in v2.

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