[OpenWrt-Devel] [PATCH v2] Support for Edimax EW-7476RPC / EW-7478AC
Petr Štetiar
ynezz at true.cz
Mon Jun 3 08:40:11 EDT 2019
Birger Koblitz <mail at birger-koblitz.de> [2019-06-02 18:55:06]:
Hi,
it would be nice if you could read https://openwrt.org/submitting-patches one
more time.
Applying: Support for Edimax EW-7476RPC / EW-7478AC
.git/.git/rebase-apply/patch:13: trailing
whitespace.
ucidef_set_led_switch "lan" "lan" "$boardname:green:lan"
fatal: corrupt patch at line 14
Patch failed at 0001 Support for Edimax EW-7476RPC / EW-7478AC
scripts/checkpatch.pl 1108962.patch provides following hints:
ERROR: trailing whitespace
#162: FILE: target/linux/ramips/base-files/etc/board.d/01_leds:157:
+ ucidef_set_led_switch "lan" "lan" "$boardname:green:lan" $
ERROR: patch seems to be corrupt (line wrapped?)
#163: FILE: target/linux/ramips/base-files/etc/board.d/01_leds:157:
"switch0" "0x20"
ERROR: trailing whitespace
#212: FILE: target/linux/ramips/dts/EW-7476RPC.dts:24:
+ The device does not have a reset button (but there are solder pads for $
ERROR: trailing whitespace
#466: FILE: target/linux/ramips/dts/EW-7478AC.dts:24:
+ The device does not have a reset button (but there are solder pads for $
ERROR: trailing whitespace
#604: FILE: target/linux/ramips/dts/EW-7478AC.dts:161:
+ ralink,group = "i2c", "uartf", "nd_sd", $
ERROR: trailing whitespace
#725: FILE: target/linux/ramips/dts/EW-7478AC.dts:1355:
+ err = of_property_read_u32(priv->dev->of_node, $
ERROR: trailing whitespace
#729: FILE: target/linux/ramips/dts/EW-7478AC.dts:1358:
+ phy_reset = devm_gpiod_get_optional(priv->dev, "phy-reset", $
ERROR: trailing whitespace
#733: FILE: target/linux/ramips/dts/EW-7478AC.dts:1361:
+ dev_err(priv->dev, "Error acquiring reset gpio $
ERROR: trailing whitespace
#736: FILE: target/linux/ramips/dts/EW-7478AC.dts:1363:
+ dev_info(priv->dev, "Resetting PHY, reset $
ERROR: trailing whitespace
#761: FILE: target/linux/ramips/image/mt7620.mk:626:
+ edimax-header -s CSYS -m RN79 -f 0x70000 -S 0x01100000 | $
ERROR: trailing whitespace
#775: FILE: target/linux/ramips/image/mt7620.mk:639:
+ edimax-header -s CSYS -m RN70 -f 0x70000 -S 0x01100000 | $
ERROR: Missing Signed-off-by: line(s)
total: 12 errors, 0 warnings, 590 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
1108962.patch has style problems, please review.
> Subject: Re: [OpenWrt-Devel] [PATCH v2] Support for Edimax EW-7476RPC
It should contain prefix, so the subject should be `ramips: add support for ...`
> Installation
> ------------
> Update the factory image via the web-interfaces (by default:
> http://edimaxext.setup)
> Or push wpa button on power on and send firmware via tftp to 192.168.1.6
>
> The EW-7478AC is identical to the EW-7476RPC, except instead of 2 internal
> antennas it has 2 external ones.
Missing Signed-off-by.
> --- /dev/null
> +++ b/target/linux/ramips/dts/EW-7476RPC.dts
> @@ -0,0 +1,246 @@
> +/*
> + * Device Tree file for the Edimax EW-7476RPC
> + * based on Edimax BR-6478AC V2
> + *
> + * Copyright (C) 2016 Rohan Murch <rohan.murch at gmail.com>
> + * Copyright (C) 2016 Hans Ulli Kroll <ulli.kroll at googlemail.com>
> + * Copyright (C) 2017 James McKenzie <openwrt at madingley.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
Use SPDX-License-Identifier instead. It's DTS checklist hint No1.
> + The following definitions were found in the orignal GPL firmware source
> + HW_LED_WIRELESS_ABAND_="71"
> + HW_LED_WIRELESS_GBAND_="70"
> + HW_LED_WIRELES_="69"
> + HW_LED_POWER_="67"
> + HW_LED_WPS_="68"
> + HW_LED_LAN_="66"
> + HW_BUTTON_APSWITCH_BUT_1_="62"
> + HW_BUTTON_APSWITCH_BUT_2_="63"
> + HW_BUTTON_RESET_="60"
> +
> + The device does not have a reset button (but there are solder pads for a
> button), WPS and reset are swapped.
> + */
This comment should be removed, if it's important please add it to the commit
message or to the Wiki.
>
Put SPDX-License-Identifier here.
> +/dts-v1/;
> +
[...]
> + keys {
> + compatible = "gpio-keys";
> +
> + reset_wps {
> + label = "reset_wps";
> + gpios = <&gpio2 20 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_RESTART>;
> + };
In order to be consistent within the file, can you please separate the nodes
with the newline?
> + switch_high {
> + label = "switch high";
> + gpios = <&gpio2 22 GPIO_ACTIVE_LOW>;
> + linux,code = <BTN_0>;
> + linux,input-type = <EV_SW>;
> + };
> + switch_off {
> + label = "switch off";
> + gpios = <&gpio2 23 GPIO_ACTIVE_LOW>;
> + linux,code = <BTN_1>;
> + linux,input-type = <EV_SW>;
> + };
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> +
> + led_power: power {
> + label = "ew-7476rpc:green:power";
> + gpios = <&gpio2 27 GPIO_ACTIVE_LOW>;
> + };
In order to be consistent within the file, can you please separate the nodes
with the newline?
> + lan {
> + label = "ew-7476rpc:green:lan";
> + gpios = <&gpio2 26 GPIO_ACTIVE_LOW>;
> + };
> + wlan2g {
> + label = "ew-7476rpc:blue:wlan2g";
> + gpios = <&gpio2 30 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "phy1radio";
> + };
> + wlan5g {
> + label = "ew-7476rpc:blue:wlan5g";
> + gpios = <&gpio2 31 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "phy0radio";
> + };
> + wps {
> + label = "ew-7476rpc:green:wps";
> + gpios = <&gpio2 28 GPIO_ACTIVE_LOW>;
> + };
> + crossband {
> + label = "ew-7476rpc:green:crossband";
> + gpios = <&gpio2 29 GPIO_ACTIVE_LOW>;
> + };
> + };
> +};
> +
> +&gpio1 {
> + status = "okay";
> +};
> +
> +&gpio2 {
> + status = "okay";
> +};
> +
> +&spi0 {
> + status = "okay";
> +
> + flash at 0 {
> + compatible = "jedec,spi-nor";
> + reg = <0 0>;
This is wrong and dtc probably complains, it should be `reg = <0>;`
> +&pinctrl {
> + state_default: pinctrl0 {
> + gpio {
> + ralink,group = "i2c", "uartf", "nd_sd", "rgmii2";
> + ralink,function = "gpio";
> + };
> + };
> + /* the reset pin 39 is part of spi refclk */
You can remove this comment as it's clear from the definition bellow.
> + phy_reset_pins: phy-reset {
> + gpio {
> + ralink,group = "spi refclk";
> + ralink,function = "gpio";
> + };
> + };
> +};
> +
> +
> +ðernet {
> +
No need to add newline here.
> + status = "okay";
> + mtd-mac-address = <&factory 0x4>;
But here would be better.
> + pinctrl-names = "default";
> + pinctrl-0 = <&rgmii1_pins &mdio_pins &phy_reset_pins>;
Here as well.
> + mediatek,portmap = "l";
> + mediatek,mdio-mode = <1>;
And here as well.
> +++ b/target/linux/ramips/dts/EW-7478AC.dts
It seems like you've a lot of common between those two devices, so you should
create DTSI include file with a common stuff and use it instead of this
copy&pasting.
[...]
You should either read Linux kernel coding style or run your patches through
scripts/checkpatch.pl from Linux kernel tree.
> a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++
> b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -32,6 +32,9 @@
> #include <linux/bug.h>
> #include <linux/netfilter.h>
> #include <net/netfilter/nf_flow_table.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>
> #include <asm/mach-ralink/ralink_regs.h>
>
> @@ -1338,7 +1341,8 @@ static int __init fe_init(struct net_device *dev)
> struct fe_priv *priv = netdev_priv(dev);
> struct device_node *port;
> const char *mac_addr;
> - int err;
> + int err, msec =30;
msec = 30;
> @@ -1348,6 +1352,20 @@ static int __init fe_init(struct net_device *dev)
> return -ENODEV;
> }
I would create separate fe_reset_phy() and move the stuff there as fe_init is
already long enough and as a bonus you can then simply make the indentation
level lower(helps with 80 chars line lenght limit) as you can simply do:
static void fe_reset_phy(...)
{
...
if (IS_ERR_OR_NULL(phy_reset))
return;
...
}
> + err = of_property_read_u32(priv->dev->of_node, "phy-reset-duration",
> &msec);
> + if (!err && msec > 1000)
> + msec = 30;
This should be handled only if phy-reset gpio is defined, so it should be
moved to the `if (phy_reset) {` block bellow.
> + phy_reset = devm_gpiod_get_optional(priv->dev, "phy-reset",
> GPIOD_OUT_HIGH);
You define:
phy-reset-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
and now defaulting to GPIOD_OUT_HIGH ? Shouldn't you handle both reset
active low and reset active high cases?
> + } else {
> + dev_info(priv->dev, "Resetting PHY, reset duration:
> %d\n", msec);
This is not needed.
> + mdelay(msec);
msleep < 20ms can sleep for up to 20ms, you should use usleep_range for < 20ms.
> +define Device/edimax_ew-7476rpc
> + DTS := EW-7476RPC
> + DEVICE_TITLE := Edimax EW-7476RPC
> + BLOCKSIZE := 4k
> + IMAGE_SIZE := 7616k
0x790000 = 7744k
-- ynezz
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list