Hi Marek,
On Fri, Oct 26, 2012 at 3:48 PM, Marek Vasut <ma...@denx.de> wrote:
> 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.
>
Sure, will split this small change.
>> >> 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().
>
Ok, will switch to debug(...).
And may be in that case we may try to eliminate USB _PRINTF and
other such macro altogether from sub framework ?
>> >> + 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.
>
Yeah, sure will keep this in common.h and update ehci-hcd.
>> > [...]
>> >
>> > Rest is just great.
>>
>> Thanks for reviewing this Marek. I shall update the patchset soon.
>
> Welcome, it's pleasure to work with you ;-)
--
Thanks & Regards
Vivek
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
