[OpenWrt-Devel] [PATCH v2] ath79: add D-Link DIR-615 E4

[Adrian Schmutzler]

Hi Paul,

[...]

> +	aliases {
> +		led-boot = &power_amber;

Please include "led_" prefix for the labels, so &led_power_amber in this case.

> +		led-failsafe = &power_amber;
> +		led-running = &power_green;
> +		led-upgrade = &power_amber;
> +	};
> +

[...]

> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&switch_led_pins>;
> +
> +		power_green: power_green {

As stated above, change the _label_ to include a "led_" prefix, so this becomes "led_power_green: power_green {". Same for power_amber below.

> +			label = "dir-615-e4:green:power";

Sorry for causing confusion here. I have had a look into ar71xx mach files and they consistent use "d-link" as vendor for the led labels. Thus, I think it makes more sense to revert that to the previous version "d-link:green:power".

> +			gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		power_amber: power_amber {
> +			label = "dir-615-e4:amber:power";
> +			gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		wps {
> +			label = "dir-615-e4:blue:wps";
> +			gpios = <&gpio 0 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		lan1 {
> +			label = "dir-615-e4:green:lan1";
> +			gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		lan2 {
> +			label = "dir-615-e4:green:lan2";
> +			gpios = <&gpio 14 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		lan3 {
> +			label = "dir-615-e4:green:lan3";
> +			gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		lan4 {
> +			label = "dir-615-e4:green:lan4";
> +			gpios = <&gpio 16 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		wan_amber {
> +			label = "dir-615-e4:amber:wan";
> +			gpios = <&gpio 7 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		wan_green {
> +			label = "dir-615-e4:green:wan";
> +			gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		wlan {
> +			label = "dir-615-e4:green:wlan";
> +			gpios = <&ath9k 1 GPIO_ACTIVE_LOW>;
> +			linux,default-trigger = "phy0tpt";
> +		};

At some point, we started to put ath9k leds into a node of their own:

	ath9k-leds {
		wlan {
			label = "dir-615-e4:green:wlan";
			gpios = <&ath9k 1 GPIO_ACTIVE_LOW>;
			linux,default-trigger = "phy0tpt";
		};
	};

> +	};
> +};
> +
> +&spi {
> +	status = "okay";
> +	num-cs = <1>;

Please add empty line after status.

> +
> +	flash at 0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <33000000>;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			uboot: partition at 0 {
> +				reg = <0x0 0x30000>;
> +				label = "u-boot";
> +				read-only;
> +			};
> +
> +			nvram: partition at 30000 {
> +				reg = <0x30000 0x10000>;
> +				label = "nvram";
> +				read-only;
> +			};

Remove the node labels for the two partitions above, as they are not used anyway (but not the label properties).

> +
> +			firmware: partition at 40000 {
> +				compatible = "denx,uimage";
> +				reg = <0x40000 0x3b0000>;
> +				label = "firmware";
> +			};
> +
> +			/*
> +			These two partitions are defined by CameoAP99 layout
> +			but not needed for vendor firmware: MAC address is
> taken
> +			from "nvram", language pack can be flashed separately.
> +
> +			mac: partition at 3b0000 {
> +				reg = <0x3b0000 0x10000>;
> +				label = "mac";
> +				read-only;
> +			};
> +
> +			lp: partition at 3c0000 {
> +				reg = <0x3c0000 0x30000>;
> +				label = "lp";
> +				read-only;
> +			};
> +			*/

Well, we still do not know whether they are present in vendor firmware. I'm still skeptical about removing them.
Nevertheless, I won't prevent you from doing that, but please remove this comment from the DTS then and put an extensive description into the commit message instead.

> +
> +			art: partition at 3f0000 {
> +				reg = <0x3f0000 0x10000>;
> +				label = "art";
> +				read-only;
> +			};
> +		};
> +	};
> +};
> +
> +&eth0 {
> +	status = "okay";
> +
> +	/* ethernet MAC is stored in nvram */

Remove those comments. You are setting up stuff in 02_network, which are relatively standard, so I think extra comments are not necessary.

> +};
> +
> +&eth1 {
> +	status = "okay";
> +
> +	/* ethernet MAC is stored in nvram */

Remove comment.

> +};
> +
> +&pcie {
> +	status = "okay";
> +
> +	ath9k: wifi at 0,0 {
> +		compatible = "pci168c,002b";
> +		reg = <0x0000 0 0 0 0>;
> +		qca,no-eeprom;
> +		/* LAN MAC is supposed to be used for wifi */

Remove comment.

Don't forget to split the 01_leds definitions again ...

Best

Adrian 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20191111/a49ac1b9/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel