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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
