Re: [PATCH 0/2] twl4030-usb patches

[Felipe Balbi <me@felipebalbi.com>]

On Tue, Sep 02, 2008 at 08:43:56AM -0700, David Brownell wrote:
> Snapshot of what's in my tree ... 

Great, a few comments below.

> # should have the base GPIO set up better; minimally
> # in a system header, ideally platform data ...

Could you describe this a bit more ? What do you mean by "base GPIO" ?
If you mean the first gpio number for passing to struct gpio_chip we
could use a platform_data if we make this one also a platform_driver,
right ?

> 
> # the whole twl driver should be "new style".

maybe only twl4030-core.c should hold the whole i2c new style thingy,
all the other drivers (usb, vibrators, keypad, rtc, etc) should/could
be a platform_driver.

>  static struct irq_chip twl4030_gpio_irq_chip = {
> -	.name	= "twl4030-gpio",
> +	.name	= "twl4030",

twl4030-core.c already uses this name. Wouldn't it be odd when cat
/proc/interrupts ? I'd like to see which are gpio interrupts and which
aren't

>  	down(&gpio_sem);
>  	if (gpio_usage_count & (0x1 << gpio))
>  		ret = -EBUSY;

how about also putting the brackets for the if(), maybe in a cleanup
patch before this one ?

>  	if ((gpio_usage_count & (0x1 << gpio)) == 0)
>  		ret = -EPERM;

missing brackets.

-- 
balbi