Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: srivasam=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org B9F65C433C6
Subject: Re: [PATCH v3] ASoC: qcom: Fix playback recover problem in suspend
 resume
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        agross@kernel.org, bjorn.andersson@linaro.org, lgirdwood@gmail.com,
        broonie@kernel.org, robh+dt@kernel.org, plai@codeaurora.org,
        bgoswami@codeaurora.org, perex@perex.cz, tiwai@suse.com,
        rohitkr@codeaurora.org, linux-arm-msm@vger.kernel.org,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     V Sujith Kumar Reddy <vsujithk@codeaurora.org>
References: <1606470988-26965-1-git-send-email-srivasam@codeaurora.org>
 <1146040c-5559-f4bf-fafe-eb7468e577a0@linaro.org>
From:   Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
Organization: Qualcomm India Private Limited.
Message-ID: <0055cc2e-87f1-bcc3-247b-8a4bd1bea883@codeaurora.org>
Date:   Fri, 27 Nov 2020 23:18:52 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.0
MIME-Version: 1.0
In-Reply-To: <1146040c-5559-f4bf-fafe-eb7468e577a0@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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.