[alsa-devel] Question about pinctrl_pm_select_xxx()
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
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 | |
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
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
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
participants (2)
-
Kuninori Morimoto -
Mark Brown