| Message ID | 20200522174815.3723-1-foss@volatilesystems.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [OpenWrt-Devel,v2] ath79/nand: add support for Netgear WNDR4300 SW | expand |
Hi, this starts with some nitpicks, and becomes more important closer to the end: > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Stijn Segers > Sent: Freitag, 22. Mai 2020 19:48 > To: openwrt-devel@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@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@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi Adrian, Op vrijdag 22 mei 2020 om 20u17 schreef mail@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@lists.openwrt.org] >> On Behalf Of Stijn Segers >> Sent: Freitag, 22. Mai 2020 19:48 >> To: openwrt-devel@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@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@lists.openwrt.org >> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
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 + 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 + + 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|\ netgear,wndr4300-v2|\ netgear,wndr4500-v3) caldata_extract "caldata" 0x5000 0x440
The Netgear WNDR4300 SW is identical to the regular WNDR4300 and only differs by its BOARD_ID. 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). [1] https://forum.openwrt.org/t/porting-wndr4300-to-ath79/16006/57 Signed-off-by: Stijn Segers <foss@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