Hi Niklas, On Sun, Apr 15, 2018 at 10:47:37PM +0200, Niklas Söderlund wrote: > Hi Sakari, > > Thanks for your feedback. > > On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote: > > [snip] > > > > > + pm_runtime_enable(&pdev->dev); > > > > > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > > > device uninitialised at probe, and pm_runtime_get_sync will not > > > initialise it either if CONFIG_PM is not enabled. I guess you could > > > call your runtime_resume function unconditionally, and mark the device > > > as active in runtime_pm using pm_runtime_set_active. > > > > There doesn't seem to be any runtime_resume function. Was there supposed > > to be one? > > No there is not suppose to be one. Ok. > > > > > Assuming runtime PM would actually do something here, you might add > > pm_runtime_idle() to power the device off after probing. > > > > I guess pm_runtime_set_active() should precede pm_runtime_enable(). > > The CSI-2 is in the always on power domain so the calls to > pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() > callback to enable and disable the module clock. I'm no expert on PM but > in my testing the pm_ calls in this driver seems to be correct. > > 1. In probe I call pm_runtime_enable(). And rudimentary tests shows the > clock is off (but I might miss something) as I wish it to be until > stream on time. > 2. In s_stream() I call pm_runtime_get_sync() before writing any > register when starting a stream. And likewise I call pm_runtime_put() > when stopping and I no longer need to write to a register. > 3. In remove() I call pm_runtime_disable(). > > Am I missing something obvious here? Looking at the code again, it seems fine in this respect. -- Sakari Ailus e-mail: sakari.ailus@xxxxxx
