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

[Daniel Scally <djrscally@gmail.com>]

Hi Andy, thanks for the comments

On 18/12/2020 21:17, Andy Shevchenko wrote:
> 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 ?

It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:

https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36

That enum's not in an accessible header, but I can define it in this
module's header

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

Yes ok


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

Okedokey. It is indeed intended to match OF and ACPI case, both of which
mandate that format (though only ACPI's functions seem to enforce it).
fwnode.h seems as good a place as any to me, though I'm not sure there's
anywhere in the driver code for OF or ACPI that would actually use it at
the moment.

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

Sure, I can do that, but since it wouldn't need any cleanup I could also
just return 0 here as Laurent suggest (but with a comment explaining why
that's ok as you say) - do you have a preference?

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

cio2_bridge_read_acpi_buffer() returns the buffer length on success at
the moment, but I can change it to return 0 and have this be if (ret)

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

Okedokey

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

Depends on question above I think

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

Yep, will do.

Thanks
Dan