Is that reasonable to support pinctrl PM in ASoC core?

Is that reasonable to support pinctrl PM in ASoC core?

[Nicolin Chen]

Hi all,

It's quite popular that more drivers are using pinctrl PM, for example:
(Documentation/devicetree/bindings/arm/primecell.txt). Just like what
runtime PM does, it would de-active and en-active pin group depending
on whether it's being used or not.

And I think this pinctrl PM might be also beneficial to cpu dai drivers
since they might have actual pins, and they can hypnotize/wake them up
along with runtime PM:

@@ -183,6 +183,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
	struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
	int ret = 0;

+       pinctrl_pm_select_default_state(cpu_dai->dev);
	pm_runtime_get_sync(cpu_dai->dev);
	pm_runtime_get_sync(codec_dai->dev);
	pm_runtime_get_sync(platform->dev);

		       
As pinctrl PM is also reference counted and would return 0 if there
is no pinctrl settings, this would not break current ASoC subsystem
and any cpu dai driver.

So I just want to ask if it's reasonable to add it. Or another one:
is there anything similar to sleep pins? Although I've searched the
whole ./sound directory, it seems no drivers under it is using this
pinctrl PM.

Best regards,
Nicolin Chen

Re: Is that reasonable to support pinctrl PM in ASoC core?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 774 bytes --]

On Thu, Oct 24, 2013 at 07:07:43PM +0800, Nicolin Chen wrote:

> And I think this pinctrl PM might be also beneficial to cpu dai drivers
> since they might have actual pins, and they can hypnotize/wake them up
> along with runtime PM:

I think you're right that this is a useful and obvious feature but the
question I have is that since the standard implementation is going to be
to do the pinctrl calls in lockstep with the runtime PM handling and
since this is likely to be the same for pretty much any subsystem might
it be better to add the hooks at the runtime PM level rather than in the
subsystem?  Or perhaps we should just add them in subsystems and then
once some reasonable number of subsystems have done this then pull it
into the PM core?

Linus, any thoughts?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Is that reasonable to support pinctrl PM in ASoC core?

[Linus Walleij]

On Thu, Oct 24, 2013 at 1:52 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Oct 24, 2013 at 07:07:43PM +0800, Nicolin Chen wrote:
>
>> And I think this pinctrl PM might be also beneficial to cpu dai drivers
>> since they might have actual pins, and they can hypnotize/wake them up
>> along with runtime PM:
>
> I think you're right that this is a useful and obvious feature but the
> question I have is that since the standard implementation is going to be
> to do the pinctrl calls in lockstep with the runtime PM handling and
> since this is likely to be the same for pretty much any subsystem might
> it be better to add the hooks at the runtime PM level rather than in the
> subsystem?  Or perhaps we should just add them in subsystems and then
> once some reasonable number of subsystems have done this then pull it
> into the PM core?

This makes a lot of sense, the only problem I have traditionally had
with this is that there exist cases where you want to put pinctrl
handles to sleep or idle aside from runtime PM.

For example:

- UARTs disabled from userspace shall have their pins put to
  sleep (see drivers/tty/serial/uart-pl011.c)

- I2C or SPI hosts with no devices connected to them should be
  nailed to sleep. (It's not possible to detect an unused I2C or SPI
  host today, but a feature we should add.)

Maybe the right way to do these is also to use runtime_pm*,
but the thing is that today these things are done also when
CONFIG_PM_RUNTIME is not set. Which is something you
might sometimes want.

Or not? Should we say that unless you have
CONFIG_PM_RUNTIME you don't deserve saving these
microamps anyway?

Yours,
Linus Walleij

Re: Is that reasonable to support pinctrl PM in ASoC core?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1453 bytes --]

On Thu, Oct 24, 2013 at 03:19:03PM +0200, Linus Walleij wrote:

> This makes a lot of sense, the only problem I have traditionally had
> with this is that there exist cases where you want to put pinctrl
> handles to sleep or idle aside from runtime PM.

Yeah, that was my concern.

> - I2C or SPI hosts with no devices connected to them should be
>   nailed to sleep. (It's not possible to detect an unused I2C or SPI
>   host today, but a feature we should add.)

The SPI case should be mostly handled already if this is integrated via
runtime PM - we have core support for idling controllers whenever there
is no transfer active.  That should go off for devices that aren't
connected to anything.

I should probably go and implement an I2C version of that.

> Maybe the right way to do these is also to use runtime_pm*,
> but the thing is that today these things are done also when
> CONFIG_PM_RUNTIME is not set. Which is something you
> might sometimes want.

> Or not? Should we say that unless you have
> CONFIG_PM_RUNTIME you don't deserve saving these
> microamps anyway?

Personally I tend to think that people who care probably ought to be
using runtime PM but that's not a sufficiently general view to insist
on.

As we discussed in person earlier how about we merge this on a subsystem
basis and then once we've established what the pattern really is we
think about putting it into the core.  Nicolin, do you want to write a
patch for this?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Is that reasonable to support pinctrl PM in ASoC core?

[Nicolin Chen]

On Thu, Oct 24, 2013 at 03:15:24PM +0100, Mark Brown wrote:
> On Thu, Oct 24, 2013 at 03:19:03PM +0200, Linus Walleij wrote:
> As we discussed in person earlier how about we merge this on a subsystem
> basis and then once we've established what the pattern really is we
> think about putting it into the core.  Nicolin, do you want to write a
> patch for this?

I'd like to, sir.

Thank you,
Nicolin Chen

Re: Is that reasonable to support pinctrl PM in ASoC core?

[Nicolin Chen]

On Thu, Oct 24, 2013 at 07:07:43PM +0800, Nicolin Chen wrote:
> Hi all,
> 
> It's quite popular that more drivers are using pinctrl PM, for example:
> (Documentation/devicetree/bindings/arm/primecell.txt). Just like what
> runtime PM does, it would de-active and en-active pin group depending
> on whether it's being used or not.
> 
> And I think this pinctrl PM might be also beneficial to cpu dai drivers
> since they might have actual pins, and they can hypnotize/wake them up
> along with runtime PM:
> 
> @@ -183,6 +183,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> 	struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
> 	int ret = 0;
> 
> +       pinctrl_pm_select_default_state(cpu_dai->dev);
> 	pm_runtime_get_sync(cpu_dai->dev);
> 	pm_runtime_get_sync(codec_dai->dev);
> 	pm_runtime_get_sync(platform->dev);
> 
> 		       
> As pinctrl PM is also reference counted and would return 0 if there
> is no pinctrl settings, this would not break current ASoC subsystem
> and any cpu dai driver.

This was a mistake that pinctrl PM seems to be not reference counting.
So I have to correct it here.

And I'm sorry if I misguided anyone.

Nicolin Chen


> 
> So I just want to ask if it's reasonable to add it. Or another one:
> is there anything similar to sleep pins? Although I've searched the
> whole ./sound directory, it seems no drivers under it is using this
> pinctrl PM.
> 
> Best regards,
> Nicolin Chen
>