On Tue, Nov 16, 2021 at 1:57 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
>
> On Fri, Nov 5, 2021 at 9:52 PM Cao, Bingbu <bingbu.cao@xxxxxxxxx> wrote:
> >
> > > -----Original Message-----
> > > From: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > > Sent: Friday, November 5, 2021 2:55 PM
> > > To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
> > > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
> > > dongchun.zhu@xxxxxxxxxxxx; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>;
> > > bingbu.cao@xxxxxxxxxxxxxxx
> > > 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@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.
> > >
> >
> > > 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
> > > >
