diff mbox series

[v2,14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

Message ID 20230630110643.209761-15-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand

Commit Message

Hans de Goede June 30, 2023, 11:06 a.m. UTC 
In most cases when a VCM is used there is a single integrated module
with the sensor + VCM + lens. This means that the sensor and VCM often
share regulators and possibly also something like a powerdown pin.

In the ACPI tables this is modelled as a single ACPI device with
multiple I2cSerialBus resources.

On atomisp devices the regulators and clks are modelled as ACPI
power-resources, which are controlled by the (ACPI) power state
of the sensor. So the sensor must be in D0 power state for the VCM
to work.

To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
so that the sensor will automatically be runtime-resumed whenever
the VCM is runtime-resumed.

This requires the probing of the VCM and thus the creation of the VCM
I2C-client to be delayed till after the sensor driver has bound.

Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
bound op, so that it is done after the sensor driver has bound; and
add code to add the device-link.

This fixes the problem with the shared ACPI power-resources on atomisp2
and this avoids the need for VCM related workarounds on IPU3 / IPU6.

E.g. until now the dw9719 driver needed to get and control a Vsio
(V sensor IO) regulator since that needs to be enabled to enable I2C
pass-through on the PMIC on the sensor module. So the driver was
controlling this regulator even though the actual dw9719 chip has no
Vsio pin / power-plane.

This also removes the need for ipu_bridge_init() to return
-EPROBE_DEFER since the VCM is now instantiated later.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c     | 159 +++++++++++++++--------
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |   5 +
 include/media/ipu-bridge.h               |   5 +-
 3 files changed, 109 insertions(+), 60 deletions(-)

Comments

Andy Shevchenko June 30, 2023, 2:47 p.m. UTC | #1 
On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> In most cases when a VCM is used there is a single integrated module
> with the sensor + VCM + lens. This means that the sensor and VCM often
> share regulators and possibly also something like a powerdown pin.
>
> In the ACPI tables this is modelled as a single ACPI device with
> multiple I2cSerialBus resources.
>
> On atomisp devices the regulators and clks are modelled as ACPI
> power-resources, which are controlled by the (ACPI) power state
> of the sensor. So the sensor must be in D0 power state for the VCM
> to work.
>
> To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
> so that the sensor will automatically be runtime-resumed whenever
> the VCM is runtime-resumed.
>
> This requires the probing of the VCM and thus the creation of the VCM
> I2C-client to be delayed till after the sensor driver has bound.
>
> Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
> bound op, so that it is done after the sensor driver has bound; and
> add code to add the device-link.
>
> This fixes the problem with the shared ACPI power-resources on atomisp2
> and this avoids the need for VCM related workarounds on IPU3 / IPU6.
>
> E.g. until now the dw9719 driver needed to get and control a Vsio
> (V sensor IO) regulator since that needs to be enabled to enable I2C
> pass-through on the PMIC on the sensor module. So the driver was
> controlling this regulator even though the actual dw9719 chip has no
> Vsio pin / power-plane.
>
> This also removes the need for ipu_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.

...

> +static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
> +{
> +       struct ipu_bridge_instantiate_vcm_work_data *work =
> +               container_of(_work,
> +                            struct ipu_bridge_instantiate_vcm_work_data,
> +                            work);

Just noticed this plenty of *work.

Perhaps call the parameter work and the stack variable wdata or so?

>  }
Daniel Scally July 4, 2023, 3:07 p.m. UTC | #2 
Hi Hans

On 30/06/2023 13:06, Hans de Goede wrote:
> In most cases when a VCM is used there is a single integrated module
> with the sensor + VCM + lens. This means that the sensor and VCM often
> share regulators and possibly also something like a powerdown pin.
>
> In the ACPI tables this is modelled as a single ACPI device with
> multiple I2cSerialBus resources.
>
> On atomisp devices the regulators and clks are modelled as ACPI
> power-resources, which are controlled by the (ACPI) power state
> of the sensor. So the sensor must be in D0 power state for the VCM
> to work.
>
> To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
> so that the sensor will automatically be runtime-resumed whenever
> the VCM is runtime-resumed.
>
> This requires the probing of the VCM and thus the creation of the VCM
> I2C-client to be delayed till after the sensor driver has bound.
>
> Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
> bound op, so that it is done after the sensor driver has bound; and
> add code to add the device-link.
>
> This fixes the problem with the shared ACPI power-resources on atomisp2
> and this avoids the need for VCM related workarounds on IPU3 / IPU6.
>
> E.g. until now the dw9719 driver needed to get and control a Vsio
> (V sensor IO) regulator since that needs to be enabled to enable I2C
> pass-through on the PMIC on the sensor module. So the driver was
> controlling this regulator even though the actual dw9719 chip has no
> Vsio pin / power-plane.
>
> This also removes the need for ipu_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


This is a really cool way of solving the problem I think; Sakari mentioned it to me a little while 
ago, it's nice to see the implementation, thanks for doing it. Just one comment below for 
ipu_bridge_instantiate_vcm(), but:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>


I also tested the set on my Go and as far as I can tell everything works fine. Not really applicable 
to 13/15 since that's atomisp specific but otherwise:


Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/media/pci/intel/ipu-bridge.c     | 159 +++++++++++++++--------
>   drivers/media/pci/intel/ipu3/ipu3-cio2.c |   5 +
>   include/media/ipu-bridge.h               |   5 +-
>   3 files changed, 109 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 07a34f20af8e..32dabc16a7b4 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -4,7 +4,9 @@
>   #include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/property.h>
> +#include <linux/workqueue.h>
>   
>   #include <media/ipu-bridge.h>
>   #include <media/v4l2-fwnode.h>
> @@ -289,29 +291,112 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>   	ipu_bridge_init_swnode_group(sensor);
>   }
>   
> -static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
> -{
> -	struct i2c_board_info board_info = { };
> +/*
> + * The actual instantiation must be done from a workqueue to avoid
> + * a deadlock on taking list_lock from v4l2-async twice.
> + */
> +struct ipu_bridge_instantiate_vcm_work_data {
> +	struct work_struct work;
> +	struct device *sensor;
>   	char name[16];
> +	struct i2c_board_info board_info;
> +};
>   
> -	if (!sensor->vcm_type)
> -		return;
> +static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
> +{
> +	struct ipu_bridge_instantiate_vcm_work_data *work =
> +		container_of(_work,
> +			     struct ipu_bridge_instantiate_vcm_work_data,
> +			     work);
> +	struct acpi_device *adev = ACPI_COMPANION(work->sensor);
> +	struct i2c_client *vcm_client;
> +	bool put_fwnode = true;
> +	int ret;
>   
> -	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
> -	board_info.dev_name = name;
> -	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
> -	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
> -
> -	sensor->vcm_i2c_client =
> -		i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(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;
> +	/*
> +	 * The client may get probed before the device_link gets added below
> +	 * make sure the sensor is powered-up during probe.
> +	 */
> +	ret = pm_runtime_get_sync(work->sensor);
> +	if (ret < 0) {
> +		dev_err(work->sensor, "Error %d runtime-resuming sensor, cannot instantiate VCM\n",
> +			ret);
> +		goto out;
>   	}
> +
> +	/*
> +	 * Note the client is created only once and then kept around
> +	 * even after a rmmod, just like the software-nodes.
> +	 */
> +	vcm_client = i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(adev),
> +						   1, &work->board_info);
> +	if (IS_ERR(vcm_client)) {
> +		dev_err(work->sensor, "Error instantiating VCM client: %ld\n",
> +			PTR_ERR(vcm_client));
> +		goto out;
> +	}
> +
> +	device_link_add(&vcm_client->dev, work->sensor, DL_FLAG_PM_RUNTIME);
> +
> +	dev_info(work->sensor, "Instantiated %s VCM\n", work->board_info.type);
> +	put_fwnode = false; /* Ownership has passed to the i2c-client */
> +
> +out:
> +	pm_runtime_put(work->sensor);
> +	put_device(work->sensor);
> +	if (put_fwnode)
> +		fwnode_handle_put(work->board_info.fwnode);
> +	kfree(work);
>   }
>   
> +int ipu_bridge_instantiate_vcm(struct device *sensor)
> +{
> +	struct ipu_bridge_instantiate_vcm_work_data *work;
> +	struct fwnode_handle *vcm_fwnode;
> +	struct i2c_client *vcm_client;
> +	struct acpi_device *adev;
> +	char *sep;
> +
> +	adev = ACPI_COMPANION(sensor);
> +	if (!adev)
> +		return 0;
> +
> +	vcm_fwnode = fwnode_find_reference(dev_fwnode(sensor), "lens-focus", 0);
> +	if (IS_ERR(vcm_fwnode))
> +		return 0;
> +
> +	/* When reloading modules the client will already exist */
> +	vcm_client = i2c_find_device_by_fwnode(vcm_fwnode);
> +	if (vcm_client) {
> +		fwnode_handle_put(vcm_fwnode);
> +		put_device(&vcm_client->dev);
> +		return 0;
> +	}
> +
> +	work = kzalloc(sizeof(*work), GFP_KERNEL);
> +	if (!work) {
> +		fwnode_handle_put(vcm_fwnode);
> +		return -ENOMEM;
> +	}
> +
> +	INIT_WORK(&work->work, ipu_bridge_instantiate_vcm_work);
> +	work->sensor = get_device(sensor);
> +	snprintf(work->name, sizeof(work->name), "%s-VCM",
> +		 acpi_dev_name(adev));
> +	work->board_info.dev_name = work->name;
> +	work->board_info.fwnode = vcm_fwnode;
> +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> +		I2C_NAME_SIZE);
> +	/* Strip "-<link>" postfix */
> +	sep = strchrnul(work->board_info.type, '-');
> +	*sep = 0;

I think strreplace(work->board_info.type, '-', '\0') here would be cleaner, and either way probably 
we need #include <linux/string.h> for the str* funcs here


> +
> +	queue_work(system_long_wq, &work->work);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_bridge_instantiate_vcm);
> +
>   static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   {
>   	struct ipu_sensor *sensor;
> @@ -321,7 +406,6 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   		sensor = &bridge->sensors[i];
>   		software_node_unregister_node_group(sensor->group);
>   		acpi_dev_put(sensor->adev);
> -		i2c_unregister_device(sensor->vcm_i2c_client);
>   	}
>   }
>   
> @@ -371,8 +455,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   		primary = acpi_fwnode_handle(adev);
>   		primary->secondary = fwnode;
>   
> -		ipu_bridge_instantiate_vcm_i2c_client(sensor);
> -
>   		dev_info(bridge->dev, "Found supported sensor %s\n",
>   			 acpi_dev_name(adev));
>   
> @@ -409,40 +491,6 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>   	return ret;
>   }
>   
> -/*
> - * The VCM cannot be probed until the PMIC is completely setup. We cannot rely
> - * on -EPROBE_DEFER for this, since the consumer<->supplier relations between
> - * the VCM and regulators/clks are not described in ACPI, instead they are
> - * passed as board-data to the PMIC drivers. Since -PROBE_DEFER does not work
> - * for the clks/regulators the VCM i2c-clients must not be instantiated until
> - * the PMIC is fully setup.
> - *
> - * The sensor/VCM ACPI device has an ACPI _DEP on the PMIC, check this using the
> - * acpi_dev_ready_for_enumeration() helper, like the i2c-core-acpi code does
> - * for the sensors.
> - */
> -static int ipu_bridge_sensors_are_ready(void)
> -{
> -	struct acpi_device *adev;
> -	bool ready = true;
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> -		const struct ipu_sensor_config *cfg =
> -			&ipu_supported_sensors[i];
> -
> -		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> -			if (!adev->status.enabled)
> -				continue;
> -
> -			if (!acpi_dev_ready_for_enumeration(adev))
> -				ready = false;
> -		}
> -	}
> -
> -	return ready;
> -}
> -
>   int ipu_bridge_init(struct device *dev,
>   		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
>   					       struct ipu_sensor *sensor))
> @@ -452,9 +500,6 @@ int ipu_bridge_init(struct device *dev,
>   	unsigned int i;
>   	int ret;
>   
> -	if (!ipu_bridge_sensors_are_ready())
> -		return -EPROBE_DEFER;
> -
>   	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
>   	if (!bridge)
>   		return -ENOMEM;
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 51a6d7cc44d2..690fc1c919af 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1388,10 +1388,15 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>   	struct cio2_device *cio2 = to_cio2_device(notifier);
>   	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
>   	struct cio2_queue *q;
> +	int ret;
>   
>   	if (cio2->queue[s_asd->csi2.port].sensor)
>   		return -EBUSY;
>   
> +	ret = ipu_bridge_instantiate_vcm(sd->dev);
> +	if (ret)
> +		return ret;
> +
>   	q = &cio2->queue[s_asd->csi2.port];
>   
>   	q->csi2 = s_asd->csi2;
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 969d8d7d6b93..31b138170c73 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -7,8 +7,6 @@
>   #include <linux/types.h>
>   #include <media/v4l2-fwnode.h>
>   
> -struct i2c_client;
> -
>   #define IPU_HID				"INT343E"
>   #define IPU_MAX_LANES				4
>   #define IPU_MAX_PORTS				4
> @@ -117,7 +115,6 @@ struct ipu_sensor {
>   	/* append ssdb.link(u8) in "-%u" format as suffix of HID */
>   	char name[ACPI_ID_LEN + 4];
>   	struct acpi_device *adev;
> -	struct i2c_client *vcm_i2c_client;
>   
>   	/* SWNODE_COUNT + 1 for terminating NULL */
>   	const struct software_node *group[SWNODE_COUNT + 1];
> @@ -156,9 +153,11 @@ int ipu_bridge_init(struct device *dev,
>   		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
>   					       struct ipu_sensor *sensor));
>   int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor);
> +int ipu_bridge_instantiate_vcm(struct device *sensor);
>   #else
>   /* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */
>   #define ipu_bridge_init(dev, parse_sensor_fwnode)	(0)
> +static inline int ipu_bridge_instantiate_vcm(struct device *s) { return 0; }
>   #endif
>   
>   #endif
Andy Shevchenko July 4, 2023, 3:27 p.m. UTC | #3 
On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
> On 30/06/2023 13:06, Hans de Goede wrote:

