Re: [PATCH 1/3] add USB setup code for 8349emds PB

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 06 February 2007 10:05, Li Yang wrote:
>  
> +static volatile u8 *bcsr_regs = NULL;

A 'static volatile u8*' type sounds very wrong. You use the variable
only in one function, so it should be local to that. There is normally
no reason to ever mark a variable volatile in driver code, instead
you want it to be '__iomem'. It should probably also be 'void *'
instead of 'u8 *', so it doesn't get dereferenced by accident.
Your references should be changed to use in_8/out_8 or readb/writeb,
depending on whether it's on-chip or PCI space.

> +	if ((np = of_find_compatible_node(np, "usb", "fsl-usb2-dr")) != NULL)
> +		port0_is_dr = 1;
> +	if ((np = of_find_compatible_node(np, "usb", "fsl-usb2-mph")) != NULL){
> +		if (port0_is_dr) {
> +			printk(KERN_WARNING
> +				"There is only one USB port on PB board! \n");
> +			return -1;
> +		} else if (!port0_is_dr)
> +			/* No usb port enabled */
> +			return -1;
> +	}

I'm not sure if scanning through the device tree to find mmio addresses
is still considered valid style. I would instead prefer you to provide a
of_platform_driver that attaches to the respective 'compatible' property
and does the register access in its probe() function in order to not
conflict with other drivers.

	Arnd <><