linux-kernel - Re: [PATCH] ASoC: qcom: lpass-cpu: Remove bit clock state check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d33fd359-9dbf-b03c-ccd1-d93c7d207ccf@codeaurora.org>
Date:   Wed, 27 Jan 2021 16:24:36 +0530
From:   Srinivasa Rao Mandadapu <srivasam@...eaurora.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        agross@...nel.org, bjorn.andersson@...aro.org, lgirdwood@...il.com,
        broonie@...nel.org, robh+dt@...nel.org, plai@...eaurora.org,
        bgoswami@...eaurora.org, perex@...ex.cz, tiwai@...e.com,
        rohitkr@...eaurora.org, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: qcom: lpass-cpu: Remove bit clock state check

Thanks Srinivas For Your time!!!

On 1/27/2021 3:21 PM, Srinivas Kandagatla wrote:
>
>
> On 27/01/2021 06:30, Srinivasa Rao Mandadapu wrote:
>> No need of BCLK state maintenance from driver side as
>> clock_enable and clk_disable API's maintaing state counter.
>>
>> One of the major issue was spotted when Headset jack inserted
>> while playback continues, due to same PCM device node opens twice
>> for playaback/capture and closes once for capture and playback 
>> continues.
>>
>> It can resolve the errors in such scenarios.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@...eaurora.org>
>
> Thanks for the cleanup, yes clk core will take care of this by 
> enable_count check!
>
> You should add
>
> Fixes: b1824968221c ("ASoC: qcom: Fix enabling BCLK and LRCLK in LPAIF 
> invalid state")
>
Okay. I will do in next patch.
>
>> ---
>>   sound/soc/qcom/lpass-cpu.c       | 22 ++++++++--------------
>>   sound/soc/qcom/lpass-lpaif-reg.h |  3 ---
>>   sound/soc/qcom/lpass.h           |  1 -
>>   3 files changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
>> index ae8efbc89af2..a669202e0001 100644
>> --- a/sound/soc/qcom/lpass-cpu.c
>> +++ b/sound/soc/qcom/lpass-cpu.c
>> @@ -286,16 +286,12 @@ static int lpass_cpu_daiops_trigger(struct 
>> snd_pcm_substream *substream,
>>               dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>>                   ret);
>>   -        if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_DISABLE) {
>> -            ret = clk_enable(drvdata->mi2s_bit_clk[id]);
>> -            if (ret) {
>> -                dev_err(dai->dev, "error in enabling mi2s bit clk: 
>> %d\n", ret);
>> -                clk_disable(drvdata->mi2s_osr_clk[id]);
>> -                return ret;
>> -            }
>> -            drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_ENABLE;
>> +        ret = clk_enable(drvdata->mi2s_bit_clk[id]);
>> +        if (ret) {
>> +            dev_err(dai->dev, "error in enabling mi2s bit clk: 
>> %d\n", ret);
>> +            clk_disable(drvdata->mi2s_osr_clk[id]);
>
> Can you also remove this unnecessary disable here!
>
>
Actually this is MI2S OSR clock disable on failure of bit clock enable.

Do You think it's redundant?

> -srini
>
>> +            return ret;
>>           }
>> -
>>           break;
>>       case SNDRV_PCM_TRIGGER_STOP:
>>       case SNDRV_PCM_TRIGGER_SUSPEND:
>> @@ -310,10 +306,9 @@ static int lpass_cpu_daiops_trigger(struct 
>> snd_pcm_substream *substream,
>>           if (ret)
>>               dev_err(dai->dev, "error writing to i2sctl reg: %d\n",
>>                   ret);
>> -        if (drvdata->bit_clk_state[id] == LPAIF_BIT_CLK_ENABLE) {
>> - clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
>> -            drvdata->bit_clk_state[id] = LPAIF_BIT_CLK_DISABLE;
>> -        }
>> +
>> + clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]);
>> +
>>           break;
>>       }
>>   @@ -861,7 +856,6 @@ int asoc_qcom_lpass_cpu_platform_probe(struct 
>> platform_device *pdev)
>>                   PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
>>               return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
>>           }
>> -        drvdata->bit_clk_state[dai_id] = LPAIF_BIT_CLK_DISABLE;
>>       }
>>         /* Allocation for i2sctl regmap fields */
>> diff --git a/sound/soc/qcom/lpass-lpaif-reg.h 
>> b/sound/soc/qcom/lpass-lpaif-reg.h
>> index 405542832e99..c8e1d75340b2 100644
>> --- a/sound/soc/qcom/lpass-lpaif-reg.h
>> +++ b/sound/soc/qcom/lpass-lpaif-reg.h
>> @@ -60,9 +60,6 @@
>>   #define LPAIF_I2SCTL_BITWIDTH_24    1
>>   #define LPAIF_I2SCTL_BITWIDTH_32    2
>>   -#define LPAIF_BIT_CLK_DISABLE        0
>> -#define LPAIF_BIT_CLK_ENABLE        1
>> -
>>   #define LPAIF_I2SCTL_RESET_STATE    0x003C0004
>>   #define LPAIF_DMACTL_RESET_STATE    0x00200000
>>   diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
>> index 2d68af0da34d..83b2e08ade06 100644
>> --- a/sound/soc/qcom/lpass.h
>> +++ b/sound/soc/qcom/lpass.h
>> @@ -68,7 +68,6 @@ struct lpass_data {
>>       unsigned int mi2s_playback_sd_mode[LPASS_MAX_MI2S_PORTS];
>>       unsigned int mi2s_capture_sd_mode[LPASS_MAX_MI2S_PORTS];
>>       int hdmi_port_enable;
>> -    int bit_clk_state[LPASS_MAX_MI2S_PORTS];
>>         /* low-power audio interface (LPAIF) registers */
>>       void __iomem *lpaif;
>>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.