Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> Currently on platforms designed for Windows, connections between CIO2 and
> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> driver to compensate by building software_node connections, parsing the
> connection properties from the sensor's SSDB buffer.

...

> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);

Does 4 has any meaning that can be described by #define ?

...

> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);

Hmm... I think you should use actual size of remote_port instead of 7.

> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));

Yeah, I would rather like to see one point of the definition of the format.
If it's the same as per OF case, perhaps some generic header (like fwnode.h?) is good for this?
In this case the 5 in one of the previous patches Also can be derived from the format.

> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));

Similar here.

> +}

...

> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {

> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");

> +				/* overflow i so outer loop ceases */
> +				i = ARRAY_SIZE(cio2_supported_sensors);
> +				break;

Why not to create a new label below and assign ret here with probably comment
why it's not an error?

> +			}

...

> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +							   &sensor->ssdb,
> +							   sizeof(sensor->ssdb));
> +			if (ret < 0)

if (ret) (because positive case can be returned just by next conditional).

> +				goto err_put_adev;
> +
> +			if (sensor->ssdb.lanes > 4) {
> +				dev_err(&adev->dev,
> +					"Number of lanes in SSDB is invalid\n");
> +				goto err_put_adev;
> +			}

...

> +			dev_info(&cio2->dev, "Found supported sensor %s\n",
> +				 acpi_dev_name(adev));
> +
> +			bridge->n_sensors++;
> +		}
> +	}

	return 0;

> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);

err_out:

> +	return ret;
> +}

...

> +enum cio2_sensor_swnodes {
> +	SWNODE_SENSOR_HID,
> +	SWNODE_SENSOR_PORT,
> +	SWNODE_SENSOR_ENDPOINT,
> +	SWNODE_CIO2_PORT,
> +	SWNODE_CIO2_ENDPOINT,

> +	NR_OF_SENSOR_SWNODES

Perhaps same namespace, i.e.

	SWNODE_SENSOR_NR

> +};

-- 
With Best Regards,
Andy Shevchenko