* Question about pinctrl_pm_select_xxx()
@ 2019-06-19 7:22 Kuninori Morimoto
2019-06-19 11:43 ` Mark Brown
0 siblings, 1 reply; 7+ messages in thread
From: Kuninori Morimoto @ 2019-06-19 7:22 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
Hi ALSA ML
I noticed pinctrl_pm_select_xxx() unbalance.
I think these are paired function
pinctrl_pm_select_default_state()
pinctrl_pm_select_sleep_state()
I can find pinctrl_pm_select_sleep_state() for component at
snd_soc_suspend().
int snd_soc_suspend(struct device *dev)
{
...
if (!snd_soc_component_is_suspended(component)) {
switch (snd_soc_dapm_get_bias_level(dapm)) {
...
case SND_SOC_BIAS_OFF:
...
/* deactivate pins to sleep state */
=> pinctrl_pm_select_sleep_state(component->dev);
break;
}
...
}
...
}
But, I can't find its paired pinctrl_pm_select_default_state().
It looks strange for me. Is this really needed ??
And about pinctrl_pm_select_xxx() for CPU/Codec DAI,
Many places are calling pinctrl_pm_select_xxx() for both CPU/Codec.
snd_soc_suspend() cares only CPU only, but snd_soc_resume() cares both.
Is this bug ??
int snd_soc_suspend(struct device *dev)
{
...
for_each_card_rtds(card, rtd) {
/* deactivate pins to sleep state */
=> pinctrl_pm_select_sleep_state(cpu_dai->dev);
}
...
}
int snd_soc_resume(struct device *dev)
{
...
for_each_card_rtds(card, rtd) {
...
if (cpu_dai->active)
=> pinctrl_pm_select_default_state(cpu_dai->dev);
for_each_rtd_codec_dai(rtd, i, codec_dai) {
if (codec_dai->active)
=> pinctrl_pm_select_default_state(codec_dai->dev);
}
}
...
}
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-19 7:22 Question about pinctrl_pm_select_xxx() Kuninori Morimoto
@ 2019-06-19 11:43 ` Mark Brown
2019-06-20 0:35 ` Kuninori Morimoto
0 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2019-06-19 11:43 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Linux-ALSA
[-- Attachment #1.1: Type: text/plain, Size: 264 bytes --]
On Wed, Jun 19, 2019 at 04:22:11PM +0900, Kuninori Morimoto wrote:
> But, I can't find its paired pinctrl_pm_select_default_state().
> It looks strange for me. Is this really needed ??
It's in snd_soc_resume() for active DAIs, or otherwise when we open the
PCM.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-19 11:43 ` Mark Brown
@ 2019-06-20 0:35 ` Kuninori Morimoto
2019-06-20 11:54 ` Mark Brown
0 siblings, 1 reply; 7+ messages in thread
From: Kuninori Morimoto @ 2019-06-20 0:35 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
Hi Mark
Thank you for your feedback
> > But, I can't find its paired pinctrl_pm_select_default_state().
> > It looks strange for me. Is this really needed ??
>
> It's in snd_soc_resume() for active DAIs, or otherwise when we open the
> PCM.
OK, I see.
Now, I double checked about it.
In my understanding, we need these at open/close/suspend/resume.
But for component, it only has for suspend.
If we need these, I think we want to have these at open/close/resume.
Now, it uses pinctrl_pm_select_xxx() with "component->dev".
pinctrl_pm_select_sleep_state(component->dev);
~~~~~~~~~~~~~~
But, this component->dev = dai->dev
static struct snd_soc_dai *soc_add_dai(...)
{
=> struct device *dev = component->dev;
...
=> dai->dev = dev;
...
}
Thus, calling it from DAI only is very enough, I think.
In my check, open/close/suspend/resume all has DAI pinctrl_pm_select_xxx()
except suspend:Codec.
I guess, because suspend doesn't have Codec pinctrl_pm_select_xxx() somehow,
thus, someone added Component pinctrl_pm_select_xxx() instead somehow ??
But, what do you think ?
|CPU |Codec |Component |
open |O |O | |
close |O |O | |
suspend |O | |O |
resume |O |O | |
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-20 0:35 ` Kuninori Morimoto
@ 2019-06-20 11:54 ` Mark Brown
2019-06-20 23:55 ` Kuninori Morimoto
0 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2019-06-20 11:54 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Linux-ALSA
[-- Attachment #1.1: Type: text/plain, Size: 257 bytes --]
On Thu, Jun 20, 2019 at 09:35:06AM +0900, Kuninori Morimoto wrote:
> Thus, calling it from DAI only is very enough, I think.
I'd expect that only the DAI should have pins so probably yes, but we
can't guarantee that this won't break anything in practice.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-20 11:54 ` Mark Brown
@ 2019-06-20 23:55 ` Kuninori Morimoto
2019-06-21 1:15 ` Kuninori Morimoto
0 siblings, 1 reply; 7+ messages in thread
From: Kuninori Morimoto @ 2019-06-20 23:55 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
Hi Mark
> > Thus, calling it from DAI only is very enough, I think.
>
> I'd expect that only the DAI should have pins so probably yes, but we
> can't guarantee that this won't break anything in practice.
Yes, indeed.
But we can just revert it if it breaks something.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-20 23:55 ` Kuninori Morimoto
@ 2019-06-21 1:15 ` Kuninori Morimoto
2019-06-21 1:38 ` Kuninori Morimoto
0 siblings, 1 reply; 7+ messages in thread
From: Kuninori Morimoto @ 2019-06-21 1:15 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Linux-ALSA, Mark Brown
Hi Mark, again
> > > Thus, calling it from DAI only is very enough, I think.
> >
> > I'd expect that only the DAI should have pins so probably yes, but we
> > can't guarantee that this won't break anything in practice.
>
> Yes, indeed.
> But we can just revert it if it breaks something.
I could confirm. Original code was for Codec,
and it was converted to Component by this patch.
9178feb4538e055bf22be44c38b90cc31d2baf99
("ASoC: add Component level suspend/resume")
...
- pinctrl_pm_select_sleep_state(codec->dev);
+ pinctrl_pm_select_sleep_state(component->dev);
...
I will add this to git-log message
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Question about pinctrl_pm_select_xxx()
2019-06-21 1:15 ` Kuninori Morimoto
@ 2019-06-21 1:38 ` Kuninori Morimoto
0 siblings, 0 replies; 7+ messages in thread
From: Kuninori Morimoto @ 2019-06-21 1:38 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Linux-ALSA, Mark Brown
Hi Mark, again and again
> > > I'd expect that only the DAI should have pins so probably yes, but we
> > > can't guarantee that this won't break anything in practice.
> >
> > Yes, indeed.
> > But we can just revert it if it breaks something.
>
> I could confirm. Original code was for Codec,
> and it was converted to Component by this patch.
>
> 9178feb4538e055bf22be44c38b90cc31d2baf99
> ("ASoC: add Component level suspend/resume")
But, Hmm..
It seems Codec needs very special suspend sequence,
and current code is its result...
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-21 1:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 7:22 Question about pinctrl_pm_select_xxx() Kuninori Morimoto
2019-06-19 11:43 ` Mark Brown
2019-06-20 0:35 ` Kuninori Morimoto
2019-06-20 11:54 ` Mark Brown
2019-06-20 23:55 ` Kuninori Morimoto
2019-06-21 1:15 ` Kuninori Morimoto
2019-06-21 1:38 ` Kuninori Morimoto
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.