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

From: Evgeni Dobrev
Date: Fri Dec 26 2014 - 06:07:01 EST

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@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> >+ *
> >+ * 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@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 = <KEY_POWER>;
> >+ gpios = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> >+ };
> >+ button@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@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.
>
> >+};
> >+
> >+&eth0 {
> >+ status = "okay";
> >+ ethernet0-port@0 {
> >+ phy-handle = <&ethphy0>;
> >+ };
> >+};
> >
>
> Overall, this look fine to me, with the nits taken care of
>
> Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx>
>
> Thanks!
>

Thanks,

evgeni
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/