Hi Marek,
> Dear Lukasz Majewski,
>
> > Support for f_dfu USB function.
> >
> > Signed-off-by: Lukasz Majewski <l.majew...@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> > Cc: Marek Vasut <ma...@denx.de>
> > ---
> [...]
> > +
> > +static const char dfu_name[] = "Device Firmware Upgrade";
> > +
> > +/* static strings, in UTF-8 */
> > +/*
> > + * dfu_genericiguration specific
>
>
> generi....what?
Misspelled, fixed.
>
> > + */
> > +static struct usb_string strings_dfu_generic[] = {
> > + [0].s = dfu_name,
> > + { } /* end of list */
> > +};
> > +
> > +static struct usb_gadget_strings stringtab_dfu_generic = {
> > + .language = 0x0409, /* en-us */
> > + .strings = strings_dfu_generic,
> > +};
> > +
> > +static struct usb_gadget_strings *dfu_generic_strings[] = {
> > + &stringtab_dfu_generic,
> > + NULL,
> > +};
> > +
> > +/*
> > + * usb_function specific
> > + */
> > +static struct usb_gadget_strings stringtab_dfu = {
> > + .language = 0x0409, /* en-us */
> > + /*
> > + * .strings
> > + *
> > + * assigned during initialization,
> > + * depends on number of flash entities
> > + *
> > + */
> > +};
> > +
> > +static struct usb_gadget_strings *dfu_strings[] = {
> > + &stringtab_dfu,
> > + NULL,
> > +};
> > +
> > +/*------------------------------------------------------------------------
> > -*/ +
> > +static void dnload_request_complete(struct usb_ep *ep, struct
> > usb_request *req) +{
> > + struct f_dfu *f_dfu = req->context;
> > +
> > + dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
> > + req->length, f_dfu->blk_seq_num);
> > +
> > + if (req->length == 0) {
> > + puts("DOWNLOAD ... OK\n");
> > + puts("Ctrl+C to exit ...\n");
> > + }
> > +}
> > +
> > +static void handle_getstatus(struct usb_request *req)
> > +{
> > + struct dfu_status *dstat = (struct dfu_status *)req->buf;
> > + struct f_dfu *f_dfu = req->context;
> > +
> > + switch (f_dfu->dfu_state) {
> > + case DFU_STATE_dfuDNLOAD_SYNC:
>
> What's this crazy cammel case in here ? :-)
I know, that camel case descriptions are not welcome :-),
but those are written in this way for purpose.
dfuDNLOAD_SYNC is the exact state name mentioned at DFU specification.
Other states - like dfuDNBUSY are exactly the same.
>From mine experience it is more readable. Please consider for
example the Fig. A1 - Interface state transition diagram from page 28
at DFU_1.1.pdf spec (link below).
http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
>
> > + case DFU_STATE_dfuDNBUSY:
> > + f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
> > + break;
> > + case DFU_STATE_dfuMANIFEST_SYNC:
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + /* send status response */
> > + dstat->bStatus = f_dfu->dfu_status;
> > + /* FIXME: set dstat->bwPollTimeout */
>
> FIXME ... so fix it ;-)
This is not u-boot related - will be removed
>
> > + dstat->bState = f_dfu->dfu_state;
> > + dstat->iString = 0;
> > +}
> > +
> > +static void handle_getstate(struct usb_request *req)
> > +{
> > + struct f_dfu *f_dfu = req->context;
> > +
> > + ((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;
>
> Now ... this is ubercrazy ... can't this be made without this
> typecasting voodoo?
The problem is that req->buf is a void pointer. And the goal is to
store dfu state at 8 bits.
>
> > + req->actual = sizeof(u8);
> > +}
> > +
> [...]
> > +static int handle_dnload(struct usb_gadget *gadget, u16 len)
> > +{
> > + struct usb_composite_dev *cdev = get_gadget_data(gadget);
> > + struct usb_request *req = cdev->req;
> > + struct f_dfu *f_dfu = req->context;
> > +
> > + if (len == 0)
> > + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> > +
> > + req->complete = dnload_request_complete;
> > +
> > + return len;
> > +}
>
> One newline too much below.
Fixed.
>
> > +
> > +
> > +static int
> > +dfu_handle(struct usb_function *f, const struct usb_ctrlrequest
> > *ctrl) +{
> > + struct usb_gadget *gadget = f->config->cdev->gadget;
> > + struct usb_request *req = f->config->cdev->req;
> > + struct f_dfu *f_dfu = f->config->cdev->req->context;
> > + u16 len = le16_to_cpu(ctrl->wLength);
> > + u16 w_value = le16_to_cpu(ctrl->wValue);
> > + int value = 0;
> > + int standard;
> > +
> > + standard = (ctrl->bRequestType & USB_TYPE_MASK)
> > + ==
> > USB_TYPE_STANDARD; +
> > + debug("w_value: 0x%x len: 0x%x\n", w_value, len);
> > + debug("standard: 0x%x ctrl->bRequest: 0x%x
> > f_dfu->dfu_state: 0x%x\n",
> > + standard, ctrl->bRequest, f_dfu->dfu_state);
> > +
> > + if (!standard) {
>
> This function doesn't fit on my screen ... that means it's waaaay too
> long and shall be split into more functions ;-)
>
> That's also remove the need for your crazy conditional assignment of
> standard.
You are correct :-). I will split the if(!standard) clause to two
separate. It shall improve readability.
>
> > + switch (f_dfu->dfu_state) {
> > + case DFU_STATE_appIDLE:
> > + switch (ctrl->bRequest) {
> > + case USB_REQ_DFU_GETSTATUS:
> > + handle_getstatus(req);
> > + value = RET_STAT_LEN;
> > + break;
> [...]
>
> > +
> > +/*------------------------------------------------------------------------
> > -*/ +
> > +static int
> > +dfu_prepare_strings(struct f_dfu *f_dfu, int n)
> > +{
> > + struct dfu_entity *de = NULL;
> > + int i = 0;
> > +
> > + f_dfu->strings = calloc(((n + 1) * sizeof(struct
> > usb_string)), 1);
>
> calloc(n, 1) ... that's the same as malloc(), isn't it ?
I now wonder how mine mind produced this :-)
Correct version:
f_dfu->strings = calloc(sizeof(struct usb_string), n + 1);
I prefer calloc, since it delivers zeroed memory.
>
> > + if (!f_dfu->strings)
> > + goto enomem;
> > +
> > + for (i = 0; i < n; ++i) {
> > + de = dfu_get_entity(i);
> > + f_dfu->strings[i].s = de->name;
> > + }
> > +
> > + f_dfu->strings[i].id = 0;
> > + f_dfu->strings[i].s = NULL;
> > +
> > + return 0;
> > +
> > +enomem:
> > + while (i)
> > + f_dfu->strings[--i].s = NULL;
> > +
> > + kfree(f_dfu->strings);
> > +
> > + return -ENOMEM;
> > +}
> > +
> > +static int dfu_prepare_function(struct f_dfu *f_dfu, int n)
> > +{
> > + struct usb_interface_descriptor *d;
> > + int i = 0;
> > +
> > + f_dfu->function = calloc(n * sizeof(struct
> > usb_descriptor_header *), 1);
>
> DITTO
As above.
>
> > + if (!f_dfu->function)
> > + goto enomem;
> > +
> > + for (i = 0; i < n; ++i) {
> > + d = kzalloc(sizeof(*d), GFP_KERNEL);
>
> Why use the kernel alternatives ? just use malloc()/free() ?
I will use calloc(sizeof(*d), 1) to receive cleared memory.
>
> > + if (!d)
> > + goto enomem;
> > +
> > + d->bLength = sizeof(*d);
> > + d->bDescriptorType = USB_DT_INTERFACE;
> > + d->bAlternateSetting = i;
> > + d->bNumEndpoints = 0;
> > + d->bInterfaceClass = USB_CLASS_APP_SPEC;
> > + d->bInterfaceSubClass = 1;
> > + d->bInterfaceProtocol = 2;
> > +
> > + f_dfu->function[i] = (struct usb_descriptor_header
> > *)d;
> > + }
> > + f_dfu->function[i] = NULL;
> > +
> > + return 0;
> > +
> > +enomem:
> > + while (i) {
> > + kfree(f_dfu->function[--i]);
> > + f_dfu->function[i] = NULL;
> > + }
> > + kfree(f_dfu->function);
> > +
> > + return -ENOMEM;
> > +}
> > +
> > +static int dfu_bind(struct usb_configuration *c, struct
> > usb_function *f) +{
> > + struct usb_composite_dev *cdev = c->cdev;
> > + struct f_dfu *f_dfu = func_to_dfu(f);
> > + int alt_num = dfu_get_alt_number();
> > + int rv, id, i;
> > +
> > + id = usb_interface_id(c, f);
> > + if (id < 0)
> > + return id;
> > + dfu_intf_runtime.bInterfaceNumber = id;
> > +
> > + f_dfu->dfu_state = DFU_STATE_appIDLE;
> > + f_dfu->dfu_status = DFU_STATUS_OK;
> > +
> > + rv = dfu_prepare_function(f_dfu, alt_num);
> > + if (rv)
> > + goto error;
> > +
> > + rv = dfu_prepare_strings(f_dfu, alt_num);
> > + if (rv)
> > + goto error;
> > + for (i = 0; i < alt_num; i++) {
> > + id = usb_string_id(cdev);
> > + if (id < 0)
> > + return id;
> > + f_dfu->strings[i].id = id;
> > + ((struct usb_interface_descriptor
> > *)f_dfu->function[i])
> > + ->iInterface = id;
> > + }
> > +
> > + stringtab_dfu.strings = f_dfu->strings;
> > +
> > + cdev->req->context = f_dfu;
> > +
> > + return 0;
> > +error:
> > + return rv;
>
> So just return the retval ...
Fixed.
>
> > +}
> > +
>
> Every time I review patches and find multiple small issues, I feel
> bad :-(
All will be OK :-)
--
Best regards,
Lukasz Majewski
Samsung Poland R&D Center | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
