Re: [RFC 3/3] ARM:Tegra: Device Tree Support: Initialize from wm8903 the device tree

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Thu, May 12, 2011 at 1:49 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, May 11, 2011 at 04:27:18PM -0700, John Bonesio wrote:
>> This patch makes it so the wm8903 is initialized from it's device tree node.
>>
>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> ---
>>
>>  arch/arm/boot/dts/tegra-harmony.dts |   17 ++++++
>>  sound/soc/codecs/wm8903.c           |   93 +++++++++++++++++++++++++++++++++--
>
> This needs to be sent separately to the relevant mailing lists and
> maintainers.  You can't go making substantial changes to drivers without
> even telling the maintainers about it - this will apply to any device
> tree work you're doing.  In this case one of the maintainers happens to
> be me, but even so.
>
>> +                     interrupts = < 347 >;
>> +                     irq-active-low = <0>;
>> +                     micdet-cfg = <0>;
>> +                     micdet-delay = <100>;
>
> Some of this looks like chip default, why is it being configured?
>
>> +                       gpio-controller;
>> +                       #gpio-cells = <2>;
>
> The fact that this device is a GPIO controller is a physical property of
> the device, we shouldn't need to be putting it into the device tree.
>
>> +                     gpio-num-cfg = < 5 >;
>
> Similarly here, the device has a fixed number of GPIOs.
>
>> +                     /* #define WM8903_GPIO_NO_CONFIG 0x8000 */
>> +                     gpio-cfg = < 0x8000 0x8000 0 0x8000 0x8000 >;
>
> This doesn't seem great for usability.  I'd suggest key/value pairs
> rather than an array.
>
>> -     if (pdata && pdata->gpio_base)
>> +     wm8903->gpio_chip.base = -1;
>> +     if (pdata && pdata->gpio_base) {
>>               wm8903->gpio_chip.base = pdata->gpio_base;
>> -     else
>> -             wm8903->gpio_chip.base = -1;
>> +     } else if (codec->dev->of_node) {
>> +             prop = of_get_property(codec->dev->of_node, "gpio-base", NULL);
>> +             if (prop)
>> +                     wm8903->gpio_chip.base = be32_to_cpup(prop);
>> +     }
>
> We have to do manual endianness conversions to read from the device
> tree?  Oh, well.
>
>> +
>> +             prop = of_get_property(codec->dev->of_node, "interrupts", NULL);
>> +             if (prop)
>> +                     wm8903->irq = be32_to_cpup(prop);
>> +
>
> We have a standard way of passing the IRQ number to I2C devices, surely
> we should make sure that works at the bus level rather than having to
> replicate this code everywhere?

Yes actually there is.  The code in drivers/of/of_i2c.c already
correctly populates the i2c_client irq from the device tree.  This
hunk can be completely removed.

g.