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

[Mateusz Zalega]

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.

-- 
Mateusz Zalega
Samsung R&D Institute Poland