Re: [RFC PATCH v3 9/9] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows

[Sakari Ailus]

Hi Laurent, Daniel,

On Sat, Oct 24, 2020 at 04:24:11AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Mon, Oct 19, 2020 at 11:59:03PM +0100, Daniel Scally wrote:
> > Currently on platforms designed for Windows, connections between CIO2 and
> > sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> > driver to compensate by building software_node connections, parsing the
> > connection properties from the sensor's SSDB buffer.
> > 
> > Suggested-by: Jordan Hand <jorhand@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> > ---
> > Changes in v3:
> > 	- Rather than overwriting the device's primary fwnode, we now
> > 	simply assign a secondary. Some of the preceding patches alter the
> > 	existing driver code and v4l2 framework to allow for that.
> > 	- Rather than reprobe() the sensor after connecting the devices in
> > 	cio2-bridge we create the software_nodes right away. In this case,
> > 	sensor drivers will have to defer probing until they detect that a
> > 	fwnode graph is connecting them to the CIO2 device.
> > 	- Error paths in connect_supported_devices() moved outside the
> > 	loop
> > 	- Replaced pr_*() with dev_*() throughout
> > 	- Moved creation of software_node / property_entry arrays to stack
> > 	- A lot of formatting changes.
> > 
> >  MAINTAINERS                                   |   1 +
> >  drivers/media/pci/intel/ipu3/Kconfig          |  18 +
> >  drivers/media/pci/intel/ipu3/Makefile         |   3 +-
> >  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 327 ++++++++++++++++++
> >  drivers/media/pci/intel/ipu3/cio2-bridge.h    |  94 +++++
> >  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  21 ++
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   9 +
> >  7 files changed, 472 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5d768d5ad..4c9c646c7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8848,6 +8848,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
> >  M:	Yong Zhi <yong.zhi@xxxxxxxxx>
> >  M:	Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >  M:	Bingbu Cao <bingbu.cao@xxxxxxxxx>
> > +M:	Dan Scally <djrscally@xxxxxxxxx>
> >  R:	Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
> >  L:	linux-media@xxxxxxxxxxxxxxx
> >  S:	Maintained
> > diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> > index 82d7f17e6..d14cbceae 100644
> > --- a/drivers/media/pci/intel/ipu3/Kconfig
> > +++ b/drivers/media/pci/intel/ipu3/Kconfig
> > @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
> >  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> >  	  connected camera.
> >  	  The module will be called ipu3-cio2.
> > +
> > +config CIO2_BRIDGE
> > +	bool "IPU3 CIO2 Sensors Bridge"
> > +	depends on VIDEO_IPU3_CIO2
> > +	help
> > +	  This extension provides an API for the ipu3-cio2 driver to create
> > +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
> > +	  can be used to enable support for cameras in detachable / hybrid
> > +	  devices that ship with Windows.
> > +
> > +	  Say Y here if your device is a detachable / hybrid laptop that comes
> > +	  with Windows installed by the OEM, for example:
> > +
> > +	  	- Some Microsoft Surface models
> > +		- The Lenovo Miix line
> > +		- Dell 7285
> > +
> > +	  If in doubt, say N here.
> > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> > index b4e3266d9..933777e6e 100644
> > --- a/drivers/media/pci/intel/ipu3/Makefile
> > +++ b/drivers/media/pci/intel/ipu3/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> >  
> > -ipu3-cio2-y += ipu3-cio2-main.o
> > \ No newline at end of file
> > +ipu3-cio2-y += ipu3-cio2-main.o
> > +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> > diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > new file mode 100644
> > index 000000000..bbe072f04
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > @@ -0,0 +1,327 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Author: Dan Scally <djrscally@xxxxxxxxx>
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/fwnode.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/property.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include "cio2-bridge.h"
> > +
> > +/*
> > + * Extend this array with ACPI Hardware ID's of devices known to be
> > + * working
> > + */
> > +static const char * const supported_devices[] = {
> > +	"INT33BE",
> > +	"OVTI2680",
> > +};
> > +
> > +static struct software_node cio2_hid_node = { CIO2_HID };
> > +
> > +static struct cio2_bridge bridge;
> 
> While there shouldn't be more than one CIO2 instance, we try to develop
> drivers in a way that avoids global per-device variables. Could all this
> be allocated dynamically, with the pointer returned by
> cio2_bridge_build() and stored in the cio2_device structure ?

I don't mind but the Windows ACPI table design assumes there's a single
CIO2. Thus the same assumption can be made here, too. Admittedly, I think
it could be cleaner that way.

...

> > +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> > +		if (!dev) {
> > +			ret = -EPROBE_DEFER;
> > +			goto err_rollback;
> > +		}
> > +
> > +		sensor = &bridge.sensors[bridge.n_sensors];
> > +		sensor->dev = dev;
> > +		sensor->adev = adev;
> > +
> > +		snprintf(sensor->name, ACPI_ID_LEN, "%s",
> > +			 supported_devices[i]);
> 
> How about strlcpy() ?

Or even strscpy()?

> > +void cio2_bridge_burn(struct pci_dev *cio2)
> 
> Interesting function name :-) I like the creativity, but I think
> consistency with the rest of the kernel code should unfortunately be
> favoured.

I guess we can use any pairs that make sense. Create and destroy? Build and
plunder?

-- 
Regards,

Sakari Ailus