Message-ID: <20201201223053.GB4569@pendragon.ideasonboard.com>
Date:   Wed, 2 Dec 2020 00:30:53 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Dan Scally <djrscally@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-media@...r.kernel.org, devel@...ica.org, rjw@...ysocki.net,
        lenb@...nel.org, gregkh@...uxfoundation.org,
        mika.westerberg@...ux.intel.com, andriy.shevchenko@...ux.intel.com,
        linus.walleij@...aro.org, bgolaszewski@...libre.com,
        wsa@...nel.org, yong.zhi@...el.com, sakari.ailus@...ux.intel.com,
        bingbu.cao@...el.com, tian.shu.qiu@...el.com, mchehab@...nel.org,
        robert.moore@...el.com, erik.kaneda@...el.com, pmladek@...e.com,
        rostedt@...dmis.org, sergey.senozhatsky@...il.com,
        linux@...musvillemoes.dk, kieran.bingham+renesas@...asonboard.com,
        jacopo+renesas@...ndi.org,
        laurent.pinchart+renesas@...asonboard.com,
        jorhand@...ux.microsoft.com, kitakar@...il.com,
        heikki.krogerus@...ux.intel.com
Subject: Re: [PATCH 13/18] ipu3-cio2: Add functionality allowing
 software_node connections to sensors on platforms designed for Windows

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@...ux.microsoft.com>
> >> Signed-off-by: Daniel Scally <djrscally@...il.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@...el.com>
> >>  M:	Sakari Ailus <sakari.ailus@...ux.intel.com>
> >>  M:	Bingbu Cao <bingbu.cao@...el.com>
> >> +M:	Dan Scally <djrscally@...il.com>
> >>  R:	Tianshu Qiu <tian.shu.qiu@...el.com>
> >>  L:	linux-media@...r.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@...il.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@...il.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

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists