[RFC] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

Currently on ACPI platforms, sensors that are intended to be connected to
a CIO2 device for use with the ipu3-cio2 driver lack the necessary
connection information in firmware. This patch adds a module to parse the
connection properties from the SSDB buffer in DSDT and build the connection
using software nodes.

The ipu3-cio2 driver itself is modified to insert the cio2-bridge module
after all sensors that have created a device link between themselves and
the CIO2 have probed. Sensors wishing to use this bridge will need to add
a device link between themselves and the CIO2 device as part of their own
.probe() call.

Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
This module's born out of efforts by the linux-surface github community
to get functioning webcams on Microsoft Surface and similar platforms. it
is dependent on this patch (which implements the software node graph family
of functions):

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

I wanted to raise this as an RFC as although I don't think it's ready for
integration it has some things that I'd like feedback on, in particular the
method I chose to make the module be auto-inserted. A more ideal method would
have been to have the driver be an ACPI driver for the INT343E device, but each
of the the devices we've tested this on that dev has status 0 and so the module
won't bind to it. The device links method seems a little clunky, but does work,
and I think I have done the conditional processing correctly so that ipu3-cio2
continues to work on non-ACPI platforms.

 MAINTAINERS                              |   6 +
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-
 drivers/staging/media/ipu3/Kconfig       |  15 +
 drivers/staging/media/ipu3/Makefile      |   1 +
 drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++
 5 files changed, 534 insertions(+), 3 deletions(-)
 create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c

On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
>  MAINTAINERS                              |   6 +
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-

staging drivers should be self-contained, and not modify stuff outside
of drivers/staging/

>  drivers/staging/media/ipu3/Kconfig       |  15 +
>  drivers/staging/media/ipu3/Makefile      |   1 +
>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++

Why does this have to be in drivers/staging/ at all?  Why not spend the
time to fix it up properly and get it merged correctly?  It's a very
small driver, and should be smaller, so it should not be a lot of work
to do.  And it would be faster to do that than to take it through
staging...

>  5 files changed, 534 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b5cfab015bd6..55b0b9888bc0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9152,6 +9152,12 @@ S:	Maintained
>  W:	http://www.adaptec.com/
>  F:	drivers/scsi/ips*
>  
> +IPU3 CIO2 Bridge Driver
> +M:	Daniel Scally <djrscally@gmail.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	drivers/staging/media/ipu3/cio2-bridge.c
> +
>  IPVS
>  M:	Wensong Zhang <wensong@linux-vs.org>
>  M:	Simon Horman <horms@verge.net.au>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 92f5eadf2c99..fd941d2c7581 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>  
> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
> +{
> +	void *sensor;

This is a huge flag that something is wrong, why void?

> +
> +	/*
> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect
> +	 * to cio2 have added a device link. If there are no consumers yet, then
> +	 * we need to defer. The .sync_state() callback will then be called after
> +	 * all linked sensors have probed
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_ACPI)) {
> +		sensor = (struct device *)list_first_entry_or_null(

And you cast it???  Not right at all.

> +								&pci_dev->dev.links.consumers,
> +								struct dev_links_info,
> +								consumers);

How do you "know" this is the first link?  This feels really really
wrong and very fragile.

> +
> +		if (!sensor)
> +			return -EPROBE_DEFER;

So any random value will work?  I doubt it :)

> +	}
> +
> +	return 0;
> +}
> +
> +void cio2_sync_state(struct device *dev)
> +{
> +	struct cio2_device *cio2;
> +	int ret = 0;
> +
> +	if (IS_ENABLED(CONFIG_ACPI)) {
> +		cio2 = dev_get_drvdata(dev);
> +
> +		if (!cio2) {
> +			dev_err(dev, "Failed to retrieve driver data\n");

How can this fail?

> +			return;

No error value?

> +		}
> +
> +		/* insert the bridge driver to connect sensors via software nodes */
> +		ret = request_module("cio2-bridge");
> +
> +		if (ret)
> +			dev_err(dev, "Failed to insert cio2-bridge\n");

Yet you keep on in the function???

> +
> +		ret = cio2_parse_firmware(cio2);
> +
> +		if (ret) {
> +			v4l2_async_notifier_unregister(&cio2->notifier);
> +			v4l2_async_notifier_cleanup(&cio2->notifier);
> +			cio2_queues_exit(cio2);

But you clean up after this error?

> +		}
> +	}

And again, do not tell anyone?

Feels really wrong...

> +}
> +
>  /**************** PCI interface ****************/
>  
>  static int cio2_pci_config_setup(struct pci_dev *dev)
> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	void __iomem *const *iomap;
>  	int r;
>  
> +	r = cio2_probe_can_progress(pci_dev);
> +
> +	if (r)
> +		return -EPROBE_DEFER;
> +
>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
>  	if (!cio2)
>  		return -ENOMEM;
> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	v4l2_async_notifier_init(&cio2->notifier);
>  
>  	/* Register notifier for subdevices we care */
> -	r = cio2_parse_firmware(cio2);
> -	if (r)
> -		goto fail_clean_notifier;
> +	if (!IS_ENABLED(CONFIG_ACPI)) {
> +		r = cio2_parse_firmware(cio2);
> +		if (r)
> +			goto fail_clean_notifier;
> +	}
>  
>  	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,
>  			     IRQF_SHARED, CIO2_NAME, cio2);
> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {
>  	.remove = cio2_pci_remove,
>  	.driver = {
>  		.pm = &cio2_pm_ops,
> +		.sync_state = cio2_sync_state
>  	},
>  };
>  
> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
> index 3e9640523e50..08842fd8c0da 100644
> --- a/drivers/staging/media/ipu3/Kconfig
> +++ b/drivers/staging/media/ipu3/Kconfig
> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU
>  
>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
>  	  camera. The module will be called ipu3-imgu.
> +
> +config VIDEO_CIO2_BRIDGE
> +	tristate "IPU3 CIO2 Sensor Bridge Driver"
> +	depends on PCI && VIDEO_V4L2
> +	depends on ACPI
> +	depends on X86

Why x86?

Why not CONFIG_TEST?



> +	help
> +	  This module provides a bridge connecting sensors (I.E. cameras) to
> +	  the CIO2 device infrastructure via software nodes built from information
> +	  parsed from the SSDB buffer.
> +
> +	  Say Y or M here if your platform's cameras use IPU3 with connections
> +	  that should be defined in ACPI. The module will be called cio2-bridge.
> +
> +	  If in doubt, say N here.
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
> index 9def80ef28f3..12dff56dbb9e 100644
> --- a/drivers/staging/media/ipu3/Makefile
> +++ b/drivers/staging/media/ipu3/Makefile
> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \
>  		ipu3-css.o ipu3-v4l2.o ipu3.o
>  
>  obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..5115aeeb35a1
> --- /dev/null
> +++ b/drivers/staging/media/ipu3/cio2-bridge.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include <linux/fwnode.h>
> +#include <linux/kref.h>

Why kref.h?



> +
> +static void cio2_bridge_exit(void);
> +static int cio2_bridge_init(void);
> +
> +#define MAX_CONNECTED_DEVICES			4
> +#define SWNODE_SENSOR_HID			0
> +#define SWNODE_SENSOR_PORT			1
> +#define SWNODE_SENSOR_ENDPOINT			2
> +#define SWNODE_CIO2_PORT			3
> +#define SWNODE_CIO2_ENDPOINT			4
> +#define SWNODE_NULL_TERMINATOR			5
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_PCI_ID				0x9d32
> +
> +#define ENDPOINT_SENSOR				0
> +#define ENDPOINT_CIO2				1
> +
> +#define NODE_HID(_HID)				\
> +((const struct software_node) {			\
> +	_HID,					\
> +})
> +
> +#define NODE_PORT(_PORT, _HID_NODE)		\
> +((const struct software_node) {			\
> +	_PORT,					\
> +	_HID_NODE,				\
> +})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +((const struct software_node) {			\
> +	_EP,					\
> +	_PORT,					\
> +	_PROPS,					\
> +})
> +
> +#define PROPERTY_ENTRY_NULL			\
> +((const struct property_entry) { })
> +#define SOFTWARE_NODE_NULL			\
> +((const struct software_node) { })
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be
> + * working
> + */
> +
> +static char *supported_devices[] = {
> +	"INT33BE",
> +	"OVTI2680",
> +	"OVTI5648",
> +};
> +
> +/*
> + * software_node needs const char * names. Can't snprintf a const char *,
> + * so instead we need an array of them and use the port num from SSDB as
> + * an index.
> + */
> +
> +const char *port_names[] = {
> +	"port0", "port1", "port2", "port3", "port4",
> +	"port5", "port6", "port7", "port8", "port9"
> +};
> +
> +struct software_node cio2_hid_node = { CIO2_HID, };
> +
> +struct sensor {
> +	struct device *dev;
> +	struct software_node swnodes[5];
> +	struct property_entry sensor_props[6];
> +	struct property_entry cio2_props[3];
> +	struct fwnode_handle *fwnode;
> +};
> +
> +struct cio2_bridge {
> +	int n_sensors;
> +	struct sensor sensors[MAX_CONNECTED_DEVICES];
> +	struct pci_dev *cio2;
> +	struct fwnode_handle *cio2_fwnode;
> +};
> +
> +struct cio2_bridge bridge = { 0, };
> +
> +static const struct property_entry remote_endpoints[] = {
> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */
> +			   &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */
> +			   &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	{ }
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct sensor_bios_data_packed {
> +	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];
> +} __attribute__((__packed__));

Endian issues???

This doesn't look "packed" to me, did you check it?

I've stopped here, sorry, ran out of time...

greg k-h

On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 92f5eadf2c99..fd941d2c7581 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>  
> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
> +{
> +	void *sensor;
> +
> +	/*
> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect
> +	 * to cio2 have added a device link. If there are no consumers yet, then
> +	 * we need to defer. The .sync_state() callback will then be called after
> +	 * all linked sensors have probed
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_ACPI)) {

Reverse this condition.

	if (!IS_ENABLED(CONFIG_ACPI))
		return 0;


> +		sensor = (struct device *)list_first_entry_or_null(
> +								&pci_dev->dev.links.consumers,
> +								struct dev_links_info,
> +								consumers);
> +
> +		if (!sensor)
> +			return -EPROBE_DEFER;

Get rid of the cast.

	if (list_empty(&pci_dev->dev.links.consumers))
		return -EPROBE_DEFER;

	return 0;

> +	}
> +
> +	return 0;
> +}
> +
> +void cio2_sync_state(struct device *dev)
> +{
> +	struct cio2_device *cio2;
> +	int ret = 0;

Delete the initialization.

> +
> +	if (IS_ENABLED(CONFIG_ACPI)) {

Reverse.

> +		cio2 = dev_get_drvdata(dev);
> +
> +		if (!cio2) {

Delete the blank line between the call and the test.  They're part of
the same step.  "cio2" can't be NULL anyway, so delete the test.

> +			dev_err(dev, "Failed to retrieve driver data\n");
> +			return;
> +		}
> +
> +		/* insert the bridge driver to connect sensors via software nodes */
> +		ret = request_module("cio2-bridge");
> +
> +		if (ret)
> +			dev_err(dev, "Failed to insert cio2-bridge\n");
> +
> +		ret = cio2_parse_firmware(cio2);
> +
> +		if (ret) {
> +			v4l2_async_notifier_unregister(&cio2->notifier);
> +			v4l2_async_notifier_cleanup(&cio2->notifier);
> +			cio2_queues_exit(cio2);
> +		}
> +	}
> +}
> +
>  /**************** PCI interface ****************/
>  
>  static int cio2_pci_config_setup(struct pci_dev *dev)
> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	void __iomem *const *iomap;
>  	int r;
>  
> +	r = cio2_probe_can_progress(pci_dev);
> +
> +	if (r)
> +		return -EPROBE_DEFER;
> +
>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
>  	if (!cio2)
>  		return -ENOMEM;
> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	v4l2_async_notifier_init(&cio2->notifier);
>  
>  	/* Register notifier for subdevices we care */
> -	r = cio2_parse_firmware(cio2);
> -	if (r)
> -		goto fail_clean_notifier;
> +	if (!IS_ENABLED(CONFIG_ACPI)) {
> +		r = cio2_parse_firmware(cio2);
> +		if (r)
> +			goto fail_clean_notifier;
> +	}
>  
>  	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,
>  			     IRQF_SHARED, CIO2_NAME, cio2);
> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {
>  	.remove = cio2_pci_remove,
>  	.driver = {
>  		.pm = &cio2_pm_ops,
> +		.sync_state = cio2_sync_state
>  	},
>  };
>  
> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
> index 3e9640523e50..08842fd8c0da 100644
> --- a/drivers/staging/media/ipu3/Kconfig
> +++ b/drivers/staging/media/ipu3/Kconfig
> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU
>  
>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
>  	  camera. The module will be called ipu3-imgu.
> +
> +config VIDEO_CIO2_BRIDGE
> +	tristate "IPU3 CIO2 Sensor Bridge Driver"
> +	depends on PCI && VIDEO_V4L2
> +	depends on ACPI
> +	depends on X86
> +	help
> +	  This module provides a bridge connecting sensors (I.E. cameras) to
> +	  the CIO2 device infrastructure via software nodes built from information
> +	  parsed from the SSDB buffer.
> +
> +	  Say Y or M here if your platform's cameras use IPU3 with connections
> +	  that should be defined in ACPI. The module will be called cio2-bridge.
> +
> +	  If in doubt, say N here.
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
> index 9def80ef28f3..12dff56dbb9e 100644
> --- a/drivers/staging/media/ipu3/Makefile
> +++ b/drivers/staging/media/ipu3/Makefile
> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \
>  		ipu3-css.o ipu3-v4l2.o ipu3.o
>  
>  obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..5115aeeb35a1
> --- /dev/null
> +++ b/drivers/staging/media/ipu3/cio2-bridge.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include <linux/fwnode.h>
> +#include <linux/kref.h>
> +
> +static void cio2_bridge_exit(void);
> +static int cio2_bridge_init(void);
> +
> +#define MAX_CONNECTED_DEVICES			4
> +#define SWNODE_SENSOR_HID			0
> +#define SWNODE_SENSOR_PORT			1
> +#define SWNODE_SENSOR_ENDPOINT			2
> +#define SWNODE_CIO2_PORT			3
> +#define SWNODE_CIO2_ENDPOINT			4
> +#define SWNODE_NULL_TERMINATOR			5
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_PCI_ID				0x9d32
> +
> +#define ENDPOINT_SENSOR				0
> +#define ENDPOINT_CIO2				1
> +
> +#define NODE_HID(_HID)				\
> +((const struct software_node) {			\
> +	_HID,					\
> +})
> +
> +#define NODE_PORT(_PORT, _HID_NODE)		\
> +((const struct software_node) {			\
> +	_PORT,					\
> +	_HID_NODE,				\
> +})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +((const struct software_node) {			\
> +	_EP,					\
> +	_PORT,					\
> +	_PROPS,					\
> +})
> +
> +#define PROPERTY_ENTRY_NULL			\
> +((const struct property_entry) { })
> +#define SOFTWARE_NODE_NULL			\
> +((const struct software_node) { })
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be
> + * working
> + */
> +
> +static char *supported_devices[] = {
> +	"INT33BE",
> +	"OVTI2680",
> +	"OVTI5648",
> +};
> +
> +/*
> + * software_node needs const char * names. Can't snprintf a const char *,
> + * so instead we need an array of them and use the port num from SSDB as
> + * an index.
> + */
> +
> +const char *port_names[] = {
> +	"port0", "port1", "port2", "port3", "port4",
> +	"port5", "port6", "port7", "port8", "port9"
> +};
> +
> +struct software_node cio2_hid_node = { CIO2_HID, };
> +
> +struct sensor {
> +	struct device *dev;
> +	struct software_node swnodes[5];
> +	struct property_entry sensor_props[6];
> +	struct property_entry cio2_props[3];
> +	struct fwnode_handle *fwnode;
> +};
> +
> +struct cio2_bridge {
> +	int n_sensors;
> +	struct sensor sensors[MAX_CONNECTED_DEVICES];
> +	struct pci_dev *cio2;
> +	struct fwnode_handle *cio2_fwnode;
> +};
> +
> +struct cio2_bridge bridge = { 0, };
> +
> +static const struct property_entry remote_endpoints[] = {
> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */
> +			   &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */
> +			   &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),
> +	PROPERTY_ENTRY_REF("remote-endpoint",
> +			   &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),
> +	{ }
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct sensor_bios_data_packed {
> +	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];
> +} __attribute__((__packed__));
> +
> +/* Fields needed by bridge driver */
> +struct sensor_bios_data {
> +	struct device *dev;
> +	u8 link;
> +	u8 lanes;
> +	u32 mclkspeed;
> +};
> +
> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)
> +{
> +	union acpi_object *obj;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	struct acpi_handle *dev_handle = ACPI_HANDLE(dev);
> +	int status;
> +	u32 buffer_length;
> +
> +	status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);
> +	if (!ACPI_SUCCESS(status))
> +		return -ENODEV;
> +
> +	obj = (union acpi_object *)buffer.pointer;
> +	if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> +		dev_err(dev, "Could't read acpi buffer\n");
> +		status = -ENODEV;
> +		goto err;
> +	}
> +
> +	if (obj->buffer.length > size) {
> +		dev_err(dev, "Given buffer is too small\n");
> +		status = -ENODEV;
> +		goto err;
> +	}
> +
> +	memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));

