On Wed, Jan 29, 2014 at 5:32 AM, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote: > On Wed, Jan 29, 2014 at 12:25:20PM +0000, Mark Brown wrote: >> On Wed, Jan 29, 2014 at 12:10:48PM +0100, Maxime Ripard wrote: >> >> > +config SPI_SUN6I >> > + tristate "Allwinner A31 SPI controller" >> > + depends on ARCH_SUNXI || COMPILE_TEST >> > + select PM_RUNTIME >> > + help >> > + This enables using the SPI controller on the Allwinner A31 SoCs. >> > + >> >> A select of PM_RUNTIME is both surprising and odd - why is that there? >> The usual idiom is that the device starts out powered up (flagged using >> pm_runtime_set_active()) and then runtime PM then suspends it when it's >> compiled in. That way if for some reason people want to avoid runtime >> PM they can still use the device. > > Since pm_runtime_set_active and all the pm_runtime* callbacks in > general are defined to pretty much empty functions, how the > suspend/resume callbacks are called then? Obviously, we need them to > be run, hence why I added the select here, but now I'm seeing a > construct like what's following acceptable then? Even with your 'select', The runtime PM callbacks will never be called in the current driver. pm_runtime_enable() doesn't do any runtime PM transitions. It just allows transitions to happen when they're triggered by _get()/_put()/etc. > pm_runtime_enable(&pdev->dev); > if (!pm_runtime_enabled(&pdev->dev)) > sun6i_spi_runtime_resume(&pdev->dev); Similarily here, it's not the pm_runtime_enable that will fail when runtime PM is disabled (or not built-in), it's a pm_runtime_get_sync() that will fail. What you want is something like this in ->probe() sun6i_spi_runtime_resume(); /* now, device is always activated whether or not runtime PM is enabled */ pm_runtime_enable(); pm_runtime_set_active(); /* tells runtime PM core device is already active */ pm_runtime_get_sync(); This 'get' will increase the usecount, but not actually call the callbacks because we told the RPM core that the device was already activated with _set_active(). And then, in ->remove(), you'll want pm_runtime_put(); pm_runtime_disable(); And if runtime PM is not enabled in the kernel, then the device will be left on (which is kinda what you want if you didn't build runtime PM into the kernel.) Kevin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
