[PATCH] ASoC: rt5640: add device tree support

[PATCH] ASoC: rt5640: add device tree support

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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

Slight re-ordering of rt5640_i2c_probe() to better fit the DT parsing.

Since ldo1_en is optional, guard usage of it with gpio_is_valid(), rather
than open-coding an if (gpio) check.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Mark, If the underlying rt5640 driver is reposted, this might need a new
version to rebase it, but otherwise I believe should be complete.

Bard, FYI you can find this patch and the DT changes for Dalmore at:
git://nv-tegra.nvidia.com/user/swarren/linux-2.6 linux-next_common

 Documentation/devicetree/bindings/sound/rt5640.txt | 30 ++++++++++++++++
 sound/soc/codecs/rt5640.c                          | 40 ++++++++++++++++++----
 2 files changed, 64 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/rt5640.txt

diff --git a/Documentation/devicetree/bindings/sound/rt5640.txt b/Documentation/devicetree/bindings/sound/rt5640.txt
new file mode 100644
index 0000000..005bcb2
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rt5640.txt
@@ -0,0 +1,30 @@
+RT5640 audio CODEC
+
+This device supports I2C only.
+
+Required properties:
+
+- compatible : "realtek,rt5640".
+
+- reg : The I2C address of the device.
+
+- interrupts : The CODEC's interrupt output.
+
+Optional properties:
+
+- realtek,in1-differential
+- realtek,in2-differential
+  Boolean. Indicate MIC1/2 input are differential, rather than single-ended.
+
+- realtek,ldo1-en-gpios : The GPIO that controls the CODEC's LDO1_EN pin.
+
+Example:
+
+rt5640 {
+	compatible = "realtek,rt5640";
+	reg = <0x1c>;
+	interrupt-parent = <&gpio>;
+	interrupts = <TEGRA_GPIO(W, 3) GPIO_ACTIVE_HIGH>;
+	realtek,ldo1-en-gpios =
+		<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
+};
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index 288c17c..f7e73d2 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -18,6 +18,7 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 #include <sound/core.h>
@@ -1998,6 +1999,28 @@ static const struct i2c_device_id rt5640_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id);
 
+static int rt5640_parse_dt(struct rt5640_priv *rt5640, struct device_node *np)
+{
+	rt5640->pdata.in1_diff = of_property_read_bool(np,
+					"realtek,in1-differential");
+	rt5640->pdata.in2_diff = of_property_read_bool(np,
+					"realtek,in2-differential");
+
+	rt5640->pdata.ldo1_en = of_get_named_gpio(np,
+					"realtek,ldo1-en-gpios", 0);
+	/*
+	 * LDO1_EN 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 (!gpio_is_valid(rt5640->pdata.ldo1_en) &&
+			(rt5640->pdata.ldo1_en != -ENOENT))
+		return rt5640->pdata.ldo1_en;
+
+	return 0;
+}
+
 static int rt5640_i2c_probe(struct i2c_client *i2c,
 		    const struct i2c_device_id *id)
 {
@@ -2011,6 +2034,16 @@ static int rt5640_i2c_probe(struct i2c_client *i2c,
 				GFP_KERNEL);
 	if (NULL == rt5640)
 		return -ENOMEM;
+	i2c_set_clientdata(i2c, rt5640);
+
+	if (pdata)
+		rt5640->pdata = *pdata;
+	else if (i2c->dev.of_node) {
+		ret = rt5640_parse_dt(rt5640, i2c->dev.of_node);
+		if (ret)
+			return ret;
+	} else
+		rt5640->pdata.ldo1_en = -1;
 
 	rt5640->regmap = devm_regmap_init_i2c(i2c, &rt5640_regmap);
 	if (IS_ERR(rt5640->regmap)) {
@@ -2020,12 +2053,7 @@ static int rt5640_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	if (pdata)
-		rt5640->pdata = *pdata;
-
-	i2c_set_clientdata(i2c, rt5640);
-
-	if (rt5640->pdata.ldo1_en) {
+	if (gpio_is_valid(rt5640->pdata.ldo1_en)) {
 		ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
 					    GPIOF_OUT_INIT_HIGH,
 					    "RT5640 LDO1_EN");
-- 
1.8.1.5

Re: [PATCH] ASoC: rt5640: add device tree support

[Mark Brown]

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

On Tue, Jun 11, 2013 at 02:40:40PM -0600, Stephen Warren wrote:

> +- realtek,ldo1-en-gpios : The GPIO that controls the CODEC's LDO1_EN pin.

Why gpios and not gpio?

> -	if (rt5640->pdata.ldo1_en) {
> +	if (gpio_is_valid(rt5640->pdata.ldo1_en)) {
>  		ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
>  					    GPIOF_OUT_INIT_HIGH,

Unfortunately gpio_is_valid() is unhelpful for platform data since often
zero is a valid GPIO but it's also the default "do nothing" platform
data.  It's therefore better to either include a check for non-zero as
well or have code that takes a zero in the platform data and sets it to
a negative value instead.

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

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] ASoC: rt5640: add device tree support

[Stephen Warren]

On 06/12/2013 10:46 AM, Mark Brown wrote:
> On Tue, Jun 11, 2013 at 02:40:40PM -0600, Stephen Warren wrote:
> 
>> +- realtek,ldo1-en-gpios : The GPIO that controls the CODEC's
>> LDO1_EN pin.
> 
> Why gpios and not gpio?

For some reason, GPIO properties have always been named "gpios" rather
than "gpio", even when only a single entry is expected. I don't really
understand why, but I've been asked (or seen others asked) to
s/gpio/gpios/ in DT bindings before. I explicitly CC'd Grant and Rob
here in case they can shed any light.

>> -	if (rt5640->pdata.ldo1_en) { +	if
>> (gpio_is_valid(rt5640->pdata.ldo1_en)) { ret =
>> devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en, 
>> GPIOF_OUT_INIT_HIGH,
> 
> Unfortunately gpio_is_valid() is unhelpful for platform data since
> often zero is a valid GPIO but it's also the default "do nothing"
> platform data.  It's therefore better to either include a check for
> non-zero as well or have code that takes a zero in the platform
> data and sets it to a negative value instead.

I think people filling in platform data should simply be required to
put a valid/correct value in that field. There aren't any users of
this, so it's not like adding a new field where
backwards-compatibility might be a concern (and even then, updating
all users of this platform data type wouldn't be hard), and it should
be obvious if you get it wrong.

Re: [PATCH] ASoC: rt5640: add device tree support

[Mark Brown]

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

On Wed, Jun 12, 2013 at 10:56:46AM -0600, Stephen Warren wrote:
> On 06/12/2013 10:46 AM, Mark Brown wrote:

> > Unfortunately gpio_is_valid() is unhelpful for platform data since
> > often zero is a valid GPIO but it's also the default "do nothing"
> > platform data.  It's therefore better to either include a check for
> > non-zero as well or have code that takes a zero in the platform
> > data and sets it to a negative value instead.

> I think people filling in platform data should simply be required to
> put a valid/correct value in that field. There aren't any users of
> this, so it's not like adding a new field where
> backwards-compatibility might be a concern (and even then, updating
> all users of this platform data type wouldn't be hard), and it should
> be obvious if you get it wrong.

It's not idiomatic to do it this way, it'll catch people out - sadly the
effects are often non-obvious, depending on what actually happens with
the GPIO.  Were it not for renumbering pain it'd probably be most
sensible to just make 0 never a valid GPIO :/

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

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss