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

[Dan Scally <djrscally@gmail.com>]

Hi Sakari

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

Thanks!

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

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


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