On Mon, Aug 27, 2012 at 03:34:21PM +0200, Michal Nazarewicz wrote:
>
> Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> writes:
>
> > On 08/24/2012 11:53 PM, Michal Nazarewicz wrote:
> >> I wouldn't duplicate the documentation. One in header file should be
> >> enough.
> >
> > I think I go for the .c file where this is implemented. If you change
> > the .c file most people don't care about the header file.
> >
> >>> +int usb_gadget_controller_number(struct usb_gadget *gadget)
> >>> +{
> >>> + if (gadget_is_net2280(gadget))
> >>> + return 0x01;
> >
> > .. snip...
> >
> >>> + else if (gadget_is_lpc32xx(gadget))
> >>> + return 0x33;
> >>> +
> >>> + return -ENOENT;
> >>> +}
> >>
> >> How about using a table with a loop. For instance something like:
> >>
> >> int usb_gadget_controller_number(struct usb_gadget *gadget)
> >> {
> >> static const char *const map[] = {
> >> "\x01net2280",
> >
> > ..snip..
> >
> >> "\x33lpc32xx_udc",
> >> };
> >>
> >> const char *const *it = map, *const *end = map + ARRAY_SIZE(map);
> >> const char *const name = gadget->name;
> >>
> >> for (; it != end; ++it) {
> >> if (!strcmp(name, *it + 1))
> >> return (unsigned char)**it;
> >> }
> >>
> >> return -ENOENT
> >> }
> >>
> >> Something less fancy should also do.
> >
> > I am a friend of explicit naming and less hacks. And if you sort the
> > map then you have less reads to get to the last entry. This can be
> > limited even further if make it a member of a struct.
>
> Yeah, that's why I wrote that something less fancy should do too, like:
>
> int usb_gadget_controller_number(struct usb_gadget *gadget)
> {
> static const struct name_to_num {
> const *name;
> int id;
> } map[] = {
> { "net2280", 0x01 },
> { "dummy_udc", 0x02 },
> { "pxa25x_udc", 0x03 },
> { "goku_udc", 0x06 },
> { "omap_udc", 0x08 },
> { "pxa27x_udc", 0x11 },
> { "s3c2410_udc", 0x12 },
> { "at91_udc", 0x13 },
> { "imx_udc", 0x14 },
> { "musb-hdrc", 0x16 },
> { "atmel_usba_udc", 0x18 },
> { "fsl-usb2-udc", 0x19 },
> { "amd5536udc", 0x20 },
> { "m66592_udc", 0x21 },
> { "fsl_qe_udc", 0x22 },
> { "ci13xxx_pci", 0x23 },
> { "langwell_udc", 0x24 },
> { "r8a66597_udc", 0x25 },
> { "s3c-hsotg", 0x26 },
> { "pch_udc", 0x27 },
> { "ci13xxx_msm", 0x28 },
> { "renesas_usbhs_udc", 0x29 },
> { "s3c-hsudc", 0x30 },
> { "net2272", 0x31 },
> { "dwc3-gadget", 0x32 },
> { "lpc32xx_udc", 0x33 },
> { NULL, -ENOENT }
> };
>
> const struct name_to_num *it = map;
> const char *const name = gadget->name;
> while (it->name && strcmp(name, it->name))
> ++it;
> return it->num;
> }
>
> > Which brings me to: How important is this function?
> > - hacks / limitations should be implement differently i.e. a feature
> > bitmap
>
> I totally agree.
>
> > - do we need a unique bcd value across all udcs? Is somebody using
> > this? Useful?
> > Most of my device have either 1.00 or 2.00.
> > We set this to 0x200 + UDC number. Why? Why the 0x200? The spec says
> > "device release number" and "release number" is defined as 0xJJMN
> > (with J major, M minor, N sub minor minor). I don't say we have to
> > follow this I just for a sane reason and a use case.
>
> Yeah, I was always wondering about that myself.
>
> > Using here something like 0x305 for kernel version 3.5 would be a
> > little more useful. We would know that the kernel on this device is
> > based on v3.5 (plus a big number of patches on top).
>
> Worth noting that version is also included in the default manufacturer.
The manufacturer is always overwritten by the actual device
manufacturer. I think adding linux kernel version to bcdDevice is much
more useful then what we have today.
--
balbi
Attachment:
signature.asc
Description: Digital signature
