[PATCH 1/3] ASoC: rt5645: fix add missing widget

[PATCH 1/3] ASoC: rt5645: fix add missing widget

[Bard Liao]

"IF1 DAC0" and "IF1 DAC3" are used in rt5645_dapm_routes but missing
in rt5645_dapm_widgets.

Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
Signed-off-by: Bard Liao <bardliao@realtek.com>
---
 sound/soc/codecs/rt5645.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 7996c9c..a72d989 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -1697,8 +1697,10 @@ static const struct snd_soc_dapm_widget rt5645_dapm_widgets[] = {
 	/* Digital Interface */
 	SND_SOC_DAPM_SUPPLY("I2S1", RT5645_PWR_DIG1,
 		RT5645_PWR_I2S1_BIT, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("IF1 DAC0", SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_PGA("IF1 DAC1", SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_PGA("IF1 DAC2", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("IF1 DAC3", SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_MUX("RT5645 IF1 DAC1 L Mux", SND_SOC_NOPM, 0, 0,
 		&rt5645_if1_dac0_tdm_sel_mux),
 	SND_SOC_DAPM_MUX("RT5645 IF1 DAC1 R Mux", SND_SOC_NOPM, 0, 0,
-- 
1.8.1.1.439.g50a6b54

[PATCH 2/3] ASoC: rt5645: remove unused field in pdata

[Bard Liao]

We can know if dmic is used by reading the value of dmic1_data_pin
and dmic2_data_pin. Also IRQ must be used if codec JD or button
detection function is used. So, dmic_en and en_jd_func can be remove
from platform data.

Signed-off-by: Bard Liao <bardliao@realtek.com>
---
 include/sound/rt5645.h    |   3 --
 sound/soc/codecs/rt5645.c | 124 ++++++++++++++++++++++------------------------
 sound/soc/codecs/rt5645.h |   2 +
 3 files changed, 61 insertions(+), 68 deletions(-)

diff --git a/include/sound/rt5645.h b/include/sound/rt5645.h
index 120d961..652cb9e 100644
--- a/include/sound/rt5645.h
+++ b/include/sound/rt5645.h
@@ -15,7 +15,6 @@ struct rt5645_platform_data {
 	/* IN2 can optionally be differential */
 	bool in2_diff;
 
-	bool dmic_en;
 	unsigned int dmic1_data_pin;
 	/* 0 = IN2N; 1 = GPIO5; 2 = GPIO11 */
 	unsigned int dmic2_data_pin;
@@ -24,8 +23,6 @@ struct rt5645_platform_data {
 	unsigned int hp_det_gpio;
 	bool gpio_hp_det_active_high;
 
-	/* true if codec's jd function is used */
-	bool en_jd_func;
 	unsigned int jd_mode;
 };
 
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index a72d989..e435680 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -2968,7 +2968,7 @@ static int rt5645_probe(struct snd_soc_codec *codec)
 	snd_soc_update_bits(codec, RT5645_CHARGE_PUMP, 0x0300, 0x0200);
 
 	/* for JD function */
-	if (rt5645->pdata.en_jd_func) {
+	if (rt5645->pdata.jd_mode) {
 		snd_soc_dapm_force_enable_pin(&codec->dapm, "JD Power");
 		snd_soc_dapm_force_enable_pin(&codec->dapm, "LDO2");
 		snd_soc_dapm_sync(&codec->dapm);
@@ -3111,10 +3111,8 @@ MODULE_DEVICE_TABLE(acpi, rt5645_acpi_match);
 static struct rt5645_platform_data *rt5645_pdata;
 
 static struct rt5645_platform_data strago_platform_data = {
-	.dmic_en = true,
-	.dmic1_data_pin = -1,
+	.dmic1_data_pin = RT5645_DMIC1_DISABLE,
 	.dmic2_data_pin = RT5645_DMIC_DATA_IN2P,
-	.en_jd_func = true,
 	.jd_mode = 3,
 };
 
@@ -3214,83 +3212,79 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 		regmap_update_bits(rt5645->regmap, RT5645_IN2_CTRL,
 					RT5645_IN_DF2, RT5645_IN_DF2);
 
-	if (rt5645->pdata.dmic_en) {
+	if (rt5645->pdata.dmic1_data_pin || rt5645->pdata.dmic2_data_pin) {
 		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
 			RT5645_GP2_PIN_MASK, RT5645_GP2_PIN_DMIC1_SCL);
+	}
+	switch (rt5645->pdata.dmic1_data_pin) {
+	case RT5645_DMIC_DATA_IN2N:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_IN2N);
+		break;
 
-		switch (rt5645->pdata.dmic1_data_pin) {
-		case RT5645_DMIC_DATA_IN2N:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_IN2N);
-			break;
-
-		case RT5645_DMIC_DATA_GPIO5:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO5);
-			regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
-				RT5645_GP5_PIN_MASK, RT5645_GP5_PIN_DMIC1_SDA);
-			break;
-
-		case RT5645_DMIC_DATA_GPIO11:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO11);
-			regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
-				RT5645_GP11_PIN_MASK,
-				RT5645_GP11_PIN_DMIC1_SDA);
-			break;
+	case RT5645_DMIC_DATA_GPIO5:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO5);
+		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
+			RT5645_GP5_PIN_MASK, RT5645_GP5_PIN_DMIC1_SDA);
+		break;
 
-		default:
-			break;
-		}
+	case RT5645_DMIC_DATA_GPIO11:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO11);
+		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
+			RT5645_GP11_PIN_MASK,
+			RT5645_GP11_PIN_DMIC1_SDA);
+		break;
 
-		switch (rt5645->pdata.dmic2_data_pin) {
-		case RT5645_DMIC_DATA_IN2P:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_IN2P);
-			break;
+	default:
+		break;
+	}
 
-		case RT5645_DMIC_DATA_GPIO6:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO6);
-			regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
-				RT5645_GP6_PIN_MASK, RT5645_GP6_PIN_DMIC2_SDA);
-			break;
+	switch (rt5645->pdata.dmic2_data_pin) {
+	case RT5645_DMIC_DATA_IN2P:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_IN2P);
+		break;
 
-		case RT5645_DMIC_DATA_GPIO10:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO10);
-			regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
-				RT5645_GP10_PIN_MASK,
-				RT5645_GP10_PIN_DMIC2_SDA);
-			break;
+	case RT5645_DMIC_DATA_GPIO6:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO6);
+		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
+			RT5645_GP6_PIN_MASK, RT5645_GP6_PIN_DMIC2_SDA);
+		break;
 
-		case RT5645_DMIC_DATA_GPIO12:
-			regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
-				RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO12);
-			regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
-				RT5645_GP12_PIN_MASK,
-				RT5645_GP12_PIN_DMIC2_SDA);
-			break;
+	case RT5645_DMIC_DATA_GPIO10:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO10);
+		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
+			RT5645_GP10_PIN_MASK,
+			RT5645_GP10_PIN_DMIC2_SDA);
+		break;
 
-		default:
-			break;
-		}
+	case RT5645_DMIC_DATA_GPIO12:
+		regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1,
+			RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO12);
+		regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1,
+			RT5645_GP12_PIN_MASK,
+			RT5645_GP12_PIN_DMIC2_SDA);
+		break;
 
+	default:
+		break;
 	}
 
-	if (rt5645->pdata.en_jd_func) {
+	if (rt5645->pdata.jd_mode) {
 		regmap_update_bits(rt5645->regmap, RT5645_GEN_CTRL3,
-			RT5645_IRQ_CLK_GATE_CTRL, RT5645_IRQ_CLK_GATE_CTRL);
+				   RT5645_IRQ_CLK_GATE_CTRL,
+				   RT5645_IRQ_CLK_GATE_CTRL);
 		regmap_update_bits(rt5645->regmap, RT5645_IN1_CTRL1,
-			RT5645_CBJ_BST1_EN, RT5645_CBJ_BST1_EN);
+				   RT5645_CBJ_BST1_EN, RT5645_CBJ_BST1_EN);
 		regmap_update_bits(rt5645->regmap, RT5645_JD_CTRL3,
-			RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL,
-			RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL);
+				   RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL,
+				   RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL);
 		regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
-			RT5645_IRQ_CLK_INT, RT5645_IRQ_CLK_INT);
-	}
-
-	if (rt5645->pdata.jd_mode) {
+				   RT5645_IRQ_CLK_INT, RT5645_IRQ_CLK_INT);
 		regmap_update_bits(rt5645->regmap, RT5645_IRQ_CTRL2,
 				   RT5645_IRQ_JD_1_1_EN, RT5645_IRQ_JD_1_1_EN);
 		regmap_update_bits(rt5645->regmap, RT5645_GEN_CTRL3,
diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h
index c204861..9ec4e89 100644
--- a/sound/soc/codecs/rt5645.h
+++ b/sound/soc/codecs/rt5645.h
@@ -2145,6 +2145,7 @@ enum {
 };
 
 enum {
+	RT5645_DMIC1_DISABLE,
 	RT5645_DMIC_DATA_IN2P,
 	RT5645_DMIC_DATA_GPIO6,
 	RT5645_DMIC_DATA_GPIO10,
@@ -2152,6 +2153,7 @@ enum {
 };
 
 enum {
+	RT5645_DMIC2_DISABLE,
 	RT5645_DMIC_DATA_IN2N,
 	RT5645_DMIC_DATA_GPIO5,
 	RT5645_DMIC_DATA_GPIO11,
-- 
1.8.1.1.439.g50a6b54

[PATCH 3/3] ASoC: rt5645: add device tree support

[Bard Liao]

Modify the RT5645 driver to parse platform data from device tree.
Write a DT binding document to describe those properties.

Signed-off-by: Bard Liao <bardliao@realtek.com>
---
 Documentation/devicetree/bindings/sound/rt5645.txt | 72 ++++++++++++++++++++++
 sound/soc/codecs/rt5645.c                          | 35 +++++++++++
 2 files changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/rt5645.txt

diff --git a/Documentation/devicetree/bindings/sound/rt5645.txt b/Documentation/devicetree/bindings/sound/rt5645.txt
new file mode 100644
index 0000000..f676c22
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rt5645.txt
@@ -0,0 +1,72 @@
+RT5650/RT5645 audio CODEC
+
+This device supports I2C only.
+
+Required properties:
+
+- compatible : One of "realtek,rt5645" or "realtek,rt5650".
+
+- reg : The I2C address of the device.
+
+- interrupts : The CODEC's interrupt output.
+
+Optional properties:
+
+- realtek,in2-differential
+  Boolean. Indicate MIC2 input are differential, rather than single-ended.
+
+- realtek,dmic1-data-pin
+  0: dmic1 is not used
+  1: using IN2P pin as dmic1 data pin
+  2: using GPIO6 pin as dmic1 data pin
+  3: using GPIO10 pin as dmic1 data pin
+  4: using GPIO12 pin as dmic1 data pin
+
+- realtek,dmic2-data-pin
+  0: dmic2 is not used
+  1: using IN2N pin as dmic2 data pin
+  2: using GPIO5 pin as dmic2 data pin
+  3: using GPIO11 pin as dmic2 data pin
+
+- realtek,hp-det-gpio : The GPIO that indicate a JD event is triggered.
+
+- realtek,gpio-hp_det-active_high
+  Boolean. Indicate if hp-det-gpio is active high or not.
+
+-- realtek,jd-mode : The JD mode of rt5645/rt5650
+   0 : rt5645/rt5650 JD function is not used
+   1 : Mode-0 (VDD=3.3V), two port jack detection
+   2 : Mode-1 (VDD=3.3V), one port jack detection
+   3 : Mode-2 (VDD=1.8V), one port jack detection
+
+Pins on the device (for linking into audio routes) for RT5645/RT5650:
+
+  * DMIC L1
+  * DMIC R1
+  * DMIC L2
+  * DMIC R2
+  * IN1P
+  * IN1N
+  * IN2P
+  * IN2N
+  * Haptic Generator
+  * HPOL
+  * HPOR
+  * LOUTL
+  * LOUTR
+  * PDM1L
+  * PDM1R
+  * SPOL
+  * SPOR
+
+Example:
+
+codec: rt5650@1a {
+	compatible = "realtek,rt5650";
+	reg = <0x1a>;
+	interrupt-parent = <&gpio>;
+	interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+	realtek,dmic-en = "true";
+	realtek,en-jd-func = "true";
+	realtek,jd-mode = <3>;
+};
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index e435680..36e22b6 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -3134,6 +3134,37 @@ static struct dmi_system_id dmi_platform_intel_braswell[] = {
 	{ }
 };
 
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device_node *np)
+{
+	rt5645->pdata.in2_diff = of_property_read_bool(np,
+		"realtek,in2-differential");
+	of_property_read_u32(np,
+		"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);
+	of_property_read_u32(np,
+		"realtek,dmic2-data-pin", &rt5645->pdata.dmic2_data_pin);
+	rt5645->pdata.hp_det_gpio = of_get_named_gpio(np,
+		"realtek,hp-det-gpio", 0);
+	rt5645->pdata.gpio_hp_det_active_high = of_property_read_bool(np,
+		"realtek,gpio-hp_det-active_high");
+	of_property_read_u32(np,
+		"realtek,jd-mode", &rt5645->pdata.jd_mode);
+	/*
+	 * HP_DET_GPIO is optional (it may be statically tied on the board).
+	 * -ENOENT means that the property doesn't exist, i.e. there is no
+	 * GPIO, so is not an error. Any other error code means the property
+	 * exists, but could not be parsed.
+	 */
+
+	if (!rt5645->pdata.hp_det_gpio)
+			rt5645->pdata.hp_det_gpio = -EINVAL;
+
+	if (!gpio_is_valid(rt5645->pdata.hp_det_gpio) &&
+			(rt5645->pdata.hp_det_gpio != -ENOENT))
+		return rt5645->pdata.hp_det_gpio;
+
+	return 0;
+}
+
 static int rt5645_i2c_probe(struct i2c_client *i2c,
 		    const struct i2c_device_id *id)
 {
@@ -3153,6 +3184,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 
 	if (pdata) {
 		rt5645->pdata = *pdata;
+	} else if (i2c->dev.of_node) {
+		ret = rt5645_parse_dt(rt5645, i2c->dev.of_node);
+		if (ret)
+			return ret;
 	} else {
 		if (dmi_check_system(dmi_platform_intel_braswell)) {
 			rt5645->pdata = *rt5645_pdata;
-- 
1.8.1.1.439.g50a6b54

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Liam Girdwood]

On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
> Modify the RT5645 driver to parse platform data from device tree.
> Write a DT binding document to describe those properties.
> 
> Signed-off-by: Bard Liao <bardliao@realtek.com>
> ---
>  Documentation/devicetree/bindings/sound/rt5645.txt | 72 ++++++++++++++++++++++
>  sound/soc/codecs/rt5645.c                          | 35 +++++++++++
>  2 files changed, 107 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/rt5645.txt
> 

> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index e435680..36e22b6 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -3134,6 +3134,37 @@ static struct dmi_system_id dmi_platform_intel_braswell[] = {
>  	{ }
>  };
>  
> +static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device_node *np)
> +{
> +	rt5645->pdata.in2_diff = of_property_read_bool(np,
> +		"realtek,in2-differential");
> +	of_property_read_u32(np,
> +		"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);

We should really be using device_property_() instead of of_property_()
APIs since we will have to support both DT and ACPI properties.

Liam

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Bard Liao]

> -----Original Message-----
> From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com]
> Sent: Tuesday, May 05, 2015 10:46 PM
> To: Bard Liao
> Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou;
> alsa-devel@alsa-project.org; lars@metafoo.de;
> zhengxing@rock-chips.com; yang.a.fang@intel.com;
> koro.chen@mediatek.com; John Lin; Flove
> Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree
> support
> 
> On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
> > Modify the RT5645 driver to parse platform data from device tree.
> > Write a DT binding document to describe those properties.
> >
> > Signed-off-by: Bard Liao <bardliao@realtek.com>
> > ---
> >  Documentation/devicetree/bindings/sound/rt5645.txt | 72
> ++++++++++++++++++++++
> >  sound/soc/codecs/rt5645.c                          | 35
> +++++++++++
> >  2 files changed, 107 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/sound/rt5645.txt
> >
> 
> > diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> > index e435680..36e22b6 100644
> > --- a/sound/soc/codecs/rt5645.c
> > +++ b/sound/soc/codecs/rt5645.c
> > @@ -3134,6 +3134,37 @@ static struct dmi_system_id
> dmi_platform_intel_braswell[] = {
> >  	{ }
> >  };
> >
> > +static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct
> > +device_node *np) {
> > +	rt5645->pdata.in2_diff = of_property_read_bool(np,
> > +		"realtek,in2-differential");
> > +	of_property_read_u32(np,
> > +		"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);
> 
> We should really be using device_property_() instead of of_property_()
> APIs since we will have to support both DT and ACPI properties.

We will modify it and send a new version patch for it.

Thanks.

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

Re: [PATCH 1/3] ASoC: rt5645: fix add missing widget

[Mark Brown]

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

On Tue, May 05, 2015 at 09:42:00PM +0800, Bard Liao wrote:
> "IF1 DAC0" and "IF1 DAC3" are used in rt5645_dapm_routes but missing
> in rt5645_dapm_widgets.

Applied, thanks.

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

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

Re: [PATCH 2/3] ASoC: rt5645: remove unused field in pdata

[Mark Brown]

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

On Tue, May 05, 2015 at 09:42:01PM +0800, Bard Liao wrote:
> We can know if dmic is used by reading the value of dmic1_data_pin
> and dmic2_data_pin. Also IRQ must be used if codec JD or button
> detection function is used. So, dmic_en and en_jd_func can be remove
> from platform data.

Applied, thanks.

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

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

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Bard Liao]

> -----Original Message-----
> From: Bard Liao
> Sent: Wednesday, May 06, 2015 2:43 PM
> To: 'Liam Girdwood'
> Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou;
> alsa-devel@alsa-project.org; lars@metafoo.de;
> zhengxing@rock-chips.com; yang.a.fang@intel.com;
> koro.chen@mediatek.com; John Lin; Flove
> Subject: RE: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree
> support
> 
> > -----Original Message-----
> > From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com]
> > Sent: Tuesday, May 05, 2015 10:46 PM
> > To: Bard Liao
> > Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou;
> > alsa-devel@alsa-project.org; lars@metafoo.de;
> > zhengxing@rock-chips.com; yang.a.fang@intel.com;
> > koro.chen@mediatek.com; John Lin; Flove
> > Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree
> > support
> >
> > On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
> > > Modify the RT5645 driver to parse platform data from device tree.
> > > Write a DT binding document to describe those properties.
> > >
> > > Signed-off-by: Bard Liao <bardliao@realtek.com>
> > > ---
> > >  Documentation/devicetree/bindings/sound/rt5645.txt | 72
> > ++++++++++++++++++++++
> > >  sound/soc/codecs/rt5645.c                          | 35
> > +++++++++++
> > >  2 files changed, 107 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/sound/rt5645.txt
> > >
> >
> > > diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> > > index e435680..36e22b6 100644
> > > --- a/sound/soc/codecs/rt5645.c
> > > +++ b/sound/soc/codecs/rt5645.c
> > > @@ -3134,6 +3134,37 @@ static struct dmi_system_id
> > dmi_platform_intel_braswell[] = {
> > >  	{ }
> > >  };
> > >
> > > +static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct
> > > +device_node *np) {
> > > +	rt5645->pdata.in2_diff = of_property_read_bool(np,
> > > +		"realtek,in2-differential");
> > > +	of_property_read_u32(np,
> > > +		"realtek,dmic1-data-pin",
> &rt5645->pdata.dmic1_data_pin);
> >
> > We should really be using device_property_() instead of of_property_()
> > APIs since we will have to support both DT and ACPI properties.

Unfortunately, I can't find a way to test it.
Is that ok we just replace all of_property_ with device_property_?
Also, is there any corresponding API for of_get_named_gpio?
Or we can replace it with device_property_read_u32?
I tried the change above, and it can build. However I don't know
if it can work.

> 
> We will modify it and send a new version patch for it.
> 
> Thanks.
> 
> >
> > Liam
> >
> >
> > ------Please consider the environment before printing this e-mail.

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Liam Girdwood]

On Thu, 2015-05-07 at 05:34 +0000, Bard Liao wrote:
> > > > +static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct
> > > > +device_node *np) {
> > > > + rt5645->pdata.in2_diff = of_property_read_bool(np,
> > > > +         "realtek,in2-differential");
> > > > + of_property_read_u32(np,
> > > > +         "realtek,dmic1-data-pin",
> > &rt5645->pdata.dmic1_data_pin);
> > >
> > > We should really be using device_property_() instead of
> of_property_()
> > > APIs since we will have to support both DT and ACPI properties.
> 
> Unfortunately, I can't find a way to test it.

device_property() API calls abstract the device tree calls and ACPI
calls. So testing with your DT based HW will work.

> Is that ok we just replace all of_property_ with device_property_?

Yes, that's the intention.

> Also, is there any corresponding API for of_get_named_gpio?
> Or we can replace it with device_property_read_u32?
> I tried the change above, and it can build. However I don't know
> if it can work.

Oh, I think that's a question for Rafael. I think the intention is to
have a 1:1 mapping between the APIs so that there are no gaps ?

Liam

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Mark Brown]

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

On Thu, May 07, 2015 at 05:34:27AM +0000, Bard Liao wrote:

> > > We should really be using device_property_() instead of of_property_()
> > > APIs since we will have to support both DT and ACPI properties.

> Unfortunately, I can't find a way to test it.

Why can't you test it - weren't you testing your original version of the
code?

> Is that ok we just replace all of_property_ with device_property_?
> Also, is there any corresponding API for of_get_named_gpio?
> Or we can replace it with device_property_read_u32?
> I tried the change above, and it can build. However I don't know
> if it can work.

You shouldn't be using of_get_named_gpio() for DT stuff either, use
gpiod_get() which follows the usual pattern of taking a string which is
used to do the lookup with whatever firmware is in use.

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

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

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Rafael J. Wysocki]

On 5/7/2015 1:26 PM, Liam Girdwood wrote:
> On Thu, 2015-05-07 at 05:34 +0000, Bard Liao wrote:
>>>>> +static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct
>>>>> +device_node *np) {
>>>>> + rt5645->pdata.in2_diff = of_property_read_bool(np,
>>>>> +         "realtek,in2-differential");
>>>>> + of_property_read_u32(np,
>>>>> +         "realtek,dmic1-data-pin",
>>> &rt5645->pdata.dmic1_data_pin);
>>>> We should really be using device_property_() instead of
>> of_property_()
>>>> APIs since we will have to support both DT and ACPI properties.
>> Unfortunately, I can't find a way to test it.
> device_property() API calls abstract the device tree calls and ACPI
> calls. So testing with your DT based HW will work.
>
>> Is that ok we just replace all of_property_ with device_property_?
> Yes, that's the intention.
>
>> Also, is there any corresponding API for of_get_named_gpio?
>> Or we can replace it with device_property_read_u32?
>> I tried the change above, and it can build. However I don't know
>> if it can work.
> Oh, I think that's a question for Rafael. I think the intention is to
> have a 1:1 mapping between the APIs so that there are no gaps ?

Yes, that's the idea, but then the GPIO maintainers would like to clean 
up the API too.

Mika is more familiar with the GPIO area than I am.

Mika, can you please help us here?

Rafael

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Pierre-Louis Bossart]

On 5/7/15 7:51 AM, Mark Brown wrote:
> On Thu, May 07, 2015 at 05:34:27AM +0000, Bard Liao wrote:
>
>>>> We should really be using device_property_() instead of of_property_()
>>>> APIs since we will have to support both DT and ACPI properties.
>
>> Unfortunately, I can't find a way to test it.
>
> Why can't you test it - weren't you testing your original version of the
> code?

You can test it on an platform using device tree, the device_property_ 
code will fetch the same information as before.
If you want to test on an ACPI platform, I can help with some examples 
(off-list) showing how to override the DSDT table and to add those 
properties in an ASL device entry. You may not be able to test all the 
configurations but at least you can add basic checks that show the data 
is passed from ACPI to your driver. I used all this stuff for a proof of 
concept on AsusT100 where the audio routing in the machine driver is 
passed from ACPI.

>
>> Is that ok we just replace all of_property_ with device_property_?
>> Also, is there any corresponding API for of_get_named_gpio?
>> Or we can replace it with device_property_read_u32?
>> I tried the change above, and it can build. However I don't know
>> if it can work.
>
> You shouldn't be using of_get_named_gpio() for DT stuff either, use
> gpiod_get() which follows the usual pattern of taking a string which is
> used to do the lookup with whatever firmware is in use.

But to Bard's credit the use of_get_named_gpio() is pretty common - i 
see 18 occurrences in soc/codecs alone, others will have the same 
problem...
we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching 
out to gpio and audio maintainers and aligning some sort of coordinated 
change to the gpiod framework w/ guidance to developers, did that thread 
start?

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Mark Brown]

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

On Thu, May 07, 2015 at 01:28:39PM -0500, Pierre-Louis Bossart wrote:
> On 5/7/15 7:51 AM, Mark Brown wrote:

> >You shouldn't be using of_get_named_gpio() for DT stuff either, use
> >gpiod_get() which follows the usual pattern of taking a string which is
> >used to do the lookup with whatever firmware is in use.

> But to Bard's credit the use of_get_named_gpio() is pretty common - i see 18
> occurrences in soc/codecs alone, others will have the same problem...
> we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching out
> to gpio and audio maintainers and aligning some sort of coordinated change
> to the gpiod framework w/ guidance to developers, did that thread start?

I'm not sure there's any particular need for coordination here, the APIs
are in place already - the gpiod_ APIs will already transparently look
up both ACPI and DT (see __gpiod_get_index() for the implementation) and
new drivers should really be using gpiod_ anyway regardless of trying to
do both ACPI and DT.

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

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

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Mark Brown]

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

On Fri, May 08, 2015 at 11:13:39AM +0300, Mika Westerberg wrote:

> Looks like some of the context is lost but if I decode the above
> right, you want to know if there is anything that corresponds to
> of_get_named_gpio() in the generic property API?

> You can use fwnode_get_named_gpiod() if you need to extract a GPIO
> directly from firmware node. Otherwise you should be using
> devm_gpiod_get() and friends.

Right, almost certainly devm_gpiod_get() in this case as I've said
several times now :(

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

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

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Pierre-Louis Bossart]

On 5/7/15 1:36 PM, Mark Brown wrote:
> On Thu, May 07, 2015 at 01:28:39PM -0500, Pierre-Louis Bossart wrote:
>> On 5/7/15 7:51 AM, Mark Brown wrote:
>
>>> You shouldn't be using of_get_named_gpio() for DT stuff either, use
>>> gpiod_get() which follows the usual pattern of taking a string which is
>>> used to do the lookup with whatever firmware is in use.
>
>> But to Bard's credit the use of_get_named_gpio() is pretty common - i see 18
>> occurrences in soc/codecs alone, others will have the same problem...
>> we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching out
>> to gpio and audio maintainers and aligning some sort of coordinated change
>> to the gpiod framework w/ guidance to developers, did that thread start?
>
> I'm not sure there's any particular need for coordination here, the APIs
> are in place already - the gpiod_ APIs will already transparently look
> up both ACPI and DT (see __gpiod_get_index() for the implementation) and
> new drivers should really be using gpiod_ anyway regardless of trying to
> do both ACPI and DT.

Agree, but there wasn't a clear message provided to the readers of this 
mailing list. A 'should' is a recommendation that provides no real 
incentive to move to the new gpiod framework. The message would be 
clearer if maintainers stated that new contributions using 
of_get_named_gpio() will no longer be merged in the mainline (maybe 
after a specific milestone). That would set the direction for everyone.

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Mark Brown]

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

On Fri, May 08, 2015 at 08:30:52AM -0500, Pierre-Louis Bossart wrote:

> Agree, but there wasn't a clear message provided to the readers of this
> mailing list. A 'should' is a recommendation that provides no real incentive
> to move to the new gpiod framework. The message would be clearer if
> maintainers stated that new contributions using of_get_named_gpio() will no
> longer be merged in the mainline (maybe after a specific milestone). That
> would set the direction for everyone.

That's what's been happening...

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

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

Re: [PATCH 3/3] ASoC: rt5645: add device tree support

[Pierre-Louis Bossart]

On 5/8/15 11:46 AM, Mark Brown wrote:
> On Fri, May 08, 2015 at 08:30:52AM -0500, Pierre-Louis Bossart wrote:
>
>> Agree, but there wasn't a clear message provided to the readers of this
>> mailing list. A 'should' is a recommendation that provides no real incentive
>> to move to the new gpiod framework. The message would be clearer if
>> maintainers stated that new contributions using of_get_named_gpio() will no
>> longer be merged in the mainline (maybe after a specific milestone). That
>> would set the direction for everyone.
>
> That's what's been happening...

We are in agreement. I wasn't trying to argue but make sure everyone 
noticed this change. I saw a commit with of_get_named_gpio in sound/soc 
as recent as December 30, 2014 and we talked to 3rd parties unaware of 
this change. Now they are :-)