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

[Lukasz Majewski <l.majewski@samsung.com>]

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