Hi Bingbu,
On Wed, Nov 01, 2023 at 06:26:39AM +0000, Cao, Bingbu wrote:
> Sakari and Hans,
>
> ------------------------------------------------------------------------
> BRs,
> Bingbu Cao
>
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >Sent: Monday, October 30, 2023 5:24 PM
> >To: Hans de Goede <hdegoede@xxxxxxxxxx>
> >Cc: Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx>; Laurent Pinchart
> ><laurent.pinchart@xxxxxxxxxxxxxxxx>; Daniel Scally
> ><dan.scally@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> >Andy Shevchenko <andy@xxxxxxxxxx>; Kate Hsuan <hpa@xxxxxxxxxx>; Cao, Bingbu
> ><bingbu.cao@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx
> >Subject: Re: [PATCH 11/12] media: intel-cio2-bridge: Add a runtime-pm
> >device-link between VCM and sensor
> >
> >Hi Hans,
> >
> >On Mon, Oct 30, 2023 at 09:58:09AM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/30/23 09:55, Sakari Ailus wrote:
> >> > Hi Bingbu,
> >> >
> >> > On Mon, Oct 30, 2023 at 04:30:39PM +0800, Bingbu Cao wrote:
> >> >>> +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?
> >> >
> >> > The sensor driver needs to enable runtime PM in probe, otherwise
> >> > this won't work. But I don't see why a driver wouldn't? Of course
> >> > otherwise it wouldn't be a hard requirement.
> >
> >I meant to write "wasn't earlier a hard requirement". This could also be
> >documented, I can write a patch for this.
>
> The problem here is that some driver doesn't enable the runtime PM before
> v4l2_async_register_subdev_sensor(), that may cause race condition and
> VCM instantiation failure as the runtime PM is not enabled yet. Then there
> is no chance for VCM instantiation to succeed.
These drivers need to be fixed.
All initialisation needs to complete by the time the async sub-device is
registered as this is when the sensor may be bound to a notifier and also
accessible via the UAPI.
In most of the cases the rest of the probe completes even if runtime PM is
enabled after registering the sub-device (without IPU bridge) but it's
still not correct.
>
> Does this 'requirement' or limit really make sense?
--
Regards,
Sakari Ailus
