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