[U-Boot] [PATCH 1/3] usbether: Fixed bug when using with PXA25X chips

[Łukasz Dałek]

On 17.08.2012 22:48, Marek Vasut wrote:
> Dear Łukasz Dałek,
>
>> PXA25X chips don't support alternate settings so code in ether.c
>> disables usage of CDC.
>> But only code defined between DEV_CONFIG_CDC signals that network is up.
>> This patch is fixing this bug by addding pxa_connect_gadget() which on
>> pxa25x chips signals that network is up and do nothing on any other
>> chips.
> You're getting better, it's a good sign you're learning, I'm glad to see it :-)
It's my first big project in which I got involved so I'm ready for 
criticism.

>> Signed-off-by: Łukasz Dałek<luk0104 at gmail.com>
>> ---
>>   drivers/usb/gadget/ether.c |   21 ++++++++++++++++++++-
>>   1 files changed, 20 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
>> index d975fb6..964fe2e 100644
>> --- a/drivers/usb/gadget/ether.c
>> +++ b/drivers/usb/gadget/ether.c
>> @@ -44,7 +44,12 @@ extern struct platform_data brd;
>>
>>   unsigned packet_received, packet_sent;
>>
>> -#define DEV_CONFIG_CDC	1
>> +#ifdef CONFIG_USB_GADGET_PXA2XX
>> +# undef DEV_CONFIG_CDC
>> +# define DEV_CONFIG_SUBSET 1
> Can't we make it into a config value?
>
> Does this work for you:
> 1) Add:
> CONFIG_USB_ETH_SUBSET
> CONFIG_USB_ETH_CDC
>
> 2) Change this to
>
> #if !defined(...SUBSET) || !defined(...CDC) || !defined(RNDIS)
> #define DEV_CONFIG_CDC /* preserve default behavior */
> #endif
>
> #if defined(...SUBSET)
> #define DEV_CONFIG_SUBSET
> #endif
>
> #if defined(...CDC)
> #define DEV_CONFIG_CDC
> #endif
>
> /* Note that RNDIS is already fixed */
Ok.

>
> -- FROM HERE IT GOES INTO DIFFERENT PATCH --
>
> 3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if defined
> above
Which one #if? That one I'd define in previous patch?

> 4) DTTO for SUBSET
>
> 5) define SUBSET in your header file
In my board's header file?

>> +#else
>> +# define DEV_CONFIG_CDC	1
>> +#endif
>>   #define GFP_ATOMIC ((gfp_t) 0)
>>   #define GFP_KERNEL ((gfp_t) 0)
>>
>> @@ -864,7 +869,9 @@ static struct usb_gadget_strings	stringtab = {
>>
>>   /*========================================================================
>> ====*/ static u8 control_req[USB_BUFSIZ];
>> +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS)
>>   static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
>> +#endif
> What trouble do you face here?
>
> Tom, this aligned(4) doesn't look correct, some ALLOC_ALIGNED or friend would
> help here, right ? :)
If you don't define DEV_CONFIG_CDC nor ...RNDIS and define DEV_CONFIG_SUBSET
then it won't compile (because of STATUS_BYTECOUNT).

>>   /**
>> @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep
>> *ep, struct usb_request *req)
>>
>>   #endif	/* RNDIS */
>>
>> +#ifdef CONFIG_USB_GADGET_PXA2XX
>> +static inline void pxa_connect_gadget(void)
>> +{
>> +	debug("PXA connecting gadget...\n");
>> +	l_ethdev.network_started = 1;
>> +	printf("USB network up!\n");
> Definitelly not printf(), but is this really PXA specific or is this part of the
> CDC subset goo?
I've took pattern from code in this file. What should I use instead of 
printf?
CDC subset disable some code for PXA which signalise that network was
started (l_ethdev... = 1) so this is quick-fix for this bug.

Łukasz Dałek