...

> > +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> > +		I2C_NAME_SIZE);
> > +	/* Strip "-<link>" postfix */
> > +	sep = strchrnul(work->board_info.type, '-');
> > +	*sep = 0;
> 
> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
> and either way probably we need #include <linux/string.h> for the str* funcs
> here

What we need is something like strcut(str, '<$CHAR>').

But related to the above code we can (besides using sizeof() instead
of I2C_NAME_SIZE):

	snprintf(work->board_info.type, sizeof(work->board_info.type),
		 "%pfwP", vcm_fwnode);
Hans de Goede July 4, 2023, 3:52 p.m. UTC | #4 
Hi,

On 7/4/23 17:27, Andy Shevchenko wrote:
> On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
>> On 30/06/2023 13:06, Hans de Goede wrote:
> 
> ...
> 
>>> +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
>>> +		I2C_NAME_SIZE);
>>> +	/* Strip "-<link>" postfix */
>>> +	sep = strchrnul(work->board_info.type, '-');
>>> +	*sep = 0;
>>
>> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
>> and either way probably we need #include <linux/string.h> for the str* funcs
>> here
> 
> What we need is something like strcut(str, '<$CHAR>').
> 
> But related to the above code we can (besides using sizeof() instead
> of I2C_NAME_SIZE):
> 
> 	snprintf(work->board_info.type, sizeof(work->board_info.type),
> 		 "%pfwP", vcm_fwnode);

