Birger Koblitz <[email protected]> [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 <[email protected]> > + * Copyright (C) 2016 Hans Ulli Kroll <[email protected]> > + * Copyright (C) 2017 James McKenzie <[email protected]> > + * > + * 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@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 [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
