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

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

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 ?

...

> +#define NEXT_PROPERTY(index, max)			   \
> +	(WARN_ON(++(index) > ACPI_DEVICE_SWNODE_##max + 1) ?	\

'>' -- > '>=' and drop ' + 1'?

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

...

> +			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? :-)

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

> +			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?
Also seems like {} are missing since the body is multi-line.

> +			port->ep_props[NEXT_PROPERTY(ep_prop_index,
> +						     EP_LINK_FREQUENCIES)] =

Index on one line?

> +				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()

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

-- 
With Best Regards,
Andy Shevchenko