The min() is not required because we checked the length earlier.

> +	buffer_length = obj->buffer.length;

No need for the "buffer_length" variable.

> +	kfree(buffer.pointer);
> +
> +	return buffer_length;
> +err:
> +	kfree(buffer.pointer);
> +	return status;
> +}
> +
> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> +				     struct sensor_bios_data *sensor)
> +{
> +	struct sensor_bios_data_packed sensor_data;
> +	int ret = read_acpi_block(dev, "SSDB", &sensor_data,
> +				  sizeof(sensor_data));

Don't put functions which can fail into the declaration block.

> +
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to fetch SSDB data\n");
> +		return ret;
> +	}
> +
> +	sensor->link = sensor_data.link;
> +	sensor->lanes = sensor_data.lanes;
> +	sensor->mclkspeed = sensor_data.mclkspeed;
> +
> +	return 0;
> +}
> +
> +static int create_endpoint_properties(struct device *dev,
> +				      struct sensor_bios_data *ssdb,
> +				      struct property_entry *sensor_props,
> +				      struct property_entry *cio2_props)
> +{
> +		u32 *data_lanes;
> +		int i;

Indented too far.

> +
> +		data_lanes = devm_kmalloc(dev, sizeof(u32) * (int)ssdb->lanes,

No need for the cast.  Use devm_kmalloc_array().

> +					  GFP_KERNEL);
> +
> +		if (!data_lanes) {
> +			dev_err(dev,
> +				"Couldn't allocate memory for data lanes array\n");

Delete the error message (checkpatch.pl --strict).

> +			return -ENOMEM;
> +		}
> +
> +		for (i = 0; i < (int)ssdb->lanes; i++)

Delete the cast.

> +			data_lanes[i] = (u32)i + 1;

Delete the cast.

> +
> +		sensor_props[0] = PROPERTY_ENTRY_U32("clock-frequency",
> +						     ssdb->mclkspeed);
> +		sensor_props[1] = PROPERTY_ENTRY_U32("bus-type", 5);
> +		sensor_props[2] = PROPERTY_ENTRY_U32("clock-lanes", 0);
> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> +							       data_lanes,
> +							       (int)ssdb->lanes);
> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];
> +		sensor_props[5] = PROPERTY_ENTRY_NULL;
> +
> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> +							     data_lanes,
> +							     (int)ssdb->lanes);
> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];
> +		cio2_props[2] = PROPERTY_ENTRY_NULL;
> +
> +		return 0;
> +}
> +
> +static int connect_supported_devices(void)
> +{
> +	struct acpi_device *adev;
> +	struct device *dev;
> +	struct sensor_bios_data ssdb;
> +	struct sensor *sensor;
> +	struct property_entry *sensor_props;
> +	struct property_entry *cio2_props;
> +	struct fwnode_handle *fwnode;
> +	struct software_node *nodes;
> +	struct v4l2_subdev *sd;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
> +		adev = acpi_dev_get_first_match_dev(supported_devices[i],
> +						    NULL, -1);
> +
> +		if (!adev)
> +			continue;
> +
> +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> +
> +		if (!dev) {
> +			pr_info("ACPI match for %s, but it has no i2c device\n",
> +				supported_devices[i]);
> +			continue;
> +		}
> +
> +		if (!dev->driver_data) {
> +			pr_info("ACPI match for %s, but it has no driver\n",
> +				supported_devices[i]);

put_device(dev);

> +			continue;
> +		} else {
> +			pr_info("Found supported device %s\n",
> +				supported_devices[i]);
> +		}
> +
> +		sensor = &bridge.sensors[bridge.n_sensors];
> +		/*
> +		 * Store sensor's existing fwnode so that it can be restored if
> +		 * this module is removed.
> +		 */
> +		sensor->fwnode = fwnode_handle_get(dev->fwnode);
> +
> +		get_acpi_ssdb_sensor_data(dev, &ssdb);
> +
> +		nodes = sensor->swnodes;
> +		sensor_props = sensor->sensor_props;
> +		cio2_props = sensor->cio2_props;
> +		fwnode = sensor->fwnode;
> +
> +		ret = create_endpoint_properties(dev, &ssdb, sensor_props,
> +						 cio2_props);
> +
> +		if (ret)
> +			return ret;
> +
> +		/* build the software nodes */
> +
> +		nodes[SWNODE_SENSOR_HID] = NODE_HID(supported_devices[i]);
> +		nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0",
> +						      &nodes[SWNODE_SENSOR_HID]);
> +		nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> +							      &nodes[SWNODE_SENSOR_PORT],
> +							      sensor_props);
> +		nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[(int)ssdb.link],
> +						    &cio2_hid_node);
> +		nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> +							    &nodes[SWNODE_CIO2_PORT],
> +							    cio2_props);
> +		nodes[SWNODE_NULL_TERMINATOR]   = SOFTWARE_NODE_NULL;
> +
> +		ret = software_node_register_nodes(nodes);
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to register software nodes for %s\n",
> +				supported_devices[i]);
> +			return ret;
> +		}
> +
> +		fwnode = software_node_fwnode(&nodes[SWNODE_SENSOR_HID]);
> +		if (!fwnode) {
> +			dev_err(dev,
> +				"Failed to get software node for %s\n",
> +				supported_devices[i]);
> +			return ret;


"ret" is zero here.  return -ENODEV;?

> +		}
> +
> +		fwnode->secondary = ERR_PTR(-ENODEV);
> +		dev->fwnode = fwnode;
> +
> +		/*
> +		 * The device should by this point has driver_data set to an
> +		 * instance of struct v4l2_subdev; set the fwnode for that too.
> +		 */
> +
> +		sd = dev_get_drvdata(dev);
> +		sd->fwnode = fwnode;
> +
> +		sensor->dev = dev;
> +		bridge.n_sensors++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cio2_bridge_init(void)
> +{
> +	struct fwnode_handle *fwnode;
> +	int ret;
> +
> +	ret = software_node_register(&cio2_hid_node);
> +
> +	if (ret < 0) {
> +		pr_err("Failed to register the CIO2 HID node\n");
> +		return -EINVAL;

Propagate the error code from software_node_register().

> +	}
> +
> +	ret = connect_supported_devices();
> +
> +	if ((ret < 0) || (bridge.n_sensors <= 0)) {
> +		pr_err("cio2_bridge: Failed to connect any devices\n");
> +		goto out;

If (bridge.n_sensors <= 0) is true then we need to set ret = -EINVAL
or something.  Really .n_sensors can't be negative.

The name "out" is a crappy label name because it doesn't say what the
goto does.  When I scroll down then it turns out that "goto out;" calls
a free_everything() function.  That kind of error handling is always
buggy.

There are several typical bugs.  1) Something leaks because the error
handling style is too complicated to be audited.  2)  Dereferencing
uninitialized pointers.  3)  Undoing something which hasn't been done.

I believe that in this case one bug is with the handling of the
bridge.cio2_fwnode.  We "restore" it back to the original state
as soon as we have a non-NULL bridge.cio2 instead of waiting until we
have stored the original state.

The best way to do error handling is this.

Every function cleans up after itself.  The connect_supported_devices()
function is a bit special because it's a loop.  I would would write it
so that if it fails then it cleans up the partial loop iteration and
then at the end it cleans up all the failed loop iterations.

	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
		a = frob();
		if (!a)
			goto unwind;
		b = frob();
		if (!b) {
			free(a);
			goto unwind;
		}
		...
	}

	return 0;

unwind:
	for (i = 0; i < bridge.n_sensors; i++) {
		free(b);
		free(a);
	}
	bridge.n_sensors = 0;

	return ret;

The problem with cio2_bridge_unregister_sensors() is that it doesn't
clean up partial iterations through the loop.  (Missing calls to
put_device(dev)).

Loops are complicated but the rest is simple.  1) Every allocation
function needs a matching cleanup function.  2) Use good label names
which say what the goto does.  3)  The goto should free the most recent
successful allocation.

	a = frob();
	if (!a)
		return -ENOMEM;

	b = frob();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

	c = frob();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;
	}

	return 0;

free_b:
	free(b);
free_a:
	free(a);

	return ret;

The free function doesn't have any if statements.

void free_function()
{
	free(c);
	free(b);
	free(a);
}

The reviewer only needs to keep track of the most recent allocation
and verify that the goto free_foo matches what should be freed.  This
system means the code is auditable (no leaks), you never free anything
which wasn't allocated.

> +	} else {
> +		pr_info("Found %d supported devices\n", bridge.n_sensors);
> +	}
> +
> +	bridge.cio2 = pci_get_device(PCI_VENDOR_ID_INTEL, CIO2_PCI_ID, NULL);
> +	if (!bridge.cio2) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	fwnode = software_node_fwnode(&cio2_hid_node);
> +	if (!fwnode) {
> +		pr_err("Error getting fwnode from cio2 software_node\n");
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	/*
> +	 * We store the pci_dev's existing fwnode, beccause in the event we
> +	 * want to reload (I.E. rmmod and insmod) this module we need to give
> +	 * the device its original fwnode back to prevent problems down the
> +	 * line
> +	 */
> +
> +	bridge.cio2_fwnode = fwnode_handle_get(bridge.cio2->dev.fwnode);
> +
> +	fwnode->secondary = ERR_PTR(-ENODEV);
> +	bridge.cio2->dev.fwnode = fwnode;
> +
> +	return 0;
> +out:
> +	cio2_bridge_exit();
> +	return ret;
> +}
> +
> +static int cio2_bridge_unregister_sensors(void)

Make this a void function.

regards,
dan carpenter

> +{
> +	int i, j;
> +	struct sensor *sensor;
> +
> +	for (i = 0; i < bridge.n_sensors; i++) {
> +		sensor = &bridge.sensors[i];
> +
> +		/* give the sensor its original fwnode back */
> +		sensor->dev->fwnode = sensor->fwnode;
> +		fwnode_handle_put(sensor->fwnode);
> +		put_device(sensor->dev);
> +
> +		for (j = 4; j >= 0; j--)
> +			software_node_unregister(&sensor->swnodes[j]);
> +	}
> +
> +	return 0;
> +}
> +
> +static void cio2_bridge_exit(void)
> +{
> +	int ret;
> +
> +	/* Give the pci_dev its original fwnode back */
> +	if (bridge.cio2) {
> +		bridge.cio2->dev.fwnode = bridge.cio2_fwnode;
> +		fwnode_handle_put(bridge.cio2_fwnode);
> +		pci_dev_put(bridge.cio2);
> +	}
> +
> +	ret = cio2_bridge_unregister_sensors();
> +
> +	if (ret)
> +		pr_err("An error occurred unregistering the sensors\n");
> +
> +	software_node_unregister(&cio2_hid_node);
> +}
> +
> +module_init(cio2_bridge_init);
> +module_exit(cio2_bridge_exit);
> +
> +MODULE_DESCRIPTION("A bridge driver to connect sensors to CIO2 infrastructure.");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("acpi*:INT343E:*");
> -- 
> 2.17.1
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Hi Greg - thanks for the comments, appreciate it (sorry there's so many,
I'm new to both C and kernel work)

On 17/09/2020 08:53, Greg KH wrote:
> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
>>  MAINTAINERS                              |   6 +
>>  drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-
> staging drivers should be self-contained, and not modify stuff outside
> of drivers/staging/
>
>>  drivers/staging/media/ipu3/Kconfig       |  15 +
>>  drivers/staging/media/ipu3/Makefile      |   1 +
>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++
> Why does this have to be in drivers/staging/ at all?  Why not spend the
> time to fix it up properly and get it merged correctly?  It's a very
> small driver, and should be smaller, so it should not be a lot of work
> to do.  And it would be faster to do that than to take it through
> staging...
I was just under the impression that that was the process to be honest,
if that's not right I'll just move it directly to drivers/media/ipu3
>>  5 files changed, 534 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b5cfab015bd6..55b0b9888bc0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9152,6 +9152,12 @@ S:	Maintained
>>  W:	http://www.adaptec.com/
>>  F:	drivers/scsi/ips*
>>  
>> +IPU3 CIO2 Bridge Driver
>> +M:	Daniel Scally <djrscally@gmail.com>
>> +L:	linux-media@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/staging/media/ipu3/cio2-bridge.c
>> +
>>  IPVS
>>  M:	Wensong Zhang <wensong@linux-vs.org>
>>  M:	Simon Horman <horms@verge.net.au>
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> index 92f5eadf2c99..fd941d2c7581 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>>  }
>>  
>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
>> +{
>> +	void *sensor;
> This is a huge flag that something is wrong, why void?
>
>> +
>> +	/*
>> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect
>> +	 * to cio2 have added a device link. If there are no consumers yet, then
>> +	 * we need to defer. The .sync_state() callback will then be called after
>> +	 * all linked sensors have probed
>> +	 */
>> +
>> +	if (IS_ENABLED(CONFIG_ACPI)) {
>> +		sensor = (struct device *)list_first_entry_or_null(
> And you cast it???  Not right at all.

Yeah sorry; misunderstood entirely how that was supposed to work. From
the following comment, this probably needs re-implementing as
list_for_each_entry anyway

>
>> +								&pci_dev->dev.links.consumers,
>> +								struct dev_links_info,
>> +								consumers);
> How do you "know" this is the first link?  This feels really really
> wrong and very fragile.
>
>> +
>> +		if (!sensor)
>> +			return -EPROBE_DEFER;
> So any random value will work?  I doubt it :)
So the intention was just to check that there is _a_ linked device,
which I had been assuming would be a sensor that wanted to use the cio2
device. That's probably not very smart in retrospect; I hadn't
considered other none-me pieces of code linking in. I guess a better
approach would be to check all the linked devices with list_each_entry
and determine if at least one of them is a sensor.
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void cio2_sync_state(struct device *dev)
>> +{
>> +	struct cio2_device *cio2;
>> +	int ret = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_ACPI)) {
>> +		cio2 = dev_get_drvdata(dev);
>> +
>> +		if (!cio2) {
>> +			dev_err(dev, "Failed to retrieve driver data\n");
> How can this fail?
Yeah I guess if the cio2_pci_probe() never made it to setting driver
data then the sync_state() shouldn't get called; thanks.
>
>> +			return;
> No error value?
The prototype for sync_state callbacks is to return void, so my
understanding is it can't return an error value.  I guess a better thing
to do might be call another function performing cleanup and unloading
the driver before the return or something along those lines though.
>
>> +		}
>> +
>> +		/* insert the bridge driver to connect sensors via software nodes */
>> +		ret = request_module("cio2-bridge");
>> +
>> +		if (ret)
>> +			dev_err(dev, "Failed to insert cio2-bridge\n");
> Yet you keep on in the function???
>> +
>> +		ret = cio2_parse_firmware(cio2);
>> +
>> +		if (ret) {
>> +			v4l2_async_notifier_unregister(&cio2->notifier);
>> +			v4l2_async_notifier_cleanup(&cio2->notifier);
>> +			cio2_queues_exit(cio2);
> But you clean up after this error?
>
If the bridge doesn't work, the cio2_parse_firmware() call should behave
as it does now on these platforms - I.E. just not connect anything. If
felt ok for that to happen, but I can have it perform cleanup at this
point if that's a better approach.
>> +		}
>> +	}
> And again, do not tell anyone?
>
> Feels really wrong...
I think return type void prevents that, but I could be completely wrong
about that.
>> +}
>> +
>>  /**************** PCI interface ****************/
>>  
>>  static int cio2_pci_config_setup(struct pci_dev *dev)
>> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>>  	void __iomem *const *iomap;
>>  	int r;
>>  
>> +	r = cio2_probe_can_progress(pci_dev);
>> +
>> +	if (r)
>> +		return -EPROBE_DEFER;
>> +
>>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
>>  	if (!cio2)
>>  		return -ENOMEM;
>> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>>  	v4l2_async_notifier_init(&cio2->notifier);
>>  
>>  	/* Register notifier for subdevices we care */
>> -	r = cio2_parse_firmware(cio2);
>> -	if (r)
>> -		goto fail_clean_notifier;
>> +	if (!IS_ENABLED(CONFIG_ACPI)) {
>> +		r = cio2_parse_firmware(cio2);
>> +		if (r)
>> +			goto fail_clean_notifier;
>> +	}
>>  
>>  	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,
>>  			     IRQF_SHARED, CIO2_NAME, cio2);
>> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {
>>  	.remove = cio2_pci_remove,
>>  	.driver = {
>>  		.pm = &cio2_pm_ops,
>> +		.sync_state = cio2_sync_state
>>  	},
>>  };
>>  
>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
>> index 3e9640523e50..08842fd8c0da 100644
>> --- a/drivers/staging/media/ipu3/Kconfig
>> +++ b/drivers/staging/media/ipu3/Kconfig
>> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU
>>  
>>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
>>  	  camera. The module will be called ipu3-imgu.
>> +
>> +config VIDEO_CIO2_BRIDGE
>> +	tristate "IPU3 CIO2 Sensor Bridge Driver"
>> +	depends on PCI && VIDEO_V4L2
>> +	depends on ACPI
>> +	depends on X86
> Why x86?
>
> Why not CONFIG_TEST?
X86 because as far as I know this is already working properly on other
platforms, so wouldn't be needed. Not sure what CONFIG_TEST is for; sorry.
>> +	help
>> +	  This module provides a bridge connecting sensors (I.E. cameras) to
>> +	  the CIO2 device infrastructure via software nodes built from information
>> +	  parsed from the SSDB buffer.
>> +
>> +	  Say Y or M here if your platform's cameras use IPU3 with connections
>> +	  that should be defined in ACPI. The module will be called cio2-bridge.
>> +
>> +	  If in doubt, say N here.
>> \ No newline at end of file
>> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
>> index 9def80ef28f3..12dff56dbb9e 100644
>> --- a/drivers/staging/media/ipu3/Makefile
>> +++ b/drivers/staging/media/ipu3/Makefile
>> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \
>>  		ipu3-css.o ipu3-v4l2.o ipu3.o
>>  
>>  obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
>> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o
>> \ No newline at end of file
>> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c
>> new file mode 100644
>> index 000000000000..5115aeeb35a1
>> --- /dev/null
>> +++ b/drivers/staging/media/ipu3/cio2-bridge.c
>> @@ -0,0 +1,448 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <linux/acpi.h>
>> +#include <acpi/acpi_bus.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#include <linux/fwnode.h>
>> +#include <linux/kref.h>
> Why kref.h?
Oops; a leftover from debugging some reference count issues in the
earlier days - I'll take that out.
>> +
>> +static void cio2_bridge_exit(void);
>> +static int cio2_bridge_init(void);
>> +
>> +#define MAX_CONNECTED_DEVICES			4
>> +#define SWNODE_SENSOR_HID			0
>> +#define SWNODE_SENSOR_PORT			1
>> +#define SWNODE_SENSOR_ENDPOINT			2
>> +#define SWNODE_CIO2_PORT			3
>> +#define SWNODE_CIO2_ENDPOINT			4
>> +#define SWNODE_NULL_TERMINATOR			5
>> +
>> +#define CIO2_HID				"INT343E"
>> +#define CIO2_PCI_ID				0x9d32
>> +
>> +#define ENDPOINT_SENSOR				0
>> +#define ENDPOINT_CIO2				1
>> +
>> +#define NODE_HID(_HID)				\
>> +((const struct software_node) {			\
>> +	_HID,					\
>> +})
>> +
>> +#define NODE_PORT(_PORT, _HID_NODE)		\
>> +((const struct software_node) {			\
>> +	_PORT,					\
>> +	_HID_NODE,				\
>> +})
>> +
>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
>> +((const struct software_node) {			\
>> +	_EP,					\
>> +	_PORT,					\
>> +	_PROPS,					\
>> +})
>> +
>> +#define PROPERTY_ENTRY_NULL			\
>> +((const struct property_entry) { })
>> +#define SOFTWARE_NODE_NULL			\
>> +((const struct software_node) { })
>> +
>> +/*
>> + * Extend this array with ACPI Hardware ID's of devices known to be
>> + * working
>> + */
>> +
>> +static char *supported_devices[] = {
>> +	"INT33BE",
>> +	"OVTI2680",
>> +	"OVTI5648",
>> +};
>> +
>> +/*
>> + * software_node needs const char * names. Can't snprintf a const char *,
>> + * so instead we need an array of them and use the port num from SSDB as
>> + * an index.
>> + */
>> +
>> +const char *port_names[] = {
>> +	"port0", "port1", "port2", "port3", "port4",
>> +	"port5", "port6", "port7", "port8", "port9"
>> +};
>> +
>> +struct software_node cio2_hid_node = { CIO2_HID, };
>> +
>> +struct sensor {
>> +	struct device *dev;
>> +	struct software_node swnodes[5];
>> +	struct property_entry sensor_props[6];
>> +	struct property_entry cio2_props[3];
>> +	struct fwnode_handle *fwnode;
>> +};
>> +
>> +struct cio2_bridge {
>> +	int n_sensors;
>> +	struct sensor sensors[MAX_CONNECTED_DEVICES];
>> +	struct pci_dev *cio2;
>> +	struct fwnode_handle *cio2_fwnode;
>> +};
>> +
>> +struct cio2_bridge bridge = { 0, };
>> +
>> +static const struct property_entry remote_endpoints[] = {
>> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */
>> +			   &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */
>> +			   &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),
>> +	PROPERTY_ENTRY_REF("remote-endpoint",
>> +			   &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),
>> +	{ }
>> +};
>> +
>> +/* Data representation as it is in ACPI SSDB buffer */
>> +struct sensor_bios_data_packed {
>> +	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];
>> +} __attribute__((__packed__));
> Endian issues???
>
> This doesn't look "packed" to me, did you check it?
>
> I've stopped here, sorry, ran out of time...
>
I didn't - I'll do that. I appreciate you spending the time at all -
thanks, and sorry it's been so messy!

