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

[Srinivasa Rao Mandadapu]

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@xxxxxxxxxxxxxx>
>
> 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.