Jump to conversation
Unresolved conversations (62)
@adschm adschm Sep 11, 2020
Just pulled the PR and ran "make kernel_oldconfig" on it, changed quite a bit. Didn't touch CONFIG_SWCONFIG yet ...
target/linux/rtl838x/config-5.4
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...h/mips/include/asm/mach-rtl838x/ioremap.h
@adschm adschm Sep 11, 2020
WARNING: simple_strtoul is obsolete, use kstrtoul instead
Outdated
...838x/files-5.4/arch/mips/rtl838x/serial.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments can be removed?
Outdated
...38x/files-5.4/drivers/gpio/gpio-rtl838x.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments Can be removed?
Outdated
...l838x/files-5.4/drivers/net/dsa/rtl838x.h
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments Can be removed?
Outdated
...x/files-5.4/drivers/net/dsa/rtl838x_phy.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments see earlier comment.
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments Can be removed?
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments Might be removed anyway?
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
ERROR: space prohibited after that open parenthesis '('
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
WARNING: quoted string split across lines
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
checkpatch.pl: `ERROR: do not use C99 // comments`
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
From checkpatch.pl: `WARNING: externs should be avoided in .c files`
...es-5.4/drivers/net/ethernet/rtl838x_eth.c
@adschm adschm Sep 11, 2020
From checkpatch.pl: `ERROR: do not use C99 // comments`
...es-5.4/drivers/net/ethernet/rtl838x_eth.c
@adschm adschm Sep 11, 2020
From checkpatch.pl: `ERROR: do not use C99 // comments` Is this needed anyway? Same comment for line 84.
Outdated
...es-5.4/drivers/net/ethernet/rtl838x_eth.h
@adschm adschm Sep 11, 2020
From checkpatch.pl: `ERROR: return is not a function, parentheses are not required` here and several cases below.
...es-5.4/drivers/net/ethernet/rtl838x_eth.h
adschm
@adschm adschm Sep 11, 2020
Can you post an example /etc/board.json and /etc/config/network created from this script?
...rtl838x/base-files/etc/board.d/02_network
adschm
@adschm adschm Sep 11, 2020
Missing space after hash.
Outdated
target/linux/rtl838x/config-5.4
@adschm adschm Sep 11, 2020
That shouldn't be required as well for pure DSA.
target/linux/rtl838x/config-5.4
adschm
@adschm adschm Sep 11, 2020
This double empty line could be removed if touched again.
Outdated
...ux/rtl838x/base-files/etc/board.d/01_leds
@adschm adschm Sep 9, 2020
Though other files do it differently, I'd add an empty line here before the eval.
target/linux/rtl838x/profiles/00-default.mk
adschm
@adschm adschm Sep 9, 2020
Generally strange indent/tabs in this file. But I don't see any reason to have tabs directly after "define" here.
...s/include/asm/mach-rtl838x/mach-rtl838x.h
adschm rmilecki
@adschm adschm Sep 9, 2020
what about 0x9a0000 till the end?
Outdated
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
adschm
@adschm adschm Sep 9, 2020
Can this go faster?
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
Off-topic here, but if you use mtd_get_mac_binary you can also use mtd-mac-address in DTS which would be preferred.
Outdated
...rtl838x/base-files/etc/board.d/02_network
@dengqf6 dengqf6 Sep 2, 2020
Does this switch support setting up VLANs when VLAN filtering is disabled? If yes, add `ds->configure_vlan_while_not_filtering = true;` here
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
You can use `%pM` instead of multiple `%02x`s to print a MAC address.
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Please remove the commented-out `printk`, or use `dev_dbg` instead.
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Please use `dev_err` to print error messages.
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
What about `-ENOSPC`?
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Please check for existent entries and return `-EEXIST` before adding a new one.
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Why do you remove both ingress and egress entries without checking `mirror->ingress`?
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Ditto
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
Please move the lock further down, just before the sw_w32 function.
Outdated
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@dengqf6 dengqf6 Sep 2, 2020
If the GPIO controller supports interrupt, use `"gpio-keys"` instead and remove the `poll-interval`
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@hauke hauke Sep 1, 2020
normally you should get the based address of this IP block from the device tree and then only use the offsets here.
Outdated
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@hauke hauke Sep 1, 2020
Please do not overwrite this file but add a patch
Outdated
...x/rtl838x/files-5.4/drivers/gpio/Makefile
@hauke hauke Sep 1, 2020
Please do not overwrite this file, but add a patch with your changes.
Outdated
...ux/rtl838x/files-5.4/drivers/gpio/Kconfig
@hauke hauke Sep 1, 2020
It looks like flow is only written ad never read.
...838x/files-5.4/arch/mips/rtl838x/serial.c
@hauke hauke Sep 1, 2020
Please run "./scripts/checkpatch.pl" from the kernel over the kernel changes.
Outdated
...tl838x/files-5.4/arch/mips/rtl838x/prom.c
@hauke hauke Sep 1, 2020
Do you use swconfig or DSA or both?
target/linux/rtl838x/Makefile
adschm
@hauke hauke Sep 1, 2020
Do you know if this format was invented by allnet or is this from Realtek? If this is seen on different Realtek switches, you should probably rename this to Realtek.
...es/drivers/mtd/mtdsplit/mtdsplit_uimage.c
@adschm adschm Sep 1, 2020
Typically, devices can go faster than 10 MHz and it's a good idea to do so, as this will improve read speed considerably.
Outdated
...rtl838x/dts/rtl8380_zyxel_gs1900-10hp.dts
adschm
@adschm adschm Sep 1, 2020
It's recommended to add one empty line after status (generally).
Outdated
...rtl838x/dts/rtl8380_zyxel_gs1900-10hp.dts
@adschm adschm Sep 1, 2020
port@8? also below?
Outdated
...rtl838x/dts/rtl8380_zyxel_gs1900-10hp.dts
@adschm adschm Sep 1, 2020
Shouldn't that be port@8 and similarly below?
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
adschm
@adschm adschm Sep 1, 2020
This was not about removing the other variables, but about the pointless initialization to "". Please add lan_mac and wan_mac back, but drop the ="".
Outdated
...rtl838x/base-files/etc/board.d/02_network
adschm
@adschm adschm Sep 1, 2020
I don't think ip-full and tc will be acceptable for default setup.
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 1, 2020
$$$$(IMAGE_SIZE) can be dropped, check-size defaults to that.
Outdated
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 1, 2020
Did you explain somewhere why you don't just use the generic mtdsplit.c split driver?
Outdated
...files-5.4/drivers/mtd/mtdsplit/mtdsplit.c
@adschm adschm Sep 1, 2020
These DT labels don't seem to be required? Just lan1g should be enough?
Outdated
...l838x/dts/rtl8380_tp-link_t2500g-10ts.dts
@adschm adschm Sep 1, 2020
Please add empty lines between nodes.
Outdated
...l838x/dts/rtl8380_tp-link_t2500g-10ts.dts
@adschm adschm Sep 1, 2020
T2500 or T2500G?
Outdated
...l838x/dts/rtl8380_tp-link_t2500g-10ts.dts
@adschm adschm Sep 1, 2020
Please add a SPDX license identifier for all DTS(I) files.
Outdated
...l838x/dts/rtl8380_tp-link_t2500g-10ts.dts
@adschm adschm Sep 1, 2020
That looks like a hack?
Outdated
.../lib/preinit/07_set_preinit_iface_rtl838x
@adschm adschm Sep 1, 2020
preinit are library files, so should be non-executable and thus not require a shebang (and remove the copyright as stated above).
Outdated
.../lib/preinit/07_set_preinit_iface_rtl838x
@adschm adschm Sep 1, 2020
Please use $() syntax instead of deprecated backticks (also below). I'm not sure whether this kind of network setup is acceptable, though.
Outdated
...rtl838x/base-files/etc/board.d/02-network
@adschm adschm Sep 1, 2020
The offset looks like a type (haven't checked), is it correct?
Outdated
...rtl838x/base-files/etc/board.d/02-network
@adschm adschm Sep 1, 2020
Just `local label_mac` should be enough
Outdated
...rtl838x/base-files/etc/board.d/02-network
@adschm adschm Sep 1, 2020
dlink (see tplink comment above)
Outdated
...rtl838x/base-files/etc/board.d/02-network
@adschm adschm Sep 1, 2020
Please stick to the underscore for the file name (02_network)
Outdated
...rtl838x/base-files/etc/board.d/02-network
@adschm adschm Sep 1, 2020
Please use "tplink" (without hyphen) in the board name (https://www.kernel.org/doc/Documentation/devicetree/bindings/vendor-prefixes.txt)
Outdated
...ux/rtl838x/base-files/etc/board.d/01_leds
Resolved conversations (51)
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt same at line 150
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...s/include/asm/mach-rtl838x/mach-rtl838x.h
@adschm adschm Sep 11, 2020
WARNING: externs should be avoided in .c files also above and below
...rtl838x/files-5.4/arch/mips/rtl838x/irq.c
@adschm adschm Sep 11, 2020
WARNING: braces {} are not necessary for single statement blocks
...rtl838x/files-5.4/arch/mips/rtl838x/irq.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments
...tl838x/files-5.4/arch/mips/rtl838x/prom.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments WARNING: externs should be avoided in .c files
...tl838x/files-5.4/arch/mips/rtl838x/prom.c
@adschm adschm Sep 11, 2020
Remove empty line at EOF
...tl838x/files-5.4/arch/mips/rtl838x/prom.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments
...838x/files-5.4/arch/mips/rtl838x/serial.c
@adschm adschm Sep 11, 2020
WARNING: externs should be avoided in .c files
...838x/files-5.4/arch/mips/rtl838x/serial.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments
...l838x/files-5.4/arch/mips/rtl838x/setup.c
@adschm adschm Sep 11, 2020
WARNING: externs should be avoided in .c files
...l838x/files-5.4/arch/mips/rtl838x/setup.c
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...l838x/files-5.4/arch/mips/rtl838x/setup.c
@adschm adschm Sep 11, 2020
do not use C99 // comments
...38x/files-5.4/drivers/gpio/gpio-rtl838x.c
@adschm adschm Sep 11, 2020
externs should be avoided in .c files
...38x/files-5.4/drivers/gpio/gpio-rtl838x.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments ERROR: trailing statements should be on next line
...38x/files-5.4/drivers/gpio/gpio-rtl838x.c
@adschm adschm Sep 11, 2020
// comment
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@adschm adschm Sep 11, 2020
WARNING: externs should be avoided in .c files
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
Outdated
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments 3 times
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments
Outdated
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments see earlier comment.
...x/files-5.4/drivers/net/dsa/rtl838x_phy.c
@adschm adschm Sep 11, 2020
ERROR: return is not a function, parentheses are not required
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
WARNING: suspect code indent for conditional statements (8, 8)
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
ERROR: do not use C99 // comments
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
checkpatch.pl: `ERROR: do not use C99 // comments`
...8x/files-5.4/drivers/net/dsa/rtl838x_sw.c
@adschm adschm Sep 11, 2020
Get `ERROR: do not use C99 // comments` here as well. However, I don't know what's the proper way for the SPDX license in c files.
...es-5.4/drivers/net/ethernet/rtl838x_eth.c
@rmilecki rmilecki Sep 10, 2020
What's the point of that custom `spi_read_id()`? Below you call `spi_nor_scan()` with `NULL` as name. That results in reading flash ID anyway.
Outdated
...les-5.4/drivers/mtd/spi-nor/rtl838x-nor.c
@rmilecki rmilecki Sep 10, 2020
Can you drop `.type` assignment (and verify it works)? `type` is required for pre-DT devices only that don't specify `firmware` partition type directly. As long as you use `allnet,uimage` in DT there should be no need for the `MTD_PARSER_TYPE_FIRMWARE`.
Outdated
...es/drivers/mtd/mtdsplit/mtdsplit_uimage.c
@adschm adschm Sep 9, 2020
drop second empty line here.
Outdated
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 9, 2020
Please wrap after 74 or 80 characters per line: ``` IMAGE/sysupgrade.bin := append-kernel | pad-to 64k | append-rootfs | \ pad-rootfs | append-metadata | check-size ``` Hanging indent with one single tab in this case (Device nodes).
Outdated
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 9, 2020
I'm not a fan of these "shortcut" variables, and since you don't actually need it (yet), I'd prefer to remove it.
Outdated
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 9, 2020
SUPPORTED_DEVICES should be moved (up), so IMAGES is next to the IMAGE/ definitions.
Outdated
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 9, 2020
The Build blocks should be before any Device blocks (even Device/Default). You may add _two_ empty lines after the Build blocks to separate them from the Device blocks (as an exception).
target/linux/rtl838x/image/Makefile
@adschm adschm Sep 9, 2020
Remove empty line at EOF?
Outdated
...838x/files-5.4/arch/mips/rtl838x/Makefile
@adschm adschm Sep 9, 2020
Add empty line before fixed-link node
target/linux/rtl838x/dts/rtl838x.dtsi
@adschm adschm Sep 9, 2020
@2a0000 instead of @b2a0000
Outdated
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
0x1a0000 instead of 0x160000?
Outdated
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
First part needs to be full "model", i.e. "all-sg8208m"
Outdated
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
Is the mem bootarg required? Can this be replaced by a memory node?
Outdated
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
Add empty line before reset node.
...tl838x/dts/rtl8382_allnet_all-sg8208m.dts
@adschm adschm Sep 9, 2020
Just keep the allnet,all-sg8208m, as the others are not added yet.
...rtl838x/base-files/etc/board.d/02_network
@adschm adschm Sep 9, 2020
Please use $() instead of backticks
Outdated
...rtl838x/base-files/etc/board.d/02_network
@adschm adschm Sep 9, 2020
Please use $() instead of backticks
Outdated
...rtl838x/base-files/etc/board.d/02_network
@adschm adschm Sep 9, 2020
tplink device is not included anymore, but if you like you can already set up this file just with ``` [...] case $board in esac [...] ```
...ux/rtl838x/base-files/etc/board.d/01_leds
@adschm adschm Sep 9, 2020
I don't think this include is needed?
Outdated
...ux/rtl838x/base-files/etc/board.d/01_leds
@adschm adschm Sep 1, 2020
Remove execute bit on this file.
...tl838x/base-files/lib/upgrade/platform.sh
@adschm adschm Sep 1, 2020
You are not affiliated with OpenWrt, so please remove all the "LEDE" and "OpenWrt.org" copyright notices.
Outdated
target/linux/rtl838x/Makefile