On Wed, Nov 30, 2011 at 05:47:35PM -0700, Stephen Warren wrote:
> v2: (swarren) Significantly reworked based on review feedback.
> v1: (swarren) Applied the following modifications relative to John's code:
> * Cleaned up DT parsing code
> * Documented DT binding
> * Set up wm8903->gpio_chip.of_node, extracted from another patch by John.
Don't put stuff like this in the changelog. "Significantly reworked"
isn't going to help anyone reading the logs...
> + micdet-cfg = <0>;
> + micdet-delay = <100>;
> + gpio-cfg = < 0xffffffff 0xffffffff 0 0xffffffff 0xffffffff >;
Better to have sane examples for gpio-cfg.
> -static void wm8903_init_gpio(struct snd_soc_codec *codec)
> +static void wm8903_init_gpio(struct snd_soc_codec *codec,
> + int gpio_base)
> {
Can you do this restructuring separately please? The diff is *much*
larger than I'd expect and stuff liket his isn't helping.
> +#ifdef CONFIG_OF_GPIO
> + wm8903->gpio_chip.of_node = codec->dev->of_node;
> +#endif
Ick, ifdefs mid code :(
>
> @@ -1931,18 +1942,65 @@ static int wm8903_probe(struct snd_soc_codec *codec)
>
> wm8903_reset(codec);
>
> - /* Set up GPIOs and microphone detection */
> + /* Get configuration from platform data, or device tree */
> if (pdata) {
> - bool mic_gpio = false;
> + have_pdata = true;
> + gpio_base = pdata->gpio_base;
> + irq_active_low = pdata->irq_active_low;
> + micdet_cfg = pdata->micdet_cfg;
> + wm8903->mic_delay = pdata->micdet_delay;
> + gpio_cfg = &pdata->gpio_cfg[0];
> + } else if (codec->dev->of_node) {
> + have_pdata = true;
This all seems really complicated and invasive, especially the
have_pdata flag. Why not just always have a platform data structure and
fill it in from the device tree?
> + if (wm8903->irq) {
> + irq_data = irq_get_irq_data(wm8903->irq);
> + if (!irq_data) {
> + dev_err(codec->dev, "Invalid IRQ: %d\n",
> + wm8903->irq);
> + return -EINVAL;
> + }
> + trigger = irqd_get_trigger_type(irq_data);
> + switch (trigger) {
> + case IRQ_TYPE_NONE:
> + case IRQ_TYPE_LEVEL_HIGH:
> + irq_active_low = false;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + irq_active_low = true;
> + break;
> + default:
> + dev_err(codec->dev,
> + "Unsupported IRQ trigger: %x\n",
> + trigger);
> + return -EINVAL;
> + }
> + }
This stuff isn't device tree specific.
> + /* Set up GPIOs */
> + if (gpio_cfg) {
> + for (i = 0; i < WM8903_NUM_GPIO; i++) {
> + if (gpio_cfg[i] > 0x7fff)
> continue;
>
> snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
> - pdata->gpio_cfg[i] & 0xffff);
> + gpio_cfg[i] & 0x7fff);
This is a separate change to the semantics and should be split out.
> +#ifdef 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);
> +#else
> +#define wm8903_of_match NULL
> +#endif
If you're going to do this then go through and consistently add the
defines. I'd also be inclined to drop the comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
