Re: [PATCH v7 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging

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

On Tue, Mar 28, 2023 at 01:12:58PM +0300, Sakari Ailus wrote:
> Prepare generating 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 NO_CSI2_PORT (UINT_MAX - 1)

Has it been used before this patch?

...

> +/* Print graph port name into a buffer, return non-zero if failed. */
> +#define GRAPH_PORT_NAME(var, num)					    \
> +	(snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) >= \
> +	 sizeof(var))

This macro evaluates sizeof(var) twice. Is it a problem?

...

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

This macro full of interesting effects. On top of that it's written in
hard-to-read form.

Why not at least

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

?

...

> +	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-lane-polarities");
> +	if (ret > 0) {
> +		unsigned int bytes = min_t(unsigned int, ret, sizeof(val));
> +
> +		fwnode_property_read_u8_array(mipi_port_fwnode,
> +					      "mipi-img-lane-polarities",
> +					      val, bytes);
> +
> +		/* Total number of lanes here is clock lane + data lanes */
> +		if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
> +			unsigned int i;
> +
> +			/* Move polarity bits to the lane polarity u32 array */
> +			for (i = 0; i < 1 + num_lanes; i++)
> +				port->lane_polarities[i] =
> +					(bool)(val[i >> 3] & (1 << (i & 7)));

Casting to bool?!

Can we read the array and convert it to bitmap, then this voodoo-ish code can
be simplified to

	for_each_set_bit(i, ...)
		..._polarities[i] = 1;

(assuming initially they are 0:s)?

> +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_LANE_POLARITIES)] =
> +				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);
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko