On Mon, Apr 6, 2020 at 2:03 PM Rafał Miłecki <zaj...@gmail.com> wrote: > > 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.
Luxul has no plans to create a "v2" of this device _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
