Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Andy

On 04/01/2021 12:09, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:35PM +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.
> Few nitpicks below (I consider it's good enough as is, though).
Thanks!
>> +static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>> +				      struct cio2_bridge *bridge,
>> +				      struct pci_dev *cio2)
>> +{
>> +	struct fwnode_handle *fwnode;
>> +	struct cio2_sensor *sensor;
>> +	struct acpi_device *adev;
>> +	int ret;
>> +	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> (1)
>
>> +		if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>> +			dev_err(&cio2->dev, "Exceeded available CIO2 ports\n");
>> +			cio2_bridge_unregister_sensors(bridge);
>> +			ret = -EINVAL;
>> +			goto err_out;
>> +		}
>> +		if (!adev->status.enabled)
>> +			continue;
> A nit: this would be better to be at (1) location.


Yep, agreed

>
> Then possible to factor out the rest of the body of this loop as well.
>
> (Also can be considered as a hint for the future improvement)
Yeah I can look at this, there will probably be some future changes
anyway as we discover more details about the data in the SSDB buffer and
so on
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> new file mode 100644
> index 000000000000..3ec4ed44aced
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@xxxxxxxxx> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "ipu3-cio2.h"
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_MAX_LANES				4
> +#define MAX_NUM_LINK_FREQS			3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
> +	{					\
> +		.hid = _HID,			\
> +		.nr_link_freqs = _NR,		\
> +		.link_freqs = { __VA_ARGS__ }	\
> +	}
> Perhaps also good to declare it as a compound literal.
>
> (Means to add (struct ...) to the initializer.
>
Yep ok
>> +#define NODE_SENSOR(_HID, _PROPS)		\
>> +	((const struct software_node) {		\
>> +		.name = _HID,			\
>> +		.properties = _PROPS,		\
>> +	})
>> +
>> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
>> +	((const struct software_node) {		\
>> +		.name = _PORT,			\
>> +		.parent = _SENSOR_NODE,		\
>> +	})
>> +
>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
>> +	((const struct software_node) {		\
>> +		.name = _EP,			\
>> +		.parent = _PORT,		\
>> +		.properties = _PROPS,		\
>> +	})
> In all three I didn't get why you need outer parentheses. Without them it will
> be well defined compound literal and should work as is.
The code works fine, but checkpatch complains that macros with complex
values should be enclosed in parentheses. I guess now that I'm more
familiar with the code I'd call that a false-positive though, as nowhere
else in the kernel that I've seen encloses them the same way.
>>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>>  			  const struct pci_device_id *id)
>>  {
>> +	struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
>>  	struct cio2_device *cio2;
>>  	int r;
>>  
>> @@ -1715,6 +1732,22 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>>  		return -ENOMEM;
>>  	cio2->pci_dev = pci_dev;
>>  
>> +	/*
>> +	 * On some platforms no connections to sensors are defined in firmware,
>> +	 * if the device has no endpoints then we can try to build those as
>> +	 * software_nodes parsed from SSDB.
>> +	 */
>> +	if (!cio2_check_fwnode_graph(fwnode)) {
> A nit:
> I prefer form of
>
> 	r = cio2_check_fwnode_graph(fwnode);
> 	if (!r) { // alternatively if (r == 0), depends on maintainer's taste
This is fine by me; I can switch to that
>
>> +		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
>> +			dev_err(&pci_dev->dev, "fwnode graph has no endpoints connected\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		r = cio2_bridge_init(pci_dev);
>> +		if (r)
>> +			return r;
>> +	}
>> +
>>  	r = pcim_enable_device(pci_dev);
>>  	if (r) {
>>  		dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> index 62187ab5ae43..dc3e343a37fb 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> @@ -455,4 +455,10 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
>>  	return container_of(vq, struct cio2_queue, vbq);
>>  }
>>  
>> +#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
>> +int cio2_bridge_init(struct pci_dev *cio2);
>> +#else
>> +int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
> static inline?

Ah, yes - thanks. Hadn't read about inline until now