ramips: add D-Link DIR-506L support #3191
Conversation
|
Does it work with testing kernel 5.4 too? |
I do checks only with default 4.14*. Also I found that (defaults: 2, 1024) |
|
|
||
| keys { | ||
| compatible = "gpio-keys-polled"; | ||
| poll-interval = <20>; |
There was a problem hiding this comment.
Can you try with gpio-keys for compatible and dropping poll-interval?
There was a problem hiding this comment.
I check other rt5350 based devices dts files, they all use compatible = "gpio-keys-polled";.
There was a problem hiding this comment.
Newly added keenetic lite b uses it. https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/rt5350_zyxel_keenetic-lite-b.dts#L35
And I'm pretty sure it'll also work on your device.
There was a problem hiding this comment.
Ok, I will make build with this and spi change and test on real h/w.
But I remember that polling required for this chip, do not remember where I read this.
There was a problem hiding this comment.
You are using battery status gpios as keys here, and I'm not sure it will fit in this case... Typically keys are for state transition (except for EV_SW).
There was a problem hiding this comment.
https://openwrt.org/docs/guide-user/hardware/hardware.button - Sliding switches
https://github.com/jefferyto/openwrt-slide-switch
this can be handled by scripts.
TP-Link TL-MR3020 and some others use GPIO input as switch.
There was a problem hiding this comment.
Yeah, TL-MR3020 has a real hardware slide switch...
There was a problem hiding this comment.
To me, using gpio-export here also seems more desirable if it can be done properly.
There was a problem hiding this comment.
Is any disadvantage for this, except that this is not physical button/switch?
Is there any devices that uses GPIO input from non button/switch in base to use it as example?
For me is clear how to use this buttons, and examples exist in wiki, and usage of generic GPIO input is not.
There was a problem hiding this comment.
The disadvantage is that it's a hack. These are no buttons, so they shouldn't be implemented as such.
I won't merge it this way.
| ralink,mtd-eeprom = <&config 0xe090>; | ||
| ralink,led-polarity = <1>; | ||
| mtd-mac-address = <&config 0xe084>; | ||
| mtd-mac-address-increment = <(1)>; |
There was a problem hiding this comment.
Will you get a correct MAC address if you just drop mtd-mac-address here? (so, it's taken from the eeprom automatically)
There was a problem hiding this comment.
I can check, but IMHO this required.
Other devices read MAC from config or do almost same via ramips_setup_macs() in 02_network with variations.
Vendor set same MAC for lan and wifi netif, in ramips_setup_macs() no case for it.
| ucidef_set_led_gpio "lo_bat" "Low battery" "${boardname}:red:status" "10" "0" | ||
| ucidef_set_led_default "status" "Status" "${boardname}:green:status" "1" | ||
| ucidef_set_led_netdev "lan" "LAN" "${boardname}:green:usb" "eth0" "tx rx" | ||
| ucidef_set_led_netdev "wifi" "WiFi" "rt2800soc-phy0::radio" "wlan0" "tx rx" |
There was a problem hiding this comment.
I'm trying to avoid make cristmass tree form device, it already have one green led that always on after boot.
Also this saves few mWatts battery power.
|
Though I left some comments here, be aware that I probably won't merge this; since this is a 32 MB RAM device, you might have a hard time finding somebody to merge it. |
rozhuk-im
force-pushed
the
dir-506l
branch
3 times, most recently
from
21a1c73
to
0ee0600
Compare
| @@ -66,6 +66,12 @@ asiarf,awapn2403) | |||
| dlink,dcs-930l-b1) | |||
| ucidef_set_led_netdev "wifi" "WiFi" "$boardname:blue:wps" | |||
| ;; | |||
| dlink,dir-506l) | |||
| ucidef_set_led_gpio "lo_bat" "Low battery" "${boardname}:red:status" "10" "0" | |||
There was a problem hiding this comment.
Device have battery, this rule connect internal gpio "button" (event?) to red status led to show that battery have low power.
Also charging and on_battery available, but I do not know how to make green led blink on charging.
There was a problem hiding this comment.
boardname can be dropped here (see comment for LEDs in DTS).
|
Please don't just close all the comments, no matter whether they are resolved or not. Those are meant to be discussed, if you just close anything it will look like you want to hide the problems of your proposal. |
target/linux/ramips/image/rt305x.mk
Outdated
| DEVICE_VENDOR := D-Link | ||
| DEVICE_MODEL := DIR-506L | ||
| DEVICE_PACKAGES := jboot-tools kmod-usb2 kmod-spi-dev \ | ||
| kmod-usb-serial-option kmod-usb-net-cdc-ether comgt-ncm \ |
There was a problem hiding this comment.
I'm not familiar with it, do you really require comgt-ncm here?
If yes, you may drop kmod-usb-serial-option here as well.
There was a problem hiding this comment.
I think we usually don't include any 3G/4G modem related packages unless the modem is actually built-in.
Just checked DIR-510L and DWR-116, and there are no WWAN-related packages included for those devices either, although they are advertised to support 4G (just because the OEM firmware has the drivers included).
Anyways, great to see a new PR for this device (I actually thought the first one was closed due to 32 MiB RAM back then).
There was a problem hiding this comment.
There is exactly one device in entire ramips that adds comgt-ncm by default. If the modem is not built-in as stated, we should remove it here as well.
rozhuk-im
force-pushed
the
dir-506l
branch
3 times, most recently
from
809838c
to
af6857d
Compare
|
I got message: "Switch switch0 has an unknown topology - the VLAN settings might not be accurate." and LUCI show switch with 5+cpu ports. |
rozhuk-im
force-pushed
the
dir-506l
branch
3 times, most recently
from
e2ee5de
to
3dc00c6
Compare
So, you closed everything as resolved again? Please don't hide critical opinions by just resolving them. If you receive feedback and don't apply it, the comment should stay unresolved until the very end, so everybody can see it and think about it. This is your final warning. Edit: I just saw that you actually removed comgt-ncm already, so my comment above only applies to the keys vs. gpio-export discussion, and I might have worded it a bit weaker then, as it's only a single case. Anyway, I had to spend time to go through all closed comment to find this out... |
Sorry, misunderstanding from my side. |
From my personal understanding, either when you have been asked for a change and you did that change, or when both person giving feedback and you agreed that the subject is settled. |
I was thinking that after answer I should mark as resolved, and if there are more questions then some one add comment and mark it as unresolved. And I was miss you prev message to not mark as resolved. |
Just imagine somebody else coming to this PR who hasn't followed it beforehand. If everything is "resolved", it will look like everything is fine, though there actually are point which are unclear or where reviewer disagreed with you so far. He might just overlook particular problems when going through the code. In contrast, if the relevant section are still open, scanning through them will give the reviewer a first impression about where he/she has to look specifically or may provide help; apart from reviewing, people might give you specific advice on open problems without having to look through everything. |
|
While I don't mean to add further complexity to this PR, I was wondering what is the preferred way of setting GPIOs for USB ports. When adding support for DIR-510L, it was mentioned that Thus, for DIR-510L the USB GPIOs were added via In ath79, there are some devices with external hub ports declared (e.g. |
|
I have DIR-510L and use it with patched dts file, that have gpio_export section with reset and both usb ports power control. How to find in LUCI web UI things from |
This subtarget needs to move to 5.4 soon, or it will be removed anyway. So, this should be checked with 5.4. Testing support is already enabled, just enable testing kernel in "Global build settings". |
| @@ -0,0 +1,159 @@ | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
| /dts-v1/; | |||
There was a problem hiding this comment.
dts-v1 can be dropped, it's in dtsi now.
| compatible = "gpio-leds"; | ||
|
|
||
| usb { | ||
| label = "dir-506l:green:usb"; |
There was a problem hiding this comment.
Model (dir-506l:) can be dropped from LED label now, so you can just keep "green:usb" etc.
Specifications: - Ralink RT5350 id:1 rev:3 (360 MHz) - 32 MB RAM - 8 MB Flash - 802.11bgn 1T1R - 1x internal, non-detachable antenna - 1x 10/100 Mbps Ethernet - 1x USB 2.0 (Type-A) - 5 leds - 2 buttons - 3 gpio power events - UART header on PCB (57600 8n1) - JBOOT bootloader - 1700 - 2200 mAh battery (Fujifilm NP-120 / Pentax D-Li7, D-L17, DB-43) Features: - upgrade from stock fw via web gui - all leds avaible - all buttons work - battery/charger gpio events added - red led on battery low - USB power on by default - keep jboot recovery Installation: Apply factory image via http web-gui or JBOOT recovery page How to revert to OEM firmware: - push the reset button and turn on the power. Wait until LED start blinking (~10sec.) - upload original factory image via JBOOT http (IP: 192.168.123.254) If http doesn't work, it can be done with curl command: curl -F FN=@XXXXX.bin http://192.168.123.254/upg where XXXXX.bin is name of firmware file. Signed-off-by: Rozhuk Ivan <rozhuk.im@gmail.com>
|
@rozhuk-im Ping. Please address the various open issues. Time is not on your side if you want to add a 32 MiB RAM device... |
adschm
added
the
stale
|
Since 21.02 has been branched and there is no response here, I'm closing this now... |
Specifications:
Features:
Installation:
Apply factory image via http web-gui or JBOOT recovery page
How to revert to OEM firmware:
If http doesn't work, it can be done with curl command:
curl -F FN=@XXXXX.bin http://192.168.123.254/upg
where XXXXX.bin is name of firmware file.
Signed-off-by: Rozhuk Ivan rozhuk.im@gmail.com