Re: [PATCH 2/3] ARM: mach-moxart: add MOXA ART device tree files

[Jonas Jensen]

Hi,

Thanks for the replies.

What isn't commented below should already be fixed.

On 13 June 2013 00:49, Olof Johansson <olof@xxxxxxxxx> wrote:
> Hi,
>
> You should add a bindings description for the MOXA SoC platforms, similar to
> how others do it, under Documentation/devicetree/bindings/arm/.

The following is now added under Documentation:

 Documentation/devicetree/bindings/arm/moxart.txt   |    8 +++
 .../arm/moxart/moxart-interrupt-controller.txt     |   29 ++++++++++++
 .../bindings/arm/moxart/moxart-timer.txt           |   17 +++++++

> Same goes for other device bindings below -- they all need to be added to the
> bindings documentation. You might be better off removing some of them until you
> post and merge the corresponding drivers.

I'll remove them for now with the intent of adding them later. Maybe
when (and if) all drivers are posted and merged.

>> +/dts-v1/;
>> +/include/ "moxart.dtsi"
>> +
>> +/ {
>> +     model = "MOXA UC-7112-LX";
>> +     compatible = "moxa,moxart-uc-7112-lx";
>
> Is there a generic board design / eval board that you can have as a fallback
> compatible value? That way you won't have to add every board to the table in
> the c file.

There isn't really a generic board but the same can be accomplished by
adding "moxa,moxart" to compatible? New boards can then be added
without patching arch/arm/mach-moxart/moxart.c.

>> +     mxser@98200040 {
>> +             compatible = "moxa,moxart-mxser";
>> +             reg =   <0x98200040 0x00000080>,        /* UART "3" base */
>> +                             <0x982000e4 0x00000080>,        /* UART mode base */
>> +                             <0x982000c0 0x00000020>;        /* UART interrupt vector */
>
> Are there other registers for other devices inbetween, or could you just do one
> large memory area here?

No, reg could simply be 0x98200040 to 0x982000e0. I split them out as
documentation but those could be defined in the driver instead, also
because it's sort of easier to assign values with of_iomap.

>> +             interrupts = <31 1>;
>> +     };
>> +
>> +     mac0: mac@90900000 {
>> +             compatible = "moxa,moxart-mac0";
>> +             reg =   <0x90900000 0x1000>,
>> +                             <0x80000050 0x5>;                       /* MAC address stored on flash */
>
> This is a pretty unusal way to indicate mac address location. While I suppose
> it works, I'd like to get the device tree maintainers in the loop. ADding them
> to cc.
>
> Also, there should be a bindings doc for this device (and a driver)

I'm removing ethernet until the driver is submitted. This allows me to
submit bindings doc later?

> I'm also surprised that you have a 5-byte mac address. 6 bytes is much more
> common. :)

That _is_ surprising :) Corrected to 6 bytes.

> Finally, are the two MACs using the same driver? if so, using the same
> compatible value makes more sense.

Yes, using the same driver is the point. I see now they can use the
same value, they'll still be probed from the DT entries.


Best regards,
Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/