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

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

[Bingbu Cao]

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);
 	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

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

[Tomasz Figa]

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?

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
>

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

[Cao, Bingbu]

> -----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.

> 
> 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
> >

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

[Tomasz Figa]

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).

> >
> > 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
> > >

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

[Tomasz Figa]

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?

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
> > > >

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

[Bingbu Cao]

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

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

[Tomasz Figa]

On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
>
>
> 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.
>

I believe it worked on ACPI systems, just runtime PM wasn't suspending
the device.

That said, if my comment above was addressed instead of being ignored,
this regression wouldn't have happened. The problem is described in my
previous messages, please get back to them and address the issue I
pointed out.

Best regards,
Tomasz

> >
> > 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

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

[sakari.ailus]

Hi Tomasz, Bingbu,

On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
> On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> >
> >
> >
> > 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.
> >
> 
> I believe it worked on ACPI systems, just runtime PM wasn't suspending
> the device.
> 
> That said, if my comment above was addressed instead of being ignored,
> this regression wouldn't have happened. The problem is described in my
> previous messages, please get back to them and address the issue I
> pointed out.

First of all, thanks for catching this.

What I believe happened was that the patch was merged to my tree before you
commented on it and then I missed the related follow-up discussion.

Looking at the patch itself, it seems fine as such but there's a problem
with the driver to begin with: the device isn't powered on in probe on DT
systems but still its runtime suspend callback is called through
pm_runtime_idle().

Normally calling the RT suspend callback is what we want, but in this case
disabling a regulator that wasn't enabled is a problem.

There also seems to be a problem in error handling... and the driver does
not support probing while powered off on ACPI. Oh well.

Let's revert the patch now but it seems there's something to fix
afterwards.

-- 
Regards,

Sakari Ailus

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

[Tomasz Figa]

On Wed, Apr 13, 2022 at 8:40 PM sakari.ailus@linux.intel.com
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz, Bingbu,
>
> On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
> > On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > >
> > >
> > >
> > > 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.
> > >
> >
> > I believe it worked on ACPI systems, just runtime PM wasn't suspending
> > the device.
> >
> > That said, if my comment above was addressed instead of being ignored,
> > this regression wouldn't have happened. The problem is described in my
> > previous messages, please get back to them and address the issue I
> > pointed out.
>
> First of all, thanks for catching this.
>
> What I believe happened was that the patch was merged to my tree before you
> commented on it and then I missed the related follow-up discussion.
>
> Looking at the patch itself, it seems fine as such but there's a problem
> with the driver to begin with: the device isn't powered on in probe on DT
> systems but still its runtime suspend callback is called through
> pm_runtime_idle().
>
> Normally calling the RT suspend callback is what we want, but in this case
> disabling a regulator that wasn't enabled is a problem.
>
> There also seems to be a problem in error handling... and the driver does
> not support probing while powered off on ACPI. Oh well.
>
> Let's revert the patch now but it seems there's something to fix
> afterwards.

Thanks Sakari.

One of possible ways to fix this would be to always turn on the
regulators in the probe, although it would result in the privacy LED
blinking issue on our ARM systems.

I wonder if we're missing something in how the ACPI runtime PM works
on Linux. It sounds strange to me that the driver needs to be aware of
the ACPI internals and know that the default boot-up state is powered
on. Maybe there is another function that we could call instead of
pm_runtime_set_active()+pm_runtime_idle() that would only shut down
the PM domain, while leaving the device itself alone?

Best regards,
Tomasz

>
> --
> Regards,
>
> Sakari Ailus

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

[sakari.ailus]

Hi Tomasz,

