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@xxxxxxx>
> > > > ---
> > > > 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
