Re: [PATCH v2 1/5] ARM: dts: omap3: Add support for INCOstartec a83x module

Hi Nishanth,

 thanks for reviewing this patch. Please see my comments below.

On Wed, Jan 22, 2014 at 01:48:14PM -0600, Nishanth Menon wrote:

On 01/22/2014 01:04 PM, Christoph Fritz wrote:

quoted

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

Shrug, just moving the fixed regulator to root also will do the job,
not sure simple-bus much sense here :(

will get fixed

quoted

+		reg_vcc3: vcc3 {
+                        compatible = "regulator-fixed";
+                        regulator-name = "VCC3";
+                        regulator-min-microvolt = <3300000>;
+                        regulator-max-microvolt = <3300000>;
+                        regulator-always-on;
+		};
+	};
+
+	hsusb1_phy: hsusb1_phy {
+		compatible = "usb-nop-xceiv";
+		vcc-supply = <&reg_vcc3>;
+	};
+};
+
+&omap3_pmx_wkup {
+	pinctrl-names = "default";
+
+	lan9221_pins: pinmux_lan9221_pins {
+		pinctrl-single,pins = <
+			0x5A (PIN_INPUT | MUX_MODE4)   /* gpio_129 */

umm.. you might want to follow the convention as you followed later in
comments.

My convention was: If pin can be described unique with one name,
don't use two. But I'll change that to be uniform with all comments.

quoted

+		>;
+	};
+
+	tsc2048_pins: pinmux_tsc2048_pins {
+		pinctrl-single,pins = <
+			0x16 (PIN_INPUT_PULLUP | MUX_MODE4)   /* gpio_8 */

umm.. you might want to follow the convention as you followed later in
comments.

will get fixed

quoted

+		>;
+	};
+
+	mmc1cd_pins: pinmux_mmc1cd_pins {
+		pinctrl-single,pins = <
+			0x56 (PIN_INPUT | MUX_MODE4)   /* gpio_126 */

umm.. you might want to follow the convention as you followed later in
comments.

will get fixed

quoted

+		>;
+	};
+};
+
+&omap3_pmx_core {
+	pinctrl-names = "default";
+
+	gpio1_pins: pinmux_gpio1_pins {
+		pinctrl-single,pins = <
+			0x5ca (PIN_OUTPUT_PULLDOWN | MUX_MODE4)   /* gpio_29 */

umm.. you might want to follow the convention as you followed later in
comments.

will get fixed

quoted

+		>;
+	};
+
+	uart1_pins: pinmux_uart1_pins {
+		pinctrl-single,pins = <
+			0x14c (PIN_OUTPUT | MUX_MODE0)   /* uart1_tx.uart1_tx */
+			0x14e (PIN_OUTPUT | MUX_MODE0)   /* uart1_rts.uart1_rts */
+			0x150 (PIN_INPUT | MUX_MODE0)    /* uart1_cts.uart1_cts */
+			0x152 (PIN_INPUT | MUX_MODE0)    /* uart1_rx.uart1_rx */
+		>;

you may be interested in include/dt-bindings/pinctrl/omap.h
OMAP3_CORE1_IOPAD, OMAP3_CORE2_IOPAD as needed here.

will get fixed

quoted

+	};
+
+	uart2_pins: pinmux_uart2_pins {
+		pinctrl-single,pins = <
+			0x140 (PIN_OUTPUT | MUX_MODE1)   /* mcbsp3_clkx.uart2_tx */
+			0x142 (PIN_INPUT | MUX_MODE1)    /* mcbsp3_fsx.uart2_rx */
+		>;
+	};
+
+	uart3_pins: pinmux_uart3_pins {
+		pinctrl-single,pins = <
+			0x16e (PIN_INPUT | MUX_MODE0)    /* uart3_rx_irrx.uart3_rx_irrx */
+			0x170 (PIN_OUTPUT | MUX_MODE0)   /* uart3_tx_irtx.uart3_tx_irtx */
+		>;
+	};
+
+	i2c1_pins: pinmux_i2c1_pins {
+		pinctrl-single,pins = <
+			0x18a (PIN_INPUT_PULLUP | MUX_MODE0)    /* i2c1_scl */
+			0x18c (PIN_INPUT_PULLUP | MUX_MODE0)    /* i2c1_sda */

umm.. you might want to follow the convention as you followed
elsewhere in comments.

will get fixed

quoted

+		>;
+	};
+
+	i2c2_pins: pinmux_i2c2_pins {
+		pinctrl-single,pins = <
+			0x18e (PIN_INPUT | MUX_MODE0)   /* i2c2_scl.i2c2_scl */
+			0x190 (PIN_INPUT | MUX_MODE0)   /* i2c2_sda.i2c2_sda */
+		>;
+	};
+
+	i2c3_pins: pinmux_i2c3_pins {
+		pinctrl-single,pins = <
+			0x192 (PIN_INPUT | MUX_MODE0)   /* i2c3_scl.i2c3_scl */
+			0x194 (PIN_INPUT | MUX_MODE0)   /* i2c3_sda.i2c3_sda */
+		>;
+	};
+
+	hsusb1_pins: pinmux_hsusb1_pins {
+		pinctrl-single,pins = <
+			0x5a8 (PIN_OUTPUT | MUX_MODE3)  /* etk_clk.hsusb1_stp */

&omap3_pmx_core2 and OMAP3_CORE2_IOPAD probably here. and probably see
similar usage in other board dtsi.

will get fixed

quoted

+
+#include "twl4030.dtsi"
+#include "twl4030_omap3.dtsi"
+
+&twl {
+	vmmc1: regulator-vmmc1 {
+		regulator-always-on;
+	};
+
+	vdd1: regulator-vdd1 {
+		regulator-always-on;
+	};
+
+	vdd2: regulator-vdd2 {
+		regulator-always-on;
+	};

I hope you have covered all required regulators here including the
ones you might need for IO.1P8 perhaps?

Yes, I rechecked all and it's fine now. IO.1P8 was low due to a
hardware error which is now fixed.

quoted

+
+&mmc1 {
+	reg = <0x4809c000 0x400>;

little curious as to why this. is that to override length 0x200 to
0x400? that belongs to soc.dtsi then.

This is a remnant from last year and will get purged.

Let me reroll this patch in a new v3 set.

Thanks
  -- Christoph