On Thu, Sep 17, 2020 at 10:47:50AM +0100, Dan Scally wrote:
> Hi Greg - thanks for the comments, appreciate it (sorry there's so many,

> I'm new to both C and kernel work)


It's pretty impressive work if you're new to C...

> >

> >> +			return;

> > No error value?

> The prototype for sync_state callbacks is to return void, so my

> understanding is it can't return an error value.  I guess a better thing

> to do might be call another function performing cleanup and unloading

> the driver before the return or something along those lines though.


Yeah.  I suspect you should be using a different callback instead of
->sync_state() but I don't know what... :/

regards,
dan carpenter

On Thu, 2020-09-17 at 12:34 +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:

> > diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c

[]
> > +		if (!dev->driver_data) {

> > +			pr_info("ACPI match for %s, but it has no driver\n",

> > +				supported_devices[i]);

> 

> put_device(dev);

> 

> > +			continue;

> > +		} else {


No need for an else either.

> > +			pr_info("Found supported device %s\n",

> > +				supported_devices[i]);


so this can be unindented too.

On 17/09/2020 11:15, Dan Carpenter wrote:
> On Thu, Sep 17, 2020 at 10:47:50AM +0100, Dan Scally wrote:
>> Hi Greg - thanks for the comments, appreciate it (sorry there's so many,
>> I'm new to both C and kernel work)
> It's pretty impressive work if you're new to C...
Thanks (and for your other reply too, haven't had time to go through it
in depth yet). I've been using python for a while, but this is my first
attempt at anything serious with C.
>>>> +			return;
>>> No error value?
>> The prototype for sync_state callbacks is to return void, so my
>> understanding is it can't return an error value.  I guess a better thing
>> to do might be call another function performing cleanup and unloading
>> the driver before the return or something along those lines though.
> Yeah.  I suspect you should be using a different callback instead of
> ->sync_state() but I don't know what... :/
Yeah, this is why I wanted to submit it now really; I too suspect I
should really be using a different callback but I couldn't find a better
option.
> regards,
> dan carpenter
>

Hi Daniel,

Thank you for the patch.

Is this all that it takes to add support for some machines shipped with
Windows? The ones I know require PMIC control done in software (not even
sensors are accessible without that).

One possibility would be to put this to platform code. That would
effectively also require it's compiled to the kernel (yuck).

How about just squashing this to the CIO2 driver instead (but still as a
separate file)? It's not exactly pretty, no, but it could allow this being
a module and not enlarge everyone's kernel, and the initialisation would at
the same time take place before the rest of what the CIO2 driver does in
probe.

I think you should still check whether CIO2 has graph endpoints before
proceeding with parsing SSDB buffer or looking up random-looking devices.

Cc Andy, too.

On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> Currently on ACPI platforms, sensors that are intended to be connected to

> a CIO2 device for use with the ipu3-cio2 driver lack the necessary

> connection information in firmware. This patch adds a module to parse the

> connection properties from the SSDB buffer in DSDT and build the connection

> using software nodes.

> 

> The ipu3-cio2 driver itself is modified to insert the cio2-bridge module

> after all sensors that have created a device link between themselves and

> the CIO2 have probed. Sensors wishing to use this bridge will need to add

> a device link between themselves and the CIO2 device as part of their own

> .probe() call.

> 

> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>

> 

> Signed-off-by: Daniel Scally <djrscally@gmail.com>

> ---

> This module's born out of efforts by the linux-surface github community

> to get functioning webcams on Microsoft Surface and similar platforms. it

> is dependent on this patch (which implements the software node graph family

> of functions):

> 

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

> 

> I wanted to raise this as an RFC as although I don't think it's ready for

> integration it has some things that I'd like feedback on, in particular the

> method I chose to make the module be auto-inserted. A more ideal method would

> have been to have the driver be an ACPI driver for the INT343E device, but each


What do you think this device does represent? Devices whose status is
always zero may exist in the table even if they would not be actually
present.

CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not
have one.

> of the the devices we've tested this on that dev has status 0 and so the module

> won't bind to it. The device links method seems a little clunky, but does work,

> and I think I have done the conditional processing correctly so that ipu3-cio2

> continues to work on non-ACPI platforms.


I don't think anyone uses ipu3-cio2 driver on non-ACPI platforms. It really
does require ACPI.

> 

>  MAINTAINERS                              |   6 +

>  drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-

>  drivers/staging/media/ipu3/Kconfig       |  15 +

>  drivers/staging/media/ipu3/Makefile      |   1 +

>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++

>  5 files changed, 534 insertions(+), 3 deletions(-)

>  create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c

> 

> diff --git a/MAINTAINERS b/MAINTAINERS

> index b5cfab015bd6..55b0b9888bc0 100644

> --- a/MAINTAINERS

> +++ b/MAINTAINERS

> @@ -9152,6 +9152,12 @@ S:	Maintained

>  W:	http://www.adaptec.com/

>  F:	drivers/scsi/ips*

>  

> +IPU3 CIO2 Bridge Driver

> +M:	Daniel Scally <djrscally@gmail.com>

> +L:	linux-media@vger.kernel.org

> +S:	Maintained

> +F:	drivers/staging/media/ipu3/cio2-bridge.c

> +

>  IPVS

>  M:	Wensong Zhang <wensong@linux-vs.org>

>  M:	Simon Horman <horms@verge.net.au>

> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> index 92f5eadf2c99..fd941d2c7581 100644

> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)

>  		cio2_queue_exit(cio2, &cio2->queue[i]);

>  }

>  

> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)

> +{

> +	void *sensor;

> +

> +	/*

> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect

> +	 * to cio2 have added a device link. If there are no consumers yet, then

> +	 * we need to defer. The .sync_state() callback will then be called after

> +	 * all linked sensors have probed

> +	 */

> +

> +	if (IS_ENABLED(CONFIG_ACPI)) {

> +		sensor = (struct device *)list_first_entry_or_null(

> +								&pci_dev->dev.links.consumers,

> +								struct dev_links_info,

> +								consumers);


Please wrap so it's under 80.

> +

> +		if (!sensor)

> +			return -EPROBE_DEFER;

> +	}

> +

> +	return 0;

> +}

> +

> +void cio2_sync_state(struct device *dev)

> +{

> +	struct cio2_device *cio2;

> +	int ret = 0;

> +

> +	if (IS_ENABLED(CONFIG_ACPI)) {

> +		cio2 = dev_get_drvdata(dev);

> +

> +		if (!cio2) {

> +			dev_err(dev, "Failed to retrieve driver data\n");

> +			return;

> +		}

> +

> +		/* insert the bridge driver to connect sensors via software nodes */

> +		ret = request_module("cio2-bridge");

> +

> +		if (ret)

> +			dev_err(dev, "Failed to insert cio2-bridge\n");

> +

> +		ret = cio2_parse_firmware(cio2);

> +

> +		if (ret) {

> +			v4l2_async_notifier_unregister(&cio2->notifier);

> +			v4l2_async_notifier_cleanup(&cio2->notifier);

> +			cio2_queues_exit(cio2);

> +		}

> +	}

> +}

> +

>  /**************** PCI interface ****************/

>  

>  static int cio2_pci_config_setup(struct pci_dev *dev)

> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,

>  	void __iomem *const *iomap;

>  	int r;

>  

> +	r = cio2_probe_can_progress(pci_dev);

> +

> +	if (r)

> +		return -EPROBE_DEFER;

> +

>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);

>  	if (!cio2)

>  		return -ENOMEM;

> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,

>  	v4l2_async_notifier_init(&cio2->notifier);

>  

>  	/* Register notifier for subdevices we care */

> -	r = cio2_parse_firmware(cio2);

> -	if (r)

> -		goto fail_clean_notifier;

> +	if (!IS_ENABLED(CONFIG_ACPI)) {

> +		r = cio2_parse_firmware(cio2);

> +		if (r)

> +			goto fail_clean_notifier;

> +	}

>  

>  	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,

>  			     IRQF_SHARED, CIO2_NAME, cio2);

> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {

>  	.remove = cio2_pci_remove,

>  	.driver = {

>  		.pm = &cio2_pm_ops,

> +		.sync_state = cio2_sync_state

>  	},

>  };

>  

> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig

> index 3e9640523e50..08842fd8c0da 100644

> --- a/drivers/staging/media/ipu3/Kconfig

> +++ b/drivers/staging/media/ipu3/Kconfig

> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU

>  

>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI

>  	  camera. The module will be called ipu3-imgu.

> +

> +config VIDEO_CIO2_BRIDGE

> +	tristate "IPU3 CIO2 Sensor Bridge Driver"

> +	depends on PCI && VIDEO_V4L2

> +	depends on ACPI

> +	depends on X86

> +	help

> +	  This module provides a bridge connecting sensors (I.E. cameras) to

> +	  the CIO2 device infrastructure via software nodes built from information

> +	  parsed from the SSDB buffer.

> +

> +	  Say Y or M here if your platform's cameras use IPU3 with connections

> +	  that should be defined in ACPI. The module will be called cio2-bridge.

> +

> +	  If in doubt, say N here.

> \ No newline at end of file

> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile

> index 9def80ef28f3..12dff56dbb9e 100644

> --- a/drivers/staging/media/ipu3/Makefile

> +++ b/drivers/staging/media/ipu3/Makefile

> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \

>  		ipu3-css.o ipu3-v4l2.o ipu3.o

>  

>  obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o

> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o

> \ No newline at end of file

> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c

> new file mode 100644

> index 000000000000..5115aeeb35a1

> --- /dev/null

> +++ b/drivers/staging/media/ipu3/cio2-bridge.c

> @@ -0,0 +1,448 @@

> +// SPDX-License-Identifier: GPL-2.0

> +#include <linux/acpi.h>

> +#include <acpi/acpi_bus.h>

> +#include <linux/device.h>

> +#include <linux/i2c.h>

> +#include <linux/kernel.h>

> +#include <linux/module.h>

> +#include <linux/pci.h>

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

> +

> +#include <linux/fwnode.h>

> +#include <linux/kref.h>

> +

> +static void cio2_bridge_exit(void);

> +static int cio2_bridge_init(void);

> +

> +#define MAX_CONNECTED_DEVICES			4

> +#define SWNODE_SENSOR_HID			0

> +#define SWNODE_SENSOR_PORT			1

> +#define SWNODE_SENSOR_ENDPOINT			2

> +#define SWNODE_CIO2_PORT			3

> +#define SWNODE_CIO2_ENDPOINT			4

> +#define SWNODE_NULL_TERMINATOR			5

> +

> +#define CIO2_HID				"INT343E"

> +#define CIO2_PCI_ID				0x9d32

> +

> +#define ENDPOINT_SENSOR				0

> +#define ENDPOINT_CIO2				1

