[PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[mengdong.lin]

From: Bard Liao <bardliao@realtek.com>

This patch fixes bit definitions for two ASRC control registers 0x84 and 0x85.

Signed-off-by: Bard Liao <bardliao@realtek.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index 54933f3..7c37b00 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -1023,50 +1023,33 @@
 #define RT5670_DMIC_2_M_NOR			(0x0 << 8)
 #define RT5670_DMIC_2_M_ASYN			(0x1 << 8)
 
+/* ASRC clock source selection (0x84, 0x85) */
+#define RT5670_CLK_SEL_SYS			(0x0)
+#define RT5670_CLK_SEL_I2S1_ASRC		(0x1)
+#define RT5670_CLK_SEL_I2S2_ASRC		(0x2)
+#define RT5670_CLK_SEL_I2S3_ASRC		(0x3)
+#define RT5670_CLK_SEL_SYS2			(0x5)
+#define RT5670_CLK_SEL_SYS3			(0x6)
+
 /* ASRC Control 2 (0x84) */
-#define RT5670_MDA_L_M_MASK			(0x1 << 15)
-#define RT5670_MDA_L_M_SFT			15
-#define RT5670_MDA_L_M_NOR			(0x0 << 15)
-#define RT5670_MDA_L_M_ASYN			(0x1 << 15)
-#define RT5670_MDA_R_M_MASK			(0x1 << 14)
-#define RT5670_MDA_R_M_SFT			14
-#define RT5670_MDA_R_M_NOR			(0x0 << 14)
-#define RT5670_MDA_R_M_ASYN			(0x1 << 14)
-#define RT5670_MAD_L_M_MASK			(0x1 << 13)
-#define RT5670_MAD_L_M_SFT			13
-#define RT5670_MAD_L_M_NOR			(0x0 << 13)
-#define RT5670_MAD_L_M_ASYN			(0x1 << 13)
-#define RT5670_MAD_R_M_MASK			(0x1 << 12)
-#define RT5670_MAD_R_M_SFT			12
-#define RT5670_MAD_R_M_NOR			(0x0 << 12)
-#define RT5670_MAD_R_M_ASYN			(0x1 << 12)
-#define RT5670_ADC_M_MASK			(0x1 << 11)
-#define RT5670_ADC_M_SFT			11
-#define RT5670_ADC_M_NOR			(0x0 << 11)
-#define RT5670_ADC_M_ASYN			(0x1 << 11)
-#define RT5670_STO_DAC_M_MASK			(0x1 << 5)
-#define RT5670_STO_DAC_M_SFT			5
-#define RT5670_STO_DAC_M_NOR			(0x0 << 5)
-#define RT5670_STO_DAC_M_ASYN			(0x1 << 5)
-#define RT5670_I2S1_R_D_MASK			(0x1 << 4)
-#define RT5670_I2S1_R_D_SFT			4
-#define RT5670_I2S1_R_D_DIS			(0x0 << 4)
-#define RT5670_I2S1_R_D_EN			(0x1 << 4)
-#define RT5670_I2S2_R_D_MASK			(0x1 << 3)
-#define RT5670_I2S2_R_D_SFT			3
-#define RT5670_I2S2_R_D_DIS			(0x0 << 3)
-#define RT5670_I2S2_R_D_EN			(0x1 << 3)
-#define RT5670_PRE_SCLK_MASK			(0x3)
-#define RT5670_PRE_SCLK_SFT			0
-#define RT5670_PRE_SCLK_512			(0x0)
-#define RT5670_PRE_SCLK_1024			(0x1)
-#define RT5670_PRE_SCLK_2048			(0x2)
+#define RT5670_DA_STO_CLK_SEL_MASK		(0xf << 12)
+#define RT5670_DA_STO_CLK_SEL_SFT		12
+#define RT5670_DA_MONOL_CLK_SEL_MASK		(0xf << 8)
+#define RT5670_DA_MONOL_CLK_SEL_SFT		8
+#define RT5670_DA_MONOR_CLK_SEL_MASK		(0xf << 4)
+#define RT5670_DA_MONOR_CLK_SEL_SFT		4
+#define RT5670_AD_STO1_CLK_SEL_MASK		(0xf << 0)
+#define RT5670_AD_STO1_CLK_SEL_SFT		0
 
 /* ASRC Control 3 (0x85) */
-#define RT5670_I2S1_RATE_MASK			(0xf << 12)
-#define RT5670_I2S1_RATE_SFT			12
-#define RT5670_I2S2_RATE_MASK			(0xf << 8)
-#define RT5670_I2S2_RATE_SFT			8
+#define RT5670_UP_CLK_SEL_MASK			(0xf << 12)
+#define RT5670_UP_CLK_SEL_SFT			12
+#define RT5670_DOWN_CLK_SEL_MASK		(0xf << 8)
+#define RT5670_DOWN_CLK_SEL_SFT			8
+#define RT5670_AD_MONOL_CLK_SEL_MASK		(0xf << 4)
+#define RT5670_AD_MONOL_CLK_SEL_SFT		4
+#define RT5670_AD_MONOR_CLK_SEL_MASK		(0xf << 0)
+#define RT5670_AD_MONOR_CLK_SEL_SFT		0
 
 /* ASRC Control 4 (0x89) */
 #define RT5670_I2S1_PD_MASK			(0x7 << 12)
-- 
1.9.1

[PATCH 2/3] ASoC: rt5670: add API to select ASRC clock source

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

When codec is in slave mode, ASRC can suppress noise for asynchronous
MCLK and LRCLK or special I2S format. This patch defines an API to select
the clock source for specified filters.  And the codec driver will turn on ASRC
for these filters if ASRC is selected as their clock source.

Signed-off-by: Bard Liao <bardliao@realtek.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
---
Bard reviewed the patch and adds clarification for ASRC.

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index da210fd..498dda2 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -728,6 +728,89 @@ static int can_use_asrc(struct snd_soc_dapm_widget *source,
 	return 0;
 }
 
+
+/**
+ * rt5670_sel_asrc_clk_src - select ASRC clock source for a set of filters
+ * @codec: SoC audio codec device.
+ * @filter_mask: mask of filters.
+ * @clk_src: clock source
+ *
+ * The ASRC function is for asynchronous MCLK and LRCK. Also, since RT5670 can
+ * only support standard 32fs or 64fs i2s format, ASRC should be enabled to
+ * support special i2s clock format such as Intel's 100fs(100 * sampling rate).
+ * ASRC function will track i2s clock and generate a corresponding system clock
+ * for codec. This function provides an API to select the clock source for a
+ * set of filters specified by the mask. And the codec driver will turn on ASRC
+ * for these filters if ASRC is selected as their clock source.
+ */
+int rt5670_sel_asrc_clk_src(struct snd_soc_codec *codec,
+			    unsigned int filter_mask, unsigned int clk_src)
+{
+	unsigned int asrc2_mask = 0, asrc2_value = 0;
+	unsigned int asrc3_mask = 0, asrc3_value = 0;
+
+	if (clk_src > RT5670_CLK_SEL_SYS3)
+		return -EINVAL;
+
+	if (filter_mask & RT5670_DA_STEREO_FILTER) {
+		asrc2_mask |= RT5670_DA_STO_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_STO_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_STO_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DA_MONO_L_FILTER) {
+		asrc2_mask |= RT5670_DA_MONOL_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_MONOL_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_MONOL_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DA_MONO_R_FILTER) {
+		asrc2_mask |= RT5670_DA_MONOR_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_MONOR_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_MONOR_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_STEREO_FILTER) {
+		asrc2_mask |= RT5670_AD_STO1_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_AD_STO1_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_STO1_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_MONO_L_FILTER) {
+		asrc3_mask |= RT5670_AD_MONOL_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_AD_MONOL_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_MONOL_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_MONO_R_FILTER)  {
+		asrc3_mask |= RT5670_AD_MONOR_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_AD_MONOR_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_MONOR_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_UP_RATE_FILTER) {
+		asrc3_mask |= RT5670_UP_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_UP_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_UP_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DOWN_RATE_FILTER) {
+		asrc3_mask |= RT5670_DOWN_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_DOWN_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DOWN_CLK_SEL_SFT);
+	}
+
+	if (asrc2_mask)
+		snd_soc_update_bits(codec, RT5670_ASRC_2,
+				    asrc2_mask, asrc2_value);
+
+	if (asrc3_mask)
+		snd_soc_update_bits(codec, RT5670_ASRC_3,
+				    asrc3_mask, asrc3_value);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rt5670_sel_asrc_clk_src);
+
 /* Digital Mixer */
 static const struct snd_kcontrol_new rt5670_sto1_adc_l_mix[] = {
 	SOC_DAPM_SINGLE("ADC1 Switch", RT5670_STO1_ADC_MIXER,
diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index 7c37b00..991314f 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -1966,6 +1966,21 @@ enum {
 	RT5670_DMIC_DATA_GPIO5,
 };
 
+/* filter mask */
+enum {
+	RT5670_DA_STEREO_FILTER = 0x1,
+	RT5670_DA_MONO_L_FILTER = (0x1 << 1),
+	RT5670_DA_MONO_R_FILTER = (0x1 << 2),
+	RT5670_AD_STEREO_FILTER = (0x1 << 3),
+	RT5670_AD_MONO_L_FILTER = (0x1 << 4),
+	RT5670_AD_MONO_R_FILTER = (0x1 << 5),
+	RT5670_UP_RATE_FILTER   = (0x1 << 6),
+	RT5670_DOWN_RATE_FILTER = (0x1 << 7),
+};
+
+int rt5670_sel_asrc_clk_src(struct snd_soc_codec *codec,
+			    unsigned int filter_mask, unsigned int clk_src);
+
 struct rt5670_priv {
 	struct snd_soc_codec *codec;
 	struct rt5670_platform_data pdata;
-- 
1.9.1

[PATCH 3/3] ASoC: Intel: Select RT5672 ASRC clock source on Cherrytrail and Braswell

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

On Cherrytrail and Braswell, the I2S BCLK is 100FS which cannot be supported
by RT5670 in slave mode and can cause noise.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/soc/intel/cht_bsw_rt5672.c b/sound/soc/intel/cht_bsw_rt5672.c
index 9b8b561..54a8f72 100644
--- a/sound/soc/intel/cht_bsw_rt5672.c
+++ b/sound/soc/intel/cht_bsw_rt5672.c
@@ -140,6 +140,7 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
 	struct snd_soc_dai *codec_dai = runtime->codec_dai;
+	struct snd_soc_codec *codec = codec_dai->codec;
 
 	/* TDM 4 slots 24 bit, set Rx & Tx bitmask to 4 active slots */
 	ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0xF, 4, 24);
@@ -148,6 +149,19 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
+	/* Select codec ASRC clock source to track I2S1 clock, because codec
+	 * is in slave mode and 100fs I2S format (BCLK = 100 * LRCLK) cannot
+	 * be supported by RT5672. Otherwise, ASRC will be disabled and cause
+	 * noise.
+	 */
+	rt5670_sel_asrc_clk_src(codec,
+				RT5670_DA_STEREO_FILTER
+				| RT5670_DA_MONO_L_FILTER
+				| RT5670_DA_MONO_R_FILTER
+				| RT5670_AD_STEREO_FILTER
+				| RT5670_AD_MONO_L_FILTER
+				| RT5670_AD_MONO_R_FILTER,
+				RT5670_CLK_SEL_I2S1_ASRC);
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2872 bytes --]

On Mon, Jan 05, 2015 at 05:48:15PM +0800, mengdong.lin@intel.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> This patch fixes bit definitions for two ASRC control registers 0x84 and 0x85.

This appears to do something more or different to fixing definitions -
if it was fixing I'd expect to see equal numbers of lines added and
removed in blocks identical apart from some numbers but this appears to
be adding some new definitions like these...

> +/* ASRC clock source selection (0x84, 0x85) */
> +#define RT5670_CLK_SEL_SYS			(0x0)
> +#define RT5670_CLK_SEL_I2S1_ASRC		(0x1)
> +#define RT5670_CLK_SEL_I2S2_ASRC		(0x2)
> +#define RT5670_CLK_SEL_I2S3_ASRC		(0x3)
> +#define RT5670_CLK_SEL_SYS2			(0x5)
> +#define RT5670_CLK_SEL_SYS3			(0x6)

...among other things, and this block here:

>  /* ASRC Control 2 (0x84) */
> -#define RT5670_MDA_L_M_MASK			(0x1 << 15)
> -#define RT5670_MDA_L_M_SFT			15
> -#define RT5670_MDA_L_M_NOR			(0x0 << 15)
> -#define RT5670_MDA_L_M_ASYN			(0x1 << 15)
> -#define RT5670_MDA_R_M_MASK			(0x1 << 14)
> -#define RT5670_MDA_R_M_SFT			14
> -#define RT5670_MDA_R_M_NOR			(0x0 << 14)
> -#define RT5670_MDA_R_M_ASYN			(0x1 << 14)
> -#define RT5670_MAD_L_M_MASK			(0x1 << 13)
> -#define RT5670_MAD_L_M_SFT			13
> -#define RT5670_MAD_L_M_NOR			(0x0 << 13)
> -#define RT5670_MAD_L_M_ASYN			(0x1 << 13)
> -#define RT5670_MAD_R_M_MASK			(0x1 << 12)
> -#define RT5670_MAD_R_M_SFT			12
> -#define RT5670_MAD_R_M_NOR			(0x0 << 12)
> -#define RT5670_MAD_R_M_ASYN			(0x1 << 12)
> -#define RT5670_ADC_M_MASK			(0x1 << 11)
> -#define RT5670_ADC_M_SFT			11
> -#define RT5670_ADC_M_NOR			(0x0 << 11)
> -#define RT5670_ADC_M_ASYN			(0x1 << 11)
> -#define RT5670_STO_DAC_M_MASK			(0x1 << 5)
> -#define RT5670_STO_DAC_M_SFT			5
> -#define RT5670_STO_DAC_M_NOR			(0x0 << 5)
> -#define RT5670_STO_DAC_M_ASYN			(0x1 << 5)
> -#define RT5670_I2S1_R_D_MASK			(0x1 << 4)
> -#define RT5670_I2S1_R_D_SFT			4
> -#define RT5670_I2S1_R_D_DIS			(0x0 << 4)
> -#define RT5670_I2S1_R_D_EN			(0x1 << 4)
> -#define RT5670_I2S2_R_D_MASK			(0x1 << 3)
> -#define RT5670_I2S2_R_D_SFT			3
> -#define RT5670_I2S2_R_D_DIS			(0x0 << 3)
> -#define RT5670_I2S2_R_D_EN			(0x1 << 3)
> -#define RT5670_PRE_SCLK_MASK			(0x3)
> -#define RT5670_PRE_SCLK_SFT			0
> -#define RT5670_PRE_SCLK_512			(0x0)
> -#define RT5670_PRE_SCLK_1024			(0x1)
> -#define RT5670_PRE_SCLK_2048			(0x2)
> +#define RT5670_DA_STO_CLK_SEL_MASK		(0xf << 12)
> +#define RT5670_DA_STO_CLK_SEL_SFT		12
> +#define RT5670_DA_MONOL_CLK_SEL_MASK		(0xf << 8)
> +#define RT5670_DA_MONOL_CLK_SEL_SFT		8
> +#define RT5670_DA_MONOR_CLK_SEL_MASK		(0xf << 4)
> +#define RT5670_DA_MONOR_CLK_SEL_SFT		4
> +#define RT5670_AD_STO1_CLK_SEL_MASK		(0xf << 0)
> +#define RT5670_AD_STO1_CLK_SEL_SFT		0

removes a lot more things than it adds, with different names too.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[Bard Liao]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, January 06, 2015 2:17 AM
> To: mengdong.lin@intel.com
> Cc: alsa-devel@alsa-project.org; Bard Liao
> Subject: Re: [PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control
> 
> On Mon, Jan 05, 2015 at 05:48:15PM +0800, mengdong.lin@intel.com
> wrote:
> > From: Bard Liao <bardliao@realtek.com>
> >
> > This patch fixes bit definitions for two ASRC control registers 0x84 and
> 0x85.
> 
> This appears to do something more or different to fixing definitions - if it
> was fixing I'd expect to see equal numbers of lines added and removed in
> blocks identical apart from some numbers but this appears to be adding
> some new definitions like these...

Maybe we should call redefine rather than fix. The previous definition of
registers 0x84 and 0x85 doesn't match the register's description. To make
the code more readable, we would like to rewrite the definition of registers
0x84 and 0x85.

> 
> > +/* ASRC clock source selection (0x84, 0x85) */
> > +#define RT5670_CLK_SEL_SYS			(0x0)
> > +#define RT5670_CLK_SEL_I2S1_ASRC		(0x1)
> > +#define RT5670_CLK_SEL_I2S2_ASRC		(0x2)
> > +#define RT5670_CLK_SEL_I2S3_ASRC		(0x3)
> > +#define RT5670_CLK_SEL_SYS2			(0x5)
> > +#define RT5670_CLK_SEL_SYS3			(0x6)
> 
> ...among other things, and this block here:

The description above is the same for all ASRC blocks. To save code size,
we define it in a separate part.
If we define it in each ASRC block, it would be something like
+#define RT5670_DA_STO_CLK_SEL_MASK		(0xf << 12)
+#define RT5670_DA_STO_CLK_SEL_SFT		12
+#define RT5670_DA_STO_CLK_SEL_SYS		(0 << 12)
+#define RT5670_DA_STO_CLK_SEL_I2S1_ASRC	(1 << 12)
+#define RT5670_DA_STO_CLK_SEL_I2S2_ASRC	(2 << 12)
+#define RT5670_DA_STO_CLK_SEL_I2S3_ASRC	(3 << 12)
+#define RT5670_DA_STO_CLK_SEL_ SYS2		(5 << 12)
+#define RT5670_DA_STO_CLK_SEL_ SYS3		(6 << 12)
And same thing for other ASRC blocks.

> 
> >  /* ASRC Control 2 (0x84) */
> > -#define RT5670_MDA_L_M_MASK			(0x1 << 15)
> > -#define RT5670_MDA_L_M_SFT			15
> > -#define RT5670_MDA_L_M_NOR			(0x0 << 15)
> > -#define RT5670_MDA_L_M_ASYN			(0x1 << 15)
> > -#define RT5670_MDA_R_M_MASK			(0x1 << 14)
> > -#define RT5670_MDA_R_M_SFT			14
> > -#define RT5670_MDA_R_M_NOR			(0x0 << 14)
> > -#define RT5670_MDA_R_M_ASYN			(0x1 << 14)
> > -#define RT5670_MAD_L_M_MASK			(0x1 << 13)
> > -#define RT5670_MAD_L_M_SFT			13
> > -#define RT5670_MAD_L_M_NOR			(0x0 << 13)
> > -#define RT5670_MAD_L_M_ASYN			(0x1 << 13)
> > -#define RT5670_MAD_R_M_MASK			(0x1 << 12)
> > -#define RT5670_MAD_R_M_SFT			12
> > -#define RT5670_MAD_R_M_NOR			(0x0 << 12)
> > -#define RT5670_MAD_R_M_ASYN			(0x1 << 12)
> > -#define RT5670_ADC_M_MASK			(0x1 << 11)
> > -#define RT5670_ADC_M_SFT			11
> > -#define RT5670_ADC_M_NOR			(0x0 << 11)
> > -#define RT5670_ADC_M_ASYN			(0x1 << 11)
> > -#define RT5670_STO_DAC_M_MASK			(0x1 << 5)
> > -#define RT5670_STO_DAC_M_SFT			5
> > -#define RT5670_STO_DAC_M_NOR			(0x0 << 5)
> > -#define RT5670_STO_DAC_M_ASYN			(0x1 << 5)
> > -#define RT5670_I2S1_R_D_MASK			(0x1 << 4)
> > -#define RT5670_I2S1_R_D_SFT			4
> > -#define RT5670_I2S1_R_D_DIS			(0x0 << 4)
> > -#define RT5670_I2S1_R_D_EN			(0x1 << 4)
> > -#define RT5670_I2S2_R_D_MASK			(0x1 << 3)
> > -#define RT5670_I2S2_R_D_SFT			3
> > -#define RT5670_I2S2_R_D_DIS			(0x0 << 3)
> > -#define RT5670_I2S2_R_D_EN			(0x1 << 3)
> > -#define RT5670_PRE_SCLK_MASK			(0x3)
> > -#define RT5670_PRE_SCLK_SFT			0
> > -#define RT5670_PRE_SCLK_512			(0x0)
> > -#define RT5670_PRE_SCLK_1024			(0x1)
> > -#define RT5670_PRE_SCLK_2048			(0x2)
> > +#define RT5670_DA_STO_CLK_SEL_MASK		(0xf << 12)
> > +#define RT5670_DA_STO_CLK_SEL_SFT		12
> > +#define RT5670_DA_MONOL_CLK_SEL_MASK		(0xf << 8)
> > +#define RT5670_DA_MONOL_CLK_SEL_SFT		8
> > +#define RT5670_DA_MONOR_CLK_SEL_MASK		(0xf << 4)
> > +#define RT5670_DA_MONOR_CLK_SEL_SFT		4
> > +#define RT5670_AD_STO1_CLK_SEL_MASK		(0xf << 0)
> > +#define RT5670_AD_STO1_CLK_SEL_SFT		0
> 
> removes a lot more things than it adds, with different names too.

The previous definition is totally wrong, so we just remove it and
write a new definition for it.

> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 859 bytes --]

On Tue, Jan 06, 2015 at 02:08:45AM +0000, Bard Liao wrote:

> > > This patch fixes bit definitions for two ASRC control registers 0x84 and
> > 0x85.

> > This appears to do something more or different to fixing definitions - if it
> > was fixing I'd expect to see equal numbers of lines added and removed in
> > blocks identical apart from some numbers but this appears to be adding
> > some new definitions like these...

> Maybe we should call redefine rather than fix. The previous definition of
> registers 0x84 and 0x85 doesn't match the register's description. To make
> the code more readable, we would like to rewrite the definition of registers
> 0x84 and 0x85.

Yes, if it's just redefining the definitions that are currently unused
then it should be described as such - a fix is something that should get
sent to Linus and possibly also to stable.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ASoC: rt5670: fix bit definition for ASRC control

[Lin, Mengdong]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, January 06, 2015 7:03 PM

> On Tue, Jan 06, 2015 at 02:08:45AM +0000, Bard Liao wrote:
> 
> > > > This patch fixes bit definitions for two ASRC control registers
> > > > 0x84 and
> > > 0x85.
> 
> > > This appears to do something more or different to fixing definitions
> > > - if it was fixing I'd expect to see equal numbers of lines added
> > > and removed in blocks identical apart from some numbers but this
> > > appears to be adding some new definitions like these...
> 
> > Maybe we should call redefine rather than fix. The previous definition
> > of registers 0x84 and 0x85 doesn't match the register's description.
> > To make the code more readable, we would like to rewrite the
> > definition of registers
> > 0x84 and 0x85.
> 
> Yes, if it's just redefining the definitions that are currently unused then it
> should be described as such - a fix is something that should get sent to Linus
> and possibly also to stable.

I've changed this patch subject to "redefine" and submitted the v2 patches. 
Please have a review.

Thanks
Mengdong

[PATCH v2 1/3] ASoC: rt5670: redefine ASRC control registers 0x84 and 0x85

[mengdong.lin]

From: Bard Liao <bardliao@realtek.com>

The previous definition of registers 0x84 and 0x85 doesn't match the datasheet.
So this patch removes the wrong definition and writes a new one for the two
registers.

Signed-off-by: Bard Liao <bardliao@realtek.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
---
v2: change subject from "fix definition" to "redefine"

diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index 54933f3..7c37b00 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -1023,50 +1023,33 @@
 #define RT5670_DMIC_2_M_NOR			(0x0 << 8)
 #define RT5670_DMIC_2_M_ASYN			(0x1 << 8)
 
+/* ASRC clock source selection (0x84, 0x85) */
+#define RT5670_CLK_SEL_SYS			(0x0)
+#define RT5670_CLK_SEL_I2S1_ASRC		(0x1)
+#define RT5670_CLK_SEL_I2S2_ASRC		(0x2)
+#define RT5670_CLK_SEL_I2S3_ASRC		(0x3)
+#define RT5670_CLK_SEL_SYS2			(0x5)
+#define RT5670_CLK_SEL_SYS3			(0x6)
+
 /* ASRC Control 2 (0x84) */
-#define RT5670_MDA_L_M_MASK			(0x1 << 15)
-#define RT5670_MDA_L_M_SFT			15
-#define RT5670_MDA_L_M_NOR			(0x0 << 15)
-#define RT5670_MDA_L_M_ASYN			(0x1 << 15)
-#define RT5670_MDA_R_M_MASK			(0x1 << 14)
-#define RT5670_MDA_R_M_SFT			14
-#define RT5670_MDA_R_M_NOR			(0x0 << 14)
-#define RT5670_MDA_R_M_ASYN			(0x1 << 14)
-#define RT5670_MAD_L_M_MASK			(0x1 << 13)
-#define RT5670_MAD_L_M_SFT			13
-#define RT5670_MAD_L_M_NOR			(0x0 << 13)
-#define RT5670_MAD_L_M_ASYN			(0x1 << 13)
-#define RT5670_MAD_R_M_MASK			(0x1 << 12)
-#define RT5670_MAD_R_M_SFT			12
-#define RT5670_MAD_R_M_NOR			(0x0 << 12)
-#define RT5670_MAD_R_M_ASYN			(0x1 << 12)
-#define RT5670_ADC_M_MASK			(0x1 << 11)
-#define RT5670_ADC_M_SFT			11
-#define RT5670_ADC_M_NOR			(0x0 << 11)
-#define RT5670_ADC_M_ASYN			(0x1 << 11)
-#define RT5670_STO_DAC_M_MASK			(0x1 << 5)
-#define RT5670_STO_DAC_M_SFT			5
-#define RT5670_STO_DAC_M_NOR			(0x0 << 5)
-#define RT5670_STO_DAC_M_ASYN			(0x1 << 5)
-#define RT5670_I2S1_R_D_MASK			(0x1 << 4)
-#define RT5670_I2S1_R_D_SFT			4
-#define RT5670_I2S1_R_D_DIS			(0x0 << 4)
-#define RT5670_I2S1_R_D_EN			(0x1 << 4)
-#define RT5670_I2S2_R_D_MASK			(0x1 << 3)
-#define RT5670_I2S2_R_D_SFT			3
-#define RT5670_I2S2_R_D_DIS			(0x0 << 3)
-#define RT5670_I2S2_R_D_EN			(0x1 << 3)
-#define RT5670_PRE_SCLK_MASK			(0x3)
-#define RT5670_PRE_SCLK_SFT			0
-#define RT5670_PRE_SCLK_512			(0x0)
-#define RT5670_PRE_SCLK_1024			(0x1)
-#define RT5670_PRE_SCLK_2048			(0x2)
+#define RT5670_DA_STO_CLK_SEL_MASK		(0xf << 12)
+#define RT5670_DA_STO_CLK_SEL_SFT		12
+#define RT5670_DA_MONOL_CLK_SEL_MASK		(0xf << 8)
+#define RT5670_DA_MONOL_CLK_SEL_SFT		8
+#define RT5670_DA_MONOR_CLK_SEL_MASK		(0xf << 4)
+#define RT5670_DA_MONOR_CLK_SEL_SFT		4
+#define RT5670_AD_STO1_CLK_SEL_MASK		(0xf << 0)
+#define RT5670_AD_STO1_CLK_SEL_SFT		0
 
 /* ASRC Control 3 (0x85) */
-#define RT5670_I2S1_RATE_MASK			(0xf << 12)
-#define RT5670_I2S1_RATE_SFT			12
-#define RT5670_I2S2_RATE_MASK			(0xf << 8)
-#define RT5670_I2S2_RATE_SFT			8
+#define RT5670_UP_CLK_SEL_MASK			(0xf << 12)
+#define RT5670_UP_CLK_SEL_SFT			12
+#define RT5670_DOWN_CLK_SEL_MASK		(0xf << 8)
+#define RT5670_DOWN_CLK_SEL_SFT			8
+#define RT5670_AD_MONOL_CLK_SEL_MASK		(0xf << 4)
+#define RT5670_AD_MONOL_CLK_SEL_SFT		4
+#define RT5670_AD_MONOR_CLK_SEL_MASK		(0xf << 0)
+#define RT5670_AD_MONOR_CLK_SEL_SFT		0
 
 /* ASRC Control 4 (0x89) */
 #define RT5670_I2S1_PD_MASK			(0x7 << 12)
-- 
1.9.1

[PATCH v2 2/3] ASoC: rt5670: add API to select ASRC clock source

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

When codec is in slave mode, ASRC can suppress noise for asynchronous
MCLK and LRCLK or special I2S format. This patch defines an API to select
the clock source for specified filters.  And the codec driver will turn on ASRC
for these filters if ASRC is selected as their clock source.

Signed-off-by: Bard Liao <bardliao@realtek.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
---
Bard reviewed the patch and adds clarification for ASRC.

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index da210fd..498dda2 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -728,6 +728,89 @@ static int can_use_asrc(struct snd_soc_dapm_widget *source,
 	return 0;
 }
 
+
+/**
+ * rt5670_sel_asrc_clk_src - select ASRC clock source for a set of filters
+ * @codec: SoC audio codec device.
+ * @filter_mask: mask of filters.
+ * @clk_src: clock source
+ *
+ * The ASRC function is for asynchronous MCLK and LRCK. Also, since RT5670 can
+ * only support standard 32fs or 64fs i2s format, ASRC should be enabled to
+ * support special i2s clock format such as Intel's 100fs(100 * sampling rate).
+ * ASRC function will track i2s clock and generate a corresponding system clock
+ * for codec. This function provides an API to select the clock source for a
+ * set of filters specified by the mask. And the codec driver will turn on ASRC
+ * for these filters if ASRC is selected as their clock source.
+ */
+int rt5670_sel_asrc_clk_src(struct snd_soc_codec *codec,
+			    unsigned int filter_mask, unsigned int clk_src)
+{
+	unsigned int asrc2_mask = 0, asrc2_value = 0;
+	unsigned int asrc3_mask = 0, asrc3_value = 0;
+
+	if (clk_src > RT5670_CLK_SEL_SYS3)
+		return -EINVAL;
+
+	if (filter_mask & RT5670_DA_STEREO_FILTER) {
+		asrc2_mask |= RT5670_DA_STO_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_STO_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_STO_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DA_MONO_L_FILTER) {
+		asrc2_mask |= RT5670_DA_MONOL_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_MONOL_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_MONOL_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DA_MONO_R_FILTER) {
+		asrc2_mask |= RT5670_DA_MONOR_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_DA_MONOR_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DA_MONOR_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_STEREO_FILTER) {
+		asrc2_mask |= RT5670_AD_STO1_CLK_SEL_MASK;
+		asrc2_value = (asrc2_value & ~RT5670_AD_STO1_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_STO1_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_MONO_L_FILTER) {
+		asrc3_mask |= RT5670_AD_MONOL_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_AD_MONOL_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_MONOL_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_AD_MONO_R_FILTER)  {
+		asrc3_mask |= RT5670_AD_MONOR_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_AD_MONOR_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_AD_MONOR_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_UP_RATE_FILTER) {
+		asrc3_mask |= RT5670_UP_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_UP_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_UP_CLK_SEL_SFT);
+	}
+
+	if (filter_mask & RT5670_DOWN_RATE_FILTER) {
+		asrc3_mask |= RT5670_DOWN_CLK_SEL_MASK;
+		asrc3_value = (asrc3_value & ~RT5670_DOWN_CLK_SEL_MASK)
+				| (clk_src <<  RT5670_DOWN_CLK_SEL_SFT);
+	}
+
+	if (asrc2_mask)
+		snd_soc_update_bits(codec, RT5670_ASRC_2,
+				    asrc2_mask, asrc2_value);
+
+	if (asrc3_mask)
+		snd_soc_update_bits(codec, RT5670_ASRC_3,
+				    asrc3_mask, asrc3_value);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rt5670_sel_asrc_clk_src);
+
 /* Digital Mixer */
 static const struct snd_kcontrol_new rt5670_sto1_adc_l_mix[] = {
 	SOC_DAPM_SINGLE("ADC1 Switch", RT5670_STO1_ADC_MIXER,
diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index 7c37b00..991314f 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -1966,6 +1966,21 @@ enum {
 	RT5670_DMIC_DATA_GPIO5,
 };
 
+/* filter mask */
+enum {
+	RT5670_DA_STEREO_FILTER = 0x1,
+	RT5670_DA_MONO_L_FILTER = (0x1 << 1),
+	RT5670_DA_MONO_R_FILTER = (0x1 << 2),
+	RT5670_AD_STEREO_FILTER = (0x1 << 3),
+	RT5670_AD_MONO_L_FILTER = (0x1 << 4),
+	RT5670_AD_MONO_R_FILTER = (0x1 << 5),
+	RT5670_UP_RATE_FILTER   = (0x1 << 6),
+	RT5670_DOWN_RATE_FILTER = (0x1 << 7),
+};
+
+int rt5670_sel_asrc_clk_src(struct snd_soc_codec *codec,
+			    unsigned int filter_mask, unsigned int clk_src);
+
 struct rt5670_priv {
 	struct snd_soc_codec *codec;
 	struct rt5670_platform_data pdata;
-- 
1.9.1

[PATCH v2 3/3] ASoC: Intel: Select RT5672 ASRC clock source on Cherrytrail and Braswell

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

On Cherrytrail and Braswell, the I2S BCLK is 100FS which cannot be supported
by RT5672 in slave mode and can cause noise. This patch selects codec ASRC
clock source to track I2S1 clock so that codec ASRC can be enabled to suppress
the noise.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
Reviewed-by: Bard Liao <bardliao@realtek.com>

diff --git a/sound/soc/intel/cht_bsw_rt5672.c b/sound/soc/intel/cht_bsw_rt5672.c
index 9b8b561..54a8f72 100644
--- a/sound/soc/intel/cht_bsw_rt5672.c
+++ b/sound/soc/intel/cht_bsw_rt5672.c
@@ -140,6 +140,7 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
 	struct snd_soc_dai *codec_dai = runtime->codec_dai;
+	struct snd_soc_codec *codec = codec_dai->codec;
 
 	/* TDM 4 slots 24 bit, set Rx & Tx bitmask to 4 active slots */
 	ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0xF, 4, 24);
@@ -148,6 +149,19 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
+	/* Select codec ASRC clock source to track I2S1 clock, because codec
+	 * is in slave mode and 100fs I2S format (BCLK = 100 * LRCLK) cannot
+	 * be supported by RT5672. Otherwise, ASRC will be disabled and cause
+	 * noise.
+	 */
+	rt5670_sel_asrc_clk_src(codec,
+				RT5670_DA_STEREO_FILTER
+				| RT5670_DA_MONO_L_FILTER
+				| RT5670_DA_MONO_R_FILTER
+				| RT5670_AD_STEREO_FILTER
+				| RT5670_AD_MONO_L_FILTER
+				| RT5670_AD_MONO_R_FILTER,
+				RT5670_CLK_SEL_I2S1_ASRC);
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v2 1/3] ASoC: rt5670: redefine ASRC control registers 0x84 and 0x85

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 319 bytes --]

On Wed, Jan 07, 2015 at 10:19:06AM +0800, mengdong.lin@intel.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> The previous definition of registers 0x84 and 0x85 doesn't match the datasheet.
> So this patch removes the wrong definition and writes a new one for the two
> registers.

Applied all, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]