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

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

On Thu, Jan 26, 2023 at 12:40:57AM +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.

...

> +static unsigned int next_csi2_port_index(struct acpi_device_software_nodes *ads,
> +					 unsigned int port_nr)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ads->num_ports; i++) {
> +		struct acpi_device_software_node_port *port = &ads->ports[i];
> +
> +		if (port->port_nr == port_nr)
> +			return i;

> +		if (port->port_nr != NO_CSI2_PORT)
> +			continue;
> +
> +		port->port_nr = port_nr;
> +
> +		return i;

Maybe it would be better to use the same pattern as above?

		if (port->port_nr == NO_CSI2_PORT) {
			port->port_nr = port_nr;
			return i;
		}

> +	}
> +
> +	return NO_CSI2_PORT;
> +}

...

> +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> +						  unsigned int port)
> +{
> +	static const char mipi_port_prefix[] = "mipi-img-port-";
> +	char mipi_port_name[sizeof(mipi_port_prefix) + 2];

I think the following will be better:

	char mipi_port_name[16];


> +	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> +		     mipi_port_prefix, port) >= sizeof(mipi_port_name)) {

	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "mipi-img-port-%u",
		     port) >= sizeof(mipi_port_name)) {

> +		acpi_handle_info(acpi_device_handle(device),
> +				 "mipi port name too long for port %u\n", port);
> +		return NULL;
> +	}
> +
> +	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
> +					   mipi_port_name);
> +}

...

> +	union {
> +		u32 val;

OK. I stopped here, because I'm not sure you have really sent an updated
version. For sure the val and union is not used.

> +		/* Data lanes + the clock lane */
> +		u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
> +	} u;

...

Please, address all in v3.

-- 
With Best Regards,
Andy Shevchenko