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 <jorh...@linux.microsoft.com>
>> Signed-off-by: Daniel Scally <djrsca...@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....@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 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 <djrsca...@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.
