Dear Remy Bohmer,
This patch implements the low-level part of the USB CDC layer for AT91 Other boards can use this patch as an example to connect their own. diff --git a/arch/arm/include/asm/arch-at91/at91_dbgu.h b/arch/arm/include/asm/arch-at91/at91_dbgu.h new file mode 100644
Please see my patch introducing at91_dbgu.h (4/5.July 2010)
+#define AT91_DBGU_CR (AT91_DBGU + 0x00) /* Control Register */ +#define AT91_DBGU_MR (AT91_DBGU + 0x04) /* Mode Register */ +#define AT91_DBGU_IER (AT91_DBGU + 0x08) /* Interrupt Enable */
Use struct access for SoC registers. Offset adressing is depreciated. See the struct in my at91_dbgu.h. Use readl/writel to access the hardware.
+#define AT91_DBGU_TXRDY (1 << 1) /* Transmitter Ready */ +#define AT91_DBGU_TXEMPTY (1 << 9) /* Transmitter Empty */
Don't define this here, UART handling is in drivers/serial/atmel_usart.? The first 9 registers in the DBGU are identical to the normal USARTS.
+#define AT91_CIDR_SRAMSIZ_1K (1 << 16) +#define AT91_CIDR_SRAMSIZ_2K (2 << 16) +#define AT91_CIDR_SRAMSIZ_112K (4 << 16) +#define AT91_CIDR_SRAMSIZ_4K (5 << 16) +#define AT91_CIDR_SRAMSIZ_80K (6 << 16)
I think we should not define stuff that is nowhere used, and if one space or tab will do.
diff --git a/arch/arm/include/asm/arch-at91/cpu.h b/arch/arm/include/asm/arch-at91/cpu.h new file mode 100644
<snip>
+#include <asm/hardware.h> +#include <asm/arch/at91_dbgu.h> +#include <asm/arch/io.h>
Don't include other header files at this point. If a c source requires both DBGU and CPU headers, it shall include both.
<snip>
+#define ARCH_ID_AT91RM9200 0x09290780 +#define ARCH_ID_AT91SAM9260 0x019803a0 +#define ARCH_ID_AT91SAM9261 0x019703a0 +#define ARCH_ID_AT91SAM9263 0x019607a0 +#define ARCH_ID_AT91SAM9RL64 0x019b03a0 +#define ARCH_ID_AT91CAP9 0x039A03A0
Nice to have, but determining at runtime which variant we are on makes only sense to me for pin-compatible variants, e.g. 9260, 9xe, 9g20. Then you will notice that their hardware is not different much, if at all. Non pin-compatible variants are #defined in their board specific header file. Use #ifdef in the source to handle necessary variant specific stuff...
<snip>
+//#define DEBUG 1 +//#define VERBOSE 1 +//#define PACKET_TRACE 1
NO C++ comments allowed. And no commented away defines in general. Maybe say it like this: /* * to switch on debug output define any of those: * DEBUG - general debug * VERBOSE - more debug * PACKET_TRACE - also print packets */ wherever possible use the debug() macro defines in common.h
- This driver expects the board has been wired with two GPIOs suppporting
- a VBUS sensing IRQ, and a D+ pullup. (They may be omitted, but the
- testing hasn't covered such cases.)
It should. Besides AT91 currently cannot handle IRQs. Also I think it should read D- pullup.
<snip>
hang();
Does this mean the system is dead and would need a watchdog or reset button to come alive again?
<snip>
- // terminer chaque requete dans la queue
C++ comment in unknown language ;)?
<snip>
+/* TODO:
- BUG_ON(!list_empty(&req->queue));
- kfree(req); */
Wrong multi line comment.
<snip>
+static void pullup(struct at91_udc *udc, int is_on)
<snip>
I suggest to define and use a weak function to dummy on/off the pullup. Board specific source can implement a function then to really switch on/off a pullup in whatever way the hardware has provided that. That need not necessarily be a GPIO pin...
if (cpu_is_at91rm9200())at91_set_gpio_value(udc->board.pullup_pin, 1);else if (cpu_is_at91sam9260() || cpu_is_at91sam9263()) {u32 txvc = at91_udp_read(udc, AT91_UDP_TXVC);txvc |= AT91_UDP_TXVC_PUON;at91_udp_write(udc, AT91_UDP_TXVC, txvc);} else if (cpu_is_at91sam9261()) {u32 usbpucr;usbpucr = at91_sys_read(AT91_MATRIX_USBPUCR);usbpucr |= AT91_MATRIX_USBPUCR_PUON;at91_sys_write(AT91_MATRIX_USBPUCR, usbpucr);
Using a board specific function will rid you of that cascade as well.
- u32 __iomem *creg = ep->creg;
- u8 __iomem *dreg = ep->creg + (AT91_UDP_FDR(0) - AT91_UDP_CSR(0));
Always use readl/writel to access hardware.
There are more coding style problems in the source, but for source pulled from the kernel that might be ok (Wolfgang?)
Best Regards, Reinhard Meyer