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!
[...]
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
