On 06.04.2020 21:39, m...@adrianschmutzler.de wrote:
-----Original Message-----
From: openwrt-devel [mailto:openwrt-devel-boun...@lists.openwrt.org]
On Behalf Of Rafal Milecki
Sent: Montag, 6. April 2020 21:26
To: m...@adrianschmutzler.de; 'Dan Haab' <ripro...@gmail.com>; openwrt-
de...@lists.openwrt.org
Cc: 'Dan Haab' <dan.h...@legrand.com>
Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
FullMAC WiFi devices
On 06.04.2020 20:26, m...@adrianschmutzler.de wrote:
-----Original Message-----
From: openwrt-devel [mailto:openwrt-devel-
boun...@lists.openwrt.org]
On Behalf Of Dan Haab
Sent: Montag, 6. April 2020 20:20
To: openwrt-devel@lists.openwrt.org
Cc: Dan Haab <dan.h...@legrand.com>
Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
FullMAC WiFi devices
From: Dan Haab <dan.h...@legrand.com>
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 <dan.h...@legrand.com>
---
.../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)?
Right, I missed that!
So the only thing I slightly dislike about having:
[ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr" 5)
and
[ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
is code duplication. I see it screaming: "use variable" ;)
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.
I was trying to avoid repeating
offset=1
or
wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
in 99% of switch cases. I guess that's why I have some extra variables
in the first place - to avoid some code duplication.
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.
Sure & thanks for comments, it's always nice to someone's opinion.
I like bcm53xx code much more after adding bcm53xx_setup_interfaces() &
bcm53xx_setup_macs() - thanks!
[ -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.
It's always a problem to predict how many versions given device is
going to have ;)
We have "linksys-ea6300-v1" even though v2 was never developed /
released.
On ther other have netgear-r8000 has no v1 but later v2 has appeared.
Dan: if Luxul is planning XWR-3150 v2, then plase use v1 in this patch.
Otherwise I'm OK with skipping v1.
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
