Hi, > -----Original Message----- > From: openwrt-devel [mailto:[email protected]] > On Behalf Of Rafal Milecki > Sent: Montag, 6. April 2020 21:26 > To: [email protected]; 'Dan Haab' <[email protected]>; openwrt- > [email protected] > Cc: 'Dan Haab' <[email protected]> > Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul > FullMAC WiFi devices > > On 06.04.2020 20:26, [email protected] wrote: > >> -----Original Message----- > >> From: openwrt-devel [mailto:openwrt-devel- > [email protected]] > >> On Behalf Of Dan Haab > >> Sent: Montag, 6. April 2020 20:20 > >> To: [email protected] > >> Cc: Dan Haab <[email protected]> > >> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul > >> FullMAC WiFi devices > >> > >> From: Dan Haab <[email protected]> > >> > >> This prepares support for models XAP-1610 and XWR-3150. Flashing > >> requires using Luxul firmware version: > >> 1) 8.1.0 or newer for XAP-1610 > >> 2) 6.4.0 or newer for XWR-3150 > >> and uploading firmware using "Firmware Update" web UI page. > >> > >> Signed-off-by: Dan Haab <[email protected]> > >> --- > >> .../bcm53xx/base-files/etc/board.d/02_network | 22 > >> ++++++++++++++++++- > >> target/linux/bcm53xx/image/Makefile | 18 +++++++++++++++ > >> 2 files changed, 39 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/linux/bcm53xx/base-files/etc/board.d/02_network > >> b/target/linux/bcm53xx/base-files/etc/board.d/02_network > >> index f86f12407f..9256cbdc54 100755 > >> --- a/target/linux/bcm53xx/base-files/etc/board.d/02_network > >> +++ b/target/linux/bcm53xx/base-files/etc/board.d/02_network > >> @@ -36,6 +36,15 @@ bcm53xx_setup_interfaces() > >> ucidef_add_switch "switch0" \ > >> "0:wan" "1:lan:4" "2:lan:3" "3:lan:2" "4:lan:1" > >> "5@eth0" > >> ;; > >> + luxul,xap-1610-v1) > >> + ucidef_add_switch "switch0" \ > >> + "0:lan" "1:lan" "5@eth0" > >> + ucidef_set_interface_lan "eth0.1" "dhcp" > >> + ;; > >> + luxul,xwr-3150-v1) > >> + ucidef_add_switch "switch0" \ > >> + "0:lan:1" "1:lan:2" "2:lan:3" "3:lan:4" "4:wan" > >> "5@eth0" > >> + ;; > >> phicomm,k3) > >> ucidef_add_switch "switch0" \ > >> "0:lan" "1:lan" "2:lan" "3:wan" "5@eth0" > >> @@ -100,7 +109,18 @@ bcm53xx_setup_macs() > >> esac > >> > >> # If WAN MAC isn't explicitly set, calculate it using base MAC as > >> reference. > >> - [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] && > >> wan_macaddr=$(macaddr_add "$etXmacaddr" 1) > >> + [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] && { > >> + local offset=1 > >> + > >> + case "$board" in > >> + luxul,xwr-3100v1 | \ > >> + luxul,xwr-3150-v1) > >> + offset=5 > >> + ;; > >> + esac > >> + > >> + wan_macaddr=$(macaddr_add "$etXmacaddr" $offset) > >> + } > > > > This adds another level of nesting. I'd prefer if you just added your > > devices to the case directly above and just use > > > > [ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr" > 5) > > > > for them there. > > We cannot do that, because the lower > [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] && > wan_macaddr=$(macaddr_add "$etXmacaddr" 1) will overwrite what Luxul- > specific one did.
No, it won't, because wan_macaddr won't be empty then (checked by -z)? > > What about having offset set by device specific code? Like: > > > etXmacaddr=$(nvram get et0macaddr) > offset=1 > case "$board" in > asus,rt-ac87u) > etXmacaddr=$(nvram get et1macaddr) > ;; > dlink,dir-885l | \ > netgear,r7900 | \ > netgear,r8000 | \ > netgear,r8500) > etXmacaddr=$(nvram get et2macaddr) > ;; > luxul,foo) > offset=5 > ;; > esac That's a matter of taste. I personally don't like the multi-step assignment at all, and would like to just set the wan_macaddr for every case directly as it's done in ath79/ramips. For my taste, there are too many similar variables flying around here, and I would reduce that to lan_macaddr and wan_macaddr somehow. However, if I understood your earlier comment on the tidy-up patch correctly, the et.macaddr variables are a concept somehow specific to bcm53xx, and thus my version would not be logic/desirable here. Thus, I leave the decision to you, as I'm not familiar with this target and mainly doing drive-by comments here. Still, you solution here looks tidier than the additional nesting introduced in the initial patch. > > > >> [ -n "$wan_macaddr" ] && ucidef_set_interface_macaddr "wan" > >> "$wan_macaddr" > >> } > >> diff --git a/target/linux/bcm53xx/image/Makefile > >> b/target/linux/bcm53xx/image/Makefile > >> index 610af03abe..b3ec1e99a2 100644 > >> --- a/target/linux/bcm53xx/image/Makefile > >> +++ b/target/linux/bcm53xx/image/Makefile > >> @@ -291,6 +291,15 @@ define Device/luxul-abr-4500 endef > >> TARGET_DEVICES += luxul-abr-4500 > >> > >> +define Device/luxul-xap-1610 > >> + $(Device/luxul) > >> + DEVICE_MODEL := XAP-1610 > >> + DEVICE_PACKAGES := $(BRCMFMAC_4366C0) > >> + IMAGE/lxl := append-rootfs | trx-serial | luxul-lxl > >> + LUXUL_BOARD := XAP-1610 > >> +endef > >> +TARGET_DEVICES += luxul-xap-1610 > >> + > >> define Device/luxul-xbr-4500 > >> $(Device/luxul) > >> DEVICE_MODEL := XBR-4500 > >> @@ -299,6 +308,15 @@ define Device/luxul-xbr-4500 endef > >> TARGET_DEVICES += luxul-xbr-4500 > >> > >> +define Device/luxul-xwr-3150 > > > > Could you add a -v1 here as well? > > I see DTS file has "v1" in its name but does v2 exist at all? > > If there is not v2 and it's not planned right now I'm OK with filename witohut > "v1". If the rest of the patch is correct, the compatible has a -v1 as well (I haven't checked). I'm generally looking for consistency here, but on the other hand the other luxul-x... devices don't have a version suffix. Though, again, this is not my playing ground, so feel free to ignore my comments from the side. Best Adrian > > _______________________________________________ > openwrt-devel mailing list > [email protected] > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
openpgp-digital-signature.asc
Description: PGP signature
_______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
