[LEDE-DEV] [PATCH] This patch adds support for the Netgear WN3000RPv3 N300 repeater.

[Thibaut]

Hi,

> Le 18 janv. 2017 à 09:05, Mathias Kresin <dev at kresin.me> a écrit :
> 
> Hey Thibaut,
> 
> see my comment inline.
> 
> Mathias
> 
> 17.01.2017 23:12, hacks at slashdirt.org:

>> --- a/target/linux/ramips/base-files/etc/diag.sh
>> +++ b/target/linux/ramips/base-files/etc/diag.sh
>> @@ -53,7 +53,8 @@ get_status_led() {
>> 	jhr-n825r|\
>> 	mpr-a1|\
>> 	mpr-a2|\
>> -	mzk-ex750np)
>> +	mzk-ex750np|\
>> +	wn3000rpv3)
>> 		status_led="$board:red:power"
> 
> Any specific reason why you are using the red led here? I mean, you are switching on the green led already in the dts and red is at least for me some kind of warning signal, which I would not expect to see if everything is fine.

Yes, this is to match stock firmware’s behavior:

This is a dual-color led. At power on, the boot loader brings both LEDs on, giving an orange status color. During boot up, LEDE will signal the boot / failsafe by blinking the red led, giving an orange-green blink. After boot is completed, the red LED is turned off to give a solid green.

> If you decide to use the green led, you can remove the red power off led from /etc/board.d/01_leds.

I think it’s fine the way it is. It also matches the EX2700 setup which is a very similar device.

>> --- /dev/null
>> +++ b/target/linux/ramips/dts/WN3000RPV3.dts
>> @@ -0,0 +1,148 @@
>> +/dts-v1/;
>> +
>> +#include "mt7620a.dtsi"
>> +
> 
> Please include <dt-bindings/gpio/gpio.h> here as well.
> 
> Use the GPIO_ACTIVE_LOW and GPIO_ACTIVE_HIGH macros afterwards in stead of 1 and 0 in the gpio parameters.
> 
> Check the recent ramips board additions for examples.

Done.

>> +
>> +		l_arrow {
>> +			label = "wn3000rpv3:blue:l_arrow";
> 
> I guess it would be more clear to use leftarrow and rightarrow here instead of abbreviations.

Done.

>> +			gpios = <&gpio0 7 1>;
>> +		};
>> +
>> +		r_arrow {
>> +			label = "wn3000rpv3:blue:r_arrow";
>> +			gpios = <&gpio0 8 1>;
>> +		};
>> +	};
>> +
>> +	gpio-keys-polled {
>> +		compatible = "gpio-keys-polled";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		poll-interval = <20>;
>> +
>> +		reset {
>> +			label = "reset";
>> +			gpios = <&gpio0 1 1>;
>> +			linux,code = <KEY_RESTART>;
>> +		};
>> +
>> +		wps {
>> +			label = "wps";
>> +			gpios = <&gpio0 2 1>;
>> +			linux,code = <KEY_WPS_BUTTON>;
>> +		};
>> +	};
>> +};
>> +
>> +&gpio0 {
>> +	status = "okay";
>> +};
> 
> It should be safe to drop the gpio0 node. It should be already enabled in the mt7620a.dtsi.

Done.

>> +
>> +&gpio1 {
>> +	status = "okay";
>> +};
>> +
>> +&spi0 {
>> +	status = "okay";
>> +
>> +	m25p80 at 0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "jedec,spi-nor";
>> +		reg = <0>;
>> +		linux,modalias = "m25p80", "mx25l6405d";
> 
> Drop the linux,modalias line. It was only required for kernel < 4.4.

Done.

>> +		spi-max-frequency = <10000000>;
>> +
>> +		partition at 0 {
>> +			label = "u-boot";
>> +			reg = <0x0 0x30000>;
>> +			read-only;
>> +		};
>> +
>> +		partition at 30000 {
>> +			label = "u-boot-env";
>> +			reg = <0x30000 0x10000>;
>> +			read-only;
>> +		};
>> +
>> +		partition at 40000 {
>> +			label = "firmware";
>> +			reg = <0x40000 0x7b0000>;
>> +		};
>> +
>> +		art: partition at 7f0000 {
>> +			label = "art";
>> +			reg = <0x7f0000 0x10000>;
>> +			read-only;
>> +		};
>> +	};
>> +};
>> +
>> +&ethernet {
>> +	mtd-mac-address = <&art 0x0>;
>> +};
>> +
>> +&wmac {
>> +	mtd-mac-address = <&art 0x6>;
>> +	ralink,mtd-eeprom = <&art 0x1000>;
>> +};
>> +
>> +&pinctrl {
>> +	state_default: pinctrl0 {
>> +		default {
>> +			//  spi refclk: pins 37, 38, 39
>> +			//       uartf: pins 8, 9, 10, 11, 12, 13, 14
>> +			//         i2c: pins 1, 2
> 
> Remove the comments. If they are important, add the information to the commit message.

Done.

Thanks.

Thibaut