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

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.