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.
Does this 'requirement' or limit really make sense?
>
>>
>> Right. I believe that having runtime pm support is more or less a hard
>> requirement for sensor drivers used on x86. This is also necessary in
>> cases where the power on/off sequence is (partially) implemented in
>> ACPI AML code in the _PS0 and _PS3 methods of the sensor's ACPI device.
>>
>> This happens on [atom]ISP2 based devices and AFAIK also on ChromeOS
>> devices.
>>
>> So I think it is safe to assume that sensor drivers implement runtime
>> pm support and in cases where the driver does not we need to fix the
>driver.
>
>I agree.
>
>--
>Regards,
>
>Sakari Ailus
