Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751838AbaLZLG5 (ORCPT ); Fri, 26 Dec 2014 06:06:57 -0500 Received: from h1943130.stratoserver.net ([81.169.133.216]:51592 "EHLO studio-punkt.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751441AbaLZLGz convert rfc822-to-8bit (ORCPT ); Fri, 26 Dec 2014 06:06:55 -0500 Date: Fri, 26 Dec 2014 12:06:49 +0100 From: Evgeni Dobrev To: Sebastian Hesselbarth Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jason Cooper , Sebastian Hesselbarth , Gregory Clement , Andrew Lunn Subject: Re: [PATCH v3 1/1] add support for Seagate BlackArmor NAS220 Message-ID: <20141226110649.GC4700@anne> References: <20141215203855.GA28940@anne> <20141222125750.GA29163@anne> <549C0C3C.8060601@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: <549C0C3C.8060601@gmail.com> X-PGP-Key: http://evgeni.studio-punkt.com/pub.asc User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, > > 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 > >+ * > >+ * Licensed under GPLv2 or later. > >+ */ > >+ > >+/dts-v1/; > >+ > >+#include > >+#include > >+#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@f1000000 { > >+ pinctrl: pin-controller@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@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@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@11000 { > >+ status = "okay"; > >+ adt7476: adt7476a@2e { > > I know we have a lot of bad examples, but: node names should reflect > device function, not device name, i.e. > > adt7476: thermal@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@1{ > >+ label = "Reset push button"; > > Reduce label to "Reset" and "Power" below. > > >+ linux,code = ; > >+ gpios = <&gpio0 29 GPIO_ACTIVE_HIGH>; > >+ }; > >+ button@2{ > >+ label = "Power push button"; > >+ linux,code = ; > >+ 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@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@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@8 { > >+ reg = <8>; > >+ }; > > Remove extra space before brace. > > >+}; > >+ > >+ð0 { > >+ status = "okay"; > >+ ethernet0-port@0 { > >+ phy-handle = <ðphy0>; > >+ }; > >+}; > > > > Overall, this look fine to me, with the nits taken care of > > Acked-by: Sebastian Hesselbarth > > Thanks! > Thanks, evgeni -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/