Re: [PATCH v2 3/5] spi: sunxi: Add Allwinner A31 SPI controller driver

[Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes:

> Hi Kevin,
>
> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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.
>
> Actually, pm_runtime_get_sync is called by the SPI framework whenever
> the device needs to be used. And pm_runtime_put whenever it's not used
> anymore, so the callbacks are actually called.

Ah, right.  I forgot that the SPI framework is using runtime PM now.

>> 
>> > 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.
>
> In the case where pm_runtime is disabled, pm_runtime_enabled will only
> return false, and hence the resume callback will be called. get_sync
> will fail too when the framework will call it, but since the device is
> already initialized, it's fine I guess.
>
>> 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.)
>
> Yes, but that also mean that the device is actually on after the
> probe, even if it's never going to be used. From what I got reading
> the pm_runtime code, the suspend callback is called only whenever you
> call _put, so the device will be suspended only after it's been used
> the first time, right?
>
> Wouldn't it be better if it was suspended by default, and just waken
> up whenever the framework needs it?

Yes, but that's the job of runtime PM.  Without runtime PM, you have to
live with leaving the device powered up all the time.

Kevin