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

[Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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-c3HRhe8yoGxdzLpqerNdyw@public.gmane.org>

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.

> ---
>   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-c3HRhe8yoGxdzLpqerNdyw@public.gmane.org>
> + *
> + * 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html