[U-Boot] [PATCH v2 06/12] USB: gadget: added a saner gadget downloader registration API

[Marek Vasut]

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 ?
[...]