From: Wolfgang Denk <wd@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/6] usb:g_dnl:f_usbd_thor: USB Download function to support THOR protocol
Date: Sat, 14 Apr 2012 15:29:22 +0200 [thread overview]
Message-ID: <20120414132922.6BDE520022F@gemini.denx.de> (raw)
In-Reply-To: <1334215049-20362-3-git-send-email-l.majewski@samsung.com>
Dear Lukasz Majewski,
In message <1334215049-20362-3-git-send-email-l.majewski@samsung.com> you wrote:
> Implementation of USB Download function supporting THOR protocol.
Sorry, I hit "send" too fast...
> + .bDescriptorSubtype = 0x00, /* 0x00 */
> + .bcdCDC = 0x0110,
...
> + .bDescriptorType = CS_INTERFACE, /* 0x24 */
> + .bDescriptorSubtype = 0x01, /* 0x01 */
> + .bmCapabilities = 0x00,
> + .bDataInterface = 0x01,
...
> + .bDescriptorSubtype = 0x02, /* 0x02 */
> + .bmCapabilities = 0x00,
...
> + .bDescriptorSubtype = 0x06, /* 0x06 */
...
> + .bEndpointAddress = 3 | USB_DIR_IN,
...
> + .bInterval = 0x9,
...
There should be some header file with symbolic names for akll these
hard coded magic numbers.
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +
Drop that. A single blank line is sufficient.
> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
> +{
> + struct usb_request *req;
> +
> + req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> + if (req) {
> + req->length = length;
> + req->buf = malloc(length);
> + if (!req->buf) {
> + usb_ep_free_request(ep, req);
> + req = NULL;
> + }
> + }
> + return req;
Turn logic around to avoid deepo nesting of code - please apply
globally. For example here:
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return NULL;
req->buf = malloc(length);
if (!req->buf) {
usb_ep_free_request(ep, req);
return NULL;
}
req->length = length;
return req;
> + debug("dev->out_req->length:%d dev->rxdata:%d\n",
> + dev->out_req->length, dev->rxdata);
Indentation by tAB only, please.
Also make sure to run your patches through checkpatch:
WARNING: space prohibited between function name and open parenthesis
'('
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
WARNING: space prohibited between function name and open parenthesis
'('
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
WARNING: __aligned(size) is preferred over
__attribute__((aligned(size)))
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
WARNING: space prohibited between function name and open parenthesis
'('
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
WARNING: space prohibited between function name and open parenthesis
'('
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
WARNING: __aligned(size) is preferred over
__attribute__((aligned(size)))
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))
> + switch (mode) {
> + case END_BOOT:
> + run_command("run bootcmd", 0);
> + break;
> + default:
> + break;
> + }
if ... else ... ?
> + if (strncmp(usbd_rx_data_buf, recv_key, strlen(recv_key)) == 0) {
> + printf("Download request from the Host PC\n");
Make this debug() ?
> + msleep(30);
Are you sure it is a PC?
> + strncpy(usbd_tx_data_buf, tx_key, strlen(tx_key));
> + usbd_tx_data(usbd_tx_data_buf, strlen(tx_key));
> + } else {
> + puts("Wrong reply information\n");
It might be useful to print information about whart was received, and
what was expected?
> + if (ret > 0) {
> + ret = process_data(dnl);
> +
> + if (ret < 0)
> + return -1;
> + else if (ret == 0)
> + return 0;
Isn't this overkill?
return process_data(dnl);
should be equivalent ?
> + /* If the following pointers are set to NULL -> error */
> + dnl->rx_buf = (unsigned int *) CONFIG_SYS_DOWN_ADDR;
Does it make sense to hardwire this address, and have it the same on
all boards? Or would it be more useful if this could be changed by
the user (like by storing it in an environment variable) ?
> + usb_downloader_intf_int.bInterfaceNumber = status;
> + usb_downloader_cdc_union.bMasterInterface = status;
BTW: CamelCaps names are forbidden in U-Boot. Please fix globally.
Actually you should probably re-read the CodingStyle document and
re-think most variable names.
> + /* note: a status/notification endpoint is, strictly speaking,
> + * optional. We don't treat it that way though! It's simpler,
> + * and some newer profiles don't treat it as optional.
> + */
Incorrect multiline comment style. Please fix globally.
> + debug("%s: out_ep:%p out_req:%p\n",
> + __func__, dev->out_ep, dev->out_req);
> + printf("%s: dnl: 0x%p\n", __func__, dnl);
Is this really useful and needed output for production mode?
Please check output globally, this seems to be way too verbose (OK for
testing, but not for production).
> +/* Samsung's IDs */
> +#define DRIVER_VENDOR_NUM 0x04E8
> +#define DRIVER_PRODUCT_NUM 0x6601
> +#define STRING_MANUFACTURER 25
Are there chances that other vendor / product / manufacurer ID's need
to be used here?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If I had to live my life again, I'd make the same mistakes, only
sooner. -- Tallulah Bankhead
next prev parent reply other threads:[~2012-04-14 13:29 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-12 7:17 [U-Boot] [PATCH 0/6] usb:composite:download Composite download gadget Lukasz Majewski
2012-04-12 7:17 ` [U-Boot] [PATCH 1/6] usb:composite:g_dnl: Composite gadget (g_dnl) for USB downloading functions Lukasz Majewski
2012-04-14 9:55 ` Marek Vasut
2012-04-12 7:17 ` [U-Boot] [PATCH 2/6] usb:g_dnl:f_usbd_thor: USB Download function to support THOR protocol Lukasz Majewski
2012-04-14 9:58 ` Marek Vasut
2012-04-14 13:12 ` Wolfgang Denk
2012-04-14 13:29 ` Wolfgang Denk [this message]
2012-04-12 7:17 ` [U-Boot] [PATCH 3/6] usb:g_dnl:thor: THOR protocol back end support for f_usbd_thor function Lukasz Majewski
2012-04-14 10:00 ` Marek Vasut
2012-04-14 13:30 ` Wolfgang Denk
2012-04-12 7:17 ` [U-Boot] [PATCH 4/6] usb:command: Support for USB Download command Lukasz Majewski
2012-04-14 10:04 ` Marek Vasut
2012-04-14 13:33 ` Wolfgang Denk
2012-04-12 7:17 ` [U-Boot] [PATCH 5/6] usb:g_dnl: Support for g_dnl download usb gadget for GONI board Lukasz Majewski
2012-04-14 13:39 ` Wolfgang Denk
2012-04-12 7:17 ` [U-Boot] [PATCH 6/6] usb:g_dnl: Support for g_dnl download usb gadget for TRATS board Lukasz Majewski
2012-04-14 13:40 ` Wolfgang Denk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120414132922.6BDE520022F@gemini.denx.de \
--to=wd@denx.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.