[alsa-devel] Is that reasonable to support pinctrl PM in ASoC core?
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
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?
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
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?
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
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);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
participants (3)
-
Linus Walleij -
Mark Brown -
Nicolin Chen