On Tue, Apr 2, 2013 at 11:24 AM, Vivek Gautam <gautamvivek1...@gmail.com> wrote: > Hi Marek, > > > On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <ma...@denx.de> wrote: >> Dear Vivek Gautam, >> >>> Some cleanup in usb framework, nothing much on feature side. >>> >>> Signed-off-by: Vikas C Sajjan <vikas.saj...@samsung.com> >>> Signed-off-by: Vivek Gautam <gautam.vi...@samsung.com> >>> --- >>> common/usb.c | 18 ++++++++++-------- >>> common/usb_storage.c | 30 ++++++++++++++++-------------- >>> include/usb_defs.h | 2 +- >>> 3 files changed, 27 insertions(+), 23 deletions(-) >>> >>> diff --git a/common/usb.c b/common/usb.c >>> index 6fc0fc1..40c1547 100644 >>> --- a/common/usb.c >>> +++ b/common/usb.c >>> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev, >>> int index, ifno, epno, curr_if_num; >>> int i; >>> u16 ep_wMaxPacketSize; >>> + struct usb_interface *if_desc = NULL; >>> >>> ifno = -1; >>> epno = -1; >>> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device *dev, >>> &buffer[index])->bInterfaceNumber != >>> curr_if_num) { >>> /* this is a new interface, copy new desc */ >>> ifno = dev->config.no_of_if; >>> + if_desc = &dev->config.if_desc[ifno]; >>> dev->config.no_of_if++; >>> - memcpy(&dev->config.if_desc[ifno], >>> - &buffer[index], buffer[index]); >>> - dev->config.if_desc[ifno].no_of_ep = 0; >>> - dev->config.if_desc[ifno].num_altsetting = 1; >>> + memcpy(if_desc, &buffer[index], >>> buffer[index]); >>> + if_desc->no_of_ep = 0; >>> + if_desc->num_altsetting = 1; >>> curr_if_num = >>> - dev- >>>config.if_desc[ifno].desc.bInterfaceNumber; >>> + if_desc->desc.bInterfaceNumber; >>> } else { >>> /* found alternate setting for the interface >>> */ >>> - dev->config.if_desc[ifno].num_altsetting++; >>> + if_desc->num_altsetting++; >> >> This will crash as if_desc is set in the if() branch above and it will be >> NULL >> in the else{} branch. > > True, will add here > if_desc = &dev->config.if_desc[ifno]; > > One more thing actually i could notice, "ifno = -1" in this else section. > Is this what we want it here ? > >> >>> } >>> break; >>> case USB_DT_ENDPOINT: >>> epno = dev->config.if_desc[ifno].no_of_ep; >>> + if_desc = &dev->config.if_desc[ifno]; >>> /* found an endpoint */ >>> - dev->config.if_desc[ifno].no_of_ep++; >>> - memcpy(&dev->config.if_desc[ifno].ep_desc[epno], >>> + if_desc->no_of_ep++; >>> + memcpy(&if_desc->ep_desc[epno], >>> &buffer[index], buffer[index]); >>> ep_wMaxPacketSize = get_unaligned(&dev->config.\ >>> if_desc[ifno].\ >>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>> index fb322b4..475c218 100644 >>> --- a/common/usb_storage.c >>> +++ b/common/usb_storage.c >>> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode) >>> lun++) { >>> usb_dev_desc[usb_max_devs].lun = lun; >>> if (usb_stor_get_info(dev, &usb_stor[start], >>> - >> &usb_dev_desc[usb_max_devs]) == 1) { > > Do we really need this brace here? We just have one statement under this if. > >>> - usb_max_devs++; >>> - } >>> + &usb_dev_desc[usb_max_devs]) == 1) >>> + usb_max_devs++; >>> } >>> + >> >> >> What is this change here doing? > > There were these unaligned braces for if part, which we removed.
Please ignore above comment. It was meant for change prior to this, above. Will remove this extra line here. > >> >>> } >>> /* if storage device */ >>> if (usb_max_devs == USB_MAX_STOR_DEV) { >> >> [..] >> >> Best regards, >> Marek Vasut >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > > > > -- > Thanks & Regards > Vivek -- Thanks & Regards Vivek _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot