| Message ID | 1634278119-32158-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: dw9768: activate runtime PM and turn off device | expand |
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 >
> -----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 > >
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 > > >
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 > > > >
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 >>>>>
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
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 >>
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.
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
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
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.
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
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 >
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
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;
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(+)