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

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
<sakari.ailus@linux.intel.com> 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@linux.intel.com/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