[RFC 0/3] ARM: Tegra: Device Tree: i2c & wm8903

[RFC 0/3] ARM: Tegra: Device Tree: i2c & wm8903

[John Bonesio]

This is revision 2 of my previous patch series:
     Tegra: Harmony: Device Tree first steps

The following patch series is a step in the direction of getting board specific
devices initialized from a device tree.

This patch set adds intiailization of i2c and the wm8903. The i2c controller is
in the SoC and is initialized statically, but the board specific information is
pulled in from the device tree. The wm8903 is initialized using only the device
tree when using the board-dt.c general Tegra board initialization file.

---

John Bonesio (3):
      ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized
      ARM: Tegra: Device Tree Support: Add i2c devices
      ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree


 arch/arm/boot/dts/tegra-harmony.dts |   37 ++++++++++++++
 arch/arm/boot/dts/tegra250.dtsi     |   37 ++++++++++++++
 arch/arm/mach-tegra/Makefile        |    1 
 arch/arm/mach-tegra/board-dt.c      |   94 ++++-------------------------------
 drivers/i2c/busses/i2c-tegra.c      |   16 ++++++
 sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--
 sound/soc/tegra/Kconfig             |    2 -
 sound/soc/tegra/harmony.c           |    8 +++
 8 files changed, 200 insertions(+), 88 deletions(-)

-- 
- John

[RFC 0/3] ARM: Tegra: Device Tree: i2c & wm8903

[John Bonesio]

This is revision 2 of my previous patch series:
     Tegra: Harmony: Device Tree first steps

The following patch series is a step in the direction of getting board specific
devices initialized from a device tree.

This patch set adds intiailization of i2c and the wm8903. The i2c controller is
in the SoC and is initialized statically, but the board specific information is
pulled in from the device tree. The wm8903 is initialized using only the device
tree when using the board-dt.c general Tegra board initialization file.

---

John Bonesio (3):
      ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized
      ARM: Tegra: Device Tree Support: Add i2c devices
      ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree


 arch/arm/boot/dts/tegra-harmony.dts |   37 ++++++++++++++
 arch/arm/boot/dts/tegra250.dtsi     |   37 ++++++++++++++
 arch/arm/mach-tegra/Makefile        |    1 
 arch/arm/mach-tegra/board-dt.c      |   94 ++++-------------------------------
 drivers/i2c/busses/i2c-tegra.c      |   16 ++++++
 sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--
 sound/soc/tegra/Kconfig             |    2 -
 sound/soc/tegra/harmony.c           |    8 +++
 8 files changed, 200 insertions(+), 88 deletions(-)

-- 
- John

[RFC 1/3] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[John Bonesio]

This patch changes how sdhci devices are initialized so that a later patch can
easily add support for i2c devies.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/mach-tegra/Makefile   |    1 
 arch/arm/mach-tegra/board-dt.c |   88 ++--------------------------------------
 2 files changed, 6 insertions(+), 83 deletions(-)

diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index 842ec1a..54d2215 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -31,6 +31,7 @@ obj-${CONFIG_MACH_SEABOARD}             += board-seaboard.o
 obj-${CONFIG_MACH_SEABOARD}             += board-seaboard-pinmux.o
 
 obj-${CONFIG_MACH_TEGRA_DT}             += board-dt.o
+obj-${CONFIG_MACH_TEGRA_DT}             += board-harmony-pinmux.o
 
 obj-${CONFIG_MACH_TRIMSLICE}            += board-trimslice.o
 obj-${CONFIG_MACH_TRIMSLICE}            += board-trimslice-pinmux.o
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 9a35741..50ab328 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -36,92 +36,14 @@
 
 #include <mach/iomap.h>
 #include <mach/irqs.h>
+#include <mach/sdhci.h>
 
 #include "board.h"
-#include "board-harmony.h"
 #include "clock.h"
+#include "devices.h"
+#include "gpio-names.h"
 
-static struct resource sdhci_resource1[] = {
-	[0] = {
-		.start  = INT_SDMMC1,
-		.end    = INT_SDMMC1,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC1_BASE,
-		.end	= TEGRA_SDMMC1_BASE + TEGRA_SDMMC1_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device1 = {
-	.name		= "sdhci-tegra",
-	.id		= 0,
-	.resource	= sdhci_resource1,
-	.num_resources	= ARRAY_SIZE(sdhci_resource1),
-};
-
-static struct resource sdhci_resource2[] = {
-	[0] = {
-		.start  = INT_SDMMC2,
-		.end    = INT_SDMMC2,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC2_BASE,
-		.end	= TEGRA_SDMMC2_BASE + TEGRA_SDMMC2_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device2 = {
-	.name		= "sdhci-tegra",
-	.id		= 1,
-	.resource	= sdhci_resource2,
-	.num_resources	= ARRAY_SIZE(sdhci_resource2),
-};
-
-static struct resource sdhci_resource3[] = {
-	[0] = {
-		.start  = INT_SDMMC3,
-		.end    = INT_SDMMC3,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC3_BASE,
-		.end	= TEGRA_SDMMC3_BASE + TEGRA_SDMMC3_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device3 = {
-	.name		= "sdhci-tegra",
-	.id		= 2,
-	.resource	= sdhci_resource3,
-	.num_resources	= ARRAY_SIZE(sdhci_resource3),
-};
-
-static struct resource sdhci_resource4[] = {
-	[0] = {
-		.start  = INT_SDMMC4,
-		.end    = INT_SDMMC4,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC4_BASE,
-		.end	= TEGRA_SDMMC4_BASE + TEGRA_SDMMC4_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device4 = {
-	.name		= "sdhci-tegra",
-	.id		= 3,
-	.resource	= sdhci_resource4,
-	.num_resources	= ARRAY_SIZE(sdhci_resource4),
-};
-
-static struct platform_device *harmony_devices[] __initdata = {
+static struct platform_device *tegra250_devices[] __initdata = {
 	&tegra_sdhci_device1,
 	&tegra_sdhci_device2,
 	&tegra_sdhci_device3,
@@ -164,7 +86,7 @@ static void __init tegra_dt_init(void)
 
 	harmony_pinmux_init();
 
-	platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
+	platform_add_devices(tegra250_devices, ARRAY_SIZE(tegra250_devices));
 
 	/*
 	 * Finished with the static registrations now; fill in the missing

[RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[John Bonesio]

This patch initializes i2c controller devices in board-dt.c. The i2c controller
is added to tegra250.dtsi so later on-board i2c devices can be found and
initialized based on the device tree information.

Board specific i2c parameters are placed in tegra-harmony.dts.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/boot/dts/tegra-harmony.dts |   20 +++++++++++++++++++
 arch/arm/boot/dts/tegra250.dtsi     |   37 +++++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/board-dt.c      |    8 ++++++++
 drivers/i2c/busses/i2c-tegra.c      |   16 ++++++++++++++-
 4 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
index 23592b2..af169aa 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -16,6 +16,26 @@
 		reg = < 0x00000000 0x40000000 >;
 	};
 
+	i2c@7000c000 {
+		status = "ok";
+		clock-frequency = <400000>;
+	};
+
+	i2c@7000c400 {
+		status = "ok";
+		clock-frequency = <400000>;
+	};
+
+	i2c@7000c500 {
+		status = "ok";
+		clock-frequency = <400000>;
+	};
+
+	i2c@7000d000 {
+		status = "ok";
+		clock-frequency = <400000>;
+	};
+
 	serial@70006300 {
 		status = "ok";
 		clock-frequency = < 216000000 >;
diff --git a/arch/arm/boot/dts/tegra250.dtsi b/arch/arm/boot/dts/tegra250.dtsi
index f1801b8..0a056a2 100644
--- a/arch/arm/boot/dts/tegra250.dtsi
+++ b/arch/arm/boot/dts/tegra250.dtsi
@@ -20,6 +20,43 @@
 		};
 	};
 
+
+	i2c@7000c000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C000 0x100>;
+		interrupts = < 70 >;
+		status = "disabled";
+	};
+
+	i2c@7000c400 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C400 0x100>;
+		interrupts = < 116 >;
+		status = "disabled";
+	};
+
+	i2c@7000c500 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C500 0x100>;
+		interrupts = < 124 >;
+		status = "disabled";
+	};
+
+	i2c@7000d000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000D000 0x200>;
+		interrupts = < 85 >;
+		status = "disabled";
+	};
+
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra250-gpio";
 		reg = < 0x6000d000 0x1000 >;
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 50ab328..c498e84 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -28,6 +28,8 @@
 #include <linux/of_platform.h>
 #include <linux/pda_power.h>
 #include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/i2c-tegra.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -43,11 +45,17 @@
 #include "devices.h"
 #include "gpio-names.h"
 
+void harmony_pinmux_init(void);
+
 static struct platform_device *tegra250_devices[] __initdata = {
 	&tegra_sdhci_device1,
 	&tegra_sdhci_device2,
 	&tegra_sdhci_device3,
 	&tegra_sdhci_device4,
+	&tegra_i2c_device1,
+	&tegra_i2c_device2,
+	&tegra_i2c_device3,
+	&tegra_i2c_device4,
 };
 
 static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b4ab39b..1693c6a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/i2c-tegra.h>
+#include <linux/of_i2c.h>
 
 #include <asm/unaligned.h>
 
@@ -511,6 +512,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	struct resource *iomem;
 	struct clk *clk;
 	struct clk *i2c_clk;
+	const unsigned int *prop;
 	void *base;
 	int irq;
 	int ret = 0;
@@ -568,7 +570,16 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
-	i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
+
+	i2c_dev->bus_clk_rate = 100000; /* default clock rate */
+	if (i2c_dev->dev->of_node) {    /* if there is a device tree node ... */
+		prop = of_get_property(i2c_dev->dev->of_node,
+				"clock-frequency", NULL);
+		if (prop)
+			i2c_dev->bus_clk_rate = be32_to_cpup(prop);
+
+	} else if (pdata)
+		i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
 
 	if (pdev->id == 3)
 		i2c_dev->is_dvc = 1;
@@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.dev.parent = &pdev->dev;
 	i2c_dev->adapter.nr = pdev->id;
+	i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
 
 	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
 	if (ret) {
@@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
+	of_i2c_register_devices(&i2c_dev->adapter);
+
 	return 0;
 err_free_irq:
 	free_irq(i2c_dev->irq, i2c_dev);

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[John Bonesio]

This patch makes it so the wm8903 is initialized from it's device tree node.

Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
 sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--
 sound/soc/tegra/Kconfig             |    2 -
 sound/soc/tegra/harmony.c           |    8 +++
 4 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
index af169aa..05521a5 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -19,6 +19,23 @@
 	i2c@7000c000 {
 		status = "ok";
 		clock-frequency = <400000>;
+
+		codec: wm8903@1a {
+			compatible = "wlf,wm8903";
+			reg = <0x1a>;
+			interrupts = < 347 >;
+			irq-active-low = <0>;
+			micdet-cfg = <0>;
+			micdet-delay = <100>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			gpio-base = < 224 >;
+			gpio-num-cfg = < 5 >;
+			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
+			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
+		};
 	};
 
 	i2c@7000c400 {
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index f52b623..2347201 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -1865,10 +1865,14 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
 	wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO;
 	wm8903->gpio_chip.dev = codec->dev;
 
-	if (pdata && pdata->gpio_base)
+	wm8903->gpio_chip.base = -1;
+	if (pdata && pdata->gpio_base) {
 		wm8903->gpio_chip.base = pdata->gpio_base;
-	else
-		wm8903->gpio_chip.base = -1;
+	} else if (codec->dev->of_node) {
+		prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
+		if (prop)
+			wm8903->gpio_chip.base = be32_to_cpup(prop);
+	}
 
 	ret = gpiochip_add(&wm8903->gpio_chip);
 	if (ret != 0)
@@ -1898,6 +1902,11 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 {
 	struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev);
 	struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec);
+	const unsigned int *prop;
+	int micdet_cfg = 0;
+	int irq_active_low;
+	int num_gpios = 0;
+	int gpio_cfg = 0;
 	int ret, i;
 	int trigger, irq_pol;
 	u16 val;
@@ -1964,10 +1973,76 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 		WARN_ON(!mic_gpio && (pdata->micdet_cfg & WM8903_MICDET_ENA));
 
 		wm8903->mic_delay = pdata->micdet_delay;
+	} else if (codec->dev->of_node) {
+		bool mic_gpio = false;
+
+		prop = of_get_property(codec->dev->of_node, "gpio-num-cfg", NULL);
+		if (prop)
+			num_gpios = be32_to_cpup(prop);
+
+		prop = of_get_property(codec->dev->of_node, "gpio-cfg", NULL);
+		if (num_gpios && prop) {
+			for (i = 0; i < num_gpios; i++) {
+				gpio_cfg = be32_to_cpu(prop[i]);
+
+				if (gpio_cfg == WM8903_GPIO_NO_CONFIG)
+					continue;
+
+				snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
+					      gpio_cfg & 0xffff);
+
+				val = (gpio_cfg & WM8903_GP1_FN_MASK)
+					>> WM8903_GP1_FN_SHIFT;
+
+				switch (val) {
+				case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT:
+				case WM8903_GPn_FN_MICBIAS_SHORT_DETECT:
+					mic_gpio = true;
+					break;
+				default:
+					break;
+				}
+			}
+		}
+
+		prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
+		if (prop)
+			wm8903->irq = be32_to_cpup(prop);
+
+		prop = of_get_property(codec->dev->of_node, "micdet-cfg", NULL);
+		if (prop)
+			micdet_cfg = be32_to_cpup(prop);
+
+		snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, micdet_cfg);
+
+		if (micdet_cfg)
+			snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
+					    WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
+		
+		/* If microphone detection is enabled in device tree but
+		 * detected via IRQ then interrupts can be lost before
+		 * the machine driver has set up microphone detection
+		 * IRQs as the IRQs are clear on read.  The detection
+		 * will be enabled when the machine driver configures.
+		 */
+		WARN_ON(!mic_gpio && (micdet_cfg & WM8903_MICDET_ENA));
+
+		prop = of_get_property(codec->dev->of_node, "micdet-delay", NULL);
+		if (prop)
+			wm8903->mic_delay = be32_to_cpup(prop);
+
 	}
 	
 	if (wm8903->irq) {
-		if (pdata && pdata->irq_active_low) {
+		if (pdata) {
+			irq_active_low = pdata->irq_active_low;
+		} else if (codec->dev->of_node) {
+			prop = of_get_property(codec->dev->of_node, "irq-active-low", NULL);
+			if (prop)
+				irq_active_low = be32_to_cpup(prop);
+		}
+
+		if (irq_active_low) {
 			trigger = IRQF_TRIGGER_LOW;
 			irq_pol = WM8903_IRQ_POL;
 		} else {
@@ -2057,6 +2132,16 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8903 = {
 };
 
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+
+#if defined(CONFIG_OF)
+/* Match table for of_platform binding */
+static const struct of_device_id wm8903_of_match[] __devinitconst = {
+        { .compatible = "wlf,wm8903", },
+        {},
+};
+MODULE_DEVICE_TABLE(of, wm8903_of_match);
+#endif
+
 static __devinit int wm8903_i2c_probe(struct i2c_client *i2c,
 				      const struct i2c_device_id *id)
 {
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
index 66b504f..6dc89bc 100644
--- a/sound/soc/tegra/Kconfig
+++ b/sound/soc/tegra/Kconfig
@@ -16,7 +16,7 @@ config SND_TEGRA_SOC_I2S
 
 config SND_TEGRA_SOC_HARMONY
 	tristate "SoC Audio support for Tegra Harmony reference board"
-	depends on SND_TEGRA_SOC && MACH_HARMONY && I2C
+	depends on SND_TEGRA_SOC && (MACH_HARMONY || MACH_TEGRA_DT) && I2C
 	default m
 	select SND_TEGRA_SOC_I2S
 	select SND_SOC_WM8903
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
index 556a571..f225087 100644
--- a/sound/soc/tegra/harmony.c
+++ b/sound/soc/tegra/harmony.c
@@ -292,12 +292,20 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 	struct snd_soc_card *card = &snd_soc_harmony;
 	struct tegra_harmony *harmony;
 	struct harmony_audio_platform_data *pdata;
+	const char *sprop;
 	int ret;
 
+#ifdef CONFIG_OF
+	if ((!of_machine_is_compatible("nvidia,harmony")) && (!machine_is_harmony())) {
+		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
+		return -ENODEV;
+	}
+#else
 	if (!machine_is_harmony()) {
 		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
 		return -ENODEV;
 	}
+#endif
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {

RE: [RFC 1/3] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch changes how sdhci devices are initialized so that a later patch can
> easily add support for i2c devies.
>...
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>...
> +#include <mach/sdhci.h>
>...
> +#include "gpio-names.h"

I don't think either of those headers are needed any more.

Otherwise, looks good.

-- 
nvpublic

[RFC 1/3] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch changes how sdhci devices are initialized so that a later patch can
> easily add support for i2c devies.
>...
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>...
> +#include <mach/sdhci.h>
>...
> +#include "gpio-names.h"

I don't think either of those headers are needed any more.

Otherwise, looks good.

-- 
nvpublic

RE: [RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch initializes i2c controller devices in board-dt.c. The i2c controller
> is added to tegra250.dtsi so later on-board i2c devices can be found and
> initialized based on the device tree information.
>...
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>...
> +#include <linux/i2c.h>
> +#include <linux/i2c-tegra.h>

I don't think those headers are needed now the platform data isn't set up here.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>...
> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	i2c_dev->adapter.algo = &tegra_i2c_algo;
>  	i2c_dev->adapter.dev.parent = &pdev->dev;
>  	i2c_dev->adapter.nr = pdev->id;
> +	i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);

It seems like users of this of_node (i.e. the probe function) could just
access pdev->dev.of_node directly (since pdev is already passed in)
rather than storing a copy here. At least, sdhci-tegra.c works that way.
Still, this isn't a big deal, I think.

> @@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
> 
> +	of_i2c_register_devices(&i2c_dev->adapter);
> +

I would have expected that to be performed inside the core I2C code,
probably inside i2c_add_numbered_adapter? Still, it looks like the
other I2C controllers don't already work that way, so this is probably
more a suggestion for future cleanup than something to be addressed in
this patch.

-- 
nvpublic

[RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch initializes i2c controller devices in board-dt.c. The i2c controller
> is added to tegra250.dtsi so later on-board i2c devices can be found and
> initialized based on the device tree information.
>...
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>...
> +#include <linux/i2c.h>
> +#include <linux/i2c-tegra.h>

I don't think those headers are needed now the platform data isn't set up here.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>...
> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	i2c_dev->adapter.algo = &tegra_i2c_algo;
>  	i2c_dev->adapter.dev.parent = &pdev->dev;
>  	i2c_dev->adapter.nr = pdev->id;
> +	i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);

It seems like users of this of_node (i.e. the probe function) could just
access pdev->dev.of_node directly (since pdev is already passed in)
rather than storing a copy here. At least, sdhci-tegra.c works that way.
Still, this isn't a big deal, I think.

> @@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
> 
> +	of_i2c_register_devices(&i2c_dev->adapter);
> +

I would have expected that to be performed inside the core I2C code,
probably inside i2c_add_numbered_adapter? Still, it looks like the
other I2C controllers don't already work that way, so this is probably
more a suggestion for future cleanup than something to be addressed in
this patch.

-- 
nvpublic

Re: [RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Grant Likely]

On Thu, May 12, 2011 at 6:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
>> This patch initializes i2c controller devices in board-dt.c. The i2c controller
>> is added to tegra250.dtsi so later on-board i2c devices can be found and
>> initialized based on the device tree information.
>>...
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>>...
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-tegra.h>
>
> I don't think those headers are needed now the platform data isn't set up here.
>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>...
>> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>>       i2c_dev->adapter.algo = &tegra_i2c_algo;
>>       i2c_dev->adapter.dev.parent = &pdev->dev;
>>       i2c_dev->adapter.nr = pdev->id;
>> +     i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
>
> It seems like users of this of_node (i.e. the probe function) could just
> access pdev->dev.of_node directly (since pdev is already passed in)
> rather than storing a copy here. At least, sdhci-tegra.c works that way.
> Still, this isn't a big deal, I think.

Actually, using of_node_get() here is the right thing since it
increases the reference count on the of_node.  However, the patch
should also do an of_node_put() in the remove hook, or in the .probe
error path.

>
>> @@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>>               goto err_free_irq;
>>       }
>>
>> +     of_i2c_register_devices(&i2c_dev->adapter);
>> +
>
> I would have expected that to be performed inside the core I2C code,
> probably inside i2c_add_numbered_adapter? Still, it looks like the
> other I2C controllers don't already work that way, so this is probably
> more a suggestion for future cleanup than something to be addressed in
> this patch.

This may move into core code in the future, but for the moment the
drivers need to call it explicitly.

g.

[RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Grant Likely]

On Thu, May 12, 2011 at 6:34 AM, Stephen Warren <swarren@nvidia.com> wrote:
> John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
>> This patch initializes i2c controller devices in board-dt.c. The i2c controller
>> is added to tegra250.dtsi so later on-board i2c devices can be found and
>> initialized based on the device tree information.
>>...
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>>...
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-tegra.h>
>
> I don't think those headers are needed now the platform data isn't set up here.
>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>...
>> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>> ? ? ? i2c_dev->adapter.algo = &tegra_i2c_algo;
>> ? ? ? i2c_dev->adapter.dev.parent = &pdev->dev;
>> ? ? ? i2c_dev->adapter.nr = pdev->id;
>> + ? ? i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
>
> It seems like users of this of_node (i.e. the probe function) could just
> access pdev->dev.of_node directly (since pdev is already passed in)
> rather than storing a copy here. At least, sdhci-tegra.c works that way.
> Still, this isn't a big deal, I think.

Actually, using of_node_get() here is the right thing since it
increases the reference count on the of_node.  However, the patch
should also do an of_node_put() in the remove hook, or in the .probe
error path.

>
>> @@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>> ? ? ? ? ? ? ? goto err_free_irq;
>> ? ? ? }
>>
>> + ? ? of_i2c_register_devices(&i2c_dev->adapter);
>> +
>
> I would have expected that to be performed inside the core I2C code,
> probably inside i2c_add_numbered_adapter? Still, it looks like the
> other I2C controllers don't already work that way, so this is probably
> more a suggestion for future cleanup than something to be addressed in
> this patch.

This may move into core code in the future, but for the moment the
drivers need to call it explicitly.

g.

RE: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch makes it so the wm8903 is initialized from it's device tree
> node.

> diff --git a/arch/arm/boot/dts/tegra-harmony.dts
> b/arch/arm/boot/dts/tegra-harmony.dts
> index af169aa..05521a5 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -19,6 +19,23 @@
>  	i2c@7000c000 {
>  		status = "ok";
>  		clock-frequency = <400000>;
> +
> +		codec: wm8903@1a {
> +			compatible = "wlf,wm8903";
> +			reg = <0x1a>;
> +			interrupts = < 347 >;
> +			irq-active-low = <0>;
> +			micdet-cfg = <0>;
> +			micdet-delay = <100>;

Nit: Maybe group all the WM8903-specific fields together, rather than
spacing them out and interleaving the gpio-controller/gpio-cells in the
middle of them?

> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			gpio-base = < 224 >;
> +			gpio-num-cfg = < 5 >;

I don't think gpio-num-cfg is required; the codec always exports 5 GPIOs.

> +			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
> +			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
> +		};
>  	};
> 
>  	i2c@7000c400 {
> diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
> index f52b623..2347201 100644
> --- a/sound/soc/codecs/wm8903.c
> +++ b/sound/soc/codecs/wm8903.c
> @@ -1865,10 +1865,14 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
>  	wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO;
>  	wm8903->gpio_chip.dev = codec->dev;
> 
> -	if (pdata && pdata->gpio_base)
> +	wm8903->gpio_chip.base = -1;
> +	if (pdata && pdata->gpio_base) {
>  		wm8903->gpio_chip.base = pdata->gpio_base;
> -	else
> -		wm8903->gpio_chip.base = -1;
> +	} else if (codec->dev->of_node) {
> +		prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
> +		if (prop)
> +			wm8903->gpio_chip.base = be32_to_cpup(prop);
> +	}

The above checks for pdata first, then falls back to of_node. However,
The previous i2c-tegra.c patch checks of_node first then falls back to
pdata. It seems like the ordering should be consistent (although
changing the I2C patch might be best, since I imagine checking pdata
first would lead to least compatibility issues)

> @@ -1964,10 +1973,76 @@ static int wm8903_probe(struct snd_soc_codec
> *codec)
>  		WARN_ON(!mic_gpio && (pdata->micdet_cfg & WM8903_MICDET_ENA));
> 
>  		wm8903->mic_delay = pdata->micdet_delay;
> +	} else if (codec->dev->of_node) {
> +		bool mic_gpio = false;
> +
> +		prop = of_get_property(codec->dev->of_node, "gpio-num-cfg", NULL);
> +		if (prop)
> +			num_gpios = be32_to_cpup(prop);

Again, I'd just hard-code num_gpios==WM8903_NUM_GPIO in the
loop below, instead of making it configurable, since the HW is fixed.

> +		prop = of_get_property(codec->dev->of_node, "gpio-cfg", NULL);
> +		if (num_gpios && prop) {
> +			for (i = 0; i < num_gpios; i++) {
> +				gpio_cfg = be32_to_cpu(prop[i]);
> +
> +				if (gpio_cfg == WM8903_GPIO_NO_CONFIG)
> +					continue;
> +
> +				snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
> +					      gpio_cfg & 0xffff);
> +
> +				val = (gpio_cfg & WM8903_GP1_FN_MASK)
> +					>> WM8903_GP1_FN_SHIFT;
> +
> +				switch (val) {
> +				case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT:
> +				case WM8903_GPn_FN_MICBIAS_SHORT_DETECT:
> +					mic_gpio = true;
> +					break;
> +				default:
> +					break;
> +				}
> +			}
> +		}
> +
> +		prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
> +		if (prop)
> +			wm8903->irq = be32_to_cpup(prop);
> +
> +		prop = of_get_property(codec->dev->of_node, "micdet-cfg", NULL);
> +		if (prop)
> +			micdet_cfg = be32_to_cpup(prop);
> +
> +		snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, micdet_cfg);
> +
> +		if (micdet_cfg)
> +			snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
> +					    WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
> +
> +		/* If microphone detection is enabled in device tree but
> +		 * detected via IRQ then interrupts can be lost before
> +		 * the machine driver has set up microphone detection
> +		 * IRQs as the IRQs are clear on read.  The detection
> +		 * will be enabled when the machine driver configures.
> +		 */
> +		WARN_ON(!mic_gpio && (micdet_cfg & WM8903_MICDET_ENA));
> +
> +		prop = of_get_property(codec->dev->of_node, "micdet-delay", NULL);
> +		if (prop)
> +			wm8903->mic_delay = be32_to_cpup(prop);
> +

The above chunk duplicates a lot of code in the pdata and of_node paths.

Instead, perhaps it'd be better to do something more like:

if (!pdata && of_node) {
    pdata = kalloc()
    read of_node, convert & write to pdata
}
if (pdata) {
    // existing code to handle pdata
}

That way, the code to handle the pdata is re-used and not duplicated.

> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
>...
> +#ifdef CONFIG_OF
> +	if ((!of_machine_is_compatible("nvidia,harmony")) &&
> (!machine_is_harmony())) {
> +		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
> +		return -ENODEV;
> +	}
> +#else
>  	if (!machine_is_harmony()) {
>  		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
>  		return -ENODEV;
>  	}
> +#endif

Is that backwards-compatible if CONFIG_OF is enabled, but a devicetree
wasn't provided, but instead the old-style Harmony machine ID was used?
I note that the board-harmony.c machine description doesn't include a
DT_MACHINE_START listing that compatible value; only board-dt.c does
this, and that has a different ARM machine ID.

-- 
nvpublic

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Stephen Warren]

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> This patch makes it so the wm8903 is initialized from it's device tree
> node.

> diff --git a/arch/arm/boot/dts/tegra-harmony.dts
> b/arch/arm/boot/dts/tegra-harmony.dts
> index af169aa..05521a5 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -19,6 +19,23 @@
>  	i2c at 7000c000 {
>  		status = "ok";
>  		clock-frequency = <400000>;
> +
> +		codec: wm8903 at 1a {
> +			compatible = "wlf,wm8903";
> +			reg = <0x1a>;
> +			interrupts = < 347 >;
> +			irq-active-low = <0>;
> +			micdet-cfg = <0>;
> +			micdet-delay = <100>;

Nit: Maybe group all the WM8903-specific fields together, rather than
spacing them out and interleaving the gpio-controller/gpio-cells in the
middle of them?

> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			gpio-base = < 224 >;
> +			gpio-num-cfg = < 5 >;

I don't think gpio-num-cfg is required; the codec always exports 5 GPIOs.

> +			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
> +			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
> +		};
>  	};
> 
>  	i2c at 7000c400 {
> diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
> index f52b623..2347201 100644
> --- a/sound/soc/codecs/wm8903.c
> +++ b/sound/soc/codecs/wm8903.c
> @@ -1865,10 +1865,14 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
>  	wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO;
>  	wm8903->gpio_chip.dev = codec->dev;
> 
> -	if (pdata && pdata->gpio_base)
> +	wm8903->gpio_chip.base = -1;
> +	if (pdata && pdata->gpio_base) {
>  		wm8903->gpio_chip.base = pdata->gpio_base;
> -	else
> -		wm8903->gpio_chip.base = -1;
> +	} else if (codec->dev->of_node) {
> +		prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
> +		if (prop)
> +			wm8903->gpio_chip.base = be32_to_cpup(prop);
> +	}

The above checks for pdata first, then falls back to of_node. However,
The previous i2c-tegra.c patch checks of_node first then falls back to
pdata. It seems like the ordering should be consistent (although
changing the I2C patch might be best, since I imagine checking pdata
first would lead to least compatibility issues)

> @@ -1964,10 +1973,76 @@ static int wm8903_probe(struct snd_soc_codec
> *codec)
>  		WARN_ON(!mic_gpio && (pdata->micdet_cfg & WM8903_MICDET_ENA));
> 
>  		wm8903->mic_delay = pdata->micdet_delay;
> +	} else if (codec->dev->of_node) {
> +		bool mic_gpio = false;
> +
> +		prop = of_get_property(codec->dev->of_node, "gpio-num-cfg", NULL);
> +		if (prop)
> +			num_gpios = be32_to_cpup(prop);

Again, I'd just hard-code num_gpios==WM8903_NUM_GPIO in the
loop below, instead of making it configurable, since the HW is fixed.

> +		prop = of_get_property(codec->dev->of_node, "gpio-cfg", NULL);
> +		if (num_gpios && prop) {
> +			for (i = 0; i < num_gpios; i++) {
> +				gpio_cfg = be32_to_cpu(prop[i]);
> +
> +				if (gpio_cfg == WM8903_GPIO_NO_CONFIG)
> +					continue;
> +
> +				snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
> +					      gpio_cfg & 0xffff);
> +
> +				val = (gpio_cfg & WM8903_GP1_FN_MASK)
> +					>> WM8903_GP1_FN_SHIFT;
> +
> +				switch (val) {
> +				case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT:
> +				case WM8903_GPn_FN_MICBIAS_SHORT_DETECT:
> +					mic_gpio = true;
> +					break;
> +				default:
> +					break;
> +				}
> +			}
> +		}
> +
> +		prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
> +		if (prop)
> +			wm8903->irq = be32_to_cpup(prop);
> +
> +		prop = of_get_property(codec->dev->of_node, "micdet-cfg", NULL);
> +		if (prop)
> +			micdet_cfg = be32_to_cpup(prop);
> +
> +		snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, micdet_cfg);
> +
> +		if (micdet_cfg)
> +			snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
> +					    WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
> +
> +		/* If microphone detection is enabled in device tree but
> +		 * detected via IRQ then interrupts can be lost before
> +		 * the machine driver has set up microphone detection
> +		 * IRQs as the IRQs are clear on read.  The detection
> +		 * will be enabled when the machine driver configures.
> +		 */
> +		WARN_ON(!mic_gpio && (micdet_cfg & WM8903_MICDET_ENA));
> +
> +		prop = of_get_property(codec->dev->of_node, "micdet-delay", NULL);
> +		if (prop)
> +			wm8903->mic_delay = be32_to_cpup(prop);
> +

The above chunk duplicates a lot of code in the pdata and of_node paths.

Instead, perhaps it'd be better to do something more like:

if (!pdata && of_node) {
    pdata = kalloc()
    read of_node, convert & write to pdata
}
if (pdata) {
    // existing code to handle pdata
}

That way, the code to handle the pdata is re-used and not duplicated.

> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
>...
> +#ifdef CONFIG_OF
> +	if ((!of_machine_is_compatible("nvidia,harmony")) &&
> (!machine_is_harmony())) {
> +		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
> +		return -ENODEV;
> +	}
> +#else
>  	if (!machine_is_harmony()) {
>  		dev_err(&pdev->dev, "Not running on Tegra Harmony!\n");
>  		return -ENODEV;
>  	}
> +#endif

Is that backwards-compatible if CONFIG_OF is enabled, but a devicetree
wasn't provided, but instead the old-style Harmony machine ID was used?
I note that the board-harmony.c machine description doesn't include a
DT_MACHINE_START listing that compatible value; only board-dt.c does
this, and that has a different ARM machine ID.

-- 
nvpublic

RE: [RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Stephen Warren]

Grant Likely wrote at Wednesday, May 11, 2011 10:47 PM:
> On Thu, May 12, 2011 at 6:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> >> This patch initializes i2c controller devices in board-dt.c. The i2c controller
> >> is added to tegra250.dtsi so later on-board i2c devices can be found and
> >> initialized based on the device tree information.
> >>...
> >> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >>       i2c_dev->adapter.algo = &tegra_i2c_algo;
> >>       i2c_dev->adapter.dev.parent = &pdev->dev;
> >>       i2c_dev->adapter.nr = pdev->id;
> >> +     i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> >
> > It seems like users of this of_node (i.e. the probe function) could just
> > access pdev->dev.of_node directly (since pdev is already passed in)
> > rather than storing a copy here. At least, sdhci-tegra.c works that way.
> > Still, this isn't a big deal, I think.
> 
> Actually, using of_node_get() here is the right thing since it
> increases the reference count on the of_node.  However, the patch
> should also do an of_node_put() in the remove hook, or in the .probe
> error path.

Ah OK, I was missing that the later all of of_i2c_register_devices()
uses i2c_dev->adapter.dev.of_node; I was thinking that it'd just use
i2c_dev->adapter.dev.parent->of_node directly, in which case we
wouldn't need to copy the pointer, nor increment the refcount.

So, I agree this is OK.

-- 
nvpublic

[RFC 2/3] ARM: Tegra: Device Tree Support: Add i2c devices

[Stephen Warren]

Grant Likely wrote at Wednesday, May 11, 2011 10:47 PM:
> On Thu, May 12, 2011 at 6:34 AM, Stephen Warren <swarren@nvidia.com> wrote:
> > John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:
> >> This patch initializes i2c controller devices in board-dt.c. The i2c controller
> >> is added to tegra250.dtsi so later on-board i2c devices can be found and
> >> initialized based on the device tree information.
> >>...
> >> @@ -598,6 +609,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >> ? ? ? i2c_dev->adapter.algo = &tegra_i2c_algo;
> >> ? ? ? i2c_dev->adapter.dev.parent = &pdev->dev;
> >> ? ? ? i2c_dev->adapter.nr = pdev->id;
> >> + ? ? i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> >
> > It seems like users of this of_node (i.e. the probe function) could just
> > access pdev->dev.of_node directly (since pdev is already passed in)
> > rather than storing a copy here. At least, sdhci-tegra.c works that way.
> > Still, this isn't a big deal, I think.
> 
> Actually, using of_node_get() here is the right thing since it
> increases the reference count on the of_node.  However, the patch
> should also do an of_node_put() in the remove hook, or in the .probe
> error path.

Ah OK, I was missing that the later all of of_i2c_register_devices()
uses i2c_dev->adapter.dev.of_node; I was thinking that it'd just use
i2c_dev->adapter.dev.parent->of_node directly, in which case we
wouldn't need to copy the pointer, nor increment the refcount.

So, I agree this is OK.

-- 
nvpublic

Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Mark Brown]

On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
> This patch makes it so the wm8903 is initialized from it's device tree node.
> 
> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
>  sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--

This needs to be sent separately to the relevant mailing lists and
maintainers.  You can't go making substantial changes to drivers without
even telling the maintainers about it - this will apply to any device
tree work you're doing.  In this case one of the maintainers happens to
be me, but even so.

> +			interrupts = < 347 >;
> +			irq-active-low = <0>;
> +			micdet-cfg = <0>;
> +			micdet-delay = <100>;

Some of this looks like chip default, why is it being configured?

> +                       gpio-controller;
> +                       #gpio-cells = <2>;

The fact that this device is a GPIO controller is a physical property of
the device, we shouldn't need to be putting it into the device tree.

> +			gpio-num-cfg = < 5 >;

Similarly here, the device has a fixed number of GPIOs.

> +			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
> +			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;

This doesn't seem great for usability.  I'd suggest key/value pairs
rather than an array.

> -	if (pdata && pdata->gpio_base)
> +	wm8903->gpio_chip.base = -1;
> +	if (pdata && pdata->gpio_base) {
>  		wm8903->gpio_chip.base = pdata->gpio_base;
> -	else
> -		wm8903->gpio_chip.base = -1;
> +	} else if (codec->dev->of_node) {
> +		prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
> +		if (prop)
> +			wm8903->gpio_chip.base = be32_to_cpup(prop);
> +	}

We have to do manual endianness conversions to read from the device
tree?  Oh, well.

> +
> +		prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
> +		if (prop)
> +			wm8903->irq = be32_to_cpup(prop);
> +

We have a standard way of passing the IRQ number to I2C devices, surely
we should make sure that works at the bus level rather than having to
replicate this code everywhere?

> +		prop = of_get_property(codec->dev->of_node, "micdet-cfg", NULL);
> +		if (prop)
> +			micdet_cfg = be32_to_cpup(prop);
> +
> +		snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, micdet_cfg);
> +
> +		if (micdet_cfg)
> +			snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
> +					    WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
> +		
> +		/* If microphone detection is enabled in device tree but
> +		 * detected via IRQ then interrupts can be lost before
> +		 * the machine driver has set up microphone detection
> +		 * IRQs as the IRQs are clear on read.  The detection
> +		 * will be enabled when the machine driver configures.
> +		 */
> +		WARN_ON(!mic_gpio && (micdet_cfg & WM8903_MICDET_ENA));
> +

There's an awful lot of cut'n'paste code for the parsing code from the
platform data code.

>  config SND_TEGRA_SOC_HARMONY
>  	tristate "SoC Audio support for Tegra Harmony reference board"
> -	depends on SND_TEGRA_SOC && MACH_HARMONY && I2C
> +	depends on SND_TEGRA_SOC && (MACH_HARMONY || MACH_TEGRA_DT) && I2C

You've not added anything to the device tree to register the platform
device for the machine and I rather fear this won't apply to current
code.  You should develop against -next.

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Mark Brown]

On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
> This patch makes it so the wm8903 is initialized from it's device tree node.
> 
> Signed-off-by: John Bonesio<bones@secretlab.ca>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
>  sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--

This needs to be sent separately to the relevant mailing lists and
maintainers.  You can't go making substantial changes to drivers without
even telling the maintainers about it - this will apply to any device
tree work you're doing.  In this case one of the maintainers happens to
be me, but even so.

> +			interrupts = < 347 >;
> +			irq-active-low = <0>;
> +			micdet-cfg = <0>;
> +			micdet-delay = <100>;

Some of this looks like chip default, why is it being configured?

> +                       gpio-controller;
> +                       #gpio-cells = <2>;

The fact that this device is a GPIO controller is a physical property of
the device, we shouldn't need to be putting it into the device tree.

> +			gpio-num-cfg = < 5 >;

Similarly here, the device has a fixed number of GPIOs.

> +			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
> +			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;

This doesn't seem great for usability.  I'd suggest key/value pairs
rather than an array.

> -	if (pdata && pdata->gpio_base)
> +	wm8903->gpio_chip.base = -1;
> +	if (pdata && pdata->gpio_base) {
>  		wm8903->gpio_chip.base = pdata->gpio_base;
> -	else
> -		wm8903->gpio_chip.base = -1;
> +	} else if (codec->dev->of_node) {
> +		prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
> +		if (prop)
> +			wm8903->gpio_chip.base = be32_to_cpup(prop);
> +	}

We have to do manual endianness conversions to read from the device
tree?  Oh, well.

> +
> +		prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
> +		if (prop)
> +			wm8903->irq = be32_to_cpup(prop);
> +

We have a standard way of passing the IRQ number to I2C devices, surely
we should make sure that works at the bus level rather than having to
replicate this code everywhere?

> +		prop = of_get_property(codec->dev->of_node, "micdet-cfg", NULL);
> +		if (prop)
> +			micdet_cfg = be32_to_cpup(prop);
> +
> +		snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, micdet_cfg);
> +
> +		if (micdet_cfg)
> +			snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
> +					    WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
> +		
> +		/* If microphone detection is enabled in device tree but
> +		 * detected via IRQ then interrupts can be lost before
> +		 * the machine driver has set up microphone detection
> +		 * IRQs as the IRQs are clear on read.  The detection
> +		 * will be enabled when the machine driver configures.
> +		 */
> +		WARN_ON(!mic_gpio && (micdet_cfg & WM8903_MICDET_ENA));
> +

There's an awful lot of cut'n'paste code for the parsing code from the
platform data code.

>  config SND_TEGRA_SOC_HARMONY
>  	tristate "SoC Audio support for Tegra Harmony reference board"
> -	depends on SND_TEGRA_SOC && MACH_HARMONY && I2C
> +	depends on SND_TEGRA_SOC && (MACH_HARMONY || MACH_TEGRA_DT) && I2C

You've not added anything to the device tree to register the platform
device for the machine and I rather fear this won't apply to current
code.  You should develop against -next.

Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Grant Likely]

On Thu, May 12, 2011 at 1:49 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
>> -     if (pdata && pdata->gpio_base)
>> +     wm8903->gpio_chip.base = -1;
>> +     if (pdata && pdata->gpio_base) {
>>               wm8903->gpio_chip.base = pdata->gpio_base;
>> -     else
>> -             wm8903->gpio_chip.base = -1;
>> +     } else if (codec->dev->of_node) {
>> +             prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
>> +             if (prop)
>> +                     wm8903->gpio_chip.base = be32_to_cpup(prop);
>> +     }
>
> We have to do manual endianness conversions to read from the device
> tree?  Oh, well.

Yeah, it's not great.  I'd really like to have a set of helper
functions that locate and decode the data for the most common types of
properties, but I've not sat down to focus on it and I've not seen a
pattern I'm happy with yet.  It does need to be solved though.  The
issue is I want something that handles fetching the property, error
checking and decoding, all in a consistent way so that it actually
reduces the amount of parsing code required.

Perhaps something like:

/* Not sure about this; in this case the return value is an error code
so that bad properties can be detected */
int dt_decode_cell(struct *property, int index, u32 *out_val);
int dt_decode_string(struct *property, int index, char **out_val);
int dt_decode_number(struct *property, int start, int len, unsigned
long long *out_val);

and perhaps a set of companion functions:
int dt_get_prop_cell(struct *device_node, char *propname, int index,
u32 *out_val);
int dt_get_prop_string(struct *device_node, char *propname, int index,
char **out_val);
int dt_get_prop_number(struct *device_node, char *propname, int start,
int len, unsigned long long *out_val);

The first set would be used when the property pointer has already be
obtained, and multiple datum need to be extracted.  The second would
be most useful when only one value will be extracted from a property.
I'm not totally happy with returning the value via a passed in pointer
though.  Thoughts?

g.

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Grant Likely]

On Thu, May 12, 2011 at 1:49 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
>> - ? ? if (pdata && pdata->gpio_base)
>> + ? ? wm8903->gpio_chip.base = -1;
>> + ? ? if (pdata && pdata->gpio_base) {
>> ? ? ? ? ? ? ? wm8903->gpio_chip.base = pdata->gpio_base;
>> - ? ? else
>> - ? ? ? ? ? ? wm8903->gpio_chip.base = -1;
>> + ? ? } else if (codec->dev->of_node) {
>> + ? ? ? ? ? ? prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
>> + ? ? ? ? ? ? if (prop)
>> + ? ? ? ? ? ? ? ? ? ? wm8903->gpio_chip.base = be32_to_cpup(prop);
>> + ? ? }
>
> We have to do manual endianness conversions to read from the device
> tree? ?Oh, well.

Yeah, it's not great.  I'd really like to have a set of helper
functions that locate and decode the data for the most common types of
properties, but I've not sat down to focus on it and I've not seen a
pattern I'm happy with yet.  It does need to be solved though.  The
issue is I want something that handles fetching the property, error
checking and decoding, all in a consistent way so that it actually
reduces the amount of parsing code required.

Perhaps something like:

/* Not sure about this; in this case the return value is an error code
so that bad properties can be detected */
int dt_decode_cell(struct *property, int index, u32 *out_val);
int dt_decode_string(struct *property, int index, char **out_val);
int dt_decode_number(struct *property, int start, int len, unsigned
long long *out_val);

and perhaps a set of companion functions:
int dt_get_prop_cell(struct *device_node, char *propname, int index,
u32 *out_val);
int dt_get_prop_string(struct *device_node, char *propname, int index,
char **out_val);
int dt_get_prop_number(struct *device_node, char *propname, int start,
int len, unsigned long long *out_val);

The first set would be used when the property pointer has already be
obtained, and multiple datum need to be extracted.  The second would
be most useful when only one value will be extracted from a property.
I'm not totally happy with returning the value via a passed in pointer
though.  Thoughts?

g.

Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Mark Brown]

On Thu, Jun 02, 2011 at 10:46:18AM -0600, Grant Likely wrote:

> Yeah, it's not great.  I'd really like to have a set of helper
> functions that locate and decode the data for the most common types of
> properties, but I've not sat down to focus on it and I've not seen a
> pattern I'm happy with yet.  It does need to be solved though.  The

Yes, looking at the examples you've got below it looks like you've got a
similar set of issues to the ones that I've been running into trying to
factor the register map access code out of ASoC.

> Perhaps something like:

> /* Not sure about this; in this case the return value is an error code
> so that bad properties can be detected */
> int dt_decode_cell(struct *property, int index, u32 *out_val);
> int dt_decode_string(struct *property, int index, char **out_val);
> int dt_decode_number(struct *property, int start, int len, unsigned
> long long *out_val);

> and perhaps a set of companion functions:
> int dt_get_prop_cell(struct *device_node, char *propname, int index,
> u32 *out_val);
> int dt_get_prop_string(struct *device_node, char *propname, int index,
> char **out_val);
> int dt_get_prop_number(struct *device_node, char *propname, int start,
> int len, unsigned long long *out_val);

> The first set would be used when the property pointer has already be
> obtained, and multiple datum need to be extracted.  The second would
> be most useful when only one value will be extracted from a property.
> I'm not totally happy with returning the value via a passed in pointer
> though.  Thoughts?

Yes, I think this sort of format with the error out of band from the
number is best if a little ugly, and especially the second set is
definitely going to cut down the amount of boiler plate code.  It's a
little painful having to pass a pointer to the result but otherwise the
error handling gets a bit rubbish, especially for the numbers where
there really isn't a sensible invalid value.

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Mark Brown]

On Thu, Jun 02, 2011 at 10:46:18AM -0600, Grant Likely wrote:

> Yeah, it's not great.  I'd really like to have a set of helper
> functions that locate and decode the data for the most common types of
> properties, but I've not sat down to focus on it and I've not seen a
> pattern I'm happy with yet.  It does need to be solved though.  The

Yes, looking at the examples you've got below it looks like you've got a
similar set of issues to the ones that I've been running into trying to
factor the register map access code out of ASoC.

> Perhaps something like:

> /* Not sure about this; in this case the return value is an error code
> so that bad properties can be detected */
> int dt_decode_cell(struct *property, int index, u32 *out_val);
> int dt_decode_string(struct *property, int index, char **out_val);
> int dt_decode_number(struct *property, int start, int len, unsigned
> long long *out_val);

> and perhaps a set of companion functions:
> int dt_get_prop_cell(struct *device_node, char *propname, int index,
> u32 *out_val);
> int dt_get_prop_string(struct *device_node, char *propname, int index,
> char **out_val);
> int dt_get_prop_number(struct *device_node, char *propname, int start,
> int len, unsigned long long *out_val);

> The first set would be used when the property pointer has already be
> obtained, and multiple datum need to be extracted.  The second would
> be most useful when only one value will be extracted from a property.
> I'm not totally happy with returning the value via a passed in pointer
> though.  Thoughts?

Yes, I think this sort of format with the error out of band from the
number is best if a little ugly, and especially the second set is
definitely going to cut down the amount of boiler plate code.  It's a
little painful having to pass a pointer to the result but otherwise the
error handling gets a bit rubbish, especially for the numbers where
there really isn't a sensible invalid value.

Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Grant Likely]

On Thu, May 12, 2011 at 1:49 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
>> This patch makes it so the wm8903 is initialized from it's device tree node.
>>
>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> ---
>>
>>  arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
>>  sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--
>
> This needs to be sent separately to the relevant mailing lists and
> maintainers.  You can't go making substantial changes to drivers without
> even telling the maintainers about it - this will apply to any device
> tree work you're doing.  In this case one of the maintainers happens to
> be me, but even so.
>
>> +                     interrupts = < 347 >;
>> +                     irq-active-low = <0>;
>> +                     micdet-cfg = <0>;
>> +                     micdet-delay = <100>;
>
> Some of this looks like chip default, why is it being configured?
>
>> +                       gpio-controller;
>> +                       #gpio-cells = <2>;
>
> The fact that this device is a GPIO controller is a physical property of
> the device, we shouldn't need to be putting it into the device tree.
>
>> +                     gpio-num-cfg = < 5 >;
>
> Similarly here, the device has a fixed number of GPIOs.
>
>> +                     /* #define WM8903_GPIO_NO_CONFIG 0x8000 */
>> +                     gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
>
> This doesn't seem great for usability.  I'd suggest key/value pairs
> rather than an array.
>
>> -     if (pdata && pdata->gpio_base)
>> +     wm8903->gpio_chip.base = -1;
>> +     if (pdata && pdata->gpio_base) {
>>               wm8903->gpio_chip.base = pdata->gpio_base;
>> -     else
>> -             wm8903->gpio_chip.base = -1;
>> +     } else if (codec->dev->of_node) {
>> +             prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
>> +             if (prop)
>> +                     wm8903->gpio_chip.base = be32_to_cpup(prop);
>> +     }
>
> We have to do manual endianness conversions to read from the device
> tree?  Oh, well.
>
>> +
>> +             prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
>> +             if (prop)
>> +                     wm8903->irq = be32_to_cpup(prop);
>> +
>
> We have a standard way of passing the IRQ number to I2C devices, surely
> we should make sure that works at the bus level rather than having to
> replicate this code everywhere?

Yes actually there is.  The code in drivers/of/of_i2c.c already
correctly populates the i2c_client irq from the device tree.  This
hunk can be completely removed.

g.

[RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Grant Likely]

On Thu, May 12, 2011 at 1:49 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
>> This patch makes it so the wm8903 is initialized from it's device tree node.
>>
>> Signed-off-by: John Bonesio<bones@secretlab.ca>
>> ---
>>
>> ?arch/arm/boot/dts/tegra-harmony.dts | ? 17 ++++++
>> ?sound/soc/codecs/wm8903.c ? ? ? ? ? | ? 93 +++++++++++++++++++++++++++++++++--
>
> This needs to be sent separately to the relevant mailing lists and
> maintainers. ?You can't go making substantial changes to drivers without
> even telling the maintainers about it - this will apply to any device
> tree work you're doing. ?In this case one of the maintainers happens to
> be me, but even so.
>
>> + ? ? ? ? ? ? ? ? ? ? interrupts = < 347 >;
>> + ? ? ? ? ? ? ? ? ? ? irq-active-low = <0>;
>> + ? ? ? ? ? ? ? ? ? ? micdet-cfg = <0>;
>> + ? ? ? ? ? ? ? ? ? ? micdet-delay = <100>;
>
> Some of this looks like chip default, why is it being configured?
>
>> + ? ? ? ? ? ? ? ? ? ? ? gpio-controller;
>> + ? ? ? ? ? ? ? ? ? ? ? #gpio-cells = <2>;
>
> The fact that this device is a GPIO controller is a physical property of
> the device, we shouldn't need to be putting it into the device tree.
>
>> + ? ? ? ? ? ? ? ? ? ? gpio-num-cfg = < 5 >;
>
> Similarly here, the device has a fixed number of GPIOs.
>
>> + ? ? ? ? ? ? ? ? ? ? /* #define WM8903_GPIO_NO_CONFIG 0x8000 */
>> + ? ? ? ? ? ? ? ? ? ? gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
>
> This doesn't seem great for usability. ?I'd suggest key/value pairs
> rather than an array.
>
>> - ? ? if (pdata && pdata->gpio_base)
>> + ? ? wm8903->gpio_chip.base = -1;
>> + ? ? if (pdata && pdata->gpio_base) {
>> ? ? ? ? ? ? ? wm8903->gpio_chip.base = pdata->gpio_base;
>> - ? ? else
>> - ? ? ? ? ? ? wm8903->gpio_chip.base = -1;
>> + ? ? } else if (codec->dev->of_node) {
>> + ? ? ? ? ? ? prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
>> + ? ? ? ? ? ? if (prop)
>> + ? ? ? ? ? ? ? ? ? ? wm8903->gpio_chip.base = be32_to_cpup(prop);
>> + ? ? }
>
> We have to do manual endianness conversions to read from the device
> tree? ?Oh, well.
>
>> +
>> + ? ? ? ? ? ? prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
>> + ? ? ? ? ? ? if (prop)
>> + ? ? ? ? ? ? ? ? ? ? wm8903->irq = be32_to_cpup(prop);
>> +
>
> We have a standard way of passing the IRQ number to I2C devices, surely
> we should make sure that works at the bus level rather than having to
> replicate this code everywhere?

Yes actually there is.  The code in drivers/of/of_i2c.c already
correctly populates the i2c_client irq from the device tree.  This
hunk can be completely removed.

g.