"%pfwP" ? what on earth does that do ?

Regards,

Hans
Andy Shevchenko July 5, 2023, 8:43 a.m. UTC | #5 
On Tue, Jul 4, 2023 at 6:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/4/23 17:27, Andy Shevchenko wrote:
> > On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
> >> On 30/06/2023 13:06, Hans de Goede wrote:

...

> >>> +   strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> >>> +           I2C_NAME_SIZE);
> >>> +   /* Strip "-<link>" postfix */
> >>> +   sep = strchrnul(work->board_info.type, '-');
> >>> +   *sep = 0;
> >>
> >> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
> >> and either way probably we need #include <linux/string.h> for the str* funcs
> >> here
> >
> > What we need is something like strcut(str, '<$CHAR>').
> >
> > But related to the above code we can (besides using sizeof() instead
> > of I2C_NAME_SIZE):
> >
> >       snprintf(work->board_info.type, sizeof(work->board_info.type),
> >                "%pfwP", vcm_fwnode);
>
> "%pfwP" ? what on earth does that do ?

Prints fwnode name (as you opencoded with strscpy() + fwnode_get_name() above).
diff mbox series

Patch

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 07a34f20af8e..32dabc16a7b4 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,7 +4,9 @@ 
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
 #include <media/v4l2-fwnode.h>
