On Wed, Apr 13, 2022 at 10:36 PM sakari.ailus@xxxxxxxxxxxxxxx <sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx > > <sakari.ailus@xxxxxxxxxxxxxxx> 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@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. > > > > 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
