Hello Tobias,
I saw on patchwork that your patch is already marked as accepted, but
I have a lot of comments, please see them inline, below.
PS. Sorry for being so pedantic, but it took me lot of time to clean
ramips target few weeks ago.
2015-08-17 22:47 GMT+02:00 Tobias Welz <t...@wiznet.eu>:
> From: Tobias Welz <t...@wiznet.eu>
Your patch message should have "ramips:" prefix.
It would be also nice to place a small description about the hardware
you are adding support for and this is the right place for it.
>
>
> Signed-off-by: Tobias Welz <t...@wiznet.eu>
> ---
> .../linux/ramips/base-files/etc/board.d/02_network | 5 +
> target/linux/ramips/base-files/etc/diag.sh | 3 +
> target/linux/ramips/base-files/lib/ramips.sh | 3 +
> .../ramips/base-files/lib/upgrade/platform.sh | 1 +
> target/linux/ramips/dts/WIZFI630A.dts | 294
> ++++++++++++++++++++
> target/linux/ramips/image/Makefile | 4 +-
> target/linux/ramips/rt305x/profiles/wizfi630a.mk | 18 ++
> 7 files changed, 327 insertions(+), 1 deletion(-)
> create mode 100644 target/linux/ramips/dts/WIZFI630A.dts
> create mode 100644 target/linux/ramips/rt305x/profiles/wizfi630a.mk
>
> diff --git a/target/linux/ramips/base-files/etc/board.d/02_network
> b/target/linux/ramips/base-files/etc/board.d/02_network
> index 6a2f042..cfd0b26 100755
> --- a/target/linux/ramips/base-files/etc/board.d/02_network
> +++ b/target/linux/ramips/base-files/etc/board.d/02_network
> @@ -217,6 +217,7 @@ ramips_setup_interfaces()
> ucidef_add_switch_vlan "switch0" "1" "0 1 2 3 6t"
> ucidef_add_switch_vlan "switch0" "2" "5 6t"
> ;;
> + wizfi630a | \
Please, follow the general convention in this file and use "|\"
instead of " | \".
> y1s)
> ucidef_set_interfaces_lan_wan "eth0.1" "eth0.2"
> ucidef_add_switch "switch0" "1" "1"
> @@ -320,6 +321,10 @@ ramips_setup_macs()
> wcr-150gn)
> wan_mac=$(mtd_get_mac_binary factory 40)
> ;;
> + wizfi630a)
> + lan_mac=$(mtd_get_mac_binary factory 4)
> + wan_mac=$(mtd_get_mac_binary factory 40)
> + ;;
Please, follow the general convention and keep boards in alphabetical
order (board are sorted in two passes: in file and in common blocks):
[...]
wcr-150gn)
[...]
whr-1166d|\
whr-300hp2|\
whr-600d|\
wsr-600)
[...]
--> this is the right place for your board
wsr-1166)
[...]
> whr-1166d|\
> whr-300hp2|\
> whr-600d|\
> diff --git a/target/linux/ramips/base-files/etc/diag.sh
> b/target/linux/ramips/base-files/etc/diag.sh
> index a4911b0..54b84e5 100644
> --- a/target/linux/ramips/base-files/etc/diag.sh
> +++ b/target/linux/ramips/base-files/etc/diag.sh
> @@ -143,6 +143,9 @@ get_status_led() {
> whr-600d)
> status_led="$board:orange:wifi"
> ;;
> + wizfi630a)
> + status_led="wizfi630a::run"
> + ;;
Same as above, please keep boards in alphabetical order:
[...]
wcr-150gn|\
wl-351)
[...]
whr-g300n|\
wzr-agl300nh)
[...]
--> this is the right place for your board
wsr-1166|\
wsr-600)
> rt-n10-plus|\
> tew-691gr|\
> tew-692gr|\
> diff --git a/target/linux/ramips/base-files/lib/ramips.sh
> b/target/linux/ramips/base-files/lib/ramips.sh
> index 5cafd45..b6d1023 100755
> --- a/target/linux/ramips/base-files/lib/ramips.sh
> +++ b/target/linux/ramips/base-files/lib/ramips.sh
> @@ -367,6 +367,9 @@ ramips_board_detect() {
> *"WIZARD 8800")
> name="wizard8800"
> ;;
> + *"WIZnet WizFi630A")
> + name="wizfi630a"
> + ;;
I don't see any reason here to use whole name of the device, including
manufacturer name (WIZnet WizFi630A) - there are no other similar
models on the list and as you can see we use here *"..." pattern.
Please, follow the general convention in this file and use only
board/model (WizFi630A) name and keep boards in alphabetical order.
> *"WL-330N")
> name="wl-330n"
> ;;
> diff --git a/target/linux/ramips/base-files/lib/upgrade/platform.sh
> b/target/linux/ramips/base-files/lib/upgrade/platform.sh
> index 53e7070..947a328 100755
> --- a/target/linux/ramips/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ramips/base-files/lib/upgrade/platform.sh
> @@ -107,6 +107,7 @@ platform_check_image() {
> whr-300hp2|\
> whr-600d|\
> whr-g300n|\
> + wizfi630a |\
Please, follow the general convention in this file and use "|\"
instead of " |\".
> wl-330n|\
> wl-330n3g|\
> wl-341v3|\
> diff --git a/target/linux/ramips/dts/WIZFI630A.dts
> b/target/linux/ramips/dts/WIZFI630A.dts
> new file mode 100644
> index 0000000..0d44c71
> --- /dev/null
> +++ b/target/linux/ramips/dts/WIZFI630A.dts
> @@ -0,0 +1,294 @@
>
> +/dts-v1/;
> +
> +/include/ "rt5350.dtsi"
> +
> +/ {
> + compatible = "wizfi630a", "ralink,rt5350-soc";
> + model = "WIZnet WizFi630A";
> +
> + chosen {
> + bootargs = "console=ttyS1,115200";
> + };
> +
> + palmbus@10000000 {
> + gpio1: gpio@660 {
> + status = "okay";
> + };
> +
> + spi@b00 {
> + status = "okay";
> +
> + m25p80@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "w25q128";
> + reg = <0>;
> + linux,modalias = "m25p80", "w25q128";
> + spi-max-frequency = <10000000>;
> +
> + partition@0 {
> + label = "uboot";
> + reg = <0x0 0x30000>;
> + read-only;
> + };
> +
> + partition@30000 {
> + label = "uboot-env";
> + reg = <0x30000 0x10000>;
> + read-only;
> + };
> +
> + factory: partition@40000 {
> + label = "factory";
> + reg = <0x40000 0x10000>;
> + read-only;
> + };
> +
> + partition@50000 {
> + label = "firmware";
> + reg = <0x50000 0xfb0000>;
> + };
> + };
> + };
> +
> + uart@500 {
> + compatible = "ralink,mt7620a-uart",
> "ralink,rt2880-uart", "ns16550a";
> + reg = <0x500 0x100>;
> +
> + resets = <&rstctrl 12>;
> + reset-names = "uart";
> +
> + interrupt-parent = <&intc>;
> + interrupts = <5>;
> +
> + reg-shift = <2>;
> +
> + status = "okay";
Are these empty lines really needed here?
> + };
> +
> +
Keep only one empty line here, please.
> + uartlite@c00 {
> + compatible = "ralink,mt7620a-uart",
> "ralink,rt2880-uart", "ns16550a";
> + reg = <0xc00 0x100>;
> +
> + resets = <&rstctrl 19>;
> + reset-names = "uartl";
> +
> + interrupt-parent = <&intc>;
> + interrupts = <12>;
> +
> + reg-shift = <2>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uartlite_pins>;
Are these empty lines really needed here too?
> + };
> + };
> +
> + pinctrl {
> + state_default: pinctrl0 {
> + gpio {
> + ralink,group = "i2c", "jtag" ;
> + ralink,function = "gpio";
> + };
> + };
> +
> + uartf_gpio_pins: uartf_gpio {
> + uartf_gpio {
> + ralink,group = "uartf";
> + ralink,function = "uartf";
>
> + };
> + };
> +
> + uartlite_pins: uartlite {
> + uart {
> + ralink,group = "uartlite";
> + ralink,function = "uartlite";
> + };
> + };
> + };
> +
> + ethernet@10100000 {
> + mtd-mac-address = <&factory 0x4>;
> + };
> +
> + esw@10110000 {
> + ralink,portmap = <0x17>;
> + };
> +
> + wmac@10180000 {
> + ralink,mtd-eeprom = <&factory 0>;
> + };
> +
> + ehci@101c0000 {
> + status = "okay";
> + };
> +
> + ohci@101c1000 {
> + status = "okay";
> + };
> +
> + gpio-export {
>
> + compatible = "gpio-export";
> + #size-cells = <0>;
Empty line here, please.
> +/*
> + gpio22 {
> + // WAN + gpio-export,name =
> "gpio22";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio1 0 0>; // path lfd dir
> + };
Empty line here, please.
> + gpio23 {
> + // LAN1 + gpio-export,name =
> "gpio23";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio1 1 0>;
> + };
Empty line here, please.
> + gpio24 {
> + // LAN2 + gpio-export,name =
> "gpio24";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio1 2 0>;
> + };
> +
> + // UARTF - UART1 + gpio7 {
This line looks broken (missing new line after comment line).
> + // UARTF_RTS_N - HW Pin 6
> + gpio-export,name = "gpio7";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 7 0>;
> + };
Empty line here, please.
> + gpio8 {
> + // UARTF_TXD - HW Pin 10 +
> gpio-export,name = "gpio8";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 8 0>;
> + };
Empty line here, please.
> + gpio9 {
> + // UARTF_CTS_N - HW Pin 5
> + gpio-export,name = "gpio9";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 9 0>;
> + };
Empty line here, please.
> + gpio10 {
> + // UARTF_RXD - HW Pin 9
> + gpio-export,name = "gpio10";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 10 0>;
> + };
Empty line here, please.
> + gpio11 {
> + // UARTF_DTR_N - HW Pin 8
> + gpio-export,name = "gpio11";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 11 0>;
> + };
Empty line here, please.
> + gpio12 {
> + // UARTF_DCD_N - HW Pin 12 +
> gpio-export,name = "gpio12";
This line looks broken (missing new line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 12 0>;
> + };
Empty line here, please.
> + gpio13 {
> + // UARTF_DSR_N - HW Pin 11
> + gpio-export,name = "gpio13";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 13 0>;
> + };
Empty line here, please.
> + gpio14 {
> + // UARTF_RIN - HW Pin 7 +
> gpio-export,name = "gpio14";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 14 0>;
> + };
> +
> + // UARTLITE - UART2 + gpio15 {
This line looks broken (missing new line after comment line).
> + // UART2_TXD - HW Pin 20 +
> gpio-export,name = "gpio15";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 15 0>;
> + };
Empty line here, please.
> + gpio16 {
> + // UART2_RXD - HW Pin 18 +
> gpio-export,name = "gpio16";
This line looks broken (missing new line after comment line).
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 16 0>;
> + };
> +
Move this empty line...
> +*/
... here, please.
> +/*
> + // If you want to use this comment out the corrosponding
> gpio-keys-polled
> + gpio0 {
> + // wps btn
> + gpio-export,name = "gpio0";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 0 1>;
> + };
> +*/
> +
> + gpio19 {
> + // SCM1
> + gpio-export,name = "gpio19";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 19 1>;
> + };
Empty line here, please.
> + gpio2 {
> + // SCM2
> + gpio-export,name = "gpio2";
> + gpio-export,direction_may_change = <1>;
> + gpios = <&gpio0 2 1>;
> + };
> +
This empty line is unnecessary here.
> + };
> +
> + gpio-leds {
> + compatible = "gpio-leds";
Empty line here, please.
> + run {
> + label = "wizfi630a::run";
> + gpios = <&gpio0 1 1>;
> + };
Empty line here, please.
> + uart1 {
> + label = "wizfi630a::uart1";
> + gpios = <&gpio0 18 1>;
> + };
Empty line here, please.
> + uart2 {
> + label = "wizfi630a::uart2";
> + gpios = <&gpio0 21 1>;
> + };
Empty line here, please.
> + wps {
> + label = "wizfi630a::wps";
> + gpios = <&gpio0 20 1>;
> + };
> + };
> +
> + gpio-keys-polled {
>
> + compatible = "gpio-keys-polled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + poll-interval = <20>;
Empty line here, please.
> + reset {
> + label = "reset";
> + gpios = <&gpio0 17 1>;
> + linux,code = <0x198>;
> + };
> +
> + wps {
> + label = "wps";
> + gpios = <&gpio0 0 1>;
> + linux,code = <0x211>;
> + };
Empty line here, please.
> +/*
> + // If you want to use this comment out the corrosponding
> export
> + scm1 {
> + label = "SCM1";
> + gpios = <&gpio0 19 1>;
> + linux,code = <0x100>;
> + };
> +
> + scm2 {
> + label = "SCM2";
> + gpios = <&gpio0 2 1>;
> + linux,code = <0x101>;
> + };
> +*/
> + };
> +};
> diff --git a/target/linux/ramips/image/Makefile
> b/target/linux/ramips/image/Makefile
> index 2f4d4cb..ac2e83c 100644
> --- a/target/linux/ramips/image/Makefile
> +++ b/target/linux/ramips/image/Makefile
> @@ -608,7 +608,6 @@ Image/Build/Profile/W502U=$(call
> BuildFirmware/Default8M/$(1),$(1),w502u,W502U)
> Image/Build/Profile/WCR150GN=$(call
> BuildFirmware/Default4M/$(1),$(1),wcr150gn,WCR150GN)
> -
> Image/Build/Profile/MZK-DP150N=$(call
> BuildFirmware/Default4M/$(1),$(1),mzk-dp150n,MZK-DP150N)
> buffalo_whrg300n_mtd_size=3801088
> @@ -631,6 +630,8 @@ Image/Build/Profile/WHRG300N=$(call
> BuildFirmware/WHRG300N/$(1),$(1))
> Image/Build/Profile/WIZARD8800=$(call
> BuildFirmware/Default8M/$(1),$(1),wizard-8800,WIZARD8800,Linux Kernel Image)
> +Image/Build/Profile/WIZFI630A=$(call
> BuildFirmware/Default16M/$(1),$(1),wizfi630a,WIZFI630A)
> +
> Image/Build/Profile/WL-330N=$(call
> BuildFirmware/Default4M/$(1),$(1),wl-330n,WL-330N)
> Image/Build/Profile/WL-330N3G=$(call
> BuildFirmware/Default4M/$(1),$(1),wl-330n3g,WL-330N3G)
> @@ -763,6 +764,7 @@ define Image/Build/Profile/Default
> $(call Image/Build/Profile/WCR150GN,$(1))
> $(call Image/Build/Profile/WHRG300N,$(1))
> $(call Image/Build/Profile/WIZARD8800,$(1))
> + $(call Image/Build/Profile/WIZFI630A,$(1))
> $(call Image/Build/Profile/WL-330N,$(1))
> $(call Image/Build/Profile/WL-330N3G,$(1))
> $(call Image/Build/Profile/WL-341V3,$(1))
> diff --git a/target/linux/ramips/rt305x/profiles/wizfi630a.mk
> b/target/linux/ramips/rt305x/profiles/wizfi630a.mk
This filename is wrong, please follow the general convention and:
- keep all boards from same manufacturer in file "manufacturer.mk" (in
this case: wiznet.mk)
- keep all profiles inside this file in alphabetical order
- follow the general structure, see below
> new file mode 100644
> index 0000000..2d2c001
> --- /dev/null
> +++ b/target/linux/ramips/rt305x/profiles/wizfi630a.mk
> @@ -0,0 +1,18 @@
> +#
> +# Copyright (C) 2014 OpenWrt.org
Shouldn't it be 2015? :)
> +#
> +# This is free software, licensed under the GNU General Public License v2.
> +# See /LICENSE for more information.
> +#
> +
> +define Profile/WIZFI630A
> + NAME:=WIZnet WizFi630A
> + PACKAGES:=\
> + kmod-usb2 +endef
>
> +
> +define Profile/WIZFI630A/Description
> + Package set for WIZnet WizFi630A board
> +endef
> +
This empty line is unnecessary here.
Please, see other *.mk profile files structure.
> +$(eval $(call Profile,WIZFI630A))
> --
> 1.7.10.4
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
