On Thu, Jan 30, 2014 at 6:29 PM, Felipe Balbi <balbi@xxxxxx> wrote: > Hi, > > On Thu, Jan 30, 2014 at 03:52:16PM -0800, Kevin Hilman wrote: >> 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 */ > > shouldn't this be done before pm_runtime_enable() ? hmm, possibly yes. I was doing this from the top of my head without looking to closely at the code. >> 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(); > > in ->remove() you actually want a put_sync() right ? You don't want to > schedule anything since you're just about to disable pm_runtime. Yes, you're correct. Thanks for the corrections. 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
