[alsa-devel] [PATCH] ASoC: rt5640: add device tree support
From: Stephen Warren swarren@nvidia.com
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@nvidia.com --- 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");
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.
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.
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 :/
participants (2)
-
Mark Brown -
Stephen Warren