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.