On Mon, Apr 03, 2023 at 12:19:20PM +0200, Ulf Hansson wrote: > On Sat, 1 Apr 2023 at 15:16, Uwe Kleine-König > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > Hello, > > > > I looked at the sdhci_am654's probe function and concerning runtime-pm > > this is at least unconvential: > > > > It does > > > > clk_xin = devm_clk_get(dev, "clk_xin"); > > if (IS_ERR(clk_xin)) > > ... > > > > /* Clocks are enabled using pm_runtime */ > > pm_runtime_enable(dev); > > ret = pm_runtime_resume_and_get(dev); > > ... > > > > I'm not fluent in runtime-pm stuff (so I added Rafael to Cc), but I > > thought it is to be used the other way around, i.e. put the device in > > operational state and then runtime-pm cares to suspend the device under > > some conditions (e.g. CONFIG_PM being enabled). > > > > With CONFIG_PM unset the driver is broken for sure, as then > > pm_runtime_enable() and pm_runtime_resume_and_get() have no effect. So > > the clk stays off. > > In principle, you are correct. I wouldn't recommend the above pattern > in general, but it doesn't mean that it can't work. > > Some platforms are selecting "PM" from some of their toplevel Kconfig, > as they simply can't work without it. That means that the code you > refer to above, doesn't have to be broken. If I understand correctly the driver under discussion is supposed to be used on SoCs in the CONFIG_ARCH_K3 family. This isn't one of those that enforce PM. But yes, adding a depends on PM would be an action that improves the situation. Having said that even on such platforms you can disable runtime PM, so even a select on PM isn't a complete fix. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
