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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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²C 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²C 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.

-- 
Kind regards,

Sakari Ailus