Skip to contentSkip to conversation

ASoC: rt5645: Add codec driver

PATCH Superseded Oder Chiou 2 participants 5 messages Apr 11, 2014 – Apr 22, 2014

Subsystems (3)
Sound ASoC Realtek audio codecs

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Oder Chiou

author maintainer
#0 lore raw

PATCH 1/1 ASoC: rt5645: Add codec driver

This patch adds the Realtek ALC5645 codec driver. It is the base version that because the jack detect function is not implemented to it, the headphone and AMIC1 are not workable. We will fill up the further functions later.

Signed-off-by Bard Liao <[email protected]>
Signed-off-by Oder Chiou <[email protected]>
---
 sound/soc/codecs/Kconfig  |    4 +
 sound/soc/codecs/Makefile |    2 +
 sound/soc/codecs/rt5645.c | 2517 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt5645.h | 2176 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 4699 insertions(+)
 create mode 100644 sound/soc/codecs/rt5645.c
 create mode 100644 sound/soc/codecs/rt5645.h
sound/soc/codecs/Kconfig +4 -0
index f0e8401..8113081 100644
@@ -71,6 +71,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_PCM512x_SPI if SPI_MASTER
 	select SND_SOC_RT5631 if I2C
 	select SND_SOC_RT5640 if I2C
+	select SND_SOC_RT5645 if I2C
 	select SND_SOC_SGTL5000 if I2C
 	select SND_SOC_SI476X if MFD_SI476X_CORE
 	select SND_SOC_SIRF_AUDIO_CODEC

@@ -396,6 +397,9 @@ config SND_SOC_RT5631
 config SND_SOC_RT5640
 	tristate
 
+config SND_SOC_RT5645
+	tristate
+
 #Freescale sgtl5000 codec
 config SND_SOC_SGTL5000
 	tristate "Freescale SGTL5000 CODEC"
sound/soc/codecs/Makefile +2 -0
index 3c4d275..ce05f2a 100644
@@ -60,6 +60,7 @@ snd-soc-pcm512x-i2c-objs := pcm512x-i2c.o
 snd-soc-pcm512x-spi-objs := pcm512x-spi.o
 snd-soc-rt5631-objs := rt5631.o
 snd-soc-rt5640-objs := rt5640.o
+snd-soc-rt5645-objs := rt5645.o
 snd-soc-sgtl5000-objs := sgtl5000.o
 snd-soc-alc5623-objs := alc5623.o
 snd-soc-alc5632-objs := alc5632.o

@@ -211,6 +212,7 @@ obj-$(CONFIG_SND_SOC_PCM512x_I2C)	+= snd-soc-pcm512x-i2c.o
 obj-$(CONFIG_SND_SOC_PCM512x_SPI)	+= snd-soc-pcm512x-spi.o
 obj-$(CONFIG_SND_SOC_RT5631)	+= snd-soc-rt5631.o
 obj-$(CONFIG_SND_SOC_RT5640)	+= snd-soc-rt5640.o
+obj-$(CONFIG_SND_SOC_RT5645)    += snd-soc-rt5645.o
 obj-$(CONFIG_SND_SOC_SGTL5000)  += snd-soc-sgtl5000.o
 obj-$(CONFIG_SND_SOC_SIGMADSP)	+= snd-soc-sigmadsp.o
 obj-$(CONFIG_SND_SOC_SI476X)	+= snd-soc-si476x.o
sound/soc/codecs/rt5645.c new file +2517 -0

2,517 lines not on this page · raw message

sound/soc/codecs/rt5645.h new file +2176 -0

453 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 4 lines · Oder Chiou

This patch adds the Realtek ALC5645 codec driver. It is the base version that because the jack detect function is not implemented to it, the headphone and AMIC1 are not workable. We will fill up the further functions later.

This looks pretty good, a few things below but none of them too big.

sound/soc/codecs/rt5645.c line 258

Quoted 11 lines
sound/soc/codecs/rt5645.c new file +11 -0
+static bool rt5645_readable_register(struct device *dev, unsigned int reg)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(rt5645_ranges); i++)
+		if ((reg >= rt5645_ranges[i].window_start &&
+		     reg <= rt5645_ranges[i].window_start +
+		     rt5645_ranges[i].window_len) ||
+		    (reg >= rt5645_ranges[i].range_min &&
+		     reg <= rt5645_ranges[i].range_max))
+			return true;

Please include some braces in these loops for legibility.

sound/soc/codecs/rt5645.c line 574

Quoted 12 lines
sound/soc/codecs/rt5645.c new file +12 -0
+static int rt5645_set_dmic1_event(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = w->codec;
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		snd_soc_update_bits(codec, RT5645_GPIO_CTRL1,
+			RT5645_GP2_PIN_MASK, RT5645_GP2_PIN_DMIC1_SCL);
+		snd_soc_update_bits(codec, RT5645_DMIC1, RT5645_DMIC_1_DP_MASK,
+			RT5645_DMIC_1_DP_IN2N);
+		break;

For rt5640 we've got this configued by platform data - why not do the same here? In general I'm seeing some similarities again, though I didn't check to see if the same driver can be used yet. Is that the case?

sound/soc/codecs/rt5645.c line 1093

Quoted 5 lines
sound/soc/codecs/rt5645.c new file +5 -0
+static const struct snd_kcontrol_new hp_r_vol_control =
+	SOC_DAPM_SINGLE_AUTODISABLE("Switch", RT5645_HP_VOL,
+
+		RT5645_R_MUTE_SFT, 1, 1);
+static void hp_amp_power(struct snd_soc_codec *codec, int on)

Coding style, your blank line is in the wrong place.

sound/soc/codecs/rt5645.c line 1213

Quoted 13 lines
sound/soc/codecs/rt5645.c new file +13 -0
+static int rt5645_hp_event(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = w->codec;
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		rt5645_pmu_depop(codec);
+		break;
+
+	case SND_SOC_DAPM_PRE_PMD:
+		rt5645_pmd_depop(codec);
+		break;

May as well just inline the power up/down sequences?

sound/soc/codecs/rt5645.c line 1235

Quoted 7 lines
sound/soc/codecs/rt5645.c new file +7 -0
+	case SND_SOC_DAPM_POST_PMU:
+		regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x1c, 0xfd20);
+		regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x20, 0x611f);
+		regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x21, 0x4040);
+		regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x23, 0x0004);
+		snd_soc_update_bits(codec, RT5645_PWR_DIG1,
+			RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |

Either use snd_soc_ or regmap_ - either is fine but be consistent.

sound/soc/codecs/rt5645.c line 1244

Quoted 9 lines
sound/soc/codecs/rt5645.c new file +9 -0
+			RT5645_PWR_CLS_D_L,
+			RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |
+			RT5645_PWR_CLS_D_L);
+		break;
+
+	case SND_SOC_DAPM_PRE_PMD:
+		snd_soc_update_bits(codec, RT5645_PWR_DIG1,
+			RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |
+			RT5645_PWR_CLS_D_L, 0);

It's a bit odd that the first bank of writes done with regmap_write() isn't undone here, can it be done once on init?

sound/soc/codecs/rt5645.c line 1316

Quoted 10 lines
sound/soc/codecs/rt5645.c new file +10 -0
+static int rt5645_pdm1_l_event(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = w->codec;
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		snd_soc_update_bits(codec, RT5645_PDM_OUT_CTRL,
+			RT5645_M_PDM1_L, 0);
+		break;

Why are these done by an event - I'd expect this to be done using the normal DAPM register update support?

sound/soc/codecs/rt5645.c line 1941

Quoted 1 line
sound/soc/codecs/rt5645.c new file +1 -0
+	bclk_ms = frame_size > 32 ? 1 : 0;

bclk_ms = frame_size > 32.

Quoted 9 lines
sound/soc/codecs/rt5645.c new file +9 -0
+static int rt5645_set_bias_level(struct snd_soc_codec *codec,
+			enum snd_soc_bias_level level)
+{
+	switch (level) {
+	case SND_SOC_BIAS_STANDBY:
+		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
+			snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
+				RT5645_PWR_VREF1 | RT5645_PWR_MB |
+				RT5645_PWR_BG | RT5645_PWR_VREF2,

sound/soc/codecs/rt5645.c line 2286

Quoted 10 lines
sound/soc/codecs/rt5645.c new file +10 -0
+				RT5645_PWR_VREF1 | RT5645_PWR_MB |
+				RT5645_PWR_BG | RT5645_PWR_VREF2);
+			mdelay(10);
+			snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
+				RT5645_PWR_FV1 | RT5645_PWR_FV2,
+				RT5645_PWR_FV1 | RT5645_PWR_FV2);
+			snd_soc_update_bits(codec, RT5645_GEN_CTRL1,
+				RT5645_DIG_GATE_CTRL, RT5645_DIG_GATE_CTRL);
+		}
+		break;

Since the device can power up and down very quickly it should make sense to set idle_bias_off for a power saving when idle.

Quoted 5 lines
sound/soc/codecs/rt5645.c new file +5 -0
+#ifdef CONFIG_PM
+static int rt5645_suspend(struct snd_soc_codec *codec)
+{
+	rt5645_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	return 0;

sound/soc/codecs/rt5645.c line 2340

Quoted 10 lines
sound/soc/codecs/rt5645.c new file +10 -0
+}
+
+static int rt5645_resume(struct snd_soc_codec *codec)
+{
+	return 0;
+}
+#else
+#define rt5645_suspend NULL
+#define rt5645_resume NULL
+#endif

