Re: [U-Boot] [PATCH v3 1/7] dfu:usb: Support for g_dnl composite download gadget.

2012-08-02 Thread Lukasz Majewski 
Hi Mike,

 On Tuesday 31 July 2012 02:36:57 Lukasz Majewski wrote:
  --- /dev/null
  +++ b/drivers/usb/gadget/g_dnl.c
 
  +static const struct usb_descriptor_header *otg_desc[] = {
  +   (struct usb_descriptor_header *) (struct
  usb_otg_descriptor){
 
 since you're just casting away things, you could just use (void*) to
 make it a bit more compact
 
 also, can this be:
 static const struct usb_descriptor_header * const otg_desc[]
 (notice the 2nd const)
 

After looking into the code I can say that otg_support is not needed
(at least for this gadget) in u-boot. Will be removed.

  +static struct usb_gadget_strings *g_dnl_composite_strings[] = {
 
 here too:
 static struct usb_gadget_strings * const g_dnl_composite_strings[] = {

This const is anyway discarded by compiler (GCC 4.6.1) when .string
field of struct usb_composite_driver is initialized. 

 
  +static int g_dnl_do_config(struct usb_configuration *c)
  +{
  +   int ret = -1;
  +   char *s = (char *) c-cdev-driver-name;
 
 pretty sure this should be:
   const char *s = c-cdev-driver-name;

Yes, you are right. Corrected.
 
  +static int g_dnl_bind(struct usb_composite_dev *cdev)
  +{
  +   int gcnum;
  +   int id, ret;
  +   struct usb_gadget   *gadget = cdev-gadget;
 
 can we stick to a single space for indentation between the type and
 the variable name please ?

Corrected.
 
  +int g_dnl_register(const char *type)
  +{
  +   int ret;
  +   /* We only allow dfu atm, so 3 should be enough */
  +   static char name[sizeof(shortname) + 3];
  +
  +   if (!strcmp(type, dfu)) {
  +   strcpy(name, shortname);
  +   strcat(name, type);
 
 if u-boot had stpcpy(), we could do:
   strcpy(stpcpy(name, shortname), type);
 
  --- /dev/null
  +++ b/include/g_dnl.h
 
  +/* USB initialization declaration - board specific*/
 
 needs a space before that */

Corrected.

 -mike



-- 
Best regards,

Lukasz Majewski

Samsung Poland RD Center | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 1/7] dfu:usb: Support for g_dnl composite download gadget.

2012-08-01 Thread Mike Frysinger 
On Tuesday 31 July 2012 02:36:57 Lukasz Majewski wrote:
 --- /dev/null
 +++ b/drivers/usb/gadget/g_dnl.c

 +static const struct usb_descriptor_header *otg_desc[] = {
 + (struct usb_descriptor_header *) (struct usb_otg_descriptor){

since you're just casting away things, you could just use (void*) to make it a 
bit more compact

also, can this be:
static const struct usb_descriptor_header * const otg_desc[]
(notice the 2nd const)

 +static struct usb_gadget_strings *g_dnl_composite_strings[] = {

here too:
static struct usb_gadget_strings * const g_dnl_composite_strings[] = {

 +static int g_dnl_do_config(struct usb_configuration *c)
 +{
 + int ret = -1;
 + char *s = (char *) c-cdev-driver-name;

pretty sure this should be:
const char *s = c-cdev-driver-name;

 +static int g_dnl_bind(struct usb_composite_dev *cdev)
 +{
 + int gcnum;
 + int id, ret;
 + struct usb_gadget   *gadget = cdev-gadget;

can we stick to a single space for indentation between the type and the 
variable name please ?

 +int g_dnl_register(const char *type)
 +{
 + int ret;
 + /* We only allow dfu atm, so 3 should be enough */
 + static char name[sizeof(shortname) + 3];
 +
 + if (!strcmp(type, dfu)) {
 + strcpy(name, shortname);
 + strcat(name, type);

if u-boot had stpcpy(), we could do:
strcpy(stpcpy(name, shortname), type);

 --- /dev/null
 +++ b/include/g_dnl.h

 +/* USB initialization declaration - board specific*/

needs a space before that */
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot