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

[René Griessl]

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