[PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Oder Chiou]

The patch uses the platform data for DMIC settings.

Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
---
 include/sound/rt5640.h    |  4 +++
 sound/soc/codecs/rt5640.c | 77 ++++++++++++++---------------------------------
 2 files changed, 27 insertions(+), 54 deletions(-)

diff --git a/include/sound/rt5640.h b/include/sound/rt5640.h
index 27cc75e..59d26dd 100644
--- a/include/sound/rt5640.h
+++ b/include/sound/rt5640.h
@@ -16,6 +16,10 @@ struct rt5640_platform_data {
 	bool in1_diff;
 	bool in2_diff;
 
+	bool dmic_en;
+	bool dmic1_data_pin; /* 0 = IN1P; 1 = GPIO3 */
+	bool dmic2_data_pin; /* 0 = IN1N; 1 = GPIO4 */
+
 	int ldo1_en; /* GPIO for LDO1_EN */
 };
 
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index bcd6513..cf041e3 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -872,54 +872,6 @@ static SOC_ENUM_SINGLE_DECL(rt5640_sdi_sel_enum, RT5640_I2S2_SDP,
 static const struct snd_kcontrol_new rt5640_sdi_mux =
 	SOC_DAPM_ENUM("SDI select", rt5640_sdi_sel_enum);
 
-static int rt5640_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, RT5640_GPIO_CTRL1,
-			RT5640_GP2_PIN_MASK | RT5640_GP3_PIN_MASK,
-			RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP3_PIN_DMIC1_SDA);
-		snd_soc_update_bits(codec, RT5640_DMIC,
-			RT5640_DMIC_1L_LH_MASK | RT5640_DMIC_1R_LH_MASK |
-			RT5640_DMIC_1_DP_MASK,
-			RT5640_DMIC_1L_LH_FALLING | RT5640_DMIC_1R_LH_RISING |
-			RT5640_DMIC_1_DP_IN1P);
-		break;
-
-	default:
-		return 0;
-	}
-
-	return 0;
-}
-
-static int rt5640_set_dmic2_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, RT5640_GPIO_CTRL1,
-			RT5640_GP2_PIN_MASK | RT5640_GP4_PIN_MASK,
-			RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP4_PIN_DMIC2_SDA);
-		snd_soc_update_bits(codec, RT5640_DMIC,
-			RT5640_DMIC_2L_LH_MASK | RT5640_DMIC_2R_LH_MASK |
-			RT5640_DMIC_2_DP_MASK,
-			RT5640_DMIC_2L_LH_FALLING | RT5640_DMIC_2R_LH_RISING |
-			RT5640_DMIC_2_DP_IN1N);
-		break;
-
-	default:
-		return 0;
-	}
-
-	return 0;
-}
-
 static void hp_amp_power_on(struct snd_soc_codec *codec)
 {
 	struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec);
@@ -1054,12 +1006,10 @@ static const struct snd_soc_dapm_widget rt5640_dapm_widgets[] = {
 
 	SND_SOC_DAPM_SUPPLY("DMIC CLK", SND_SOC_NOPM, 0, 0,
 		set_dmic_clk, SND_SOC_DAPM_PRE_PMU),
-	SND_SOC_DAPM_SUPPLY("DMIC1 Power", RT5640_DMIC,
-		RT5640_DMIC_1_EN_SFT, 0, rt5640_set_dmic1_event,
-		SND_SOC_DAPM_PRE_PMU),
-	SND_SOC_DAPM_SUPPLY("DMIC2 Power", RT5640_DMIC,
-		RT5640_DMIC_2_EN_SFT, 0, rt5640_set_dmic2_event,
-		SND_SOC_DAPM_PRE_PMU),
+	SND_SOC_DAPM_SUPPLY("DMIC1 Power", RT5640_DMIC, RT5640_DMIC_1_EN_SFT, 0,
+		NULL, 0),
+	SND_SOC_DAPM_SUPPLY("DMIC2 Power", RT5640_DMIC, RT5640_DMIC_2_EN_SFT, 0,
+		NULL, 0),
 	/* Boost */
 	SND_SOC_DAPM_PGA("BST1", RT5640_PWR_ANLG2,
 		RT5640_PWR_BST1_BIT, 0, NULL, 0),
@@ -2179,6 +2129,25 @@ static int rt5640_i2c_probe(struct i2c_client *i2c,
 		regmap_update_bits(rt5640->regmap, RT5640_IN3_IN4,
 					RT5640_IN_DF2, RT5640_IN_DF2);
 
+	if (rt5640->pdata.dmic_en) {
+		regmap_update_bits(rt5640->regmap, RT5640_GPIO_CTRL1,
+			RT5640_GP2_PIN_MASK, RT5640_GP2_PIN_DMIC1_SCL);
+
+		if (rt5640->pdata.dmic1_data_pin) {
+			regmap_update_bits(rt5640->regmap, RT5640_DMIC,
+				RT5640_DMIC_1_DP_MASK, RT5640_DMIC_1_DP_GPIO3);
+			regmap_update_bits(rt5640->regmap, RT5640_GPIO_CTRL1,
+				RT5640_GP3_PIN_MASK, RT5640_GP3_PIN_DMIC1_SDA);
+		}
+
+		if (rt5640->pdata.dmic2_data_pin) {
+			regmap_update_bits(rt5640->regmap, RT5640_DMIC,
+				RT5640_DMIC_2_DP_MASK, RT5640_DMIC_2_DP_GPIO4);
+			regmap_update_bits(rt5640->regmap, RT5640_GPIO_CTRL1,
+				RT5640_GP4_PIN_MASK, RT5640_GP4_PIN_DMIC2_SDA);
+		}
+	}
+
 	rt5640->hp_mute = 1;
 
 	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5640,
-- 
1.8.1.1.439.g50a6b54

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Mark Brown]

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

On Fri, Mar 28, 2014 at 10:46:18AM +0800, Oder Chiou wrote:
> The patch uses the platform data for DMIC settings.

Applied, thanks.

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

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

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Stephen Warren]

On 03/27/2014 08:46 PM, Oder Chiou wrote:
> The patch uses the platform data for DMIC settings.

Shouldn't this also support parsing the same information from device tree?

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Mark Brown]

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

On Fri, Mar 28, 2014 at 10:05:49AM -0600, Stephen Warren wrote:
> On 03/27/2014 08:46 PM, Oder Chiou wrote:

> > The patch uses the platform data for DMIC settings.

> Shouldn't this also support parsing the same information from device tree?

The driver has no DT support at all at the minute but if it's being used
on platforms using DT (which of course it is now I think about it - I
just looked for the DT support when reviewing) then yes it should.

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

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

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Stephen Warren]

On 03/28/2014 05:46 PM, Mark Brown wrote:
> On Fri, Mar 28, 2014 at 10:05:49AM -0600, Stephen Warren wrote:
>> On 03/27/2014 08:46 PM, Oder Chiou wrote:
> 
>>> The patch uses the platform data for DMIC settings.
> 
>> Shouldn't this also support parsing the same information from device tree?
> 
> The driver has no DT support at all at the minute but if it's being used
> on platforms using DT (which of course it is now I think about it - I
> just looked for the DT support when reviewing) then yes it should.

The driver doesn't have an OF match table (I'll send a patch to fix that
soon), but certainly does support DT; see rt5640_parse_dt().

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Mark Brown]

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

On Mon, Mar 31, 2014 at 09:45:01AM -0600, Stephen Warren wrote:
> On 03/28/2014 05:46 PM, Mark Brown wrote:

> > The driver has no DT support at all at the minute but if it's being used
> > on platforms using DT (which of course it is now I think about it - I
> > just looked for the DT support when reviewing) then yes it should.

> The driver doesn't have an OF match table (I'll send a patch to fix that
> soon), but certainly does support DT; see rt5640_parse_dt().

Oh, dear.  That's not clever and we do need the IDs adding, that's the
baseline thing needed for DT support.

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

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

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Stephen Warren]

On 03/31/2014 10:05 AM, Mark Brown wrote:
> On Mon, Mar 31, 2014 at 09:45:01AM -0600, Stephen Warren wrote:
>> On 03/28/2014 05:46 PM, Mark Brown wrote:
> 
>>> The driver has no DT support at all at the minute but if it's being used
>>> on platforms using DT (which of course it is now I think about it - I
>>> just looked for the DT support when reviewing) then yes it should.
> 
>> The driver doesn't have an OF match table (I'll send a patch to fix that
>> soon), but certainly does support DT; see rt5640_parse_dt().
> 
> Oh, dear.  That's not clever and we do need the IDs adding, that's the
> baseline thing needed for DT support.

I really wish we would make up our minds about this.

For I2C (and SPI and perhaps others) the I2C match table works fine as a
replacement for the of_match table. The only issue might be different
manufacturers with the same chip names. If this is a problem, why is
fallback to the I2C match table even allowed any more; we should mandate
that OF matching only works via the OF match table.

When DT was young, Grant tried to require of_match for everything for
completeness, and then I tried enforcing that for reviews, and then
Grant said not to bother with that, so I stopped, and now you're saying
it's required again. I really wish I could get consistency in how this
kind of thing is supposed to work. It's difficult for contributors to
know what to do if reviewers keep flip-flopping over time.

Re: [PATCH] ASoC: rt5640: Use the platform data for DMIC settings

[Mark Brown]

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

On Mon, Mar 31, 2014 at 10:37:39AM -0600, Stephen Warren wrote:

> I really wish we would make up our minds about this.

> For I2C (and SPI and perhaps others) the I2C match table works fine as a
> replacement for the of_match table. The only issue might be different
> manufacturers with the same chip names. If this is a problem, why is
> fallback to the I2C match table even allowed any more; we should mandate
> that OF matching only works via the OF match table.

> When DT was young, Grant tried to require of_match for everything for
> completeness, and then I tried enforcing that for reviews, and then
> Grant said not to bother with that, so I stopped, and now you're saying
> it's required again. I really wish I could get consistency in how this
> kind of thing is supposed to work. It's difficult for contributors to
> know what to do if reviewers keep flip-flopping over time.

Well, *I've* not been flip flopping on this, frankly I was unaware that
anyone thought it was a particularly good idea to actively not include
the match table.  It's true that as a matter of practicality you don't
need to bother at the minute but I think especially once you're adding
any explicit code at all to the driver the explicit match strings ought
to be there too.

I suspect this may have been a pragmatic suggestion due to all the
complaints about churn generated by DT.

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

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