Dear Vivek Gautam,
[...]
>
> This comes as an affect of introduction of "struct usb_ep_desc"
> which eventually contains "struct usb_endpoint_descriptor" and
> "struct usb_ss_ep_comp_descriptor".
I see, can we split this? I really enjoy small incremental patches, it's much
easier to bisect issues then.
> >> put_unaligned(le16_to_cpu(ep_wMaxPacketSize),
> >>
> >> &dev->config.\
> >> if_desc[ifno].\
> >> ep_desc[epno].\
> >>
> >> - wMaxPacketSize);
> >> + ep_desc.wMaxPacketSize);
> >>
> >> USB_PRINTF("if %d, ep %d\n", ifno, epno);
> >> break;
> >>
> >> + case USB_DT_SS_ENDPOINT_COMP:
> >> + if_desc = &dev->config.if_desc[ifno];
> >> + memcpy(&(if_desc->ep_desc[epno].ss_ep_comp),
> >
> > Do you need these braces in &(...) ?
>
> True, we can remove these braces.
>
> >> + &buffer[index], buffer[index]);
> >> + break;
> >>
> >> default:
> >> if (head->bLength == 0)
> >>
> >> return 1;
> >>
> >> @@ -659,6 +666,18 @@ static int usb_get_string(struct usb_device *dev,
> >> unsigned short langid, return result;
> >>
> >> }
> >>
> >> +/* Allocate usb device */
> >> +int usb_alloc_dev(struct usb_device *dev)
> >> +{
> >> + int res;
> >> +
> >> + USB_PRINTF("alloc device\n");
> >
> > this is a debug print.
>
> Isn't USB_PRINTF itself a conditional debug print ?
Yes, but I'd prefer to kill USB_PRINTF altogether in favor of debug().
> >> + res = usb_control_msg(dev, usb_sndctrlpipe(dev->parent, 0),
> >> + USB_REQ_ALLOC_DEV, 0, 0, 0,
> >> + NULL, 0, USB_CNTL_TIMEOUT);
> >
> > How does this "allocate" a device? Please, do a proper documentation.
> > Actually, take a look at include/linker_lists.h
> >
> > Or see here:
> > http://git.denx.de/?p=u-
> > boot.git;a=blob;f=include/linker_lists.h;h=0b405d78ea34df1c528fbc4e24ed2a
> > ad756ac4a2;hb=HEAD
> >
> > And here (U-Boot Code Documentation):
> > http://www.denx.de/wiki/U-Boot/CodingStyle
> >
> > It'd be really nice if you could follow such pattern of documentation!
>
> Yes, thanks for pointing out. Will document it properly to make things
> more understandable.
_AWESOME_ !
> >> + return res;
> >> +}
> >>
[...]
> >> diff --git a/include/common.h b/include/common.h
> >> index b23e90b..ef5f687 100644
> >> --- a/include/common.h
> >> +++ b/include/common.h
> >> @@ -211,6 +211,8 @@ typedef void (interrupt_handler_t)(void *);
> >>
> >> #define MIN(x, y) min(x, y)
> >> #define MAX(x, y) max(x, y)
> >>
> >> +#define min3(a, b, c) min(min(a, b), c)
> >> +
> >
> > Isn't this defined somewhere already?
>
> Can you please guide me here, i can see a similar inline function in
> ehci-hcd only :(
ICK, so we have ad-hoc implementation of this :-( I'd say, put it into common.h
and remove the ehci's ad-hoc implementation.
> > [...]
> >
> > Rest is just great.
>
> Thanks for reviewing this Marek. I shall update the patchset soon.
Welcome, it's pleasure to work with you ;-)
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
