Question about pinctrl_pm_select_xxx()

Question about pinctrl_pm_select_xxx()

[Kuninori Morimoto]

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

Re: Question about pinctrl_pm_select_xxx()

[Mark Brown]

[-- 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 --]

Re: Question about pinctrl_pm_select_xxx()

[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	|		|

Re: Question about pinctrl_pm_select_xxx()

[Mark Brown]

[-- 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 --]

Re: Question about pinctrl_pm_select_xxx()

[Kuninori Morimoto]

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

Re: Question about pinctrl_pm_select_xxx()

[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

Re: Question about pinctrl_pm_select_xxx()

[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