Re: [RFC] Atmel-supplied hardware headers for AT91RM9200 SoC processor

[Alexey Dobriyan <adobriyan@gmail.com>]

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/