Skip to contentSkip to conversation

ASoC: add RT5639 CODEC driver

PATCH In review [email protected] 4 participants 10 messages Mar 24, 2014 – Mar 26, 2014

Subsystems (3)
Sound ASoC Realtek audio codecs
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

PATCH 1/1 ASoC: add RT5639 CODEC driver

From: Oder Chiou <[email protected]>

This patch adds the RT5639 codec driver.

Signed-off-by Bard Liao <[email protected]>
Signed-off-by Oder Chiou <[email protected]>

---

It is migrated from RT5640 codec driver,
and modified for RT5639's specification.

---
 include/sound/rt5639.h    |   22 +
 sound/soc/codecs/Kconfig  |    4 +
 sound/soc/codecs/Makefile |    2 +
 sound/soc/codecs/rt5639.c | 2092 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt5639.h | 1928 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 4048 insertions(+)
 create mode 100644 include/sound/rt5639.h
 create mode 100644 sound/soc/codecs/rt5639.c
 create mode 100644 sound/soc/codecs/rt5639.h
include/sound/rt5639.h new file +22 -0
new file mode 100644
index 0000000..7101120
@@ -0,0 +1,22 @@
+/*
+ * linux/sound/rt5639.h -- Platform data for RT5639
+ *
+ * Copyright 2011 Realtek Microelectronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_SND_RT5639_H
+#define __LINUX_SND_RT5639_H
+
+struct rt5639_platform_data {
+	/* IN1 & IN2 can optionally be differential */
+	bool in1_diff;
+	bool in2_diff;
+
+	int ldo1_en; /* GPIO for LDO1_EN */
+};
+
+#endif
sound/soc/codecs/Kconfig +4 -0
index 983d087a..ed791fb 100644
@@ -60,6 +60,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_PCM1792A if SPI_MASTER
 	select SND_SOC_PCM3008
 	select SND_SOC_RT5631 if I2C
+	select SND_SOC_RT5639 if I2C
 	select SND_SOC_RT5640 if I2C
 	select SND_SOC_SGTL5000 if I2C
 	select SND_SOC_SI476X if MFD_SI476X_CORE

@@ -316,6 +317,9 @@ config SND_SOC_PCM3008
 config SND_SOC_RT5631
 	tristate
 
+config SND_SOC_RT5639
+	tristate
+
 config SND_SOC_RT5640
 	tristate
sound/soc/codecs/Makefile +2 -0
index bc12676..051f3fb 100644
@@ -47,6 +47,7 @@ snd-soc-pcm1681-objs := pcm1681.o
 snd-soc-pcm1792a-codec-objs := pcm1792a.o
 snd-soc-pcm3008-objs := pcm3008.o
 snd-soc-rt5631-objs := rt5631.o
+snd-soc-rt5639-objs := rt5639.o
 snd-soc-rt5640-objs := rt5640.o
 snd-soc-sgtl5000-objs := sgtl5000.o
 snd-soc-alc5623-objs := alc5623.o

@@ -180,6 +181,7 @@ obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
 obj-$(CONFIG_SND_SOC_PCM1792A)	+= snd-soc-pcm1792a-codec.o
 obj-$(CONFIG_SND_SOC_PCM3008)	+= snd-soc-pcm3008.o
 obj-$(CONFIG_SND_SOC_RT5631)	+= snd-soc-rt5631.o
+obj-$(CONFIG_SND_SOC_RT5639)	+= snd-soc-rt5639.o
 obj-$(CONFIG_SND_SOC_RT5640)	+= snd-soc-rt5640.o
 obj-$(CONFIG_SND_SOC_SGTL5000)  += snd-soc-sgtl5000.o
 obj-$(CONFIG_SND_SOC_SIGMADSP)	+= snd-soc-sigmadsp.o
sound/soc/codecs/rt5639.c new file +2092 -0

2,092 lines not on this page · raw message

sound/soc/codecs/rt5639.h new file +1928 -0

856 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
1.8.1.1.439.g50a6b54

Part of the diff is not shown here. View the raw message.

maintainer
#1 lore raw
Quoted 3 lines · On Mon, Mar 24, 2014 at 10:16:37AM +0800, [email protected] wrote:

From: Oder Chiou <[email protected]>

This patch adds the RT5639 codec driver.

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared. What are the issues that prevent the code being shared, there may be something I've missed?

There's also a few issues below that should be fixed, most of them should be fixed in rt5640 too.

Quoted 5 lines
sound/soc/codecs/rt5639.c new file +5 -0
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		val_len |= RT5639_I2S_DL_20;

sound/soc/codecs/rt5639.c line 1524

Quoted 10 lines
sound/soc/codecs/rt5639.c new file +10 -0
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		val_len |= RT5639_I2S_DL_24;
+		break;
+	case SNDRV_PCM_FORMAT_S8:
+		val_len |= RT5639_I2S_DL_8;
+		break;
+	default:
+		return -EINVAL;
+	}

This should be converted to use params_width().

Quoted 8 lines
sound/soc/codecs/rt5639.c new file +8 -0
+static int rt5639_set_bias_level(struct snd_soc_codec *codec,
+			enum snd_soc_bias_level level)
+{
+	struct rt5639_priv *rt5639 = snd_soc_codec_get_drvdata(codec);
+	switch (level) {
+	case SND_SOC_BIAS_STANDBY:
+		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
+			regcache_cache_only(rt5639->regmap, false);

...

sound/soc/codecs/rt5639.c line 1804

Quoted 11 lines
sound/soc/codecs/rt5639.c new file +11 -0
+	case SND_SOC_BIAS_OFF:
+		snd_soc_write(codec, RT5639_DEPOP_M1, 0x0004);
+		snd_soc_write(codec, RT5639_DEPOP_M2, 0x1100);
+		snd_soc_update_bits(codec, RT5639_DUMMY1, 0x1, 0);
+		snd_soc_write(codec, RT5639_PWR_DIG1, 0x0000);
+		snd_soc_write(codec, RT5639_PWR_DIG2, 0x0000);
+		snd_soc_write(codec, RT5639_PWR_VOL, 0x0000);
+		snd_soc_write(codec, RT5639_PWR_MIXER, 0x0000);
+		snd_soc_write(codec, RT5639_PWR_ANLG1, 0x0000);
+		snd_soc_write(codec, RT5639_PWR_ANLG2, 0x0000);
+		break;

I'd expect to see the regmap being made cache_only in _OFF given that it is resynced in _STANDBY when exiting _OFF, or alternatively to see the cache restore moved to the resume to mirror where it's currently made cache only.

sound/soc/codecs/rt5639.c line 1826

Quoted 13 lines
sound/soc/codecs/rt5639.c new file +13 -0
+static int rt5639_probe(struct snd_soc_codec *codec)
+{
+	struct rt5639_priv *rt5639 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	rt5639->codec = codec;
+	codec->control_data = rt5639->regmap;
+
+	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_REGMAP);
+	if (ret != 0) {
+		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
+		return ret;
+	}

There have been some API changes here this release cycle, you should be able to just remove the cache_io() and regmap setting code here.

sound/soc/codecs/rt5639.c line 2057

Quoted 7 lines
sound/soc/codecs/rt5639.c new file +7 -0
+	if (rt5639->pdata.in1_diff)
+		regmap_update_bits(rt5639->regmap, RT5639_IN1_IN2,
+					RT5639_IN_DF1, RT5639_IN_DF1);
+
+	if (rt5639->pdata.in2_diff)
+		regmap_update_bits(rt5639->regmap, RT5639_IN3_IN4,
+					RT5639_IN_DF2, RT5639_IN_DF2);

Why not have platform data to register if the DMICs are in use rather than twiddling the GPIOs in an event? It'd be more idiomatic.

#2 lore raw
Quoted 5 lines · Mark Brown
Quoted 3 lines · On Mon, Mar 24, 2014 at 10:16:37AM +0800, [email protected] wrote:

From: Oder Chiou <[email protected]>

This patch adds the RT5639 codec driver.

Quoted 7 lines

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared. What are the issues that prevent the code being shared, there may be something I've missed?

FWIW, I was informed internally (although this certainly isn't normative) that RT5639 is RT5640 plus a DSP, and otherwise compatible. Related, I have a board that has a RT5639 and I'm running the RT5640 driver on it for at least a simple use-case, without issue.

maintainer
#3 lore raw
Quoted 10 lines · Stephen Warren
Quoted 7 lines · Mark Brown

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared. What are the issues that prevent the code being shared, there may be something I've missed?

Quoted 4 lines

FWIW, I was informed internally (although this certainly isn't normative) that RT5639 is RT5640 plus a DSP, and otherwise compatible. Related, I have a board that has a RT5639 and I'm running the RT5640 driver on it for at least a simple use-case, without issue.

OK, it looked to be something like that - in that case the two definitely should be sharing code. Something like the current Wolfson drivers or wm8994 for example, though probably even more sharing is possible if it's just the addition of the DSP.

author maintainer
#4 lore raw
Quoted 8 lines

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:58 AM
To: Stephen Warren
Cc: Oder Chiou; Bard Liao; [email protected]; [email protected]; Flove
Subject: Re: [alsa-devel] [PATCH] ASoC: add RT5639 CODEC driver

Quoted 15 lines
Quoted 14 lines · Stephen Warren
Quoted 7 lines · Mark Brown

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared. What are the issues that prevent the code being shared, there may be something I've missed?

FWIW, I was informed internally (although this certainly isn't normative) that RT5639 is RT5640 plus a DSP, and otherwise compatible. Related, I have a board that has a RT5639 and I'm running the RT5640 driver on it for at least a simple use-case, without issue.

The RT5639 is a simplified version of RT5640. There was only a pair of DACs and there was no MONO out. The RT5639 and RT5640 didn't have the DSP function, and the RT5642 was the RT5640 plus a DSP. In our side, the Tegra4 was broken, and it also was EOL in your company.
So, do you have the code base of Tegra5 which can use for verifying the upsteam driver?

#5 lore raw
Quoted 24 lines · Oder Chiou
Quoted 8 lines

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:58 AM
To: Stephen Warren
Cc: Oder Chiou; Bard Liao; [email protected]; [email protected]; Flove
Subject: Re: [alsa-devel] [PATCH] ASoC: add RT5639 CODEC driver

Quoted 15 lines
Quoted 14 lines · Stephen Warren
Quoted 7 lines · Mark Brown

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared. What are the issues that prevent the code being shared, there may be something I've missed?

FWIW, I was informed internally (although this certainly isn't normative) that RT5639 is RT5640 plus a DSP, and otherwise compatible. Related, I have a board that has a RT5639 and I'm running the RT5640 driver on it for at least a simple use-case, without issue.

Quoted 4 lines

The RT5639 is a simplified version of RT5640. There was only a pair of DACs and there was no MONO out. The RT5639 and RT5640 didn't have the DSP function, and the RT5642 was the RT5640 plus a DSP. In our side, the Tegra4 was broken, and it also was EOL in your company.
So, do you have the code base of Tegra5 which can use for verifying the upsteam driver?

Don't you still have the Dalmore (Tegra114/Tegra4) board that you used last time? While our internal/downstream SW stack deprecates chip SW at a rather annoying pace, upstream Linux still supports everything all the way back to Tegra2/Tegra20, including Dalmore specifically. That has RT5640.

#6 lore raw
In reply to Oder Chiou
Quoted 9 lines · Oder Chiou
Quoted 9 lines

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:58 AM
To: Stephen Warren
Cc: Oder Chiou; Bard Liao; [email protected]; [email protected]; Flove
Subject: Re: [alsa-devel] [PATCH] ASoC: add RT5639 CODEC driver

On Tue, Mar 25, 2014 at 09:33:42AM -0600, Stephen Warren wrote:

Quoted 9 lines
Quoted 4 lines
Quoted 4 lines

FWIW, I was informed internally (although this certainly isn't normative) that RT5639 is RT5640 plus a DSP, and otherwise compatible. Related, I have a board that has a RT5639 and I'm running the RT5640 driver on it for at least a simple use-case, without issue.

The RT5639 is a simplified version of RT5640. There was only a pair of DACs and there was no MONO out. The RT5639 and RT5640 didn't have the DSP function, and the RT5642 was the RT5640 plus a DSP. In our side, the Tegra4 was broken, and it also was EOL in your company.
So, do you have the code base of Tegra5 which can use for verifying the upsteam driver?

FWIW, we are using RT5640 upstream driver on Intel reference designs that have RT5642 so if those DSP features are going to be supported some day I propose to have them implemented in a same driver.

sound/soc/codecs/tlv320aic3x.c comes to my mind how to implement support for different codec variants in the same driver.

Signature · 1 line
Jarkko
maintainer
#7 lore raw
Quoted 3 lines · Jarkko Nikula

FWIW, we are using RT5640 upstream driver on Intel reference designs that have RT5642 so if those DSP features are going to be supported some day I propose to have them implemented in a same driver.

Can we at the very least add the device IDs to the driver so we minimise the chances of boards shipping with the wrong IDs?

Quoted 2 lines

sound/soc/codecs/tlv320aic3x.c comes to my mind how to implement support for different codec variants in the same driver.

Yeah, like I say the wm8994 and Arizona drivers are another example - wm8958 in the wm8994 driver might be particularly useful as it's the addition of a DSP to a simpler device with not much other change.

author maintainer
#8 lore raw
In reply to Mark Brown
Quoted 12 lines

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, March 25, 2014 9:30 PM
To: Oder Chiou
Cc: [email protected]; [email protected]; Flove; Bard Liao
Subject: Re: [PATCH] ASoC: add RT5639 CODEC driver

Quoted 3 lines · On Mon, Mar 24, 2014 at 10:16:37AM +0800, [email protected] wrote:

From: Oder Chiou <[email protected]>

This patch adds the RT5639 codec driver.

Quoted 8 lines

Looking at the level of code similiarity between this and the rt5640 driver I can't help but think that these should be supported from a single driver - a few quick spot checks of the register map suggests that there's at least some overlap. There will need to be some device specific handling but it looks like there's more shared than not shared.
What are the issues that prevent the code being shared, there may be something I've missed?

We will merge the RT5639 driver to RT5640.

Quoted 7 lines
Quoted 7 lines
sound/soc/codecs/rt5639.c new file +7 -0
+static int rt5639_probe(struct snd_soc_codec *codec) {
+	struct rt5639_priv *rt5639 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	rt5639->codec = codec;
+	codec->control_data = rt5639->regmap;
+
Quoted 8 lines

sound/soc/codecs/rt5639.c line 1826

Quoted 5 lines
sound/soc/codecs/rt5639.c new file +5 -0
+	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_REGMAP);
+	if (ret != 0) {
+		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
+		return ret;
+	}

There have been some API changes here this release cycle, you should be able to just remove the cache_io() and regmap setting code here.

Do you mean that the cache_io() can be removed here?
Please tell us which codec driver can be for our reference, thank you.

maintainer
#9 lore raw
Quoted 4 lines · Oder Chiou
Quoted 2 lines

There have been some API changes here this release cycle, you should be able to just remove the cache_io() and regmap setting code here.

Do you mean that the cache_io() can be removed here?

Yes.

Quoted 1 line

Please tell us which codec driver can be for our reference, thank you.

All of them, the API has been changed.