[PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Bard Liao]

Move gpio to gpio_desc and use gpiod APIs in codec driver.

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

diff --git a/include/sound/rt5645.h b/include/sound/rt5645.h
index 652cb9e..22734bc 100644
--- a/include/sound/rt5645.h
+++ b/include/sound/rt5645.h
@@ -20,9 +20,6 @@ struct rt5645_platform_data {
 	unsigned int dmic2_data_pin;
 	/* 0 = IN2P; 1 = GPIO6; 2 = GPIO10; 3 = GPIO12 */
 
-	unsigned int hp_det_gpio;
-	bool gpio_hp_det_active_high;
-
 	unsigned int jd_mode;
 };
 
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index aaede45..0cb4942 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -2938,17 +2938,11 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645)
 
 	switch (rt5645->pdata.jd_mode) {
 	case 0: /* Not using rt5645 JD */
-		if (gpio_is_valid(rt5645->pdata.hp_det_gpio)) {
-			gpio_state = gpio_get_value(rt5645->pdata.hp_det_gpio);
-			dev_dbg(rt5645->codec->dev, "gpio = %d(%d)\n",
-				rt5645->pdata.hp_det_gpio, gpio_state);
-		}
-		if ((rt5645->pdata.gpio_hp_det_active_high && gpio_state) ||
-			(!rt5645->pdata.gpio_hp_det_active_high &&
-			 !gpio_state)) {
-			report = rt5645_jack_detect(rt5645->codec, 1);
-		} else {
-			report = rt5645_jack_detect(rt5645->codec, 0);
+		if (rt5645->gpiod_hp_det) {
+			gpio_state = gpiod_get_value(rt5645->gpiod_hp_det);
+			dev_dbg(rt5645->codec->dev, "gpio_state = %d\n",
+				gpio_state);
+			report = rt5645_jack_detect(rt5645->codec, gpio_state);
 		}
 		snd_soc_jack_report(rt5645->hp_jack,
 				    report, SND_JACK_HEADPHONE);
@@ -3253,19 +3247,17 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 	} else {
 		if (dmi_check_system(dmi_platform_intel_braswell)) {
 			rt5645->pdata = *rt5645_pdata;
-			gpiod = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);
-
-			if (IS_ERR(gpiod) || gpiod_direction_input(gpiod)) {
-				rt5645->pdata.hp_det_gpio = -1;
-				dev_err(&i2c->dev, "failed to initialize gpiod\n");
-			} else {
-				rt5645->pdata.hp_det_gpio = desc_to_gpio(gpiod);
-				rt5645->pdata.gpio_hp_det_active_high
-						= !gpiod_is_active_low(gpiod);
-			}
 		}
 	}
 
+	rt5645->gpiod_hp_det = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);
+
+	if (IS_ERR(rt5645->gpiod_hp_det) ||
+	    gpiod_direction_input(rt5645->gpiod_hp_det)) {
+		rt5645->gpiod_hp_det = NULL;
+		dev_err(&i2c->dev, "failed to initialize gpiod\n");
+	}
+
 	rt5645->regmap = devm_regmap_init_i2c(i2c, &rt5645_regmap);
 	if (IS_ERR(rt5645->regmap)) {
 		ret = PTR_ERR(rt5645->regmap);
@@ -3425,16 +3417,6 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 			dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret);
 	}
 
-	if (gpio_is_valid(rt5645->pdata.hp_det_gpio)) {
-		ret = gpio_request(rt5645->pdata.hp_det_gpio, "rt5645");
-		if (ret)
-			dev_err(&i2c->dev, "Fail gpio_request hp_det_gpio\n");
-
-		ret = gpio_direction_input(rt5645->pdata.hp_det_gpio);
-		if (ret)
-			dev_err(&i2c->dev, "Fail gpio_direction hp_det_gpio\n");
-	}
-
 	INIT_DELAYED_WORK(&rt5645->jack_detect_work, rt5645_jack_detect_work);
 
 	return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5645,
@@ -3450,9 +3432,6 @@ static int rt5645_i2c_remove(struct i2c_client *i2c)
 
 	cancel_delayed_work_sync(&rt5645->jack_detect_work);
 
-	if (gpio_is_valid(rt5645->pdata.hp_det_gpio))
-		gpio_free(rt5645->pdata.hp_det_gpio);
-
 	snd_soc_unregister_codec(&i2c->dev);
 
 	return 0;
diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h
index 9ec4e89..0353a6a 100644
--- a/sound/soc/codecs/rt5645.h
+++ b/sound/soc/codecs/rt5645.h
@@ -2182,6 +2182,7 @@ struct rt5645_priv {
 	struct rt5645_platform_data pdata;
 	struct regmap *regmap;
 	struct i2c_client *i2c;
+	struct gpio_desc *gpiod_hp_det;
 	struct snd_soc_jack *hp_jack;
 	struct snd_soc_jack *mic_jack;
 	struct snd_soc_jack *btn_jack;
-- 
1.8.1.1.439.g50a6b54

[PATCH v2 2/2] 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>
---
* Change of_property_ APIs to device_property_ APIs
---
 Documentation/devicetree/bindings/sound/rt5645.txt | 68 ++++++++++++++++++++++
 sound/soc/codecs/rt5645.c                          | 16 +++++
 2 files changed, 84 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..f21e9af
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rt5645.txt
@@ -0,0 +1,68 @@
+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,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>;
+	rt5645-gpios = <&gpio 19 0>;
+	interrupt-parent = <&gpio>;
+	interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+	realtek,dmic-en = "true";
+	realtek,en-jd-func = "true";
+	realtek,jd-mode = <3>;
+};
\ No newline at end of file
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 0cb4942..76b1efd 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -3225,6 +3225,20 @@ 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 = device_property_read_bool(np,
+		"realtek,in2-differential");
+	device_property_read_u32(np,
+		"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);
+	device_property_read_u32(np,
+		"realtek,dmic2-data-pin", &rt5645->pdata.dmic2_data_pin);
+	device_property_read_u32(np,
+		"realtek,jd-mode", &rt5645->pdata.jd_mode);
+
+	return 0;
+}
+
 static int rt5645_i2c_probe(struct i2c_client *i2c,
 		    const struct i2c_device_id *id)
 {
@@ -3244,6 +3258,8 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 
 	if (pdata) {
 		rt5645->pdata = *pdata;
+	} else if (i2c->dev.of_node) {
+		rt5645_parse_dt(rt5645, i2c->dev.of_node);
 	} else {
 		if (dmi_check_system(dmi_platform_intel_braswell)) {
 			rt5645->pdata = *rt5645_pdata;
-- 
1.8.1.1.439.g50a6b54

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

[Lars-Peter Clausen]

On 05/29/2015 12:16 PM, Bard Liao wrote:
[...]
> +
> +Example:
> +
> +codec: rt5650@1a {
> +	compatible = "realtek,rt5650";
> +	reg = <0x1a>;
> +	rt5645-gpios = <&gpio 19 0>;

The gpio is missing from the property documentation.

> +	interrupt-parent = <&gpio>;
> +	interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +	realtek,dmic-en = "true";
> +	realtek,en-jd-func = "true";
> +	realtek,jd-mode = <3>;
> +};

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Michele Curti]

On Fri, May 29, 2015 at 06:16:46PM +0800, Bard Liao wrote:
> Move gpio to gpio_desc and use gpiod APIs in codec driver.

I don't get these errors anymore :)

[    1.805330] rt5645 i2c-10EC5648:00: Fail gpio_request hp_det_gpio
[    1.805439] rt5645 i2c-10EC5648:00: Fail gpio_direction hp_det_gpio

Thanks,
Michele

> 
> Signed-off-by: Bard Liao <bardliao@realtek.com>
> ---
>  include/sound/rt5645.h    |  3 ---
>  sound/soc/codecs/rt5645.c | 47 +++++++++++++----------------------------------
>  sound/soc/codecs/rt5645.h |  1 +
>  3 files changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/include/sound/rt5645.h b/include/sound/rt5645.h
> index 652cb9e..22734bc 100644
> --- a/include/sound/rt5645.h
> +++ b/include/sound/rt5645.h
> @@ -20,9 +20,6 @@ struct rt5645_platform_data {
>  	unsigned int dmic2_data_pin;
>  	/* 0 = IN2P; 1 = GPIO6; 2 = GPIO10; 3 = GPIO12 */
>  
> -	unsigned int hp_det_gpio;
> -	bool gpio_hp_det_active_high;
> -
>  	unsigned int jd_mode;
>  };
>  
> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index aaede45..0cb4942 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -2938,17 +2938,11 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645)
>  
>  	switch (rt5645->pdata.jd_mode) {
>  	case 0: /* Not using rt5645 JD */
> -		if (gpio_is_valid(rt5645->pdata.hp_det_gpio)) {
> -			gpio_state = gpio_get_value(rt5645->pdata.hp_det_gpio);
> -			dev_dbg(rt5645->codec->dev, "gpio = %d(%d)\n",
> -				rt5645->pdata.hp_det_gpio, gpio_state);
> -		}
> -		if ((rt5645->pdata.gpio_hp_det_active_high && gpio_state) ||
> -			(!rt5645->pdata.gpio_hp_det_active_high &&
> -			 !gpio_state)) {
> -			report = rt5645_jack_detect(rt5645->codec, 1);
> -		} else {
> -			report = rt5645_jack_detect(rt5645->codec, 0);
> +		if (rt5645->gpiod_hp_det) {
> +			gpio_state = gpiod_get_value(rt5645->gpiod_hp_det);
> +			dev_dbg(rt5645->codec->dev, "gpio_state = %d\n",
> +				gpio_state);
> +			report = rt5645_jack_detect(rt5645->codec, gpio_state);
>  		}
>  		snd_soc_jack_report(rt5645->hp_jack,
>  				    report, SND_JACK_HEADPHONE);
> @@ -3253,19 +3247,17 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>  	} else {
>  		if (dmi_check_system(dmi_platform_intel_braswell)) {
>  			rt5645->pdata = *rt5645_pdata;
> -			gpiod = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);
> -
> -			if (IS_ERR(gpiod) || gpiod_direction_input(gpiod)) {
> -				rt5645->pdata.hp_det_gpio = -1;
> -				dev_err(&i2c->dev, "failed to initialize gpiod\n");
> -			} else {
> -				rt5645->pdata.hp_det_gpio = desc_to_gpio(gpiod);
> -				rt5645->pdata.gpio_hp_det_active_high
> -						= !gpiod_is_active_low(gpiod);
> -			}
>  		}
>  	}
>  
> +	rt5645->gpiod_hp_det = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);
> +
> +	if (IS_ERR(rt5645->gpiod_hp_det) ||
> +	    gpiod_direction_input(rt5645->gpiod_hp_det)) {
> +		rt5645->gpiod_hp_det = NULL;
> +		dev_err(&i2c->dev, "failed to initialize gpiod\n");
> +	}
> +
>  	rt5645->regmap = devm_regmap_init_i2c(i2c, &rt5645_regmap);
>  	if (IS_ERR(rt5645->regmap)) {
>  		ret = PTR_ERR(rt5645->regmap);
> @@ -3425,16 +3417,6 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>  			dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret);
>  	}
>  
> -	if (gpio_is_valid(rt5645->pdata.hp_det_gpio)) {
> -		ret = gpio_request(rt5645->pdata.hp_det_gpio, "rt5645");
> -		if (ret)
> -			dev_err(&i2c->dev, "Fail gpio_request hp_det_gpio\n");
> -
> -		ret = gpio_direction_input(rt5645->pdata.hp_det_gpio);
> -		if (ret)
> -			dev_err(&i2c->dev, "Fail gpio_direction hp_det_gpio\n");
> -	}
> -
>  	INIT_DELAYED_WORK(&rt5645->jack_detect_work, rt5645_jack_detect_work);
>  
>  	return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5645,
> @@ -3450,9 +3432,6 @@ static int rt5645_i2c_remove(struct i2c_client *i2c)
>  
>  	cancel_delayed_work_sync(&rt5645->jack_detect_work);
>  
> -	if (gpio_is_valid(rt5645->pdata.hp_det_gpio))
> -		gpio_free(rt5645->pdata.hp_det_gpio);
> -
>  	snd_soc_unregister_codec(&i2c->dev);
>  
>  	return 0;
> diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h
> index 9ec4e89..0353a6a 100644
> --- a/sound/soc/codecs/rt5645.h
> +++ b/sound/soc/codecs/rt5645.h
> @@ -2182,6 +2182,7 @@ struct rt5645_priv {
>  	struct rt5645_platform_data pdata;
>  	struct regmap *regmap;
>  	struct i2c_client *i2c;
> +	struct gpio_desc *gpiod_hp_det;
>  	struct snd_soc_jack *hp_jack;
>  	struct snd_soc_jack *mic_jack;
>  	struct snd_soc_jack *btn_jack;
> -- 
> 1.8.1.1.439.g50a6b54
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Lars-Peter Clausen]

On 05/29/2015 12:16 PM, Bard Liao wrote:
> @@ -3253,19 +3247,17 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>   	} else {
>   		if (dmi_check_system(dmi_platform_intel_braswell)) {
>   			rt5645->pdata = *rt5645_pdata;
> -			gpiod = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);
> -
> -			if (IS_ERR(gpiod) || gpiod_direction_input(gpiod)) {
> -				rt5645->pdata.hp_det_gpio = -1;
> -				dev_err(&i2c->dev, "failed to initialize gpiod\n");
> -			} else {
> -				rt5645->pdata.hp_det_gpio = desc_to_gpio(gpiod);
> -				rt5645->pdata.gpio_hp_det_active_high
> -						= !gpiod_is_active_low(gpiod);
> -			}
>   		}
>   	}
>
> +	rt5645->gpiod_hp_det = devm_gpiod_get_index(&i2c->dev, "rt5645", 0);

Three things, don't use the _index API if there is only a single gpio for 
the property, either don't use a name at all or use a descriptive name 
something like "hp-detect" and use the new version of the API which has the 
flags parameter.

So this should be: devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN);

and then drop the gpiod_direction_input()...

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Mark Brown]

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

On Fri, May 29, 2015 at 06:31:08PM +0200, Lars-Peter Clausen wrote:

> Three things, don't use the _index API if there is only a single gpio for
> the property, either don't use a name at all or use a descriptive name
> something like "hp-detect" and use the new version of the API which has the
> flags parameter.

> So this should be: devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN);

> and then drop the gpiod_direction_input()...

It seems better if people use names where possible if there's any chance
that we could add support for other GPIOs in the future, that avoids
confusion further down the line with extension.

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

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

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Bard Liao]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Wednesday, June 03, 2015 1:17 AM
> To: Lars-Peter Clausen
> Cc: Bard Liao; lgirdwood@gmail.com; Oder Chiou;
> alsa-devel@alsa-project.org; zhengxing@rock-chips.com;
> yang.a.fang@intel.com; koro.chen@mediatek.com; John Lin;
> Leilk.Liu@mediatek.com; Flove
> Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: rt5645: change gpio to
> gpiod APIs
> 
> On Fri, May 29, 2015 at 06:31:08PM +0200, Lars-Peter Clausen wrote:
> 
> > Three things, don't use the _index API if there is only a single gpio
> > for the property, either don't use a name at all or use a descriptive
> > name something like "hp-detect" and use the new version of the API
> > which has the flags parameter.
> 
> > So this should be: devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN);
> 
> > and then drop the gpiod_direction_input()...
> 
> It seems better if people use names where possible if there's any chance
> that we could add support for other GPIOs in the future, that avoids
> confusion further down the line with extension.

Do you mean use a well-described gpio name such as "hp-detect" so that
we can use another name if we need to add other gpios in the future?

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

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Lars-Peter Clausen]

On 06/03/2015 02:03 PM, Bard Liao wrote:
>> -----Original Message-----
>> From: Mark Brown [mailto:broonie@kernel.org]
>> Sent: Wednesday, June 03, 2015 1:17 AM
>> To: Lars-Peter Clausen
>> Cc: Bard Liao; lgirdwood@gmail.com; Oder Chiou;
>> alsa-devel@alsa-project.org; zhengxing@rock-chips.com;
>> yang.a.fang@intel.com; koro.chen@mediatek.com; John Lin;
>> Leilk.Liu@mediatek.com; Flove
>> Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: rt5645: change gpio to
>> gpiod APIs
>>
>> On Fri, May 29, 2015 at 06:31:08PM +0200, Lars-Peter Clausen wrote:
>>
>>> Three things, don't use the _index API if there is only a single gpio
>>> for the property, either don't use a name at all or use a descriptive
>>> name something like "hp-detect" and use the new version of the API
>>> which has the flags parameter.
>>
>>> So this should be: devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN);
>>
>>> and then drop the gpiod_direction_input()...
>>
>> It seems better if people use names where possible if there's any chance
>> that we could add support for other GPIOs in the future, that avoids
>> confusion further down the line with extension.
>
> Do you mean use a well-described gpio name such as "hp-detect" so that
> we can use another name if we need to add other gpios in the future?

Yes, kind of. The name of the GPIO should be its function. Having a GPIO 
with the name rt5645 on a rt5645 does not really describe anything since we 
already know that it is a rt5645.

Re: [PATCH v2 1/2] ASoC: rt5645: change gpio to gpiod APIs

[Mark Brown]

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

On Wed, Jun 03, 2015 at 12:03:06PM +0000, Bard Liao wrote:

> > It seems better if people use names where possible if there's any chance
> > that we could add support for other GPIOs in the future, that avoids
> > confusion further down the line with extension.

> Do you mean use a well-described gpio name such as "hp-detect" so that
> we can use another name if we need to add other gpios in the future?

Yes.

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

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