Skip to contentSkip to conversation

usb: eth: fix Makefile

PATCH Superseded Rene Griessl 3 participants 11 messages Sep 26, 2014 – Oct 27, 2014 USB

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Rene Griessl

author
#0 lore raw

PATCH v3 1/3 usb: eth: fix Makefile

fix issue as discussed with Marek Vasut

Signed-off-by Rene Griessl <[email protected]>
---
 drivers/usb/eth/Makefile | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/eth/Makefile +1 -3
index 94551c4..e6ae9f1 100644
@@ -5,8 +5,6 @@
 
 # new USB host ethernet layer dependencies
 obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
-ifdef CONFIG_USB_ETHER_ASIX
-obj-y += asix.o
-endif
+obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
 obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
 obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
Signature · 1 line
1.9.1
author
#1 lore raw

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 <[email protected]>
---
 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
drivers/usb/eth/Makefile +1 -0
index e6ae9f1..c92d2b0 100644
@@ -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
drivers/usb/eth/asix88179.c new file +659 -0

659 lines not on this page · raw message

drivers/usb/eth/usb_ether.c +7 -0
index 1dda54c..7cb96e3 100644
@@ -30,6 +30,13 @@ static const struct usb_eth_prob_dev prob_dev[] = {
 		.get_info = asix_eth_get_info,
 	},
 #endif
+#ifdef CONFIG_USB_ETHER_ASIX88179
+	{
+		.before_probe = ax88179_eth_before_probe,
+		.probe = ax88179_eth_probe,
+		.get_info = ax88179_eth_get_info,
+	},
+#endif
 #ifdef CONFIG_USB_ETHER_MCS7830
 	{
 		.before_probe = mcs7830_eth_before_probe,
include/usb_ether.h +6 -0
index 35700a2..b38d037 100644
@@ -49,6 +49,12 @@ int asix_eth_probe(struct usb_device *dev, unsigned int ifnum,
 int asix_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
 		      struct eth_device *eth);
 
+void ax88179_eth_before_probe(void);
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss);
+int ax88179_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+		      struct eth_device *eth);
+
 void mcs7830_eth_before_probe(void);
 int mcs7830_eth_probe(struct usb_device *dev, unsigned int ifnum,
 		      struct ueth_data *ss);
Signature · 1 line
1.9.1
#2 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

Quoted 7 lines · Rene Griessl

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.

Quoted 27 lines
Signed-off-by Rene Griessl <[email protected]>
---
 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
drivers/usb/eth/Makefile +1 -0
index e6ae9f1..c92d2b0 100644
@@ -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
drivers/usb/eth/asix88179.c new file +0 -0
new file mode 100644
index 0000000..2079056

drivers/usb/eth/asix88179.c line 9

Quoted 9 lines
drivers/usb/eth/asix88179.c new file +9 -0
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014 Rene Griessl <[email protected]>
+ * 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.

drivers/usb/eth/asix88179.c line 17

Quoted 8 lines
drivers/usb/eth/asix88179.c new file +8 -0
+#include <common.h>
+#include <usb.h>
+#include <net.h>
+#include <linux/mii.h>
+#include "usb_ether.h"
+#include <malloc.h>
+
+

DTTO

Quoted 23 lines
drivers/usb/eth/asix88179.c new file +23 -0
+/* 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_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_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

drivers/usb/eth/asix88179.c line 50

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	#define	AX_USB_HS		0x02
+
+#define GENERAL_STATUS				0x03
+/* Check AX88179 version. UA1:Bit2 = 0,  UA2:Bit2 = 1 */
+	#define	AX_SECLD		0x04
+
+#define AX_SROM_ADDR				0x07
+#define AX_SROM_CMD				0x0a
+	#define EEP_RD			0x04
+	#define EEP_BUSY		0x10

If those are bits, then just use (1 << n) notation. [...]

drivers/usb/eth/asix88179.c line 207

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+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 ?

drivers/usb/eth/asix88179.c line 220

Quoted 12 lines
drivers/usb/eth/asix88179.c new file +4 -0
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)

drivers/usb/eth/asix88179.c new file +7 -0
+{
+	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.

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	memcpy(buf, data, size);
+
+	len = usb_control_msg(
+		dev->pusb_dev,

drivers/usb/eth/asix88179.c line 235

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+		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 ?

Quoted 27 lines
drivers/usb/eth/asix88179.c new file +3 -0
+}
+
+static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)

