[OpenWrt-Devel] [PATCH 3/3] ath79: Extend GL.iNet AR750S support to NAND file system

[Petr Štetiar]

Jeff Kletsky <lede at allycomm.com> [2019-05-14 15:39:56]:

> diff --git a/target/linux/ath79/base-files/etc/board.d/01_leds b/target/linux/ath79/base-files/etc/board.d/01_leds
> index 9c353baabe..c974c12d14 100755
> --- a/target/linux/ath79/base-files/etc/board.d/01_leds
> +++ b/target/linux/ath79/base-files/etc/board.d/01_leds
> @@ -78,6 +78,10 @@ glinet,gl-ar300m-nor)
>  glinet,gl-ar300m-lite)
>  	ucidef_set_led_netdev "lan" "LAN" "gl-ar300m-lite:green:lan" "eth0"
>  	;;
> +glinet,gl-ar750s-*)
> +	ucidef_set_led_netdev "wlan2g" "WLAN 2G" "gl-ar750s:green:wlan2g"
> +	ucidef_set_led_netdev "wlan5g" "WLAN 5G" "gl-ar750s:green:wlan5g"

why do you need this? It's already being set in the DTS by the LED triggers,
isn't it? Having it defined in DTS is preferred.

> +# During image creation the "board name" is of the format mfgr_board-name
> +# However, on a running device it is of the format mfgr,board-name

That's already clear from the code, so drop this comment.

> +comma_to_underscore() {
> +	echo "${1%%,*}_${1#*,}"
> +}
> +
>  platform_check_image() {
> -	return 0
> +	local board=$(board_name)
> +
> +	case "$board" in
> +	glinet,gl-ar750s-nand)
> +		nand_do_platform_check "$(comma_to_underscore "$board")" "$IMAGE"
> +		;;
> +	*)
> +		return 0
> +		;;
> +	esac
>  }

I think, that it would be better to add support for DT compatible based
board_name format directly into nand_do_platform_check, so it could be reused
by other platforms as well.

> diff --git a/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts b/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dtsi
> similarity index 64%
> rename from target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts
> rename to target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dtsi
> index 378de5de90..e38879182e 100644
> --- a/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts
> +++ b/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dtsi
> @@ -15,10 +15,10 @@
>  	};
>  
>  	aliases {
> -		led-boot = &power;
> -		led-failsafe = &power;
> -		led-running = &power;
> -		led-upgrade = &power;
> +		led-boot = &led_power;
> +		led-failsafe = &led_wlan2g;
> +		led-running = &led_power;
> +		led-upgrade = &led_wlan5g;
>  	};

Please don't do this, those LEDs have defined functions and using wlan5g LED
to signal an upgrade might be confusing. It has been discussed a lot of times
already.

> +	i2c at 0 {
> +		compatible = "i2c-gpio";
> +
> +		sda-gpios = <&gpio  5 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		scl-gpios = <&gpio 21 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		i2c-gpio,delay-us = <2>;	/* ~100 kHz */

What was wrong with the default value? Have you actually checked the resulting
clock frequency somehow?

> +		#address-cells = <1>;
> +		#size-cells = <0>;

The #address-cells and #size-cells properties may be used in any device node
that has children in the devicetree hierarchy and describes how child device
nodes should be addressed. The #address-cells property defines the number of
<u32> cells used to encode the address field in a child node’s reg property.
The #size-cells property defines the number of <u32> cells used to encode the
size field in a child node’s reg property.

So you can drop this.

> diff --git a/target/linux/ath79/image/nand.mk b/target/linux/ath79/image/nand.mk
> index e69de29bb2..7db5f51c98 100644
> --- a/target/linux/ath79/image/nand.mk
> +++ b/target/linux/ath79/image/nand.mk
> @@ -0,0 +1,15 @@
> +define Device/glinet_gl-ar750s-nand
> +  ATH_SOC := qca9563
> +  DEVICE_TITLE := GL.iNet GL-AR750S
> +  DEVICE_PACKAGES := kmod-usb2 kmod-ath10k-ct ath10k-firmware-qca9887-ct kmod-i2c-gpio
> +  KERNEL_SIZE := 2048k
> +  BLOCKSIZE := 128k
> +  PAGESIZE := 2048
> +  VID_HDR_OFFSET := 2048
> +  IMAGES += factory.img
> +  IMAGE/sysupgrade.bin := sysupgrade-tar | append-metadata
> +  IMAGE/factory.img := append-kernel | pad-to $$$$(KERNEL_SIZE) | append-ubi
> +  SUPPORTED_DEVICES += gl-ar750s

I'm really not sure about this. Do we've enough checks in place, that we won't
allow sysupgrade from NOR?

-- ynezz

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