On Wed, Apr 13, 2022 at 09:17:47PM +0900, Tomasz Figa wrote:
> On Wed, Apr 13, 2022 at 8:40 PM sakari.ailus@linux.intel.com
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Tomasz, Bingbu,
> >
> > On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
> > > On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > >
> > > >
> > > >
> > > > 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.
> > > >
> > >
> > > I believe it worked on ACPI systems, just runtime PM wasn't suspending
> > > the device.
> > >
> > > That said, if my comment above was addressed instead of being ignored,
> > > this regression wouldn't have happened. The problem is described in my
> > > previous messages, please get back to them and address the issue I
> > > pointed out.
> >
> > First of all, thanks for catching this.
> >
> > What I believe happened was that the patch was merged to my tree before you
> > commented on it and then I missed the related follow-up discussion.
> >
> > Looking at the patch itself, it seems fine as such but there's a problem
> > with the driver to begin with: the device isn't powered on in probe on DT
> > systems but still its runtime suspend callback is called through
> > pm_runtime_idle().
> >
> > Normally calling the RT suspend callback is what we want, but in this case
> > disabling a regulator that wasn't enabled is a problem.
> >
> > There also seems to be a problem in error handling... and the driver does
> > not support probing while powered off on ACPI. Oh well.
> >
> > Let's revert the patch now but it seems there's something to fix
> > afterwards.
> 
> Thanks Sakari.
> 
> One of possible ways to fix this would be to always turn on the
> regulators in the probe, although it would result in the privacy LED
> blinking issue on our ARM systems.
> 
> I wonder if we're missing something in how the ACPI runtime PM works
> on Linux. It sounds strange to me that the driver needs to be aware of
> the ACPI internals and know that the default boot-up state is powered

Not really ACPI internals, just that the I²C devices are powered on for
probe.

> on. Maybe there is another function that we could call instead of
> pm_runtime_set_active()+pm_runtime_idle() that would only shut down
> the PM domain, while leaving the device itself alone?

Laurent recently complained about the complexities of supporting runtime PM
on drivers with both OF and ACPI support. I was planning to reply, will
include you.

-- 
Regards,

Sakari Ailus

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

[Tomasz Figa]

On Wed, Apr 13, 2022 at 10:36 PM sakari.ailus@linux.intel.com
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz,
>
> On Wed, Apr 13, 2022 at 09:17:47PM +0900, Tomasz Figa wrote:
> > On Wed, Apr 13, 2022 at 8:40 PM sakari.ailus@linux.intel.com
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Tomasz, Bingbu,
> > >
> > > On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
> > > > On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > I believe it worked on ACPI systems, just runtime PM wasn't suspending
> > > > the device.
> > > >
> > > > That said, if my comment above was addressed instead of being ignored,
> > > > this regression wouldn't have happened. The problem is described in my
> > > > previous messages, please get back to them and address the issue I
> > > > pointed out.
> > >
> > > First of all, thanks for catching this.
> > >
> > > What I believe happened was that the patch was merged to my tree before you
> > > commented on it and then I missed the related follow-up discussion.
> > >
> > > Looking at the patch itself, it seems fine as such but there's a problem
> > > with the driver to begin with: the device isn't powered on in probe on DT
> > > systems but still its runtime suspend callback is called through
> > > pm_runtime_idle().
> > >
> > > Normally calling the RT suspend callback is what we want, but in this case
> > > disabling a regulator that wasn't enabled is a problem.
> > >
> > > There also seems to be a problem in error handling... and the driver does
> > > not support probing while powered off on ACPI. Oh well.
> > >
> > > Let's revert the patch now but it seems there's something to fix
> > > afterwards.
> >
> > Thanks Sakari.
> >
> > One of possible ways to fix this would be to always turn on the
> > regulators in the probe, although it would result in the privacy LED
> > blinking issue on our ARM systems.
> >
> > I wonder if we're missing something in how the ACPI runtime PM works
> > on Linux. It sounds strange to me that the driver needs to be aware of
> > the ACPI internals and know that the default boot-up state is powered
>
> Not really ACPI internals, just that the I涎 devices are powered on for
> probe.

Do you mean the dev_pm_domain_attach() call at [1]?
I suspect that it wouldn't have any effect on anything other than ACPI.
That said, I guess it's indeed a design decision in the I2C subsystem...

One thing that could help here would be adding a .sync callback to
acpi_general_pm_domain, which would turn off the ACPI companion device
if dev is suspended.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L544

>
> > on. Maybe there is another function that we could call instead of
> > pm_runtime_set_active()+pm_runtime_idle() that would only shut down
> > the PM domain, while leaving the device itself alone?
>
> Laurent recently complained about the complexities of supporting runtime PM
> on drivers with both OF and ACPI support. I was planning to reply, will
> include you.

That would be great, thanks!

Best regards,
Tomasz

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

[Bingbu Cao]

Tomasz and Sakari,

Is there any conclusion of this support for both OF and
ACPI platform? 


