Hi Tomasz, On Mon, Aug 01, 2022 at 04:23:54PM +0900, Tomasz Figa wrote: > [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 This isn't a property of ACPI as such, but rather what I²C core does to ACPI devices before calling probe (or after remove). > > 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? -- Regards, Sakari Ailus
