[PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[bardliao]

From: Bard Liao <bardliao@realtek.com>

We have been using rt5640.c codec driver with RT5642 codec chip before commit
022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using
device ID reading in reset register for adding device specific controls and
routes runtime.

Now since device ID appears to be different between RT5640 and RT5642 the 
driver doesn't add those controls and routes that are valid also on RT5642.

Fix this by adding a device ID found by debugging and minimal code for 
supporting RT5642.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Bard Liao <bardliao@realtek.com>
---
This patch add a mask for reading the device ID. This can remove the affect of
other bits which is not related to device ID.

---
 sound/soc/codecs/rt5640.c |  8 +++++---
 sound/soc/codecs/rt5640.h | 10 +++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index 6674372..79635ee 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -1997,8 +1997,9 @@ static int rt5640_probe(struct snd_soc_codec *codec)
 	snd_soc_update_bits(codec, RT5640_MICBIAS, 0x0030, 0x0030);
 	snd_soc_update_bits(codec, RT5640_DSP_PATH2, 0xfc00, 0x0c00);
 
-	switch (snd_soc_read(codec, RT5640_RESET)) {
-	case RT5640_RESET_ID:
+	switch (snd_soc_read(codec, RT5640_RESET) & RT5640_ID_MASK) {
+	case RT5640_ID_5640:
+	case RT5640_ID_5642:
 		snd_soc_add_codec_controls(codec,
 			rt5640_specific_snd_controls,
 			ARRAY_SIZE(rt5640_specific_snd_controls));
@@ -2009,7 +2010,7 @@ static int rt5640_probe(struct snd_soc_codec *codec)
 			rt5640_specific_dapm_routes,
 			ARRAY_SIZE(rt5640_specific_dapm_routes));
 		break;
-	case RT5639_RESET_ID:
+	case RT5640_ID_5639:
 		snd_soc_dapm_new_controls(&codec->dapm,
 			rt5639_specific_dapm_widgets,
 			ARRAY_SIZE(rt5639_specific_dapm_widgets));
@@ -2149,6 +2150,7 @@ static const struct regmap_config rt5640_regmap = {
 static const struct i2c_device_id rt5640_i2c_id[] = {
 	{ "rt5640", 0 },
 	{ "rt5639", 0 },
+	{ "rt5642", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id);
diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h
index 3b50459..ded2059 100644
--- a/sound/soc/codecs/rt5640.h
+++ b/sound/soc/codecs/rt5640.h
@@ -14,9 +14,6 @@
 
 #include <sound/rt5640.h>
 
-#define RT5639_RESET_ID				0x0008
-#define RT5640_RESET_ID				0x000c
-
 /* Info */
 #define RT5640_RESET				0x00
 #define RT5640_VENDOR_ID			0xfd
@@ -195,6 +192,13 @@
 #define RT5640_R_VOL_MASK			(0x3f)
 #define RT5640_R_VOL_SFT			0
 
+/* SW Reset & Device ID (0x00) */
+#define RT5640_ID_MASK				(0x3 << 1)
+#define RT5640_ID_5639				(0x0 << 1)
+#define RT5640_ID_5640				(0x1 << 1)
+#define RT5640_ID_5642				(0x3 << 1)
+
+
 /* IN1 and IN2 Control (0x0d) */
 /* IN3 and IN4 Control (0x0e) */
 #define RT5640_BST_SFT1				12
-- 
1.8.1.1.439.g50a6b54

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Mark Brown]

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

On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote:

> From: Bard Liao <bardliao@realtek.com>

> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Bard Liao <bardliao@realtek.com>

Jarkko, are you OK with this (and the change in general)?

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

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

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Jarkko Nikula]

On 04/17/2014 10:01 PM, Mark Brown wrote:
> On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote:
>
>> From: Bard Liao <bardliao@realtek.com>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Signed-off-by: Bard Liao <bardliao@realtek.com>
> Jarkko, are you OK with this (and the change in general)?
Yes, it's effectively same as my patch with Bard's actual device ID bits 
description in reset register.

-- 
Jarkko

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Mark Brown]

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

On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> We have been using rt5640.c codec driver with RT5642 codec chip before commit
> 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using
> device ID reading in reset register for adding device specific controls and
> routes runtime.

Applied, thanks.  Now I look again and remember that it exists I see
there's no update to the DT portions of the code - please submit a
followup patch adding the device to the DT bindings and to the OF match
table.  rt5639 is also missing there.

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

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

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Oder]

2014-04-22 19:53 GMT+08:00 Mark Brown <broonie@kernel.org>:
> On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote:
>> From: Bard Liao <bardliao@realtek.com>
>>
>> We have been using rt5640.c codec driver with RT5642 codec chip before commit
>> 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using
>> device ID reading in reset register for adding device specific controls and
>> routes runtime.
>
> Applied, thanks.  Now I look again and remember that it exists I see
> there's no update to the DT portions of the code - please submit a
> followup patch adding the device to the DT bindings and to the OF match
> table.  rt5639 is also missing there.

We've sent the patch of binding document to
"/Documentation/devicetree/bindings/sound/rt5640.txt" for rt5639.
If it is not satisfied, please kindly tell us what we need to do,
thank you very much.

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Mark Brown]

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

On Tue, Apr 22, 2014 at 10:14:47PM +0800, Oder wrote:

> > Applied, thanks.  Now I look again and remember that it exists I see
> > there's no update to the DT portions of the code - please submit a
> > followup patch adding the device to the DT bindings and to the OF match
> > table.  rt5639 is also missing there.

> We've sent the patch of binding document to
> "/Documentation/devicetree/bindings/sound/rt5640.txt" for rt5639.
> If it is not satisfied, please kindly tell us what we need to do,
> thank you very much.

It's not in the match table in the driver as well, just the binding
document isn't enough.

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

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

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Stephen Warren]

On 04/16/2014 08:24 PM, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> We have been using rt5640.c codec driver with RT5642 codec chip before commit
> 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using
> device ID reading in reset register for adding device specific controls and
> routes runtime.
> 
> Now since device ID appears to be different between RT5640 and RT5642 the 
> driver doesn't add those controls and routes that are valid also on RT5642.
> 
> Fix this by adding a device ID found by debugging and minimal code for 
> supporting RT5642.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Is this derived from Jarkko's patch? If so, shouldn't he be listed as
the author, not you? If it wasn't, then presumably his S-o-b line
shouldn't be in the patch description.

> Signed-off-by: Bard Liao <bardliao@realtek.com>

This patch causes problems for me. I see an enormous amount of spew
during kernel boot along the lines of:

> [    2.285515] rt5640 0-001c: ASoC: no source widget found for OUT MIXL
> [    2.291899] rt5640 0-001c: ASoC: Failed to add route OUT MIXL -> OUT MIXL Switch -> RECMIXL
> [    2.300306] rt5640 0-001c: ASoC: no source widget found for OUT MIXR
> [    2.306662] rt5640 0-001c: ASoC: Failed to add route OUT MIXR -> OUT MIXR Switch -> RECMIXR
> [    2.315662] rt5640 0-001c: ASoC: no sink widget found for Stereo DAC MIXL

(but repeated for about 28 widget/route pairs)

Perhaps it's related to:

> diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h

> -#define RT5639_RESET_ID				0x0008
> -#define RT5640_RESET_ID				0x000c

These values cover at least bits 3:2.

> +/* SW Reset & Device ID (0x00) */
> +#define RT5640_ID_MASK				(0x3 << 1)
> +#define RT5640_ID_5639				(0x0 << 1)
> +#define RT5640_ID_5640				(0x1 << 1)
> +#define RT5640_ID_5642				(0x3 << 1)

whereas these values cover bits 2:1. Should the shift be 2? Even with
that fixed, the old 5639 value was 2 << 2 but the new value here is 0 <<
2. Similarly, the old 5640 value was 3 <<2 whereas the new value is 1 <<
2. There's obviously quite some confusion here.

The schematic of my board says I have an RT5640, everything worked
before this commit, and register 0 (where these values are stored) reads
as 0xc which matches the RT5640 value before this commit but not after.

I can see why this patch causes the driver to support the wrong chip.
However, I can't imagine why that causes all the log spew at startup.
Perhaps the driver is just broken on RT5639 at present (although I don't
recall seeing any issues when booting on a board that actually had
one...) Is part of the driver keying off this now incorrect ID register
read, yet some other part of the driver registering widgets/routes based
on which entry matched in struct i2c_device_id rt5640_i2c_id, hence
they're falling out of sync due to this change? If so, that seems like
another bug that needs fixing.

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Mark Brown]

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

On Fri, Apr 25, 2014 at 04:08:18PM -0600, Stephen Warren wrote:
> On 04/16/2014 08:24 PM, bardliao@realtek.com wrote:

> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

> Is this derived from Jarkko's patch? If so, shouldn't he be listed as
> the author, not you? If it wasn't, then presumably his S-o-b line
> shouldn't be in the patch description.

It was derived but edited; I did check with Jarkko that he was OK before
applying.

> I can see why this patch causes the driver to support the wrong chip.
> However, I can't imagine why that causes all the log spew at startup.
> Perhaps the driver is just broken on RT5639 at present (although I don't
> recall seeing any issues when booting on a board that actually had
> one...) Is part of the driver keying off this now incorrect ID register
> read, yet some other part of the driver registering widgets/routes based
> on which entry matched in struct i2c_device_id rt5640_i2c_id, hence
> they're falling out of sync due to this change? If so, that seems like
> another bug that needs fixing.

I'd be surprised if it weren't the latter.  Either way it needs fixing.

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

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

Re: [PATCH v2] ASoC: rt5640: Add minimal support for RT5642

[Jarkko Nikula]

On 04/26/2014 03:22 AM, Mark Brown wrote:
> On Fri, Apr 25, 2014 at 04:08:18PM -0600, Stephen Warren wrote:
>> On 04/16/2014 08:24 PM, bardliao@realtek.com wrote:
>>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Is this derived from Jarkko's patch? If so, shouldn't he be listed as
>> the author, not you? If it wasn't, then presumably his S-o-b line
>> shouldn't be in the patch description.
> It was derived but edited; I did check with Jarkko that he was OK before
> applying.
It was ok to me since derivate had more information (device ID bit 
descriptions) than my patch.

-- 
Jarkko

Re: [PATCH v2] ASoC: rt5640: Add minimal supportforRT5642

[Bard Liao]

> On 04/16/2014 08:24 PM, bardliao@realtek.com wrote:
> > From: Bard Liao <bardliao@realtek.com>
> >
> > We have been using rt5640.c codec driver with RT5642 codec chip before commit
> > 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using
> > device ID reading in reset register for adding device specific controls and
> > routes runtime.
> >
> > Now since device ID appears to be different between RT5640 and RT5642 the
> > driver doesn't add those controls and routes that are valid also on RT5642.
> >
> > Fix this by adding a device ID found by debugging and minimal code for
> > supporting RT5642.
> >
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Is this derived from Jarkko's patch? If so, shouldn't he be listed as
> the author, not you? If it wasn't, then presumably his S-o-b line
> shouldn't be in the patch description.

I am sorry. I don't know the meaning of S-o-b's order.

> 
> > Signed-off-by: Bard Liao <bardliao@realtek.com>
> 
> This patch causes problems for me. I see an enormous amount of spew
> during kernel boot along the lines of:
> 
> > [    2.285515] rt5640 0-001c: ASoC: no source widget found for OUT MIXL
> > [    2.291899] rt5640 0-001c: ASoC: Failed to add route OUT MIXL -> OUT MIXL Switch -> RECMIXL
> > [    2.300306] rt5640 0-001c: ASoC: no source widget found for OUT MIXR
> > [    2.306662] rt5640 0-001c: ASoC: Failed to add route OUT MIXR -> OUT MIXR Switch -> RECMIXR
> > [    2.315662] rt5640 0-001c: ASoC: no sink widget found for Stereo DAC MIXL
> 
> (but repeated for about 28 widget/route pairs)
> 
> Perhaps it's related to:
> 
> > diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h
> 
> > -#define RT5639_RESET_ID                              0x0008
> > -#define RT5640_RESET_ID                              0x000c
> 
> These values cover at least bits 3:2.

Actually, only bits 2:1 is related to device ID. Others are for other informations.

> 
> > +/* SW Reset & Device ID (0x00) */
> > +#define RT5640_ID_MASK                               (0x3 << 1)
> > +#define RT5640_ID_5639                               (0x0 << 1)
> > +#define RT5640_ID_5640                               (0x1 << 1)
> > +#define RT5640_ID_5642                               (0x3 << 1)

It's my fault. 5640's ID should be (0x2 << 1) instead (0x1 << 1).

> 
> whereas these values cover bits 2:1. Should the shift be 2? Even with
> that fixed, the old 5639 value was 2 << 2 but the new value here is 0 <<
> 2. Similarly, the old 5640 value was 3 <<2 whereas the new value is 1 <<
> 2. There's obviously quite some confusion here.
> 
> The schematic of my board says I have an RT5640, everything worked
> before this commit, and register 0 (where these values are stored) reads
> as 0xc which matches the RT5640 value before this commit but not after.
> 
> I can see why this patch causes the driver to support the wrong chip.
> However, I can't imagine why that causes all the log spew at startup.
> Perhaps the driver is just broken on RT5639 at present (although I don't
> recall seeing any issues when booting on a board that actually had
> one...) Is part of the driver keying off this now incorrect ID register
> read, yet some other part of the driver registering widgets/routes based
> on which entry matched in struct i2c_device_id rt5640_i2c_id, hence
> they're falling out of sync due to this change? If so, that seems like
> another bug that needs fixing.

We will check and fix it.

Thanks.

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

Re: [PATCH v2] ASoC: rt5640: Add minimal supportforRT5642

[Mark Brown]

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

On Sun, Apr 27, 2014 at 02:57:24PM +0000, Bard Liao wrote:
> > On 04/16/2014 08:24 PM, bardliao@realtek.com wrote:

> > Is this derived from Jarkko's patch? If so, shouldn't he be listed as
> > the author, not you? If it wasn't, then presumably his S-o-b line
> > shouldn't be in the patch description.

> I am sorry. I don't know the meaning of S-o-b's order.

It's not just the ordering, it's also the authorship information -
signed off by has a specific meaning about who wrote the code and
handing off responsibility for it which is important to licensing.
Adding a signoff for someone who didn't write the code is therefore
particularly problematic.

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

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