Hi Adrian El dom., 24 may. 2020 a las 13:15, <m...@adrianschmutzler.de> escribió: > > Hi Daniel, > > > -----Original Message----- > > From: openwrt-devel [mailto:openwrt-devel-boun...@lists.openwrt.org] > > On Behalf Of Daniel González Cabanelas > > Sent: Sonntag, 24. Mai 2020 11:06 > > To: m...@adrianschmutzler.de > > Cc: openwrt-devel@lists.openwrt.org; Álvaro Fernández Rojas > > <nolt...@gmail.com> > > Subject: Re: [OpenWrt-Devel] [PATCH] bcm63xx: DGND3700v1: device tree > > improvements > > > > Hi Adrian: > > > > El dom., 24 may. 2020 a las 1:01, <m...@adrianschmutzler.de> escribió: > > > > > > Hi, > > > > > > > -----Original Message----- > > > > From: openwrt-devel [mailto:openwrt-devel- > > boun...@lists.openwrt.org] > > > > On Behalf Of Daniel González Cabanelas > > > > Sent: Sonntag, 24. Mai 2020 00:24 > > > > To: openwrt-devel@lists.openwrt.org > > > > Cc: nolt...@gmail.com > > > > Subject: [OpenWrt-Devel] [PATCH] bcm63xx: DGND3700v1: device tree > > > > improvements > > > > > > > > Improve the device tree file and related board data for the > > > > DGND3700v1/ DGND3800B router: > > > > - Improve LEDs definitions, use shorter names. > > > > - Make the board name more readable. > > > > - Fix the switch LAN labels, the order is reversed. > > > > - Use the real name of the board for the board name instead of device > > > > name. > > > > - Use standarized names for partition nodes and leds. > > > > > > This deals with several different issues at the same time. I'd prefer to > > > have > > it split up (e.g. separate board name change from LED changes and switch > > changes). > > > > > > > I can't see the benefit of flooding with commits on every negligible change. > > These are just cosmetic changes which won't affect the behavior of the > > device, and wont produce any unexpected bug, I've made the opportune > > tests. > > Well, one of the possible bugs would be that LEDs won't work after > sysupgrade, see below. > I don't think that everything should be separate, but I don't like completely > different things stuffed together. > Not really a bug, and the solution is easy from the point of view of any user. It would be the reason for non backporting these changes, not a good idea having a diferent behavior of leds on the same Openwrt version. It's more intuitive for any user which decides to upgrade to a new version and if something isn't working, they think there is a missconconfiguration issue somewhere. I'm almost sure 100% of users make a reset to defaults on this case.
> > > > > > > > > > Signed-off-by: Daniel González Cabanelas <dgcb...@gmail.com> > > > > --- > > > > .../bcm63xx/base-files/etc/board.d/01_leds | 12 ++-- > > > > .../dts/bcm6368-netgear-dgnd3700-v1.dts | 64 +++++++++---------- > > > > .../549-board_DGND3700v1_3800B.patch | 2 +- > > > > 3 files changed, 39 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/target/linux/bcm63xx/base-files/etc/board.d/01_leds > > > > b/target/linux/bcm63xx/base-files/etc/board.d/01_leds > > > > index 91d67f0c0b..6b82d9e952 100755 > > > > --- a/target/linux/bcm63xx/base-files/etc/board.d/01_leds > > > > +++ b/target/linux/bcm63xx/base-files/etc/board.d/01_leds > > > > @@ -66,12 +66,12 @@ inventel,livebox-1) > > > > ucidef_set_led_netdev "wlan0" "WIFI" "Livebox1:red:wifi" "wlan0" > > > > ;; > > > > netgear,dgnd3700-v1) > > > > - ucidef_set_led_netdev "lan" "LAN" "DGND3700v1_3800B:green:lan" > > > > "eth0.1" > > > > - ucidef_set_led_netdev "wan" "WAN" > > > > "DGND3700v1_3800B:green:inet" "eth0.2" > > > > - ucidef_set_led_netdev "wlan0" "WIFI2G" > > > > "DGND3700v1_3800B:green:wifi2g" "wlan0" > > > > - ucidef_set_led_netdev "wlan1" "WIFI5G" > > > > "DGND3700v1_3800B:blue:wifi5g" "wlan1" > > > > - ucidef_set_led_usbdev "usb1" "USB1" > > > > "DGND3700v1_3800B:green:usb-back" "1-1" > > > > - ucidef_set_led_usbdev "usb2" "USB2" > > > > "DGND3700v1_3800B:green:usb-front" "1-2" > > > > + ucidef_set_led_netdev "lan" "LAN" "$model:green:lan" "eth0.1" > > > > + ucidef_set_led_netdev "wan" "WAN" "$model:green:inet" "eth0.2" > > > > + ucidef_set_led_netdev "wlan0" "WIFI2G" "$model:green:wifi2g" > > > > "wlan0" > > > > + ucidef_set_led_netdev "wlan1" "WIFI5G" "$model:blue:wifi5g" > > > > "wlan1" > > > > + ucidef_set_led_usbdev "usb1" "USB1" "$model:green:usb-back" > > > > + "1- > > > > 1" > > > > + ucidef_set_led_usbdev "usb2" "USB2" "$model:green:usb-front" > > > > + "1- > > > > 2" > > > > > > Is there any way to do sysupgrade on these devices? If yes, you will need > > migration of the names in /etc/config/system ... > > > > > > > The sysupgrade works out of the box using the default_do_upgrade. Not > > sure what I need to review, can you be more specific? > > On a newly installed device there won't be a problem. 01_leds will generate > the LED entries in /etc/config/system on firstboot, and after that the names > used there won't change anymore, even on upgrade. In contrast, the LED names > in the device tree will change with every upgrade, so that this change > results in the LED settings becoming broken without a reset of config files. > This can be healed with a migration script, e.g. > https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/mt76x8/base-files/etc/uci-defaults/04_led_migration > > That's BTW the reason why we haven't changed these for consistency so far on > this target. > It's nice having a script for making those migrations. It makes the life easier for end users, but life harder for patching this stuff. > > > > > > ;; > > > > netgear,dgnd3700-v2) > > > > ucidef_set_led_netdev "lan" "LAN" "$model:green:ethernet" "eth0" > > > > diff --git > > > > a/target/linux/bcm63xx/dts/bcm6368-netgear-dgnd3700-v1.dts > > > > b/target/linux/bcm63xx/dts/bcm6368-netgear-dgnd3700-v1.dts > > > > index 546b0b1f60..c17bb3a600 100644 > > > > --- a/target/linux/bcm63xx/dts/bcm6368-netgear-dgnd3700-v1.dts > > > > +++ b/target/linux/bcm63xx/dts/bcm6368-netgear-dgnd3700-v1.dts > > > > @@ -5,12 +5,12 @@ > > > > #include <dt-bindings/input/input.h> > > > > > > > > / { > > > > - model = "Netgear DGND3700v1/DGND3800B"; > > > > + model = "Netgear DGND3700v1 / DGND3800B"; > > > > > > I don't think this is really necessary ... > > > I forgot to comment this. The change makes the device model more readable.. It's purely cosmetic and keeps coherency with the device title used in the OpenWrt wiki. > > > > compatible = "netgear,dgnd3700-v1", "brcm,bcm6368"; > > > > > > > > aliases { > > > > led-boot = &led_power_green; > > > > - led-failsafe = &led_power_green; > > > > + led-failsafe = &led_power_red; > > > > > > This should be a separate commit again. > > > > > > > One commit per line, on the same file, really?, again this is a minor > > change. > > Yes, but it's a non-cosmetic (with respect to its effect) behavior change > that is not at all connected to the rest. Might be better patching all boards, having multicolor power leds, at the same time? > > Just imagine we want to backport this or the fixed port order to 19.07, but > don't want to mess with LED names. Separate commits for separate topics make > sense. > I can imagine it. But being realistic it won't ever happen. > > > > > > led-running = &led_power_green; > > > > led-upgrade = &led_power_green; > > > > }; > > > > @@ -51,49 +51,49 @@ > > > > leds { > > > > compatible = "gpio-leds"; > > > > > > > > - dsl_green { > > > > - label = "DGND3700v1_3800B:green:dsl"; > > > > + led@2 { > > > > > > I don't know whether this is different on bcm63xx, but based on what I'm > > used to the old node name is preferred (dsl_green). > > > > > > > Well, I'll never know what's the best way for naming a led node. I've taken > > the partitions nodes as a reference, and the LEDs device tree documentation > > also use this way as an example. > > Openwrt seems to generally use the name-based scheme (the one already there), > and since there is no reason for changing that and it's unconnected to the > rest of your changes, please just drop these changes (of course, only the > node name, not the label changes). I've really made this change to keep coherency with his brother DGND3700v2, supported since about a week ago, also belonging to this target. Regards > > Best > > Adrian > > > > > Regards > > > > > Best > > > > > > Adrian > > > > > > > + label = "dgnd3700-v1:green:dsl"; > > > > gpios = <&pinctrl 2 1>; > > > > }; > > > > - inet_red { > > > > - label = "DGND3700v1_3800B:red:inet"; > > > > + led@4 { > > > > + label = "dgnd3700-v1:red:inet"; > > > > gpios = <&pinctrl 4 1>; > > > > }; > > > > - inet_green { > > > > - label = "DGND3700v1_3800B:green:inet"; > > > > + led@5 { > > > > + label = "dgnd3700-v1:green:inet"; > > > > gpios = <&pinctrl 5 1>; > > > > }; > > > > - wps_green { > > > > - label = "DGND3700v1_3800B:green:wps"; > > > > + led@11 { > > > > + label = "dgnd3700-v1:green:wps"; > > > > gpios = <&pinctrl 11 1>; > > > > }; > > > > - usbfront_green { > > > > - label = "DGND3700v1_3800B:green:usb-front"; > > > > + led@13 { > > > > + label = "dgnd3700-v1:green:usb-front"; > > > > gpios = <&pinctrl 13 1>; > > > > }; > > > > - usbback_green { > > > > - label = "DGND3700v1_3800B:green:usb-back"; > > > > + led@14 { > > > > + label = "dgnd3700-v1:green:usb-back"; > > > > gpios = <&pinctrl 14 1>; > > > > }; > > > > - power_red { > > > > - label = "DGND3700v1_3800B:red:power"; > > > > + led_power_red: led@22 { > > > > + label = "dgnd3700-v1:red:power"; > > > > gpios = <&pinctrl 22 1>; > > > > }; > > > > - lan_green { > > > > - label = "DGND3700v1_3800B:green:lan"; > > > > + led@23 { > > > > + label = "dgnd3700-v1:green:lan"; > > > > gpios = <&pinctrl 23 1>; > > > > }; > > > > - led_power_green: power_green { > > > > - label = "DGND3700v1_3800B:green:power"; > > > > + led_power_green: led@24 { > > > > + label = "dgnd3700-v1:green:power"; > > > > gpios = <&pinctrl 24 1>; > > > > default-state = "on"; > > > > }; > > > > - wifi2g_green { > > > > - label = "DGND3700v1_3800B:green:wifi2g"; > > > > + led@26 { > > > > + label = "dgnd3700-v1:green:wifi2g"; > > > > gpios = <&pinctrl 26 1>; > > > > }; > > > > - wifi5g_blue { > > > > - label = "DGND3700v1_3800B:blue:wifi5g"; > > > > + led@27 { > > > > + label = "dgnd3700-v1:blue:wifi5g"; > > > > gpios = <&pinctrl 27 1>; > > > > }; > > > > }; > > > > @@ -107,25 +107,25 @@ > > > > #address-cells = <1>; > > > > #size-cells = <1>; > > > > > > > > - cfe@0 { > > > > + partition@0 { > > > > label = "CFE"; > > > > reg = <0x000000 0x020000>; > > > > read-only; > > > > }; > > > > > > > > - linux@20000 { > > > > + partition@20000 { > > > > label = "linux"; > > > > reg = <0x020000 0x1e20000>; > > > > compatible = "brcm,bcm963xx-imagetag"; > > > > }; > > > > > > > > - board_data@1e40000 { > > > > + partition@1e40000 { > > > > label = "board_data"; > > > > reg = <0x1e40000 0x1a0000>; > > > > read-only; > > > > }; > > > > > > > > - nvram@1fe0000 { > > > > + partition@1fe0000 { > > > > label = "nvram"; > > > > reg = <0x1fe0000 0x20000>; > > > > }; > > > > @@ -156,22 +156,22 @@ > > > > > > > > lan@1 { > > > > reg = <1>; > > > > - label = "lan1"; > > > > + label = "lan4"; > > > > }; > > > > > > > > lan@2 { > > > > reg = <2>; > > > > - label = "lan2"; > > > > + label = "lan3"; > > > > }; > > > > > > > > lan@3 { > > > > reg = <3>; > > > > - label = "lan3"; > > > > + label = "lan2"; > > > > }; > > > > > > > > lan@4 { > > > > reg = <4>; > > > > - label = "lan4"; > > > > + label = "lan1"; > > > > }; > > > > > > > > cpu@8 { > > > > diff --git a/target/linux/bcm63xx/patches-5.4/549- > > > > board_DGND3700v1_3800B.patch b/target/linux/bcm63xx/patches- > > 5.4/549- > > > > board_DGND3700v1_3800B.patch > > > > index 936aab115b..7569e9643e 100644 > > > > --- a/target/linux/bcm63xx/patches-5.4/549- > > > > board_DGND3700v1_3800B.patch > > > > +++ b/target/linux/bcm63xx/patches-5.4/549- > > > > board_DGND3700v1_3800B.patch > > > > @@ -5,7 +5,7 @@ > > > > }; > > > > > > > > +static struct board_info __initdata board_DGND3700v1_3800B = { > > > > -+ .name = "DGND3700v1_3800B", > > > > ++ .name = "U12L144T01", > > > > + .expected_cpu_id = 0x6368, > > > > + > > > > + .has_pci = 1, > > > > -- > > > > 2.26.2 > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > openwrt-devel mailing list > > > > openwrt-devel@lists.openwrt.org > > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
