From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Fri, 26 Sep 2014 17:49:34 +0200 Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER In-Reply-To: <54258952.3020802@cit-ec.uni-bielefeld.de> References: <1411718929-23048-1-git-send-email-rgriessl@cit-ec.uni-bielefeld.de> <201409261547.59022.marex@denx.de> <54258952.3020802@cit-ec.uni-bielefeld.de> Message-ID: <201409261749.34316.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.