diff mbox

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

Message ID 1481c597e90b5daacce80a021fa0c513a7a2ec6d.1420451186.git.mengdong.lin@intel.com (mailing list archive)
State Accepted
Commit 3aebec3a701e70d6fe2816891e5abea066492779
Headers show

Commit Message

Lin, Mengdong Jan. 5, 2015, 9:48 a.m. UTC 
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>

Comments

Mark Brown Jan. 5, 2015, 6:17 p.m. UTC | #1 
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.
Bard Liao Jan. 6, 2015, 2:08 a.m. UTC | #2 
> -----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.
Mark Brown Jan. 6, 2015, 11:03 a.m. UTC | #3 
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.
Lin, Mengdong Jan. 7, 2015, 2:16 a.m. UTC | #4 
> -----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
diff mbox

Patch

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)