[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