Hi Marek,
Thanks for review.
> Dear Lukasz Majewski,
>
> > Composite USB download gadget support (g_dnl) for download
> > functions. This code works on top of composite gadget.
> >
> > Signed-off-by: Lukasz Majewski <l.majew...@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> > Cc: Marek Vasut <ma...@denx.de>
> > ---
> > drivers/usb/gadget/Makefile | 1 +
> > drivers/usb/gadget/g_dnl.c | 235
> > +++++++++++++++++++++++++++++++++++++++++++
> > include/g_dnl.h | 33 ++++++
> > 3 files changed, 269 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/usb/gadget/g_dnl.c
> > create mode 100644 include/g_dnl.h
> >
> > diff --git a/drivers/usb/gadget/Makefile
> > b/drivers/usb/gadget/Makefile index 87d1918..2c067c8 100644
> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -29,6 +29,7 @@ LIB := $(obj)libusb_gadget.o
> > ifdef CONFIG_USB_GADGET
> > COBJS-y += epautoconf.o config.o usbstring.o
> > COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
> > +COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o
> > endif
> > ifdef CONFIG_USB_ETHER
> > COBJS-y += ether.o epautoconf.o config.o usbstring.o
> > diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> > new file mode 100644
> > index 0000000..7925d0e
> > --- /dev/null
> > +++ b/drivers/usb/gadget/g_dnl.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * g_dnl.c -- USB Downloader Gadget
> > + *
> > + * Copyright (C) 2012 Samsung Electronics
> > + * Lukasz Majewski <l.majew...@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the Free Software Foundation; either version 2 of the License,
> > or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > 02111-1307 USA + */
> > +
> > +#include <errno.h>
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <linux/usb/ch9.h>
> > +#include <usbdescriptors.h>
> > +#include <linux/usb/gadget.h>
> > +
> > +#include <mmc.h>
> > +#include <part.h>
> > +
> > +#include <g_dnl.h>
> > +#include "f_dfu.h"
> > +
> > +#include "gadget_chips.h"
> > +#include "composite.c"
> > +
> > +/* Samsung's IDs */
> > +#define DRIVER_VENDOR_NUM 0x04E8
> > +#define DRIVER_PRODUCT_NUM 0x6601
>
> Is it samsung specific? Or can we use some USB IDs that are not bound
> to samsung?
I think, that those IDs shall be defined at include/configs/<board>.h
In that way one can use any ID.
>
> > +#define STRING_MANUFACTURER 25
> > +#define STRING_PRODUCT 2
> > +#define STRING_USBDOWN 2
> > +#define CONFIG_USBDOWNLOADER 2
> > +
> > +#define DRIVER_VERSION "usb_dnl 2.0"
> > +
> > +static const char shortname[] = "usb_dnl_";
> > +static const char product[] = "SLP USB download gadget";
> > +static const char manufacturer[] = "Samsung";
>
> DITTO?
Manufacturer field can be either omitted or changed to other name.
>
> > +
> > +static struct usb_device_descriptor device_desc = {
> > + .bLength = sizeof device_desc,
> > + .bDescriptorType = USB_DT_DEVICE,
> > +
> > + .bcdUSB = __constant_cpu_to_le16(0x0200),
> > + .bDeviceClass = USB_CLASS_COMM,
> > + .bDeviceSubClass = 0x02, /*0x02:CDC-modem ,
> > 0x00:CDC-serial*/ +
> > + .idVendor =
> > __constant_cpu_to_le16(DRIVER_VENDOR_NUM),
> > + .idProduct =
> > __constant_cpu_to_le16(DRIVER_PRODUCT_NUM),
> > + .iProduct = STRING_PRODUCT,
> > + .bNumConfigurations = 1,
> > +};
> > +
> > +static const struct usb_descriptor_header *otg_desc[] = {
> > + (struct usb_descriptor_header *) &(struct
> > usb_otg_descriptor){
> > + .bLength = sizeof(struct
> > usb_otg_descriptor),
> > + .bDescriptorType = USB_DT_OTG,
> > + .bmAttributes = USB_OTG_SRP,
> > + },
> > + NULL,
> > +};
> > +
> > +/* static strings, in UTF-8 */
> > +static struct usb_string odin_string_defs[] = {
> > + { 0, manufacturer, },
> > + { 1, product, },
> > +};
> > +
> > +static struct usb_gadget_strings odin_string_tab = {
> > + .language = 0x0409, /* en-us */
> > + .strings = odin_string_defs,
> > +};
> > +
> > +static struct usb_gadget_strings *g_dnl_composite_strings[] = {
> > + &odin_string_tab,
> > + NULL,
> > +};
> > +
> > +static int g_dnl_unbind(struct usb_composite_dev *cdev)
> > +{
> > + debug("%s\n", __func__);
> > + return 0;
> > +}
> > +
> > +static int g_dnl_do_config(struct usb_configuration *c)
> > +{
> > + int ret = -1;
> > + char *s = (char *) c->cdev->driver->name;
> > +
> > + debug("%s: c: 0x%p cdev: 0x%p\n", __func__, c, c->cdev);
>
> Can you elaborate in that debug message more? It's not really
> useful ;-)
I will correct that.
>
> > + if (gadget_is_otg(c->cdev->gadget)) {
> > + c->descriptors = otg_desc;
> > + c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> > + }
> > +
> > + printf("GADGET DRIVER: %s\n", s);
> > +
> > + if (!strcmp(s, "usb_dnl_dfu"))
> > + ret = dfu_add(c);
>
> And if this fails, we should set ret= as well, no?
The dynamic variable "ret" is defined with -1 value on the beginning of
the function. When comparison fails, the -1 value is returned.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int g_dnl_config_register(struct usb_composite_dev *cdev)
> > +{
> > + debug("%s:\n", __func__);
> > + static struct usb_configuration config = {
> > + .label = "usb_dnload",
> > + .bmAttributes = USB_CONFIG_ATT_ONE |
> > USB_CONFIG_ATT_SELFPOWER,
> > + .bConfigurationValue = CONFIG_USBDOWNLOADER,
> > + .iConfiguration = STRING_USBDOWN,
> > +
> > + .bind = g_dnl_do_config,
> > + };
> > +
> > + return usb_add_config(cdev, &config);
> > +}
> > +
> > +static int g_dnl_bind(struct usb_composite_dev *cdev)
> > +{
> > + int gcnum;
> > + int id, ret;
> > + struct usb_gadget *gadget = cdev->gadget;
> > +
> > + debug("%s: gadget: 0x%p cdev: 0x%p\n", __func__, gadget,
> > cdev); +
> > + id = usb_string_id(cdev);
> > +
> > + if (id < 0)
> > + return id;
> > + odin_string_defs[0].id = id;
> > + device_desc.iManufacturer = id;
> > +
> > + id = usb_string_id(cdev);
> > + if (id < 0)
> > + return id;
> > +
> > + odin_string_defs[1].id = id;
> > + device_desc.iProduct = id;
> > +
> > + ret = g_dnl_config_register(cdev);
> > + if (ret)
> > + goto error;
> > +
> > +
>
> Two newlines
Will fix.
>
> > + gcnum = usb_gadget_controller_number(gadget);
> > +
> > + debug("gcnum: %d\n", gcnum);
> > + if (gcnum >= 0)
> > + device_desc.bcdDevice = cpu_to_le16(0x0200 +
> > gcnum);
> > + else {
> > + debug("%s: controller '%s' not recognized\n",
> > + shortname, gadget->name);
> > + device_desc.bcdDevice =
> > __constant_cpu_to_le16(0x9999);
> > + }
> > +
> > + return 0;
> > +
> > + error:
> > + g_dnl_unbind(cdev);
> > + return -ENOMEM;
> > +}
> > +
> > +static void g_dnl_suspend(struct usb_composite_dev *cdev)
> > +{
> > + if (cdev->gadget->speed == USB_SPEED_UNKNOWN)
> > + return;
> > +
> > + debug("suspend\n");
> > +}
> > +
> > +static void g_dnl_resume(struct usb_composite_dev *cdev)
> > +{
> > + debug("resume\n");
> > +}
>
> Maybe you should check if suspend/resume are set at all. Then you
> won't need the above stubs.
I will remove them.
>
> > +static struct usb_composite_driver g_dnl_driver = {
> > + .name = NULL,
> > + .dev = &device_desc,
> > + .strings = g_dnl_composite_strings,
> > +
> > + .bind = g_dnl_bind,
> > + .unbind = g_dnl_unbind,
> > +
> > + .suspend = g_dnl_suspend,
> > + .resume = g_dnl_resume,
> > +};
> > +
> > +int g_dnl_init(char *s)
> > +{
> > + int ret;
> > + static char str[16];
> > +
>
> Why use '\0' if you can just use 0 ?
I will change.
>
> > + memset(str, '\0', sizeof(str));
> > +
> > + strncpy(str, shortname, sizeof(shortname));
> > +
> > + if (!strncmp(s, "dfu", sizeof(s))) {
> > + strncat(str, s, sizeof(str));
> > + } else {
> > + printf("%s: unknown command: %s\n", __func__, s);
> > + return -EINVAL;
> > + }
> > +
> > + g_dnl_driver.name = str;
> > +
> > + debug("%s: g_dnl_driver.name: %s\n", __func__,
> > g_dnl_driver.name);
> > + ret = usb_composite_register(&g_dnl_driver);
> > +
> > + if (ret) {
> > + printf("%s: failed!, error:%d ", __func__, ret);
>
> Missing newline in printf() (...error:%d ").
I will change that.
>
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void g_dnl_cleanup(void)
> > +{
> > + usb_composite_unregister(&g_dnl_driver);
> > +}
> > diff --git a/include/g_dnl.h b/include/g_dnl.h
> > new file mode 100644
> > index 0000000..5d0e9a5
> > --- /dev/null
> > +++ b/include/g_dnl.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Copyright (C) 2012 Samsung Electronics
> > + * Lukasz Majewski <l.majew...@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#ifndef __G_DOWNLOAD_H_
> > +#define __G_DOWNLOAD_H_
> > +
> > +#include <linux/usb/ch9.h>
> > +#include <usbdescriptors.h>
> > +#include <linux/usb/gadget.h>
> > +
> > +int g_dnl_init(char *s);
> > +void g_dnl_cleanup(void);
> > +
> > +/* USB initialization declaration - board specific*/
> > +void board_usb_init(void);
> > +#endif /* __G_DOWNLOAD_H_ */
--
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
