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

[Marek Vasut]

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.