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

[Stephen Warren]

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

-- 
nvpublic