[U-Boot] [PATCH 1/8] USB Consolidate descriptor definitions

From: Tom Rix <tom@bumblecow.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 1/8] USB Consolidate descriptor definitions
Date: Wed, 14 Oct 2009 06:14:55 -0500	[thread overview]
Message-ID: <4AD5B2AF.70503@bumblecow.com> (raw)
In-Reply-To: <3efb10970910121208x77a2c1b2obc7fdd4ae7ab142c@mail.gmail.com>

Remy Bohmer wrote:
> Hi Tom,
> 
> Sorry for not responding to this series very fast, because I wanted to
> look into it carefully.
> Below my comments on this patch
> 
> 2009/9/28 Tom Rix <Tom.Rix@windriver.com>:
>> The header files usb.h and usbdescriptors.h have the same or
>> similarly named structure definitions for
>>
>> usb_device_descriptor
>> usb_string_descriptor
>> usb_endpoint_descriptor
>> usb_config_descriptor vs usb_configuration_descriptor
>> usb_interface_descriptor
>>
>> There should only be one definition of these structures.
> 
> True, I agree with that.
> 
>> Some of these structures are element-wise duplicates,
>> one has unused elements that can be reduced to a duplicate,
>> and some of the the structures have additional elements.
>>
>> These are the element-wise duplicates in usb.h
>>
>> usb_device_descriptor
>> usb_string_descriptor
>>
>> For these, the usb.h definition will be removed.
>>
>> This definition in usb.h has extra unused elements
> 
> Can these be merged to one global structure?
> 

No.
The size of the structure is kept in 'bLength' field.
Have bLength = sizeof(struct bla_descriptor)
is the right way to go.

>> usb_endpoint_descriptor
>>
>>        unsigned char   bRefresh
>>        unsigned char   bSynchAddress;
>>
>> The definition of usb_endpoint_descriptor in usb.h will
>> be removed.
>>
>> These definitions in usb.h have extra elements at the end of
>> the usb 2.0 specified descriptor:
>>
>> usb_config_descriptor
>> usb_interface_descriptor
>>
>> These structures will have their name shorted by removing the
>> '_descriptor' suffix.
>>
>> So
>>
>> usb_config_descriptor -> usb_config
>> usb_interface_descriptor -> usb_interface
>>
>> The common descriptor elements are now defined by the macros
>>
>> USB_CONFIG_DESCRIPTOR_DEFINITION
>> USB_INTERFACE_DESCRIPTOR_DEFINITION
> 
> Personally I do not like the macros for these kind of definitions, I
> would prefer a solution by nesting the structures.
> What do you think?

I have an early version of this patch that does use nested structures.
After looking at the added '.desc.' that caused a good number of
changes in the c files,  I went with the macro approach.   It seemed like
the lesser of the evils as the changes seemed smaller and were limited
to the h files.  What I really wanted to do was to use nameless
structures but had a problem getting them to work with gcc.

I do not mind reverting to the '.desc.' solution.
I will resend the patches when I have had a chance to test them.

Thanks
Tom

> 
> Kind regards,
> 
> Remy
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 