Re: [v2,2/2] powerpc/mpc85xx: add support for Keymile's kmcoge4 board

On Thu, 2014-03-20 at 09:42 +0100, Valentin Longchamp wrote:

On 03/20/2014 12:08 AM, Scott Wood wrote:

quoted

On Tue, Feb 11, 2014 at 12:50:07PM +0100, Valentin Longchamp wrote:

quoted

+		reset_cpld@1,0 {
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <1 0 0x80>;
+			interrupt-parent = <&mpic>;
+			interrupts = <
+				4 1 0 0
+				5 1 0 0>;
+		};
+
+		chassis_mgmt@3,0 {
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <3 0 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <6 1 0 0>;
+		};

Dashes are preferred to underscores in device trees.

OK.

quoted

More importantly, these nodes need proper compatibles and bindings.  Once
that's done, the name for the nodes should probably be
"board_control@whatever" for both.

The first one can be board-ctrl.

board-control (rather than board-ctrl) is already used in several device
trees, so it would be good to be consistent.

The second however manages things that are
beyond this board and important for other boards in the chassis, so I think
chassis-mgmt is correct.

OK.

For the binding/compatbiles issues: in the first discussion I had omitted these
nodes because these are not available (and honestly for such FPGAs I doubt they
will ever be mainlined). We discussed it and concluded that the DTS should
describe the HW and not the drivers available in the kernel so I have now added
them. Do you want me to add the compatible strings we use in our tree even
though there are no bindings ? Leave them as is ? Or drop them ?

Include the compatibles, and add bindings.  The bindings should state
the compatible string used, what the interrupt specifier format is, and
what reg/interrupts represents -- especially in the case of the reset
cpld node, which has two interrupts.  Make it clear which interrupt goes
first and which goes second.

It doesn't matter whether the driver will ever be mainlined.

-Scott