Re: [alsa-devel] [PATCH] ASoC: Add device tree binding for WM8903

Mark Brown wrote at Thursday, December 01, 2011 6:26 AM:

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

Sorry, I forgot to move it this time.

...

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

The first patch set did that, and you objected to how it was structured...

...

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

OK, I could remove the platform data's irq_active_low flag, and determine the IRQ polarity this way in all cases. Do you want that? I avoided doing it this time around since it'd affect non-device-tree users of the codec too, and I aimed for complete backwards compatibility (although that said, the only in-tree user is the Tegra machines, and they don't set this flag, so the impact would be minimal in-tree)

...

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

I'll just drop the ifdefs; looking at all the other codecs, they don't have ifdefs anyway - I thought I added the ifdefs to be consistent, but I must have checked something other than ASoC codecs...