--- On Wed, 7/28/10, Michal Nazarewicz <m.nazarewicz@xxxxxxxxxxx> wrote: > From: Michal Nazarewicz <m.nazarewicz@xxxxxxxxxxx> > 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???? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
