Hi, this starts with some nitpicks, and becomes more important closer to the end:
> -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-boun...@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-freif...@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 <f...@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
openpgp-digital-signature.asc
Description: PGP signature
_______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