If you use idle_bias_off you can remove this.

sound/soc/codecs/rt5645.c line 2438

Quoted 7 lines
sound/soc/codecs/rt5645.c new file +7 -0
+#if defined(CONFIG_OF)
+static const struct of_device_id rt5645_of_match[] = {
+	{ .compatible = "realtek,rt5645", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rt5645_of_match);
+#endif

We need device tree binding documentation for any device with DT bindings.

sound/soc/codecs/rt5645.c line 2491

Quoted 6 lines
sound/soc/codecs/rt5645.c new file +6 -0
+static int rt5645_i2c_remove(struct i2c_client *i2c)
+{
+	snd_soc_unregister_codec(&i2c->dev);
+	kfree(i2c_get_clientdata(i2c));
+	return 0;
+}

You used devm_kzalloc(), no need to free.

sound/soc/codecs/rt5645.c line 2500

Quoted 8 lines
sound/soc/codecs/rt5645.c new file +8 -0
+void rt5645_i2c_shutdown(struct i2c_client *client)
+{
+	struct rt5645_priv *rt5645 = i2c_get_clientdata(client);
+	struct snd_soc_codec *codec = rt5645->codec;
+
+	if (codec != NULL)
+		rt5645_set_bias_level(codec, SND_SOC_BIAS_OFF);
+}

The ASoC framework will do this for you.

author maintainer
#2 lore raw
Quoted 19 lines
Quoted 8 lines
sound/soc/codecs/rt5645.c new file +7 -0
+static int rt5645_set_bias_level(struct snd_soc_codec *codec,
+                     enum snd_soc_bias_level level)
+{
+     switch (level) {
+     case SND_SOC_BIAS_STANDBY:
+             if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
+                     snd_soc_update_bits(codec, RT5645_PWR_ANLG1,

sound/soc/codecs/rt5645.c

Quoted 10 lines
sound/soc/codecs/rt5645.c new file +10 -0
+                             RT5645_PWR_VREF1 | RT5645_PWR_MB |
+                             RT5645_PWR_BG | RT5645_PWR_VREF2,
+                             RT5645_PWR_VREF1 | RT5645_PWR_MB |
+                             RT5645_PWR_BG | RT5645_PWR_VREF2);
+                     mdelay(10);
+                     snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
+                             RT5645_PWR_FV1 | RT5645_PWR_FV2,
+                             RT5645_PWR_FV1 | RT5645_PWR_FV2);
+                     snd_soc_update_bits(codec, RT5645_GEN_CTRL1,
+                             RT5645_DIG_GATE_CTRL,

RT5645_DIG_GATE_CTRL);

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+             }
+             break;

Since the device can power up and down very quickly it should make sense to set idle_bias_off for a power saving when idle.

We've set the idle_bias_off as true in struct "soc_codec_dev_rt5645", is there any thing i miss it?

author maintainer
#3 lore raw
In reply to Mark Brown
Quoted 14 lines
Quoted 14 lines
sound/soc/codecs/rt5645.c new file +14 -0
+static int rt5645_set_bias_level(struct snd_soc_codec *codec,
+                     enum snd_soc_bias_level level)
+{
+     switch (level) {
+     case SND_SOC_BIAS_STANDBY:
+             if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
+                     snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
+                             RT5645_PWR_VREF1 | RT5645_PWR_MB |
+                             RT5645_PWR_BG | RT5645_PWR_VREF2,
+                             RT5645_PWR_VREF1 | RT5645_PWR_MB |
+                             RT5645_PWR_BG | RT5645_PWR_VREF2);
+                     mdelay(10);
+                     snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
+                             RT5645_PWR_FV1 | RT5645_PWR_FV2,
Quoted 9 lines

sound/soc/codecs/rt5645.c line 2286

Quoted 5 lines
sound/soc/codecs/rt5645.c new file +5 -0
+                             RT5645_PWR_FV1 | RT5645_PWR_FV2);
+                     snd_soc_update_bits(codec, RT5645_GEN_CTRL1,
+                             RT5645_DIG_GATE_CTRL, RT5645_DIG_GATE_CTRL);
+             }
+             break;

Since the device can power up and down very quickly it should make sense to set idle_bias_off for a power saving when idle.

We've set the idle_bias_off as true in struct "soc_codec_dev_rt5645", is there any thing i miss it?

maintainer
#4 lore raw
Quoted 5 lines · Oder
Quoted 2 lines

Since the device can power up and down very quickly it should make sense to set idle_bias_off for a power saving when idle.

We've set the idle_bias_off as true in struct "soc_codec_dev_rt5645", is there any thing i miss it?

No, nothing - sorry, I didn't notice that.