Re: [PATCH v2] media: ov5640: Use runtime PM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Fixed Jacopo's email address.]

On Mon, Aug 1, 2022 at 4:17 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
>
> On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi Laurent,
> >
> > On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > > ...
> > > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > > access). Actually the full code would be
> > > > > > >
> > > > > > >
> > > > > > >       pm_runtime_enable(dev);
> > > > > > >       pm_runtime_resume_and_get(dev);
> > > > > > >
> > > > > > >       /* Hardware access */
> > > > > > >
> > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > > >
> > > > > > > (plus error handling).
> > > > > > >
> > > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > > the above becomes
> > > > > > >
> > > > > > >       pm_runtime_enable(dev);
> > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > >
> > > > > > > instead of having to power up the device just in case !PM.
> > > > > > >
> > > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > > most of the drivers.
> > >
> > > Does the former work on ACPI systems ?
> >
> > Yes (i.e. the one that was above the quoted text).
> >
> > >
> > > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > > >
> > > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > > >
> > > > > Not through RPM I suppose ?
> > > >
> > > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > > dev_pm_domain_attach() called in i2c_device_probe()).
> > >
> > > How can we fix this ? It may have made sense a long time ago, but it's
> > > making RPM handling way too difficult in I2C drivers now. We need
> > > something better instead of continuing to rely on cargo-cult for probe
> > > functions. Most drivers are broken.
> >
> > Some could be broken, there's no question of that. A lot of drivers support
> > either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> > that number is probably increasing constantly for the same devices are used
> > on both.
> >
> > Then there are drivers that prefer not powering on the device in probe (see
> > <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@xxxxxxxxxxxxxxx/T/>),
> > it gets complicated to support all the combinatios of DT/ACPI (with or
> > without the flag / property for waiving powering device on for probe) and
> > CONFIG_PM enabled/disabled.
> >
> > What I think could be done to add a flag for drivers that handle power on
> > their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> > works. Right now it expects a property on the device but that check could
> > be moved to existing drivers using the flag. Not many drivers are currently
> > using the flag. I think this would simplify driver implementation as both
> > firmware interfaces would work the same way in this respect.
> >
> > You'd have to change one driver at a time, and people should be encouraged
> > to write new drivers with that flag. Or add the flag to all existing
> > drivers and not accept new ones with it.
> >
> > These devices I think are all I涎 but my understanding is that such
> > differences exist elsewhere in the kernel, too. If they are to be
> > addressed, it would probably be best to have a unified approach towards it.
> >
> > Added a few more people and lists to cc.
>
> + Hidenori from my team for visibility.
>
> I think we may want to take a step back and first define the problem
> itself. To do that, let's take a look separately at DT and ACPI cases
> (is platform data still relevant? are there any other firmware
> interfaces that deal with I2C devices?).
> For simplicity, let's forget about the ACPI waived power on in probe.
>
> DT:
>  1) hardware state unknown when probe is called
>  2) claim any independently managed resources (e.g. GPIOs)
>  3) enable runtime PM
>  4) if driver wants to access the hardware:
>     a) runtime PM get
>     b) enable any independently controlled resources (e.g. reset GPIO)
>     c) [do access]
>     d) disable any independently controlled resources
>     e) runtime PM put
>  5) after probe returns, regulators, clocks (and other similarly
> managed resources) would be force disabled if their enable count is 0
>  6) hardware state is off (after the runtime PM state settles)
>
> ACPI:
>  1) hardware state is active when probe is called
>  2) [n/a]
>  3) tell runtime PM framework that the state is active and then enable
> runtime PM
>  4) if driver wants to access the hardware:
>     a) runtime PM get
>     b) [n/a]
>     c) [do access]
>     d) [n/a]
>     e) runtime PM put
>  5) [n/a]
>  6) hardware state is off (after the runtime PM state settles)
>
> It seems like the relevant difference here is that for ACPI, the
> driver needs to know that the initial state is active and also relay
> this knowledge to the runtime PM subsystem. If we could make the ACPI
> PM domain work the same way as regulators and clocks and eventually
> power off some time later when the enable count is 0, then perhaps we
> could avoid the problem in the first place?
>
> Best regards,
> Tomasz




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux