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

mail at adrianschmutzler.de mail at adrianschmutzler.de
Fri May 22 14:17:20 EDT 2020 

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.

> +  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 :-)

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200522/3324a3f3/attachment.sig>
-------------- next part --------------
_______________________________________________
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