Sakari,
------------------------------------------------------------------------
BRs,
Bingbu Cao
>-----Original Message-----
>From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>Sent: Wednesday, November 1, 2023 3:12 PM
>To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
>Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; 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>; linux-
>media@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH 11/12] media: intel-cio2-bridge: Add a runtime-pm
>device-link between VCM and sensor
>
>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.
Ack. Thanks!
>
>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
