Re: [PATCH v3] ASoC: qcom: Fix playback recover problem in suspend resume

[Srinivasa Rao Mandadapu <srivasam@codeaurora.org>]

Thanks a lot  Srinivas for your valuable review comments and for your 
time!!!

On 11/27/2020 5:39 PM, Srinivas Kandagatla wrote:
>
>
> On 27/11/2020 09:56, Srinivasa Rao Mandadapu wrote:
>> To support playback continuation after hard suspend(bypass powerd)
>>   and resume:
>> Prepare device in  platform trigger callback.
>> Make I2s and DMA control registers as non volatile.
> Looks like there are two changes here, One is fixing the volatile 
> registers!
>
>
> Other is preparing device after suspend!
>
> Consider splitting them!
>
Okay will split and share.
>>
>
> Fixes tag is missing here?
>
Actually fixing volatile registers is just partial reversal of commit 
b1824968221c ("ASoC: qcom: Fix enabling BCLK and LRCLK in LPAIF invalid 
state")

Will revert and share the patch separately.

>> Signed-off-by: V Sujith Kumar Reddy <vsujithk@codeaurora.org>
>> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
>> ---
>> Changes Since v1 and v2:
>>    -- Subject lines changed
>>
>>   sound/soc/qcom/lpass-cpu.c      | 8 ++------
>>   sound/soc/qcom/lpass-platform.c | 5 +++--
>>   2 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
>> index af684fd..c99be03 100644
>> --- a/sound/soc/qcom/lpass-cpu.c
>> +++ b/sound/soc/qcom/lpass-cpu.c
>> @@ -454,20 +454,16 @@ static bool lpass_cpu_regmap_volatile(struct 
>> device *dev, unsigned int reg)
>>       struct lpass_variant *v = drvdata->variant;
>>       int i;
>>   -    for (i = 0; i < v->i2s_ports; ++i)
>> -        if (reg == LPAIF_I2SCTL_REG(v, i))
>> -            return true;
>>       for (i = 0; i < v->irq_ports; ++i)
>>           if (reg == LPAIF_IRQSTAT_REG(v, i))
>>               return true;
>>         for (i = 0; i < v->rdma_channels; ++i)
>> -        if (reg == LPAIF_RDMACURR_REG(v, i) || reg == 
>> LPAIF_RDMACTL_REG(v, i))
>> +        if (reg == LPAIF_RDMACURR_REG(v, i))
>>               return true;
>>         for (i = 0; i < v->wrdma_channels; ++i)
>> -        if (reg == LPAIF_WRDMACURR_REG(v, i + 
>> v->wrdma_channel_start) ||
>> -            reg == LPAIF_WRDMACTL_REG(v, i + v->wrdma_channel_start))
>> +        if (reg == LPAIF_WRDMACURR_REG(v, i + v->wrdma_channel_start))
>>               return true;
>>         return false;
>> diff --git a/sound/soc/qcom/lpass-platform.c 
>> b/sound/soc/qcom/lpass-platform.c
>> index 80b09de..2b0a7c1 100644
>> --- a/sound/soc/qcom/lpass-platform.c
>> +++ b/sound/soc/qcom/lpass-platform.c
>> @@ -481,8 +481,9 @@ static int lpass_platform_pcmops_trigger(struct 
>> snd_soc_component *component,
>>           return -ENOTRECOVERABLE;
>>       }
>>       switch (cmd) {
>> -    case SNDRV_PCM_TRIGGER_START:
>>       case SNDRV_PCM_TRIGGER_RESUME:
>> +        lpass_platform_pcmops_prepare(component, substream);
>
> Can you elaborate the actual issue here?
>
> Are any other registers needs to re-programmed??
>
> Does it make sense to use
> regcache_mark_dirty()
> regcache_sync() in pm suspend resume path,
> instead of calling prepare explicitly?
>
>
> --srini
>
Yes it's working fine with regcache APIs. will update the patch.
>
>> +    case SNDRV_PCM_TRIGGER_START:
>>       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>>           ret = regmap_fields_write(dmactl->enable, id,
>>                            LPAIF_DMACTL_ENABLE_ON);
>> @@ -592,7 +593,7 @@ static int lpass_platform_pcmops_trigger(struct 
>> snd_soc_component *component,
>>           break;
>>       }
>>   -    return 0;
>> +    return ret;
>>   }
>>     static snd_pcm_uframes_t lpass_platform_pcmops_pointer(
>>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.