> +

> +#define NODE_HID(_HID)				\

> +((const struct software_node) {			\

> +	_HID,					\

> +})

> +

> +#define NODE_PORT(_PORT, _HID_NODE)		\

> +((const struct software_node) {			\

> +	_PORT,					\

> +	_HID_NODE,				\

> +})

> +

> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\

> +((const struct software_node) {			\

> +	_EP,					\

> +	_PORT,					\

> +	_PROPS,					\

> +})

> +

> +#define PROPERTY_ENTRY_NULL			\

> +((const struct property_entry) { })


Alignment. Same appears to apply to other macros (please indent).

> +#define SOFTWARE_NODE_NULL			\

> +((const struct software_node) { })

> +

> +/*

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

> + * working

> + */

> +

> +static char *supported_devices[] = {

> +	"INT33BE",

> +	"OVTI2680",

> +	"OVTI5648",

> +};

> +

> +/*

> + * software_node needs const char * names. Can't snprintf a const char *,

> + * so instead we need an array of them and use the port num from SSDB as

> + * an index.

> + */

> +

> +const char *port_names[] = {

> +	"port0", "port1", "port2", "port3", "port4",

> +	"port5", "port6", "port7", "port8", "port9"


I think CIO2 is limited to 4.

> +};

> +

> +struct software_node cio2_hid_node = { CIO2_HID, };

> +

> +struct sensor {

> +	struct device *dev;

> +	struct software_node swnodes[5];

> +	struct property_entry sensor_props[6];

> +	struct property_entry cio2_props[3];

> +	struct fwnode_handle *fwnode;

> +};

> +

> +struct cio2_bridge {

> +	int n_sensors;

> +	struct sensor sensors[MAX_CONNECTED_DEVICES];

> +	struct pci_dev *cio2;

> +	struct fwnode_handle *cio2_fwnode;

> +};

> +

> +struct cio2_bridge bridge = { 0, };

> +

> +static const struct property_entry remote_endpoints[] = {

> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */

> +			   &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */

> +			   &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),

> +	PROPERTY_ENTRY_REF("remote-endpoint",

> +			   &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),

> +	{ }

> +};

> +

> +/* Data representation as it is in ACPI SSDB buffer */

> +struct sensor_bios_data_packed {

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

> +} __attribute__((__packed__));

> +

> +/* Fields needed by bridge driver */

> +struct sensor_bios_data {

> +	struct device *dev;

> +	u8 link;

> +	u8 lanes;

> +	u32 mclkspeed;

> +};

> +

> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)

> +{

> +	union acpi_object *obj;

> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

> +	struct acpi_handle *dev_handle = ACPI_HANDLE(dev);

> +	int status;

> +	u32 buffer_length;

> +

> +	status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);

> +	if (!ACPI_SUCCESS(status))

> +		return -ENODEV;

> +

> +	obj = (union acpi_object *)buffer.pointer;

> +	if (!obj || obj->type != ACPI_TYPE_BUFFER) {

> +		dev_err(dev, "Could't read acpi buffer\n");

> +		status = -ENODEV;

> +		goto err;

> +	}

> +

> +	if (obj->buffer.length > size) {

> +		dev_err(dev, "Given buffer is too small\n");

> +		status = -ENODEV;

> +		goto err;

> +	}

> +

> +	memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));

> +	buffer_length = obj->buffer.length;

> +	kfree(buffer.pointer);

> +

> +	return buffer_length;

> +err:

> +	kfree(buffer.pointer);

> +	return status;

> +}

> +

> +static int get_acpi_ssdb_sensor_data(struct device *dev,

> +				     struct sensor_bios_data *sensor)

> +{

> +	struct sensor_bios_data_packed sensor_data;

> +	int ret = read_acpi_block(dev, "SSDB", &sensor_data,

> +				  sizeof(sensor_data));

> +

> +	if (ret < 0) {

> +		dev_err(dev, "Failed to fetch SSDB data\n");

> +		return ret;

> +	}

> +

> +	sensor->link = sensor_data.link;

> +	sensor->lanes = sensor_data.lanes;

> +	sensor->mclkspeed = sensor_data.mclkspeed;

> +

> +	return 0;

> +}

> +

> +static int create_endpoint_properties(struct device *dev,

> +				      struct sensor_bios_data *ssdb,

> +				      struct property_entry *sensor_props,

> +				      struct property_entry *cio2_props)

> +{

> +		u32 *data_lanes;


Indentation.

> +		int i;

> +

> +		data_lanes = devm_kmalloc(dev, sizeof(u32) * (int)ssdb->lanes,

> +					  GFP_KERNEL);

> +

> +		if (!data_lanes) {

> +			dev_err(dev,

> +				"Couldn't allocate memory for data lanes array\n");

> +			return -ENOMEM;

> +		}

> +

> +		for (i = 0; i < (int)ssdb->lanes; i++)

> +			data_lanes[i] = (u32)i + 1;

> +

> +		sensor_props[0] = PROPERTY_ENTRY_U32("clock-frequency",

> +						     ssdb->mclkspeed);

> +		sensor_props[1] = PROPERTY_ENTRY_U32("bus-type", 5);

> +		sensor_props[2] = PROPERTY_ENTRY_U32("clock-lanes", 0);


This isn't needed on sensors in practice.

> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

> +							       data_lanes,

> +							       (int)ssdb->lanes);

> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];

> +		sensor_props[5] = PROPERTY_ENTRY_NULL;

> +

> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

> +							     data_lanes,

> +							     (int)ssdb->lanes);

> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];

> +		cio2_props[2] = PROPERTY_ENTRY_NULL;


I suppose the CSI-2 link frequency is generally encoded in drivers in this
case. A lot of drivers already check for those, could you add the
frequencies here as well (as they are known)?

> +

> +		return 0;

> +}

> +

> +static int connect_supported_devices(void)

> +{

> +	struct acpi_device *adev;

> +	struct device *dev;

> +	struct sensor_bios_data ssdb;

> +	struct sensor *sensor;

> +	struct property_entry *sensor_props;

> +	struct property_entry *cio2_props;

> +	struct fwnode_handle *fwnode;

> +	struct software_node *nodes;

> +	struct v4l2_subdev *sd;

> +	int i, ret;


unsigned int i

> +

> +	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {

> +		adev = acpi_dev_get_first_match_dev(supported_devices[i],

> +						    NULL, -1);

> +

> +		if (!adev)

> +			continue;

> +

> +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);

> +

> +		if (!dev) {

> +			pr_info("ACPI match for %s, but it has no i2c device\n",

> +				supported_devices[i]);

> +			continue;

> +		}

> +

> +		if (!dev->driver_data) {

> +			pr_info("ACPI match for %s, but it has no driver\n",

> +				supported_devices[i]);

> +			continue;

> +		} else {

> +			pr_info("Found supported device %s\n",

> +				supported_devices[i]);

> +		}

> +

> +		sensor = &bridge.sensors[bridge.n_sensors];

> +		/*

> +		 * Store sensor's existing fwnode so that it can be restored if

> +		 * this module is removed.

> +		 */

> +		sensor->fwnode = fwnode_handle_get(dev->fwnode);

> +

> +		get_acpi_ssdb_sensor_data(dev, &ssdb);

> +

> +		nodes = sensor->swnodes;

> +		sensor_props = sensor->sensor_props;

> +		cio2_props = sensor->cio2_props;

> +		fwnode = sensor->fwnode;

> +

> +		ret = create_endpoint_properties(dev, &ssdb, sensor_props,

> +						 cio2_props);

> +

> +		if (ret)

> +			return ret;

> +

> +		/* build the software nodes */

> +

> +		nodes[SWNODE_SENSOR_HID] = NODE_HID(supported_devices[i]);

> +		nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0",

> +						      &nodes[SWNODE_SENSOR_HID]);

> +		nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0",

> +							      &nodes[SWNODE_SENSOR_PORT],

> +							      sensor_props);

> +		nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[(int)ssdb.link],

> +						    &cio2_hid_node);

> +		nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0",

> +							    &nodes[SWNODE_CIO2_PORT],

> +							    cio2_props);

> +		nodes[SWNODE_NULL_TERMINATOR]   = SOFTWARE_NODE_NULL;

> +

> +		ret = software_node_register_nodes(nodes);

> +		if (ret) {

> +			dev_err(dev,

> +				"Failed to register software nodes for %s\n",

> +				supported_devices[i]);

> +			return ret;

> +		}

> +

> +		fwnode = software_node_fwnode(&nodes[SWNODE_SENSOR_HID]);

> +		if (!fwnode) {

> +			dev_err(dev,

> +				"Failed to get software node for %s\n",

> +				supported_devices[i]);

> +			return ret;

> +		}

> +

> +		fwnode->secondary = ERR_PTR(-ENODEV);

> +		dev->fwnode = fwnode;

> +

> +		/*

> +		 * The device should by this point has driver_data set to an

> +		 * instance of struct v4l2_subdev; set the fwnode for that too.

> +		 */

> +

> +		sd = dev_get_drvdata(dev);

> +		sd->fwnode = fwnode;


I'm a bit lost here. Isn't it enough to have the sensor device's fwnode,
and to use that for V4L2 async fwnode matching (as usual)?

> +

> +		sensor->dev = dev;

> +		bridge.n_sensors++;

> +	}

> +

> +	return 0;

> +}

> +

> +static int cio2_bridge_init(void)

> +{

> +	struct fwnode_handle *fwnode;

> +	int ret;

> +

> +	ret = software_node_register(&cio2_hid_node);

> +

> +	if (ret < 0) {

> +		pr_err("Failed to register the CIO2 HID node\n");

> +		return -EINVAL;

> +	}

> +

> +	ret = connect_supported_devices();

> +

> +	if ((ret < 0) || (bridge.n_sensors <= 0)) {

> +		pr_err("cio2_bridge: Failed to connect any devices\n");

> +		goto out;

> +	} else {

> +		pr_info("Found %d supported devices\n", bridge.n_sensors);

> +	}

> +

> +	bridge.cio2 = pci_get_device(PCI_VENDOR_ID_INTEL, CIO2_PCI_ID, NULL);

> +	if (!bridge.cio2) {

> +		ret = -ENODEV;

> +		goto out;

> +	}

> +

> +	fwnode = software_node_fwnode(&cio2_hid_node);

> +	if (!fwnode) {

> +		pr_err("Error getting fwnode from cio2 software_node\n");

> +		ret = -ENODEV;

> +		goto out;

> +	}

> +

> +	/*

> +	 * We store the pci_dev's existing fwnode, beccause in the event we

> +	 * want to reload (I.E. rmmod and insmod) this module we need to give

> +	 * the device its original fwnode back to prevent problems down the

> +	 * line

> +	 */

> +

> +	bridge.cio2_fwnode = fwnode_handle_get(bridge.cio2->dev.fwnode);

> +

> +	fwnode->secondary = ERR_PTR(-ENODEV);

> +	bridge.cio2->dev.fwnode = fwnode;

> +

> +	return 0;

> +out:

> +	cio2_bridge_exit();

> +	return ret;

> +}

> +

> +static int cio2_bridge_unregister_sensors(void)

> +{

> +	int i, j;

> +	struct sensor *sensor;

> +

> +	for (i = 0; i < bridge.n_sensors; i++) {

> +		sensor = &bridge.sensors[i];

> +

> +		/* give the sensor its original fwnode back */

> +		sensor->dev->fwnode = sensor->fwnode;

> +		fwnode_handle_put(sensor->fwnode);

> +		put_device(sensor->dev);

> +

> +		for (j = 4; j >= 0; j--)

> +			software_node_unregister(&sensor->swnodes[j]);

> +	}

> +

> +	return 0;

> +}

> +

> +static void cio2_bridge_exit(void)

> +{

> +	int ret;

> +

> +	/* Give the pci_dev its original fwnode back */

> +	if (bridge.cio2) {

> +		bridge.cio2->dev.fwnode = bridge.cio2_fwnode;

> +		fwnode_handle_put(bridge.cio2_fwnode);

> +		pci_dev_put(bridge.cio2);

> +	}

> +

> +	ret = cio2_bridge_unregister_sensors();

> +

> +	if (ret)

> +		pr_err("An error occurred unregistering the sensors\n");

> +

> +	software_node_unregister(&cio2_hid_node);

> +}

> +

> +module_init(cio2_bridge_init);

> +module_exit(cio2_bridge_exit);

> +

> +MODULE_DESCRIPTION("A bridge driver to connect sensors to CIO2 infrastructure.");

> +MODULE_LICENSE("GPL v2");

> +MODULE_ALIAS("acpi*:INT343E:*");


-- 
Kind regards,

Sakari Ailus

On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> > +static int connect_supported_devices(void)
> > +{
> > +	struct acpi_device *adev;
> > +	struct device *dev;
> > +	struct sensor_bios_data ssdb;
> > +	struct sensor *sensor;
> > +	struct property_entry *sensor_props;
> > +	struct property_entry *cio2_props;
> > +	struct fwnode_handle *fwnode;
> > +	struct software_node *nodes;
> > +	struct v4l2_subdev *sd;
> > +	int i, ret;
> 
> unsigned int i
> 

Why?

For list iterators then "int i;" is best...  For sizes then unsigned is
sometimes best.  Or if it's part of the hardware spec or network spec
unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
They're not as intuitive as signed variables.  Imagine if there is an
error in this loop and you want to unwind.  With a signed variable you
can do:

	while (--i >= 0)
		cleanup(&bridge.sensors[i]);

There are very few times where raising the type maximum from 2 billion
to 4 billion fixes anything.

regards,
dan carpenter

Hey Sakari - thanks for the reply

On 17/09/2020 11:33, Sakari Ailus wrote:
> Hi Daniel,

>

> Thank you for the patch.

>

> Is this all that it takes to add support for some machines shipped with

> Windows?

Almost
> The ones I know require PMIC control done in software (not even

> sensors are accessible without that).

So far we've just been getting the sensor drivers themselves to toggle
the gpio pins that turn the PMIC on (those pins are listed against the
PMIC's _CRS, and we've been finding those by evaluating the sensor's
_DEP) - once that's done the cameras show up on i2c and,with the bridge
driver installed, you can use libcamera to take photos. That's been
confusing me a bit; I think I mentioned before that I couldn't figure
out how the clocks and regulators could be working in that case. But
I've had a bunch of people test this now on about 5 different machines
(Surface devices and similar) and it seems to "just work"
> One possibility would be to put this to platform code. That would

> effectively also require it's compiled to the kernel (yuck).

>

> How about just squashing this to the CIO2 driver instead (but still as a

> separate file)? It's not exactly pretty, no, but it could allow this being

> a module and not enlarge everyone's kernel, and the initialisation would at

> the same time take place before the rest of what the CIO2 driver does in

> probe.

I thought of that as well, but wasn't sure which was preferable. I can
compress it into the CIO2 driver though sure.
> I think you should still check whether CIO2 has graph endpoints before

> proceeding with parsing SSDB buffer or looking up random-looking devices.

Yeah; I was under the impression this wasn't working at all on ACPI
platforms - if it actually is on some then I guess checking for
endpoints before making an attempt to parse SSDB to build them is the
way to go?
>

> Cc Andy, too.

>

> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:

>> Currently on ACPI platforms, sensors that are intended to be connected to

>> a CIO2 device for use with the ipu3-cio2 driver lack the necessary

>> connection information in firmware. This patch adds a module to parse the

>> connection properties from the SSDB buffer in DSDT and build the connection

>> using software nodes.

>>

>> The ipu3-cio2 driver itself is modified to insert the cio2-bridge module

>> after all sensors that have created a device link between themselves and

>> the CIO2 have probed. Sensors wishing to use this bridge will need to add

>> a device link between themselves and the CIO2 device as part of their own

>> .probe() call.

>>

>> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>

>>

>> Signed-off-by: Daniel Scally <djrscally@gmail.com>

>> ---

>> This module's born out of efforts by the linux-surface github community

>> to get functioning webcams on Microsoft Surface and similar platforms. it

>> is dependent on this patch (which implements the software node graph family

>> of functions):

>>

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

>>

>> I wanted to raise this as an RFC as although I don't think it's ready for

>> integration it has some things that I'd like feedback on, in particular the

>> method I chose to make the module be auto-inserted. A more ideal method would

>> have been to have the driver be an ACPI driver for the INT343E device, but each

> What do you think this device does represent? Devices whose status is

> always zero may exist in the table even if they would not be actually

> present.

>

> CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not

> have one.

This is the ACPI entry I mean:

Device (CIO2)
{
    Method (_STA, 0, NotSerialized)  // _STA: Status
    {
        If ((CIOE == One))
        {
            Return (0x0F)
        }
        Else
        {
            Return (Zero)
        }
    }

    Name (_HID, "INT343E")  // _HID: Hardware ID
    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
    {
        Name (CBUF, ResourceTemplate ()
        {
            Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, _Y15)
            {
                0x00000010,
            }
            Memory32Fixed (ReadWrite,
                0xFE400000,         // Address Base
                0x00010000,         // Address Length
                )
        })
        CreateDWordField (CBUF, \_SB.PCI0.CIO2._CRS._Y15._INT, CIOV)  // _INT: Interrupts
        CIOV = CIOI /* \CIOI */
        Return (CBUF) /* \_SB_.PCI0.CIO2._CRS.CBUF */
    }
}

