[OpenWrt-Devel] [PATCH v2] ath79/nand: add support for Netgear WNDR4300 SW

Stijn Segers foss at volatilesystems.org
Sat May 23 04:47:54 EDT 2020 

Hi Adrian,

Op vrijdag 22 mei 2020 om 20u17 schreef mail at adrianschmutzler.de:
> Hi,
> 
> this starts with some nitpicks, and becomes more important closer to 
> the end:
> 
>>  -----Original Message-----
>>  From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
>>  On Behalf Of Stijn Segers
>>  Sent: Freitag, 22. Mai 2020 19:48
>>  To: openwrt-devel at lists.openwrt.org
>>  Subject: [OpenWrt-Devel] [PATCH v2] ath79/nand: add support for 
>> Netgear
>>  WNDR4300 SW
>> 
>>  The Netgear WNDR4300 SW is identical to the regular WNDR4300 and 
>> only
>>  differs by its BOARD_ID.
> 
> Personally, I like to have the Specifications and Flashing 
> instructions for each device support patch, even if it's just a 
> stupid clone (which in turn should make it easy to get the relevant 
> information from the clone).
> 
>> 
>>  Resulting image has been confirmed working [1].
>> 
>>  Because of the minor difference with the regular model I am unsure 
>> about
>>  the approach, so please let me know if this overdoes it (and what I 
>> should
>>  change).
> 
> This sentence should not go into the commit description itself (as it 
> would end up in the repo if the patch was just merged), but could be 
> added after a line containing just "---". Everything after that would 
> be cut off if somebody takes the patch from patchwork.
> 
> For example, have a look at how this is done in my patch here: 
> https://patchwork.ozlabs.org/project/openwrt/patch/20200326155654.48317-1-freifunk@adrianschmutzler.de/
> 
> (Sorry if I tell you something you already know :-) ).
> 
>> 
>> 
>>  [1] https://forum.openwrt.org/t/porting-wndr4300-to-ath79/16006/57
>> 
>>  Signed-off-by: Stijn Segers <foss at volatilesystems.org>
>>  ---
>>   target/linux/ath79/dts/ar9344_netgear_wndr4300sw.dts |  9 +++++++++
>>   target/linux/ath79/image/nand.mk                     | 12 
>> ++++++++++++
>>   .../linux/ath79/nand/base-files/etc/board.d/01_leds  |  1 +
>>   .../ath79/nand/base-files/etc/board.d/02_network     |  1 +
>>   .../etc/hotplug.d/firmware/10-ath9k-eeprom           |  1 +
>>   5 files changed, 24 insertions(+)
>>   create mode 100644
>>  target/linux/ath79/dts/ar9344_netgear_wndr4300sw.dts
>> 
>>  diff --git a/target/linux/ath79/dts/ar9344_netgear_wndr4300sw.dts
>>  b/target/linux/ath79/dts/ar9344_netgear_wndr4300sw.dts
>>  new file mode 100644
>>  index 0000000000..fb90eee550
>>  --- /dev/null
>>  +++ b/target/linux/ath79/dts/ar9344_netgear_wndr4300sw.dts
>>  @@ -0,0 +1,9 @@
>>  +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT /dts-v1/;
>>  +
>>  +#include "ar9344_netgear_wndr.dtsi"
>>  +
>>  +/ {
>>  +	compatible = "netgear,wndr4300sw", "qca,ar9344";
>>  +	model = "Netgear WNDR4300SW";
>>  +};
>>  diff --git a/target/linux/ath79/image/nand.mk
>>  b/target/linux/ath79/image/nand.mk
>>  index 3ccd19914f..9814815ff1 100644
>>  --- a/target/linux/ath79/image/nand.mk
>>  +++ b/target/linux/ath79/image/nand.mk
>>  @@ -172,6 +172,18 @@ define Device/netgear_wndr4300  endef
>>  TARGET_DEVICES += netgear_wndr4300
>> 
>>  +define Device/netgear_wndr4300sw
>>  +  SOC := ar9344
>>  +  DEVICE_MODEL := WNDR4300
>>  +  DEVICE_VARIANT := SW
> 
> If you use DEVICE_VARIANT here, this implies a space between WNDR4300 
> and SW: "WNDR4300 SW"
> For consistency, this would then require a hyphen in definition and 
> compatible, DTS name etc.: netgear_wndr4300-sw
> 
> A simpler alternative would be to "decide" SW is not a variant, but 
> part of the device model name. Then you would just use
> DEVICE_MODEL := WNDR4300SW
> without DEVICE_VARIANT.
> 
> Based on googling, I could not find out what's the "true name" here.


The firmware header has it as 'WNDR4300SW', it's a suffix without 
spaces I've been told by an owner.
> 
>>  +  NETGEAR_KERNEL_MAGIC := 0x33373033
>>  +  NETGEAR_BOARD_ID := WNDR4300SW
>>  +  NETGEAR_HW_ID := 29763948+0+128+128+2x2+3x3
>>  +  $(Device/netgear_ath79_nand)
>>  +endef
>>  +TARGET_DEVICES += netgear_wndr4300sw
>>  +
>>  +
> 
> Only one empty line please.
> 
>>   define Device/netgear_wndr4300-v2
>>     SOC := qca9563
>>     DEVICE_MODEL := WNDR4300
>>  diff --git a/target/linux/ath79/nand/base-files/etc/board.d/01_leds
>>  b/target/linux/ath79/nand/base-files/etc/board.d/01_leds
>>  index d9989ec538..4f601849fc 100755
>>  --- a/target/linux/ath79/nand/base-files/etc/board.d/01_leds
>>  +++ b/target/linux/ath79/nand/base-files/etc/board.d/01_leds
>>  @@ -14,6 +14,7 @@ glinet,gl-ar300m-nor)
>>   	;;
>>   netgear,wndr3700-v4|\
>>   netgear,wndr4300|\
>>  +netgear,wndr4300sw|\
>>   netgear,wndr4300-v2|\
>>   netgear,wndr4500-v3)
>>   	ucidef_set_led_switch "wan-amber" "WAN (amber)"
>>  "netgear:amber:wan" "switch0" "0x20"
>>  diff --git 
>> a/target/linux/ath79/nand/base-files/etc/board.d/02_network
>>  b/target/linux/ath79/nand/base-files/etc/board.d/02_network
>>  index b2191eed92..42be72af53 100755
>>  --- a/target/linux/ath79/nand/base-files/etc/board.d/02_network
>>  +++ b/target/linux/ath79/nand/base-files/etc/board.d/02_network
>>  @@ -44,6 +44,7 @@ ath79_setup_macs()
>>   	case "$board" in
>>   	netgear,wndr3700-v4|\
>>   	netgear,wndr4300|\
>>  +	netgear,wndr4300sw|\
>>   	netgear,wndr4300-v2|\
>>   	netgear,wndr4500-v3)
>>   		wan_mac=$(mtd_get_mac_binary caldata 0x6) diff --git
>>  
>> a/target/linux/ath79/nand/base-files/etc/hotplug.d/firmware/10-ath9k-
>>  eeprom 
>> b/target/linux/ath79/nand/base-files/etc/hotplug.d/firmware/10-
>>  ath9k-eeprom
>>  index f5fae46dfb..f89fc83ddf 100644
>>  --- 
>> a/target/linux/ath79/nand/base-files/etc/hotplug.d/firmware/10-ath9k-
>>  eeprom
>>  +++ 
>> b/target/linux/ath79/nand/base-files/etc/hotplug.d/firmware/10-ath9k
>>  +++ -eeprom
>>  @@ -24,6 +24,7 @@ case "$FIRMWARE" in
>>   	case $board in
>>   	netgear,wndr3700-v4|\
>>   	netgear,wndr4300|\
>>  +	netgear,wndr4300sw|\
> 
> Both 02_network and 10-ath9k-eeprom have two entries for wndr4300, 
> but you add only one for the sw variant.
> 
> Consequently, an image built from this patch should have wrong 
> wan_mac and lack calibration data for one WiFi.
> 
> Fixing everything noted above should be easy, but you should improve 
> your test protocols, as the missing caldata shouldn't have slipped 
> through :-)

Yes that was sloppy. Thanks for your thorough review, v3 follows.

Stijn

> 
> Best
> 
> Adrian
> 
>>   	netgear,wndr4300-v2|\
>>   	netgear,wndr4500-v3)
>>   		caldata_extract "caldata" 0x5000 0x440
>>  --
>>  2.20.1
>> 
>> 
>>  _______________________________________________
>>  openwrt-devel mailing list
>>  openwrt-devel at lists.openwrt.org
>>  https://lists.openwrt.org/mailman/listinfo/openwrt-devel



_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

More information about the openwrt-devel mailing list