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.

> >
> > 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?

> >       ;;
> >  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 ...
>
> >       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.

> >               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.

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

Reply via email to