[00/18] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

Hello all

Previous version:

https://lore.kernel.org/linux-media/20201019225903.14276-1-djrscally@gmail.com/

This series aims to add support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. There are two main parts; the
first is extending the ipu3-cio2 driver to allow for patching the
firmware via software_nodes if endpoints aren't defined by ACPI. Patch #13
deals directly with that, all preceding patches are either supplemental
changes or incidental fixes along the way.

The second main part is a way to handle the unusual definition of resource
destined for the sensors in these ACPI tables; regulators and GPIO lines
that are supposed to be consumed by the sensor are lumped in the _CRS of
a dummy ACPI device upon which the sensor is dependent. Patch 18 defines a
new driver to handle those dummy devices and map the resources to the
sensor instead. 14-17 are supporting changes for that driver.

Changelogs mostly in the individual patches, but a broad summary:

	- Altered fwnode_device_is_available() to return true if the
	fwnode_handle doesn't implement that operation.
	- Altered fwnode_graph_get_endpoint_by_id() to parse secondary
	if no endpoint found on primary.
	- Enforce parent->child ordering of software_nodes on registration
	- Added a function to get the next ACPI device with matching _HID,
	plus an iterator macro
	- Altered cio2-bridge.c to store the bridge struct (and basically
	everything else) in heap, plus removed the requirement to delay
	ipu3-cio2 probe until after the i2c devices were instantiated. 
	Also now ensured we handle multiple sensors with the same _HID.
	- Added a function to get devices _dependent_ on a given ACPI dev,
	according to their _DEP entries.
	- Added a function to explicitly construct the name of an I2C dev
	created from an ACPI dev.
	- Added a driver to handle the dummy ACPI devices discussed above.

Comments very welcome!

Dan Scally (1):
  i2c: i2c-core-base: Use the new i2c_acpi_dev_name() in
    i2c_set_dev_name()

Daniel Scally (16):
  property: Return true in fwnode_device_is_available for node types
    that do not implement this operation
  property: Add support for calling fwnode_graph_get_endpoint_by_id()
    for fwnode->secondary
  software_node: Fix failure to put() and get() references to children
    in software_node_get_next_child()
  software_node: Enforce parent before child ordering of nodes array for
    software_node_register_nodes()
  software_node: Alter software_node_unregister_nodes() to unregister
    the array in reverse order
  software_node: amend software_node_unregister_node_group() to perform
    unregistration of array in reverse order to be consistent with
    software_node_unregister_nodes()
  lib/test_printf.c: Use helper function to unwind array of
    software_nodes
  ipu3-cio2: Add T: entry to MAINTAINERS
  ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from
    multiple source files retaining ipu3-cio2 name
  media: v4l2-core: v4l2-async: Check possible match in match_fwnode
    based on sd->fwnode->secondary
  acpi: Add acpi_dev_get_next_match_dev() and macro to iterate through
    acpi_devices matching a given _HID
  ipu3-cio2: Add functionality allowing software_node connections to
    sensors on platforms designed for Windows
  acpi: utils: Add function to fetch dependent acpi_devices
  i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
  gpio: gpiolib-acpi: Export acpi_get_gpiod()
  ipu3: Add driver for dummy INT3472 ACPI device

Heikki Krogerus (1):
  software_node: Add support for fwnode_graph*() family of functions

 MAINTAINERS                                   |   9 +
 drivers/acpi/utils.c                          |  98 ++++-
 drivers/base/property.c                       |   9 +
 drivers/base/swnode.c                         | 157 +++++++-
 drivers/gpio/gpiolib-acpi.c                   |   3 +-
 drivers/i2c/i2c-core-acpi.c                   |  14 +
 drivers/i2c/i2c-core-base.c                   |   2 +-
 drivers/media/pci/intel/ipu3/Kconfig          |  32 ++
 drivers/media/pci/intel/ipu3/Makefile         |   4 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 +++++
 drivers/media/pci/intel/ipu3/int3472.c        | 381 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/int3472.h        |  96 +++++
 .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c}    |  27 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 drivers/media/v4l2-core/v4l2-async.c          |   8 +
 include/acpi/acpi_bus.h                       |   9 +
 include/linux/acpi.h                          |   5 +
 include/linux/i2c.h                           |   5 +
 lib/test_printf.c                             |   4 +-
 20 files changed, 1213 insertions(+), 24 deletions(-)
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
 create mode 100644 drivers/media/pci/intel/ipu3/int3472.c
 create mode 100644 drivers/media/pci/intel/ipu3/int3472.h
 rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)

Hi Daniel,

Thank you for the patch.

On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed to this function.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since RFC v3:
> 
> 	Patch introduced
> 
>  drivers/base/swnode.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 615a0c93e116..af7930b3679e 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
>  	int i;
>  
>  	for (i = 0; nodes[i].name; i++) {
> +		if (nodes[i].parent)
> +			if (!software_node_to_swnode(nodes[i].parent)) {
> +				ret = -EINVAL;
> +				goto err_unregister_nodes;
> +			}
> +
>  		ret = software_node_register(&nodes[i]);
> -		if (ret) {
> -			software_node_unregister_nodes(nodes);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_unregister_nodes;
>  	}
>  
>  	return 0;

I'd add a blank line here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +err_unregister_nodes:
> +	software_node_unregister_nodes(nodes);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(software_node_register_nodes);
>

Hi Daniel,

Thank you for the patch.

On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> this function unregisters software_nodes.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes since RFC v3:
> 
> 	Switched this functionality from a new function to replacing
> 	the existing software_nodes_unregister_nodes()
> 
>  drivers/base/swnode.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index af7930b3679e..d39e1c76d98d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -720,20 +720,25 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
>  
>  /**
>   * software_node_unregister_nodes - Unregister an array of software nodes
> - * @nodes: Zero terminated array of software nodes to be unregistered
> + * @nodes: Zero terminated array of software nodes to be unregistered. If
> + * parent pointers are set up in any of the software nodes then the array
> + * MUST be ordered such that parents come before their children.
>   *
>   * Unregister multiple software nodes at once.
>   *
> - * NOTE: Be careful using this call if the nodes had parent pointers set up in
> - * them before registering.  If so, it is wiser to remove the nodes
> - * individually, in the correct order (child before parent) instead of relying
> - * on the sequential order of the list of nodes in the array.
> + * NOTE: If you are uncertain whether the array is ordered such that
> + * parents will be unregistered before their children, it is wiser to
> + * remove the nodes individually, in the correct order (child before
> + * parent).
>   */
>  void software_node_unregister_nodes(const struct software_node *nodes)
>  {
> -	int i;
> +	unsigned int i = 0;
> +
> +	while (nodes[i].name)
> +		i++;
>  
> -	for (i = 0; nodes[i].name; i++)
> +	while (i--)
>  		software_node_unregister(&nodes[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_nodes);

Hi Daniel,

Thank you for the patch.

On Mon, Nov 30, 2020 at 01:31:22PM +0000, Daniel Scally wrote:
> Where the fwnode graph is comprised of software_nodes, these will be
> assigned as the secondary to dev->fwnode. Check the v4l2_subdev's fwnode
> for a secondary and attempt to match against it during match_fwnode() to
> accommodate that possibility.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes since RFC v3:
> 
> 	- None
> 
>  drivers/media/v4l2-core/v4l2-async.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e3ab003a6c85..6486dbde784f 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -87,6 +87,14 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>  	if (sd->fwnode == asd->match.fwnode)
>  		return true;
>  
> +	/*
> +	 * Check the same situation for any possible secondary assigned to the
> +	 * subdev's fwnode
> +	 */
> +	if ((!IS_ERR_OR_NULL(sd->fwnode->secondary)) &&
> +	    sd->fwnode->secondary == asd->match.fwnode)
> +		return true;
> +
>  	/*
>  	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
>  	 * endpoint or a device. If they're of the same type, there's no match.

Hi Daniel,

Thank you for the patch.

On Mon, Nov 30, 2020 at 01:31:24PM +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.
> 
> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since RFC v3:
> 
> 	- Removed almost all global variables, dynamically allocated
> 	the cio2_bridge structure, plus a bunch of associated changes
> 	like 
> 	- Added a new function to ipu3-cio2-main.c to check for an 
> 	existing fwnode_graph before calling cio2_bridge_init()
> 	- Prefixed cio2_bridge_ to any variables and functions that
> 	lacked it
> 	- Assigned the new fwnode directly to the sensor's ACPI device
> 	fwnode as secondary. This removes the requirement to delay until
> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
> 	it has a side effect, which is that those devices then grab a ref
> 	to the new software_node. This effectively prevents us from
> 	unloading the driver, because we can't free the memory that they
> 	live in whilst the device holds a reference to them. The work
> 	around at the moment is to _not_ unregister the software_nodes
> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
> 	is simply skipped if the module is reloaded.
> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
> 	- Replaced ints with unsigned ints where appropriate
> 	- Iterated over all ACPI devices of a matching _HID rather than
> 	just the first to ensure we handle a device with multiple sensors
> 	of the same model.
> 
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 421 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9702b886d6a4..188559a0a610 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>  M:	Yong Zhi <yong.zhi@intel.com>
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  M:	Bingbu Cao <bingbu.cao@intel.com>
> +M:	Dan Scally <djrscally@gmail.com>
>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> index 82d7f17e6a02..2b3350d042be 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>  	  connected camera.
>  	  The module will be called ipu3-cio2.
> +
> +config CIO2_BRIDGE
> +	bool "IPU3 CIO2 Sensors Bridge"
> +	depends on VIDEO_IPU3_CIO2
> +	help
> +	  This extension provides an API for the ipu3-cio2 driver to create
> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
> +	  can be used to enable support for cameras in detachable / hybrid
> +	  devices that ship with Windows.
> +
> +	  Say Y here if your device is a detachable / hybrid laptop that comes
> +	  with Windows installed by the OEM, for example:
> +
> +	  	- Microsoft Surface models (except Surface Pro 3)
> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
> +		- Dell 7285
> +
> +	  If in doubt, say N here.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> index 429d516452e4..933777e6ea8a 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>  
>  ipu3-cio2-y += ipu3-cio2-main.o
> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..fd3f8ba07274
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */

Could you please add a blank line here ?

> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>

Is this header needed ?

> +#include <linux/kernel.h>
> +#include <linux/module.h>

And this one ?

> +#include <linux/pci.h>
> +#include <linux/property.h>
> +#include <media/v4l2-subdev.h>

And this one ?

> +
> +#include "cio2-bridge.h"
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be working.
> + * Do not add a HID for a sensor that is not actually supported.
> + */
> +static const char * const cio2_supported_devices[] = {

Maybe cio2_supported_sensors ?

> +	"INT33BE",
> +	"OVTI2680",
> +};
> +
> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> +					void *data, u32 size)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int ret;
> +
> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = buffer.pointer;
> +	if (!obj) {
> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> +		return -ENODEV;
> +	}
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
> +		ret = -ENODEV;
> +		goto out_free_buff;
> +	}
> +
> +	if (obj->buffer.length > size) {
> +		dev_err(&adev->dev, "Given buffer is too small\n");
> +		ret = -EINVAL;
> +		goto out_free_buff;
> +	}
> +
> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
> +	ret = obj->buffer.length;
> +
> +out_free_buff:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +
> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> +{
> +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
> +	strcpy(sensor->prop_names.rotation, "rotation");
> +	strcpy(sensor->prop_names.bus_type, "bus-type");
> +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
> +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");

This is a bit fragile, as there's no len check. How about the following
?
static const struct cio2_property_names prop_names = {
	.clock_frequency = "clock-frequency",
	.rotation = "rotation",
	.bus_type = "bus-type",
	.data_lanes = "data-lanes",
	.remote_endpoint = "remote-endpoint",
};

static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
{
	sensor->prop_names = prop_names;
}

This shoudl generate a compilation warning if the string is too long.

You could even inline that line in
cio2_bridge_create_fwnode_properties().

> +}
> +
> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
> +{
> +	unsigned int i;
> +
> +	cio2_bridge_init_property_names(sensor);
> +
> +	for (i = 0; i < 4; i++)
> +		sensor->data_lanes[i] = i + 1;

Is there no provision in the SSDB for data lane remapping ?

> +
> +	/*
> +	 * Can't use PROPERTY_ENTRY_REF because it creates a new variable to
> +	 * point to, which doesn't survive the function.
> +	 */
> +	sensor->local_ref[0] = (struct software_node_ref_args){
> +		.node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT]
> +		};

I'd remove one tab here. Or just write

	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];

> +	sensor->remote_ref[0] = (struct software_node_ref_args){
> +		.node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT]
> +		};
> +
> +	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> +						       sensor->ssdb.mclkspeed);
> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> +						      sensor->ssdb.degree);
> +
> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 5);
> +	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								sensor->data_lanes,
> +								sensor->ssdb.lanes);
> +	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							    sensor->local_ref);
> +
> +	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								  sensor->data_lanes,
> +								  sensor->ssdb.lanes);
> +	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							      sensor->remote_ref);
> +}
> +
> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +	snprintf(sensor->node_names.remote_port, 6, "port%u", sensor->ssdb.link);
> +	strcpy(sensor->node_names.port, "port0");
> +	strcpy(sensor->node_names.endpoint, "endpoint0");
> +}
> +
> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> +						  struct cio2_sensor *sensor)
> +{
> +	struct software_node *nodes = sensor->swnodes;
> +
> +	cio2_bridge_init_swnode_names(sensor);
> +
> +	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> +					       sensor->dev_properties);
> +	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> +					      &nodes[SWNODE_SENSOR_HID]);
> +	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						      &nodes[SWNODE_SENSOR_PORT],
> +						      sensor->ep_properties);
> +	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> +					    &bridge->cio2_hid_node);
> +	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						    &nodes[SWNODE_CIO2_PORT],
> +						    sensor->cio2_properties);
> +}
> +
> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> +{
> +	struct cio2_sensor *sensor;
> +	unsigned int i;
> +
> +	for (i = 0; i < bridge->n_sensors; i++) {
> +		sensor = &bridge->sensors[i];
> +		software_node_unregister_nodes(sensor->swnodes);
> +		acpi_dev_put(sensor->adev);
> +	}
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct cio2_sensor *sensor;
> +	struct acpi_device *adev;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_devices); i++) {
> +		const char *this_device = cio2_supported_devices[i];

s/this_device/name/ (or sensor_name, ...) ?

> +
> +		for_each_acpi_dev_match(adev, this_device, NULL, -1) {
> +			if (!adev || !(adev->status.present && adev->status.enabled))

			if (!adev || !adev->status.present || !adev->status.enabled))

may be a bit more readable. Does for_each_acpi_dev_match() return NULL
devices though ? If no, you could drop the !adev check. You may also be
able to drop the !present check, as I don't think ACPI allows !present
&& enabled.

> +				continue;
> +
> +			sensor = &bridge->sensors[bridge->n_sensors];
> +			sensor->adev = adev;
> +			strscpy(sensor->name, this_device, sizeof(sensor->name));
> +
> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +							   &sensor->ssdb,
> +							   sizeof(sensor->ssdb));
> +			if (ret < 0)
> +				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;
> +			}
> +
> +			cio2_bridge_create_fwnode_properties(sensor);
> +			cio2_bridge_create_connection_swnodes(bridge, sensor);
> +
> +			ret = software_node_register_nodes(sensor->swnodes);
> +			if (ret)
> +				goto err_put_adev;
> +
> +			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> +			if (!fwnode) {
> +				ret = -ENODEV;
> +				goto err_free_swnodes;
> +			}
> +
> +			adev->fwnode.secondary = fwnode;
> +
> +			dev_info(&bridge->cio2->dev,
> +				 "Found supported sensor %s\n",
> +				 acpi_dev_name(adev));
> +
> +			bridge->n_sensors++;

We probably want a check here to avoid overflowing bridge->sensors. The
other option is to make bridge->sensors a struct list_head and allocate
sensors dynamically.

> +		}
> +	}
> +
> +	return ret;
> +
> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);
> +
> +	return ret;
> +}
> +
> +int cio2_bridge_init(struct pci_dev *cio2)
> +{
> +	struct device *dev = &cio2->dev;
> +	struct fwnode_handle *fwnode;
> +	struct cio2_bridge *bridge;
> +	int ret;
> +
> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> +	bridge->cio2_hid_node = (const struct software_node){ bridge->cio2_node_name };

Maybe just

	bridge->cio2_hid_node.name = bridge->cio2_node_name;

as the rest is already zeroed by the kzalloc() call ?

> +	bridge->cio2 = pci_dev_get(cio2);

As the cio2 pointer is only used to print a message in
cio2_bridge_connect_sensors(), do we need to store it in the bridge
structure, and take a reference to the device ?

> +
> +	ret = software_node_register(&bridge->cio2_hid_node);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register the CIO2 HID node\n");
> +		goto err_put_cio2;
> +	}
> +
> +	ret = cio2_bridge_connect_sensors(bridge);
> +	if (ret || bridge->n_sensors == 0)
> +		goto err_unregister_cio2;
> +
> +	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> +
> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> +	if (!fwnode) {
> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> +		ret = -ENODEV;
> +		goto err_unregister_sensors;

Can this happen ?

> +	}
> +
> +	set_secondary_fwnode(dev, fwnode);
> +
> +	return 0;
> +
> +err_unregister_sensors:
> +	cio2_bridge_unregister_sensors(bridge);
> +err_unregister_cio2:
> +	software_node_unregister(&bridge->cio2_hid_node);
> +err_put_cio2:
> +	pci_dev_put(bridge->cio2);
> +
> +	kfree(bridge);
> +	return ret;
> +}
> 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..96f5c8a12be0
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h

This file is only included by cio2-bridge.c, so you could inline it
there. Up to you.

> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_NUM_PORTS			  4

There are a few rogue spaces before '4'.

> +
> +#define NODE_SENSOR(_HID, _PROPS)		\
> +	((const struct software_node) {		\
> +		.name = _HID,			\
> +		.properties = _PROPS,		\
> +	})
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
> +	((const struct software_node) {		\
> +		_PORT,				\
> +		_SENSOR_NODE,			\
> +	})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +	((const struct software_node) {		\
> +		_EP,				\
> +		_PORT,				\
> +		_PROPS,				\
> +	})
> +
> +enum cio2_sensor_swnodes {
> +	SWNODE_SENSOR_HID,
> +	SWNODE_SENSOR_PORT,
> +	SWNODE_SENSOR_ENDPOINT,
> +	SWNODE_CIO2_PORT,
> +	SWNODE_CIO2_ENDPOINT,
> +	NR_OF_SENSOR_SWNODES
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct cio2_sensor_ssdb {
> +	u8 version;
> +	u8 sku;
> +	u8 guid_csi2[16];
> +	u8 devfunction;
> +	u8 bus;
> +	u32 dphylinkenfuses;
> +	u32 clockdiv;
> +	u8 link;
> +	u8 lanes;
> +	u32 csiparams[10];
> +	u32 maxlanespeed;
> +	u8 sensorcalibfileidx;
> +	u8 sensorcalibfileidxInMBZ[3];
> +	u8 romtype;
> +	u8 vcmtype;
> +	u8 platforminfo;
> +	u8 platformsubinfo;
> +	u8 flash;
> +	u8 privacyled;
> +	u8 degree;
> +	u8 mipilinkdefined;
> +	u32 mclkspeed;
> +	u8 controllogicid;
> +	u8 reserved1[3];
> +	u8 mclkport;
> +	u8 reserved2[13];
> +} __packed__;
> +
> +struct cio2_property_names {
> +	char clock_frequency[16];
> +	char rotation[9];
> +	char bus_type[9];
> +	char data_lanes[11];
> +	char remote_endpoint[16];
> +};
> +
> +struct cio2_node_names {
> +	char port[6];
> +	char endpoint[10];
> +	char remote_port[6];
> +};
> +
> +struct cio2_sensor {
> +	char name[ACPI_ID_LEN];
> +	struct acpi_device *adev;
> +
> +	struct software_node swnodes[6];
> +	struct cio2_node_names node_names;
> +
> +	u32 data_lanes[4];
> +	struct cio2_sensor_ssdb ssdb;
> +	struct cio2_property_names prop_names;
> +	struct property_entry ep_properties[4];
> +	struct property_entry dev_properties[3];
> +	struct property_entry cio2_properties[3];
> +	struct software_node_ref_args local_ref[1];
> +	struct software_node_ref_args remote_ref[1];
> +};
> +
> +struct cio2_bridge {
> +	struct pci_dev *cio2;
> +	char cio2_node_name[ACPI_ID_LEN];
> +	struct software_node cio2_hid_node;
> +	unsigned int n_sensors;
> +	struct cio2_sensor sensors[CIO2_NUM_PORTS];
> +};
> +
> +#endif
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 36e354ecf71e..0d69b593e9f0 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1702,6 +1702,22 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>  
> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *endpoint;
> +
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return false;
> +
> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (endpoint) {
> +		fwnode_handle_put(endpoint);
> +		return true;
> +	}
> +
> +	return cio2_check_fwnode_graph(fwnode->secondary);

If we have a fwnode->secondary and this check fails there's something
seriously wrong, I wonder if we should print an error message.

Overall this is nice. I think the next version will get my ack :-)

> +}
> +
>  /**************** PCI interface ****************/
>  
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
> @@ -1715,6 +1731,17 @@ 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(dev_fwnode(&pci_dev->dev))) {
> +		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 ccf0b85ae36f..520a27c9cdad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -437,4 +437,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; }
> +#endif
> +
>  #endif

On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed to this function.

I agree with Laurent.

...

>  	for (i = 0; nodes[i].name; i++) {
> +		if (nodes[i].parent)
> +			if (!software_node_to_swnode(nodes[i].parent)) {
> +				ret = -EINVAL;
> +				goto err_unregister_nodes;
> +			}
> +

Besides that can we pack these conditionals together?

		if (nodes[i].parent && !software_node_to_swnode(nodes[i].parent)) {


Do we have sane ordering in software_node_unregister_nodes()?

On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> this function unregisters software_nodes.

Should be folded in the previous patch. Otherwise we will have a history point
where register() behaves differently to unregister().

...

> + * @nodes: Zero terminated array of software nodes to be unregistered. If
> + * parent pointers are set up in any of the software nodes then the array
> + * MUST be ordered such that parents come before their children.

Please, leave field description short. Rather add another note to the
Description below.

>   *
>   * Unregister multiple software nodes at once.
>   *
> - * NOTE: Be careful using this call if the nodes had parent pointers set up in
> - * them before registering.  If so, it is wiser to remove the nodes
> - * individually, in the correct order (child before parent) instead of relying
> - * on the sequential order of the list of nodes in the array.
> + * NOTE: If you are uncertain whether the array is ordered such that
> + * parents will be unregistered before their children, it is wiser to
> + * remove the nodes individually, in the correct order (child before
> + * parent).
>   */

On Mon, Nov 30, 2020 at 01:31:23PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
>  - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since RFC v3:
> 
> 	- Patch introduced
> 
>  drivers/acpi/utils.c    | 30 ++++++++++++++++++++++++++----
>  include/acpi/acpi_bus.h |  7 +++++++
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index d5411a166685..c177165c8db2 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>  EXPORT_SYMBOL(acpi_dev_present);
>  
>  /**
> - * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> + * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv
>   * @hid: Hardware ID of the device.
>   * @uid: Unique ID of the device, pass NULL to not check _UID
>   * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
>   *
> - * Return the first match of ACPI device if a matching device was present
> + * Return the next match of ACPI device if another matching device was present
>   * at the moment of invocation, or NULL otherwise.
>   *
>   * The caller is responsible to call put_device() on the returned device.
> @@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
>   * See additional information in acpi_dev_present() as well.
>   */
>  struct acpi_device *
> -acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> +acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
>  {
> +	struct device *start = adev ? &adev->dev : NULL;
>  	struct acpi_dev_match_info match = {};
>  	struct device *dev;
>  
> @@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>  	match.uid = uid;
>  	match.hrv = hrv;
>  
> -	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
> +	dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
>  	return dev ? to_acpi_device(dev) : NULL;
>  }
> +EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
> +
> +/**
> + * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return the first match of ACPI device if a matching device was present
> + * at the moment of invocation, or NULL otherwise.
> + *
> + * The caller is responsible to call put_device() on the returned device.
> + *
> + * See additional information in acpi_dev_present() as well.
> + */
> +struct acpi_device *
> +acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> +{
> +	return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
> +}
>  EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
>  
>  /*
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a3abcc4b7d9f..0a028ba967d3 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
>  
>  bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>  
> +struct acpi_device *
> +acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
>  struct acpi_device *
>  acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
>  
> +#define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
> +	for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);	\
> +	     adev;							\
> +	     adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
> +
>  static inline void acpi_dev_put(struct acpi_device *adev)
>  {
>  	put_device(&adev->dev);
> -- 
> 2.25.1
>

Hi Daniel,

Thanks for the update! This is starting to look really nice!

Please still see my comments below.

On Mon, Nov 30, 2020 at 01:31:24PM +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.
> 
> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since RFC v3:
> 
> 	- Removed almost all global variables, dynamically allocated
> 	the cio2_bridge structure, plus a bunch of associated changes
> 	like 
> 	- Added a new function to ipu3-cio2-main.c to check for an 
> 	existing fwnode_graph before calling cio2_bridge_init()
> 	- Prefixed cio2_bridge_ to any variables and functions that
> 	lacked it
> 	- Assigned the new fwnode directly to the sensor's ACPI device
> 	fwnode as secondary. This removes the requirement to delay until
> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
> 	it has a side effect, which is that those devices then grab a ref
> 	to the new software_node. This effectively prevents us from
> 	unloading the driver, because we can't free the memory that they
> 	live in whilst the device holds a reference to them. The work
> 	around at the moment is to _not_ unregister the software_nodes
> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
> 	is simply skipped if the module is reloaded.
> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
> 	- Replaced ints with unsigned ints where appropriate
> 	- Iterated over all ACPI devices of a matching _HID rather than
> 	just the first to ensure we handle a device with multiple sensors
> 	of the same model.
> 
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 421 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9702b886d6a4..188559a0a610 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>  M:	Yong Zhi <yong.zhi@intel.com>
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  M:	Bingbu Cao <bingbu.cao@intel.com>
> +M:	Dan Scally <djrscally@gmail.com>
>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> index 82d7f17e6a02..2b3350d042be 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>  	  connected camera.
>  	  The module will be called ipu3-cio2.
> +
> +config CIO2_BRIDGE
> +	bool "IPU3 CIO2 Sensors Bridge"
> +	depends on VIDEO_IPU3_CIO2
> +	help
> +	  This extension provides an API for the ipu3-cio2 driver to create
> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
> +	  can be used to enable support for cameras in detachable / hybrid
> +	  devices that ship with Windows.
> +
> +	  Say Y here if your device is a detachable / hybrid laptop that comes
> +	  with Windows installed by the OEM, for example:
> +
> +	  	- Microsoft Surface models (except Surface Pro 3)
> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
> +		- Dell 7285
> +
> +	  If in doubt, say N here.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> index 429d516452e4..933777e6ea8a 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>  
>  ipu3-cio2-y += ipu3-cio2-main.o
> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..fd3f8ba07274
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/property.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include "cio2-bridge.h"
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be working.
> + * Do not add a HID for a sensor that is not actually supported.
> + */
> +static const char * const cio2_supported_devices[] = {
> +	"INT33BE",
> +	"OVTI2680",

I guess we don't have the known-good frequencies for the CSI-2 bus in
firmware?

One option would be to put there what the drivers currently use. This
assumes the support for these devices is, well, somewhat opportunistic but
I guess there's no way around that right now at least.

As the systems are laptops, they're likely somewhat less prone to EMI
issues to begin with than mobile phones anyway.

> +};
> +
> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> +					void *data, u32 size)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int ret;
> +
> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = buffer.pointer;
> +	if (!obj) {
> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> +		return -ENODEV;
> +	}
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
> +		ret = -ENODEV;
> +		goto out_free_buff;
> +	}
> +
> +	if (obj->buffer.length > size) {
> +		dev_err(&adev->dev, "Given buffer is too small\n");
> +		ret = -EINVAL;
> +		goto out_free_buff;
> +	}
> +
> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
> +	ret = obj->buffer.length;
> +
> +out_free_buff:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +
> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> +{
> +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
> +	strcpy(sensor->prop_names.rotation, "rotation");
> +	strcpy(sensor->prop_names.bus_type, "bus-type");
> +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
> +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");

Please use the actual field size instead with strncpy / strscpy.

> +}
> +
> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
> +{
> +	unsigned int i;
> +
> +	cio2_bridge_init_property_names(sensor);
> +
> +	for (i = 0; i < 4; i++)
> +		sensor->data_lanes[i] = i + 1;
> +
> +	/*
> +	 * Can't use PROPERTY_ENTRY_REF because it creates a new variable to
> +	 * point to, which doesn't survive the function.
> +	 */
> +	sensor->local_ref[0] = (struct software_node_ref_args){
> +		.node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT]
> +		};

I guess this line should be unindented by one tab stop. Same for the
similar case below.

> +	sensor->remote_ref[0] = (struct software_node_ref_args){
> +		.node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT]
> +		};
> +
> +	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> +						       sensor->ssdb.mclkspeed);
> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> +						      sensor->ssdb.degree);
> +
> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 5);
> +	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								sensor->data_lanes,
> +								sensor->ssdb.lanes);
> +	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							    sensor->local_ref);
> +
> +	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								  sensor->data_lanes,
> +								  sensor->ssdb.lanes);
> +	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							      sensor->remote_ref);
> +}
> +
> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +	snprintf(sensor->node_names.remote_port, 6, "port%u", sensor->ssdb.link);
> +	strcpy(sensor->node_names.port, "port0");
> +	strcpy(sensor->node_names.endpoint, "endpoint0");

Please use the actual size of the field, and strncpy / strscpy.

> +}
> +
> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> +						  struct cio2_sensor *sensor)
> +{
> +	struct software_node *nodes = sensor->swnodes;
> +
> +	cio2_bridge_init_swnode_names(sensor);
> +
> +	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> +					       sensor->dev_properties);
> +	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> +					      &nodes[SWNODE_SENSOR_HID]);
> +	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						      &nodes[SWNODE_SENSOR_PORT],
> +						      sensor->ep_properties);
> +	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> +					    &bridge->cio2_hid_node);
> +	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						    &nodes[SWNODE_CIO2_PORT],
> +						    sensor->cio2_properties);
> +}
> +
> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> +{
> +	struct cio2_sensor *sensor;
> +	unsigned int i;
> +
> +	for (i = 0; i < bridge->n_sensors; i++) {
> +		sensor = &bridge->sensors[i];
> +		software_node_unregister_nodes(sensor->swnodes);
> +		acpi_dev_put(sensor->adev);
> +	}
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct cio2_sensor *sensor;
> +	struct acpi_device *adev;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_devices); i++) {
> +		const char *this_device = cio2_supported_devices[i];
> +
> +		for_each_acpi_dev_match(adev, this_device, NULL, -1) {
> +			if (!adev || !(adev->status.present && adev->status.enabled))
> +				continue;
> +
> +			sensor = &bridge->sensors[bridge->n_sensors];
> +			sensor->adev = adev;
> +			strscpy(sensor->name, this_device, sizeof(sensor->name));
> +
> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +							   &sensor->ssdb,
> +							   sizeof(sensor->ssdb));
> +			if (ret < 0)
> +				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;
> +			}
> +
> +			cio2_bridge_create_fwnode_properties(sensor);
> +			cio2_bridge_create_connection_swnodes(bridge, sensor);
> +
> +			ret = software_node_register_nodes(sensor->swnodes);
> +			if (ret)
> +				goto err_put_adev;
> +
> +			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> +			if (!fwnode) {
> +				ret = -ENODEV;
> +				goto err_free_swnodes;
> +			}
> +
> +			adev->fwnode.secondary = fwnode;
> +
> +			dev_info(&bridge->cio2->dev,
> +				 "Found supported sensor %s\n",
> +				 acpi_dev_name(adev));
> +
> +			bridge->n_sensors++;
> +		}
> +	}
> +
> +	return ret;
> +
> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);
> +
> +	return ret;
> +}
> +
> +int cio2_bridge_init(struct pci_dev *cio2)
> +{
> +	struct device *dev = &cio2->dev;
> +	struct fwnode_handle *fwnode;
> +	struct cio2_bridge *bridge;
> +	int ret;
> +
> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> +	bridge->cio2_hid_node = (const struct software_node){ bridge->cio2_node_name };
> +	bridge->cio2 = pci_dev_get(cio2);
> +
> +	ret = software_node_register(&bridge->cio2_hid_node);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register the CIO2 HID node\n");
> +		goto err_put_cio2;
> +	}
> +
> +	ret = cio2_bridge_connect_sensors(bridge);
> +	if (ret || bridge->n_sensors == 0)
> +		goto err_unregister_cio2;
> +
> +	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> +
> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> +	if (!fwnode) {
> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> +		ret = -ENODEV;
> +		goto err_unregister_sensors;
> +	}
> +
> +	set_secondary_fwnode(dev, fwnode);
> +
> +	return 0;
> +
> +err_unregister_sensors:
> +	cio2_bridge_unregister_sensors(bridge);
> +err_unregister_cio2:
> +	software_node_unregister(&bridge->cio2_hid_node);
> +err_put_cio2:
> +	pci_dev_put(bridge->cio2);
> +
> +	kfree(bridge);
> +	return ret;
> +}
> 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..96f5c8a12be0
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_NUM_PORTS			  4
> +
> +#define NODE_SENSOR(_HID, _PROPS)		\
> +	((const struct software_node) {		\
> +		.name = _HID,			\
> +		.properties = _PROPS,		\
> +	})
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
> +	((const struct software_node) {		\
> +		_PORT,				\
> +		_SENSOR_NODE,			\
> +	})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +	((const struct software_node) {		\
> +		_EP,				\
> +		_PORT,				\
> +		_PROPS,				\
> +	})
> +
> +enum cio2_sensor_swnodes {
> +	SWNODE_SENSOR_HID,
> +	SWNODE_SENSOR_PORT,
> +	SWNODE_SENSOR_ENDPOINT,
> +	SWNODE_CIO2_PORT,
> +	SWNODE_CIO2_ENDPOINT,
> +	NR_OF_SENSOR_SWNODES
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct cio2_sensor_ssdb {
> +	u8 version;
> +	u8 sku;
> +	u8 guid_csi2[16];
> +	u8 devfunction;
> +	u8 bus;
> +	u32 dphylinkenfuses;
> +	u32 clockdiv;
> +	u8 link;
> +	u8 lanes;
> +	u32 csiparams[10];
> +	u32 maxlanespeed;
> +	u8 sensorcalibfileidx;
> +	u8 sensorcalibfileidxInMBZ[3];
> +	u8 romtype;
> +	u8 vcmtype;
> +	u8 platforminfo;
> +	u8 platformsubinfo;
> +	u8 flash;
> +	u8 privacyled;
> +	u8 degree;
> +	u8 mipilinkdefined;
> +	u32 mclkspeed;
> +	u8 controllogicid;
> +	u8 reserved1[3];
> +	u8 mclkport;
> +	u8 reserved2[13];
> +} __packed__;

This should be "__packed".

> +
> +struct cio2_property_names {
> +	char clock_frequency[16];
> +	char rotation[9];
> +	char bus_type[9];
> +	char data_lanes[11];
> +	char remote_endpoint[16];
> +};
> +
> +struct cio2_node_names {
> +	char port[6];
> +	char endpoint[10];
> +	char remote_port[6];
> +};
> +
> +struct cio2_sensor {
> +	char name[ACPI_ID_LEN];
> +	struct acpi_device *adev;
> +
> +	struct software_node swnodes[6];
> +	struct cio2_node_names node_names;
> +
> +	u32 data_lanes[4];
> +	struct cio2_sensor_ssdb ssdb;
> +	struct cio2_property_names prop_names;
> +	struct property_entry ep_properties[4];
> +	struct property_entry dev_properties[3];
> +	struct property_entry cio2_properties[3];
> +	struct software_node_ref_args local_ref[1];
> +	struct software_node_ref_args remote_ref[1];
> +};
> +
> +struct cio2_bridge {
> +	struct pci_dev *cio2;
> +	char cio2_node_name[ACPI_ID_LEN];
> +	struct software_node cio2_hid_node;
> +	unsigned int n_sensors;
> +	struct cio2_sensor sensors[CIO2_NUM_PORTS];
> +};
> +
> +#endif
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 36e354ecf71e..0d69b593e9f0 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1702,6 +1702,22 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>  
> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *endpoint;
> +
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return false;
> +
> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (endpoint) {
> +		fwnode_handle_put(endpoint);
> +		return true;
> +	}
> +
> +	return cio2_check_fwnode_graph(fwnode->secondary);
> +}
> +
>  /**************** PCI interface ****************/
>  
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
> @@ -1715,6 +1731,17 @@ 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(dev_fwnode(&pci_dev->dev))) {
> +		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 ccf0b85ae36f..520a27c9cdad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -437,4 +437,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; }
> +#endif
> +
>  #endif

Hi Laurent

On 30/11/2020 16:12, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 06:11:52PM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> Thank you for the patch.
>>
>> On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
>>> Registering software_nodes with the .parent member set to point to a
>>> currently unregistered software_node has the potential for problems,
>>> so enforce parent -> child ordering in arrays passed to this function.
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>> ---
>>> Changes since RFC v3:
>>>
>>> 	Patch introduced
>>>
>>>  drivers/base/swnode.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>> index 615a0c93e116..af7930b3679e 100644
>>> --- a/drivers/base/swnode.c
>>> +++ b/drivers/base/swnode.c
>>> @@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
>>>  	int i;
>>>  
>>>  	for (i = 0; nodes[i].name; i++) {
>>> +		if (nodes[i].parent)
>>> +			if (!software_node_to_swnode(nodes[i].parent)) {
>>> +				ret = -EINVAL;
>>> +				goto err_unregister_nodes;
>>> +			}
>>> +
>>>  		ret = software_node_register(&nodes[i]);
>>> -		if (ret) {
>>> -			software_node_unregister_nodes(nodes);
>>> -			return ret;
>>> -		}
>>> +		if (ret)
>>> +			goto err_unregister_nodes;
>>>  	}
>>>  
>>>  	return 0;
>> I'd add a blank line here.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> I spoke a bit too soon. Could you update the documentation of the
> function to explain this new requirement ?
Oops - of course, will do
>>> +err_unregister_nodes:
>>> +	software_node_unregister_nodes(nodes);
>>> +	return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(software_node_register_nodes);
>>>  
>> -- 
>> Regards,
>>
>> Laurent Pinchart

Hi Sakari

On 30/11/2020 20:35, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the update! This is starting to look really nice!
>
> Please still see my comments below.

Thanks!

>
> On Mon, Nov 30, 2020 at 01:31:24PM +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.
>>
>> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes since RFC v3:
>>
>> 	- Removed almost all global variables, dynamically allocated
>> 	the cio2_bridge structure, plus a bunch of associated changes
>> 	like 
>> 	- Added a new function to ipu3-cio2-main.c to check for an 
>> 	existing fwnode_graph before calling cio2_bridge_init()
>> 	- Prefixed cio2_bridge_ to any variables and functions that
>> 	lacked it
>> 	- Assigned the new fwnode directly to the sensor's ACPI device
>> 	fwnode as secondary. This removes the requirement to delay until
>> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
>> 	it has a side effect, which is that those devices then grab a ref
>> 	to the new software_node. This effectively prevents us from
>> 	unloading the driver, because we can't free the memory that they
>> 	live in whilst the device holds a reference to them. The work
>> 	around at the moment is to _not_ unregister the software_nodes
>> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
>> 	is simply skipped if the module is reloaded.
>> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
>> 	- Replaced ints with unsigned ints where appropriate
>> 	- Iterated over all ACPI devices of a matching _HID rather than
>> 	just the first to ensure we handle a device with multiple sensors
>> 	of the same model.
>>
>>  MAINTAINERS                                   |   1 +
>>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
>>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
>>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
>>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>>  7 files changed, 421 insertions(+)
>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9702b886d6a4..188559a0a610 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>>  M:	Yong Zhi <yong.zhi@intel.com>
>>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>>  M:	Bingbu Cao <bingbu.cao@intel.com>
>> +M:	Dan Scally <djrscally@gmail.com>
>>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>>  L:	linux-media@vger.kernel.org
>>  S:	Maintained
>> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
>> index 82d7f17e6a02..2b3350d042be 100644
>> --- a/drivers/media/pci/intel/ipu3/Kconfig
>> +++ b/drivers/media/pci/intel/ipu3/Kconfig
>> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>>  	  connected camera.
>>  	  The module will be called ipu3-cio2.
>> +
>> +config CIO2_BRIDGE
>> +	bool "IPU3 CIO2 Sensors Bridge"
>> +	depends on VIDEO_IPU3_CIO2
>> +	help
>> +	  This extension provides an API for the ipu3-cio2 driver to create
>> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
>> +	  can be used to enable support for cameras in detachable / hybrid
>> +	  devices that ship with Windows.
>> +
>> +	  Say Y here if your device is a detachable / hybrid laptop that comes
>> +	  with Windows installed by the OEM, for example:
>> +
>> +	  	- Microsoft Surface models (except Surface Pro 3)
>> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
>> +		- Dell 7285
>> +
>> +	  If in doubt, say N here.
>> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
>> index 429d516452e4..933777e6ea8a 100644
>> --- a/drivers/media/pci/intel/ipu3/Makefile
>> +++ b/drivers/media/pci/intel/ipu3/Makefile
>> @@ -2,3 +2,4 @@
>>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>>  
>>  ipu3-cio2-y += ipu3-cio2-main.o
>> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>> new file mode 100644
>> index 000000000000..fd3f8ba07274
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>> @@ -0,0 +1,260 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Dan Scally <djrscally@gmail.com> */
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/property.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#include "cio2-bridge.h"
>> +
>> +/*
>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>> + * Do not add a HID for a sensor that is not actually supported.
>> + */
>> +static const char * const cio2_supported_devices[] = {
>> +	"INT33BE",
>> +	"OVTI2680",
> I guess we don't have the known-good frequencies for the CSI-2 bus in
> firmware?
You mean link-frequencies? Indeed I can't see it anywhere in the buffers
from ACPI
> One option would be to put there what the drivers currently use. This
> assumes the support for these devices is, well, somewhat opportunistic but
> I guess there's no way around that right now at least.
>
> As the systems are laptops, they're likely somewhat less prone to EMI
> issues to begin with than mobile phones anyway.

Ah I guess that's a good point...and then add it as a property along
with the rest.


Ack to the other comments; I'll make those changes.

On Mon, Nov 30, 2020 at 10:35:51PM +0200, Sakari Ailus wrote:
> On Mon, Nov 30, 2020 at 01:31:24PM +0000, Daniel Scally wrote:

...

> > +/*
> > + * Extend this array with ACPI Hardware ID's of devices known to be working.
> > + * Do not add a HID for a sensor that is not actually supported.
> > + */
> > +static const char * const cio2_supported_devices[] = {
> > +	"INT33BE",
> > +	"OVTI2680",
> 
> I guess we don't have the known-good frequencies for the CSI-2 bus in
> firmware?
> 
> One option would be to put there what the drivers currently use. This
> assumes the support for these devices is, well, somewhat opportunistic but
> I guess there's no way around that right now at least.
> 
> As the systems are laptops, they're likely somewhat less prone to EMI
> issues to begin with than mobile phones anyway.

ACPI has only XTAL clock frequency (dunno if it's the same as CSI-2 bus clock).
Currently it may be one out of 19.2 MHz, 24 MHz (with a remark that all sensors
must use same value as PMIC can't produce several clocks).

> > +};

...

> > +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
> > +	strcpy(sensor->prop_names.rotation, "rotation");
> > +	strcpy(sensor->prop_names.bus_type, "bus-type");
> > +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
> > +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");
> 
> Please use the actual field size instead with strncpy / strscpy.

Perhaps Laurent's proposal is better?

Hi Laurent - thanks for reviewing

On 30/11/2020 17:09, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Mon, Nov 30, 2020 at 01:31:24PM +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.
>>
>> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes since RFC v3:
>>
>> 	- Removed almost all global variables, dynamically allocated
>> 	the cio2_bridge structure, plus a bunch of associated changes
>> 	like 
>> 	- Added a new function to ipu3-cio2-main.c to check for an 
>> 	existing fwnode_graph before calling cio2_bridge_init()
>> 	- Prefixed cio2_bridge_ to any variables and functions that
>> 	lacked it
>> 	- Assigned the new fwnode directly to the sensor's ACPI device
>> 	fwnode as secondary. This removes the requirement to delay until
>> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
>> 	it has a side effect, which is that those devices then grab a ref
>> 	to the new software_node. This effectively prevents us from
>> 	unloading the driver, because we can't free the memory that they
>> 	live in whilst the device holds a reference to them. The work
>> 	around at the moment is to _not_ unregister the software_nodes
>> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
>> 	is simply skipped if the module is reloaded.
>> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
>> 	- Replaced ints with unsigned ints where appropriate
>> 	- Iterated over all ACPI devices of a matching _HID rather than
>> 	just the first to ensure we handle a device with multiple sensors
>> 	of the same model.
>>
>>  MAINTAINERS                                   |   1 +
>>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
>>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
>>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
>>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>>  7 files changed, 421 insertions(+)
>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9702b886d6a4..188559a0a610 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>>  M:	Yong Zhi <yong.zhi@intel.com>
>>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>>  M:	Bingbu Cao <bingbu.cao@intel.com>
>> +M:	Dan Scally <djrscally@gmail.com>
>>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>>  L:	linux-media@vger.kernel.org
>>  S:	Maintained
>> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
>> index 82d7f17e6a02..2b3350d042be 100644
>> --- a/drivers/media/pci/intel/ipu3/Kconfig
>> +++ b/drivers/media/pci/intel/ipu3/Kconfig
>> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>>  	  connected camera.
>>  	  The module will be called ipu3-cio2.
>> +
>> +config CIO2_BRIDGE
>> +	bool "IPU3 CIO2 Sensors Bridge"
>> +	depends on VIDEO_IPU3_CIO2
>> +	help
>> +	  This extension provides an API for the ipu3-cio2 driver to create
>> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
>> +	  can be used to enable support for cameras in detachable / hybrid
>> +	  devices that ship with Windows.
>> +
>> +	  Say Y here if your device is a detachable / hybrid laptop that comes
>> +	  with Windows installed by the OEM, for example:
>> +
>> +	  	- Microsoft Surface models (except Surface Pro 3)
>> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
>> +		- Dell 7285
>> +
>> +	  If in doubt, say N here.
>> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
>> index 429d516452e4..933777e6ea8a 100644
>> --- a/drivers/media/pci/intel/ipu3/Makefile
>> +++ b/drivers/media/pci/intel/ipu3/Makefile
>> @@ -2,3 +2,4 @@
>>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>>  
>>  ipu3-cio2-y += ipu3-cio2-main.o
>> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>> new file mode 100644
>> index 000000000000..fd3f8ba07274
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>> @@ -0,0 +1,260 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Dan Scally <djrscally@gmail.com> */
> 
> Could you please add a blank line here ?

Yes
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
> 
> Is this header needed ?
> 
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
> 
> And this one ?
> 
>> +#include <linux/pci.h>
>> +#include <linux/property.h>
>> +#include <media/v4l2-subdev.h>
> 
> And this one ?

Ah yes - bit sloppy, they're orphaned from earlier versions, sorry about
that.

>> +
>> +#include "cio2-bridge.h"
>> +
>> +/*
>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>> + * Do not add a HID for a sensor that is not actually supported.
>> + */
>> +static const char * const cio2_supported_devices[] = {
> 
> Maybe cio2_supported_sensors ?

Sure

>> +	"INT33BE",
>> +	"OVTI2680",
>> +};
>> +
>> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
>> +					void *data, u32 size)
>> +{
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	union acpi_object *obj;
>> +	acpi_status status;
>> +	int ret;
>> +
>> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>> +
>> +	obj = buffer.pointer;
>> +	if (!obj) {
>> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (obj->type != ACPI_TYPE_BUFFER) {
>> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
>> +		ret = -ENODEV;
>> +		goto out_free_buff;
>> +	}
>> +
>> +	if (obj->buffer.length > size) {
>> +		dev_err(&adev->dev, "Given buffer is too small\n");
>> +		ret = -EINVAL;
>> +		goto out_free_buff;
>> +	}
>> +
>> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
>> +	ret = obj->buffer.length;
>> +
>> +out_free_buff:
>> +	kfree(buffer.pointer);
>> +	return ret;
>> +}
>> +
>> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>> +{
>> +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
>> +	strcpy(sensor->prop_names.rotation, "rotation");
>> +	strcpy(sensor->prop_names.bus_type, "bus-type");
>> +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
>> +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");
> 
> This is a bit fragile, as there's no len check. How about the following
> ?
> static const struct cio2_property_names prop_names = {
> 	.clock_frequency = "clock-frequency",
> 	.rotation = "rotation",
> 	.bus_type = "bus-type",
> 	.data_lanes = "data-lanes",
> 	.remote_endpoint = "remote-endpoint",
> };
> 
> static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> {
> 	sensor->prop_names = prop_names;
> }
> 
> This shoudl generate a compilation warning if the string is too long.
> 
> You could even inline that line in
> cio2_bridge_create_fwnode_properties().

Yes, I like that, thanks - I'll make the change.

>> +}
>> +
>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
>> +{
>> +	unsigned int i;
>> +
>> +	cio2_bridge_init_property_names(sensor);
>> +
>> +	for (i = 0; i < 4; i++)
>> +		sensor->data_lanes[i] = i + 1;
> 
> Is there no provision in the SSDB for data lane remapping ?

Sorry; don't follow what you mean by data lane remapping here.

>> +
>> +	/*
>> +	 * Can't use PROPERTY_ENTRY_REF because it creates a new variable to
>> +	 * point to, which doesn't survive the function.
>> +	 */
>> +	sensor->local_ref[0] = (struct software_node_ref_args){
>> +		.node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT]
>> +		};
> 
> I'd remove one tab here. Or just write
> 
> 	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];

Yep, changed.

>> +	sensor->remote_ref[0] = (struct software_node_ref_args){
>> +		.node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT]
>> +		};
>> +
>> +	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
>> +						       sensor->ssdb.mclkspeed);
>> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
>> +						      sensor->ssdb.degree);
>> +
>> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 5);
>> +	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
>> +								sensor->data_lanes,
>> +								sensor->ssdb.lanes);
>> +	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
>> +							    sensor->local_ref);
>> +
>> +	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
>> +								  sensor->data_lanes,
>> +								  sensor->ssdb.lanes);
>> +	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
>> +							      sensor->remote_ref);
>> +}
>> +
>> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
>> +{
>> +	snprintf(sensor->node_names.remote_port, 6, "port%u", sensor->ssdb.link);
>> +	strcpy(sensor->node_names.port, "port0");
>> +	strcpy(sensor->node_names.endpoint, "endpoint0");
>> +}
>> +
>> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
>> +						  struct cio2_sensor *sensor)
>> +{
>> +	struct software_node *nodes = sensor->swnodes;
>> +
>> +	cio2_bridge_init_swnode_names(sensor);
>> +
>> +	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
>> +					       sensor->dev_properties);
>> +	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
>> +					      &nodes[SWNODE_SENSOR_HID]);
>> +	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
>> +						      &nodes[SWNODE_SENSOR_PORT],
>> +						      sensor->ep_properties);
>> +	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
>> +					    &bridge->cio2_hid_node);
>> +	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
>> +						    &nodes[SWNODE_CIO2_PORT],
>> +						    sensor->cio2_properties);
>> +}
>> +
>> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
>> +{
>> +	struct cio2_sensor *sensor;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < bridge->n_sensors; i++) {
>> +		sensor = &bridge->sensors[i];
>> +		software_node_unregister_nodes(sensor->swnodes);
>> +		acpi_dev_put(sensor->adev);
>> +	}
>> +}
>> +
>> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge)
>> +{
>> +	struct fwnode_handle *fwnode;
>> +	struct cio2_sensor *sensor;
>> +	struct acpi_device *adev;
>> +	unsigned int i;
>> +	int ret = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_devices); i++) {
>> +		const char *this_device = cio2_supported_devices[i];
> 
> s/this_device/name/ (or sensor_name, ...) ?

I went for hid as Andy suggested.
> 
>> +
>> +		for_each_acpi_dev_match(adev, this_device, NULL, -1) {
>> +			if (!adev || !(adev->status.present && adev->status.enabled))
> 
> 			if (!adev || !adev->status.present || !adev->status.enabled))
> 
> may be a bit more readable. Does for_each_acpi_dev_match() return NULL
> devices though ? If no, you could drop the !adev check. You may also be
> able to drop the !present check, as I don't think ACPI allows !present
> && enabled.

You're right, the spec mandates enabled be 0 if present is 0. The
iterator will return NULL when the previous return value was the last
matching device, so that part needs to stay, but it can become:

if (!adev || !adev->status.enabled)

>> +				continue;
>> +
>> +			sensor = &bridge->sensors[bridge->n_sensors];
>> +			sensor->adev = adev;
>> +			strscpy(sensor->name, this_device, sizeof(sensor->name));
>> +
>> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
>> +							   &sensor->ssdb,
>> +							   sizeof(sensor->ssdb));
>> +			if (ret < 0)
>> +				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;
>> +			}
>> +
>> +			cio2_bridge_create_fwnode_properties(sensor);
>> +			cio2_bridge_create_connection_swnodes(bridge, sensor);
>> +
>> +			ret = software_node_register_nodes(sensor->swnodes);
>> +			if (ret)
>> +				goto err_put_adev;
>> +
>> +			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
>> +			if (!fwnode) {
>> +				ret = -ENODEV;
>> +				goto err_free_swnodes;
>> +			}
>> +
>> +			adev->fwnode.secondary = fwnode;
>> +
>> +			dev_info(&bridge->cio2->dev,
>> +				 "Found supported sensor %s\n",
>> +				 acpi_dev_name(adev));
>> +
>> +			bridge->n_sensors++;
> 
> We probably want a check here to avoid overflowing bridge->sensors. The
> other option is to make bridge->sensors a struct list_head and allocate
> sensors dynamically.

Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
the maximum. Seems easier to just do a check, unless the wasted memory
is enough that it's worth allocating dynamically. I don't mind either
approach.

>> +		}
>> +	}
>> +
>> +	return ret;
>> +
>> +err_free_swnodes:
>> +	software_node_unregister_nodes(sensor->swnodes);
>> +err_put_adev:
>> +	acpi_dev_put(sensor->adev);
>> +
>> +	return ret;
>> +}
>> +
>> +int cio2_bridge_init(struct pci_dev *cio2)
>> +{
>> +	struct device *dev = &cio2->dev;
>> +	struct fwnode_handle *fwnode;
>> +	struct cio2_bridge *bridge;
>> +	int ret;
>> +
>> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
>> +	if (!bridge)
>> +		return -ENOMEM;
>> +
>> +	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
>> +	bridge->cio2_hid_node = (const struct software_node){ bridge->cio2_node_name };
> 
> Maybe just
> 
> 	bridge->cio2_hid_node.name = bridge->cio2_node_name;
> 
> as the rest is already zeroed by the kzalloc() call ?
> 
>> +	bridge->cio2 = pci_dev_get(cio2);
> 
> As the cio2 pointer is only used to print a message in
> cio2_bridge_connect_sensors(), do we need to store it in the bridge
> structure, and take a reference to the device ?
> 
>> +
>> +	ret = software_node_register(&bridge->cio2_hid_node);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to register the CIO2 HID node\n");
>> +		goto err_put_cio2;
>> +	}
>> +
>> +	ret = cio2_bridge_connect_sensors(bridge);
>> +	if (ret || bridge->n_sensors == 0)
>> +		goto err_unregister_cio2;
>> +
>> +	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
>> +
>> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
>> +	if (!fwnode) {
>> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
>> +		ret = -ENODEV;
>> +		goto err_unregister_sensors;
> 
> Can this happen ?

It _shouldn't_ happen, as long as nothing else is touching the swnodes
I've registered or anything. I've never seen it happen. That didn't feel
like quite enough to say it can't ever happen - but I'm happy to skip
the check if you think thats ok.

>> +	}
>> +
>> +	set_secondary_fwnode(dev, fwnode);
>> +
>> +	return 0;
>> +
>> +err_unregister_sensors:
>> +	cio2_bridge_unregister_sensors(bridge);
>> +err_unregister_cio2:
>> +	software_node_unregister(&bridge->cio2_hid_node);
>> +err_put_cio2:
>> +	pci_dev_put(bridge->cio2);
>> +
>> +	kfree(bridge);
>> +	return ret;
>> +}
>> 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..96f5c8a12be0
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> 
> This file is only included by cio2-bridge.c, so you could inline it
> there. Up to you.

I think I like them separate


>> @@ -0,0 +1,108 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Author: Dan Scally <djrscally@gmail.com> */
>> +#ifndef __CIO2_BRIDGE_H
>> +#define __CIO2_BRIDGE_H
>> +
>> +#include <linux/property.h>
>> +
>> +#define CIO2_HID				"INT343E"
>> +#define CIO2_NUM_PORTS			  4
> 
> There are a few rogue spaces before '4'.

Argh, thanks, this is the curse of using VS code on multiple machines...
> 
>> +
>> +#define NODE_SENSOR(_HID, _PROPS)		\
>> +	((const struct software_node) {		\
>> +		.name = _HID,			\
>> +		.properties = _PROPS,		\
>> +	})
>> +
>> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
>> +	((const struct software_node) {		\
>> +		_PORT,				\
>> +		_SENSOR_NODE,			\
>> +	})
>> +
>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
>> +	((const struct software_node) {		\
>> +		_EP,				\
>> +		_PORT,				\
>> +		_PROPS,				\
>> +	})
>> +
>> +enum cio2_sensor_swnodes {
>> +	SWNODE_SENSOR_HID,
>> +	SWNODE_SENSOR_PORT,
>> +	SWNODE_SENSOR_ENDPOINT,
>> +	SWNODE_CIO2_PORT,
>> +	SWNODE_CIO2_ENDPOINT,
>> +	NR_OF_SENSOR_SWNODES
>> +};
>> +
>> +/* Data representation as it is in ACPI SSDB buffer */
>> +struct cio2_sensor_ssdb {
>> +	u8 version;
>> +	u8 sku;
>> +	u8 guid_csi2[16];
>> +	u8 devfunction;
>> +	u8 bus;
>> +	u32 dphylinkenfuses;
>> +	u32 clockdiv;
>> +	u8 link;
>> +	u8 lanes;
>> +	u32 csiparams[10];
>> +	u32 maxlanespeed;
>> +	u8 sensorcalibfileidx;
>> +	u8 sensorcalibfileidxInMBZ[3];
>> +	u8 romtype;
>> +	u8 vcmtype;
>> +	u8 platforminfo;
>> +	u8 platformsubinfo;
>> +	u8 flash;
>> +	u8 privacyled;
>> +	u8 degree;
>> +	u8 mipilinkdefined;
>> +	u32 mclkspeed;
>> +	u8 controllogicid;
>> +	u8 reserved1[3];
>> +	u8 mclkport;
>> +	u8 reserved2[13];
>> +} __packed__;
>> +
>> +struct cio2_property_names {
>> +	char clock_frequency[16];
>> +	char rotation[9];
>> +	char bus_type[9];
>> +	char data_lanes[11];
>> +	char remote_endpoint[16];
>> +};
>> +
>> +struct cio2_node_names {
>> +	char port[6];
>> +	char endpoint[10];
>> +	char remote_port[6];
>> +};
>> +
>> +struct cio2_sensor {
>> +	char name[ACPI_ID_LEN];
>> +	struct acpi_device *adev;
>> +
>> +	struct software_node swnodes[6];
>> +	struct cio2_node_names node_names;
>> +
>> +	u32 data_lanes[4];
>> +	struct cio2_sensor_ssdb ssdb;
>> +	struct cio2_property_names prop_names;
>> +	struct property_entry ep_properties[4];
>> +	struct property_entry dev_properties[3];
>> +	struct property_entry cio2_properties[3];
>> +	struct software_node_ref_args local_ref[1];
>> +	struct software_node_ref_args remote_ref[1];
>> +};
>> +
>> +struct cio2_bridge {
>> +	struct pci_dev *cio2;
>> +	char cio2_node_name[ACPI_ID_LEN];
>> +	struct software_node cio2_hid_node;
>> +	unsigned int n_sensors;
>> +	struct cio2_sensor sensors[CIO2_NUM_PORTS];
>> +};
>> +
>> +#endif
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>> index 36e354ecf71e..0d69b593e9f0 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>> @@ -1702,6 +1702,22 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>>  }
>>  
>> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
>> +{
>> +	struct fwnode_handle *endpoint;
>> +
>> +	if (IS_ERR_OR_NULL(fwnode))
>> +		return false;
>> +
>> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
>> +	if (endpoint) {
>> +		fwnode_handle_put(endpoint);
>> +		return true;
>> +	}
>> +
>> +	return cio2_check_fwnode_graph(fwnode->secondary);
> 
> If we have a fwnode->secondary and this check fails there's something
> seriously wrong, I wonder if we should print an error message.

Yes, probably a good thought, since nothing will work in that case. I'll
add something appropriate.

> 
> Overall this is nice. I think the next version will get my ack :-)

Excellent :)

>> +}
>> +
>>  /**************** PCI interface ****************/
>>  
>>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>> @@ -1715,6 +1731,17 @@ 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(dev_fwnode(&pci_dev->dev))) {
>> +		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 ccf0b85ae36f..520a27c9cdad 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> @@ -437,4 +437,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; }
>> +#endif
>> +
>>  #endif
>

On 01/12/2020 22:08, Dan Scally wrote:
>>> +
>>> +		for_each_acpi_dev_match(adev, this_device, NULL, -1) {
>>> +			if (!adev || !(adev->status.present && adev->status.enabled))
>> 			if (!adev || !adev->status.present || !adev->status.enabled))
>>
>> may be a bit more readable. Does for_each_acpi_dev_match() return NULL
>> devices though ? If no, you could drop the !adev check. You may also be
>> able to drop the !present check, as I don't think ACPI allows !present
>> && enabled.
> You're right, the spec mandates enabled be 0 if present is 0. The
> iterator will return NULL when the previous return value was the last
> matching device, so that part needs to stay, but it can become:
>
> if (!adev || !adev->status.enabled)
>
Wait, that's silly, the loop won't start if the check is null so you're
right of course.

Hi Daniel,

On Tue, Dec 01, 2020 at 10:08:25PM +0000, Dan Scally wrote:
> On 30/11/2020 17:09, Laurent Pinchart wrote:
> > On Mon, Nov 30, 2020 at 01:31:24PM +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.
> >>
> >> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> Changes since RFC v3:
> >>
> >> 	- Removed almost all global variables, dynamically allocated
> >> 	the cio2_bridge structure, plus a bunch of associated changes
> >> 	like 
> >> 	- Added a new function to ipu3-cio2-main.c to check for an 
> >> 	existing fwnode_graph before calling cio2_bridge_init()
> >> 	- Prefixed cio2_bridge_ to any variables and functions that
> >> 	lacked it
> >> 	- Assigned the new fwnode directly to the sensor's ACPI device
> >> 	fwnode as secondary. This removes the requirement to delay until
> >> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
> >> 	it has a side effect, which is that those devices then grab a ref
> >> 	to the new software_node. This effectively prevents us from
> >> 	unloading the driver, because we can't free the memory that they
> >> 	live in whilst the device holds a reference to them. The work
> >> 	around at the moment is to _not_ unregister the software_nodes
> >> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
> >> 	is simply skipped if the module is reloaded.
> >> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
> >> 	- Replaced ints with unsigned ints where appropriate
> >> 	- Iterated over all ACPI devices of a matching _HID rather than
> >> 	just the first to ensure we handle a device with multiple sensors
> >> 	of the same model.
> >>
> >>  MAINTAINERS                                   |   1 +
> >>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
> >>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
> >>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
> >>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
> >>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
> >>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
> >>  7 files changed, 421 insertions(+)
> >>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
> >>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 9702b886d6a4..188559a0a610 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
> >>  M:	Yong Zhi <yong.zhi@intel.com>
> >>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
> >>  M:	Bingbu Cao <bingbu.cao@intel.com>
> >> +M:	Dan Scally <djrscally@gmail.com>
> >>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
> >>  L:	linux-media@vger.kernel.org
> >>  S:	Maintained
> >> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> >> index 82d7f17e6a02..2b3350d042be 100644
> >> --- a/drivers/media/pci/intel/ipu3/Kconfig
> >> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> >> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
> >>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> >>  	  connected camera.
> >>  	  The module will be called ipu3-cio2.
> >> +
> >> +config CIO2_BRIDGE
> >> +	bool "IPU3 CIO2 Sensors Bridge"
> >> +	depends on VIDEO_IPU3_CIO2
> >> +	help
> >> +	  This extension provides an API for the ipu3-cio2 driver to create
> >> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
> >> +	  can be used to enable support for cameras in detachable / hybrid
> >> +	  devices that ship with Windows.
> >> +
> >> +	  Say Y here if your device is a detachable / hybrid laptop that comes
> >> +	  with Windows installed by the OEM, for example:
> >> +
> >> +	  	- Microsoft Surface models (except Surface Pro 3)
> >> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
> >> +		- Dell 7285
> >> +
> >> +	  If in doubt, say N here.
> >> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> >> index 429d516452e4..933777e6ea8a 100644
> >> --- a/drivers/media/pci/intel/ipu3/Makefile
> >> +++ b/drivers/media/pci/intel/ipu3/Makefile
> >> @@ -2,3 +2,4 @@
> >>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> >>  
> >>  ipu3-cio2-y += ipu3-cio2-main.o
> >> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> >> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> >> new file mode 100644
> >> index 000000000000..fd3f8ba07274
> >> --- /dev/null
> >> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> >> @@ -0,0 +1,260 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Author: Dan Scally <djrscally@gmail.com> */
> > 
> > Could you please add a blank line here ?
> 
> Yes
>
> >> +#include <linux/acpi.h>
> >> +#include <linux/device.h>
> >> +#include <linux/i2c.h>
> > 
> > Is this header needed ?
> > 
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> > 
> > And this one ?
> > 
> >> +#include <linux/pci.h>
> >> +#include <linux/property.h>
> >> +#include <media/v4l2-subdev.h>
> > 
> > And this one ?
> 
> Ah yes - bit sloppy, they're orphaned from earlier versions, sorry about
> that.
> 
> >> +
> >> +#include "cio2-bridge.h"
> >> +
> >> +/*
> >> + * Extend this array with ACPI Hardware ID's of devices known to be working.
> >> + * Do not add a HID for a sensor that is not actually supported.
> >> + */
> >> +static const char * const cio2_supported_devices[] = {
> > 
> > Maybe cio2_supported_sensors ?
> 
> Sure
> 
> >> +	"INT33BE",
> >> +	"OVTI2680",
> >> +};
> >> +
> >> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> >> +					void *data, u32 size)
> >> +{
> >> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> +	union acpi_object *obj;
> >> +	acpi_status status;
> >> +	int ret;
> >> +
> >> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> >> +	if (ACPI_FAILURE(status))
> >> +		return -ENODEV;
> >> +
> >> +	obj = buffer.pointer;
> >> +	if (!obj) {
> >> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	if (obj->type != ACPI_TYPE_BUFFER) {
> >> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
> >> +		ret = -ENODEV;
> >> +		goto out_free_buff;
> >> +	}
> >> +
> >> +	if (obj->buffer.length > size) {
> >> +		dev_err(&adev->dev, "Given buffer is too small\n");
> >> +		ret = -EINVAL;
> >> +		goto out_free_buff;
> >> +	}
> >> +
> >> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
> >> +	ret = obj->buffer.length;
> >> +
> >> +out_free_buff:
> >> +	kfree(buffer.pointer);
> >> +	return ret;
> >> +}
> >> +
> >> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> >> +{
> >> +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
> >> +	strcpy(sensor->prop_names.rotation, "rotation");
> >> +	strcpy(sensor->prop_names.bus_type, "bus-type");
> >> +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
> >> +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");
> > 
> > This is a bit fragile, as there's no len check. How about the following
> > ?
> > static const struct cio2_property_names prop_names = {
> > 	.clock_frequency = "clock-frequency",
> > 	.rotation = "rotation",
> > 	.bus_type = "bus-type",
> > 	.data_lanes = "data-lanes",
> > 	.remote_endpoint = "remote-endpoint",
> > };
> > 
> > static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> > {
> > 	sensor->prop_names = prop_names;
> > }
> > 
> > This shoudl generate a compilation warning if the string is too long.
> > 
> > You could even inline that line in
> > cio2_bridge_create_fwnode_properties().
> 
> Yes, I like that, thanks - I'll make the change.
> 
> >> +}
> >> +
> >> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
> >> +{
> >> +	unsigned int i;
> >> +
> >> +	cio2_bridge_init_property_names(sensor);
> >> +
> >> +	for (i = 0; i < 4; i++)
> >> +		sensor->data_lanes[i] = i + 1;
> > 
> > Is there no provision in the SSDB for data lane remapping ?
> 
> Sorry; don't follow what you mean by data lane remapping here.

Some CSI-2 receivers can remap data lanes. The routing inside the SoC
from the data lane input pins to the PHYs is configurable. This makes
board design easier as you can route the data lanes to any of the
inputs. That's why the data lanes DT property is a list of lane numbers
instead of a number of lanes. I'm actually not sure if the CIO2 supports
this.
 
> >> +
> >> +	/*
> >> +	 * Can't use PROPERTY_ENTRY_REF because it creates a new variable to
> >> +	 * point to, which doesn't survive the function.
> >> +	 */
> >> +	sensor->local_ref[0] = (struct software_node_ref_args){
> >> +		.node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT]
> >> +		};
> > 
> > I'd remove one tab here. Or just write
> > 
> > 	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
> 
> Yep, changed.
> 
> >> +	sensor->remote_ref[0] = (struct software_node_ref_args){
> >> +		.node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT]
> >> +		};
> >> +
> >> +	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> >> +						       sensor->ssdb.mclkspeed);
> >> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> >> +						      sensor->ssdb.degree);
> >> +
> >> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 5);
> >> +	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> >> +								sensor->data_lanes,
> >> +								sensor->ssdb.lanes);
> >> +	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> >> +							    sensor->local_ref);
> >> +
> >> +	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> >> +								  sensor->data_lanes,
> >> +								  sensor->ssdb.lanes);
> >> +	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> >> +							      sensor->remote_ref);
> >> +}
> >> +
> >> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> >> +{
> >> +	snprintf(sensor->node_names.remote_port, 6, "port%u", sensor->ssdb.link);
> >> +	strcpy(sensor->node_names.port, "port0");
> >> +	strcpy(sensor->node_names.endpoint, "endpoint0");
> >> +}
> >> +
> >> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> >> +						  struct cio2_sensor *sensor)
> >> +{
> >> +	struct software_node *nodes = sensor->swnodes;
> >> +
> >> +	cio2_bridge_init_swnode_names(sensor);
> >> +
> >> +	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> >> +					       sensor->dev_properties);
> >> +	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> >> +					      &nodes[SWNODE_SENSOR_HID]);
> >> +	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> >> +						      &nodes[SWNODE_SENSOR_PORT],
> >> +						      sensor->ep_properties);
> >> +	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> >> +					    &bridge->cio2_hid_node);
> >> +	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> >> +						    &nodes[SWNODE_CIO2_PORT],
> >> +						    sensor->cio2_properties);
> >> +}
> >> +
> >> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> >> +{
> >> +	struct cio2_sensor *sensor;
> >> +	unsigned int i;
> >> +
> >> +	for (i = 0; i < bridge->n_sensors; i++) {
> >> +		sensor = &bridge->sensors[i];
> >> +		software_node_unregister_nodes(sensor->swnodes);
> >> +		acpi_dev_put(sensor->adev);
> >> +	}
> >> +}
> >> +
> >> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge)
> >> +{
> >> +	struct fwnode_handle *fwnode;
> >> +	struct cio2_sensor *sensor;
> >> +	struct acpi_device *adev;
> >> +	unsigned int i;
> >> +	int ret = 0;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_devices); i++) {
> >> +		const char *this_device = cio2_supported_devices[i];
> > 
> > s/this_device/name/ (or sensor_name, ...) ?
> 
> I went for hid as Andy suggested.
>
> > 
> >> +
> >> +		for_each_acpi_dev_match(adev, this_device, NULL, -1) {
> >> +			if (!adev || !(adev->status.present && adev->status.enabled))
> > 
> > 			if (!adev || !adev->status.present || !adev->status.enabled))
> > 
> > may be a bit more readable. Does for_each_acpi_dev_match() return NULL
> > devices though ? If no, you could drop the !adev check. You may also be
> > able to drop the !present check, as I don't think ACPI allows !present
> > && enabled.
> 
> You're right, the spec mandates enabled be 0 if present is 0. The
> iterator will return NULL when the previous return value was the last
> matching device, so that part needs to stay, but it can become:
> 
> if (!adev || !adev->status.enabled)

As you've commented out in a reply, the loop will stop when adev is NULL
:-)

> >> +				continue;
> >> +
> >> +			sensor = &bridge->sensors[bridge->n_sensors];
> >> +			sensor->adev = adev;
> >> +			strscpy(sensor->name, this_device, sizeof(sensor->name));
> >> +
> >> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> >> +							   &sensor->ssdb,
> >> +							   sizeof(sensor->ssdb));
> >> +			if (ret < 0)
> >> +				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;
> >> +			}
> >> +
> >> +			cio2_bridge_create_fwnode_properties(sensor);
> >> +			cio2_bridge_create_connection_swnodes(bridge, sensor);
> >> +
> >> +			ret = software_node_register_nodes(sensor->swnodes);
> >> +			if (ret)
> >> +				goto err_put_adev;
> >> +
> >> +			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> >> +			if (!fwnode) {
> >> +				ret = -ENODEV;
> >> +				goto err_free_swnodes;
> >> +			}
> >> +
> >> +			adev->fwnode.secondary = fwnode;
> >> +
> >> +			dev_info(&bridge->cio2->dev,
> >> +				 "Found supported sensor %s\n",
> >> +				 acpi_dev_name(adev));
> >> +
> >> +			bridge->n_sensors++;
> > 
> > We probably want a check here to avoid overflowing bridge->sensors. The
> > other option is to make bridge->sensors a struct list_head and allocate
> > sensors dynamically.
> 
> Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
> the maximum. Seems easier to just do a check, unless the wasted memory
> is enough that it's worth allocating dynamically. I don't mind either
> approach.

In theory we could route multiple sensors to the same receiver, as long
as only one of them drives the lanes at any given time. It's one way to
support multiple sensors in cheap designs. I doubt we'll ever encounter
that with the IPU3, so we could just limit the count to 4.

> >> +		}
> >> +	}
> >> +
> >> +	return ret;
> >> +
> >> +err_free_swnodes:
> >> +	software_node_unregister_nodes(sensor->swnodes);
> >> +err_put_adev:
> >> +	acpi_dev_put(sensor->adev);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +int cio2_bridge_init(struct pci_dev *cio2)
> >> +{
> >> +	struct device *dev = &cio2->dev;
> >> +	struct fwnode_handle *fwnode;
> >> +	struct cio2_bridge *bridge;
> >> +	int ret;
> >> +
> >> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> >> +	if (!bridge)
> >> +		return -ENOMEM;
> >> +
> >> +	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> >> +	bridge->cio2_hid_node = (const struct software_node){ bridge->cio2_node_name };
> > 
> > Maybe just
> > 
> > 	bridge->cio2_hid_node.name = bridge->cio2_node_name;
> > 
> > as the rest is already zeroed by the kzalloc() call ?
> > 
> >> +	bridge->cio2 = pci_dev_get(cio2);
> > 
> > As the cio2 pointer is only used to print a message in
> > cio2_bridge_connect_sensors(), do we need to store it in the bridge
> > structure, and take a reference to the device ?
> > 
> >> +
> >> +	ret = software_node_register(&bridge->cio2_hid_node);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "Failed to register the CIO2 HID node\n");
> >> +		goto err_put_cio2;
> >> +	}
> >> +
> >> +	ret = cio2_bridge_connect_sensors(bridge);
> >> +	if (ret || bridge->n_sensors == 0)
> >> +		goto err_unregister_cio2;
> >> +
> >> +	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> >> +
> >> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> >> +	if (!fwnode) {
> >> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> >> +		ret = -ENODEV;
> >> +		goto err_unregister_sensors;
> > 
> > Can this happen ?
> 
> It _shouldn't_ happen, as long as nothing else is touching the swnodes
> I've registered or anything. I've never seen it happen. That didn't feel
> like quite enough to say it can't ever happen - but I'm happy to skip
> the check if you think thats ok.

It seems a bit overkill to me, but I'm not a swnode specialist :-)

> >> +	}
> >> +
> >> +	set_secondary_fwnode(dev, fwnode);
> >> +
> >> +	return 0;
> >> +
> >> +err_unregister_sensors:
> >> +	cio2_bridge_unregister_sensors(bridge);
> >> +err_unregister_cio2:
> >> +	software_node_unregister(&bridge->cio2_hid_node);
> >> +err_put_cio2:
> >> +	pci_dev_put(bridge->cio2);
> >> +
> >> +	kfree(bridge);
> >> +	return ret;
> >> +}
> >> 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..96f5c8a12be0
> >> --- /dev/null
> >> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> > 
> > This file is only included by cio2-bridge.c, so you could inline it
> > there. Up to you.
> 
> I think I like them separate
> 
> >> @@ -0,0 +1,108 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Author: Dan Scally <djrscally@gmail.com> */
> >> +#ifndef __CIO2_BRIDGE_H
> >> +#define __CIO2_BRIDGE_H
> >> +
> >> +#include <linux/property.h>
> >> +
> >> +#define CIO2_HID				"INT343E"
> >> +#define CIO2_NUM_PORTS			  4
> > 
> > There are a few rogue spaces before '4'.
> 
> Argh, thanks, this is the curse of using VS code on multiple machines...

I recommend vim ;-)

> >> +
> >> +#define NODE_SENSOR(_HID, _PROPS)		\
> >> +	((const struct software_node) {		\
> >> +		.name = _HID,			\
> >> +		.properties = _PROPS,		\
> >> +	})
> >> +
> >> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
> >> +	((const struct software_node) {		\
> >> +		_PORT,				\
> >> +		_SENSOR_NODE,			\
> >> +	})
> >> +
> >> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> >> +	((const struct software_node) {		\
> >> +		_EP,				\
> >> +		_PORT,				\
> >> +		_PROPS,				\
> >> +	})
> >> +
> >> +enum cio2_sensor_swnodes {
> >> +	SWNODE_SENSOR_HID,
> >> +	SWNODE_SENSOR_PORT,
> >> +	SWNODE_SENSOR_ENDPOINT,
> >> +	SWNODE_CIO2_PORT,
> >> +	SWNODE_CIO2_ENDPOINT,
> >> +	NR_OF_SENSOR_SWNODES
> >> +};
> >> +
> >> +/* Data representation as it is in ACPI SSDB buffer */
> >> +struct cio2_sensor_ssdb {
> >> +	u8 version;
> >> +	u8 sku;
> >> +	u8 guid_csi2[16];
> >> +	u8 devfunction;
> >> +	u8 bus;
> >> +	u32 dphylinkenfuses;
> >> +	u32 clockdiv;
> >> +	u8 link;
> >> +	u8 lanes;
> >> +	u32 csiparams[10];
> >> +	u32 maxlanespeed;
> >> +	u8 sensorcalibfileidx;
> >> +	u8 sensorcalibfileidxInMBZ[3];
> >> +	u8 romtype;
> >> +	u8 vcmtype;
> >> +	u8 platforminfo;
> >> +	u8 platformsubinfo;
> >> +	u8 flash;
> >> +	u8 privacyled;
> >> +	u8 degree;
> >> +	u8 mipilinkdefined;
> >> +	u32 mclkspeed;
> >> +	u8 controllogicid;
> >> +	u8 reserved1[3];
> >> +	u8 mclkport;
> >> +	u8 reserved2[13];
> >> +} __packed__;
> >> +
> >> +struct cio2_property_names {
> >> +	char clock_frequency[16];
> >> +	char rotation[9];
> >> +	char bus_type[9];
> >> +	char data_lanes[11];
> >> +	char remote_endpoint[16];
> >> +};
> >> +
> >> +struct cio2_node_names {
> >> +	char port[6];
> >> +	char endpoint[10];
> >> +	char remote_port[6];
> >> +};
> >> +
> >> +struct cio2_sensor {
> >> +	char name[ACPI_ID_LEN];
> >> +	struct acpi_device *adev;
> >> +
> >> +	struct software_node swnodes[6];
> >> +	struct cio2_node_names node_names;
> >> +
> >> +	u32 data_lanes[4];
> >> +	struct cio2_sensor_ssdb ssdb;
> >> +	struct cio2_property_names prop_names;
> >> +	struct property_entry ep_properties[4];
> >> +	struct property_entry dev_properties[3];
> >> +	struct property_entry cio2_properties[3];
> >> +	struct software_node_ref_args local_ref[1];
> >> +	struct software_node_ref_args remote_ref[1];
> >> +};
> >> +
> >> +struct cio2_bridge {
> >> +	struct pci_dev *cio2;
> >> +	char cio2_node_name[ACPI_ID_LEN];
> >> +	struct software_node cio2_hid_node;
> >> +	unsigned int n_sensors;
> >> +	struct cio2_sensor sensors[CIO2_NUM_PORTS];
> >> +};
> >> +
> >> +#endif
> >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> index 36e354ecf71e..0d69b593e9f0 100644
> >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> @@ -1702,6 +1702,22 @@ static void cio2_queues_exit(struct cio2_device *cio2)
> >>  		cio2_queue_exit(cio2, &cio2->queue[i]);
> >>  }
> >>  
> >> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> >> +{
> >> +	struct fwnode_handle *endpoint;
> >> +
> >> +	if (IS_ERR_OR_NULL(fwnode))
> >> +		return false;
> >> +
> >> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> >> +	if (endpoint) {
> >> +		fwnode_handle_put(endpoint);
> >> +		return true;
> >> +	}
> >> +
> >> +	return cio2_check_fwnode_graph(fwnode->secondary);
> > 
> > If we have a fwnode->secondary and this check fails there's something
> > seriously wrong, I wonder if we should print an error message.
> 
> Yes, probably a good thought, since nothing will work in that case. I'll
> add something appropriate.
> 
> > Overall this is nice. I think the next version will get my ack :-)
> 
> Excellent :)
> 
> >> +}
> >> +
> >>  /**************** PCI interface ****************/
> >>  
> >>  static int cio2_pci_probe(struct pci_dev *pci_dev,
> >> @@ -1715,6 +1731,17 @@ 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(dev_fwnode(&pci_dev->dev))) {
> >> +		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 ccf0b85ae36f..520a27c9cdad 100644
> >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> >> @@ -437,4 +437,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; }
> >> +#endif
> >> +
> >>  #endif

Hi Laurent
On 01/12/2020 22:30, Laurent Pinchart wrote:
>>>> +}
>>>> +
>>>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	cio2_bridge_init_property_names(sensor);
>>>> +
>>>> +	for (i = 0; i < 4; i++)
>>>> +		sensor->data_lanes[i] = i + 1;
>>> Is there no provision in the SSDB for data lane remapping ?
>> Sorry; don't follow what you mean by data lane remapping here.
> Some CSI-2 receivers can remap data lanes. The routing inside the SoC
> from the data lane input pins to the PHYs is configurable. This makes
> board design easier as you can route the data lanes to any of the
> inputs. That's why the data lanes DT property is a list of lane numbers
> instead of a number of lanes. I'm actually not sure if the CIO2 supports
> this.

I don't see anything in the SSDB that might refer to that, though of
course we're lacking documentation for it so it could be a part that we
don't understand yet.


>>>> +			dev_info(&bridge->cio2->dev,
>>>> +				 "Found supported sensor %s\n",
>>>> +				 acpi_dev_name(adev));
>>>> +
>>>> +			bridge->n_sensors++;
>>> We probably want a check here to avoid overflowing bridge->sensors. The
>>> other option is to make bridge->sensors a struct list_head and allocate
>>> sensors dynamically.
>> Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
>> the maximum. Seems easier to just do a check, unless the wasted memory
>> is enough that it's worth allocating dynamically. I don't mind either
>> approach.
> In theory we could route multiple sensors to the same receiver, as long
> as only one of them drives the lanes at any given time. It's one way to
> support multiple sensors in cheap designs. I doubt we'll ever encounter
> that with the IPU3, so we could just limit the count to 4.
Ah, that's neat though. But I'll leave it at a check at the top of the
loop for now.
>>>> +
>>>> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
>>>> +	if (!fwnode) {
>>>> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
>>>> +		ret = -ENODEV;
>>>> +		goto err_unregister_sensors;
>>> Can this happen ?
>> It _shouldn't_ happen, as long as nothing else is touching the swnodes
>> I've registered or anything. I've never seen it happen. That didn't feel
>> like quite enough to say it can't ever happen - but I'm happy to skip
>> the check if you think thats ok.
> It seems a bit overkill to me, but I'm not a swnode specialist :-)
I'm going to keep it, if you have no strong feelings, partly through
caution but also because the other place swnodes are most heavily used
(drivers/platform/x86/intel_cht_int33fe_typec.c) _does_ perform the
check, so consistency too.
>>>> @@ -0,0 +1,108 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Author: Dan Scally <djrscally@gmail.com> */
>>>> +#ifndef __CIO2_BRIDGE_H
>>>> +#define __CIO2_BRIDGE_H
>>>> +
>>>> +#include <linux/property.h>
>>>> +
>>>> +#define CIO2_HID				"INT343E"
>>>> +#define CIO2_NUM_PORTS			  4
>>> There are a few rogue spaces before '4'.
>> Argh, thanks, this is the curse of using VS code on multiple machines...
> I recommend vim ;-)
You're not the only one - maybe I need to spend the time and it'll save
time in the future

Hi Andy

On 30/11/2020 17:45, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
>> Software nodes that are children of another software node should be
>> unregistered before their parent. To allow easy unregistering of an array
>> of software_nodes ordered parent to child, reverse the order in which
>> this function unregisters software_nodes.
> 
> Should be folded in the previous patch. Otherwise we will have a history point
> where register() behaves differently to unregister().

OK sure, I'll squash them - and thanks for your comments on the previous
patch, I condensed the conditionals as you suggest

> ...
> 
>> + * @nodes: Zero terminated array of software nodes to be unregistered. If
>> + * parent pointers are set up in any of the software nodes then the array
>> + * MUST be ordered such that parents come before their children.
> 
> Please, leave field description short. Rather add another note to the
> Description below.

Ack

On 30/11/2020 17:47, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 06:17:16PM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> Thank you for the patch.
>>
>> The subject line is very long. We try to keep it within a 72 characters
>> limit in the kernel. That can be a challenge sometimes, and expections
>> can be accepted, but this one is reaaaally long.
>>
>> (The same comment holds for other patches in the series)
> 
> +1.

My bad; I'll go through the series and condense them down as much as
possible.

>> On Mon, Nov 30, 2020 at 01:31:17PM +0000, Daniel Scally wrote:
>>> To maintain consistency with software_node_unregister_nodes(), reverse
>>> the order in which the software_node_unregister_node_group() function
>>> unregisters nodes.
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>
>> I"d squash this with the previous patch to avoid introducing an
>> inconsistency.
> 
> It's different to previous. It touches not complementary API, but different
> one. However, I would follow your comment about documenting the behaviour of
> these two APIs as well…

I'll update the documentation for this function too.

On 02/12/2020 10:38, Sakari Ailus wrote:
> Hi Laurent,
>
> On Wed, Dec 02, 2020 at 12:30:53AM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Tue, Dec 01, 2020 at 10:08:25PM +0000, Dan Scally wrote:
>>> On 30/11/2020 17:09, Laurent Pinchart wrote:
>>>> On Mon, Nov 30, 2020 at 01:31:24PM +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.
>>>>>
>>>>> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>>> ---
>>>>> Changes since RFC v3:
>>>>>
>>>>> 	- Removed almost all global variables, dynamically allocated
>>>>> 	the cio2_bridge structure, plus a bunch of associated changes
>>>>> 	like 
>>>>> 	- Added a new function to ipu3-cio2-main.c to check for an 
>>>>> 	existing fwnode_graph before calling cio2_bridge_init()
>>>>> 	- Prefixed cio2_bridge_ to any variables and functions that
>>>>> 	lacked it
>>>>> 	- Assigned the new fwnode directly to the sensor's ACPI device
>>>>> 	fwnode as secondary. This removes the requirement to delay until
>>>>> 	the I2C devices are instantiated before ipu3-cio2 can probe, but
>>>>> 	it has a side effect, which is that those devices then grab a ref
>>>>> 	to the new software_node. This effectively prevents us from
>>>>> 	unloading the driver, because we can't free the memory that they
>>>>> 	live in whilst the device holds a reference to them. The work
>>>>> 	around at the moment is to _not_ unregister the software_nodes
>>>>> 	when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
>>>>> 	is simply skipped if the module is reloaded.
>>>>> 	- Moved the sensor's SSDB struct to be a member of cio2_sensor
>>>>> 	- Replaced ints with unsigned ints where appropriate
>>>>> 	- Iterated over all ACPI devices of a matching _HID rather than
>>>>> 	just the first to ensure we handle a device with multiple sensors
>>>>> 	of the same model.
>>>>>
>>>>>  MAINTAINERS                                   |   1 +
>>>>>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>>>>>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>>>>>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 260 ++++++++++++++++++
>>>>>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 108 ++++++++
>>>>>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  27 ++
>>>>>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>>>>>  7 files changed, 421 insertions(+)
>>>>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 9702b886d6a4..188559a0a610 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -8927,6 +8927,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>>>>>  M:	Yong Zhi <yong.zhi@intel.com>
>>>>>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>  M:	Bingbu Cao <bingbu.cao@intel.com>
>>>>> +M:	Dan Scally <djrscally@gmail.com>
>>>>>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>>>>>  L:	linux-media@vger.kernel.org
>>>>>  S:	Maintained
>>>>> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
>>>>> index 82d7f17e6a02..2b3350d042be 100644
>>>>> --- a/drivers/media/pci/intel/ipu3/Kconfig
>>>>> +++ b/drivers/media/pci/intel/ipu3/Kconfig
>>>>> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>>>>>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>>>>>  	  connected camera.
>>>>>  	  The module will be called ipu3-cio2.
>>>>> +
>>>>> +config CIO2_BRIDGE
>>>>> +	bool "IPU3 CIO2 Sensors Bridge"
>>>>> +	depends on VIDEO_IPU3_CIO2
>>>>> +	help
>>>>> +	  This extension provides an API for the ipu3-cio2 driver to create
>>>>> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
>>>>> +	  can be used to enable support for cameras in detachable / hybrid
>>>>> +	  devices that ship with Windows.
>>>>> +
>>>>> +	  Say Y here if your device is a detachable / hybrid laptop that comes
>>>>> +	  with Windows installed by the OEM, for example:
>>>>> +
>>>>> +	  	- Microsoft Surface models (except Surface Pro 3)
>>>>> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
>>>>> +		- Dell 7285
>>>>> +
>>>>> +	  If in doubt, say N here.
>>>>> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
>>>>> index 429d516452e4..933777e6ea8a 100644
>>>>> --- a/drivers/media/pci/intel/ipu3/Makefile
>>>>> +++ b/drivers/media/pci/intel/ipu3/Makefile
>>>>> @@ -2,3 +2,4 @@
>>>>>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>>>>>  
>>>>>  ipu3-cio2-y += ipu3-cio2-main.o
>>>>> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
>>>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>> new file mode 100644
>>>>> index 000000000000..fd3f8ba07274
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
>>>>> @@ -0,0 +1,260 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/* Author: Dan Scally <djrscally@gmail.com> */
>>>> Could you please add a blank line here ?
>>> Yes
>>>
>>>>> +#include <linux/acpi.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/i2c.h>
>>>> Is this header needed ?
>>>>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>> And this one ?
>>>>
>>>>> +#include <linux/pci.h>
>>>>> +#include <linux/property.h>
>>>>> +#include <media/v4l2-subdev.h>
>>>> And this one ?
>>> Ah yes - bit sloppy, they're orphaned from earlier versions, sorry about
>>> that.
>>>
>>>>> +
>>>>> +#include "cio2-bridge.h"
>>>>> +
>>>>> +/*
>>>>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>>>>> + * Do not add a HID for a sensor that is not actually supported.
>>>>> + */
>>>>> +static const char * const cio2_supported_devices[] = {
>>>> Maybe cio2_supported_sensors ?
>>> Sure
>>>
>>>>> +	"INT33BE",
>>>>> +	"OVTI2680",
>>>>> +};
>>>>> +
>>>>> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
>>>>> +					void *data, u32 size)
>>>>> +{
>>>>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>>>>> +	union acpi_object *obj;
>>>>> +	acpi_status status;
>>>>> +	int ret;
>>>>> +
>>>>> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
>>>>> +	if (ACPI_FAILURE(status))
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	obj = buffer.pointer;
>>>>> +	if (!obj) {
>>>>> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +
>>>>> +	if (obj->type != ACPI_TYPE_BUFFER) {
>>>>> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
>>>>> +		ret = -ENODEV;
>>>>> +		goto out_free_buff;
>>>>> +	}
>>>>> +
>>>>> +	if (obj->buffer.length > size) {
>>>>> +		dev_err(&adev->dev, "Given buffer is too small\n");
>>>>> +		ret = -EINVAL;
>>>>> +		goto out_free_buff;
>>>>> +	}
>>>>> +
>>>>> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
>>>>> +	ret = obj->buffer.length;
>>>>> +
>>>>> +out_free_buff:
>>>>> +	kfree(buffer.pointer);
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>>>>> +{
>>>>> +	strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
>>>>> +	strcpy(sensor->prop_names.rotation, "rotation");
>>>>> +	strcpy(sensor->prop_names.bus_type, "bus-type");
>>>>> +	strcpy(sensor->prop_names.data_lanes, "data-lanes");
>>>>> +	strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");
>>>> This is a bit fragile, as there's no len check. How about the following
>>>> ?
>>>> static const struct cio2_property_names prop_names = {
>>>> 	.clock_frequency = "clock-frequency",
>>>> 	.rotation = "rotation",
>>>> 	.bus_type = "bus-type",
>>>> 	.data_lanes = "data-lanes",
>>>> 	.remote_endpoint = "remote-endpoint",
>>>> };
>>>>
>>>> static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>>>> {
>>>> 	sensor->prop_names = prop_names;
>>>> }
>>>>
>>>> This shoudl generate a compilation warning if the string is too long.
>>>>
>>>> You could even inline that line in
>>>> cio2_bridge_create_fwnode_properties().
>>> Yes, I like that, thanks - I'll make the change.
>>>
>>>>> +}
>>>>> +
>>>>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
>>>>> +{
>>>>> +	unsigned int i;
>>>>> +
>>>>> +	cio2_bridge_init_property_names(sensor);
>>>>> +
>>>>> +	for (i = 0; i < 4; i++)
>>>>> +		sensor->data_lanes[i] = i + 1;
>>>> Is there no provision in the SSDB for data lane remapping ?
>>> Sorry; don't follow what you mean by data lane remapping here.
>> Some CSI-2 receivers can remap data lanes. The routing inside the SoC
>> from the data lane input pins to the PHYs is configurable. This makes
>> board design easier as you can route the data lanes to any of the
>> inputs. That's why the data lanes DT property is a list of lane numbers
>> instead of a number of lanes. I'm actually not sure if the CIO2 supports
>> this.
> To my knowledge it does not. Only the number of lanes allocated to
> different ports matters.
>
So nothing to change here then I think?
>>>>> @@ -0,0 +1,108 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/* Author: Dan Scally <djrscally@gmail.com> */
>>>>> +#ifndef __CIO2_BRIDGE_H
>>>>> +#define __CIO2_BRIDGE_H
>>>>> +
>>>>> +#include <linux/property.h>
>>>>> +
>>>>> +#define CIO2_HID				"INT343E"
>>>>> +#define CIO2_NUM_PORTS			  4
>>>> There are a few rogue spaces before '4'.
>>> Argh, thanks, this is the curse of using VS code on multiple machines...
>> I recommend vim ;-)
> What is VS code? Very Serious Code?

Visual Studio Code - it has some nice features, but the
facepalm-to-productivity ratio is a bit high.

> I can recommend Emacs; that could help, too.

On Wed, Dec 02, 2020 at 12:38:51PM +0200, Sakari Ailus wrote:
> On Wed, Dec 02, 2020 at 12:30:53AM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 01, 2020 at 10:08:25PM +0000, Dan Scally wrote:
> > > On 30/11/2020 17:09, Laurent Pinchart wrote:
> > > > On Mon, Nov 30, 2020 at 01:31:24PM +0000, Daniel Scally wrote:

...

> > > >> +#define CIO2_NUM_PORTS			  4
> > > > 
> > > > There are a few rogue spaces before '4'.
> > > 
> > > Argh, thanks, this is the curse of using VS code on multiple machines...
> > 
> > I recommend vim ;-)
> 
> What is VS code? Very Serious Code?

Visual Studio Code. Something good from MS (no sarcasm, btw).

> I can recommend Emacs; that could help, too.

On 02/12/2020 12:02, Andy Shevchenko wrote:
> On Wed, Dec 02, 2020 at 10:53:05AM +0000, Dan Scally wrote:
>> On 02/12/2020 10:38, Sakari Ailus wrote:
> 
> ...
> 
>>>>> Argh, thanks, this is the curse of using VS code on multiple machines...
>>>> I recommend vim ;-)
>>> What is VS code? Very Serious Code?
>>
>> Visual Studio Code - it has some nice features, but the
>> facepalm-to-productivity ratio is a bit high.
> 
> Perhaps you can submit an issue report on GitHub. I found VS Code pretty nice
> to be with Linux kernel development.

Yeah I like it too; it's the one I've stuck with despite the annoyances
I find. It has some super handy features for someone who doesn't know
most of the kernel APIs very well yet. Writing up the issues is on my
to-do list but I hate to do it without at least putting some effort into
figuring out what the problem is and I so far didn't get round to that yet

Morning Sakari

On 30/11/2020 20:35, Sakari Ailus wrote:
>> +/*
>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>> + * Do not add a HID for a sensor that is not actually supported.
>> + */
>> +static const char * const cio2_supported_devices[] = {
>> +	"INT33BE",
>> +	"OVTI2680",
> 
> I guess we don't have the known-good frequencies for the CSI-2 bus in
> firmware?
> 
> One option would be to put there what the drivers currently use. This
> assumes the support for these devices is, well, somewhat opportunistic but
> I guess there's no way around that right now at least.
> 
> As the systems are laptops, they're likely somewhat less prone to EMI
> issues to begin with than mobile phones anyway.

Just looking at this; we're currently using this with the ov2680 driver
that's in mainline currently (with very minor tweaks) plus a
hacked-into-roughly-working version of the atomisp-ov5693 driver (ACPI
ID INT33BE = ov5693 physical device). Neither of those drivers lists any
link frequencies, nor provides a link frequency control for v4l2 to work
with.

On the other hand, the ov5648 [1] and ov8865 [2] drivers which Paul has
submitted recently, which we also want to be able to support, _do_
include that. I can register the frequencies Paul's defined there as a
link-frequencies property but this gives rise to two questions:


1. Is this _mandatory_? Do I need to be finding the link-frequencies for
the OV2680 and OV5693 drivers too? Or can I skip that property where the
driver doesn't handle it anyway. Seems to be working fine without
controlling it in driver.
2. Can I trust all the values in the drivers to be available on each
platform? For example for the ov5648 Paul lists these as available:

 938static const s64 ov5648_link_freq_menu[] = {


 939        210000000,


 940        168000000,


 941};

But can I safely register a link-frequencies property for both of those
and trust that that'll work on all IPU3 platforms with an ov5648 in them?

Thanks
Dan

On 15/12/2020 10:28, Daniel Scally wrote:
> Morning Sakari
>
> On 30/11/2020 20:35, Sakari Ailus wrote:
>>> +/*
>>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>>> + * Do not add a HID for a sensor that is not actually supported.
>>> + */
>>> +static const char * const cio2_supported_devices[] = {
>>> +	"INT33BE",
>>> +	"OVTI2680",
>> I guess we don't have the known-good frequencies for the CSI-2 bus in
>> firmware?
>>
>> One option would be to put there what the drivers currently use. This
>> assumes the support for these devices is, well, somewhat opportunistic but
>> I guess there's no way around that right now at least.
>>
>> As the systems are laptops, they're likely somewhat less prone to EMI
>> issues to begin with than mobile phones anyway.
> Just looking at this; we're currently using this with the ov2680 driver
> that's in mainline currently (with very minor tweaks) plus a
> hacked-into-roughly-working version of the atomisp-ov5693 driver (ACPI
> ID INT33BE = ov5693 physical device). Neither of those drivers lists any
> link frequencies, nor provides a link frequency control for v4l2 to work
> with.
>
> On the other hand, the ov5648 [1] and ov8865 [2] drivers which Paul has
> submitted recently


Forgot to actually link these:


[1]
https://lore.kernel.org/linux-media/20201211154027.153535-1-paul.kocialkowski@bootlin.com/T/#m5eb18611b7df1538ed4924422583b62cc61dbfae

[2]
https://lore.kernel.org/linux-media/20201211154428.153762-1-paul.kocialkowski@bootlin.com/T/#m6d4fd5e590b1c4583d4a74f5ae938ea011408640

Hi Daniel,

On Tue, Dec 15, 2020 at 10:28:59AM +0000, Daniel Scally wrote:
> Morning Sakari

> 

> On 30/11/2020 20:35, Sakari Ailus wrote:

> >> +/*

> >> + * Extend this array with ACPI Hardware ID's of devices known to be working.

> >> + * Do not add a HID for a sensor that is not actually supported.

> >> + */

> >> +static const char * const cio2_supported_devices[] = {

> >> +	"INT33BE",

> >> +	"OVTI2680",

> > 

> > I guess we don't have the known-good frequencies for the CSI-2 bus in

> > firmware?

> > 

> > One option would be to put there what the drivers currently use. This

> > assumes the support for these devices is, well, somewhat opportunistic but

> > I guess there's no way around that right now at least.

> > 

> > As the systems are laptops, they're likely somewhat less prone to EMI

> > issues to begin with than mobile phones anyway.

> 

> Just looking at this; we're currently using this with the ov2680 driver

> that's in mainline currently (with very minor tweaks) plus a

> hacked-into-roughly-working version of the atomisp-ov5693 driver (ACPI

> ID INT33BE = ov5693 physical device). Neither of those drivers lists any

> link frequencies, nor provides a link frequency control for v4l2 to work

> with.

> 

> On the other hand, the ov5648 [1] and ov8865 [2] drivers which Paul has

> submitted recently, which we also want to be able to support, _do_

> include that. I can register the frequencies Paul's defined there as a

> link-frequencies property but this gives rise to two questions:

> 

> 

> 1. Is this _mandatory_? Do I need to be finding the link-frequencies for

> the OV2680 and OV5693 drivers too? Or can I skip that property where the

> driver doesn't handle it anyway. Seems to be working fine without

> controlling it in driver.


Receiver drivers generally need the information to program the receiver
timing. It may work for you without using the correct frequency, but the
risk of failure on another unit increases.

> 2. Can I trust all the values in the drivers to be available on each

> platform? For example for the ov5648 Paul lists these as available:

> 

>  938static const s64 ov5648_link_freq_menu[] = {

> 

> 

>  939        210000000,

> 

> 

>  940        168000000,

> 

> 

>  941};

> 

> But can I safely register a link-frequencies property for both of those

> and trust that that'll work on all IPU3 platforms with an ov5648 in them?


Ideally we'd know which frequency Windows uses, and use the same.

Using another frequency may have adverse effects elsewhere in the system.
AFAIU mostly this concerns radios of all sorts.

Now that this is in the kernel in any case, it can be fixed later on so I'm
not too worried about it. Having still a comment there that the
configuration is opportunistic would be nice.

-- 
Kind regards,

Kind regards,

Sakari Ailus

Hi Sakari - sorry for delayed reply. I didn't get this email actually,
just spotted it on the newsgroup by chance.

On 15/12/2020 22:02, Sakari Ailus wrote:
> Hi Daniel,
> 
> On Tue, Dec 15, 2020 at 10:28:59AM +0000, Daniel Scally wrote:
>> Morning Sakari
>>
>> On 30/11/2020 20:35, Sakari Ailus wrote:
>>>> +/*
>>>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
>>>> + * Do not add a HID for a sensor that is not actually supported.
>>>> + */
>>>> +static const char * const cio2_supported_devices[] = {
>>>> +	"INT33BE",
>>>> +	"OVTI2680",
>>>
>>> I guess we don't have the known-good frequencies for the CSI-2 bus in
>>> firmware?
>>>
>>> One option would be to put there what the drivers currently use. This
>>> assumes the support for these devices is, well, somewhat opportunistic but
>>> I guess there's no way around that right now at least.
>>>
>>> As the systems are laptops, they're likely somewhat less prone to EMI
>>> issues to begin with than mobile phones anyway.
>>
>> Just looking at this; we're currently using this with the ov2680 driver
>> that's in mainline currently (with very minor tweaks) plus a
>> hacked-into-roughly-working version of the atomisp-ov5693 driver (ACPI
>> ID INT33BE = ov5693 physical device). Neither of those drivers lists any
>> link frequencies, nor provides a link frequency control for v4l2 to work
>> with.
>>
>> On the other hand, the ov5648 [1] and ov8865 [2] drivers which Paul has
>> submitted recently, which we also want to be able to support, _do_
>> include that. I can register the frequencies Paul's defined there as a
>> link-frequencies property but this gives rise to two questions:
>>
>>
>> 1. Is this _mandatory_? Do I need to be finding the link-frequencies for
>> the OV2680 and OV5693 drivers too? Or can I skip that property where the
>> driver doesn't handle it anyway. Seems to be working fine without
>> controlling it in driver.
> 
> Receiver drivers generally need the information to program the receiver
> timing. It may work for you without using the correct frequency, but the
> risk of failure on another unit increases.

Hmm, ok. I'll see if I can find the correct values then to add to the
existing drivers.

>> 2. Can I trust all the values in the drivers to be available on each
>> platform? For example for the ov5648 Paul lists these as available:
>>
>>  938static const s64 ov5648_link_freq_menu[] = {
>>
>>
>>  939        210000000,
>>
>>
>>  940        168000000,
>>
>>
>>  941};
>>
>> But can I safely register a link-frequencies property for both of those
>> and trust that that'll work on all IPU3 platforms with an ov5648 in them?
> 
> Ideally we'd know which frequency Windows uses, and use the same.
> 
> Using another frequency may have adverse effects elsewhere in the system.
> AFAIU mostly this concerns radios of all sorts.
> 
> Now that this is in the kernel in any case, it can be fixed later on so I'm
> not too worried about it. Having still a comment there that the
> configuration is opportunistic would be nice.
> 

Understood - I'll add in the ability to add the link-frequencies plus a
comment explaining.

Thanks
Dan