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

[S.j. Wang]

Hi

> 
> Shengjiu,
> 
> 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.

> 
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	/* Bypass divider settings if the requirement doesn't change */
> >  	if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
> >  		return 0;
> > @@ -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?
> The code behind this part is doing the same setting to RCR. If that is not
> needed any more for a synchronous recording, we should reuse it instead
> of inserting a piece of redundant code. Otherwise, if we need to set both,
> we should have two regmap_update_bits operations back-to-back for TCR
> and RCR (and other registers too).

Both TCR and RCR.  RCR will be set in normal flow.
> 
> > +
> >  	/* 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 ?
> 
> In case that we only need to set TCR (more likely I feel), it would feel less
> confusing to me, if we changed REG_ESAI_xCR(tx) here, for example, to
> REG_ESAI_xCR(tx || sync). Yea, please add to the top a 'bool sync =
> esai_priv->synchronous;'.
> 
> Similarly, for ECR_ETO and ECR_ERO:
> 	(tx || sync) ? ESAI_ECR_ETO : ESAI_ECR_ERO;

Both TCR and RCR should be set.