On 4/13/22 9:44 PM, Tomasz Figa wrote:
> On Wed, Apr 13, 2022 at 10:36 PM sakari.ailus@linux.intel.com
> <sakari.ailus@linux.intel.com> wrote:
>>
>> Hi Tomasz,
>>
>> On Wed, Apr 13, 2022 at 09:17:47PM +0900, Tomasz Figa wrote:
>>> On Wed, Apr 13, 2022 at 8:40 PM sakari.ailus@linux.intel.com
>>> <sakari.ailus@linux.intel.com> wrote:
>>>>
>>>> Hi Tomasz, Bingbu,
>>>>
>>>> On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
>>>>> On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>
>>>>> I believe it worked on ACPI systems, just runtime PM wasn't suspending
>>>>> the device.
>>>>>
>>>>> That said, if my comment above was addressed instead of being ignored,
>>>>> this regression wouldn't have happened. The problem is described in my
>>>>> previous messages, please get back to them and address the issue I
>>>>> pointed out.
>>>>
>>>> First of all, thanks for catching this.
>>>>
>>>> What I believe happened was that the patch was merged to my tree before you
>>>> commented on it and then I missed the related follow-up discussion.
>>>>
>>>> Looking at the patch itself, it seems fine as such but there's a problem
>>>> with the driver to begin with: the device isn't powered on in probe on DT
>>>> systems but still its runtime suspend callback is called through
>>>> pm_runtime_idle().
>>>>
>>>> Normally calling the RT suspend callback is what we want, but in this case
>>>> disabling a regulator that wasn't enabled is a problem.
>>>>
>>>> There also seems to be a problem in error handling... and the driver does
>>>> not support probing while powered off on ACPI. Oh well.
>>>>
>>>> Let's revert the patch now but it seems there's something to fix
>>>> afterwards.
>>>
>>> Thanks Sakari.
>>>
>>> One of possible ways to fix this would be to always turn on the
>>> regulators in the probe, although it would result in the privacy LED
>>> blinking issue on our ARM systems.
>>>
>>> I wonder if we're missing something in how the ACPI runtime PM works
>>> on Linux. It sounds strange to me that the driver needs to be aware of
>>> the ACPI internals and know that the default boot-up state is powered
>>
>> Not really ACPI internals, just that the I涎 devices are powered on for
>> probe.
> 
> Do you mean the dev_pm_domain_attach() call at [1]?
> I suspect that it wouldn't have any effect on anything other than ACPI.
> That said, I guess it's indeed a design decision in the I2C subsystem...
> 
> One thing that could help here would be adding a .sync callback to
> acpi_general_pm_domain, which would turn off the ACPI companion device
> if dev is suspended.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L544
> 
>>
>>> on. Maybe there is another function that we could call instead of
>>> pm_runtime_set_active()+pm_runtime_idle() that would only shut down
>>> the PM domain, while leaving the device itself alone?
>>
>> Laurent recently complained about the complexities of supporting runtime PM
>> on drivers with both OF and ACPI support. I was planning to reply, will
>> include you.
> 
> That would be great, thanks!
> 
> Best regards,
> Tomasz
> 

-- 
Best regards,
Bingbu Cao

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

[Tomasz Figa]

Hi Bingbu,

On Thu, Jul 14, 2022 at 6:10 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
> Tomasz and Sakari,
>
> Is there any conclusion of this support for both OF and
> ACPI platform?
>

Sakari did indeed reply and add me to the thread, but I somehow missed
it, sorry.  I just took a look at it and need some time to think about
how we could solve this in a general way.

For the time being, I'd just make this driver behave as other drivers
- power the regulators on in probe and let the runtime PM suspend
callback power them down. WDYT?

Best regards,
Tomasz

