Re: [PATCH 1/1] gpio: add a driver for the Synopsys DesignWare APB GPIO block

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 11/07/2013 12:34 AM, Jamie Iles wrote:
> On Wed, Nov 06, 2013 at 04:49:42PM -0600, Alan Tull wrote:
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> new file mode 100644
>> index 0000000..7957dfd
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -0,0 +1,458 @@
>> +/*
>> + * Copyright (c) 2011 Jamie Iles
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * All enquiries to support@picochip.com
>> + */
>> +#include <linux/basic_mmio_gpio.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define GPIO_SWPORTA_DR		0x00
>> +#define GPIO_SWPORTA_DDR	0x04
>> +#define GPIO_SWPORTB_DR		0x0c
>> +#define GPIO_SWPORTB_DDR	0x10
>> +#define GPIO_SWPORTC_DR		0x18
>> +#define GPIO_SWPORTC_DDR	0x1c
>> +#define GPIO_SWPORTD_DR		0x24
>> +#define GPIO_SWPORTD_DDR	0x28
>> +#define GPIO_INTEN		0x30
>> +#define GPIO_INTMASK		0x34
>> +#define GPIO_INTTYPE_LEVEL	0x38
>> +#define GPIO_INT_POLARITY	0x3c
>> +#define GPIO_INTSTATUS		0x40
>> +#define GPIO_PORTA_EOI		0x4c
>> +#define GPIO_EXT_PORTA		0x50
>> +#define GPIO_EXT_PORTB		0x54
>> +#define GPIO_EXT_PORTC		0x58
>> +#define GPIO_EXT_PORTD		0x5c
>> +
>> +struct dwapb_gpio;
>> +
>> +struct dwapb_gpio_port {
>> +	struct bgpio_chip	bgc;
>> +	bool			is_registered;
>> +	struct dwapb_gpio	*gpio;
>> +};
>> +
>> +struct dwapb_gpio {
>> +	struct	device		*dev;
>> +	void __iomem		*regs;
>> +	struct dwapb_gpio_port	*ports;
>> +	unsigned int		nr_ports;
>> +	struct irq_domain	*domain;
>> +	int			hwirq;
>
> I'm not sure I fully understand what hwirq is in this context - is it
> the IRQ line from the Synopsys block to the system interrupt controller?
> If so I don't think this covers all configurations - the Picochip
> devices for instance have each GPIO in port A as an individual IRQ going
> to the VIC.

Usually, hwirq is the interrupt as seen from this very device, i.e. in
this case it is 0 up to 32 referencing the portA gpio line that
triggered an interrupt. In this drivers context 'hwirq' above is
actually the virtual irq number Linux made up to identify an interrupt
coming from this IP block.

Therefore, above should really be irq instead of hwirq - but with a
more detailed review, I think it can be removed completely.

> It looks here like hwirq is used for all of the interrupt registers so
> only one GPIO interrupt is supported?

Looking in _probe, currently only one upstream interrupt is requested.
Jamie is right, that you should grab all interrupts - but you can forget
their virtual number after registering the handler. There are helper
functions to get it back in irq handler.

>> +static struct platform_driver dwapb_gpio_driver = {
>> +	.driver		= {
>> +		.name	= "gpio-dwapb",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = of_match_ptr(dwapb_of_match),
>> +	},
>> +	.probe		= dwapb_gpio_probe,
>> +	.remove		= dwapb_gpio_remove,
>> +};
>> +
>> +static int __init dwapb_gpio_init(void)
>> +{
>> +	return platform_driver_register(&dwapb_gpio_driver);
>> +}
>> +postcore_initcall(dwapb_gpio_init);
>> +
>> +static void __exit dwapb_gpio_exit(void)
>> +{
>> +	platform_driver_unregister(&dwapb_gpio_driver);
>> +}
>> +module_exit(dwapb_gpio_exit);
>
> We can replace the registration and unregistration with
> module_platform_driver() now.

+1 (again)

Sebastian