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@xxxxxxxxxxxxxxx> wrote: > > > > > > > > On 4/12/22 5:39 PM, Tomasz Figa wrote: > > > 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? > > > > 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
