Re: Fwd: Surface Go VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go (version1))

[Hans de Goede <hdegoede@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8823 bytes --]



On 10/29/21 13:50, Daniel Scally wrote:
> Hi all
> 
> 
> +CC linux-media and libcamera-devel, as it's probably a good time to
> broaden this out. Also Andy because I'm hoping you can help :) The
> background of the discussion is about how we identify and enumerate
> (correctly, I.E. with a type matching the vcm driver's i2c_device_id,
> and there are a few different vcm's in scope which seem encoded in the
> SSDB buffer) which VCM module is linked to a sensor in Intel's IPU3
> centric ACPI tables. The I2C address for the device is just a second
> I2cSerialBusV2 against the sensor's acpi device rather than a separate
> one, which is no awkward. We also need to get firmware created for the
> VCM such that the sensor will link to it via the lens-focus property.
> 
> On 28/10/2021 09:57, Hans de Goede wrote:
>> Hi,
>>
>> On 10/28/21 10:49, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> On Thu, Oct 28, 2021 at 09:51:08AM +0200, Hans de Goede wrote:
>>>> On 10/28/21 09:10, Daniel Scally wrote:
>>>>> On 27/10/2021 15:16, Hans de Goede wrote:
>>>>>> On 10/27/21 12:07, Daniel Scally wrote:
>>>>>>> On 26/10/2021 11:14, Hans de Goede wrote:
>>>>>>>>>> So yesterday I already sorta guessed it would be the DW9714 because of
>>>>>>>>>> the 0x0c address and I tried:
>>>>>>>>>>
>>>>>>>>>> i2ctransfer -y 2 w2@0x0c 0x00 0x00
>>>>>>>>>>
>>>>>>>>>> And the transfer fails, while according to the driver that is a valid
>>>>>>>>>> value. So maybe we are missing a regulator enable? Or its not a DW9714.
>>>>>>>>>>
>>>>>>>>>> Also "i2cdetect -y -r 2" does not see anything at address 0x0c (but some of
>>>>>>>>>> these VCMs seem to be write only...) it does OTOH see an unknown device at
>>>>>>>>>> address 0x21.
>>>>>>>>> Well, when debugging the necessary TPS68470 settings I used a poor man's
>>>>>>>>> i2ctransfer on Windows whilst the camera was running to read the values
>>>>>>>>> that were set for both the PMIC and the camera sensor. Using the same
>>>>>>>>> program I can connect to and read values from a device at 0x0c,
>>>>>>> Just as further testing I dumped the contents of the device at 0x0c,
>>>>>>> which comes back as
>>>>>>>
>>>>>>> f1 1 2 1 61 0 40 60
>>>>>>>
>>>>>>> Byte 0 is given in the driver you linked as the ID field and expected to
>>>>>>> be f1. The driver controls focus by writing to the 3rd and 4th byte
>>>>>>> (with the 4th being the LSB); the only value that seemed to fluctuate
>>>>>>> when running windows and moving my hand in front of the sensor was byte
>>>>>>> 4 and testing it out I wrote values into that byte and the focus
>>>>>>> changes. So the device at 0x0c is definitely the vcm and it sure looks
>>>>>>> like it's the DW9719
>>>>>>>
>>>>>>> The device at 0x21 is only available on Windows when the camera is
>>>>>>> running, I thought it was quite likely that one of the "spare"
>>>>>>> regulators from the TPS68470. One line is called VCM, and sure enough
>>>>>>> it's enabled whilst the world-facing camera is running. I switched to
>>>>>>> linux and started streaming the back camera, then enabled that voltage
>>>>>>> regulator via i2ctransfer:
>>>>>>>
>>>>>>> sudo i2ctransfer 2 w2@0x4d 0x3c 0x6d
>>>>>>>
>>>>>>> sudo i2ctransfer 2 w2@0x4d 0x44 0x01
>>>>>>>
>>>>>>> And now i2cdetect shows the device at 0x0c on bus 2 - so we need more
>>>>>>> jiggery pokey to map that VCM regulator to this new device (once we've
>>>>>>> gotten it enumerated...) and the driver needs to have a tweak to call
>>>>>>> regulator get and do a power on at some point.
>>>>>> Awesome, great job on figuring this out!
>>>>>>
>>>>>> As you know I can spend $dayjob time on this, so I'll take on the job
>>>>>> of creating the i2c-client and hooking up the regulator in some
>>>>>> upstreamable manner.
>>>>> Okedokey cool. I'd probably start at the cio2-bridge, if only because we
>>>>> already have the adev there and the SSDB buffer loaded, so should be
>>>>> easy enough to add an enum for the vcm_type and a call to
>>>>> i2c_acpi_new_device()...bit of a weird place for that though I guess.
>>>> Ah, I was actually thinking about doing this int he int3472 code for
>>>> a number of reasons:
>>>>
>>>> 1. We already have the regulator_init_data there and we will need to
>>>> expand it for this.
>>>>
>>>> 2. It is sorta the central place where we deal with all this glue-stuff
>>> I'm not too sure about that. The INT3472 model the "Intel camera PMIC"
>>> (I don't remember the exact wording, but that's more or less how the
>>> device is described in Windows, and it matches the intent we see in the
>>> DSDT).
>> I agree that the INT3472 models the PMIC, or whatever discrete bits
>> which offer similar functionality.
>>
>>> Given that we already have cio2-bridge, and that it hooks up the
>>> sensor to the CIO2, it seems to me that it would be a better central
>>> place.
>> Ok, I was sorta expecting you to want to keep glue code like this
>> out of drivers/media. But I guess that only applies to putting ACPI
>> specific stuff in sensor drivers; and since the cio2-bridge code is
>> already x86/ACPI specific you are fine with adding ACPI code there?
>>
>> I'm fine with putting the VCM i2c-client instantiation in the
>> cio2-bridge code, that may also make it easier to tie the 2 together
>> at the media-controller level.
> 
> 
> Having looked at this yesterday evening I'm more and more convinced it's
> necessary. I hacked it into the ov8865 driver in the interim (just by
> calling i2c_acpi_new_device() in probe) and then worked on that dw9719
> code you found [1] to turn it into an i2c driver (attached, though still
> needs a bit of work), which will successfully bind to the i2c client
> enumerated by that i2c_acpi_new_device() call. From there though it
> needs a way for the v4l2 subdev to be matched to the sensor's subdev.
> This can happen automatically by way of the lens-focus firmware property
> against the sensor - we currently build those in the cio2-bridge, so
> adding another software node for the VCM and creating a lens-focus
> property for the sensor's software_node with a pointer to the VCM's node
> seems like the best way to do that.

So besides prepping a v5 of my previous series, with update regulator
init-data for the VCM I've also been looking into this, attached are
the results.

Some notes from initial testing:

1. The driver you attached will only successful probe if I insmod
it while streaming video from the sensor. So I think we need another
regulator or the clk for just the VCM too, I will investigate this
later this week.

2. I need some help with all the fwnode link stuff (I'm not very familiar
with this). There seems to be a chicken and egg problem here though,
because the v4l2subdev for the VCM does not register because of async stuff
and if we add it to the "graph" then my idea to enumerate the VCMs
from the SSDB on the complete() callback won't work. But we can do this
on a per sensor basis instead from the cio2_notifier_bound() callback
instead I guess ?

Can someone give me some hints how the fwnode link code should look/work
and how I can get the async registering of the subdev for the VCM to
complete ?

Regards,

Hans




> 
> 
> To throw a spanner in the works though; I noticed this delightful _CRS
> for the OV9734 sensor of a  Surface Laptop 1 earlier:
> 
> 
> Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
> {
>     Name (SBUF, ResourceTemplate ()
>     {
>         I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80,
>             AddressingMode7Bit, "\\_SB.PCI0.I2C2",
>             0x00, ResourceConsumer, , Exclusive,
>             )
>         I2cSerialBusV2 (0x0050, ControllerInitiated, 0x00061A80,
>             AddressingMode7Bit, "\\_SB.PCI0.I2C2",
>             0x00, ResourceConsumer, , Exclusive,
>             )
>         I2cSerialBusV2 (0x0051, ControllerInitiated, 0x00061A80,
>             AddressingMode7Bit, "\\_SB.PCI0.I2C2",
>             0x00, ResourceConsumer, , Exclusive,
>             )
>         I2cSerialBusV2 (0x0052, ControllerInitiated, 0x00061A80,
>             AddressingMode7Bit, "\\_SB.PCI0.I2C2",
>             0x00, ResourceConsumer, , Exclusive,
>             )
>         I2cSerialBusV2 (0x0053, ControllerInitiated, 0x00061A80,
>             AddressingMode7Bit, "\\_SB.PCI0.I2C2",
>             0x00, ResourceConsumer, , Exclusive,
>             )
>     })
>     Return (SBUF) /* \_SB_.PCI0.I2C2.CAMF._CRS.SBUF */
> }

