On 22.12.2014 13:57, Evgeni Dobrev wrote:

This patch adds support for Seagate BlackArmor NAS220.

The Seagate BlackArmor NAS 220 is a NAS system based on Marvell 88f6192. It has
32MB NAND and 128MB DRAM. It has two SATA slots, one Gigabit Ethernet port, two
USB 2.0 ports, two buttons and three LEDs. There is a serial port available on
the CN5 connector on the board (1 - TX, 4 - RX, 6 - GND).

The only functionality still not implemented is the bi-color led on the front
panel (status). Pins mpp22 and mpp23 control this led. Setting mpp22 to high and
mpp23 to low results in orange color. Setting mpp22 to low and mpp23 to high
results in blue color.

The third led is wired to show the SATA activity on the two drives.

Signed-off-by: Evgeni Dobrev <redacted>

Evgeni,

sorry for the late review, but I do have some nits that should
remove some inconsistencies. If we don't do it now, we'd never
change that later.

I am not too happy with choosing "nas220" as the base name for the
board. Can we make it a little bit more specific like
"seagate-nas220" or "blackarmor-nas220" ?

model = "Seagate BlackArmor NAS220";

i.e. choose the same spelling in comment above and model name here.

+	compatible = "seagate,nas220","marvell,kirkwood-88f6192","marvell,kirkwood";

compatible should reflect the chosen base name above.

+
+	memory { /* 128 MB */
+		device_type = "memory";
+		reg = <0x00000000 0x8000000>;
+	};
+
+	chosen {
+		bootargs = "console=ttyS0,115200n8";
+		stdout-path = &uart0;
+	};
+
+	ocp at f1000000 {
+		pinctrl: pin-controller at 10000 {

v3.19 should already have a label for the common pinctrl node.
Please do not replay the bus structure but use a node reference
like &nand and friends below.

+			pinctrl-0 = <&pmx_uart0
+				     &pmx_button_reset
+				     &pmx_button_power>;
+			pinctrl-names = "default";
+
+			pmx_act_sata0: pmx-act-sata0 {
+				marvell,pins = "mpp15";
+				marvell,function = "sata0";
+			};

Insert a blank line between each of the pmx_foo nodes?

+			pmx_act_sata1: pmx-act-sata1 {
+				marvell,pins = "mpp16";
+				marvell,function = "sata1";
+			};
+			pmx_power_sata0: pmx-power-sata0 {
+				marvell,pins = "mpp24";
+				marvell,function = "gpio";
+			};
+			pmx_power_sata1: pmx-power-sata1 {
+				marvell,pins = "mpp28";
+				marvell,function = "gpio";
+			};
+			pmx_button_reset: pmx-button-reset {
+				marvell,pins = "mpp29";
+				marvell,function = "gpio";
+			};
+			pmx_button_power: pmx-button-power {
+				marvell,pins = "mpp26";
+				marvell,function = "gpio";
+			};
+		};
+
+

Remove extra blank line.

+		/*
+		 * Serial port routed to connector CN5
+		 *
+		 * pin 1 - TX
+		 * pin 4 - RX
+		 * pin 6 - GND
+		 */

Nice! Can you also clarify if TX/RX are as seen from the SoC or
remote end?

+		serial at 12000 {

Please use node references where possible, IIRC v3.19 should have
labels for serial, sata, and i2c. Avoid to replay the bus structure.

+			status = "okay";
+		};
+
+		sata at 80000 {
+			status = "okay";
+			nr-ports = <2>;

I need some update from the other mvebu guys here: Do we have SATA
PHY nodes in v3.19 for Kirkwood already? If so, please update to the
new binding.

+		};
+
+		i2c at 11000 {
+			status = "okay";
+			adt7476: adt7476a at 2e {

I know we have a lot of bad examples, but: node names should reflect
device function, not device name, i.e.

adt7476: thermal at 2e {
	...
};

+				compatible = "adi,adt7476";
+				reg = <0x2e>;
+			};
+		};
+	};
+
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&gpio0 14 GPIO_ACTIVE_LOW>;
+	};
+
+	gpio_keys {
+		compatible = "gpio-keys";

Add a blank line between nodes.

+		button at 1{
+			label = "Reset push button";

Reduce label to "Reset" and "Power" below.

+			linux,code = <KEY_POWER>;
+			gpios = <&gpio0 29 GPIO_ACTIVE_HIGH>;
+		};
+		button at 2{
+			label = "Power push button";
+			linux,code = <KEY_SLEEP>;
+			gpios = <&gpio0 26 GPIO_ACTIVE_LOW>;
+		};
+	};
+
+	gpio-leds {
+		compatible = "gpio-leds";

Add a blank line between nodes.

+		blue-power {
+			label = "nas220:blue:power";
+			gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "default-on";
+		};
+	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-0 = <&pmx_power_sata0 &pmx_power_sata1>;
+		pinctrl-names = "default";
+
+		sata0_power: regulator at 1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "SATA0 Power";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			regulator-always-on;
+			regulator-boot-on;
+			gpio = <&gpio0 24 0>;

Hmm, do you need "regulator-always-on" when it is GPIO controlled?
Also, gpio property could use GPIO_ACTIVE_HIGH/LOW here too.

+		};
+
+		sata1_power: regulator at 2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+			regulator-name = "SATA1 Power";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			regulator-always-on;
+			regulator-boot-on;
+			gpio = <&gpio0 28 0>;

ditto.

+		};
+	};
+};
+
+&nand {
+	status = "okay";
+};
+
+&mdio {
+	status = "okay";
+	ethphy0: ethernet-phy at 8 {
+		 reg = <8>;
+	 };

Remove extra space before brace.

+};
+
+&eth0 {
+	status = "okay";
+	ethernet0-port at 0 {
+		phy-handle = <&ethphy0>;
+	};
+};

Overall, this look fine to me, with the nits taken care of

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Thanks!