Hi Bingbu, On Thu, Jul 14, 2022 at 6:10 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote: > > Tomasz and Sakari, > > Is there any conclusion of this support for both OF and > ACPI platform? > Sakari did indeed reply and add me to the thread, but I somehow missed it, sorry. I just took a look at it and need some time to think about how we could solve this in a general way. For the time being, I'd just make this driver behave as other drivers - power the regulators on in probe and let the runtime PM suspend callback power them down. WDYT? Best regards, Tomasz > > 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
