| Message ID | 1468994313-13538-7-git-send-email-andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2016 at 1:58 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > From: Joel Stanley <joel@jms.id.au> > > The Aspeed SoCs contain GPIOs grouped by letter, where each letter group > contains 8 pins. The GPIO letter groups are then banked in sets of four > in the register layout. > > The implementation exposes multiple banks through the one driver, and > requests and releases pins via the pinctrl subsystem. The hardware > supports generation of interrupts with per-pin triggers, and exposes this > capability through an irqchip and devicetree. > > A number of supported features are not yet implemented: Configuration of > interrupt direction (ARM or LPC), debouncing, and provides WDT reset > tolerance for output ports. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > Signed-off-by: Alistair Popple <alistair@popple.id.au> > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > arch/arm/mach-aspeed/Kconfig | 4 + > drivers/gpio/Kconfig | 8 +- > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-aspeed.c | 456 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 468 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpio/gpio-aspeed.c > > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig > index 25a0ae01429e..a52de9d3adfb 100644 > --- a/arch/arm/mach-aspeed/Kconfig > +++ b/arch/arm/mach-aspeed/Kconfig > @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED > select ASPEED_WATCHDOG > select MOXART_TIMER > select PINCTRL > + select GPIOLIB > + select GPIO_ASPEED > + select GPIO_SYSFS > + > help > Say Y here if you want to run your kernel on an ASpeed BMC SoC. > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 536112fd2466..2c21b5db09cd 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -137,6 +137,13 @@ config GPIO_ATH79 > Select this option to enable GPIO driver for > Atheros AR71XX/AR724X/AR913X SoC devices. > > +config GPIO_ASPEED > + bool "Aspeed GPIO support" Since this is a bool Kconfig... > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF > + select GENERIC_IRQ_CHIP > + help > + Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers. > + [...] > diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c > new file mode 100644 > index 000000000000..7885adc1332a > --- /dev/null > +++ b/drivers/gpio/gpio-aspeed.c > @@ -0,0 +1,456 @@ > +/* > + * Copyright 2015 IBM Corp > + * > + * Joel Stanley <joel@jms.id.au> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + */ > + > +#include <linux/module.h> ...can you please get rid of module.h and all the MODULE_<xyz> stuff and use the built in registration? Alternatively change it to a tristate if there is a valid use case for it to be modular. Thanks. Paul. -- > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/spinlock.h> > +#include <linux/platform_device.h> > +#include <linux/gpio/driver.h> > +#include <linux/pinctrl/consumer.h> > + > +struct aspeed_gpio { > + struct gpio_chip chip; > + spinlock_t lock; > + void __iomem *base; > + int irq; > + struct irq_chip irq_chip; > + struct irq_domain *irq_domain; > +}; > +
On Thu, 2016-07-21 at 16:12 -0400, Paul Gortmaker wrote: > On Wed, Jul 20, 2016 at 1:58 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > From: Joel Stanley <joel@jms.id.au> > > > > The Aspeed SoCs contain GPIOs grouped by letter, where each letter group > > contains 8 pins. The GPIO letter groups are then banked in sets of four > > in the register layout. > > > > The implementation exposes multiple banks through the one driver, and > > requests and releases pins via the pinctrl subsystem. The hardware > > supports generation of interrupts with per-pin triggers, and exposes this > > capability through an irqchip and devicetree. > > > > A number of supported features are not yet implemented: Configuration of > > interrupt direction (ARM or LPC), debouncing, and provides WDT reset > > tolerance for output ports. > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > arch/arm/mach-aspeed/Kconfig | 4 + > > drivers/gpio/Kconfig | 8 +- > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-aspeed.c | 456 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 468 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpio/gpio-aspeed.c > > > > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig > > index 25a0ae01429e..a52de9d3adfb 100644 > > --- a/arch/arm/mach-aspeed/Kconfig > > +++ b/arch/arm/mach-aspeed/Kconfig > > @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED > > select ASPEED_WATCHDOG > > select MOXART_TIMER > > select PINCTRL > > + select GPIOLIB > > + select GPIO_ASPEED > > + select GPIO_SYSFS > > + > > help > > Say Y here if you want to run your kernel on an ASpeed BMC SoC. > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 536112fd2466..2c21b5db09cd 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -137,6 +137,13 @@ config GPIO_ATH79 > > Select this option to enable GPIO driver for > > Atheros AR71XX/AR724X/AR913X SoC devices. > > > > +config GPIO_ASPEED > > + bool "Aspeed GPIO support" > Since this is a bool Kconfig... > > > > > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF > > + select GENERIC_IRQ_CHIP > > + help > > + Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers. > > + > [...] > > > diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c > > > > new file mode 100644 > > index 000000000000..7885adc1332a > > --- /dev/null > > +++ b/drivers/gpio/gpio-aspeed.c > > @@ -0,0 +1,456 @@ > > +/* > > + * Copyright 2015 IBM Corp > > + * > > + * Joel Stanley <joel@jms.id.au> > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version > > + * 2 of the License, or (at your option) any later version. > > + */ > > + > > +#include > ...can you please get rid of module.h and all the MODULE_ > stuff and use the built in registration? Alternatively change it to > a tristate if there is a valid use case for it to be modular. > I'll change it to tristate; I don't see a reason to require it be built in. Cheers, Andrew
On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig > index 25a0ae01429e..a52de9d3adfb 100644 > --- a/arch/arm/mach-aspeed/Kconfig > +++ b/arch/arm/mach-aspeed/Kconfig > @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED > select ASPEED_WATCHDOG > select MOXART_TIMER > select PINCTRL > + select GPIOLIB > + select GPIO_ASPEED Again this needs to be a separate patch to ARM SoC. > + select GPIO_SYSFS NAK over my dead body. I strongly discourage the use of the GPIO sysfs, use the new character device, see toos/gpio/* for examples. > +config GPIO_ASPEED > + bool "Aspeed GPIO support" > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF > + select GENERIC_IRQ_CHIP Why are you using GENERIC_IRQ_CHIP but not GPIOLIB_IRQCHIP? Well I guess I may find out by reading the code... > + help > + Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers. > + > config GPIO_BCM_KONA > bool "Broadcom Kona GPIO" > depends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST) > @@ -1072,7 +1079,6 @@ config GPIO_SODAVILLE > select GENERIC_IRQ_CHIP > help > Say Y here to support Intel Sodaville GPIO. > - > endmenu Drop this unrelated whitespace change. > +#define GPIO_BANK(x) ((x) >> 5) > +#define GPIO_OFFSET(x) ((x) & 0x1f) > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x)) Clever, maybe needs some comments on how they work? Or is it obvious from context? > +#define GPIO_DATA 0x00 > +#define GPIO_DIR 0x04 > + > +#define GPIO_IRQ_ENABLE 0x00 > +#define GPIO_IRQ_TYPE0 0x04 > +#define GPIO_IRQ_TYPE1 0x08 > +#define GPIO_IRQ_TYPE2 0x0c > +#define GPIO_IRQ_STATUS 0x10 > +static inline struct aspeed_gpio *to_aspeed_gpio(struct gpio_chip *chip) > +{ > + return container_of(chip, struct aspeed_gpio, chip); > +} NAK rewrite your code to use devm_gpiochip_add_data() and then use gpiochip_get_data() inline in every function that needs to get the state container from the gpiochip. Read the code in the upstream kernel for *any* driver because I think I changed this virtually everywhere. (...) > +static void __aspeed_gpio_irq_set_mask(struct irq_data *d, bool set) Why __underscoring. It is just confusing, drop the underscores. The function does set the mask. > +{ > + const struct aspeed_gpio_bank *bank; > + struct aspeed_gpio *gpio; > + unsigned long flags; > + u32 reg, bit; > + void *addr; > + int rc; > + > + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); > + if (rc) > + return; > + > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE); > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + reg = ioread32(addr); > + if (set) > + reg |= bit; > + else > + reg &= bit; > + iowrite32(reg, addr); Hm, if this was done with regmap it would be regmap_update_bits() simply ... maybe you should just throw a 32bit MMIO regmap over the registers? (Not required, just an idea to simplify stuff...) > +static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type) > +{ > + u32 type0, type1, type2, bit, reg; > + const struct aspeed_gpio_bank *bank; > + irq_flow_handler_t handler; > + struct aspeed_gpio *gpio; > + unsigned long flags; > + void *addr; > + int rc; > + > + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); > + if (rc) > + return -EINVAL; > + > + type0 = type1 = type2 = 0; Assign zero when declaring instead: u32 type0 = 0; u32 type1 = 0; ... > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + type2 |= bit; > + case IRQ_TYPE_EDGE_RISING: > + type0 |= bit; > + case IRQ_TYPE_EDGE_FALLING: > + handler = handle_edge_irq; > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + type0 |= bit; > + case IRQ_TYPE_LEVEL_LOW: > + type1 |= bit; > + handler = handle_level_irq; > + break; > + default: > + return -EINVAL; > + } Very nice, this looks exactly as it should, handling the different IRQs very nicely. > + spin_lock_irqsave(&gpio->lock, flags); > + > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type0; > + iowrite32(reg, addr); > + > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type1; > + iowrite32(reg, addr); > + > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type2; > + iowrite32(reg, addr); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + irq_set_handler_locked(d, handler); > + > + return 0; > +} Overall very nice .set_type(). > +static void aspeed_gpio_irq_handler(struct irq_desc *desc) > +{ > + struct aspeed_gpio *gpio = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned int i, p, girq; > + unsigned long reg; > + > + chained_irq_enter(chip, desc); > + > + for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) { > + const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i]; > + > + reg = ioread32(bank_irq_reg(gpio, bank, GPIO_IRQ_STATUS)); > + > + for_each_set_bit(p, ®, 32) { > + girq = irq_find_mapping(gpio->irq_domain, i * 32 + p); > + generic_handle_irq(girq); > + } > + > + } > + > + chained_irq_exit(chip, desc); > +} This looks so generic so I think you should be using GPIOLIB_IRQCHIP. > + > +static struct irq_chip aspeed_gpio_irqchip = { > + .name = "aspeed-gpio", > + .irq_ack = aspeed_gpio_irq_ack, > + .irq_mask = aspeed_gpio_irq_mask, > + .irq_unmask = aspeed_gpio_irq_unmask, > + .irq_set_type = aspeed_gpio_set_type, > +}; There is a missing .request/.release resources marking the lines as irq to the GPIO core. But if you switch to using GPIOLIB_IRQCHIP the core will handle all that for you. > +static int aspeed_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct aspeed_gpio *gpio = to_aspeed_gpio(chip); > + > + return irq_find_mapping(gpio->irq_domain, offset); > +} > + > +static void aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio, > + struct platform_device *pdev) > +{ > + int i, irq; > + > + /* request our upstream IRQ */ > + gpio->irq = platform_get_irq(pdev, 0); > + if (gpio->irq < 0) > + return; > + > + /* establish our irq domain to provide IRQs for each extended bank */ > + gpio->irq_domain = irq_domain_add_linear(pdev->dev.of_node, > + gpio->chip.ngpio, &irq_domain_simple_ops, NULL); > + if (!gpio->irq_domain) > + return; > + > + for (i = 0; i < gpio->chip.ngpio; i++) { > + irq = irq_create_mapping(gpio->irq_domain, i); > + irq_set_chip_data(irq, gpio); > + irq_set_chip_and_handler(irq, &aspeed_gpio_irqchip, > + handle_simple_irq); > + irq_set_probe(irq); > + } > + > + irq_set_chained_handler_and_data(gpio->irq, > + aspeed_gpio_irq_handler, gpio); > +} Also all this goes away with GPILIB_IRQCHIP, so use it. See e.g. drivers/gpio/gpio-pl061.c for an example of a similar driver doing this. > +static int __init aspeed_gpio_probe(struct platform_device *pdev) > +{ > + struct aspeed_gpio *gpio; > + struct resource *res; > + int rc; > + > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > + if (!gpio) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENXIO; > + > + gpio->base = devm_ioremap_resource(&pdev->dev, res); > + if (!gpio->base) > + return -ENOMEM; > + > + spin_lock_init(&gpio->lock); > + > + gpio->chip.ngpio = ARRAY_SIZE(aspeed_gpio_banks) * 32; > + > + gpio->chip.parent = &pdev->dev; > + gpio->chip.direction_input = aspeed_gpio_dir_in; > + gpio->chip.direction_output = aspeed_gpio_dir_out; Please add gpio->chip.get_direction() to complete the picture. > + gpio->chip.request = aspeed_gpio_request; > + gpio->chip.free = aspeed_gpio_free; > + gpio->chip.get = aspeed_gpio_get; > + gpio->chip.set = aspeed_gpio_set; > + gpio->chip.to_irq = aspeed_gpio_to_irq; .to_irq goes away with GPILIB_IRQCHIP. > + gpio->chip.label = dev_name(&pdev->dev); > + gpio->chip.base = -1; > + > + platform_set_drvdata(pdev, gpio); > + > + rc = gpiochip_add(&gpio->chip); > + if (rc < 0) > + return rc; Use devm_gpiochip_add_data(), then gpiochip_irqchip_add(). > +static int aspeed_gpio_remove(struct platform_device *pdev) > +{ > + struct aspeed_gpio *gpio = platform_get_drvdata(pdev); > + > + gpiochip_remove(&gpio->chip); > + return 0; > +} And then this is not even needed. (devm*) Yours, Linus Walleij
On Thu, 2016-08-11 at 11:20 +0200, Linus Walleij wrote: > On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@aj.id.au> > wrote: > > > > > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach- > > aspeed/Kconfig > > index 25a0ae01429e..a52de9d3adfb 100644 > > --- a/arch/arm/mach-aspeed/Kconfig > > +++ b/arch/arm/mach-aspeed/Kconfig > > @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED > > select ASPEED_WATCHDOG > > select MOXART_TIMER > > select PINCTRL > > + select GPIOLIB > > + select GPIO_ASPEED > Again this needs to be a separate patch to ARM SoC. > > > > > + select GPIO_SYSFS > NAK over my dead body. I strongly discourage the use of the > GPIO sysfs, use the new character device, see > toos/gpio/* for examples. > > > > > +config GPIO_ASPEED > > + bool "Aspeed GPIO support" > > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF > > + select GENERIC_IRQ_CHIP > Why are you using GENERIC_IRQ_CHIP but not > GPIOLIB_IRQCHIP? Well I guess I may find out by > reading the code... > > > > > + help > > + Say Y here to support Aspeed AST2400 and AST2500 GPIO > > controllers. > > + > > config GPIO_BCM_KONA > > bool "Broadcom Kona GPIO" > > depends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST) > > @@ -1072,7 +1079,6 @@ config GPIO_SODAVILLE > > select GENERIC_IRQ_CHIP > > help > > Say Y here to support Intel Sodaville GPIO. > > - > > endmenu > Drop this unrelated whitespace change. > > > > > +#define GPIO_BANK(x) ((x) >> 5) > > +#define GPIO_OFFSET(x) ((x) & 0x1f) > > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x)) > Clever, maybe needs some comments on how they work? > Or is it obvious from context? > > > > > +#define GPIO_DATA 0x00 > > +#define GPIO_DIR 0x04 > > + > > +#define GPIO_IRQ_ENABLE 0x00 > > +#define GPIO_IRQ_TYPE0 0x04 > > +#define GPIO_IRQ_TYPE1 0x08 > > +#define GPIO_IRQ_TYPE2 0x0c > > +#define GPIO_IRQ_STATUS 0x10 > > > > +static inline struct aspeed_gpio *to_aspeed_gpio(struct gpio_chip > > *chip) > > +{ > > + return container_of(chip, struct aspeed_gpio, chip); > > +} > NAK rewrite your code to use devm_gpiochip_add_data() and > then use gpiochip_get_data() inline in every function that > needs to get the state container from the gpiochip. Read > the code in the upstream kernel for *any* driver because I > think I changed this virtually everywhere. > > (...) > > > > +static void __aspeed_gpio_irq_set_mask(struct irq_data *d, bool > > set) > Why __underscoring. It is just confusing, drop the underscores. > The function does set the mask. > > > > > +{ > > + const struct aspeed_gpio_bank *bank; > > + struct aspeed_gpio *gpio; > > + unsigned long flags; > > + u32 reg, bit; > > + void *addr; > > + int rc; > > + > > + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); > > + if (rc) > > + return; > > + > > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE); > > + > > + spin_lock_irqsave(&gpio->lock, flags); > > + > > + reg = ioread32(addr); > > + if (set) > > + reg |= bit; > > + else > > + reg &= bit; > > + iowrite32(reg, addr); > Hm, if this was done with regmap it would be regmap_update_bits() > simply ... maybe you should just throw a 32bit MMIO regmap over > the registers? (Not required, just an idea to simplify stuff...) > > > > > +static int aspeed_gpio_set_type(struct irq_data *d, unsigned int > > type) > > +{ > > + u32 type0, type1, type2, bit, reg; > > + const struct aspeed_gpio_bank *bank; > > + irq_flow_handler_t handler; > > + struct aspeed_gpio *gpio; > > + unsigned long flags; > > + void *addr; > > + int rc; > > + > > + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); > > + if (rc) > > + return -EINVAL; > > + > > + type0 = type1 = type2 = 0; > Assign zero when declaring instead: > > u32 type0 = 0; > u32 type1 = 0; > ... > > > > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_EDGE_BOTH: > > + type2 |= bit; > > + case IRQ_TYPE_EDGE_RISING: > > + type0 |= bit; > > + case IRQ_TYPE_EDGE_FALLING: > > + handler = handle_edge_irq; > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + type0 |= bit; > > + case IRQ_TYPE_LEVEL_LOW: > > + type1 |= bit; > > + handler = handle_level_irq; > > + break; > > + default: > > + return -EINVAL; > > + } > Very nice, this looks exactly as it should, handling the different > IRQs very nicely. > > > > > + spin_lock_irqsave(&gpio->lock, flags); > > + > > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0); > > + reg = ioread32(addr); > > + reg = (reg & ~bit) | type0; > > + iowrite32(reg, addr); > > + > > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1); > > + reg = ioread32(addr); > > + reg = (reg & ~bit) | type1; > > + iowrite32(reg, addr); > > + > > + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2); > > + reg = ioread32(addr); > > + reg = (reg & ~bit) | type2; > > + iowrite32(reg, addr); > > + > > + spin_unlock_irqrestore(&gpio->lock, flags); > > + > > + irq_set_handler_locked(d, handler); > > + > > + return 0; > > +} > Overall very nice .set_type(). > > > > > +static void aspeed_gpio_irq_handler(struct irq_desc *desc) > > +{ > > + struct aspeed_gpio *gpio = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned int i, p, girq; > > + unsigned long reg; > > + > > + chained_irq_enter(chip, desc); > > + > > + for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) { > > + const struct aspeed_gpio_bank *bank = > > &aspeed_gpio_banks[i]; > > + > > + reg = ioread32(bank_irq_reg(gpio, bank, > > GPIO_IRQ_STATUS)); > > + > > + for_each_set_bit(p, ®, 32) { > > + girq = irq_find_mapping(gpio->irq_domain, i > > * 32 + p); > > + generic_handle_irq(girq); > > + } > > + > > + } > > + > > + chained_irq_exit(chip, desc); > > +} > This looks so generic so I think you should be using > GPIOLIB_IRQCHIP. > > > > > + > > +static struct irq_chip aspeed_gpio_irqchip = { > > + .name = "aspeed-gpio", > > + .irq_ack = aspeed_gpio_irq_ack, > > + .irq_mask = aspeed_gpio_irq_mask, > > + .irq_unmask = aspeed_gpio_irq_unmask, > > + .irq_set_type = aspeed_gpio_set_type, > > +}; > There is a missing .request/.release resources marking the lines > as irq to the GPIO core. But if you switch to using GPIOLIB_IRQCHIP > the core will handle all that for you. > > > > > +static int aspeed_gpio_to_irq(struct gpio_chip *chip, unsigned int > > offset) > > +{ > > + struct aspeed_gpio *gpio = to_aspeed_gpio(chip); > > + > > + return irq_find_mapping(gpio->irq_domain, offset); > > +} > > + > > +static void aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio, > > + struct platform_device *pdev) > > +{ > > + int i, irq; > > + > > + /* request our upstream IRQ */ > > + gpio->irq = platform_get_irq(pdev, 0); > > + if (gpio->irq < 0) > > + return; > > + > > + /* establish our irq domain to provide IRQs for each > > extended bank */ > > + gpio->irq_domain = irq_domain_add_linear(pdev->dev.of_node, > > + gpio->chip.ngpio, &irq_domain_simple_ops, > > NULL); > > + if (!gpio->irq_domain) > > + return; > > + > > + for (i = 0; i < gpio->chip.ngpio; i++) { > > + irq = irq_create_mapping(gpio->irq_domain, i); > > + irq_set_chip_data(irq, gpio); > > + irq_set_chip_and_handler(irq, &aspeed_gpio_irqchip, > > + handle_simple_irq); > > + irq_set_probe(irq); > > + } > > + > > + irq_set_chained_handler_and_data(gpio->irq, > > + aspeed_gpio_irq_handler, gpio); > > +} > Also all this goes away with GPILIB_IRQCHIP, so use it. > > See e.g. drivers/gpio/gpio-pl061.c for an example of a similar > driver doing this. > > > > > +static int __init aspeed_gpio_probe(struct platform_device *pdev) > > +{ > > + struct aspeed_gpio *gpio; > > + struct resource *res; > > + int rc; > > + > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > + if (!gpio) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -ENXIO; > > + > > + gpio->base = devm_ioremap_resource(&pdev->dev, res); > > + if (!gpio->base) > > + return -ENOMEM; > > + > > + spin_lock_init(&gpio->lock); > > + > > + gpio->chip.ngpio = ARRAY_SIZE(aspeed_gpio_banks) * 32; > > + > > + gpio->chip.parent = &pdev->dev; > > + gpio->chip.direction_input = aspeed_gpio_dir_in; > > + gpio->chip.direction_output = aspeed_gpio_dir_out; > Please add gpio->chip.get_direction() to complete the picture. > > > > > + gpio->chip.request = aspeed_gpio_request; > > + gpio->chip.free = aspeed_gpio_free; > > + gpio->chip.get = aspeed_gpio_get; > > + gpio->chip.set = aspeed_gpio_set; > > + gpio->chip.to_irq = aspeed_gpio_to_irq; > .to_irq goes away with GPILIB_IRQCHIP. > > > > > + gpio->chip.label = dev_name(&pdev->dev); > > + gpio->chip.base = -1; > > + > > + platform_set_drvdata(pdev, gpio); > > + > > + rc = gpiochip_add(&gpio->chip); > > + if (rc < 0) > > + return rc; > Use devm_gpiochip_add_data(), then gpiochip_irqchip_add(). > > > > > +static int aspeed_gpio_remove(struct platform_device *pdev) > > +{ > > + struct aspeed_gpio *gpio = platform_get_drvdata(pdev); > > + > > + gpiochip_remove(&gpio->chip); > > + return 0; > > +} > And then this is not even needed. (devm*) > > Yours, > Linus Walleij Thanks, I will clean up the issues. I brought the patch into the series because I figured that's what made sense. I should have looked at it a bit closer. Cheers, Andrew
diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig index 25a0ae01429e..a52de9d3adfb 100644 --- a/arch/arm/mach-aspeed/Kconfig +++ b/arch/arm/mach-aspeed/Kconfig @@ -6,6 +6,10 @@ menuconfig ARCH_ASPEED select ASPEED_WATCHDOG select MOXART_TIMER select PINCTRL + select GPIOLIB + select GPIO_ASPEED + select GPIO_SYSFS + help Say Y here if you want to run your kernel on an ASpeed BMC SoC. diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 536112fd2466..2c21b5db09cd 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -137,6 +137,13 @@ config GPIO_ATH79 Select this option to enable GPIO driver for Atheros AR71XX/AR724X/AR913X SoC devices. +config GPIO_ASPEED + bool "Aspeed GPIO support" + depends on (ARCH_ASPEED || COMPILE_TEST) && OF + select GENERIC_IRQ_CHIP + help + Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers. + config GPIO_BCM_KONA bool "Broadcom Kona GPIO" depends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST) @@ -1072,7 +1079,6 @@ config GPIO_SODAVILLE select GENERIC_IRQ_CHIP help Say Y here to support Intel Sodaville GPIO. - endmenu menu "SPI GPIO expanders" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 991598ea3fba..a2b18beb8af5 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -28,6 +28,7 @@ obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o obj-$(CONFIG_GPIO_AMDPT) += gpio-amdpt.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o +obj-$(CONFIG_GPIO_ASPEED) += gpio-aspeed.o obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c new file mode 100644 index 000000000000..7885adc1332a --- /dev/null +++ b/drivers/gpio/gpio-aspeed.c @@ -0,0 +1,456 @@ +/* + * Copyright 2015 IBM Corp. + * + * Joel Stanley <joel@jms.id.au> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/spinlock.h> +#include <linux/platform_device.h> +#include <linux/gpio/driver.h> +#include <linux/pinctrl/consumer.h> + +struct aspeed_gpio { + struct gpio_chip chip; + spinlock_t lock; + void __iomem *base; + int irq; + struct irq_chip irq_chip; + struct irq_domain *irq_domain; +}; + +struct aspeed_gpio_bank { + uint16_t val_regs; + uint16_t irq_regs; + const char names[4]; +}; + +static const struct aspeed_gpio_bank aspeed_gpio_banks[] = { + { + .val_regs = 0x0000, + .irq_regs = 0x0008, + .names = { 'A', 'B', 'C', 'D' }, + }, + { + .val_regs = 0x0020, + .irq_regs = 0x0028, + .names = { 'E', 'F', 'G', 'H' }, + }, + { + .val_regs = 0x0070, + .irq_regs = 0x0098, + .names = { 'I', 'J', 'K', 'L' }, + }, + { + .val_regs = 0x0078, + .irq_regs = 0x00e8, + .names = { 'M', 'N', 'O', 'P' }, + }, + { + .val_regs = 0x0080, + .irq_regs = 0x0118, + .names = { 'Q', 'R', 'S', 'T' }, + }, + { + .val_regs = 0x0088, + .irq_regs = 0x0148, + .names = { 'U', 'V', 'W', 'X' }, + }, + /* + * A bank exists for { 'Y', 'Z', "AA", "AB" }, but is not implemented. + * Only half of GPIOs Y support interrupt configuration, and none of Z, + * AA or AB do as they are output only. + */ +}; + +#define GPIO_BANK(x) ((x) >> 5) +#define GPIO_OFFSET(x) ((x) & 0x1f) +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x)) + +#define GPIO_DATA 0x00 +#define GPIO_DIR 0x04 + +#define GPIO_IRQ_ENABLE 0x00 +#define GPIO_IRQ_TYPE0 0x04 +#define GPIO_IRQ_TYPE1 0x08 +#define GPIO_IRQ_TYPE2 0x0c +#define GPIO_IRQ_STATUS 0x10 + +static inline struct aspeed_gpio *to_aspeed_gpio(struct gpio_chip *chip) +{ + return container_of(chip, struct aspeed_gpio, chip); +} + +static const struct aspeed_gpio_bank *to_bank(unsigned int offset) +{ + unsigned int bank = GPIO_BANK(offset); + + WARN_ON(bank > ARRAY_SIZE(aspeed_gpio_banks)); + return &aspeed_gpio_banks[bank]; +} + +static void *bank_val_reg(struct aspeed_gpio *gpio, + const struct aspeed_gpio_bank *bank, + unsigned int reg) +{ + return gpio->base + bank->val_regs + reg; +} + +static void *bank_irq_reg(struct aspeed_gpio *gpio, + const struct aspeed_gpio_bank *bank, + unsigned int reg) +{ + return gpio->base + bank->irq_regs + reg; +} + +static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct aspeed_gpio *gpio = to_aspeed_gpio(gc); + const struct aspeed_gpio_bank *bank = to_bank(offset); + + return !!(ioread32(bank_val_reg(gpio, bank, GPIO_DATA)) + & GPIO_BIT(offset)); +} + +static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset, + int val) +{ + struct aspeed_gpio *gpio = to_aspeed_gpio(gc); + const struct aspeed_gpio_bank *bank = to_bank(offset); + unsigned long flags; + u32 reg; + + spin_lock_irqsave(&gpio->lock, flags); + + reg = ioread32(bank_val_reg(gpio, bank, GPIO_DATA)); + if (val) + reg |= GPIO_BIT(offset); + else + reg &= ~GPIO_BIT(offset); + + iowrite32(reg, bank_val_reg(gpio, bank, GPIO_DATA)); + + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static int aspeed_gpio_dir_in(struct gpio_chip *gc, unsigned int offset) +{ + struct aspeed_gpio *gpio = to_aspeed_gpio(gc); + const struct aspeed_gpio_bank *bank = to_bank(offset); + unsigned long flags; + u32 reg; + + spin_lock_irqsave(&gpio->lock, flags); + + reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)); + iowrite32(reg & ~GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR)); + + spin_unlock_irqrestore(&gpio->lock, flags); + + return 0; +} + +static int aspeed_gpio_dir_out(struct gpio_chip *gc, + unsigned int offset, int val) +{ + struct aspeed_gpio *gpio = to_aspeed_gpio(gc); + const struct aspeed_gpio_bank *bank = to_bank(offset); + unsigned long flags; + u32 reg; + + spin_lock_irqsave(&gpio->lock, flags); + + reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)); + iowrite32(reg | GPIO_BIT(offset), bank_val_reg(gpio, bank, GPIO_DIR)); + + spin_unlock_irqrestore(&gpio->lock, flags); + + return 0; +} + +static inline int irqd_to_aspeed_gpio_data(struct irq_data *d, + struct aspeed_gpio **gpio, + const struct aspeed_gpio_bank **bank, + u32 *bit) +{ + int offset; + + offset = irqd_to_hwirq(d); + + *gpio = irq_data_get_irq_chip_data(d); + *bank = to_bank(offset); + *bit = GPIO_BIT(offset); + + return 0; +} + +static void aspeed_gpio_irq_ack(struct irq_data *d) +{ + const struct aspeed_gpio_bank *bank; + struct aspeed_gpio *gpio; + unsigned long flags; + void *status_addr; + u32 bit; + int rc; + + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); + if (rc) + return; + + status_addr = bank_irq_reg(gpio, bank, GPIO_IRQ_STATUS); + + spin_lock_irqsave(&gpio->lock, flags); + iowrite32(bit, status_addr); + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static void __aspeed_gpio_irq_set_mask(struct irq_data *d, bool set) +{ + const struct aspeed_gpio_bank *bank; + struct aspeed_gpio *gpio; + unsigned long flags; + u32 reg, bit; + void *addr; + int rc; + + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); + if (rc) + return; + + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_ENABLE); + + spin_lock_irqsave(&gpio->lock, flags); + + reg = ioread32(addr); + if (set) + reg |= bit; + else + reg &= bit; + iowrite32(reg, addr); + + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static void aspeed_gpio_irq_mask(struct irq_data *d) +{ + __aspeed_gpio_irq_set_mask(d, false); +} + +static void aspeed_gpio_irq_unmask(struct irq_data *d) +{ + __aspeed_gpio_irq_set_mask(d, true); +} + +static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type) +{ + u32 type0, type1, type2, bit, reg; + const struct aspeed_gpio_bank *bank; + irq_flow_handler_t handler; + struct aspeed_gpio *gpio; + unsigned long flags; + void *addr; + int rc; + + rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit); + if (rc) + return -EINVAL; + + type0 = type1 = type2 = 0; + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_BOTH: + type2 |= bit; + case IRQ_TYPE_EDGE_RISING: + type0 |= bit; + case IRQ_TYPE_EDGE_FALLING: + handler = handle_edge_irq; + break; + case IRQ_TYPE_LEVEL_HIGH: + type0 |= bit; + case IRQ_TYPE_LEVEL_LOW: + type1 |= bit; + handler = handle_level_irq; + break; + default: + return -EINVAL; + } + + spin_lock_irqsave(&gpio->lock, flags); + + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE0); + reg = ioread32(addr); + reg = (reg & ~bit) | type0; + iowrite32(reg, addr); + + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE1); + reg = ioread32(addr); + reg = (reg & ~bit) | type1; + iowrite32(reg, addr); + + addr = bank_irq_reg(gpio, bank, GPIO_IRQ_TYPE2); + reg = ioread32(addr); + reg = (reg & ~bit) | type2; + iowrite32(reg, addr); + + spin_unlock_irqrestore(&gpio->lock, flags); + + irq_set_handler_locked(d, handler); + + return 0; +} + +static void aspeed_gpio_irq_handler(struct irq_desc *desc) +{ + struct aspeed_gpio *gpio = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned int i, p, girq; + unsigned long reg; + + chained_irq_enter(chip, desc); + + for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) { + const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i]; + + reg = ioread32(bank_irq_reg(gpio, bank, GPIO_IRQ_STATUS)); + + for_each_set_bit(p, ®, 32) { + girq = irq_find_mapping(gpio->irq_domain, i * 32 + p); + generic_handle_irq(girq); + } + + } + + chained_irq_exit(chip, desc); +} + +static struct irq_chip aspeed_gpio_irqchip = { + .name = "aspeed-gpio", + .irq_ack = aspeed_gpio_irq_ack, + .irq_mask = aspeed_gpio_irq_mask, + .irq_unmask = aspeed_gpio_irq_unmask, + .irq_set_type = aspeed_gpio_set_type, +}; + +static int aspeed_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct aspeed_gpio *gpio = to_aspeed_gpio(chip); + + return irq_find_mapping(gpio->irq_domain, offset); +} + +static void aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio, + struct platform_device *pdev) +{ + int i, irq; + + /* request our upstream IRQ */ + gpio->irq = platform_get_irq(pdev, 0); + if (gpio->irq < 0) + return; + + /* establish our irq domain to provide IRQs for each extended bank */ + gpio->irq_domain = irq_domain_add_linear(pdev->dev.of_node, + gpio->chip.ngpio, &irq_domain_simple_ops, NULL); + if (!gpio->irq_domain) + return; + + for (i = 0; i < gpio->chip.ngpio; i++) { + irq = irq_create_mapping(gpio->irq_domain, i); + irq_set_chip_data(irq, gpio); + irq_set_chip_and_handler(irq, &aspeed_gpio_irqchip, + handle_simple_irq); + irq_set_probe(irq); + } + + irq_set_chained_handler_and_data(gpio->irq, + aspeed_gpio_irq_handler, gpio); +} + +static int aspeed_gpio_request(struct gpio_chip *chip, unsigned int offset) +{ + return pinctrl_request_gpio(chip->base + offset); +} + +static void aspeed_gpio_free(struct gpio_chip *chip, unsigned int offset) +{ + pinctrl_free_gpio(chip->base + offset); +} + +static int __init aspeed_gpio_probe(struct platform_device *pdev) +{ + struct aspeed_gpio *gpio; + struct resource *res; + int rc; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENXIO; + + gpio->base = devm_ioremap_resource(&pdev->dev, res); + if (!gpio->base) + return -ENOMEM; + + spin_lock_init(&gpio->lock); + + gpio->chip.ngpio = ARRAY_SIZE(aspeed_gpio_banks) * 32; + + gpio->chip.parent = &pdev->dev; + gpio->chip.direction_input = aspeed_gpio_dir_in; + gpio->chip.direction_output = aspeed_gpio_dir_out; + gpio->chip.request = aspeed_gpio_request; + gpio->chip.free = aspeed_gpio_free; + gpio->chip.get = aspeed_gpio_get; + gpio->chip.set = aspeed_gpio_set; + gpio->chip.to_irq = aspeed_gpio_to_irq; + gpio->chip.label = dev_name(&pdev->dev); + gpio->chip.base = -1; + + platform_set_drvdata(pdev, gpio); + + rc = gpiochip_add(&gpio->chip); + if (rc < 0) + return rc; + + aspeed_gpio_setup_irqs(gpio, pdev); + + return 0; +} + +static int aspeed_gpio_remove(struct platform_device *pdev) +{ + struct aspeed_gpio *gpio = platform_get_drvdata(pdev); + + gpiochip_remove(&gpio->chip); + return 0; +} + +static const struct of_device_id aspeed_gpio_of_table[] = { + { .compatible = "aspeed,ast2400-gpio" }, + { .compatible = "aspeed,ast2500-gpio" }, + {} +}; +MODULE_DEVICE_TABLE(of, aspeed_gpio_of_table); + +static struct platform_driver aspeed_gpio_driver = { + .remove = aspeed_gpio_remove, + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = aspeed_gpio_of_table, + }, +}; + +module_platform_driver_probe(aspeed_gpio_driver, aspeed_gpio_probe); + +MODULE_DESCRIPTION("Aspeed GPIO Driver");