[alsa-devel] [PATCH] ASoC: fsl_esai: Support synchronous mode

[S.j. Wang]

Hi

> 
> > > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > > In ESAI synchronous mode, the clock is generated by Tx, So we
> > > > should always set registers of Tx which relate with the bit clock
> > > > and frame clock generation (TCCR, TCR, ECR), even there is only Rx is
> working.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang at nxp.com>
> > > > ---
> > > >  sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> > > > index
> > > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > > --- a/sound/soc/fsl/fsl_esai.c
> > > > +++ b/sound/soc/fsl/fsl_esai.c
> > > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > > snd_soc_dai *dai, int clk_id,
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > +	if (esai_priv->synchronous && !tx) {
> > > > +		switch (clk_id) {
> > > > +		case ESAI_HCKR_FSYS:
> > > > +			fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > > +								freq, dir);
> > > > +			break;
> > > > +		case ESAI_HCKR_EXTAL:
> > > > +			fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > > +								freq, dir);
> > >
> > > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It
> > > feels very confusing to do so, especially without a comments.
> >
> > For sync mode, only RX is enabled,  the register of tx should be set,
> > so call the Set_dai_sysclk again.
> 
> Yea, I understood that. But why not just replace RX with TX on the register-
> writing level? Do we need to set both TCCR and RCCR? Your change in
> hw_params() only sets TCCR inside fsl_esai_set_bclk(), so we probably only
> need to change TCCR for recordings running in sync mode, right?
> 
> From the commit message, it feels like that only the clock-related fields in
> the TX registers need to be set. Things like calculation and setting the
> direction of HCKx pin don't need to run again.
> 
> > > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > > snd_pcm_substream *substream,
> > > >
> > > >  	bclk = params_rate(params) * slot_width * esai_priv->slots;
> > > >
> > > > -	ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > > +	ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > > +bclk);
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > > +	if (esai_priv->synchronous && !tx) {
> > > > +		/* Use Normal mode to support monaural audio */
> > > > +		regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > > +			   ESAI_xCR_xMOD_MASK,
> > > params_channels(params) > 1 ?
> > > > +			   ESAI_xCR_xMOD_NETWORK : 0);
> > > > +
> > > > +		mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > > +		val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > > +		regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > mask, val);
> > > > +	}
> > >
> > > Does synchronous mode require to set both TCR and RCR? or just TCR?
> 
> > Both TCR and RCR.  RCR will be set in normal flow.
> 
> OK. Settings both xCRs makes sense. Would you please try this:
> 
> ===============
> @@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct
> snd_pcm_substream *substream,
> 
>         bclk = params_rate(params) * slot_width * esai_priv->slots;
> 
> -       ret = fsl_esai_set_bclk(dai, tx, bclk);
> +       /* Synchronous mode uses TX clock generator */
> +       ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx,
> + bclk);
>         if (ret)
>                 return ret;
> 
> +       mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
> +       val = ESAI_xCR_xSWS(slot_width, width);
>         /* Use Normal mode to support monaural audio */
> -       regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> -                          ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
> -                          ESAI_xCR_xMOD_NETWORK : 0);
> +       val |= params_channels(params) > 1 ? ESAI_xCR_xMOD_NETWORK : 0;
> +
> +       regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> +       /* Recording in synchronous mode needs to set TCR also */
> +       if (!tx && esai_priv->synchronous)
> +               regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + mask, val);
> 
>         regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
>                            ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR); @@ -556,10
> +577,10 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
> *substream,
> 
>         regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
> 
> -       mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
> -       val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
> -
> -       regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> +       /* Only TCR has padding bit and needs to be set for synchronous
> mode */
> +       if (tx || esai_priv->synchronous)
> +               regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> +                                  ESAI_xCR_PADC, ESAI_xCR_PADC);
> 
>         /* Remove ESAI personal reset by configuring ESAI_PCRC and
> ESAI_PRRC */
>         regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> ===============
> 
> It merges those plain settings so we won't need to change both sides if
> there's a modification of xCR settings in the future.

Thanks for reviewing,  will send v2.  And the ESAI_xCR_PADC, ESAI_xCR_xMOD_NETWORK
Is not needed in sync mode.