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łekluk0104@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:
- Add:
CONFIG_USB_ETH_SUBSET CONFIG_USB_ETH_CDC
- 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 --
- 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.
DTTO for SUBSET
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