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

[Mateusz Zalega]

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 at samsung.com>
>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>> Cc: Marek Vasut <marex at 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,

-- 
Mateusz Zalega
Samsung R&D Institute Poland