[PATCH v3 1/1] add support for Seagate BlackArmor NAS220

[evgeni@studio-punkt.com (Evgeni Dobrev)]

Hello Sebastian,

On Thu, Dec 25, 2014 at 02:08:12PM +0100, Sebastian Hesselbarth wrote:
> 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 <evgeni@studio-punkt.com>
> 
> 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.
> 

thank you for your review. I will make the changes you suggest and resubmit.

> >---
> >  arch/arm/boot/dts/Makefile            |    1 +
> >  arch/arm/boot/dts/kirkwood-nas220.dts |  166 +++++++++++++++++++++++++++++++++
> >  2 files changed, 167 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/kirkwood-nas220.dts
> >
> >diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >index 38c89ca..8b9ad1d 100644
> >--- a/arch/arm/boot/dts/Makefile
> >+++ b/arch/arm/boot/dts/Makefile
> >@@ -132,6 +132,7 @@ dtb-$(CONFIG_MACH_KIRKWOOD) += kirkwood-b3.dtb \
> >  	kirkwood-lsxhl.dtb \
> >  	kirkwood-mplcec4.dtb \
> >  	kirkwood-mv88f6281gtw-ge.dtb \
> >+	kirkwood-nas220.dtb \
> 
> 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" ?
> 
> >  	kirkwood-net2big.dtb \
> >  	kirkwood-net5big.dtb \
> >  	kirkwood-netgear_readynas_duo_v2.dtb \
> >diff --git a/arch/arm/boot/dts/kirkwood-nas220.dts b/arch/arm/boot/dts/kirkwood-nas220.dts
> >new file mode 100644
> >index 0000000..8175f3d
> >--- /dev/null
> >+++ b/arch/arm/boot/dts/kirkwood-nas220.dts
> >@@ -0,0 +1,166 @@
> >+/*
> >+ * Device Tree file for Seagate BlackArmor NAS220
> >+ *
> >+ * Copyright (C) 2014 Evgeni Dobrev <evgeni@studio-punkt.com>
> >+ *
> >+ * Licensed under GPLv2 or later.
> >+ */
> >+
> >+/dts-v1/;
> >+
> >+#include <dt-bindings/gpio/gpio.h>
> >+#include <dt-bindings/input/input.h>
> >+#include "kirkwood.dtsi"
> >+#include "kirkwood-6192.dtsi"
> >+
> >+/ {
> >+	model = "Seagate NAS 220";
> 
> 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!
> 

Thanks,

evgeni