Hmm, we do have i2c_acpi_client_count(adev), so it is easy to use
that and just always use the last resource for the VCM. But that assumes
that is what is going on here and I have no idea.

Regards,

Hans

[-- Attachment #2: 0001-i2c-acpi-Change-first-param-of-i2c_acpi_new_device-t.patch --]
[-- Type: text/x-patch, Size: 7958 bytes --]

From 02075ca324a20774e4466696f501a6ed3269a09b Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 1 Nov 2021 13:59:23 +0100
Subject: [PATCH media-staging regression fix 1/3] i2c: acpi: Change first
 param of i2c_acpi_new_device() to an acpi_device *

Change the first parameter of i2c_acpi_new_device() from
a struct device * to a struct acpi_device *.

This is necessary because in some cases we may only have access
to the fwnode / acpi_device and not to the matching physical-node
struct device *.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c                                 | 5 ++---
 drivers/iio/accel/bmc150-accel-i2c.c                        | 2 +-
 drivers/iio/light/cm32181.c                                 | 2 +-
 drivers/platform/surface/surface3_power.c                   | 2 +-
 drivers/platform/x86/i2c-multi-instantiate.c                | 2 +-
 .../platform/x86/intel/int33fe/intel_cht_int33fe_microb.c   | 2 +-
 .../platform/x86/intel/int33fe/intel_cht_int33fe_typec.c    | 6 +++---
 include/linux/i2c.h                                         | 4 ++--
 8 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 71eee5bc17ab..3eae6c264bb5 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -481,7 +481,7 @@ struct notifier_block i2c_acpi_notifier = {
 
 /**
  * i2c_acpi_new_device - Create i2c-client for the Nth I2cSerialBus resource
- * @dev:     Device owning the ACPI resources to get the client from
+ * @adev:    ACPI-device owning the ACPI resources to get the client from
  * @index:   Index of ACPI resource to get
  * @info:    describes the I2C device; note this is modified (addr gets set)
  * Context: can sleep
@@ -497,10 +497,9 @@ struct notifier_block i2c_acpi_notifier = {
  * Returns a pointer to the new i2c-client, or error pointer in case of failure.
  * Specifically, -EPROBE_DEFER is returned if the adapter is not found.
  */
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
+struct i2c_client *i2c_acpi_new_device(struct acpi_device *adev, int index,
 				       struct i2c_board_info *info)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct i2c_acpi_lookup lookup;
 	struct i2c_adapter *adapter;
 	LIST_HEAD(resource_list);
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 88bd8a25f142..c957f31afef5 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -149,7 +149,7 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 
 	board_info.irq = acpi_dev_gpio_irq_get(adev, 1);
 
-	data->second_device = i2c_acpi_new_device(&client->dev, 1, &board_info);
+	data->second_device = i2c_acpi_new_device(adev, 1, &board_info);
 
 	if (!IS_ERR(data->second_device) && bmc150_acpi_enable_keyboard(data->second_device)) {
 		INIT_DELAYED_WORK(&data->resume_work, bmc150_acpi_resume_work);
diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 97649944f1df..9acff6f50db2 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -455,7 +455,7 @@ static int cm32181_probe(struct i2c_client *client)
 
 		i2c_smbus_read_byte(client);
 
-		client = i2c_acpi_new_device(dev, 1, &board_info);
+		client = i2c_acpi_new_device(ACPI_COMPANION(dev), 1, &board_info);
 		if (IS_ERR(client))
 			return PTR_ERR(client);
 	}
diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
index abac3eec565e..3c89c6926cc0 100644
--- a/drivers/platform/surface/surface3_power.c
+++ b/drivers/platform/surface/surface3_power.c
@@ -514,7 +514,7 @@ static int mshw0011_probe(struct i2c_client *client)
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "MSHW0011-bat0", I2C_NAME_SIZE);
 
-	bat0 = i2c_acpi_new_device(dev, 1, &board_info);
+	bat0 = i2c_acpi_new_device(ACPI_COMPANION(dev), 1, &board_info);
 	if (IS_ERR(bat0))
 		return PTR_ERR(bat0);
 
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index a50153ecd560..2de5955ec084 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -90,7 +90,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			board_info.irq = 0;
 			break;
 		}
-		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
+		multi->clients[i] = i2c_acpi_new_device(adev, i, &board_info);
 		if (IS_ERR(multi->clients[i])) {
 			ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]),
 					    "Error creating i2c-client, idx %d\n", i);
diff --git a/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_microb.c b/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_microb.c
index 673f41cd14b5..29c661c70f1c 100644
--- a/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_microb.c
+++ b/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_microb.c
@@ -48,7 +48,7 @@ int cht_int33fe_microb_probe(struct cht_int33fe_data *data)
 	strscpy(board_info.type, "bq27542", ARRAY_SIZE(board_info.type));
 	board_info.dev_name = "bq27542";
 	board_info.swnode = &bq27xxx_node;
-	data->battery_fg = i2c_acpi_new_device(dev, 1, &board_info);
+	data->battery_fg = i2c_acpi_new_device(ACPI_COMPANION(dev), 1, &board_info);
 
 	return PTR_ERR_OR_ZERO(data->battery_fg);
 }
diff --git a/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c b/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
index d59544167430..4d9d59ba1f1e 100644
--- a/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
+++ b/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
@@ -267,7 +267,7 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
 	strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
 	board_info.dev_name = "max17047";
 	board_info.fwnode = fwnode;
-	data->battery_fg = i2c_acpi_new_device(dev, 1, &board_info);
+	data->battery_fg = i2c_acpi_new_device(ACPI_COMPANION(dev), 1, &board_info);
 
 	return PTR_ERR_OR_ZERO(data->battery_fg);
 }
@@ -331,7 +331,7 @@ int cht_int33fe_typec_probe(struct cht_int33fe_data *data)
 	board_info.fwnode = fwnode;
 	board_info.irq = fusb302_irq;
 
