[PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support

[Scott Wood]

On Thu, 2013-09-26 at 04:27 -0500, Xie Xiaobo-R63061 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, September 26, 2013 7:10 AM
> > To: Xie Xiaobo-R63061
> > Cc: Wood Scott-B07421; linuxppc-dev at lists.ozlabs.org; Johnston Michael-
> > R49610
> > Subject: Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support
> > 
> > On Wed, 2013-09-25 at 04:50 -0500, Xie Xiaobo-R63061 wrote:
> > > Hi Scott,
> > >
> > > See the reply inline.
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 25, 2013 7:22 AM
> > > > To: Xie Xiaobo-R63061
> > > > Cc: linuxppc-dev at lists.ozlabs.org; Johnston Michael-R49610
> > > > Subject: Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board
> > > > support
> > > >
> > > > On Tue, 2013-09-24 at 18:48 +0800, Xie Xiaobo wrote:
> > > > > +		partition at 80000 {
> > > > > +			/* 3.5 MB for Linux Kernel Image */
> > > > > +			reg = <0x00080000 0x00380000>;
> > > > > +			label = "NOR Linux Kernel Image";
> > > > > +		};
> > > >
> > > > Is this enough?
> > >
> > > I will enlarge it to 6MB.
> > >
> > > >
> > > > > +		partition at 400000 {
> > > > > +			/* 58.75MB for JFFS2 based Root file System */
> > > > > +			reg = <0x00400000 0x03ac0000>;
> > > > > +			label = "NOR Root File System";
> > > > > +		};
> > > >
> > > > Don't specify jffs2.
> > >
> > > OK, I will remove "jffs2"
> > >
> > > >
> > > > > +	/* CS2 for Display */
> > > > > +	ssd1289 at 2,0 {
> > > > > +		#address-cells = <1>;
> > > > > +		#size-cells = <1>;
> > > > > +		compatible = "ssd1289";
> > > > > +		reg = <0x2 0x0000 0x0002
> > > > > +		       0x2 0x0002 0x0002>;
> > > > > +	};
> > > >
> > > > Node names should be generic.  What does ssd1289 do?  If this is
> > > > actually the display device, then it should be called "display at 2,0".
> > >
> > > OK. The ssd1289 is a LCD controller.
> > >
> > > >
> > > > How about a vendor prefix on that compatible?  Why
> > > > #address-cells/#size- cells despite no child nodes?  Where is a
> > > > binding that says what each of those two reg resources mean?
> > >
> > > I will add the vendor prefix. I review the ssd1289 driver, and the
> > #address-cells/#size-cells were un-used. I will remove them.
> > 
> > And a binding?
> > 
> > Why do you need two separate reg resources rather than just <2 0 4>?
> > Will they ever be discontiguous?
> 
> [Xie] I review the ssd1289 driver code, and found the driver need two reg resources, 

The device tree describes the hardware, not the current state of Linux
drivers.  Especially drivers that aren't yet in Linux. :-)

> if change the dts, the driver also should be modified accordingly. So I
> remove the ssd1289 node from this patch. I will submit new patch
> include the dts modification, ssd1289 driver and the binding.   

Ideally all devices (and bindings) should be described when the device
tree is initally added, regardless of whether you have a driver yet.

-Scott