diff mbox

ASoC: rt5640: Use the platform data for DMIC settings

Message ID 1395974778-4217-1-git-send-email-oder_chiou@realtek.com (mailing list archive)
State Accepted
Commit 71d97a7943017faf03707836d00a260a108f4c89
Headers show

Commit Message

Oder Chiou March 28, 2014, 2:46 a.m. UTC 
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(-)

Comments

Mark Brown March 28, 2014, 11:02 a.m. UTC | #1 
On Fri, Mar 28, 2014 at 10:46:18AM +0800, Oder Chiou wrote:
> The patch uses the platform data for DMIC settings.

Applied, thanks.
Stephen Warren March 28, 2014, 4:05 p.m. UTC | #2 
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?
Mark Brown March 28, 2014, 11:46 p.m. UTC | #3 
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.
Stephen Warren March 31, 2014, 3:45 p.m. UTC | #4 
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().
Mark Brown March 31, 2014, 4:05 p.m. UTC | #5 
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.
Stephen Warren March 31, 2014, 4:37 p.m. UTC | #6 
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.
Mark Brown March 31, 2014, 5:26 p.m. UTC | #7 
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.
diff mbox

Patch

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,