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 Ethernetconnection... ");
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!
[...]