[OpenWrt-Devel] [PATCH 3/3] ath79: add support for Teltonika RUT955

[Daniel Golle]

On Sun, Feb 02, 2020 at 06:15:41PM +0100, mail at adrianschmutzler.de wrote:
> Hi Daniel,
> 
> several comments inline below.
> > +	compatible = "tlt,rut955", "qca,ar9344";
> 
> I would prefer having the full vendor name here (teltonika,rut955), as we tried to avoid abbreviations in names for ath79 wherever possible (with the exception of ubnt).

Ack.

> > ...
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		signal0 {
> > +			label = "rut955:green:signal1";
> 
> I'd prefer having the same numbering for node and label here (signal0<>signal1).

Ack.
> > ...
> > +
> > +		led_system_red: systemred {
> 
> Please add an underscore: system_red
Ack.

> 
> > +			label = "rut955:green:red";
> 
> This should be rut955:red:system ?

Oh yes. Ack.

> > ...
> > +			gpios = <&gpio_ext 5 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> 
> Default-state "off" is default, so I think this is not needed (also above).

Ack, removed.

> > ...
> > +			partition at f70000 {
> > +				label = "event-log";
> > +				reg = <0xf70000 0x80000>;
> > +			};
> 
> There are 0x10000 left empty at the end?
> Is this writeable by intention?

No, length should be 0x90000 instead, was a typo.
Also set read-only as this is not being used by OpenWrt.

> > +&wmac {
> > +	status = "okay";
> > +
> > +	mtd-cal-data = <&art 0x1000>;
> > +	mtd-mac-address = <&config 0x0>;
> > +	mtd-mac-address-increment = <2>;
> 
> No other addresses available in flash? art 0x1002 not usable?

00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00001000  02 02 00 02 03 04 05 06  00 30 3a 30 32 3a 30 33  |.........0:02:03|
00001010  3a 30 34 3a 30 35 3a 30  36 00 00 00 00 00 1f 00  |:04:05:06.......|

Doesn't look useful to me :(
> 
> Please use newer syntax:
> 
> DEVICE_VENDOR := Teltonika
> DEVICE_MODEL := RUT955

Ack.

> 
> > +  DEVICE_PACKAGES := kmod-usb2 kmod-usb-acm  kmod-usb-net-qmi-
> > wwan
> 
> Double space after -acm; I'd like a line break here.

Fixed.

> 
> > +kmod-usb-serial-option kmod-hwmon-mcp3021 uqmi -uboot-envtools
> > +  IMAGE_SIZE := 15552k
> > +  TPLINK_HWID := 0x35000001
> > +  TPLINK_HWREV := 0x1
> > +  TPLINK_HEADER_VERSION := 1
> > +  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header
> > +  KERNEL_INITRAMFS := kernel-bin | append-dtb | lzma | uImage lzma
> > +  IMAGES := sysupgrade.bin factory.bin
> 
> Could also use "IMAGES += factory.bin" here.

True.

I will repost v2 shortly after testing it with all changes suggested
by Piotr and you applied.

Thanks to both of you for the instant review!


Cheers


Daniel

> 
> Best
> 
> Adrian
> 

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel