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@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 > -- Best regards, Bingbu Cao
