Dear Vivek Gautam,
> 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];
Yea ;-)
> One more thing actually i could notice, "ifno = -1" in this else section.
> Is this what we want it here ?
Maybe I don't see it ?
> >> }
> >> 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.
If the condition is multi-line, then we use brace, yes.
> >> - 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.
>
> >> }
> >> /* 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
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
