Dear Ł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<luk0...@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?
Yes, continuous rework so you avoid breakage and keep this bisectable. > > 4) DTTO for SUBSET > > > > 5) define SUBSET in your header file > > In my board's header file? Yes > >> +#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). Fix STATUS_BYTECOUNT then. Why is status_req global at all? > >> /** > >> > >> @@ -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? debug() ? I guess we can wrap that function directly into the code anyway and drop all the debug output in it altogether, boiling it down only to l_ethdev.network_started = 1; > CDC subset disable some code for PXA which signalise that network was > started (l_ethdev... = 1) so this is quick-fix for this bug. This is really sad :-( > Łukasz Dałek Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot