ARM: Tegra: Device Tree: i2c & wm8903

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:

arch/arm/mach-tegra/board-dt.c line 32

Quoted 8 lines

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.
...

arch/arm/mach-tegra/board-dt.c +2 -0

...
+#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.

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_probe(struct platform_device *pdev) · line 612

Quoted 7 lines

drivers/i2c/busses/i2c-tegra.c +1 -0

...

@@ -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.

Quoted 5 lines

drivers/i2c/busses/i2c-tegra.c +0 -0

@@ -605,6 +617,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 ? ? ? ? ? ? ? goto err_free_irq;
 ? ? ? }
 

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_probe(struct platform_device *pdev) · line 621

Quoted 2 lines

drivers/i2c/busses/i2c-tegra.c +2 -0

+ ? ? 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.

Quoted 15 lines

John Bonesio wrote at Wednesday, May 11, 2011 5:27 PM:

Review static int tegra_i2c_probe(struct platform_device *pdev) · line 612

Quoted 9 lines

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.
...

(quoted diff) +1 -0

@@ -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.

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

arch/arm/boot/dts/tegra-harmony.dts +0 -0

index af169aa..05521a5 100644
@@ -19,6 +19,23 @@
 	i2c at 7000c000 {

arch/arm/boot/dts/tegra-harmony.dts +3 -0

+			/* #define WM8903_GPIO_NO_CONFIG 0x8000 */
+			gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
+		};
 	};
 
 	i2c at 7000c400 {

sound/soc/codecs/wm8903.c +0 -1

index f52b623..2347201 100644
@@ -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)

sound/soc/codecs/wm8903.c +41 -0

+		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

sound/soc/tegra/harmony.c +2 -0

...
+#ifdef CONFIG_OF
+	if ((!of_machine_is_compatible("nvidia,harmony")) &&

(!machine_is_harmony())) {

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

Signed-off-by John Bonesio<[email protected]>

---

 arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
 sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--

sound/soc/codecs/wm8903.c +8 -0

+		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,

Quoted 11 lines · John Bonesio

- ? ? 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

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?

Quoted 7 lines · John Bonesio

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

Signed-off-by John Bonesio<[email protected]>

---

?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.

Quoted 4 lines

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

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

Quoted 2 lines

+ ? ? ? ? ? ? ? ? ? ? ? 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.

Quoted 1 line

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

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

Quoted 2 lines

+ ? ? ? ? ? ? ? ? ? ? /* #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.

Quoted 11 lines

- ? ? 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.

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+ ? ? ? ? ? ? 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?