Re: [PATCH v3 14/17] ASoC: fsl_ssi: Clean up _fsl_ssi_set_dai_fmt()

[Nicolin Chen]

On Mon, Jan 15, 2018 at 10:16:28PM +0100, Maciej S. Szmigiero wrote:

> > * Removing TXBIT0 configurations since this bit is set to 1 as its
> >   reset value and there is no use case so far to unset it. And it
> >   is safe to remove since regmap_update_bits() won't touch it.
> 
> I didn't get any response to a comment I've written about the point

I neglected the comments in the middle. Sorry. Will add it back then.

Re: [PATCH v3 14/17] ASoC: fsl_ssi: Clean up _fsl_ssi_set_dai_fmt()

[Maciej S. Szmigiero]

On 15.01.2018 05:21, Nicolin Chen wrote:
> The _fsl_ssi_set_dai_fmt() is a helper function being called from
> fsl_ssi_set_dai_fmt() as an ASoC operation and fsl_ssi_hw_init()
> mainly for AC97 format initialization.
> 
> This patch cleans the _fsl_ssi_set_dai_fmt() in following ways:
> * Removing *dev pointer in the parameters as it's included in the
>   *ssi pointer of struct fsl_ssi.
> * Using regmap_update_bits() instead of regmap_read() with masking
>   the value manually.
> * Removing TXBIT0 configurations since this bit is set to 1 as its
>   reset value and there is no use case so far to unset it. And it
>   is safe to remove since regmap_update_bits() won't touch it.

I didn't get any response to a comment I've written about the point
above during the previous patch iteration:> The old code set this bit in any 
mode other than AC'97 (where the
> hardware always treats this bit as set regardless of the actual value).
> I would play safe here and not rely on this bit being set by a SSI
> reset on all SSI models.

Maciej

> * Moving baudclk check to the switch-case routine to skip the I2S
>   master check. And moving SxCCR.DC settings after baudclk check.
> * Adding format settings for SND_SOC_DAIFMT_AC97 like others.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
> Changelog
> v3
>  * Put CBM_CFS behind the baudclk check to keep the same program
>flow as before
> 
>  sound/soc/fsl/fsl_ssi.c | 73 
> ++---
>  1 file changed, 33 insertions(+), 40 deletions(-)
> 

[PATCH v3 14/17] ASoC: fsl_ssi: Clean up _fsl_ssi_set_dai_fmt()

[Nicolin Chen]

The _fsl_ssi_set_dai_fmt() is a helper function being called from
fsl_ssi_set_dai_fmt() as an ASoC operation and fsl_ssi_hw_init()
mainly for AC97 format initialization.

This patch cleans the _fsl_ssi_set_dai_fmt() in following ways:
* Removing *dev pointer in the parameters as it's included in the
  *ssi pointer of struct fsl_ssi.
* Using regmap_update_bits() instead of regmap_read() with masking
  the value manually.
* Removing TXBIT0 configurations since this bit is set to 1 as its
  reset value and there is no use case so far to unset it. And it
  is safe to remove since regmap_update_bits() won't touch it.
* Moving baudclk check to the switch-case routine to skip the I2S
  master check. And moving SxCCR.DC settings after baudclk check.
* Adding format settings for SND_SOC_DAIFMT_AC97 like others.

Signed-off-by: Nicolin Chen 
Tested-by: Caleb Crome 
---
Changelog
v3
 * Put CBM_CFS behind the baudclk check to keep the same program
   flow as before

 sound/soc/fsl/fsl_ssi.c | 73 ++---
 1 file changed, 33 insertions(+), 40 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index ebb3eb9..9ff6734 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -860,42 +860,28 @@ static int fsl_ssi_hw_free(struct snd_pcm_substream 
*substream,
return 0;
 }
 
-static int _fsl_ssi_set_dai_fmt(struct device *dev,
-   struct fsl_ssi *ssi, unsigned int fmt)
+static int _fsl_ssi_set_dai_fmt(struct fsl_ssi *ssi, unsigned int fmt)
 {
-   struct regmap *regs = ssi->regs;
-   u32 strcr = 0, stcr, srcr, scr, mask;
+   u32 strcr = 0, scr = 0, stcr, srcr, mask;
 
ssi->dai_fmt = fmt;
 
-   if (fsl_ssi_is_i2s_master(ssi) && IS_ERR(ssi->baudclk)) {
-   dev_err(dev, "missing baudclk for master mode\n");
-   return -EINVAL;
-   }
-
-   regmap_read(regs, REG_SSI_SCR, );
-   scr &= ~(SSI_SCR_SYN | SSI_SCR_I2S_MODE_MASK);
/* Synchronize frame sync clock for TE to avoid data slipping */
scr |= SSI_SCR_SYNC_TX_FS;
 
-   mask = SSI_STCR_TXBIT0 | SSI_STCR_TFDIR | SSI_STCR_TXDIR |
-  SSI_STCR_TSCKP | SSI_STCR_TFSI | SSI_STCR_TFSL | SSI_STCR_TEFS;
-   regmap_read(regs, REG_SSI_STCR, );
-   regmap_read(regs, REG_SSI_SRCR, );
-   stcr &= ~mask;
-   srcr &= ~mask;
-
/* Use Network mode as default */
ssi->i2s_net = SSI_SCR_NET;
switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
case SND_SOC_DAIFMT_I2S:
-   regmap_update_bits(regs, REG_SSI_STCCR,
-  SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
-   regmap_update_bits(regs, REG_SSI_SRCCR,
-  SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
-   case SND_SOC_DAIFMT_CBM_CFS:
case SND_SOC_DAIFMT_CBS_CFS:
+   if (IS_ERR(ssi->baudclk)) {
+   dev_err(ssi->dev,
+   "missing baudclk for master mode\n");
+   return -EINVAL;
+   }
+   /* fall through */
+   case SND_SOC_DAIFMT_CBM_CFS:
ssi->i2s_net |= SSI_SCR_I2S_MODE_MASTER;
break;
case SND_SOC_DAIFMT_CBM_CFM:
@@ -905,30 +891,34 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
return -EINVAL;
}
 
+   regmap_update_bits(ssi->regs, REG_SSI_STCCR,
+  SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
+   regmap_update_bits(ssi->regs, REG_SSI_SRCCR,
+  SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
+
/* Data on rising edge of bclk, frame low, 1clk before data */
-   strcr |= SSI_STCR_TFSI | SSI_STCR_TSCKP |
-SSI_STCR_TXBIT0 | SSI_STCR_TEFS;
+   strcr |= SSI_STCR_TFSI | SSI_STCR_TSCKP | SSI_STCR_TEFS;
break;
case SND_SOC_DAIFMT_LEFT_J:
/* Data on rising edge of bclk, frame high */
-   strcr |= SSI_STCR_TXBIT0 | SSI_STCR_TSCKP;
+   strcr |= SSI_STCR_TSCKP;
break;
case SND_SOC_DAIFMT_DSP_A:
/* Data on rising edge of bclk, frame high, 1clk before data */
-   strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP |
-SSI_STCR_TXBIT0 | SSI_STCR_TEFS;
+   strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP | SSI_STCR_TEFS;
break;
case SND_SOC_DAIFMT_DSP_B:
/* Data on rising edge of bclk, frame high */
-   strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP | SSI_STCR_TXBIT0;
+   strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP;
break;
case