Re: [PATCH v3] gpio: driver for PrimeCell PL061 GPIO controller

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Tue, Jun 02, 2009 at 11:48:10PM +0300, Baruch Siach wrote:
> +static u32 (*pl061_pending_irq)(int irq);

Hmm, not sure this is entirely a good idea, especially when some
platforms may have more than one of these.  It'll work provided they
all point at the same function, but if they don't, it's bad news.

IRQs do have the ability to have chip specific data attached to them.
See set_irq_chip_data() and get_irq_chip_data().

> +static unsigned int pl061_irq_startup(unsigned irq)
> +{
> +	int ret;
> +
> +	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> +	if (ret < 0) {
> +		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> +				__func__, irq_to_gpio(irq), ret);
> +		return 0;
> +	}
> +
> +	gpio_direction_input(irq_to_gpio(irq));

I thought that it was not expected that claiming an interrupt would claim
a GPIO automatically - in other words, it's the responsibility of the
driver or platform itself to claim GPIOs for interrupts and ensure that
they're properly configured.

> +static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
> +{
> +	struct pl061_platform_data *pdata;
> +	struct pl061_gpio *chip;
> +	int ret, irq, i;
> +
> +	pdata = dev->dev.platform_data;
> +	if (pdata == NULL)
> +		return -ENODEV;

-EINVAL would be better.

> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL)
> +		return -ENOMEM;
> +
> +	if (request_mem_region(dev->res.start, SZ_4K, "pl061") == NULL) {

It would be better to keep SZ_* constants out of what are essentially
generic drivers - or we move them into some generic kernel header (but
I don't hold out that much hope of that happening.)

> diff --git a/include/linux/amba/pl061.h b/include/linux/amba/pl061.h
> new file mode 100644
> index 0000000..9e18fa3
> --- /dev/null
> +++ b/include/linux/amba/pl061.h
> @@ -0,0 +1,18 @@
> +/* platform data for the PL061 GPIO driver */
> +
> +#define GPIODIR 0x400
> +#define GPIOIS  0x404
> +#define GPIOIBE 0x408
> +#define GPIOIEV 0x40C
> +#define GPIOIE  0x410
> +#define GPIORIS 0x414
> +#define GPIOMIS 0x418
> +#define GPIOIC  0x41C

I think it would make sense to have the above register definitions in
the .c file.

Thanks.