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.
>
>> }
>> /* 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
