Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 03/03/2015 05:07 PM, Mark Brown wrote:
> On Tue, Mar 03, 2015 at 04:26:32PM +0200, Peter Ujfalusi wrote:
> 
>> @@ -553,6 +554,7 @@ static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
>>  		return -EINVAL;
>>  	}
>>  
>> +	pm_runtime_put_sync(mcasp->dev);
> 
> Why the _put_sync()s?  I know it's more symmetrical but I don't see a
> need to wait and it means we may be able to avoid some unneeded power
> cycles.

True, the _sync can be dropped. I guess I used it just for symmetry ;)

> 
>> @@ -1078,6 +1082,9 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>>  	u32 reg;
>>  	int i;
>>  
>> +	if (!dai->active)
>> +		pm_runtime_get_sync(mcasp->dev);
>> +
>>  	for (i = 0; i < ARRAY_SIZE(context_regs); i++)
>>  		context->config_regs[i] = mcasp_get_reg(mcasp, context_regs[i]);
>>  
>> @@ -1094,6 +1101,8 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>>  		context->xrsr_regs[i] = mcasp_get_reg(mcasp,
>>  						DAVINCI_MCASP_XRSRCTL_REG(i));
>>  
>> +	pm_runtime_put_sync(mcasp->dev);
>> +
> 
> On first glance this looks unbalanced without the active check in the
> put case.  I'd also need to check if runtime resume works OK in suspend
> context, I guess it must but there were issues in the past (the more
> common idiom is to not suspend if we're already runtime powered down but
> obviously here we're actually doing extra work).

McASP is only going to loose it's context when the whole device goes to
suspend. In this case I need to save the registers which I need to restore in
resume time.

> I see there's actually a balanced put in the resume path...  I see
> what's going on here but I'm thinking that perhaps something more
> explicit that calls the ops directly and checks pm_runtime_is_enabled()
> might be clearer?

The issue with the pm_runtime_is_enabled() is going to be in the resume path,
as I did get_sync() it will return true at the end all the time.

In order to save/restore McASP register the IP need to be enabled.

-If we suspend/resume while no audio was running:
dai->active is false, pm_runtime is disabled when the davinci_mcasp_suspend()
is called. So I need to get_sync(), save context and put_sync() so we are in
balance.
In resume, dai->active is false, pm_runtime is disabled, so I again need to
get_sync(), restore registers and put_sync() so we are in balance again and
mcasp stay disabled.

-If we suspend/resume while audio was running:
dai->active is true, pm_runtime is enabled when the davinci_mcasp_suspend() is
called. I don't need the get_sync(), save context and put_sync() so I release
all the resources needed for McASP. runtime_pm will be disabled.
In resume dai->active is true, pm_runtime is disabled. I need to get_sync(),
restore registers and since dai->active is true I should not put_sync() to
keep McASP enabled and be able to resume the audio.

I think this hassle is needed if I want to achieve what subject describes.

> Unconditionally do the power down/up at the end with
> a direct call outside of runtime PM and then make the checks at the
> top/bottom be "if it's runtime suspended then...".  That's also more
> direct about what's actually being checked and hence less risk that some
> future changes would go wrong.
> 


-- 
Péter