| Message ID | 1391533364-17663-7-git-send-email-m.zalega@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On Tuesday, February 04, 2014 at 06:02:38 PM, Mateusz Zalega wrote: > Preprocessor definitions and hardcoded implementation selection in > g_dnl core were replaced by a linker list made of (usb_function_name, > bind_callback) pairs. > > Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Marek Vasut <marex@denx.de> [...] > + > +/* export dfu_add to g_dnl.o */ > +ll_entry_declare(struct g_dnl_bind_callback, dfu_bind_callback, > + g_dnl_bind_callbacks) = { .usb_function_name = "usb_dnl_dfu", > + .fptr = dfu_add }; from linker-lists.h quote: 104 * ll_entry_declare() - Declare linker-generated array entry [...] 110 * This macro declares a variable that is placed into a linker-generated 111 * array. This is a basic building block for more advanced use of linker- 112 * generated arrays. The user is expected to build their own macro wrapper 113 * around this one. Can you follow this advice and build a macro for declaring these USB devices ? btw would you mind fixing the example for ll_entry_declare() in linker-lists.h ? It has four params in the example for some reason (it's a remnant from rework). [...] > +static inline struct g_dnl_bind_callback * g_dnl_first_bind_callback(void) > +{ > + return ll_entry_start(struct g_dnl_bind_callback, > + g_dnl_bind_callbacks); > +} > + > +static inline struct g_dnl_bind_callback * g_dnl_last_bind_callback(void) > +{ > + return ll_entry_end(struct g_dnl_bind_callback, > + g_dnl_bind_callbacks); > +} > + Are these two new functions called from multiple places at all? If not, just inline these ll_foo() calls and be done with it. FYI you can also make macros for these to avoid having to type all these args all around and duplicating the code. > static int g_dnl_do_config(struct usb_configuration *c) > { > const char *s = c->cdev->driver->name; > - int ret = -1; > > debug("%s: configuration: 0x%p composite dev: 0x%p\n", > - __func__, c, c->cdev); > - > + __func__, c, c->cdev); > printf("GADGET DRIVER: %s\n", s); > - if (!strcmp(s, "usb_dnl_dfu")) > - ret = dfu_add(c); > - else if (!strcmp(s, "usb_dnl_ums")) > - ret = fsg_add(c); > - else if (!strcmp(s, "usb_dnl_thor")) > - ret = thor_add(c); > - > - return ret; > + > + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); > + for (; callback != g_dnl_last_bind_callback(); ++callback) callback++ , this is not C++ where the order might matter. Nonetheless, you do want to use ll_entry_count() and ll_entry_get() with an iterator variable instead to make sure you don't step onto a corrupted field and crash in some weird way. > + if (!strcmp(s, callback->usb_function_name)) > + return callback->fptr(c); > + return -ENODEV; > } > > static int g_dnl_config_register(struct usb_composite_dev *cdev) > @@ -203,12 +210,12 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) > device_desc.bcdDevice = cpu_to_le16(gcnum); > else { > debug("%s: controller '%s' not recognized\n", > - shortname, gadget->name); > + __func__, gadget->name); > device_desc.bcdDevice = __constant_cpu_to_le16(0x9999); > } > > - debug("%s: calling usb_gadget_connect for " > - "controller '%s'\n", shortname, gadget->name); > + debug("%s: calling usb_gadget_connect for controller '%s'\n", > + __func__, gadget->name); Please split all these cleanups into a separate patch. [...]
On 02/05/14 08:13, Marek Vasut wrote: > On Tuesday, February 04, 2014 at 06:02:38 PM, Mateusz Zalega wrote: >> Preprocessor definitions and hardcoded implementation selection in >> g_dnl core were replaced by a linker list made of (usb_function_name, >> bind_callback) pairs. >> >> Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> >> Cc: Lukasz Majewski <l.majewski@samsung.com> >> Cc: Marek Vasut <marex@denx.de> > > [...] > >> + >> +/* export dfu_add to g_dnl.o */ >> +ll_entry_declare(struct g_dnl_bind_callback, dfu_bind_callback, >> + g_dnl_bind_callbacks) = { .usb_function_name = "usb_dnl_dfu", >> + .fptr = dfu_add }; > > > from linker-lists.h quote: > > 104 * ll_entry_declare() - Declare linker-generated array entry > [...] > 110 * This macro declares a variable that is placed into a linker-generated > 111 * array. This is a basic building block for more advanced use of linker- > 112 * generated arrays. The user is expected to build their own macro wrapper > 113 * around this one. > > Can you follow this advice and build a macro for declaring these USB devices ? > btw would you mind fixing the example for ll_entry_declare() in linker-lists.h ? > It has four params in the example for some reason (it's a remnant from rework). Yup > [...] > >> +static inline struct g_dnl_bind_callback * g_dnl_first_bind_callback(void) >> +{ >> + return ll_entry_start(struct g_dnl_bind_callback, >> + g_dnl_bind_callbacks); >> +} >> + >> +static inline struct g_dnl_bind_callback * g_dnl_last_bind_callback(void) >> +{ >> + return ll_entry_end(struct g_dnl_bind_callback, >> + g_dnl_bind_callbacks); >> +} >> + > > Are these two new functions called from multiple places at all? If not, just > inline these ll_foo() calls and be done with it. FYI you can also make macros > for these to avoid having to type all these args all around and duplicating the > code. Macros or static inlines, it's all the same, there's no point in changing that. The symbols aren't visible outside this compilation unit and function calls are, well, inlined. >> static int g_dnl_do_config(struct usb_configuration *c) >> { >> const char *s = c->cdev->driver->name; >> - int ret = -1; >> >> debug("%s: configuration: 0x%p composite dev: 0x%p\n", >> - __func__, c, c->cdev); >> - >> + __func__, c, c->cdev); >> printf("GADGET DRIVER: %s\n", s); >> - if (!strcmp(s, "usb_dnl_dfu")) >> - ret = dfu_add(c); >> - else if (!strcmp(s, "usb_dnl_ums")) >> - ret = fsg_add(c); >> - else if (!strcmp(s, "usb_dnl_thor")) >> - ret = thor_add(c); >> - >> - return ret; >> + >> + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); >> + for (; callback != g_dnl_last_bind_callback(); ++callback) > > callback++ , this is not C++ where the order might matter. Nonetheless, you do It doesn't matter anyway and can't be supported on Coding Style grounds, don't bug me. > want to use ll_entry_count() and ll_entry_get() with an iterator variable I don't think using ll_entry_get() in this way is possible with current implementation of linker lists. Moreover, there's plenty of code doing just the same already accepted to U-Boot. > instead to make sure you don't step onto a corrupted field and crash in some > weird way. Linker would have to split sections to make this sort of thing possible. Won't happen. >> + if (!strcmp(s, callback->usb_function_name)) >> + return callback->fptr(c); >> + return -ENODEV; >> } >> >> static int g_dnl_config_register(struct usb_composite_dev *cdev) >> @@ -203,12 +210,12 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) >> device_desc.bcdDevice = cpu_to_le16(gcnum); >> else { >> debug("%s: controller '%s' not recognized\n", >> - shortname, gadget->name); >> + __func__, gadget->name); >> device_desc.bcdDevice = __constant_cpu_to_le16(0x9999); >> } >> >> - debug("%s: calling usb_gadget_connect for " >> - "controller '%s'\n", shortname, gadget->name); >> + debug("%s: calling usb_gadget_connect for controller '%s'\n", >> + __func__, gadget->name); > > Please split all these cleanups into a separate patch. Right, I'll post v3. > [...] > Regards,
On Wednesday, February 05, 2014 at 01:40:27 PM, Mateusz Zalega wrote: [...] > > Are these two new functions called from multiple places at all? If not, > > just inline these ll_foo() calls and be done with it. FYI you can also > > make macros for these to avoid having to type all these args all around > > and duplicating the code. > > Macros or static inlines, it's all the same NAK, what you say is seriously wrong, you should know that by now! Macros do not do any kind of typechecking, functions do typechecking. Macros are expanded in place during preprocessing, functions are (usually) single-instance. etc. > there's no point in > changing that. The symbols aren't visible outside this compilation unit > and function calls are, well, inlined. It's pointless to have them pulled out so explicitly. Or would you prefer to have each function encapsulated in another function ? This doesn't make does now, does it ? What I would like to do is for you to follow the advice in linker_lists.h and produce a small shim over these crude linker lists prototypes there, so that the users here in the g_* world don't have to type the whole linker list macros with all the arguments, which do not ever change for this g_* . Does it make sense please? > >> static int g_dnl_do_config(struct usb_configuration *c) > >> { > >> > >> const char *s = c->cdev->driver->name; > >> > >> - int ret = -1; > >> > >> debug("%s: configuration: 0x%p composite dev: 0x%p\n", > >> > >> - __func__, c, c->cdev); > >> - > >> + __func__, c, c->cdev); > >> > >> printf("GADGET DRIVER: %s\n", s); > >> > >> - if (!strcmp(s, "usb_dnl_dfu")) > >> - ret = dfu_add(c); > >> - else if (!strcmp(s, "usb_dnl_ums")) > >> - ret = fsg_add(c); > >> - else if (!strcmp(s, "usb_dnl_thor")) > >> - ret = thor_add(c); > >> - > >> - return ret; > >> + > >> + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); > >> + for (; callback != g_dnl_last_bind_callback(); ++callback) > > > > callback++ , this is not C++ where the order might matter. Nonetheless, > > you do > > It doesn't matter anyway and can't be supported on Coding Style grounds, > don't bug me. Can be done on purely statistical grounds, try this: $ git grep 'for.*(.* *++[:alnum:]\+ *)' | wc -l 13 $ git grep 'for.*(.* *[:alnum:]\+++ *)' | wc -l 183 Please fix, thank you. > > want to use ll_entry_count() and ll_entry_get() with an iterator variable > > I don't think using ll_entry_get() in this way is possible with current > implementation of linker lists. Moreover, there's plenty of code doing > just the same already accepted to U-Boot. Ah meh, sorry. Seems like someone was messing with the linkerlists and misdesigned it. Dang. > > instead to make sure you don't step onto a corrupted field and crash in > > some weird way. > > Linker would have to split sections to make this sort of thing possible. > Won't happen. Can you please elaborate ? [...]
On 02/05/14 19:00, Marek Vasut wrote: > On Wednesday, February 05, 2014 at 01:40:27 PM, Mateusz Zalega wrote: > > [...] > >>> Are these two new functions called from multiple places at all? If not, >>> just inline these ll_foo() calls and be done with it. FYI you can also >>> make macros for these to avoid having to type all these args all around >>> and duplicating the code. >> >> Macros or static inlines, it's all the same > > NAK, what you say is seriously wrong, you should know that by now! > > Macros do not do any kind of typechecking, functions do typechecking. > Macros are expanded in place during preprocessing, functions are (usually) > single-instance. > > etc. Yeah, and it's all the same if you don't care for typechecking and all that, which I assumed from _you_ proposing usage of macros here. >> there's no point in >> changing that. The symbols aren't visible outside this compilation unit >> and function calls are, well, inlined. > > It's pointless to have them pulled out so explicitly. Or would you prefer to > have each function encapsulated in another function ? This doesn't make does > now, does it ? Pardon? > What I would like to do is for you to follow the advice in linker_lists.h and > produce a small shim over these crude linker lists prototypes there, so that the > users here in the g_* world don't have to type the whole linker list macros with > all the arguments, which do not ever change for this g_* . Does it make sense > please? It's taken care of by static inlines. >>>> static int g_dnl_do_config(struct usb_configuration *c) >>>> { >>>> >>>> const char *s = c->cdev->driver->name; >>>> >>>> - int ret = -1; >>>> >>>> debug("%s: configuration: 0x%p composite dev: 0x%p\n", >>>> >>>> - __func__, c, c->cdev); >>>> - >>>> + __func__, c, c->cdev); >>>> >>>> printf("GADGET DRIVER: %s\n", s); >>>> >>>> - if (!strcmp(s, "usb_dnl_dfu")) >>>> - ret = dfu_add(c); >>>> - else if (!strcmp(s, "usb_dnl_ums")) >>>> - ret = fsg_add(c); >>>> - else if (!strcmp(s, "usb_dnl_thor")) >>>> - ret = thor_add(c); >>>> - >>>> - return ret; >>>> + >>>> + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); >>>> + for (; callback != g_dnl_last_bind_callback(); ++callback) >>> >>> callback++ , this is not C++ where the order might matter. Nonetheless, >>> you do >> >> It doesn't matter anyway and can't be supported on Coding Style grounds, >> don't bug me. > > Can be done on purely statistical grounds, try this: > > $ git grep 'for.*(.* *++[:alnum:]\+ *)' | wc -l > 13 > $ git grep 'for.*(.* *[:alnum:]\+++ *)' | wc -l > 183 > > Please fix, thank you. Okay, whatever. >>> want to use ll_entry_count() and ll_entry_get() with an iterator variable >> >> I don't think using ll_entry_get() in this way is possible with current >> implementation of linker lists. Moreover, there's plenty of code doing >> just the same already accepted to U-Boot. > > Ah meh, sorry. Seems like someone was messing with the linkerlists and > misdesigned it. Dang. $ git show 42eba Yeah, it's a pity. >>> instead to make sure you don't step onto a corrupted field and crash in >>> some weird way. >> >> Linker would have to split sections to make this sort of thing possible. >> Won't happen. > > Can you please elaborate ? > [...] > You're guaranteed by the linker, and our setup, that all your linker-list data will end up in a contiguous block.
On Thursday, February 06, 2014 at 12:56:58 PM, Mateusz Zalega wrote: > On 02/05/14 19:00, Marek Vasut wrote: > > On Wednesday, February 05, 2014 at 01:40:27 PM, Mateusz Zalega wrote: > > > > [...] > > > >>> Are these two new functions called from multiple places at all? If not, > >>> just inline these ll_foo() calls and be done with it. FYI you can also > >>> make macros for these to avoid having to type all these args all around > >>> and duplicating the code. > >> > >> Macros or static inlines, it's all the same > > > > NAK, what you say is seriously wrong, you should know that by now! > > > > Macros do not do any kind of typechecking, functions do typechecking. > > Macros are expanded in place during preprocessing, functions are > > (usually) single-instance. > > > > etc. > > Yeah, and it's all the same if you don't care for typechecking and all > that, which I assumed from _you_ proposing usage of macros here. You will need macros wherever you do some kind of strange expansion. If you don't need to do strange expansion, use inline functions wherever possible. But I think in case of the linker generated lists, you really cannot avoid using macros. > >> there's no point in > >> changing that. The symbols aren't visible outside this compilation unit > >> and function calls are, well, inlined. > > > > It's pointless to have them pulled out so explicitly. Or would you prefer > > to have each function encapsulated in another function ? This doesn't > > make does now, does it ? > > Pardon? Sure ... Actually, looking at this, you are right that wrapping the LL macro with a function is a good idea. > > What I would like to do is for you to follow the advice in linker_lists.h > > and produce a small shim over these crude linker lists prototypes there, > > so that the users here in the g_* world don't have to type the whole > > linker list macros with all the arguments, which do not ever change for > > this g_* . Does it make sense please? > > It's taken care of by static inlines. Yes OK, you do have a point. [...] > >>> instead to make sure you don't step onto a corrupted field and crash in > >>> some weird way. > >> > >> Linker would have to split sections to make this sort of thing possible. > >> Won't happen. > > > > Can you please elaborate ? > > [...] > > You're guaranteed by the linker, and our setup, that all your > linker-list data will end up in a contiguous block. This doesn't mean someone won't corrupt it, but that's for an entirely different discussion I'd say.
diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 5547678..a03538d 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -22,7 +22,6 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) char *interface = argv[2]; char *devstring = argv[3]; - char *s = "dfu"; int ret, i = 0; ret = dfu_init_env_entities(interface, simple_strtoul(devstring, @@ -38,7 +37,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) int controller_index = simple_strtoul(usb_controller, NULL, 0); board_usb_init(controller_index, USB_INIT_DEVICE); - g_dnl_register(s); + g_dnl_register("usb_dnl_dfu"); while (1) { if (dfu_reset()) /* diff --git a/common/cmd_thordown.c b/common/cmd_thordown.c index c4b3511..2dd7509 100644 --- a/common/cmd_thordown.c +++ b/common/cmd_thordown.c @@ -22,7 +22,6 @@ int do_thor_down(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) char *interface = argv[2]; char *devstring = argv[3]; - const char *s = "thor"; int ret; puts("TIZEN \"THOR\" Downloader\n"); @@ -40,7 +39,7 @@ int do_thor_down(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) goto exit; } - g_dnl_register(s); + g_dnl_register("usb_dnl_thor"); ret = thor_init(); if (ret) { diff --git a/common/cmd_usb_mass_storage.c b/common/cmd_usb_mass_storage.c index 5175bd5..4c2de48 100644 --- a/common/cmd_usb_mass_storage.c +++ b/common/cmd_usb_mass_storage.c @@ -40,7 +40,7 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag, return CMD_RET_FAILURE; } - g_dnl_register("ums"); + g_dnl_register("usb_dnl_ums"); /* Timeout unit: seconds */ int cable_ready_timeout = UMS_CABLE_READY_TIMEOUT; diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c index a045864..cde1895 100644 --- a/drivers/usb/gadget/f_dfu.c +++ b/drivers/usb/gadget/f_dfu.c @@ -18,12 +18,14 @@ #include <errno.h> #include <common.h> #include <malloc.h> +#include <linker_lists.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/composite.h> #include <dfu.h> +#include <g_dnl.h> #include "f_dfu.h" struct f_dfu { @@ -768,9 +770,7 @@ static int dfu_bind_config(struct usb_configuration *c) int dfu_add(struct usb_configuration *c) { - int id; - - id = usb_string_id(c->cdev); + int id = usb_string_id(c->cdev); if (id < 0) return id; strings_dfu_generic[0].id = id; @@ -781,3 +781,8 @@ int dfu_add(struct usb_configuration *c) return dfu_bind_config(c); } + +/* export dfu_add to g_dnl.o */ +ll_entry_declare(struct g_dnl_bind_callback, dfu_bind_callback, + g_dnl_bind_callbacks) = { .usb_function_name = "usb_dnl_dfu", + .fptr = dfu_add }; diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index b1fe8bd..b7d03f2 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -243,6 +243,7 @@ #include <config.h> #include <malloc.h> #include <common.h> +#include <linker_lists.h> #include <usb.h> #include <linux/err.h> @@ -255,6 +256,7 @@ #include <linux/usb/gadget.h> #include <linux/usb/composite.h> #include <usb/lin_gadget_compat.h> +#include <g_dnl.h> /*------------------------------------------------------------------------*/ @@ -2778,3 +2780,7 @@ int fsg_init(struct ums *ums_dev) return 0; } + +ll_entry_declare(struct g_dnl_bind_callback, fsg_bind_callback, + g_dnl_bind_callbacks) = { .usb_function_name = "usb_dnl_ums", + .fptr = fsg_add }; diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c index c4c9909..3f428c8 100644 --- a/drivers/usb/gadget/f_thor.c +++ b/drivers/usb/gadget/f_thor.c @@ -17,6 +17,7 @@ #include <errno.h> #include <common.h> +#include <linker_lists.h> #include <malloc.h> #include <version.h> #include <linux/usb/ch9.h> @@ -1001,3 +1002,7 @@ int thor_add(struct usb_configuration *c) debug("%s:\n", __func__); return thor_func_init(c); } + +ll_entry_declare(struct g_dnl_bind_callback, thor_bind_callback, + g_dnl_bind_callbacks) = { .usb_function_name = "usb_dnl_thor", + .fptr = thor_add }; diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c index dd95afe..00ace2c 100644 --- a/drivers/usb/gadget/g_dnl.c +++ b/drivers/usb/gadget/g_dnl.c @@ -41,7 +41,6 @@ #define DRIVER_VERSION "usb_dnl 2.0" -static const char shortname[] = "usb_dnl_"; static const char product[] = "USB download gadget"; static char g_dnl_serial[MAX_STRING_SERIAL]; static const char manufacturer[] = CONFIG_G_DNL_MANUFACTURER; @@ -95,30 +94,38 @@ static int g_dnl_unbind(struct usb_composite_dev *cdev) free(cdev->config); cdev->config = NULL; - debug("%s: calling usb_gadget_disconnect for " - "controller '%s'\n", shortname, gadget->name); + debug("%s: calling usb_gadget_disconnect for controller '%s'\n", + __func__, gadget->name); usb_gadget_disconnect(gadget); return 0; } +static inline struct g_dnl_bind_callback * g_dnl_first_bind_callback(void) +{ + return ll_entry_start(struct g_dnl_bind_callback, + g_dnl_bind_callbacks); +} + +static inline struct g_dnl_bind_callback * g_dnl_last_bind_callback(void) +{ + return ll_entry_end(struct g_dnl_bind_callback, + g_dnl_bind_callbacks); +} + static int g_dnl_do_config(struct usb_configuration *c) { const char *s = c->cdev->driver->name; - int ret = -1; debug("%s: configuration: 0x%p composite dev: 0x%p\n", - __func__, c, c->cdev); - + __func__, c, c->cdev); printf("GADGET DRIVER: %s\n", s); - if (!strcmp(s, "usb_dnl_dfu")) - ret = dfu_add(c); - else if (!strcmp(s, "usb_dnl_ums")) - ret = fsg_add(c); - else if (!strcmp(s, "usb_dnl_thor")) - ret = thor_add(c); - - return ret; + + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); + for (; callback != g_dnl_last_bind_callback(); ++callback) + if (!strcmp(s, callback->usb_function_name)) + return callback->fptr(c); + return -ENODEV; } static int g_dnl_config_register(struct usb_composite_dev *cdev) @@ -203,12 +210,12 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) device_desc.bcdDevice = cpu_to_le16(gcnum); else { debug("%s: controller '%s' not recognized\n", - shortname, gadget->name); + __func__, gadget->name); device_desc.bcdDevice = __constant_cpu_to_le16(0x9999); } - debug("%s: calling usb_gadget_connect for " - "controller '%s'\n", shortname, gadget->name); + debug("%s: calling usb_gadget_connect for controller '%s'\n", + __func__, gadget->name); usb_gadget_connect(gadget); return 0; @@ -227,36 +234,21 @@ static struct usb_composite_driver g_dnl_driver = { .unbind = g_dnl_unbind, }; -int g_dnl_register(const char *type) +/* + * NOTICE: + * Registering via USB function name won't be necessary after rewriting + * g_dnl to support multiple USB functions. + */ +int g_dnl_register(const char *name) { - /* The largest function name is 4 */ - static char name[sizeof(shortname) + 4]; - int ret; - - if (!strcmp(type, "dfu")) { - strcpy(name, shortname); - strcat(name, type); - } else if (!strcmp(type, "ums")) { - strcpy(name, shortname); - strcat(name, type); - } else if (!strcmp(type, "thor")) { - strcpy(name, shortname); - strcat(name, type); - } else { - printf("%s: unknown command: %s\n", __func__, type); - return -EINVAL; - } - + debug("%s: g_dnl_driver.name = %s\n", __func__, name); g_dnl_driver.name = name; - debug("%s: g_dnl_driver.name: %s\n", __func__, g_dnl_driver.name); - ret = usb_composite_register(&g_dnl_driver); - + int ret = usb_composite_register(&g_dnl_driver); if (ret) { - printf("%s: failed!, error: %d\n", __func__, ret); + debug("%s: failed!, error: %d\n", __func__, ret); return ret; } - return 0; } diff --git a/include/dfu.h b/include/dfu.h index f973426..9956636 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -169,12 +169,5 @@ static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) } #endif -#ifdef CONFIG_DFU_FUNCTION int dfu_add(struct usb_configuration *c); -#else -int dfu_add(struct usb_configuration *c) -{ - return 0; -} -#endif #endif /* __DFU_ENTITY_H_ */ diff --git a/include/g_dnl.h b/include/g_dnl.h index 8f813c2..4392f76 100644 --- a/include/g_dnl.h +++ b/include/g_dnl.h @@ -10,6 +10,17 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> +#include <linux/usb/composite.h> + +typedef int (*g_dnl_bind_callback_f)(struct usb_configuration *); + +/* used in Gadget downloader callback linker list */ +struct g_dnl_bind_callback +{ + const char *usb_function_name; + g_dnl_bind_callback_f fptr; +}; + int g_dnl_bind_fixup(struct usb_device_descriptor *, const char *); int g_dnl_register(const char *s); void g_dnl_unregister(void); diff --git a/include/thor.h b/include/thor.h index afeade4..5051be7 100644 --- a/include/thor.h +++ b/include/thor.h @@ -15,13 +15,5 @@ int thor_handle(void); int thor_init(void); - -#ifdef CONFIG_THOR_FUNCTION int thor_add(struct usb_configuration *c); -#else -int thor_add(struct usb_configuration *c) -{ - return 0; -} -#endif #endif /* __THOR_H_ */ diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h index 058dcf1..ed46064 100644 --- a/include/usb_mass_storage.h +++ b/include/usb_mass_storage.h @@ -40,13 +40,5 @@ int fsg_init(struct ums *); void fsg_cleanup(void); struct ums *ums_init(unsigned int); int fsg_main_thread(void *); - -#ifdef CONFIG_USB_GADGET_MASS_STORAGE int fsg_add(struct usb_configuration *c); -#else -int fsg_add(struct usb_configuration *c) -{ - return 0; -} -#endif #endif /* __USB_MASS_STORAGE_H__ */
Preprocessor definitions and hardcoded implementation selection in g_dnl core were replaced by a linker list made of (usb_function_name, bind_callback) pairs. Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Marek Vasut <marex@denx.de> --- Changes since v1: - reordered --- common/cmd_dfu.c | 3 +- common/cmd_thordown.c | 3 +- common/cmd_usb_mass_storage.c | 2 +- drivers/usb/gadget/f_dfu.c | 11 ++++-- drivers/usb/gadget/f_mass_storage.c | 6 +++ drivers/usb/gadget/f_thor.c | 5 +++ drivers/usb/gadget/g_dnl.c | 74 +++++++++++++++++-------------------- include/dfu.h | 7 ---- include/g_dnl.h | 11 ++++++ include/thor.h | 8 ---- include/usb_mass_storage.h | 8 ---- 11 files changed, 66 insertions(+), 72 deletions(-)