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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACmBeS0T1n92CcGN6kR+eBcFPuwsWc+PsUx7Cms+4vbeVFhQKg@mail.gmail.com>
Date:	Fri, 14 Jun 2013 16:34:24 +0200
From:	Jonas Jensen <jonas.jensen@...il.com>
To:	Olof Johansson <olof@...om.net>
Cc:	linux-arm-kernel@...ts.infradead.org, linux@....linux.org.uk,
	linux-kernel@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	arm@...nel.org
Subject: Re: [PATCH 2/3] ARM: mach-moxart: add MOXA ART device tree files

Hi,

Thanks for the replies.

What isn't commented below should already be fixed.

On 13 June 2013 00:49, Olof Johansson <olof@...om.net> 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@...00040 {
>> +             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@...00000 {
>> +             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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/