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

From: Jonas Jensen
Date: Fri Jun 14 2013 - 10:34:30 EST


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,
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at