Messages in this thread | | | From | Alexey Dobriyan <> | Subject | Re: [RFC] Atmel-supplied hardware headers for AT91RM9200 SoC processor | Date | Fri, 8 Jul 2005 01:52:03 +0400 |
| |
On Thursday 07 July 2005 13:58, Andrew Victor wrote: > If the AT91RM9200+Linux community had to convert all the headers, bugs > may be introduced in the conversion process and we would have to assume > any maintenance responsibility. What we have now may be slightly ugly, > but it is atleast known to be correct. > I've appended two of their headers as an example - the System > peripherals (timer, interrupt controller, etc) and Ethernet.
> Comments?
Care to get rid of cool *_UART_MAP macros?
For those who didn't bother to download the patch, the snippet is:
#define FOO_UART_MAP { 4, 1, -1, -1, -1 } ... int serial[AT91C_NR_UART] = FOO_UART_MAP; [yes, each one is used in exactly one place]
Obviously, AT91C_NR_UART should be AT91C_NR_UARTS, because it is "the number of UART_s_". And "serial" can be renamed to uart_map or something.
You also constantly cast ioremap() return values to unsigned long and cast them back to "void __iomem *" back on iounmap()
> +static struct resource *_s1dfb_resource[] = { > + /* order *IS* significant */ > + { /* video mem */
Then rewrite it as
static struct ... = { [0] = { .name = ... ... }, [1] = { .name = ... ... }, }
And check for non-NULL data in at91_add_device_usbh() is useless: at91_add_device_usbh(&carmeva_usbh_data); at91_add_device_usbh(&csb337_usbh_data); at91_add_device_usbh(&csb637_usbh_data); at91_add_device_usbh(&dk_usbh_data); at91_add_device_usbh(&ek_usbh_data);
Same for add_device_eth(), _udc(), _cf(). In at91_add_device_mmc() you got it right.
> +static struct file_operations spidev_fops = { > + owner: THIS_MODULE,
Use C99 initializers. Everywhere.
> +static int __init at91_spidev_init(void) > +{ > +#ifdef CONFIG_DEVFS_FS
It will be removed soon.
at91_wdt_ioctl() isn't __user annotated. Let alone it is ioctl.
> +#define BYTE_SWAP4(x) \ > + (((x & 0xFF000000) >> 24) | \ > + ((x & 0x00FF0000) >> 8) | \ > + ((x & 0x0000FF00) << 8) | \ > + ((x & 0x000000FF) << 24))
It's already somewhere in include/linux/byteorder/
> unsigned* dmabuf = host->buffer;
Space before star, please. Also everywhere.
> + char* command = kmalloc(2, GFP_KERNEL);
Anyone remembers 1 kmallocated byte?
> --- linux-2.6.13-rc2.orig/include/asm-arm/arch-at91rm9200/AT91RM9200_EMAC.h > +++ linux-2.6.13-rc2/include/asm-arm/arch-at91rm9200/AT91RM9200_EMAC.h
> +typedef struct _AT91S_EMAC {
> +} AT91S_EMAC, *AT91PS_EMAC;
Potentially even worse than "PAT91S_EMAC", "AT91S_EMACP" combined. Putting P _there_... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |