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

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

On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

As promised the idea of bitmap APIs.

Also I have stumbled over couple of suspicious places. See below.

...

> +static void init_port_csi2_common(struct acpi_device *device,
> +				  struct fwnode_handle *mipi_port_fwnode,
> +				  unsigned int *ep_prop_index,
> +				  unsigned int port_nr)
> +{
> +	unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
> +	struct acpi_device_software_nodes *ads = device->swnodes;
> +	struct acpi_device_software_node_port *port = &ads->ports[port_index];
> +	unsigned int num_lanes = 0;
> +	union {
> +		u32 val;

// Not sure why this even exists.
// And hence why do we need union?

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

Somewhere

#define MAX_LANES(port)		(ARRAY_SIZE((port)->data_lanes) + 1)

	u8 val8[BITS_TO_BYTES(MAX_LANES(port))];

...

	/* Data lanes + the clock lane */
	DECLARE_BITMAP(polarity, MAX_LANES(port)));

> +	int ret;
> +
> +	*ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
> +
> +	if (GRAPH_PORT_NAME(port->port_name, port_nr))
> +		return;
> +
> +	ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
> +		SOFTWARE_NODE(port->port_name, port->port_props,
> +			      &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
> +
> +	ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", u.val8);
> +	if (!ret) {
> +		port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
> +			PROPERTY_ENTRY_U32("clock-lanes", *u.val8);
> +	}

> +	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
> +	if (ret > 0) {
> +		num_lanes = ret;
> +
> +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {

		>= MAX_LANES(port)

> +			acpi_handle_warn(acpi_device_handle(device),
> +					 "too many data lanes (%u)\n",
> +					 num_lanes);
> +			num_lanes = ARRAY_SIZE(port->data_lanes);

			= MAX_LANES(port) - 1;

> +		}
> +
> +		ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
> +						    u.val8, num_lanes);

> +		if (!ret) {
> +			unsigned int i;
> +
> +			for (i = 0; i < num_lanes; i++)
> +				port->data_lanes[i] = u.val8[i];
> +
> +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
> +				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) {

How is it supposed to work?!

> +		unsigned int bytes = ret;
> +
> +		/* 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] =
> +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> +					1U : 0U;
> +
> +			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);
> +		}
> +	}

	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
	if (ret < 0) {
		acpi_handle_debug(acpi_device_handle(device),
				  "no lane polarity provided\n");
	} else if (ret < 1 + num_lanes) {
		acpi_handle_warn(acpi_device_handle(device),
				 "too few lane polarity bytes (%u)\n", bytes);
	} else {
		// assuming we dropped the union and renamed to val...
		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
						    "mipi-img-lane-polarities",
						    val, sizeof(val));
		if (ret) {
			...can't read... (debug message?)
		} else {
			unsigned int i;

			for (i = 0; i < 1 + num_lanes; i++)
				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);

			// assuming that lane_polarities is zeroed by default...
			for_each_set_bit(i, polarity, 1 + num_lanes)
				port->lane_polarities[i] = 1;
		}
	}

> +	ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
> +		SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
> +			      &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
> +}

-- 
With Best Regards,
Andy Shevchenko