[U-Boot] [PATCH v1 3/5] usb, g_dnl: make iSerialNumber board configurable

[Heiko Schocher]

Hello Lukasz,

Am 22.10.2013 14:37, schrieb Lukasz Majewski:
> Hi Heiko,
>
>> add the possibility to set the iSerialNumber board specific.
>> Therefore the CONFIG_G_DNL_SERIAL_STRING is introduced, which
>> defines the maximum length of the iSerialNumber string.
>> The new function g_dnl_set_serialnumber() must called from
>> g_dnl_bind_fixup(), to setup the iSerialNumber string.
>>
>> Signed-off-by: Heiko Schocher<hs at denx.de>
>> Cc: Marek Vasut<marek.vasut at gmail.com>
>> Cc: Lukasz Majewski<l.majewski at samsung.com>
>> Cc: Kyungmin Park<kyungmin.park at samsung.com>
>> ---
>>   drivers/usb/gadget/g_dnl.c | 28 ++++++++++++++++++++++++++++
>>   include/g_dnl.h            |  3 +++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
>> index 40868c0..5f09d66 100644
>> --- a/drivers/usb/gadget/g_dnl.c
>> +++ b/drivers/usb/gadget/g_dnl.c
>> @@ -39,8 +39,21 @@
>>
>>   static const char shortname[] = "usb_dnl_";
>>   static const char product[] = "USB download gadget";
>> +#if defined(CONFIG_G_DNL_SERIAL_STRING)
>
> I don't like the #if defined preprocessor directives here. The g_dnl.c
> code is a generic code, to "glue" all functions together.
>
>> +#define STRING_SERIAL  3
>> +static char g_dnl_serial[CONFIG_G_DNL_SERIAL_STRING + 1];
>> +#endif
>>   static const char manufacturer[] = CONFIG_G_DNL_MANUFACTURER;
>>
>> +#if defined(CONFIG_G_DNL_SERIAL_STRING)
>> +void g_dnl_set_serialnumber(char *s)
>> +{
>> +	memset(g_dnl_serial, 0, CONFIG_G_DNL_SERIAL_STRING + 1);
>> +	if (strlen(s)<= CONFIG_G_DNL_SERIAL_STRING)
>> +		strncpy(g_dnl_serial, s, strlen(s));
>> +}
>> +#endif
>> +
>>   static struct usb_device_descriptor device_desc = {
>>   	.bLength = sizeof device_desc,
>>   	.bDescriptorType = USB_DT_DEVICE,
>> @@ -52,6 +65,9 @@ static struct usb_device_descriptor device_desc = {
>>   	.idVendor = __constant_cpu_to_le16(CONFIG_G_DNL_VENDOR_NUM),
>>   	.idProduct =
>> __constant_cpu_to_le16(CONFIG_G_DNL_PRODUCT_NUM), .iProduct =
>> STRING_PRODUCT, +#if defined(CONFIG_G_DNL_SERIAL_STRING)
>
> I think that #if defined(CONFIG_G_DNL_SERIAL_STRING) can be easily
> removed, since the iSerialNumber is a valid member of struct
> usb_descriptor.
>
> In my opinion, instead of defining #ifdefs, we can define some "default"
> iSerialNumber for all devices.

Ok! How would look like the default serial number?

> Then this value can be overridden by board when needed by call to
> g_dnl_set_serialnumber() function

Ok. Will change it in this direction, thanks for the review!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany