Re: [PATCHv5 2/3] USB: gadget: Use new composite features in some gadgets

[David Brownell <david-b@pacbell.net>]

--- On Wed, 7/28/10, Michal Nazarewicz <m.nazarewicz@samsung.com> wrote:

> From: Michal Nazarewicz <m.nazarewicz@samsung.com>
> Subject: [PATCHv5 2/3] USB: gadget: Use new composite features in some gadgets

NAK


Let's have one patch per gadget or function driver.
WITH a good explanation of what's changed in each.

What I see is a whole lot of random changes that
don't make ANY sense as one combined patch.  Plus,
some look quite dubious...


> use the new features of composite framework.  Because
> it
> handles default strings there is no longer the need for
> the
> gadgets drivers to handle many of the strings.

The gadgets should always identify the same, and
thus handle their strings -- *unless* module params
are applied by users to override those defaults.
The reason to use the module params is because a
product wants to be a *different* gadget, which
must be possible but won't be routine.  It
suffices to be different instances (serial #s)
in most routine usage.

> 
> This also adds the "needs_serial" to Mass Storage
> Gadget and

When the mass-storage only patch gets sent, I'll
want to see Alan's ack.

> Multifunction Composite Gadget which makes composite issue
> a warning if user space has not provided iSerialNumber parameter.
> 
>  
>  
> -static unsigned short gfs_vendor_id    =
> 0x0525;    /* XXX NetChip */
> -static unsigned short gfs_product_id   =
> 0xa4ac;    /* XXX */

Look -- you can't assign NetChip numbers!!!  I
personally have a handful of them, and if I didn't
assign them, they CANNOT be used.  That XXX
makes me think you (or someone) just randomly
picked a (broken) number.  The original file
storage gadget had a correctly assigned number.
If you're using anything else, fix it; there are
numbers Greg can assign from Linux Foundation's
USB-IF membership.

Comments like "XXX" need explanations, too...
if the intent is to seem like the file storage
gadget, just say so.


>  
> -    /* Vendor and product id can be
> overridden by module parameters.  */
> -    /* .idVendor   
>     = cpu_to_le16(gfs_vendor_id), */
> -    /* .idProduct   
>     = cpu_to_le16(gfs_product_id), */


Again, screwey.  Use the standard IDs unless
they get overridden.  Don't require them to be
overridden ... they were assigned in the first
place to be safe.  Module overrides are for folk
who put out their own products and want them to be
visibly different from  the generic Linux ones,
and thus need to manage their own USB-IF vid/pid
codes

What you're doing is changing the whole model so
there's no longer a standard "this is what Linux
does by default" -- and *requiring* a lot more
pain and suffering from folk configuring gadgets.
PLUS ... almost ensuring they'll get it wrong.  Not
anything vaguely like an improvement.


> -    /* .bcdDevice   
>     = f(hardware) */
> -    /* .iManufacturer    =
> DYNAMIC */
> -    /* .iProduct   
>     = DYNAMIC */
> -    /* NO SERIAL NUMBER */
> -    .bNumConfigurations    =
> 1,
> +    .idVendor   
>     = cpu_to_le16(0x0525),
> +    .idProduct   
>     = cpu_to_le16(0xa4ac),

Same as above.  You've broken ID management.
Use the (correct) symbols, not magic numbers.

Do you not understand how fundamental proper
management of vendor and product IDs is????