[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Marek Vasut <marex@denx.de>]

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!

[...]