Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
Hello Andy, today I'm back from holiday. There is a lot of different work waiting for me, so I think it will be submitted by the end of next week. Did you try the V3 Patch? br René Am 06.10.2014 19:45, schrieb Andy Pont: Hello Rene, Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER changes in v3: -added all compatible devices from linux driver -fixed issues from review changes in v2: -added usb_ether.h to change list -added 2nd patch to enable driver for arndale board Following the additional comments that came from Marek do you know when you will be submitted a v4 of this patch? Thanks for your effort on this. Regards, Andy. -- Dipl.-Ing. René Griessl Universität Bielefeld AG Kognitronik Sensorik Exzellenzcluster Cognitive Interaction Technology (CITEC) Inspiration 1 (Zehlendorfer Damm 199) 33615 Bielefeld Telefon : +49 521-106-67362 Fax : +49 521-106-12348 eMail : rgrie...@cit-ec.uni-bielefeld.de Internet: http://www.ks.cit-ec.uni-bielefeld.de/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
Hello Rene, Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER changes in v3: -added all compatible devices from linux driver -fixed issues from review changes in v2: -added usb_ether.h to change list -added 2nd patch to enable driver for arndale board Following the additional comments that came from Marek do you know when you will be submitted a v4 of this patch? Thanks for your effort on this. Regards, Andy. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
changes in v3: -added all compatible devices from linux driver -fixed issues from review changes in v2: -added usb_ether.h to change list -added 2nd patch to enable driver for arndale board Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de --- drivers/usb/eth/Makefile| 1 + drivers/usb/eth/asix88179.c | 659 drivers/usb/eth/usb_ether.c | 7 + include/usb_ether.h | 6 + 4 files changed, 673 insertions(+) create mode 100644 drivers/usb/eth/asix88179.c diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile index e6ae9f1..c92d2b0 100644 --- a/drivers/usb/eth/Makefile +++ b/drivers/usb/eth/Makefile @@ -6,5 +6,6 @@ # new USB host ethernet layer dependencies obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o obj-$(CONFIG_USB_ETHER_ASIX) += asix.o +obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c new file mode 100644 index 000..2079056 --- /dev/null +++ b/drivers/usb/eth/asix88179.c @@ -0,0 +1,659 @@ +/* + * Copyright (c) 2014 Rene Griessl rgrie...@cit-ec.uni-bielefeld.de + * based on the U-Boot Asix driver as well as information + * from the Linux AX88179_178a driver + * + * SPDX-License-Identifier:GPL-2.0+ + */ + + +#include common.h +#include usb.h +#include net.h +#include linux/mii.h +#include usb_ether.h +#include malloc.h + + +/* ASIX AX88179 based USB 3.0 Ethernet Devices */ +#define AX88179_PHY_ID 0x03 +#define AX_EEPROM_LEN 0x100 +#define AX88179_EEPROM_MAGIC 0x17900b95 +#define AX_MCAST_FLTSIZE 8 +#define AX_MAX_MCAST 64 +#define AX_INT_PPLS_LINK (1 16) +#define AX_RXHDR_L4_TYPE_MASK 0x1c +#define AX_RXHDR_L4_TYPE_UDP 4 +#define AX_RXHDR_L4_TYPE_TCP 16 +#define AX_RXHDR_L3CSUM_ERR2 +#define AX_RXHDR_L4CSUM_ERR1 +#define AX_RXHDR_CRC_ERR (1 29) +#define AX_RXHDR_DROP_ERR (1 31) +#define AX_ACCESS_MAC 0x01 +#define AX_ACCESS_PHY 0x02 +#define AX_ACCESS_EEPROM 0x04 +#define AX_ACCESS_EFUS 0x05 +#define AX_PAUSE_WATERLVL_HIGH 0x54 +#define AX_PAUSE_WATERLVL_LOW 0x55 + +#define PHYSICAL_LINK_STATUS 0x02 + #define AX_USB_SS 0x04 + #define AX_USB_HS 0x02 + +#define GENERAL_STATUS 0x03 +/* Check AX88179 version. UA1:Bit2 = 0, UA2:Bit2 = 1 */ + #define AX_SECLD0x04 + +#define AX_SROM_ADDR 0x07 +#define AX_SROM_CMD0x0a + #define EEP_RD 0x04 + #define EEP_BUSY0x10 + +#define AX_SROM_DATA_LOW 0x08 +#define AX_SROM_DATA_HIGH 0x09 + +#define AX_RX_CTL 0x0b + #define AX_RX_CTL_DROPCRCERR0x0100 + #define AX_RX_CTL_IPE 0x0200 + #define AX_RX_CTL_START 0x0080 + #define AX_RX_CTL_AP0x0020 + #define AX_RX_CTL_AM0x0010 + #define AX_RX_CTL_AB0x0008 + #define AX_RX_CTL_AMALL 0x0002 + #define AX_RX_CTL_PRO 0x0001 + #define AX_RX_CTL_STOP 0x + +#define AX_NODE_ID 0x10 +#define AX_MULFLTARY 0x16 + +#define AX_MEDIUM_STATUS_MODE 0x22 + #define AX_MEDIUM_GIGAMODE 0x01 + #define AX_MEDIUM_FULL_DUPLEX 0x02 + #define AX_MEDIUM_EN_125MHZ 0x08 + #define AX_MEDIUM_RXFLOW_CTRLEN 0x10 + #define AX_MEDIUM_TXFLOW_CTRLEN 0x20 + #define AX_MEDIUM_RECEIVE_EN0x100 + #define AX_MEDIUM_PS0x200 + #define AX_MEDIUM_JUMBO_EN 0x8040 + +#define AX_MONITOR_MOD 0x24 + #define AX_MONITOR_MODE_RWLC0x02 + #define AX_MONITOR_MODE_RWMP0x04 + #define AX_MONITOR_MODE_PMEPOL 0x20 + #define AX_MONITOR_MODE_PMETYPE 0x40 + +#define AX_GPIO_CTRL 0x25 + #define AX_GPIO_CTRL_GPIO3EN0x80 + #define AX_GPIO_CTRL_GPIO2EN0x40 + #define AX_GPIO_CTRL_GPIO1EN0x20 + +#define AX_PHYPWR_RSTCTL 0x26 + #define AX_PHYPWR_RSTCTL_BZ 0x0010 + #define AX_PHYPWR_RSTCTL_IPRL 0x0020 + #define AX_PHYPWR_RSTCTL_AT 0x1000 + +#define AX_RX_BULKIN_QCTRL 0x2e +#define AX_CLK_SELECT 0x33 + #define AX_CLK_SELECT_BCS 0x01 +
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
On Friday, September 26, 2014 at 10:08:48 AM, Rene Griessl wrote:
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a meaningful
commit message too. Also, if the driver is pulled from Linux, please specify
from which commit in there, so future developers might re-sync the driver if
needed be and they'd know from which point the driver was derived.
Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
---
drivers/usb/eth/Makefile| 1 +
drivers/usb/eth/asix88179.c | 659
drivers/usb/eth/usb_ether.c |
7 +
include/usb_ether.h | 6 +
4 files changed, 673 insertions(+)
create mode 100644 drivers/usb/eth/asix88179.c
diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index e6ae9f1..c92d2b0 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -6,5 +6,6 @@
# new USB host ethernet layer dependencies
obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
+obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o
obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c
new file mode 100644
index 000..2079056
--- /dev/null
+++ b/drivers/usb/eth/asix88179.c
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014 Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
+ * based on the U-Boot Asix driver as well as information
+ * from the Linux AX88179_178a driver
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+
One newline too many here.
+#include common.h
+#include usb.h
+#include net.h
+#include linux/mii.h
+#include usb_ether.h
+#include malloc.h
+
+
DTTO
+/* ASIX AX88179 based USB 3.0 Ethernet Devices */
+#define AX88179_PHY_ID 0x03
+#define AX_EEPROM_LEN0x100
+#define AX88179_EEPROM_MAGIC 0x17900b95
+#define AX_MCAST_FLTSIZE 8
+#define AX_MAX_MCAST 64
+#define AX_INT_PPLS_LINK (1 16)
+#define AX_RXHDR_L4_TYPE_MASK0x1c
+#define AX_RXHDR_L4_TYPE_UDP 4
+#define AX_RXHDR_L4_TYPE_TCP 16
+#define AX_RXHDR_L3CSUM_ERR 2
+#define AX_RXHDR_L4CSUM_ERR 1
+#define AX_RXHDR_CRC_ERR (1 29)
+#define AX_RXHDR_DROP_ERR(1 31)
+#define AX_ACCESS_MAC0x01
+#define AX_ACCESS_PHY0x02
+#define AX_ACCESS_EEPROM 0x04
+#define AX_ACCESS_EFUS 0x05
+#define AX_PAUSE_WATERLVL_HIGH 0x54
+#define AX_PAUSE_WATERLVL_LOW0x55
+
+#define PHYSICAL_LINK_STATUS 0x02
+ #define AX_USB_SS 0x04
+ #define AX_USB_HS 0x02
+
+#define GENERAL_STATUS 0x03
+/* Check AX88179 version. UA1:Bit2 = 0, UA2:Bit2 = 1 */
+ #define AX_SECLD0x04
+
+#define AX_SROM_ADDR 0x07
+#define AX_SROM_CMD 0x0a
+ #define EEP_RD 0x04
+ #define EEP_BUSY0x10
If those are bits, then just use (1 n) notation.
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+ int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct here or
can you just use array ?
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, + u16 size, void *data)
+{
+ int len;
+
+ debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n,
+ cmd, value, index, size);
+
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the compiler
will spew that you wrote code before variable declaration.
+ memcpy(buf, data, size);
+
+ len = usb_control_msg(
+ dev-pusb_dev,
+ usb_sndctrlpipe(dev-pusb_dev, 0),
+ cmd,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value,
+ index,
+ buf,
+ size,
+ USB_CTRL_SET_TIMEOUT);
+
+ return len == size ? 0 : -1;
Use values from errno.h here ?
+}
+
+static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, + u16 size, void *data)
+{
+ int len;
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a meaningful
commit message too. Also, if the driver is pulled from Linux, please specify
from which commit in there, so future developers might re-sync the driver if
needed be and they'd know from which point the driver was derived.
Were exactly can I find the marker?
Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
---
drivers/usb/eth/Makefile| 1 +
drivers/usb/eth/asix88179.c | 659
drivers/usb/eth/usb_ether.c |
7 +
include/usb_ether.h | 6 +
4 files changed, 673 insertions(+)
create mode 100644 drivers/usb/eth/asix88179.c
diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index e6ae9f1..c92d2b0 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -6,5 +6,6 @@
# new USB host ethernet layer dependencies
obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
+obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o
obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c
new file mode 100644
index 000..2079056
--- /dev/null
+++ b/drivers/usb/eth/asix88179.c
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014 Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
+ * based on the U-Boot Asix driver as well as information
+ * from the Linux AX88179_178a driver
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+
One newline too many here.
+#include common.h
+#include usb.h
+#include net.h
+#include linux/mii.h
+#include usb_ether.h
+#include malloc.h
+
+
DTTO
+/* ASIX AX88179 based USB 3.0 Ethernet Devices */
+#define AX88179_PHY_ID 0x03
+#define AX_EEPROM_LEN 0x100
+#define AX88179_EEPROM_MAGIC 0x17900b95
+#define AX_MCAST_FLTSIZE 8
+#define AX_MAX_MCAST 64
+#define AX_INT_PPLS_LINK (1 16)
+#define AX_RXHDR_L4_TYPE_MASK 0x1c
+#define AX_RXHDR_L4_TYPE_UDP 4
+#define AX_RXHDR_L4_TYPE_TCP 16
+#define AX_RXHDR_L3CSUM_ERR2
+#define AX_RXHDR_L4CSUM_ERR1
+#define AX_RXHDR_CRC_ERR (1 29)
+#define AX_RXHDR_DROP_ERR (1 31)
+#define AX_ACCESS_MAC 0x01
+#define AX_ACCESS_PHY 0x02
+#define AX_ACCESS_EEPROM 0x04
+#define AX_ACCESS_EFUS 0x05
+#define AX_PAUSE_WATERLVL_HIGH 0x54
+#define AX_PAUSE_WATERLVL_LOW 0x55
+
+#define PHYSICAL_LINK_STATUS 0x02
+ #define AX_USB_SS 0x04
+ #define AX_USB_HS 0x02
+
+#define GENERAL_STATUS 0x03
+/* Check AX88179 version. UA1:Bit2 = 0, UA2:Bit2 = 1 */
+ #define AX_SECLD0x04
+
+#define AX_SROM_ADDR 0x07
+#define AX_SROM_CMD0x0a
+ #define EEP_RD 0x04
+ #define EEP_BUSY0x10
If those are bits, then just use (1 n) notation.
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+ int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct here or
can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see
line 589 of c-file)
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, + u16 size, void *data)
+{
+ int len;
+
+ debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n,
+ cmd, value, index, size);
+
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the compiler
will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one
has not declaration?
+ memcpy(buf, data, size);
+
+ len = usb_control_msg(
+ dev-pusb_dev,
+ usb_sndctrlpipe(dev-pusb_dev, 0),
+ cmd,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value,
+ index,
+ buf,
+ size,
+ USB_CTRL_SET_TIMEOUT);
+
+ return len == size ? 0 : -1;
Use values from errno.h here ?
+}
+
+static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
On Friday, September 26, 2014 at 11:35:02 AM, René Griessl wrote:
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a
meaningful commit message too. Also, if the driver is pulled from Linux,
please specify from which commit in there, so future developers might
re-sync the driver if needed be and they'd know from which point the
driver was derived.
Were exactly can I find the marker?
Well, in git if you pulled the driver from Linux. Use git log path/to/file(s)
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+ int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct
here or can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see
line 589 of c-file)
OK, I see assignment of the -flags , but I don't see where this is ever used.
Is this -flags used at all ?
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, +u16 size, void *data)
+{
+ int len;
+
+ debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
size=%d\n, +cmd, value, index, size);
+
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the
compiler will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one
has not declaration?
You have this:
int len;
printf(...); /* this comes from the debug() if debug is enabled */
char blah.; /* This comes from the expansion of ALLOC_CACHE_ALIGN_BUFFER()*/
See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
definition .
[...]
+ if (link_detected) {
+ if (timeout != 0)
+ printf(done.\n);
+ return 0;
Where does this return 0; belong to ?
it quits the init function, because a link is detected. Is it more
likely to put a goto here?
It's the alignment that is confusing. Do you exit only if (!timeout) is true or
do you exit unconditionally if (link_detected) ?
+ } else {/*reset device and try again*/
+ printf(unable to connect.\n);
+ printf(Reset Ethernet Device\n);
+ asix_basic_reset(dev);
+ timeout = 0;
+ do {
+ asix_read_cmd(dev, AX_ACCESS_PHY, 0x03,
+MII_BMSR, 2, buf);
+ link_detected = *tmp16 BMSR_LSTATUS;
+ if (!link_detected) {
+ if (timeout == 0)
+ printf(Waiting for Ethernet
connection... );
+ udelay(TIMEOUT_RESOLUTION * 1000);
mdelay()
+ timeout += TIMEOUT_RESOLUTION;
+ }
+ } while (!link_detected timeout PHY_CONNECT_TIMEOUT);
+ if (link_detected) {
+ if (timeout != 0)
+ printf(done.\n);
+ return 0;
+ } else {
+ printf(unable to connect.\n);
+ goto out_err;
+ }
The indent is crazy in here ...
I will put the link detect routine in a separate function.
That's OK, but the indent could also use some work ...
Thanks!
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
On Friday, September 26, 2014 at 11:35:02 AM, René Griessl wrote:
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a
meaningful commit message too. Also, if the driver is pulled from Linux,
please specify from which commit in there, so future developers might
re-sync the driver if needed be and they'd know from which point the
driver was derived.
Were exactly can I find the marker?
Well, in git if you pulled the driver from Linux. Use git log path/to/file(s)
With git log I can see the commit messages, right?
Does that mean, that I have omit the change-log in the commit message,
or only write the
last changes in there?
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+ int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct
here or can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see
line 589 of c-file)
OK, I see assignment of the -flags , but I don't see where this is ever used.
Is this -flags used at all ?
Well thats correct. In the asix.c driver the flags are used to handle
small differences between
the devices. This is left inside for future work, if it becomes
necessary to handle things different.
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, + u16 size, void *data)
+{
+ int len;
+
+ debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
size=%d\n, +cmd, value, index, size);
+
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the
compiler will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one
has not declaration?
You have this:
int len;
printf(...); /* this comes from the debug() if debug is enabled */
char blah.; /* This comes from the expansion of ALLOC_CACHE_ALIGN_BUFFER()*/
See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
definition .
OK, now I see. Then I have a variable definition after the printf.
[...]
+ if (link_detected) {
+ if (timeout != 0)
+ printf(done.\n);
+ return 0;
Where does this return 0; belong to ?
it quits the init function, because a link is detected. Is it more
likely to put a goto here?
It's the alignment that is confusing. Do you exit only if (!timeout) is true or
do you exit unconditionally if (link_detected) ?
I changed the name of the variable to time_waited and the check is now
(time_waited 0)
so done is only printed when you really had to wait for the connection.
If the connection
was already established the messages will not be printed.
And the return has one tab less now.
[...]
--
Dipl.-Ing. René Griessl
Universität Bielefeld
AG Kognitronik Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld
Telefon : +49 521-106-67362
Fax : +49 521-106-12348
eMail : rgrie...@cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
On Friday, September 26, 2014 at 05:42:10 PM, René Griessl wrote:
On Friday, September 26, 2014 at 11:35:02 AM, René Griessl wrote:
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a
meaningful commit message too. Also, if the driver is pulled from
Linux, please specify from which commit in there, so future developers
might re-sync the driver if needed be and they'd know from which point
the driver was derived.
Were exactly can I find the marker?
Well, in git if you pulled the driver from Linux. Use git log
path/to/file(s)
With git log I can see the commit messages, right?
Does that mean, that I have omit the change-log in the commit message,
or only write the
last changes in there?
The important part is the Commit ID there.
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct
here or can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see
line 589 of c-file)
OK, I see assignment of the -flags , but I don't see where this is ever
used. Is this -flags used at all ?
Well thats correct. In the asix.c driver the flags are used to handle
small differences between
the devices. This is left inside for future work, if it becomes
necessary to handle things different.
Zap them, it's dead code.
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value,
u16 index, + u16 size, void *data)
+{
+int len;
+
+debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
size=%d\n, + cmd, value, index, size);
+
+ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the
compiler will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one
has not declaration?
You have this:
int len;
printf(...); /* this comes from the debug() if debug is enabled */
char blah.; /* This comes from the expansion of
ALLOC_CACHE_ALIGN_BUFFER()*/
See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
definition .
OK, now I see. Then I have a variable definition after the printf.
Yes.
[...]
+if (link_detected) {
+if (timeout != 0)
+printf(done.\n);
+return 0;
Where does this return 0; belong to ?
it quits the init function, because a link is detected. Is it more
likely to put a goto here?
It's the alignment that is confusing. Do you exit only if (!timeout) is
true or do you exit unconditionally if (link_detected) ?
I changed the name of the variable to time_waited and the check is now
(time_waited 0)
Timeout was OK, there is no need to make the code diverge more.
so done is only printed when you really had to wait for the connection.
If the connection
was already established the messages will not be printed.
And the return has one tab less now.
OK, so the return happens unconditionally. That's what I wanted to know.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