-	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
+	data->fusb302 = i2c_acpi_new_device(ACPI_COMPANION(dev), 2, &board_info);
 	if (IS_ERR(data->fusb302)) {
 		ret = PTR_ERR(data->fusb302);
 		goto out_unregister_max17047;
@@ -348,7 +348,7 @@ int cht_int33fe_typec_probe(struct cht_int33fe_data *data)
 	board_info.fwnode = fwnode;
 	strlcpy(board_info.type, "pi3usb30532", I2C_NAME_SIZE);
 
-	data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
+	data->pi3usb30532 = i2c_acpi_new_device(ACPI_COMPANION(dev), 3, &board_info);
 	if (IS_ERR(data->pi3usb30532)) {
 		ret = PTR_ERR(data->pi3usb30532);
 		goto out_unregister_fusb302;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2ce3efbe9198..c11e5fde0bb7 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1012,7 +1012,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
 			       struct acpi_resource_i2c_serialbus **i2c);
 int i2c_acpi_client_count(struct acpi_device *adev);
 u32 i2c_acpi_find_bus_speed(struct device *dev);
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
+struct i2c_client *i2c_acpi_new_device(struct acpi_device *adev, int index,
 				       struct i2c_board_info *info);
 struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
 #else
@@ -1029,7 +1029,7 @@ static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
 {
 	return 0;
 }
-static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
+static inline struct i2c_client *i2c_acpi_new_device(struct acpi_device *adev,
 					int index, struct i2c_board_info *info)
 {
 	return ERR_PTR(-ENODEV);
-- 
2.31.1


[-- Attachment #3: 0002-media-ipu3-cio2-Store-cio2_bridge-pointer-in-struct-.patch --]
[-- Type: text/x-patch, Size: 3648 bytes --]

From d9b33af7875dd37533c7bdaa4f9bf93b06b8e041 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 1 Nov 2021 16:18:08 +0100
Subject: [PATCH media-staging regression fix 2/3] media: ipu3-cio2: Store
 cio2_bridge pointer in struct cio2_device

This is a preparation patch for adding support for instantiating
i2c-clients for VCMs described in SSDB ACPI tables.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note there seems to be a pre-existing problem where there is no teardown
of the bridge? It looks like we need a cio2_bridge_cleanup() function,
to be called from cio2_pci_remove() ?
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 8 ++++----
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 6 +++---
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      | 9 +++++++--
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 0c1c5d8d8dfd..d5e17f6f27cf 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -306,7 +306,7 @@ static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
 	return ret;
 }
 
-int cio2_bridge_init(struct pci_dev *cio2)
+struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2)
 {
 	struct device *dev = &cio2->dev;
 	struct fwnode_handle *fwnode;
@@ -316,7 +316,7 @@ int cio2_bridge_init(struct pci_dev *cio2)
 
 	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
 	if (!bridge)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	strscpy(bridge->cio2_node_name, CIO2_HID,
 		sizeof(bridge->cio2_node_name));
@@ -353,7 +353,7 @@ int cio2_bridge_init(struct pci_dev *cio2)
 
 	set_secondary_fwnode(dev, fwnode);
 
-	return 0;
+	return bridge;
 
 err_unregister_sensors:
 	cio2_bridge_unregister_sensors(bridge);
@@ -362,5 +362,5 @@ int cio2_bridge_init(struct pci_dev *cio2)
 err_free_bridge:
 	kfree(bridge);
 
-	return ret;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 76fd4e6e8e46..5209f83c8248 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1730,9 +1730,9 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			return -EINVAL;
 		}
 
-		r = cio2_bridge_init(pci_dev);
-		if (r)
-			return r;
+		cio2->bridge = cio2_bridge_init(pci_dev);
+		if (IS_ERR(cio2->bridge))
+			return PTR_ERR(cio2->bridge);
 	}
 
 	r = pcim_enable_device(pci_dev);
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 3a1f394e05aa..64d7ca502124 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -22,6 +22,7 @@
 #include <media/videobuf2-v4l2.h>
 
 struct cio2_fbpt_entry;		/* defined here, after the first usage */
+struct cio2_bridge;
 struct pci_dev;
 
 #define CIO2_NAME					"ipu3-cio2"
@@ -381,6 +382,7 @@ struct cio2_device {
 	struct v4l2_device v4l2_dev;
 	struct cio2_queue queue[CIO2_QUEUES];
 	struct cio2_queue *cur_queue;
+	struct cio2_bridge *bridge;
 	/* mutex to be used by video_device */
 	struct mutex lock;
 
@@ -460,9 +462,12 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
 }
 
 #if IS_ENABLED(CONFIG_CIO2_BRIDGE)
-int cio2_bridge_init(struct pci_dev *cio2);
+struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2);
 #else
-static inline int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
+static inline struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2)
+{
+	return NULL;
+}
 #endif
 
 #endif
-- 
2.31.1


[-- Attachment #4: 0003-media-ipu3-cio2-Add-support-for-instantiating-i2c-cl.patch --]
[-- Type: text/x-patch, Size: 5524 bytes --]

From d043c7c1dd43144d4825c7781783099d4494b798 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 1 Nov 2021 13:37:30 +0100
Subject: [PATCH media-staging regression fix 3/3] media: ipu3-cio2: Add
 support for instantiating i2c-client for VCMs

Some sensors come with a variable-focus lens where the lens focus is
controller by a VCM (Voice Coil Motor). If there is a VCM for the
lens-focus, and if so which one, is described on the vcm_type field
of the ACPI SSDB table.

These VCMs are a second I2C device listed as an extra I2cSerialBusV2
resource in the same ACPI device as the sensor. The i2c-core-acpi.c
code only instantiates an i2c-client for the first I2cSerialBusV2
resource.

Add support for instantiating an i2c-client for the VCM with
the type of the i2c-client set based on the SSDB vcm_type field.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 52 +++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    |  3 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 11 ++++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |  2 +
 4 files changed, 68 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index d5e17f6f27cf..d393024c7f58 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -3,6 +3,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/i2c.h>
 #include <linux/pci.h>
 #include <linux/property.h>
 #include <media/v4l2-fwnode.h>
@@ -207,6 +208,8 @@ static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
 		software_node_unregister_nodes(sensor->swnodes);
 		ACPI_FREE(sensor->pld);
 		acpi_dev_put(sensor->adev);
+		if (sensor->vcm_i2c_client)
+			i2c_unregister_device(sensor->vcm_i2c_client);
 	}
 }
 
