[U-Boot] [PATCH v3 2/7] dfu:usb: DFU USB function (f_dfu) support for g_dnl composite gadget

Lukasz Majewski
2012-08-02T10:54:31Z

Dear Mike,

> On Tuesday 31 July 2012 02:36:58 Lukasz Majewski wrote:
> > --- /dev/null
> > +++ b/drivers/usb/gadget/f_dfu.c
> >
> > +static struct usb_interface_descriptor dfu_intf_runtime = {
>
> can this be made const ?

Unfortunately those structs cannot be const, since some of their fields
are filled during running DFU code(when switching between "normal" and
"dfu" mode).
>
> > +static struct usb_descriptor_header *dfu_runtime_descs[] = {
> > + (struct usb_descriptor_header *) &dfu_intf_runtime,
>
> can you change the descs array to be const ?
> static const struct usb_descriptor_header * const dfu_runtime_descs[]
> = {
>

The same as above. Moreover the DFU implementation passes the
information about available alt settings as descriptors filled at
runtime.

> then i think you can drop the cast there ...
>
> > +static struct usb_qualifier_descriptor dev_qualifier = {
Struct above can only be defined as const.
> > +static struct usb_gadget_strings stringtab_dfu_generic = {
> > +static struct usb_gadget_strings *dfu_generic_strings[] = {
> > +static struct usb_gadget_strings stringtab_dfu = {
> > +static struct usb_gadget_strings *dfu_strings[] = {
>
> can these be made const ?
>
> > +static void handle_getstate(struct usb_request *req)
> > +{
> > + struct f_dfu *f_dfu = req->context;
> > +
> > + ((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;
>
> pretty sure you don't need that "& 0xff"
OK.
>
> > +static int state_app_idle(struct f_dfu *f_dfu,
> > + const struct usb_ctrlrequest *ctrl,
> > + struct usb_gadget *gadget,
> > + struct usb_request *req)
> > +{
> > + int value = 0;
>
> might be good to push this down into the 1 case statement
> (USB_REQ_DFU_GETSTATE) that uses it rather than init it up top
>

I haven't understood this comment.

Do you suggest to combine all those functions (like state_app_* or
state_dfu_*) in one big switch - case clause?
The state machine was initially implemented in this way, but for sake
of clearness Marek Vasut asked to split it down do separate functions
as defined at dfu_state[]. I also think that this approach is better
than one bit switch - case.

> same goes for all the other funcs below that follow this style
> -mike

--
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group