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 <jorh...@linux.microsoft.com>
> > Signed-off-by: Daniel Scally <djrsca...@gmail.com>
> > ---
> > 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....@intel.com>
> > M: Sakari Ailus <sakari.ai...@linux.intel.com>
> > M: Bingbu Cao <bingbu....@intel.com>
> > +M: Dan Scally <djrsca...@gmail.com>
> > R: Tianshu Qiu <tian.shu....@intel.com>
> > L: linux-me...@vger.kernel.org
> > 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 <djrsca...@gmail.com>
> > +#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