@@ -289,29 +291,112 @@  static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 	ipu_bridge_init_swnode_group(sensor);
 }
 
-static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
-{
-	struct i2c_board_info board_info = { };
+/*
+ * The actual instantiation must be done from a workqueue to avoid
+ * a deadlock on taking list_lock from v4l2-async twice.
+ */
+struct ipu_bridge_instantiate_vcm_work_data {
+	struct work_struct work;
+	struct device *sensor;
 	char name[16];
+	struct i2c_board_info board_info;
+};
 
-	if (!sensor->vcm_type)
-		return;
+static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
+{
+	struct ipu_bridge_instantiate_vcm_work_data *work =
+		container_of(_work,
+			     struct ipu_bridge_instantiate_vcm_work_data,
+			     work);
+	struct acpi_device *adev = ACPI_COMPANION(work->sensor);
+	struct i2c_client *vcm_client;
+	bool put_fwnode = true;
+	int ret;
 
-	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
-	board_info.dev_name = name;
-	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
-	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
-
-	sensor->vcm_i2c_client =
-		i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(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;
+	/*
+	 * The client may get probed before the device_link gets added below
+	 * make sure the sensor is powered-up during probe.
+	 */
+	ret = pm_runtime_get_sync(work->sensor);
+	if (ret < 0) {
+		dev_err(work->sensor, "Error %d runtime-resuming sensor, cannot instantiate VCM\n",
+			ret);
+		goto out;
 	}
+
+	/*
+	 * Note the client is created only once and then kept around
+	 * even after a rmmod, just like the software-nodes.
+	 */
+	vcm_client = i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(adev),
+						   1, &work->board_info);
+	if (IS_ERR(vcm_client)) {
+		dev_err(work->sensor, "Error instantiating VCM client: %ld\n",
+			PTR_ERR(vcm_client));
+		goto out;
+	}
+
+	device_link_add(&vcm_client->dev, work->sensor, DL_FLAG_PM_RUNTIME);
+
+	dev_info(work->sensor, "Instantiated %s VCM\n", work->board_info.type);
+	put_fwnode = false; /* Ownership has passed to the i2c-client */
+
+out:
+	pm_runtime_put(work->sensor);
+	put_device(work->sensor);
+	if (put_fwnode)
+		fwnode_handle_put(work->board_info.fwnode);
+	kfree(work);
 }
 
+int ipu_bridge_instantiate_vcm(struct device *sensor)
+{
+	struct ipu_bridge_instantiate_vcm_work_data *work;
+	struct fwnode_handle *vcm_fwnode;
+	struct i2c_client *vcm_client;
+	struct acpi_device *adev;
+	char *sep;
+
+	adev = ACPI_COMPANION(sensor);
+	if (!adev)
+		return 0;
+
+	vcm_fwnode = fwnode_find_reference(dev_fwnode(sensor), "lens-focus", 0);
+	if (IS_ERR(vcm_fwnode))
+		return 0;
+
+	/* When reloading modules the client will already exist */
+	vcm_client = i2c_find_device_by_fwnode(vcm_fwnode);
+	if (vcm_client) {
+		fwnode_handle_put(vcm_fwnode);
+		put_device(&vcm_client->dev);
+		return 0;
+	}
+
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		fwnode_handle_put(vcm_fwnode);
+		return -ENOMEM;
+	}
+
+	INIT_WORK(&work->work, ipu_bridge_instantiate_vcm_work);
+	work->sensor = get_device(sensor);
+	snprintf(work->name, sizeof(work->name), "%s-VCM",
+		 acpi_dev_name(adev));
+	work->board_info.dev_name = work->name;
+	work->board_info.fwnode = vcm_fwnode;
+	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
+		I2C_NAME_SIZE);
+	/* Strip "-<link>" postfix */
+	sep = strchrnul(work->board_info.type, '-');
+	*sep = 0;
+
+	queue_work(system_long_wq, &work->work);
+
+	return 0;
+}
+EXPORT_SYMBOL(ipu_bridge_instantiate_vcm);
+
 static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 {
 	struct ipu_sensor *sensor;
@@ -321,7 +406,6 @@  static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 		sensor = &bridge->sensors[i];
 		software_node_unregister_node_group(sensor->group);
 		acpi_dev_put(sensor->adev);
-		i2c_unregister_device(sensor->vcm_i2c_client);
 	}
 }
 