>
> On 4/13/22 9:44 PM, Tomasz Figa wrote:
> > On Wed, Apr 13, 2022 at 10:36 PM sakari.ailus@linux.intel.com
> > <sakari.ailus@linux.intel.com> wrote:
> >>
> >> Hi Tomasz,
> >>
> >> On Wed, Apr 13, 2022 at 09:17:47PM +0900, Tomasz Figa wrote:
> >>> On Wed, Apr 13, 2022 at 8:40 PM sakari.ailus@linux.intel.com
> >>> <sakari.ailus@linux.intel.com> wrote:
> >>>>
> >>>> Hi Tomasz, Bingbu,
> >>>>
> >>>> On Tue, Apr 12, 2022 at 08:15:08PM +0900, Tomasz Figa wrote:
> >>>>> On Tue, Apr 12, 2022 at 8:05 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> 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.
> >>>>>>
> >>>>>
> >>>>> I believe it worked on ACPI systems, just runtime PM wasn't suspending
> >>>>> the device.
> >>>>>
> >>>>> That said, if my comment above was addressed instead of being ignored,
> >>>>> this regression wouldn't have happened. The problem is described in my
> >>>>> previous messages, please get back to them and address the issue I
> >>>>> pointed out.
> >>>>
> >>>> First of all, thanks for catching this.
> >>>>
> >>>> What I believe happened was that the patch was merged to my tree before you
> >>>> commented on it and then I missed the related follow-up discussion.
> >>>>
> >>>> Looking at the patch itself, it seems fine as such but there's a problem
> >>>> with the driver to begin with: the device isn't powered on in probe on DT
> >>>> systems but still its runtime suspend callback is called through
> >>>> pm_runtime_idle().
> >>>>
> >>>> Normally calling the RT suspend callback is what we want, but in this case
> >>>> disabling a regulator that wasn't enabled is a problem.
> >>>>
> >>>> There also seems to be a problem in error handling... and the driver does
> >>>> not support probing while powered off on ACPI. Oh well.
> >>>>
> >>>> Let's revert the patch now but it seems there's something to fix
> >>>> afterwards.
> >>>
> >>> Thanks Sakari.
> >>>
> >>> One of possible ways to fix this would be to always turn on the
> >>> regulators in the probe, although it would result in the privacy LED
> >>> blinking issue on our ARM systems.
> >>>
> >>> I wonder if we're missing something in how the ACPI runtime PM works
> >>> on Linux. It sounds strange to me that the driver needs to be aware of
> >>> the ACPI internals and know that the default boot-up state is powered
> >>
> >> Not really ACPI internals, just that the I涎 devices are powered on for
> >> probe.
> >
> > Do you mean the dev_pm_domain_attach() call at [1]?
> > I suspect that it wouldn't have any effect on anything other than ACPI.
> > That said, I guess it's indeed a design decision in the I2C subsystem...
> >
> > One thing that could help here would be adding a .sync callback to
> > acpi_general_pm_domain, which would turn off the ACPI companion device
> > if dev is suspended.
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L544
> >
> >>
> >>> on. Maybe there is another function that we could call instead of
> >>> pm_runtime_set_active()+pm_runtime_idle() that would only shut down
> >>> the PM domain, while leaving the device itself alone?
> >>
> >> Laurent recently complained about the complexities of supporting runtime PM
> >> on drivers with both OF and ACPI support. I was planning to reply, will
> >> include you.
> >
> > That would be great, thanks!
> >
> > Best regards,
> > Tomasz
> >
>
> --
> Best regards,
> Bingbu Cao

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

[Bingbu Cao]

On 11/5/21 2:54 PM, Tomasz Figa wrote:
> 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.

Tomasz,

Why the runtime_set_active() and runtime_idle() break the runtime
PM on non-ACPI systems? Did it cause the PM runtime enable failure or
incorrect PM usage count?

> 
> 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?
> 
> 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

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

[Tomasz Figa]

On Wed, Apr 13, 2022 at 8:38 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
>
> On 11/5/21 2:54 PM, Tomasz Figa wrote:
> > 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.
>
> Tomasz,
>
> Why the runtime_set_active() and runtime_idle() break the runtime
> PM on non-ACPI systems? Did it cause the PM runtime enable failure or
> incorrect PM usage count?

Neither. It tells the runtime PM subsystem that the device was
manually brought into the active state, but the driver doesn't power
on the voltage regulators. Then there are 2 paths to failure:

1) pm_runtime_idle() triggers a runtime PM suspend and driver callback
tries to power off the already powered off regulators, leading to a
negative regulator usage count.

OR

2) userspace opens the device, runtime PM resume doesn't happen
(because the device is still active) and then a communication failure
would happen because the chip is not powered on.

Best regards,
Tomasz

>
> >
> > 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?
> >
> > 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