lkml.org 
[lkml]   [2005]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC] Atmel-supplied hardware headers for AT91RM9200 SoC processor
Date
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/

\
 
 \ /
  Last update: 2005-07-07 23:56    [W:0.526 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site