RE: [PATCH V2] ASoC: fsl_esai: Support synchronous mode

2019-04-04 Thread S.j. Wang
Hi

> 
> This looks better :)
> 
> On Wed, Apr 03, 2019 at 10:07:40AM +, S.j. Wang wrote:
> > @@ -218,7 +218,7 @@ static int fsl_esai_set_dai_sysclk(struct
> > snd_soc_dai *dai, int clk_id,  {
> > struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
> > struct clk *clksrc = esai_priv->extalclk;
> > -   bool tx = clk_id <= ESAI_HCKT_EXTAL;
> > +   bool tx = (clk_id <= ESAI_HCKT_EXTAL || esai_priv->synchronous);
> > bool in = dir == SND_SOC_CLOCK_IN;
> > u32 ratio, ecr = 0;
> > unsigned long clk_rate;
> > @@ -253,7 +253,7 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > ecr |= ESAI_ECR_ETI;
> > /* fall through */
> 
> Btw, I am also wondering if the fall through here is a bug
> Because I don't recall that there is a specific reason to fall through here.
> Can you please help confirm? Perhaps we need to submit a separate fix as
> well by replacing it with a "break;".
Yes, I think there is issue here, will submit another patch for it.
> 
> > case ESAI_HCKR_EXTAL:
> > -   ecr |= ESAI_ECR_ERI;
> > +   ecr |= esai_priv->synchronous ? ESAI_ECR_ETI :
> ESAI_ECR_ERI;
> > break;
> > default:
> > return -EINVAL;
> 
> > @@ -537,10 +537,18 @@ 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 || tx, bclk);
> > if (ret)
> > return ret;
> >
> > +   mask = ESAI_xCR_xSWS_MASK;
> > +   val = ESAI_xCR_xSWS(slot_width, width);
> > +
> > +   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);
> > +
> > /* 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 ?
> > @@ -556,10 +564,9 @@ 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);
> > +   if (tx)
> > +   regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > +   ESAI_xCR_PADC, ESAI_xCR_PADC);
> 
> Mind aligning the indentation here like the one below?
>   regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
>  ESAI_xCR_PADC, ESAI_xCR_PADC);

Ok. Will send v3.
> 
> Once you fix the indentation, add this:
> 
> Acked-by: Nicolin Chen 
> 
> Thanks


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

2019-04-03 Thread Nicolin Chen
This looks better :)

On Wed, Apr 03, 2019 at 10:07:40AM +, S.j. Wang wrote:
> @@ -218,7 +218,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai 
> *dai, int clk_id,
>  {
>   struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
>   struct clk *clksrc = esai_priv->extalclk;
> - bool tx = clk_id <= ESAI_HCKT_EXTAL;
> + bool tx = (clk_id <= ESAI_HCKT_EXTAL || esai_priv->synchronous);
>   bool in = dir == SND_SOC_CLOCK_IN;
>   u32 ratio, ecr = 0;
>   unsigned long clk_rate;
> @@ -253,7 +253,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai 
> *dai, int clk_id,
>   ecr |= ESAI_ECR_ETI;
>   /* fall through */

Btw, I am also wondering if the fall through here is a bug
Because I don't recall that there is a specific reason to fall
through here. Can you please help confirm? Perhaps we need to
submit a separate fix as well by replacing it with a "break;".

>   case ESAI_HCKR_EXTAL:
> - ecr |= ESAI_ECR_ERI;
> + ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
>   break;
>   default:
>   return -EINVAL;

> @@ -537,10 +537,18 @@ 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 || tx, bclk);
>   if (ret)
>   return ret;
>  
> + mask = ESAI_xCR_xSWS_MASK;
> + val = ESAI_xCR_xSWS(slot_width, width);
> +
> + 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);
> +
>   /* 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 ?
> @@ -556,10 +564,9 @@ 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);
> + if (tx)
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_PADC, ESAI_xCR_PADC);

Mind aligning the indentation here like the one below?
regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
   ESAI_xCR_PADC, ESAI_xCR_PADC);

Once you fix the indentation, add this:

Acked-by: Nicolin Chen 

Thanks