Hans,
On 6/28/23 1:56 AM, 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.
>
> 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 intel_cio2_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/media/common/intel-cio2-bridge.c | 160 +++++++++++-------
> drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 5 +
> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 3 +
> include/media/intel-cio2-bridge.h | 4 +-
> 4 files changed, 111 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/media/common/intel-cio2-bridge.c b/drivers/media/common/intel-cio2-bridge.c
> index 2f4256f9152c..997581bcc3b9 100644
> --- a/drivers/media/common/intel-cio2-bridge.c
> +++ b/drivers/media/common/intel-cio2-bridge.c
> @@ -5,7 +5,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/intel-cio2-bridge.h>
> #include <media/v4l2-fwnode.h>
> @@ -189,30 +191,111 @@ static void cio2_bridge_create_connection_swnodes(
> cio2_bridge_init_swnode_group(sensor);
> }
>
> -static void cio2_bridge_instantiate_vcm_i2c_client(
> - struct intel_cio2_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 intel_cio2_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 intel_cio2_bridge_instantiate_vcm_work(struct work_struct *_work)
> +{
> + struct intel_cio2_bridge_instantiate_vcm_work_data *work =
> + container_of(_work,
> + struct intel_cio2_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;
> }
One question here: how do we make sure that the runtime PM of the sensor
is enabled during the .bound callback? Or is it a mandatory requirement
of driver of such camera sensors?
> +
> + /*
> + * 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 intel_cio2_bridge_instantiate_vcm(struct device *sensor)
> +{
> + struct intel_cio2_bridge_instantiate_vcm_work_data *work;
> + struct acpi_device *adev = ACPI_COMPANION(sensor);
> + struct fwnode_handle *vcm_fwnode;
> + struct i2c_client *vcm_client;
> + char *sep;
> +
> + 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, intel_cio2_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(intel_cio2_bridge_instantiate_vcm);
> +
> static void cio2_bridge_unregister_sensors(struct intel_cio2_bridge *bridge)
> {
> struct intel_cio2_sensor *sensor;
> @@ -222,7 +305,6 @@ static void cio2_bridge_unregister_sensors(struct intel_cio2_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);
> }
> }
>
> @@ -273,8 +355,6 @@ static int cio2_bridge_connect_sensor(
> primary = acpi_fwnode_handle(adev);
> primary->secondary = fwnode;
>
> - cio2_bridge_instantiate_vcm_i2c_client(sensor);
> -
> dev_info(bridge->dev, "Found supported sensor %s\n",
> acpi_dev_name(adev));
>
> @@ -311,41 +391,6 @@ static int cio2_bridge_connect_sensors(struct intel_cio2_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 cio2_bridge_sensors_are_ready(
> - const struct intel_cio2_sensor_config *supported_sensors)
> -{
> - struct acpi_device *adev;
> - bool ready = true;
> - unsigned int i;
> -
> - for (i = 0; supported_sensors[i].hid; i++) {
> - const struct intel_cio2_sensor_config *cfg =
> - &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 intel_cio2_bridge_init(
> struct device *dev,
> int (*parse_sensor_fwnode)(struct acpi_device *adev,
> @@ -358,9 +403,6 @@ int intel_cio2_bridge_init(
> unsigned int i;
> int ret;
>
> - if (!cio2_bridge_sensors_are_ready(supported_sensors))
> - return -EPROBE_DEFER;
> -
> bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> if (!bridge)
> return -ENOMEM;
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index afc3255b5eb8..a22d213d66e9 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1386,10 +1386,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 = intel_cio2_bridge_instantiate_vcm(sd->dev);
> + if (ret)
> + return ret;
> +
> q = &cio2->queue[s_asd->csi2.port];
>
> q->csi2 = s_asd->csi2;
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index 7ff7915f9823..208d60364f44 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -12,6 +12,7 @@
>
> #include <asm/page.h>
>
> +#include <media/intel-cio2-bridge.h>
> #include <media/media-device.h>
> #include <media/media-entity.h>
> #include <media/v4l2-async.h>
> @@ -463,6 +464,8 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
> int cio2_bridge_init(struct device *dev);
> #else
> static inline int cio2_bridge_init(struct device *dev) { return 0; }
> +static inline int
> +intel_cio2_bridge_instantiate_vcm(struct device *dev) { return 0; }
> #endif
>
> #endif
> diff --git a/include/media/intel-cio2-bridge.h b/include/media/intel-cio2-bridge.h
> index 90fde180faef..faa60a5b1cd8 100644
> --- a/include/media/intel-cio2-bridge.h
> +++ b/include/media/intel-cio2-bridge.h
> @@ -10,7 +10,6 @@
> #include <media/v4l2-fwnode.h>
>
> struct acpi_device;
> -struct i2c_client;
>
> #define INTEL_CIO2_HID "INT343E"
> #define INTEL_CIO2_MAX_LANES 4
> @@ -56,7 +55,6 @@ struct intel_cio2_sensor {
> /* append 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];
> @@ -99,6 +97,8 @@ int intel_cio2_bridge_init(
> const struct intel_cio2_sensor_config *cfg),
> const struct intel_cio2_sensor_config *supported_sensors);
>
> +int intel_cio2_bridge_instantiate_vcm(struct device *sensor);
> +
> enum v4l2_fwnode_orientation
> intel_cio2_bridge_get_orientation(struct acpi_device *adev);
>
>
--
Best regards,
Bingbu Cao