@@ -371,8 +455,6 @@  static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 		primary = acpi_fwnode_handle(adev);
 		primary->secondary = fwnode;
 
-		ipu_bridge_instantiate_vcm_i2c_client(sensor);
-
 		dev_info(bridge->dev, "Found supported sensor %s\n",
 			 acpi_dev_name(adev));
 
@@ -409,40 +491,6 @@  static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 	return ret;
 }
 
-/*
- * The VCM cannot be probed until the PMIC is completely setup. We cannot rely
- * on -EPROBE_DEFER for this, since the consumer<->supplier relations between
- * the VCM and regulators/clks are not described in ACPI, instead they are
- * passed as board-data to the PMIC drivers. Since -PROBE_DEFER does not work
- * for the clks/regulators the VCM i2c-clients must not be instantiated until
- * the PMIC is fully setup.
- *
- * The sensor/VCM ACPI device has an ACPI _DEP on the PMIC, check this using the
- * acpi_dev_ready_for_enumeration() helper, like the i2c-core-acpi code does
- * for the sensors.
- */
-static int ipu_bridge_sensors_are_ready(void)
-{
-	struct acpi_device *adev;
-	bool ready = true;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
-		const struct ipu_sensor_config *cfg =
-			&ipu_supported_sensors[i];
-
-		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
-			if (!adev->status.enabled)
-				continue;
-
-			if (!acpi_dev_ready_for_enumeration(adev))
-				ready = false;
-		}
-	}
-
-	return ready;
-}
-
 int ipu_bridge_init(struct device *dev,
 		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
 					       struct ipu_sensor *sensor))