@@ -364,3 +367,52 @@ struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2)
 
 	return ERR_PTR(ret);
 }
+
+static void cio2_bridge_instantiate_vcm_i2c_client(struct cio2_sensor *sensor)
+{
+	static const char * const vcm_types[] = {
+		"ad5823",
+		"dw9714",
+		"ad5816",
+		"dw9719",
+		"dw9718",
+		"dw9806b",
+		"wv517s",
+		"lc898122xa",
+		"lc898212axb",
+	};
+	struct i2c_board_info board_info = { };
+	char name[16];
+
+	if (!sensor->ssdb.vcmtype)
+		return;
+
+	if (sensor->ssdb.vcmtype > ARRAY_SIZE(vcm_types)) {
+		dev_warn(&sensor->adev->dev, "Unknown VCM type %d\n",
+			 sensor->ssdb.vcmtype);
+		return;
+	}
+
+	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
+	board_info.dev_name = name;
+	strscpy(board_info.type, vcm_types[sensor->ssdb.vcmtype - 1],
+		ARRAY_SIZE(board_info.type));
+
+	sensor->vcm_i2c_client = i2c_acpi_new_device(sensor->adev, 1, &board_info);
+	if (IS_ERR(sensor->vcm_i2c_client)) {
+		dev_warn(&sensor->adev->dev, "Error instantiation VCM i2c-client: %ld\n",
+			 PTR_ERR(sensor->vcm_i2c_client));
+		sensor->vcm_i2c_client = NULL;
+	}
+}
+
+void cio2_bridge_instantiate_vcm_devices(struct cio2_bridge *bridge)
+{
+	unsigned int i;
+
+	if (!bridge)
+		return;
+
+	for (i = 0; i < bridge->n_sensors; i++)
+		cio2_bridge_instantiate_vcm_i2c_client(&bridge->sensors[i]);
+}
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
index 202c7d494f7a..27a61a5d479e 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -8,6 +8,8 @@
 
 #include "ipu3-cio2.h"
 
+struct i2c_client;
+
 #define CIO2_HID				"INT343E"
 #define CIO2_MAX_LANES				4
 #define MAX_NUM_LINK_FREQS			3
@@ -106,6 +108,7 @@ struct cio2_sensor_config {
 struct cio2_sensor {
 	char name[ACPI_ID_LEN];
 	struct acpi_device *adev;
+	struct i2c_client *vcm_i2c_client;
 
 	struct software_node swnodes[6];
 	struct cio2_node_names node_names;
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 5209f83c8248..3f84e7be64a7 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1444,6 +1444,17 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 		}
 	}
 
+	/*
+	 * This is done here because, on systems where the VCMs are described
+	 * in the SSDB, the regulator connections are not described in firmware.
+	 * This is taken care of by platform code, but this causes probe-order
+	 * challenges. This also applies to the sensors and the platform code
+	 * takes care of delaying the probing of the sensors until the
+	 * regulator connection info is in place. So the sensors all being
+	 * in place means it is now also ok to probe VCMs.
+	 */
+	cio2_bridge_instantiate_vcm_devices(cio2->bridge);
+
 	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
 }
 
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 64d7ca502124..19fd64613c2d 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -463,11 +463,13 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
 
 #if IS_ENABLED(CONFIG_CIO2_BRIDGE)
 struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2);
+void cio2_bridge_instantiate_vcm_devices(struct cio2_bridge *bridge);
 #else
 static inline struct cio2_bridge *cio2_bridge_init(struct pci_dev *cio2)
 {
 	return NULL;
 }
+static inline void cio2_bridge_instantiate_vcm_devices(struct cio2_bridge *b) {}
 #endif
 
 #endif
-- 
2.31.1