drivers/usb/eth/asix88179.c new file +23 -0
+{
+	int len;
+
+	debug("asix_read_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);
+
+
+	len = usb_control_msg(
+		dev->pusb_dev,
+		usb_rcvctrlpipe(dev->pusb_dev, 0),
+		cmd,
+		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		value,
+		index,
+		buf,
+		size,
+		USB_CTRL_GET_TIMEOUT);
+
+	memcpy(data, buf, size);
+
+	return len == size ? 0 : -1;
+}

drivers/usb/eth/asix88179.c

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+
+
+static int asix_read_mac(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
+
+	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
+		asix_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, 6, 6, buf);
+		debug("asix_read_mac() returning %02x:%02x:%02x:%02x:%02x:

%02x\n",

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+		memcpy(eth->enetaddr, buf, ETH_ALEN);
+	}
+	return 0;
+}
+
+
+
+static int asix_basic_reset(struct ueth_data *dev)
+{

drivers/usb/eth/asix88179.c line 292

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+	u8 *tmp;
+
+	tmp16 = (u16 *)buf;
+	tmp = (u8 *)buf;
+
+	/* Power up ethernet PHY */
+	*tmp16 = 0;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);

The asix_write_cmd() has some bounce-buffer logic in it already, does the tmp16 need to be aligned here too? Also, you might want to use include/bouncebuf.h and friends instead of implementing your own bounce buffering.

[...]

Quoted 20 lines
drivers/usb/eth/asix88179.c new file +20 -0
+/*
+ * Asix callbacks
+ */
+static int asix_init(struct eth_device *eth, bd_t *bd)
+{
+	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
+	int timeout = 0;
+	int link_detected;
+
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+
+	tmp16 = (u16 *)buf;
+
+	debug("** %s()\n", __func__);
+
+
+	/* Configure RX control register => start operation */
+	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
+		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;

drivers/usb/eth/asix88179.c line 377

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	if (asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16) < 0)
+		goto out_err;
+
+	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()

drivers/usb/eth/asix88179.c line 380

Quoted 3 lines
drivers/usb/eth/asix88179.c new file +3 -0
+			timeout += TIMEOUT_RESOLUTION;
+		}
+	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);

Newline

drivers/usb/eth/asix88179.c line 384

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	if (link_detected) {
+		if (timeout != 0)
+			printf("done.\n");
+			return 0;

Where does this return 0; belong to ?

drivers/usb/eth/asix88179.c

Quoted 12 lines
drivers/usb/eth/asix88179.c new file +12 -0
+	} 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... ");

drivers/usb/eth/asix88179.c

Quoted 1 line
drivers/usb/eth/asix88179.c new file +1 -0
+				udelay(TIMEOUT_RESOLUTION * 1000);

mdelay()

drivers/usb/eth/asix88179.c line 408

Quoted 11 lines
drivers/usb/eth/asix88179.c new file +11 -0
+				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 ...

Quoted 15 lines
drivers/usb/eth/asix88179.c new file +15 -0
+	}
+
+	return 0;
+out_err:
+	return -1;
+}
+
+static int asix_send(struct eth_device *eth, void *packet, int length)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	int err;
+	u32 packet_len, tx_hdr2;
+	int actual_len;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
+				 PKTSIZE + (2 * sizeof(packet_len)));

drivers/usb/eth/asix88179.c line 433

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+
+	debug("** %s(), len %d\n", __func__, length);
+
+	packet_len = length;
+	cpu_to_le32s(&packet_len);
+
+	memcpy(msg, &packet_len, sizeof(packet_len));
+
+	tx_hdr2 = 0;
+	if (((length + 8) % 0x200 /*frame_size*/) == 0)

Define the frame size as a named constant, then use it here.

Quoted 20 lines
drivers/usb/eth/asix88179.c new file +20 -0
+		tx_hdr2 |= 0x80008000;	/* Enable padding */
+
+	cpu_to_le32s(&tx_hdr2);
+
+	memcpy(msg + sizeof(packet_len), &tx_hdr2, sizeof(tx_hdr2));
+
+	memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
+	       (void *)packet, length);
+
+	err = usb_bulk_msg(dev->pusb_dev,
+				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
+				(void *)msg,
+				length + sizeof(packet_len) + sizeof(tx_hdr2),
+				&actual_len,
+				USB_BULK_SEND_TIMEOUT);
+	debug("Tx: len = %u, actual = %u, err = %d\n",
+	      length + sizeof(packet_len), actual_len, err);
+
+	return err;
+}

[...]

Quoted 40 lines
drivers/usb/eth/asix88179.c new file +40 -0
+/* Probe to see if a new device is actually an asix device */
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *iface_desc;
+	int ep_in_found = 0, ep_out_found = 0;
+	int i;
+
+	/* let's examine the device now */
+	iface = &dev->config.if_desc[ifnum];
+	iface_desc = &dev->config.if_desc[ifnum].desc;
+
+	for (i = 0; asix_dongles[i].vendor != 0; i++) {
+		if (dev->descriptor.idVendor == asix_dongles[i].vendor &&
+		    dev->descriptor.idProduct == asix_dongles[i].product)
+			/* Found a supported dongle */
+			break;
+	}
+
+	if (asix_dongles[i].vendor == 0)
+		return 0;
+
+	memset(ss, 0, sizeof(struct ueth_data));
+
+	/* At this point, we know we've got a live one */
+	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
+	      dev->descriptor.idVendor, dev->descriptor.idProduct);
+
+	/* Initialize the ueth_data structure with some useful info */
+	ss->ifnum = ifnum;
+	ss->pusb_dev = dev;
+	ss->subclass = iface_desc->bInterfaceSubClass;
+	ss->protocol = iface_desc->bInterfaceProtocol;
+
+	/* alloc driver private */
+	ss->dev_priv = calloc(1, sizeof(struct asix_private));
+	if (!ss->dev_priv)
+		return 0;
+

drivers/usb/eth/asix88179.c line 598

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	((struct asix_private *)ss->dev_priv)->flags = asix_dongles[i].flags;
+
+	/*
+	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
+	 * int. We will ignore any others.
+	 */
+	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		/* is it an BULK endpoint? */
+		if ((iface->ep_desc[i].bmAttributes &
+		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {

if (! ...)
continue;

if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
something
}

if (!(ep_addr & USB_DIR_IN) && !ep_out_found) {
something
}

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
+			if (ep_addr & USB_DIR_IN) {
+				if (!ep_in_found) {
+					ss->ep_in = ep_addr &

drivers/usb/eth/asix88179.c line 612

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_in_found = 1;
+				}
+			} else {
+				if (!ep_out_found) {
+					ss->ep_out = ep_addr &
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_out_found = 1;
+				}
+			}

See above how to trim down the indent here.
[...]

author
#3 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

Quoted 5 lines
Quoted 5 lines

changes in v3:
-added all compatible devices from linux driver -fixed issues from review

Quoted 7 lines
Quoted 3 lines

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?

Quoted 84 lines
Quoted 27 lines
Signed-off-by Rene Griessl <[email protected]>
---
  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

drivers/usb/eth/Makefile +1 -0
index e6ae9f1..c92d2b0 100644
@@ -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
drivers/usb/eth/asix88179.c new file +0 -0
new file mode 100644
index 0000000..2079056

drivers/usb/eth/asix88179.c line 9

Quoted 9 lines
drivers/usb/eth/asix88179.c new file +9 -0
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014 Rene Griessl <[email protected]>
+ * 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.

drivers/usb/eth/asix88179.c line 17

Quoted 8 lines
drivers/usb/eth/asix88179.c new file +8 -0
+#include <common.h>
+#include <usb.h>
+#include <net.h>
+#include <linux/mii.h>
+#include "usb_ether.h"
+#include <malloc.h>
+
+

DTTO

Quoted 23 lines
drivers/usb/eth/asix88179.c new file +23 -0
+/* 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_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_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

drivers/usb/eth/asix88179.c line 50

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	#define	AX_USB_HS		0x02
+
+#define GENERAL_STATUS				0x03
+/* Check AX88179 version. UA1:Bit2 = 0,  UA2:Bit2 = 1 */
+	#define	AX_SECLD		0x04
+
+#define AX_SROM_ADDR				0x07
+#define AX_SROM_CMD				0x0a
+	#define EEP_RD			0x04
+	#define EEP_BUSY		0x10

If those are bits, then just use (1 << n) notation. [...]

Quoted 8 lines

drivers/usb/eth/asix88179.c line 207

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+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)

Quoted 6 lines
Quoted 6 lines
drivers/usb/eth/asix88179.c new file +4 -0
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)
+{

Quoted 8 lines

drivers/usb/eth/asix88179.c line 220

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+	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?

Quoted 69 lines
Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	memcpy(buf, data, size);
+
+	len = usb_control_msg(
+		dev->pusb_dev,

drivers/usb/eth/asix88179.c line 235

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+		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 ?

Quoted 27 lines
drivers/usb/eth/asix88179.c new file +3 -0
+}
+
+static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)

drivers/usb/eth/asix88179.c new file +23 -0
+{
+	int len;
+
+	debug("asix_read_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);
+
+
+	len = usb_control_msg(
+		dev->pusb_dev,
+		usb_rcvctrlpipe(dev->pusb_dev, 0),
+		cmd,
+		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		value,
+		index,
+		buf,
+		size,
+		USB_CTRL_GET_TIMEOUT);
+
+	memcpy(data, buf, size);
+
+	return len == size ? 0 : -1;
+}

drivers/usb/eth/asix88179.c

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+
+
+static int asix_read_mac(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
+
+	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
+		asix_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, 6, 6, buf);
+		debug("asix_read_mac() returning %02x:%02x:%02x:%02x:%02x:

%02x\n",

Quoted 15 lines
drivers/usb/eth/asix88179.c new file +15 -0
+		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+		memcpy(eth->enetaddr, buf, ETH_ALEN);
+	}
+	return 0;
+}
+
+
+
+static int asix_basic_reset(struct ueth_data *dev)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+	u8 *tmp;
+
+	tmp16 = (u16 *)buf;
Quoted 8 lines

drivers/usb/eth/asix88179.c line 292

Quoted 5 lines
drivers/usb/eth/asix88179.c new file +5 -0
+	tmp = (u8 *)buf;
+
+	/* Power up ethernet PHY */
+	*tmp16 = 0;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);

The asix_write_cmd() has some bounce-buffer logic in it already, does the tmp16 need to be aligned here too? Also, you might want to use include/bouncebuf.h and friends instead of implementing your own bounce buffering.

I will do some rework here...

Quoted 39 lines

[...]

Quoted 20 lines
drivers/usb/eth/asix88179.c new file +20 -0
+/*
+ * Asix callbacks
+ */
+static int asix_init(struct eth_device *eth, bd_t *bd)
+{
+	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
+	int timeout = 0;
+	int link_detected;
+
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+
+	tmp16 = (u16 *)buf;
+
+	debug("** %s()\n", __func__);
+
+
+	/* Configure RX control register => start operation */
+	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
+		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;

drivers/usb/eth/asix88179.c line 377

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	if (asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16) < 0)
+		goto out_err;
+
+	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()

drivers/usb/eth/asix88179.c line 380

Quoted 3 lines
drivers/usb/eth/asix88179.c new file +3 -0
+			timeout += TIMEOUT_RESOLUTION;
+		}
+	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);

Newline

Quoted 5 lines

drivers/usb/eth/asix88179.c line 384

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	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?

Quoted 16 lines

drivers/usb/eth/asix88179.c

Quoted 12 lines
drivers/usb/eth/asix88179.c new file +12 -0
+	} 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... ");

drivers/usb/eth/asix88179.c

Quoted 1 line
drivers/usb/eth/asix88179.c new file +1 -0
+				udelay(TIMEOUT_RESOLUTION * 1000);

mdelay()

Quoted 12 lines

drivers/usb/eth/asix88179.c line 408

Quoted 11 lines
drivers/usb/eth/asix88179.c new file +11 -0
+				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.

Quoted 125 lines
Quoted 15 lines
drivers/usb/eth/asix88179.c new file +15 -0
+	}
+
+	return 0;
+out_err:
+	return -1;
+}
+
+static int asix_send(struct eth_device *eth, void *packet, int length)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	int err;
+	u32 packet_len, tx_hdr2;
+	int actual_len;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
+				 PKTSIZE + (2 * sizeof(packet_len)));

drivers/usb/eth/asix88179.c line 433

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+
+	debug("** %s(), len %d\n", __func__, length);
+
+	packet_len = length;
+	cpu_to_le32s(&packet_len);
+
+	memcpy(msg, &packet_len, sizeof(packet_len));
+
+	tx_hdr2 = 0;
+	if (((length + 8) % 0x200 /*frame_size*/) == 0)

Define the frame size as a named constant, then use it here.

Quoted 20 lines
drivers/usb/eth/asix88179.c new file +20 -0
+		tx_hdr2 |= 0x80008000;	/* Enable padding */
+
+	cpu_to_le32s(&tx_hdr2);
+
+	memcpy(msg + sizeof(packet_len), &tx_hdr2, sizeof(tx_hdr2));
+
+	memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
+	       (void *)packet, length);
+
+	err = usb_bulk_msg(dev->pusb_dev,
+				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
+				(void *)msg,
+				length + sizeof(packet_len) + sizeof(tx_hdr2),
+				&actual_len,
+				USB_BULK_SEND_TIMEOUT);
+	debug("Tx: len = %u, actual = %u, err = %d\n",
+	      length + sizeof(packet_len), actual_len, err);
+
+	return err;
+}

[...]

Quoted 40 lines
drivers/usb/eth/asix88179.c new file +40 -0
+/* Probe to see if a new device is actually an asix device */
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *iface_desc;
+	int ep_in_found = 0, ep_out_found = 0;
+	int i;
+
+	/* let's examine the device now */
+	iface = &dev->config.if_desc[ifnum];
+	iface_desc = &dev->config.if_desc[ifnum].desc;
+
+	for (i = 0; asix_dongles[i].vendor != 0; i++) {
+		if (dev->descriptor.idVendor == asix_dongles[i].vendor &&
+		    dev->descriptor.idProduct == asix_dongles[i].product)
+			/* Found a supported dongle */
+			break;
+	}
+
+	if (asix_dongles[i].vendor == 0)
+		return 0;
+
+	memset(ss, 0, sizeof(struct ueth_data));
+
+	/* At this point, we know we've got a live one */
+	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
+	      dev->descriptor.idVendor, dev->descriptor.idProduct);
+
+	/* Initialize the ueth_data structure with some useful info */
+	ss->ifnum = ifnum;
+	ss->pusb_dev = dev;
+	ss->subclass = iface_desc->bInterfaceSubClass;
+	ss->protocol = iface_desc->bInterfaceProtocol;
+
+	/* alloc driver private */
+	ss->dev_priv = calloc(1, sizeof(struct asix_private));
+	if (!ss->dev_priv)
+		return 0;
+

drivers/usb/eth/asix88179.c line 598

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+	((struct asix_private *)ss->dev_priv)->flags = asix_dongles[i].flags;
+
+	/*
+	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
+	 * int. We will ignore any others.
+	 */
+	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		/* is it an BULK endpoint? */
+		if ((iface->ep_desc[i].bmAttributes &
+		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {

if (! ...)
continue;

if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
something
}

if (!(ep_addr & USB_DIR_IN) && !ep_out_found) {
something
}

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
+			if (ep_addr & USB_DIR_IN) {
+				if (!ep_in_found) {
+					ss->ep_in = ep_addr &

drivers/usb/eth/asix88179.c line 612

Quoted 10 lines
drivers/usb/eth/asix88179.c new file +10 -0
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_in_found = 1;
+				}
+			} else {
+				if (!ep_out_found) {
+					ss->ep_out = ep_addr &
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_out_found = 1;
+				}
+			}

See above how to trim down the indent here.
[...]

Signature · 13 lines
--------------------------------------------------------

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   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
#4 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

Quoted 8 lines · Ren? Griessl
Quoted 8 lines
Quoted 7 lines

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

Quoted 7 lines
Quoted 5 lines

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)

[...]

Quoted 10 lines
Quoted 9 lines

drivers/usb/eth/asix88179.c line 207

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+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 ?

Quoted 2 lines

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 ?

Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/eth/asix88179.c new file +4 -0
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)

(quoted diff) +2 -0
+{
+	int len;
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +2 -0
+
+	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x

size=%d\n", + cmd, value, index, size);

(quoted diff) +2 -0
+
+	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.

Quoted 2 lines

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 .

[...]

Quoted 9 lines
Quoted 6 lines

drivers/usb/eth/asix88179.c line 384

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	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) ?

Quoted 33 lines
Quoted 19 lines

drivers/usb/eth/asix88179.c

Quoted 12 lines
drivers/usb/eth/asix88179.c new file +12 -0
+	} 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... ");

Review

Quoted 1 line
(quoted diff) +1 -0
+				udelay(TIMEOUT_RESOLUTION * 1000);

mdelay()

Quoted 13 lines

drivers/usb/eth/asix88179.c line 408

Quoted 11 lines
drivers/usb/eth/asix88179.c new file +11 -0
+				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 ...

Quoted 1 line

I will put the link detect routine in a separate function.

That's OK, but the indent could also use some work ...

Thanks!

[...]

author
#5 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

Quoted 9 lines · Ren? Griessl
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

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

Quoted 7 lines
Quoted 6 lines
Quoted 5 lines

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?

Quoted 10 lines

[...]

Quoted 8 lines
Quoted 8 lines

drivers/usb/eth/asix88179.c line 207

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+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 ?

Quoted 4 lines
Quoted 2 lines

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.

Quoted 18 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/usb/eth/asix88179.c new file +4 -0
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16

index, + u16 size, void *data)
+{

Quoted 8 lines

Review

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +3 -0
+	int len;
+
+	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x

size=%d\n", + cmd, value, index, size);

(quoted diff) +2 -0
+
+	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.

Quoted 2 lines

Really? I took all of the variables from the function call. So with one has not declaration?

You have this:

Quoted 8 lines

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.

Quoted 11 lines

[...]

Quoted 7 lines
Quoted 5 lines

drivers/usb/eth/asix88179.c line 384

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	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.

Quoted 1 line

[...]

Signature · 13 lines
--------------------------------------------------------

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   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
#6 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

Quoted 17 lines · Ren? Griessl
Quoted 17 lines · Ren? Griessl
Quoted 15 lines
Quoted 8 lines
Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 7 lines
Quoted 2 lines

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.

Quoted 15 lines
Quoted 15 lines

[...]

Quoted 12 lines
Quoted 9 lines

drivers/usb/eth/asix88179.c line 207

Quoted 6 lines
drivers/usb/eth/asix88179.c new file +6 -0
+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)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 25 lines
Quoted 25 lines
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/eth/asix88179.c new file +4 -0
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value,

u16 index, + u16 size, void *data)

(quoted diff) +2 -0
+{
+	int len;
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +2 -0
+
+	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x

size=%d\n", + cmd, value, index, size);

(quoted diff) +2 -0
+
+	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.

Quoted 2 lines

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 */

Quoted 7 lines
Quoted 5 lines

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.

Quoted 9 lines
Quoted 9 lines

[...]

Quoted 7 lines
Quoted 6 lines

drivers/usb/eth/asix88179.c line 384

Quoted 4 lines
drivers/usb/eth/asix88179.c new file +4 -0
+	if (link_detected) {
+		if (timeout != 0)
+			printf("done.\n");
+			return 0;

Where does this return 0; belong to ?

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 4 lines

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.

#7 lore raw

PATCH v3 2/3 usb: eth: add ASIX AX88179 DRIVER

In reply to Rene Griessl

Hello Rene,

Quoted 9 lines

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.

author
#8 lore raw

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:

Quoted 20 lines

Hello Rene,

Quoted 9 lines

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.

Signature · 13 lines
--------------------------------------------------------

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   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
author
#9 lore raw

PATCH v3 3/3 usb: eth: enable AX88179 DRIVER for ARNDALE 5250

In reply to Rene Griessl

enable this driver for arndale5250 board

Signed-off-by Rene Griessl <[email protected]>
---
 include/configs/arndale.h | 1 +
 1 file changed, 1 insertion(+)
include/configs/arndale.h +1 -0
index 75f9933..cf5c39e 100644
@@ -118,6 +118,7 @@
 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS	3
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_ASIX88179
 
 /* MMC SPL */
 #define CONFIG_EXYNOS_SPL
Signature · 1 line
1.9.1
#10 lore raw

PATCH v3 1/3 usb: eth: fix Makefile

In reply to Rene Griessl
Quoted 1 line · Rene Griessl

fix issue as discussed with Marek Vasut

Now I'm twice as famous ;-)

But in fact, you should actually explain the change you did in the commit message (aka. something along the lines of the ifdef being useless and that using simpler and consistent notation is fine).

Quoted 10 lines
Signed-off-by Rene Griessl <[email protected]>
---
 drivers/usb/eth/Makefile | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/eth/Makefile +0 -0
index 94551c4..e6ae9f1 100644

drivers/usb/eth/Makefile line 10

Quoted 9 lines
drivers/usb/eth/Makefile +1 -3
@@ -5,8 +5,6 @@
 
 # new USB host ethernet layer dependencies
 obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
-ifdef CONFIG_USB_ETHER_ASIX
-obj-y += asix.o
-endif
+obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
 obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
 obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o

Best regards,
Marek Vasut