>> of the the devices we've tested this on that dev has status 0 and so the module

>> won't bind to it. The device links method seems a little clunky, but does work,

>> and I think I have done the conditional processing correctly so that ipu3-cio2

>> continues to work on non-ACPI platforms.

> I don't think anyone uses ipu3-cio2 driver on non-ACPI platforms. It really

> does require ACPI.

Oh - I've been misunderstanding that then. In that case the CONFIG_ACPI
checks can go but I need to be checking for existing endpoints from APCI
fwnodes I think
>>  MAINTAINERS                              |   6 +

>>  drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-

>>  drivers/staging/media/ipu3/Kconfig       |  15 +

>>  drivers/staging/media/ipu3/Makefile      |   1 +

>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++

>>  5 files changed, 534 insertions(+), 3 deletions(-)

>>  create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c

>>

>> diff --git a/MAINTAINERS b/MAINTAINERS

>> index b5cfab015bd6..55b0b9888bc0 100644

>> --- a/MAINTAINERS

>> +++ b/MAINTAINERS

>> @@ -9152,6 +9152,12 @@ S:	Maintained

>>  W:	http://www.adaptec.com/

>>  F:	drivers/scsi/ips*

>>  

>> +IPU3 CIO2 Bridge Driver

>> +M:	Daniel Scally <djrscally@gmail.com>

>> +L:	linux-media@vger.kernel.org

>> +S:	Maintained

>> +F:	drivers/staging/media/ipu3/cio2-bridge.c

>> +

>>  IPVS

>>  M:	Wensong Zhang <wensong@linux-vs.org>

>>  M:	Simon Horman <horms@verge.net.au>

>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> index 92f5eadf2c99..fd941d2c7581 100644

>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)

>>  		cio2_queue_exit(cio2, &cio2->queue[i]);

>>  }

>>  

>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)

>> +{

>> +	void *sensor;

>> +

>> +	/*

>> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect

>> +	 * to cio2 have added a device link. If there are no consumers yet, then

>> +	 * we need to defer. The .sync_state() callback will then be called after

>> +	 * all linked sensors have probed

>> +	 */

>> +

>> +	if (IS_ENABLED(CONFIG_ACPI)) {

>> +		sensor = (struct device *)list_first_entry_or_null(

>> +								&pci_dev->dev.links.consumers,

>> +								struct dev_links_info,

>> +								consumers);

> Please wrap so it's under 80.

>

Will do
>> +

>> +		if (!sensor)

>> +			return -EPROBE_DEFER;

>> +	}

>> +

>> +	return 0;

>> +}

>> +

>> +void cio2_sync_state(struct device *dev)

>> +{

>> +	struct cio2_device *cio2;

>> +	int ret = 0;

>> +

>> +	if (IS_ENABLED(CONFIG_ACPI)) {

>> +		cio2 = dev_get_drvdata(dev);

>> +

>> +		if (!cio2) {

>> +			dev_err(dev, "Failed to retrieve driver data\n");

>> +			return;

>> +		}

>> +

>> +		/* insert the bridge driver to connect sensors via software nodes */

>> +		ret = request_module("cio2-bridge");

>> +

>> +		if (ret)

>> +			dev_err(dev, "Failed to insert cio2-bridge\n");

>> +

>> +		ret = cio2_parse_firmware(cio2);

>> +

>> +		if (ret) {

>> +			v4l2_async_notifier_unregister(&cio2->notifier);

>> +			v4l2_async_notifier_cleanup(&cio2->notifier);

>> +			cio2_queues_exit(cio2);

>> +		}

>> +	}

>> +}

>> +

>>  /**************** PCI interface ****************/

>>  

>>  static int cio2_pci_config_setup(struct pci_dev *dev)

>> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,

>>  	void __iomem *const *iomap;

>>  	int r;

>>  

>> +	r = cio2_probe_can_progress(pci_dev);

>> +

>> +	if (r)

>> +		return -EPROBE_DEFER;

>> +

>>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);

>>  	if (!cio2)

>>  		return -ENOMEM;

>> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,

>>  	v4l2_async_notifier_init(&cio2->notifier);

>>  

>>  	/* Register notifier for subdevices we care */

>> -	r = cio2_parse_firmware(cio2);

>> -	if (r)

>> -		goto fail_clean_notifier;

>> +	if (!IS_ENABLED(CONFIG_ACPI)) {

>> +		r = cio2_parse_firmware(cio2);

>> +		if (r)

>> +			goto fail_clean_notifier;

>> +	}

>>  

>>  	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,

>>  			     IRQF_SHARED, CIO2_NAME, cio2);

>> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {

>>  	.remove = cio2_pci_remove,

>>  	.driver = {

>>  		.pm = &cio2_pm_ops,

>> +		.sync_state = cio2_sync_state

>>  	},

>>  };

>>  

>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig

>> index 3e9640523e50..08842fd8c0da 100644

>> --- a/drivers/staging/media/ipu3/Kconfig

>> +++ b/drivers/staging/media/ipu3/Kconfig

>> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU

>>  

>>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI

>>  	  camera. The module will be called ipu3-imgu.

>> +

>> +config VIDEO_CIO2_BRIDGE

>> +	tristate "IPU3 CIO2 Sensor Bridge Driver"

>> +	depends on PCI && VIDEO_V4L2

>> +	depends on ACPI

>> +	depends on X86

>> +	help

>> +	  This module provides a bridge connecting sensors (I.E. cameras) to

>> +	  the CIO2 device infrastructure via software nodes built from information

>> +	  parsed from the SSDB buffer.

>> +

>> +	  Say Y or M here if your platform's cameras use IPU3 with connections

>> +	  that should be defined in ACPI. The module will be called cio2-bridge.

>> +

>> +	  If in doubt, say N here.

>> \ No newline at end of file

>> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile

>> index 9def80ef28f3..12dff56dbb9e 100644

>> --- a/drivers/staging/media/ipu3/Makefile

>> +++ b/drivers/staging/media/ipu3/Makefile

>> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \

>>  		ipu3-css.o ipu3-v4l2.o ipu3.o

>>  

>>  obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o

>> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o

>> \ No newline at end of file

>> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c

>> new file mode 100644

>> index 000000000000..5115aeeb35a1

>> --- /dev/null

>> +++ b/drivers/staging/media/ipu3/cio2-bridge.c

>> @@ -0,0 +1,448 @@

>> +// SPDX-License-Identifier: GPL-2.0

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

>> +#include <acpi/acpi_bus.h>

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

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

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

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

>> +#include <linux/pci.h>

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

>> +

>> +#include <linux/fwnode.h>

>> +#include <linux/kref.h>

>> +

>> +static void cio2_bridge_exit(void);

>> +static int cio2_bridge_init(void);

>> +

>> +#define MAX_CONNECTED_DEVICES			4

>> +#define SWNODE_SENSOR_HID			0

>> +#define SWNODE_SENSOR_PORT			1

>> +#define SWNODE_SENSOR_ENDPOINT			2

>> +#define SWNODE_CIO2_PORT			3

>> +#define SWNODE_CIO2_ENDPOINT			4

>> +#define SWNODE_NULL_TERMINATOR			5

>> +

>> +#define CIO2_HID				"INT343E"

>> +#define CIO2_PCI_ID				0x9d32

>> +

>> +#define ENDPOINT_SENSOR				0

>> +#define ENDPOINT_CIO2				1

>> +

>> +#define NODE_HID(_HID)				\

>> +((const struct software_node) {			\

>> +	_HID,					\

>> +})

>> +

>> +#define NODE_PORT(_PORT, _HID_NODE)		\

>> +((const struct software_node) {			\

>> +	_PORT,					\

>> +	_HID_NODE,				\

>> +})

>> +

>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\

>> +((const struct software_node) {			\

>> +	_EP,					\

>> +	_PORT,					\

>> +	_PROPS,					\

>> +})

>> +

>> +#define PROPERTY_ENTRY_NULL			\

>> +((const struct property_entry) { })

> Alignment. Same appears to apply to other macros (please indent).

Yep
>

>> +#define SOFTWARE_NODE_NULL			\

>> +((const struct software_node) { })

>> +

>> +/*

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

>> + * working

>> + */

>> +

>> +static char *supported_devices[] = {

>> +	"INT33BE",

>> +	"OVTI2680",

>> +	"OVTI5648",

>> +};

>> +

>> +/*

>> + * software_node needs const char * names. Can't snprintf a const char *,

>> + * so instead we need an array of them and use the port num from SSDB as

>> + * an index.

>> + */

>> +

>> +const char *port_names[] = {

>> +	"port0", "port1", "port2", "port3", "port4",

>> +	"port5", "port6", "port7", "port8", "port9"

> I think CIO2 is limited to 4.

Yep - forgot to drop that after I checked the max
>> +};

>> +

>> +struct software_node cio2_hid_node = { CIO2_HID, };

>> +

>> +struct sensor {

>> +	struct device *dev;

>> +	struct software_node swnodes[5];

>> +	struct property_entry sensor_props[6];

>> +	struct property_entry cio2_props[3];

>> +	struct fwnode_handle *fwnode;

>> +};

>> +

>> +struct cio2_bridge {

>> +	int n_sensors;

>> +	struct sensor sensors[MAX_CONNECTED_DEVICES];

>> +	struct pci_dev *cio2;

>> +	struct fwnode_handle *cio2_fwnode;

>> +};

>> +

>> +struct cio2_bridge bridge = { 0, };

>> +

>> +static const struct property_entry remote_endpoints[] = {

>> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */

>> +			   &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */

>> +			   &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),

>> +	PROPERTY_ENTRY_REF("remote-endpoint",

>> +			   &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),

>> +	{ }

>> +};

>> +

>> +/* Data representation as it is in ACPI SSDB buffer */

>> +struct sensor_bios_data_packed {

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

>> +} __attribute__((__packed__));

>> +

>> +/* Fields needed by bridge driver */

>> +struct sensor_bios_data {

>> +	struct device *dev;

>> +	u8 link;

>> +	u8 lanes;

>> +	u32 mclkspeed;

>> +};

>> +

>> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)

>> +{

>> +	union acpi_object *obj;

>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

>> +	struct acpi_handle *dev_handle = ACPI_HANDLE(dev);

>> +	int status;

>> +	u32 buffer_length;

>> +

>> +	status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);

>> +	if (!ACPI_SUCCESS(status))

>> +		return -ENODEV;

>> +

>> +	obj = (union acpi_object *)buffer.pointer;

>> +	if (!obj || obj->type != ACPI_TYPE_BUFFER) {

>> +		dev_err(dev, "Could't read acpi buffer\n");

>> +		status = -ENODEV;

>> +		goto err;

>> +	}

>> +

>> +	if (obj->buffer.length > size) {

>> +		dev_err(dev, "Given buffer is too small\n");

>> +		status = -ENODEV;

>> +		goto err;

>> +	}

>> +

>> +	memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));

>> +	buffer_length = obj->buffer.length;

>> +	kfree(buffer.pointer);

>> +

>> +	return buffer_length;

>> +err:

>> +	kfree(buffer.pointer);

>> +	return status;

>> +}

>> +

>> +static int get_acpi_ssdb_sensor_data(struct device *dev,

>> +				     struct sensor_bios_data *sensor)

>> +{

>> +	struct sensor_bios_data_packed sensor_data;

>> +	int ret = read_acpi_block(dev, "SSDB", &sensor_data,

>> +				  sizeof(sensor_data));

>> +

>> +	if (ret < 0) {

>> +		dev_err(dev, "Failed to fetch SSDB data\n");

>> +		return ret;

>> +	}

>> +

>> +	sensor->link = sensor_data.link;

>> +	sensor->lanes = sensor_data.lanes;

>> +	sensor->mclkspeed = sensor_data.mclkspeed;

>> +

>> +	return 0;

>> +}

>> +

>> +static int create_endpoint_properties(struct device *dev,

>> +				      struct sensor_bios_data *ssdb,

>> +				      struct property_entry *sensor_props,

>> +				      struct property_entry *cio2_props)

>> +{

>> +		u32 *data_lanes;

> Indentation.

Will fix
>

>> +		int i;

>> +

>> +		data_lanes = devm_kmalloc(dev, sizeof(u32) * (int)ssdb->lanes,

>> +					  GFP_KERNEL);

>> +

>> +		if (!data_lanes) {

>> +			dev_err(dev,

>> +				"Couldn't allocate memory for data lanes array\n");

>> +			return -ENOMEM;

>> +		}

>> +

>> +		for (i = 0; i < (int)ssdb->lanes; i++)

>> +			data_lanes[i] = (u32)i + 1;

>> +

>> +		sensor_props[0] = PROPERTY_ENTRY_U32("clock-frequency",

>> +						     ssdb->mclkspeed);

>> +		sensor_props[1] = PROPERTY_ENTRY_U32("bus-type", 5);

>> +		sensor_props[2] = PROPERTY_ENTRY_U32("clock-lanes", 0);

> This isn't needed on sensors in practice.

Just clock-lanes you mean? or all 3 of those entries?
>

>> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

>> +							       data_lanes,

>> +							       (int)ssdb->lanes);

>> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];

>> +		sensor_props[5] = PROPERTY_ENTRY_NULL;

>> +

>> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

>> +							     data_lanes,

>> +							     (int)ssdb->lanes);

>> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];

>> +		cio2_props[2] = PROPERTY_ENTRY_NULL;

> I suppose the CSI-2 link frequency is generally encoded in drivers in this

> case. A lot of drivers already check for those, could you add the

> frequencies here as well (as they are known)?

Yes, I also added (after I sent this in...) the camera rotation from SSDB.
>> +

>> +		return 0;

>> +}

>> +

>> +static int connect_supported_devices(void)

>> +{

>> +	struct acpi_device *adev;

>> +	struct device *dev;

>> +	struct sensor_bios_data ssdb;

>> +	struct sensor *sensor;

>> +	struct property_entry *sensor_props;

>> +	struct property_entry *cio2_props;

>> +	struct fwnode_handle *fwnode;

>> +	struct software_node *nodes;

>> +	struct v4l2_subdev *sd;

>> +	int i, ret;

> unsigned int i

Thank you
>> +

>> +	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {

>> +		adev = acpi_dev_get_first_match_dev(supported_devices[i],

>> +						    NULL, -1);

>> +

>> +		if (!adev)

>> +			continue;

>> +

>> +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);

>> +

>> +		if (!dev) {

>> +			pr_info("ACPI match for %s, but it has no i2c device\n",

>> +				supported_devices[i]);

>> +			continue;

>> +		}

>> +

>> +		if (!dev->driver_data) {

>> +			pr_info("ACPI match for %s, but it has no driver\n",

>> +				supported_devices[i]);

>> +			continue;

>> +		} else {

>> +			pr_info("Found supported device %s\n",

>> +				supported_devices[i]);

>> +		}

>> +

>> +		sensor = &bridge.sensors[bridge.n_sensors];

>> +		/*

>> +		 * Store sensor's existing fwnode so that it can be restored if

>> +		 * this module is removed.

>> +		 */

>> +		sensor->fwnode = fwnode_handle_get(dev->fwnode);

>> +

>> +		get_acpi_ssdb_sensor_data(dev, &ssdb);

>> +

>> +		nodes = sensor->swnodes;

>> +		sensor_props = sensor->sensor_props;

>> +		cio2_props = sensor->cio2_props;

>> +		fwnode = sensor->fwnode;

>> +

>> +		ret = create_endpoint_properties(dev, &ssdb, sensor_props,

>> +						 cio2_props);

>> +

>> +		if (ret)

>> +			return ret;

>> +

>> +		/* build the software nodes */

>> +

>> +		nodes[SWNODE_SENSOR_HID] = NODE_HID(supported_devices[i]);

>> +		nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0",

>> +						      &nodes[SWNODE_SENSOR_HID]);

>> +		nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0",

>> +							      &nodes[SWNODE_SENSOR_PORT],

>> +							      sensor_props);

>> +		nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[(int)ssdb.link],

>> +						    &cio2_hid_node);

>> +		nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0",

>> +							    &nodes[SWNODE_CIO2_PORT],

>> +							    cio2_props);

>> +		nodes[SWNODE_NULL_TERMINATOR]   = SOFTWARE_NODE_NULL;

>> +

>> +		ret = software_node_register_nodes(nodes);

>> +		if (ret) {

>> +			dev_err(dev,

>> +				"Failed to register software nodes for %s\n",

>> +				supported_devices[i]);

>> +			return ret;

>> +		}

>> +

>> +		fwnode = software_node_fwnode(&nodes[SWNODE_SENSOR_HID]);

>> +		if (!fwnode) {

>> +			dev_err(dev,

>> +				"Failed to get software node for %s\n",

>> +				supported_devices[i]);

>> +			return ret;

>> +		}

>> +

>> +		fwnode->secondary = ERR_PTR(-ENODEV);

>> +		dev->fwnode = fwnode;

>> +

>> +		/*

>> +		 * The device should by this point has driver_data set to an

>> +		 * instance of struct v4l2_subdev; set the fwnode for that too.

>> +		 */

>> +

>> +		sd = dev_get_drvdata(dev);

>> +		sd->fwnode = fwnode;

> I'm a bit lost here. Isn't it enough to have the sensor device's fwnode,

> and to use that for V4L2 async fwnode matching (as usual)?

I'll double check that - I think I had decided it wasn't working without
both set, but I could very easily be wrong about that
>> +

>> +		sensor->dev = dev;

>> +		bridge.n_sensors++;

>> +	}

>> +

>> +	return 0;

>> +}

>> +

>> +static int cio2_bridge_init(void)

>> +{

>> +	struct fwnode_handle *fwnode;

>> +	int ret;

>> +

>> +	ret = software_node_register(&cio2_hid_node);

>> +

>> +	if (ret < 0) {

>> +		pr_err("Failed to register the CIO2 HID node\n");

>> +		return -EINVAL;

>> +	}

>> +

>> +	ret = connect_supported_devices();

>> +

>> +	if ((ret < 0) || (bridge.n_sensors <= 0)) {

>> +		pr_err("cio2_bridge: Failed to connect any devices\n");

>> +		goto out;

>> +	} else {

>> +		pr_info("Found %d supported devices\n", bridge.n_sensors);

>> +	}

>> +

>> +	bridge.cio2 = pci_get_device(PCI_VENDOR_ID_INTEL, CIO2_PCI_ID, NULL);

>> +	if (!bridge.cio2) {

>> +		ret = -ENODEV;

>> +		goto out;

>> +	}

>> +

>> +	fwnode = software_node_fwnode(&cio2_hid_node);

>> +	if (!fwnode) {

>> +		pr_err("Error getting fwnode from cio2 software_node\n");

>> +		ret = -ENODEV;

>> +		goto out;

>> +	}

>> +

>> +	/*

>> +	 * We store the pci_dev's existing fwnode, beccause in the event we

>> +	 * want to reload (I.E. rmmod and insmod) this module we need to give

>> +	 * the device its original fwnode back to prevent problems down the

>> +	 * line

>> +	 */

>> +

>> +	bridge.cio2_fwnode = fwnode_handle_get(bridge.cio2->dev.fwnode);

>> +

>> +	fwnode->secondary = ERR_PTR(-ENODEV);

>> +	bridge.cio2->dev.fwnode = fwnode;

>> +

>> +	return 0;

>> +out:

>> +	cio2_bridge_exit();

>> +	return ret;

>> +}

>> +

>> +static int cio2_bridge_unregister_sensors(void)

>> +{

>> +	int i, j;

>> +	struct sensor *sensor;

>> +

>> +	for (i = 0; i < bridge.n_sensors; i++) {

>> +		sensor = &bridge.sensors[i];

>> +

>> +		/* give the sensor its original fwnode back */

>> +		sensor->dev->fwnode = sensor->fwnode;

>> +		fwnode_handle_put(sensor->fwnode);

>> +		put_device(sensor->dev);

>> +

>> +		for (j = 4; j >= 0; j--)

>> +			software_node_unregister(&sensor->swnodes[j]);

>> +	}

>> +

>> +	return 0;

>> +}

>> +

>> +static void cio2_bridge_exit(void)

>> +{

>> +	int ret;

>> +

>> +	/* Give the pci_dev its original fwnode back */

>> +	if (bridge.cio2) {

>> +		bridge.cio2->dev.fwnode = bridge.cio2_fwnode;

>> +		fwnode_handle_put(bridge.cio2_fwnode);

>> +		pci_dev_put(bridge.cio2);

>> +	}

>> +

>> +	ret = cio2_bridge_unregister_sensors();

>> +

>> +	if (ret)

>> +		pr_err("An error occurred unregistering the sensors\n");

>> +

>> +	software_node_unregister(&cio2_hid_node);

>> +}

>> +

>> +module_init(cio2_bridge_init);

>> +module_exit(cio2_bridge_exit);

>> +

>> +MODULE_DESCRIPTION("A bridge driver to connect sensors to CIO2 infrastructure.");

>> +MODULE_LICENSE("GPL v2");

>> +MODULE_ALIAS("acpi*:INT343E:*");

On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:


> > > +	int i, ret;

> > 

> > unsigned int i

> > 

> 

> Why?

> 

> For list iterators then "int i;" is best...  For sizes then unsigned is

> sometimes best.  Or if it's part of the hardware spec or network spec

> unsigned is best.  Otherwise unsigned variables cause a ton of bugs.

> They're not as intuitive as signed variables.  Imagine if there is an

> error in this loop and you want to unwind.  With a signed variable you

> can do:

> 

> 	while (--i >= 0)

> 		cleanup(&bridge.sensors[i]);


Ha-ha. It's actually a counter argument to your stuff because above is the same as

	while (i--)
		cleanup(&bridge.sensors[i]);

with pretty much unsigned int i.

> There are very few times where raising the type maximum from 2 billion

> to 4 billion fixes anything.


-- 
With Best Regards,
Andy Shevchenko

On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> On 17/09/2020 11:33, Sakari Ailus wrote:

I will do better review for next version, assuming you will Cc reviewers and
TWIMC people. Below is like small part of comments I may give to the code.

...

> > The ones I know require PMIC control done in software (not even
> > sensors are accessible without that).
> So far we've just been getting the sensor drivers themselves to toggle
> the gpio pins that turn the PMIC on (those pins are listed against the
> PMIC's _CRS, and we've been finding those by evaluating the sensor's
> _DEP) - once that's done the cameras show up on i2c and,with the bridge
> driver installed, you can use libcamera to take photos.

Do I understand correctly that you are able to get pictures from the camera
hardware?

...

> > a module and not enlarge everyone's kernel, and the initialisation would at
> > the same time take place before the rest of what the CIO2 driver does in
> > probe.
> I thought of that as well, but wasn't sure which was preferable. I can
> compress it into the CIO2 driver though sure.

Sakari, I tend to agree with Dan and have the board file separated from the
driver and even framework.

...

> > Cc Andy, too.

Thanks!

...

> >> I wanted to raise this as an RFC as although I don't think it's ready for
> >> integration it has some things that I'd like feedback on, in particular the
> >> method I chose to make the module be auto-inserted. A more ideal method would
> >> have been to have the driver be an ACPI driver for the INT343E device, but each
> > What do you think this device does represent? Devices whose status is
> > always zero may exist in the table even if they would not be actually
> > present.
> >
> > CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not
> > have one.
> This is the ACPI entry I mean:
> 
> Device (CIO2)
> {
>     Method (_STA, 0, NotSerialized)  // _STA: Status
>     {
>         If ((CIOE == One))
>         {
>             Return (0x0F)
>         }
>         Else
>         {
>             Return (Zero)
>         }
>     }
> 
>     Name (_HID, "INT343E")  // _HID: Hardware ID
>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>     {
>         Name (CBUF, ResourceTemplate ()
>         {
>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, _Y15)
>             {
>                 0x00000010,
>             }
>             Memory32Fixed (ReadWrite,
>                 0xFE400000,         // Address Base
>                 0x00010000,         // Address Length
>                 )
>         })
>         CreateDWordField (CBUF, \_SB.PCI0.CIO2._CRS._Y15._INT, CIOV)  // _INT: Interrupts
>         CIOV = CIOI /* \CIOI */
>         Return (CBUF) /* \_SB_.PCI0.CIO2._CRS.CBUF */
>     }
> }

Ah, I think you misinterpreted the meaning of above. The above is a switch how
camera device appears either as PCI or an ACPI. So, it effectively means you
should *not* have any relation for this HID until you find a platform where the
device is for real enumerated via ACPI.

...

> >> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
> >> +{
> >> +	void *sensor;

Why void?

> >> +	/*
> >> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect
> >> +	 * to cio2 have added a device link. If there are no consumers yet, then
> >> +	 * we need to defer. The .sync_state() callback will then be called after
> >> +	 * all linked sensors have probed
> >> +	 */
> >> +
> >> +	if (IS_ENABLED(CONFIG_ACPI)) {

> >> +		sensor = (struct device *)list_first_entry_or_null(

Besides the fact that castings from or to void * are implicit in C, the proper
use of list API should have pretty well defined type of lvalue.

> >> +								&pci_dev->dev.links.consumers,
> >> +								struct dev_links_info,
> >> +								consumers);
> > Please wrap so it's under 80.
> >
> Will do
> >> +
> >> +		if (!sensor)
> >> +			return -EPROBE_DEFER;
> >> +	}
> >> +
> >> +	return 0;
> >> +}

...

> >> +	if (!IS_ENABLED(CONFIG_ACPI)) {
> >> +		r = cio2_parse_firmware(cio2);
> >> +		if (r)
> >> +			goto fail_clean_notifier;
> >> +	}

How comes?

...

> >> \ No newline at end of file

???

Be sure you are using good editor.

...

> >> +#include <acpi/acpi_bus.h>

Redundant. ACPI headers are designed the way that you are using a single header
in Linux kernel for all. It might be different in drivers/acpi stuff, but not
in general.

> >> +#include <linux/device.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <media/v4l2-subdev.h>
> >> +
> >> +#include <linux/fwnode.h>
> >> +#include <linux/kref.h>

Please, keep them sorted. And since it's for media, the media inclusion may be
placed last in a separate group.

...

> >> +#define PROPERTY_ENTRY_NULL			\
> >> +((const struct property_entry) { })
> > Alignment. Same appears to apply to other macros (please indent).
> Yep
> >
> >> +#define SOFTWARE_NODE_NULL			\
> >> +((const struct software_node) { })

Why?!

...

> >> +struct software_node cio2_hid_node = { CIO2_HID, };

static ?

Same for other global variables.

...

> >> +struct cio2_bridge bridge = { 0, };

When define as static the assignment will not be needed.

...

> >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)
> >> +{
> >> +	union acpi_object *obj;
> >> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

> >> +	struct acpi_handle *dev_handle = ACPI_HANDLE(dev);

Usually we use simple handle if there is no ambiguous reading.

> >> +	int status;

Should be acpi_status

> >> +	u32 buffer_length;
> >> +
> >> +	status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);

> >> +	if (!ACPI_SUCCESS(status))

ACPI_FAILURE()

> >> +		return -ENODEV;
> >> +
> >> +	obj = (union acpi_object *)buffer.pointer;

Why explicit casting?

> >> +	if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> >> +		dev_err(dev, "Could't read acpi buffer\n");

> >> +		status = -ENODEV;

Should have different int type variable for that.

> >> +		goto err;

If there is no obj, you may return directly without freeing.

> >> +	}
> >> +
> >> +	if (obj->buffer.length > size) {
> >> +		dev_err(dev, "Given buffer is too small\n");
> >> +		status = -ENODEV;
> >> +		goto err;
> >> +	}
> >> +
> >> +	memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));

Does type of size and length the same? Otherwise you need min_t().

> >> +	buffer_length = obj->buffer.length;
> >> +	kfree(buffer.pointer);
> >> +
> >> +	return buffer_length;

> >> +err:

Consider naming labels by what they are about to do. Like
	err_free:
here.

> >> +	kfree(buffer.pointer);
> >> +	return status;
> >> +}

> >> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> >> +				     struct sensor_bios_data *sensor)
> >> +{
> >> +	struct sensor_bios_data_packed sensor_data;

> >> +	int ret = read_acpi_block(dev, "SSDB", &sensor_data,
> >> +				  sizeof(sensor_data));

Please, split declaration and assignment especially in the cases where it
requires long lines.

> >> +	if (ret < 0) {
> >> +		dev_err(dev, "Failed to fetch SSDB data\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	sensor->link = sensor_data.link;
> >> +	sensor->lanes = sensor_data.lanes;
> >> +	sensor->mclkspeed = sensor_data.mclkspeed;
> >> +
> >> +	return 0;
> >> +}

...

> >> +		if (!dev->driver_data) {
> >> +			pr_info("ACPI match for %s, but it has no driver\n",
> >> +				supported_devices[i]);
> >> +			continue;
> >> +		} else {
> >> +			pr_info("Found supported device %s\n",
> >> +				supported_devices[i]);
> >> +		}

Positive conditions are easier to read, but on the other hand 'else' is
redundant in such conditionals (where if branch bails out from the flow).

On Thu, Sep 17, 2020 at 03:25:29PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> 
> > > > +	int i, ret;
> > > 
> > > unsigned int i
> > > 
> > 
> > Why?
> > 
> > For list iterators then "int i;" is best...  For sizes then unsigned is
> > sometimes best.  Or if it's part of the hardware spec or network spec
> > unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> > They're not as intuitive as signed variables.  Imagine if there is an
> > error in this loop and you want to unwind.  With a signed variable you
> > can do:
> > 
> > 	while (--i >= 0)
> > 		cleanup(&bridge.sensors[i]);
> 
> Ha-ha. It's actually a counter argument to your stuff because above is the same as
> 
> 	while (i--)
> 		cleanup(&bridge.sensors[i]);
> 
> with pretty much unsigned int i.

With vanilla "int i;" you can't go wrong because both styles work as
expected.  I was just giving examples of real life bugs that I have seen
involving unsigned iterators.

54313503f9a3 ("drm/amdgpu: signedness bug in amdgpu_cs_parser_init()")

Here are a couple more bugs involving unsigned iterators...

d72cf01f410a ("drm/mipi-dbi: fix a loop in debugfs code")
ce6c1cd2c324 ("pinctrl: nsp-gpio: forever loop in nsp_gpio_get_strength()")

I've change a lot of variables unsigned as well.  For min_t() then it
should *always* be an unsigned type.

It's pretty rare to iterate over 2 billion times in the kernel, but
there are times when you might want to do that.  Normally you would
want to declare the iterator as an unsigned ong in that case.  But most
of the time iterators should just be "int i;" to prevent bugs.

regards,
dan carpenter

Hi Dan, Greg,

On 17/09/2020 10:47, Dan Scally wrote:
> Hi Greg - thanks for the comments, appreciate it (sorry there's so many,
> I'm new to both C and kernel work)
> 
> On 17/09/2020 08:53, Greg KH wrote:
>> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
>>>  MAINTAINERS                              |   6 +
>>>  drivers/media/pci/intel/ipu3/ipu3-cio2.c |  67 +++-
>> staging drivers should be self-contained, and not modify stuff outside
>> of drivers/staging/
>>
>>>  drivers/staging/media/ipu3/Kconfig       |  15 +
>>>  drivers/staging/media/ipu3/Makefile      |   1 +
>>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++
>> Why does this have to be in drivers/staging/ at all?  Why not spend the
>> time to fix it up properly and get it merged correctly?  It's a very
>> small driver, and should be smaller, so it should not be a lot of work
>> to do.  And it would be faster to do that than to take it through
>> staging...
> I was just under the impression that that was the process to be honest,
> if that's not right I'll just move it directly to drivers/media/ipu3

The IPU3 driver is still in staging (unless I've missed something), so I
think this cio2-bridge should stay with it.

Hopefully with more users of the IPU3 brought in by this cio2-bridge,
that will help gather momentum to get the IPU3 developments required
completed and moved into drivers/media.

<snip>

Hi Andy, thanks for input (as always)

On 17/09/2020 13:45, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:

>> On 17/09/2020 11:33, Sakari Ailus wrote:

> I will do better review for next version, assuming you will Cc reviewers and

> TWIMC people. Below is like small part of comments I may give to the code.

TWIMC?
>>> The ones I know require PMIC control done in software (not even

>>> sensors are accessible without that).

>> So far we've just been getting the sensor drivers themselves to toggle

>> the gpio pins that turn the PMIC on (those pins are listed against the

>> PMIC's _CRS, and we've been finding those by evaluating the sensor's

>> _DEP) - once that's done the cameras show up on i2c and,with the bridge

>> driver installed, you can use libcamera to take photos.

> Do I understand correctly that you are able to get pictures from the camera

> hardware?


Yes, using the libcamera project's qcam program. They're poor quality at
the moment, because there's no auto-white-balance / exposure controls in
the ipu3 pipeline yet, but we can take images. Example:


https://user-images.githubusercontent.com/4592235/91197920-d1d41500-e6f3-11ea-8207-1c27cf24dd45.png


A bunch of folks have managed it so far on a couple different platforms
(Surface Book 1, Surface Pro something, an Acer A12 and a Lenovo Miix-510)

>>>> I wanted to raise this as an RFC as although I don't think it's ready for

>>>> integration it has some things that I'd like feedback on, in particular the

>>>> method I chose to make the module be auto-inserted. A more ideal method would

>>>> have been to have the driver be an ACPI driver for the INT343E device, but each

>>> What do you think this device does represent? Devices whose status is

>>> always zero may exist in the table even if they would not be actually

>>> present.

>>>

>>> CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not

>>> have one.

>> This is the ACPI entry I mean:

>>

>> Device (CIO2)

>> {

>>     Method (_STA, 0, NotSerialized)  // _STA: Status

>>     {

>>         If ((CIOE == One))

>>         {

>>             Return (0x0F)

>>         }

>>         Else

>>         {

>>             Return (Zero)

>>         }

>>     }

>>

>>     Name (_HID, "INT343E")  // _HID: Hardware ID

>>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings

>>     {

>>         Name (CBUF, ResourceTemplate ()

>>         {

>>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, _Y15)

>>             {

>>                 0x00000010,

>>             }

>>             Memory32Fixed (ReadWrite,

>>                 0xFE400000,         // Address Base

>>                 0x00010000,         // Address Length

>>                 )

>>         })

>>         CreateDWordField (CBUF, \_SB.PCI0.CIO2._CRS._Y15._INT, CIOV)  // _INT: Interrupts

>>         CIOV = CIOI /* \CIOI */

>>         Return (CBUF) /* \_SB_.PCI0.CIO2._CRS.CBUF */

>>     }

>> }

> Ah, I think you misinterpreted the meaning of above. The above is a switch how

> camera device appears either as PCI or an ACPI. So, it effectively means you

> should *not* have any relation for this HID until you find a platform where the

> device is for real enumerated via ACPI.

>

Ah, ok. So that was never going to work. Thanks. That does raise another
question; we have had some testers report failure, which turns out to be
because on their platforms the definition of their cameras in ACPI is
never translated into an i2c_client so the cio2-bridge doesn't bind.
Those have a similar conditional in the _STA method, see CAM1 in this
DSDT for example:
https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl.
Is there anything we can do to enable those cameras to be discovered too?

>>>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)

>>>> +{

>>>> +	void *sensor;

> Why void?

> Besides the fact that castings from or to void * are implicit in C, the proper

> use of list API should have pretty well defined type of lvalue.

>

Yeah, I misunderstood how this worked - after greg pointed out I was
doing it wrong I read the code a bit better and got it working assigning
to a struct device *sensor; - TIL.
>>>> +	if (!IS_ENABLED(CONFIG_ACPI)) {

>>>> +		r = cio2_parse_firmware(cio2);

>>>> +		if (r)

>>>> +			goto fail_clean_notifier;

>>>> +	}

> How comes?

Me misunderstanding again; it will be removed.
>

>>>> \ No newline at end of file

> ???

>

> Be sure you are using good editor.

>

Yeah haven't managed to track down what's causing this yet. Visual
Studio Code maybe.
>>>> +#define PROPERTY_ENTRY_NULL			\

>>>> +((const struct property_entry) { })

>>> Alignment. Same appears to apply to other macros (please indent).

>> Yep

>>>> +#define SOFTWARE_NODE_NULL			\

>>>> +((const struct software_node) { })

> Why?!

>

It felt ugly to have the other definitions be macros and not this one,
but I can change it.
>>>> +		return -ENODEV;

>>>> +

>>>> +	obj = (union acpi_object *)buffer.pointer;

> Why explicit casting?

>

>


>>>> +		if (!dev->driver_data) {

>>>> +			pr_info("ACPI match for %s, but it has no driver\n",

>>>> +				supported_devices[i]);

>>>> +			continue;

>>>> +		} else {

>>>> +			pr_info("Found supported device %s\n",

>>>> +				supported_devices[i]);

>>>> +		}

> Positive conditions are easier to read, but on the other hand 'else' is

> redundant in such conditionals (where if branch bails out from the flow).


Yeah good point - and much more readable that way. Thanks; I'll stick to
that in future.


All your other suggestions are great - thank you, I will fix them for
the v2.

On Thu, Sep 17, 2020 at 4:31 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> On 17/09/2020 10:47, Dan Scally wrote:

> > On 17/09/2020 08:53, Greg KH wrote:

> >> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:


> >>>  drivers/staging/media/ipu3/Kconfig       |  15 +

> >>>  drivers/staging/media/ipu3/Makefile      |   1 +

> >>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++

> >> Why does this have to be in drivers/staging/ at all?  Why not spend the

> >> time to fix it up properly and get it merged correctly?  It's a very

> >> small driver, and should be smaller, so it should not be a lot of work

> >> to do.  And it would be faster to do that than to take it through

> >> staging...

> > I was just under the impression that that was the process to be honest,

> > if that's not right I'll just move it directly to drivers/media/ipu3

>

> The IPU3 driver is still in staging (unless I've missed something), so I

> think this cio2-bridge should stay with it.


You missed something.
https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/pci/intel

IPU3 from Freescale (IIRC) is a different story.

> Hopefully with more users of the IPU3 brought in by this cio2-bridge,

> that will help gather momentum to get the IPU3 developments required

> completed and moved into drivers/media.


-- 
With Best Regards,
Andy Shevchenko

On Thu, Sep 17, 2020 at 4:53 PM Dan Scally <djrscally@gmail.com> wrote:
>

> Hi Andy, thanks for input (as always)


You're welcome! I'm really impressed by your activity in this area.

> On 17/09/2020 13:45, Andy Shevchenko wrote:

> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:

> >> On 17/09/2020 11:33, Sakari Ailus wrote:


To the point of placement, I think this should go under
drivers/platform/x86 (Adding Hans and Mark, who can express their
opinions).

...

> > Ah, I think you misinterpreted the meaning of above. The above is a switch how

> > camera device appears either as PCI or an ACPI. So, it effectively means you

> > should *not* have any relation for this HID until you find a platform where the

> > device is for real enumerated via ACPI.

> >

> Ah, ok. So that was never going to work. Thanks. That does raise another

> question; we have had some testers report failure, which turns out to be

> because on their platforms the definition of their cameras in ACPI is

> never translated into an i2c_client so the cio2-bridge doesn't bind.

> Those have a similar conditional in the _STA method, see CAM1 in this

> DSDT for example:

> https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl.

> Is there anything we can do to enable those cameras to be discovered too?


It means that this

...

> >>>> +#define PROPERTY_ENTRY_NULL                       \

> >>>> +((const struct property_entry) { })

> >>> Alignment. Same appears to apply to other macros (please indent).

> >> Yep

> >>>> +#define SOFTWARE_NODE_NULL                        \

> >>>> +((const struct software_node) { })

> > Why?!

> >

> It felt ugly to have the other definitions be macros and not this one,

> but I can change it.


My point is that those macros are simply redundant. The point is to
have a terminator record (all 0:s in the last entry of an array) which
is usually being achieved by allocating memory with kcalloc() which
does implicitly this for you.

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

On 17/09/2020 15:08, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 4:31 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>> On 17/09/2020 10:47, Dan Scally wrote:
>>> On 17/09/2020 08:53, Greg KH wrote:
>>>> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> 
>>>>>  drivers/staging/media/ipu3/Kconfig       |  15 +
>>>>>  drivers/staging/media/ipu3/Makefile      |   1 +
>>>>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++
>>>> Why does this have to be in drivers/staging/ at all?  Why not spend the
>>>> time to fix it up properly and get it merged correctly?  It's a very
>>>> small driver, and should be smaller, so it should not be a lot of work
>>>> to do.  And it would be faster to do that than to take it through
>>>> staging...
>>> I was just under the impression that that was the process to be honest,
>>> if that's not right I'll just move it directly to drivers/media/ipu3
>>
>> The IPU3 driver is still in staging (unless I've missed something), so I
>> think this cio2-bridge should stay with it.
> 
> You missed something.
> https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/pci/intel
> 
> IPU3 from Freescale (IIRC) is a different story.

Ayee, ok so we have 'half' the driver for IPU3 out of staging.

On Thu, Sep 17, 2020 at 5:19 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> On 17/09/2020 15:08, Andy Shevchenko wrote:


...

> Ayee, ok so we have 'half' the driver for IPU3 out of staging.


Correct. And your below analysis is correct.

> From my understanding, the IPU3 consists of two components, the CIO2

> (CSI2 capture), and the IMGU (the ISP).

>

> - drivers/media/pci/intel/ipu3

>

> This is indeed the CIO2 component (config VIDEO_IPU3_CIO2), and that is

> the part that this bridge relates to, so in fact this cio2-bridge should

> probably go there indeed. No need to go through staging.

>

> The files remaining at:

>

> - drivers/staging/media/ipu3

>

> are in fact also for the IPU3 but the ISP component (VIDEO_IPU3_IMGU).

>

> I'm sorry for the confusion, I knew that the ISP was still in staging, I

> hadn't realised the CSI2 receiver (CIO2) was not.

>

> >> Hopefully with more users of the IPU3 brought in by this cio2-bridge,

> >> that will help gather momentum to get the IPU3 developments required

> >> completed and moved into drivers/media.


-- 
With Best Regards,
Andy Shevchenko

On Thu, Sep 17, 2020 at 02:36:22PM +0100, Dan Scally wrote:
> On 17/09/2020 13:45, Andy Shevchenko wrote:
> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >> On 17/09/2020 11:33, Sakari Ailus wrote:
> > I will do better review for next version, assuming you will Cc reviewers and
> > TWIMC people. Below is like small part of comments I may give to the code.
> TWIMC?

Missed this. To Whom It May Concern.

On 17/09/2020 15:14, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 4:53 PM Dan Scally <djrscally@gmail.com> wrote:

>> Hi Andy, thanks for input (as always)

> You're welcome! I'm really impressed by your activity in this area.

Thanks - it's pretty fun so far
>>> Ah, I think you misinterpreted the meaning of above. The above is a switch how

>>> camera device appears either as PCI or an ACPI. So, it effectively means you

>>> should *not* have any relation for this HID until you find a platform where the

>>> device is for real enumerated via ACPI.

>>>

>> Ah, ok. So that was never going to work. Thanks. That does raise another

>> question; we have had some testers report failure, which turns out to be

>> because on their platforms the definition of their cameras in ACPI is

>> never translated into an i2c_client so the cio2-bridge doesn't bind.

>> Those have a similar conditional in the _STA method, see CAM1 in this

>> DSDT for example:

>> https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl.

>> Is there anything we can do to enable those cameras to be discovered too?

> It means that this

Is the rest of this comment missing?
>>>>>> +#define PROPERTY_ENTRY_NULL                       \

>>>>>> +((const struct property_entry) { })

>>>>> Alignment. Same appears to apply to other macros (please indent).

>>>> Yep

>>>>>> +#define SOFTWARE_NODE_NULL                        \

>>>>>> +((const struct software_node) { })

>>> Why?!

>>>

>> It felt ugly to have the other definitions be macros and not this one,

>> but I can change it.

> My point is that those macros are simply redundant. The point is to

> have a terminator record (all 0:s in the last entry of an array) which

> is usually being achieved by allocating memory with kcalloc() which

> does implicitly this for you.

Ah I see. TIL - thanks, I'll make that change too.

Hi Dan,

On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> > > +static int connect_supported_devices(void)
> > > +{
> > > +	struct acpi_device *adev;
> > > +	struct device *dev;
> > > +	struct sensor_bios_data ssdb;
> > > +	struct sensor *sensor;
> > > +	struct property_entry *sensor_props;
> > > +	struct property_entry *cio2_props;
> > > +	struct fwnode_handle *fwnode;
> > > +	struct software_node *nodes;
> > > +	struct v4l2_subdev *sd;
> > > +	int i, ret;
> > 
> > unsigned int i
> > 
> 
> Why?
> 
> For list iterators then "int i;" is best...  For sizes then unsigned is
> sometimes best.  Or if it's part of the hardware spec or network spec
> unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> They're not as intuitive as signed variables.  Imagine if there is an
> error in this loop and you want to unwind.  With a signed variable you
> can do:
> 
> 	while (--i >= 0)
> 		cleanup(&bridge.sensors[i]);
> 
> There are very few times where raising the type maximum from 2 billion
> to 4 billion fixes anything.

There's simply no need for the negative integers here. Sizes (as it's a
size here) are unsigned, too, so you'd be comparing signed and unsigned
numbers later in the function.

Hi Andy,

On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> > On 17/09/2020 11:33, Sakari Ailus wrote:
> > > a module and not enlarge everyone's kernel, and the initialisation would at
> > > the same time take place before the rest of what the CIO2 driver does in
> > > probe.
> > I thought of that as well, but wasn't sure which was preferable. I can
> > compress it into the CIO2 driver though sure.
> 
> Sakari, I tend to agree with Dan and have the board file separated from the
> driver and even framework.

And it'll be linked to the kernel binary then I suppose?

I don't have a strong opinion either way, just thought that this will
affect anyone using x86 machines, whether or not they have IPU3. I guess it
could be compiled in if the ipu3-cio2 driver is enabled?

I ran Smatch over the code and it spotted an off by one.

On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> +#define MAX_CONNECTED_DEVICES			4

> +#define SWNODE_SENSOR_HID			0

> +#define SWNODE_SENSOR_PORT			1

> +#define SWNODE_SENSOR_ENDPOINT			2

> +#define SWNODE_CIO2_PORT			3

> +#define SWNODE_CIO2_ENDPOINT			4

> +#define SWNODE_NULL_TERMINATOR			5

   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> +struct sensor {

> +	struct device *dev;

> +	struct software_node swnodes[5];

                             ^^^^^^^^^^
This needs to be 6 instead of 5 to prevent memory corruption.

> +	struct property_entry sensor_props[6];

> +	struct property_entry cio2_props[3];

> +	struct fwnode_handle *fwnode;

> +};



> +		nodes[SWNODE_NULL_TERMINATOR]   = SOFTWARE_NODE_NULL;

                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here.

regards,
dan carpenter

Ah, shoot - good spot, thanks

On 18/09/2020 09:03, Dan Carpenter wrote:
> I ran Smatch over the code and it spotted an off by one.

>

> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:

>> +#define MAX_CONNECTED_DEVICES			4

>> +#define SWNODE_SENSOR_HID			0

>> +#define SWNODE_SENSOR_PORT			1

>> +#define SWNODE_SENSOR_ENDPOINT			2

>> +#define SWNODE_CIO2_PORT			3

>> +#define SWNODE_CIO2_ENDPOINT			4

>> +#define SWNODE_NULL_TERMINATOR			5

>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>

>> +struct sensor {

>> +	struct device *dev;

>> +	struct software_node swnodes[5];

>                              ^^^^^^^^^^

> This needs to be 6 instead of 5 to prevent memory corruption.

>

>> +	struct property_entry sensor_props[6];

>> +	struct property_entry cio2_props[3];

>> +	struct fwnode_handle *fwnode;

>> +};

>

>> +		nodes[SWNODE_NULL_TERMINATOR]   = SOFTWARE_NODE_NULL;

>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> Here.

>

> regards,

> dan carpenter

>

On Fri, Sep 18, 2020 at 09:40:43AM +0300, Sakari Ailus wrote:
> Hi Dan,
> 
> On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> > > > +static int connect_supported_devices(void)
> > > > +{
> > > > +	struct acpi_device *adev;
> > > > +	struct device *dev;
> > > > +	struct sensor_bios_data ssdb;
> > > > +	struct sensor *sensor;
> > > > +	struct property_entry *sensor_props;
> > > > +	struct property_entry *cio2_props;
> > > > +	struct fwnode_handle *fwnode;
> > > > +	struct software_node *nodes;
> > > > +	struct v4l2_subdev *sd;
> > > > +	int i, ret;
> > > 
> > > unsigned int i
> > > 
> > 
> > Why?
> > 
> > For list iterators then "int i;" is best...  For sizes then unsigned is
> > sometimes best.  Or if it's part of the hardware spec or network spec
> > unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> > They're not as intuitive as signed variables.  Imagine if there is an
> > error in this loop and you want to unwind.  With a signed variable you
> > can do:
> > 
> > 	while (--i >= 0)
> > 		cleanup(&bridge.sensors[i]);
> > 
> > There are very few times where raising the type maximum from 2 billion
> > to 4 billion fixes anything.
> 
> There's simply no need for the negative integers here. Sizes (as it's a
> size here) are unsigned, too, so you'd be comparing signed and unsigned
> numbers later in the function.

I'm not trying to be rude, I'm honestly puzzled by this...

The "i" variable is not a size, it's an iterator...  Comparing signed
and unsigned isn't necessarily a problem, but the only comparison in
this case is here:

   253          struct property_entry *cio2_props;
   254          struct fwnode_handle *fwnode;
   255          struct software_node *nodes;
   256          struct v4l2_subdev *sd;
   257          int i, ret;
   258  
   259          for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That's obviously fine.  The compiler knows at compile time the value of
ARRAY_SIZE().  I feel like there must be a static checker which
complains about this?  ARRAY_SIZE() is size_t.

   260                  adev = acpi_dev_get_first_match_dev(supported_devices[i],
   261                                                      NULL, -1);
   262  
   263                  if (!adev)
   264                          continue;
   265  

regards,
dan carpenter

On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:
> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:

> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:

> > > On 17/09/2020 11:33, Sakari Ailus wrote:

> > > > a module and not enlarge everyone's kernel, and the initialisation would at

> > > > the same time take place before the rest of what the CIO2 driver does in

> > > > probe.

> > > I thought of that as well, but wasn't sure which was preferable. I can

> > > compress it into the CIO2 driver though sure.

> > 

> > Sakari, I tend to agree with Dan and have the board file separated from the

> > driver and even framework.

> 

> And it'll be linked to the kernel binary then I suppose?


Solely depends to your Kconfig dependencies and declaration.

From code perspective you may do it before enumeration of the certain device or
after with reprobe.

> I don't have a strong opinion either way, just thought that this will

> affect anyone using x86 machines, whether or not they have IPU3. I guess it

> could be compiled in if the ipu3-cio2 driver is enabled?


Of course!

-- 
With Best Regards,
Andy Shevchenko

Hi Dan -  thanks for all your comments. Sorry it took a while to get to
yours.

On 17/09/2020 10:34, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:

>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> index 92f5eadf2c99..fd941d2c7581 100644

>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

>> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)

>>  		cio2_queue_exit(cio2, &cio2->queue[i]);

>>  }

>>  

>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)

>> +{

>> +	void *sensor;

>> +

>> +	/*

>> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect

>> +	 * to cio2 have added a device link. If there are no consumers yet, then

>> +	 * we need to defer. The .sync_state() callback will then be called after

>> +	 * all linked sensors have probed

>> +	 */

>> +

>> +	if (IS_ENABLED(CONFIG_ACPI)) {

> Reverse this condition.

>

> 	if (!IS_ENABLED(CONFIG_ACPI))

> 		return 0;

>

>

Yeah, much better.
>> +		sensor = (struct device *)list_first_entry_or_null(

>> +								&pci_dev->dev.links.consumers,

>> +								struct dev_links_info,

>> +								consumers);

>> +

>> +		if (!sensor)

>> +			return -EPROBE_DEFER;

> Get rid of the cast.

>

> 	if (list_empty(&pci_dev->dev.links.consumers))

> 		return -EPROBE_DEFER;

>

> 	return 0;

>

Also much better, though I think possibly this whole section will be
going away now after some of the other pointers...
>> +		cio2 = dev_get_drvdata(dev);

>> +

>> +		if (!cio2) {

> Delete the blank line between the call and the test.  They're part of

> the same step.  "cio2" can't be NULL anyway, so delete the test.

Thanks - I'll skip blank lines in that situation in future
>> +

>> +	if (ret < 0) {

>> +		dev_err(dev, "Failed to fetch SSDB data\n");

>> +		return ret;

>> +	}

>> +

>> +	sensor->link = sensor_data.link;

>> +	sensor->lanes = sensor_data.lanes;

>> +	sensor->mclkspeed = sensor_data.mclkspeed;

>> +

>> +	return 0;

>> +}

>> +

>> +static int create_endpoint_properties(struct device *dev,

>> +				      struct sensor_bios_data *ssdb,

>> +				      struct property_entry *sensor_props,

>> +				      struct property_entry *cio2_props)

>> +{

>> +		u32 *data_lanes;

>> +		int i;

> Indented too far.

>

>> +

>> +		data_lanes = devm_kmalloc(dev, sizeof(u32) * (int)ssdb->lanes,

> No need for the cast.  Use devm_kmalloc_array().

Ah - TIL that that exists, thanks.
>> +					  GFP_KERNEL);

>> +

>> +		if (!data_lanes) {

>> +			dev_err(dev,

>> +				"Couldn't allocate memory for data lanes array\n");

> Delete the error message (checkpatch.pl --strict).

And that too - I wasn't using the --strict flag, I'll do that next time
>> +

>> +		sensor_props[0] = PROPERTY_ENTRY_U32("clock-frequency",

>> +						     ssdb->mclkspeed);

>> +		sensor_props[1] = PROPERTY_ENTRY_U32("bus-type", 5);

>> +		sensor_props[2] = PROPERTY_ENTRY_U32("clock-lanes", 0);

>> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

>> +							       data_lanes,

>> +							       (int)ssdb->lanes);

>> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];

>> +		sensor_props[5] = PROPERTY_ENTRY_NULL;

>> +

>> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",

>> +							     data_lanes,

>> +							     (int)ssdb->lanes);

>> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];

>> +		cio2_props[2] = PROPERTY_ENTRY_NULL;

>> +

>> +		return 0;

>> +}

>> +

>> +static int connect_supported_devices(void)

>> +{

>> +	struct acpi_device *adev;

>> +	struct device *dev;

>> +	struct sensor_bios_data ssdb;

>> +	struct sensor *sensor;

>> +	struct property_entry *sensor_props;

>> +	struct property_entry *cio2_props;

>> +	struct fwnode_handle *fwnode;

>> +	struct software_node *nodes;

>> +	struct v4l2_subdev *sd;

>> +	int i, ret;

>> +

>> +	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {

>> +		adev = acpi_dev_get_first_match_dev(supported_devices[i],

>> +						    NULL, -1);

>> +

>> +		if (!adev)

>> +			continue;

>> +

>> +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);

>> +

>> +		if (!dev) {

>> +			pr_info("ACPI match for %s, but it has no i2c device\n",

>> +				supported_devices[i]);

>> +			continue;

>> +		}

>> +

>> +		if (!dev->driver_data) {

>> +			pr_info("ACPI match for %s, but it has no driver\n",

>> +				supported_devices[i]);

> put_device(dev);

Good catch, thank you.
>> +	}

>> +

>> +	ret = connect_supported_devices();

>> +

>> +	if ((ret < 0) || (bridge.n_sensors <= 0)) {

>> +		pr_err("cio2_bridge: Failed to connect any devices\n");

>> +		goto out;

> If (bridge.n_sensors <= 0) is true then we need to set ret = -EINVAL

> or something.  Really .n_sensors can't be negative.

>

> The name "out" is a crappy label name because it doesn't say what the

> goto does.  When I scroll down then it turns out that "goto out;" calls

> a free_everything() function.  That kind of error handling is always

> buggy.

>

> There are several typical bugs.  1) Something leaks because the error

> handling style is too complicated to be audited.  2)  Dereferencing

> uninitialized pointers.  3)  Undoing something which hasn't been done.

>

> I believe that in this case one bug is with the handling of the

> bridge.cio2_fwnode.  We "restore" it back to the original state

> as soon as we have a non-NULL bridge.cio2 instead of waiting until we

> have stored the original state.

>

> The best way to do error handling is this.

>

> Every function cleans up after itself.  The connect_supported_devices()

> function is a bit special because it's a loop.  I would would write it

> so that if it fails then it cleans up the partial loop iteration and

> then at the end it cleans up all the failed loop iterations.

>

> 	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {

> 		a = frob();

> 		if (!a)

> 			goto unwind;

> 		b = frob();

> 		if (!b) {

> 			free(a);

> 			goto unwind;

> 		}

> 		...

> 	}

>

> 	return 0;

>

> unwind:

> 	for (i = 0; i < bridge.n_sensors; i++) {

> 		free(b);

> 		free(a);

> 	}

> 	bridge.n_sensors = 0;

>

> 	return ret;

>

> The problem with cio2_bridge_unregister_sensors() is that it doesn't

> clean up partial iterations through the loop.  (Missing calls to

> put_device(dev)).

>

> Loops are complicated but the rest is simple.  1) Every allocation

> function needs a matching cleanup function.  2) Use good label names

> which say what the goto does.  3)  The goto should free the most recent

> successful allocation.

>

> 	a = frob();

> 	if (!a)

> 		return -ENOMEM;

>

> 	b = frob();

> 	if (!b) {

> 		ret = -ENOMEM;

> 		goto free_a;

> 	}

>

> 	c = frob();

> 	if (!c) {

> 		ret = -ENOMEM;

> 		goto free_b;

> 	}

>

> 	return 0;

>

> free_b:

> 	free(b);

> free_a:

> 	free(a);

>

> 	return ret;

>

> The free function doesn't have any if statements.

>

> void free_function()

> {

> 	free(c);

> 	free(b);

> 	free(a);

> }

>

> The reviewer only needs to keep track of the most recent allocation

> and verify that the goto free_foo matches what should be freed.  This

> system means the code is auditable (no leaks), you never free anything

> which wasn't allocated.

>

This  section and the other comments on error handling was really
helpful - I appreciate you taking the time to explain so thoroughly.

On 18/09/2020 14:07, Andy Shevchenko wrote:
> On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:

>> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:

>>> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:

>>>> On 17/09/2020 11:33, Sakari Ailus wrote:

>>>>> a module and not enlarge everyone's kernel, and the initialisation would at

>>>>> the same time take place before the rest of what the CIO2 driver does in

>>>>> probe.

>>>> I thought of that as well, but wasn't sure which was preferable. I can

>>>> compress it into the CIO2 driver though sure.

>>> Sakari, I tend to agree with Dan and have the board file separated from the

>>> driver and even framework.

>> And it'll be linked to the kernel binary then I suppose?

> Solely depends to your Kconfig dependencies and declaration.

>

> From code perspective you may do it before enumeration of the certain device or

> after with reprobe.

>

>> I don't have a strong opinion either way, just thought that this will

>> affect anyone using x86 machines, whether or not they have IPU3. I guess it

>> could be compiled in if the ipu3-cio2 driver is enabled?

> Of course!


Apologies both - my inexperience is showing here: I thought you couldn't
make it compile into the kernel where it's dependent on another piece of
code that's configured to be a module? In my case, ipu3-cio2 plus some
other dependencies are configured as modules; VIDEO_DEV and VIDEO_V4L2
notably. Is that not right?


It would probably make the probe() ordering issues easier if it could be
compiled in, since I could just rely on late_initcall() in that case and
I guess that should work.

On Mon, Sep 21, 2020 at 02:33:57PM +0100, Dan Scally wrote:
> On 18/09/2020 14:07, Andy Shevchenko wrote:
> > On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:
> >> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:
> >>> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >>>> On 17/09/2020 11:33, Sakari Ailus wrote:
> >>>>> a module and not enlarge everyone's kernel, and the initialisation would at
> >>>>> the same time take place before the rest of what the CIO2 driver does in
> >>>>> probe.
> >>>> I thought of that as well, but wasn't sure which was preferable. I can
> >>>> compress it into the CIO2 driver though sure.
> >>> Sakari, I tend to agree with Dan and have the board file separated from the
> >>> driver and even framework.
> >> And it'll be linked to the kernel binary then I suppose?
> > Solely depends to your Kconfig dependencies and declaration.
> >
> > From code perspective you may do it before enumeration of the certain device or
> > after with reprobe.
> >
> >> I don't have a strong opinion either way, just thought that this will
> >> affect anyone using x86 machines, whether or not they have IPU3. I guess it
> >> could be compiled in if the ipu3-cio2 driver is enabled?
> > Of course!
> 
> Apologies both - my inexperience is showing here: I thought you couldn't
> make it compile into the kernel where it's dependent on another piece of
> code that's configured to be a module? In my case, ipu3-cio2 plus some
> other dependencies are configured as modules; VIDEO_DEV and VIDEO_V4L2
> notably. Is that not right?

It's not correct.

We specifically have export.h (usually implied by module.h) which provides an
API for symbols that may be used in modules independently on where they are
(in-kernel or in a module).

So, provider does

bar.h:
int foo();

bar.c:
int foo()
{
}
EXPORT_SYMBOL(foo); // normally is EXPORT_SYMBOL_GPL() in use

Consumer:
	#include <bar.h>

	ret = foo();

> It would probably make the probe() ordering issues easier if it could be
> compiled in, since I could just rely on late_initcall() in that case and
> I guess that should work.

So, you may have two cases, your module went first, the other module went
first. Some similar problems as your bridge is trying to address are mitigated
in intel_cht_int33fe_typec.c. Look at it. It's not an ideal example, but that's
due to miserable ACPI tables BIOS provided.

Hi Sakari

On 17/09/2020 11:33, Sakari Ailus wrote:
>> +		ret = software_node_register_nodes(nodes);
>> +		if (ret) {
>> +			dev_err(dev,
>> +				"Failed to register software nodes for %s\n",
>> +				supported_devices[i]);
>> +			return ret;
>> +		}
>> +
>> +		fwnode = software_node_fwnode(&nodes[SWNODE_SENSOR_HID]);
>> +		if (!fwnode) {
>> +			dev_err(dev,
>> +				"Failed to get software node for %s\n",
>> +				supported_devices[i]);
>> +			return ret;
>> +		}
>> +
>> +		fwnode->secondary = ERR_PTR(-ENODEV);
>> +		dev->fwnode = fwnode;
>> +
>> +		/*
>> +		 * The device should by this point has driver_data set to an
>> +		 * instance of struct v4l2_subdev; set the fwnode for that too.
>> +		 */
>> +
>> +		sd = dev_get_drvdata(dev);
>> +		sd->fwnode = fwnode;
> I'm a bit lost here. Isn't it enough to have the sensor device's fwnode,
> and to use that for V4L2 async fwnode matching (as usual)?

Just working through the changes everyone's suggested for the v2. For
this one the reason it had to be this way is that
v4l2_async_register_subdev() just picks up the fwnode from the device.
If we wanted to just rely on that call as part of the sensor driver's
probe() then we need to reprobe() the sensor in case it already probed
before this code has managed to run, and reprobing after assigning the
software_nodes as fwnode to the sensor no longer works - the long and
short of that is that the ACPI matching portion of i2c_device_match()
calls ACPI_COMPANION(dev), and that macro relies on dev->fwnode->ops
being acpi_device_fwnode_ops which they no longer are. This is also the
reason I was storing the original fwnode's of the sensor device and cio2
device in the cio2-bridge; so that they could be restored if the module
was removed.

On 17/09/2020 11:33, Sakari Ailus wrote:
>> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
>> +							       data_lanes,
>> +							       (int)ssdb->lanes);
>> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];
>> +		sensor_props[5] = PROPERTY_ENTRY_NULL;
>> +
>> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
>> +							     data_lanes,
>> +							     (int)ssdb->lanes);
>> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];
>> +		cio2_props[2] = PROPERTY_ENTRY_NULL;
> I suppose the CSI-2 link frequency is generally encoded in drivers in this
> case. A lot of drivers already check for those, could you add the
> frequencies here as well (as they are known)?

This one caused me some consternation; I mentioned in a previous email
that I was overwriting the v4l2_subdev's fwnode directly because I
couldn't reprobe() the devices after changing their fwnode. Turns out
that's probably not ok, because as you point out there are some drivers
that check for properties in firmware as part of their .probe() call, so
they _have_ to be there for those to work (including ov5670, which is
the canonical ipu3 driver in the kernel docs). imx258 and ov13858 are
also affected, and I'm aware of at least one other driver in development
that would be affected.


The problem preventing the reprobe working is that i2c_device_match()
relies on a device's fwnode having acpi_device_fwnode_ops to perform
ACPI matching, so after giving the device our software nodes the
matching just fails. I thrashed out a way to make the reprobe work, but
I don't really like the solution so I wanted to talk about it. The long
story short is; clone the driver but add an i2c_device_id matching the
existing i2c_client's name, then use i2c_add_driver() to install it to
the bus before calling device_reprobe(). This does work; it makes the
devices reprobe cleanly at the end of cio2-bridge's init, but it feels a
little bit hacky. Any thoughts? Or if it makes it easier to discuss, I
can just post the v2 containing all the changes that people suggested
after the v1, and showing how this reprobe would work.