Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree
From: Mark Brown <hidden>
Date: 2011-05-12 07:49:19
Also in:
linux-arm-kernel, linux-tegra
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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
+ + 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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.