Re: [PATCH 07/15] [POWERPC] Promess Motion-PRO DTS

["Grant Likely" <grant.likely@secretlab.ca>]

Hey Scott.

Pretty much all your comments below directly apply to the existing
Lite5200 device tree and the Efika firmware.  These are issues that
were created a while ago and I've never gone back to clean up the
mpc5200 device tree bindings.  (Plus we need to have code to maintian
compatibility with the Efika firmware.

g.

On 10/8/07, Scott Wood <scottwood@freescale.com> wrote:
> On Sun, Oct 07, 2007 at 01:25:33PM +0200, Marian Balakowicz wrote:
> > +             gpt@600 {       // General Purpose Timer
> > +                     compatible = "mpc5200b-gpt\0mpc5200-gpt";
> > +                     device_type = "gpt";
>
> "timer" would be a better node name than "gpt", and the device type should be
> left out entirely.  As others pointed out, compatible should be
> "fsl,mpc5200b-gpt", "fsl,mpc5200-gpt".
>
> > +                     has-wdt;
>
> fsl,has-wdt
>
> > +             rtc@800 {       // Real time clock
> > +                     compatible = "mpc5200b-rtc\0mpc5200-rtc";
> > +                     device_type = "rtc";
>
> This doesn't actually implement the OF rtc interface...
>
> > +             mscan@980 {
>
> What is mscan?
>
> > +                     device_type = "mscan";
>
> This is not a standard device type.
>
> > +             bestcomm@1200 {
> > +                     device_type = "dma-controller";
>
> dma-controller should be the node name, and device_type should be omitted.
>
> > +             ethernet@3000 {
> > +                     device_type = "network";
> > +                     compatible = "mpc5200b-fec\0mpc5200-fec";
> > +                     reg = <3000 800>;
> > +                     mac-address = [ 02 03 04 05 06 07 ]; // Bad!
>
> Should be local-mac-address.
> And yes, hardcoding a mac address is bad.  Don't do it. :-)
>
> > +             i2c@3d40 {
> > +                     device_type = "i2c";
> > +                     compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
> > +                     cell-index = <1>;
>
> What is cell-index?  The fsl-i2c driver doesn't use it AFAICT.
>
> > +             sram@8000 {
> > +                     device_type = "sram";
>
> No device type.
>
> > +     cpld {
> > +             device_type = "cpld";
> > +             compatible = "cpld";
> > +             reg = <50010000 ffff>;
> > +     };
>
> This device is compatible with every CPLD that has ever existed?  Wow! :-)
>
> -Scott
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195