@@ -452,9 +500,6 @@  int ipu_bridge_init(struct device *dev,
 	unsigned int i;
 	int ret;
 
-	if (!ipu_bridge_sensors_are_ready())
-		return -EPROBE_DEFER;
-
 	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
 	if (!bridge)
 		return -ENOMEM;
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 51a6d7cc44d2..690fc1c919af 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1388,10 +1388,15 @@  static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
 	struct cio2_device *cio2 = to_cio2_device(notifier);
 	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
 	struct cio2_queue *q;
+	int ret;
 
 	if (cio2->queue[s_asd->csi2.port].sensor)
 		return -EBUSY;
 
+	ret = ipu_bridge_instantiate_vcm(sd->dev);
+	if (ret)
+		return ret;
+
 	q = &cio2->queue[s_asd->csi2.port];
 
 	q->csi2 = s_asd->csi2;
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 969d8d7d6b93..31b138170c73 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -7,8 +7,6 @@ 
 #include <linux/types.h>
 #include <media/v4l2-fwnode.h>
 
-struct i2c_client;
-
 #define IPU_HID				"INT343E"
 #define IPU_MAX_LANES				4
 #define IPU_MAX_PORTS				4
@@ -117,7 +115,6 @@  struct ipu_sensor {
 	/* append ssdb.link(u8) in "-%u" format as suffix of HID */
 	char name[ACPI_ID_LEN + 4];
 	struct acpi_device *adev;
-	struct i2c_client *vcm_i2c_client;
 
 	/* SWNODE_COUNT + 1 for terminating NULL */
 	const struct software_node *group[SWNODE_COUNT + 1];
@@ -156,9 +153,11 @@  int ipu_bridge_init(struct device *dev,
 		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
 					       struct ipu_sensor *sensor));
 int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor);
+int ipu_bridge_instantiate_vcm(struct device *sensor);
 #else
 /* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */
 #define ipu_bridge_init(dev, parse_sensor_fwnode)	(0)
+static inline int ipu_bridge_instantiate_vcm(struct device *s) { return 0; }
 #endif
 
 #endif