Re: [PATCH] media: dw9768: activate runtime PM and turn off device

[Bingbu Cao <bingbu.cao@linux.intel.com>]

On 4/12/22 5:39 PM, Tomasz Figa wrote:
> On Tue, Nov 16, 2021 at 1:57 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>
>> On Fri, Nov 5, 2021 at 9:52 PM Cao, Bingbu <bingbu.cao@intel.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Tomasz Figa <tfiga@chromium.org>
>>>> Sent: Friday, November 5, 2021 2:55 PM
>>>> To: Cao, Bingbu <bingbu.cao@intel.com>
>>>> Cc: linux-media@vger.kernel.org; sakari.ailus@linux.intel.com;
>>>> dongchun.zhu@mediatek.com; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
>>>> bingbu.cao@linux.intel.com
>>>> Subject: Re: [PATCH] media: dw9768: activate runtime PM and turn off
>>>> device
>>>>
>>>> On Fri, Oct 15, 2021 at 3:12 PM Bingbu Cao <bingbu.cao@intel.com> wrote:
>>>>>
>>>>> When dw9768 working with ACPI systems, the dw9768 was turned by
>>>>> i2c-core during probe, driver need activate the PM runtime and ask
>>>>> runtime PM to turn off the device.
>>>>>
>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>>>>> ---
>>>>>  drivers/media/i2c/dw9768.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
>>>>> index c086580efac7..65c6acf3ced9 100644
>>>>> --- a/drivers/media/i2c/dw9768.c
>>>>> +++ b/drivers/media/i2c/dw9768.c
>>>>> @@ -469,6 +469,11 @@ static int dw9768_probe(struct i2c_client
>>>>> *client)
>>>>>
>>>>>         dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
>>>>>
>>>>> +       /*
>>>>> +        * Device is already turned on by i2c-core with ACPI domain PM.
>>>>> +        * Attempt to turn off the device to satisfy the privacy LED
>>>> concerns.
>>>>> +        */
>>>>> +       pm_runtime_set_active(dev);
>>>>
>>>> This driver is used by non-ACPI systems as well. This change will make
>>>> the PM core not call the runtime_resume() callback provided by the
>>>> driver and the power would never be turned on on such systems.
>>>>
>>>
>>>> Wasn't the intention of Sakari's ACPI patches to allow bypassing the
>>>> ACPI domain power on at boot up and eliminate the need for this change?
>>>
>>> Tomasz, thanks for your review.
>>>
>>> The comment here is invalid, it should not be strongly related to the privacy
>>> LED concern. Anyway, the device should be turned off on ACPI and non-ACPI
>>> systems even without Sakari's changes.
>>>
>>> I am wondering how the driver work with PM core on non-ACPI system.
>>>
>>
>> On non-ACPI systems it's the driver which handles the power sequencing
>> of the chip so the regulators wouldn't be implicitly enabled by the
>> subsystem before (unless they're shared with some other device and the
>> corresponding driver enabled them).
> 
> It looks like this patch made into Linus' tree and broke the driver on
> ARM devices. Could we please revert it?

If revert the patch, the device will not work on ACPI system, is there some
other solution? Have no details about the failure on ARM device.

> 
> Best regards,
> Tomasz
> 
>>
>>>>
>>>> Best regards,
>>>> Tomasz
>>>>
>>>>>
>>>>>         pm_runtime_enable(dev);
>>>>>         if (!pm_runtime_enabled(dev)) {
>>>>>                 ret = dw9768_runtime_resume(dev); @@ -483,6 +488,7 @@
>>>>> static int dw9768_probe(struct i2c_client *client)
>>>>>                 dev_err(dev, "failed to register V4L2 subdev: %d",
>>>> ret);
>>>>>                 goto err_power_off;
>>>>>         }
>>>>> +       pm_runtime_idle(dev);
>>>>>
>>>>>         return 0;
>>>>>
>>>>> --
>>>>> 2.7.4
>>>>>

-- 
Best regards,
Bingbu Cao