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