Re: [PATCH 2/2] ARM: dts: pxa: add mioa701 board description

Rob Herring [off-list ref] writes:

Hi Rob,

quoted

+       /* compatible = "mitac,mioa701"; */
+       compatible = "marvell,pxa270";

Why the comment?

Leftover, I'll remove it.

quoted

+                       pstore_region:region@0xa2000000 {

Drop the 0x

Done.

quoted

+                               compatible = "linux,contiguous-memory-region";
+                               reg = <0xa2000000 1048576>;

Use hex for the size.

Ok.

quoted

+               ffuart: uart@40100000 {

Should be "serial@...". You'll have to fix in the base dtsi.

Ok for all of these.

quoted

+               pxai2c1: i2c@40301680 {
+                       mrvl,i2c-fast-mode;
+                       pinctrl-names = "default";
+                       pinctrl-0 = <&pinctrl_i2c_default>;
+                       status = "okay";
+
+                       mt9m111: camera@5d {
+                               compatible = "micron,mt9m111";
+                               reg = <0x5d>;
+                               gpios = <&gpio 56 GPIO_ACTIVE_HIGH>;
+
+                               remote = <&pxa_camera>;

Not needed with the graph:

I don't understand that, do you mean the port node is not needed ?

quoted

+                               port {
+                                       mt9m111_1: endpoint {
+                                               bus-width = <8>;
+                                               remote-endpoint = <&pxa_camera>;
+                                       };
+                               };
+                       };
+               };
+               gpio-keys {
+                       compatible = "gpio-keys";
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+                       autorepeat;
+                       pinctrl-names = "default";
+                       pinctrl-0 = <&pinctrl_gpiokeys_default>;
+                       status = "okay";

Don't need status here? It shouldn't be defined in the base dtsi.

Heuh what ? The status is just above, and gpio-keys is not defined in any dtsi.

quoted

+
+                       button@0 {

If you have unit-address there should also be a reg property. Just
drop and name the node 'power' or 'power-button'. And similarly for
the rest.

Ok.

quoted

+
+       regulators {
+               compatible = "simple-bus";

Drop this and move children to the top level. This is not an mmio bus.

Ok.

quoted

+       docg3: flash@0 {

This probably should be a child of the bus controller.

Most certainly yes.

quoted

+               compatible = "m-systems,diskonchip-g3";
+               reg = <0x0 0x2000>;
+       };
+
+       panel {
+               compatible = "toshiba,ltm0305a776";
+               lcd-type = "color-tft";

This should be implied by the compatible.

Mmm no. Actually, this is used by the framebuffer controller, ie. pxafb to set
it up. And I don't think adding a list of panel compatibles in pxafb is a good
idea.
The corresponding code is in : drivers/video/fbdev/pxafb.c, see
"static const char * const lcd_types[]".

quoted

+               display-timings {
+                       native-mode = <&timing0>;

We generally imply this from the compatible (i.e. use simple-panel).

But you suppose there is an actual driver for the panel, right ? But there is
none.

Could use another level of indentation.

Sure.

quoted

+       ac_charger: charger@1 {

Drop the unit-address.

Ok.

Thanks for the review.

-- 
Robert