Re: [PATCH] ASoC: fsl_esai: Support synchronous mode

[Nicolin Chen <nicoleotsuka@gmail.com>]

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