On Wed, Apr 13, 2022 at 8:38 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote:
>
>
> On 11/5/21 2:54 PM, Tomasz Figa wrote:
> > On Fri, Oct 15, 2021 at 3:12 PM Bingbu Cao <bingbu.cao@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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
