| Message ID | 1411929195-23775-11-git-send-email-ryazanov.s.a@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > Atheros AR2315 SoC have a builtin GPIO controller, which could be > accessed via memory mapped registers. This patch adds new driver > for them. > > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Cc: linux-gpio@vger.kernel.org > +config GPIO_AR2315 > + bool "AR2315 SoC GPIO support" > + default y if SOC_AR2315 > + depends on SOC_AR2315 > + help > + Say yes here to enable GPIO support for Atheros AR2315+ SoCs. select GPIOLIB_IRQCHIP As far as I can see this driver should use the gpiolib irqchip helpers and create a cascading irqchip. The code uses some copy/pasting from earlier drivers which is not a good idea. Look at one of the drivers using GPIOLIB_IRQCHIP and be inspired, check e.g. gpio-pl061.c or gpio-zynq.c > +static u32 ar2315_gpio_intmask; > +static u32 ar2315_gpio_intval; > +static unsigned ar2315_gpio_irq_base; > +static void __iomem *ar2315_mem; No static locals. Allocate and use a state container, see Documentation/driver-model/design-patterns.txt > +static inline u32 ar2315_gpio_reg_read(unsigned reg) > +{ > + return __raw_readl(ar2315_mem + reg); > +} Use readl_relaxed() instead. > +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val) > +{ > + __raw_writel(val, ar2315_mem + reg); > +} Use writel_relaxed() instead. When you use the state container, you need to do a state dereference like that: mystate->base + reg So I don't think these inlines buy you anything. Just use readl/writel_relaxed directly in the code. > +static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val) > +{ > + ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) | val); > +} Too simple helper IMO, if you wanna do this in some organized fashion, use regmap. > +static void ar2315_gpio_int_setup(unsigned gpio, int trig) > +{ > + u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT); > + > + reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M); > + reg |= gpio | AR2315_GPIO_INT_TRIG(trig); > + ar2315_gpio_reg_write(AR2315_GPIO_INT, reg); > +} Trig? Isn't this supposed to be done in the .set_type() callback? > +static void ar2315_gpio_irq_unmask(struct irq_data *d) > +{ > + unsigned gpio = d->irq - ar2315_gpio_irq_base; This kind of weird calculations go away with irqdomain and that is in turn used by GPIOLIB_IRQCHIP. After that you will just use d->hwirq. And name the variable "offset" while you're at it. > + u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR); > + > + /* Enable interrupt with edge detection */ > + if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio)) > + return; > + > + ar2315_gpio_intmask |= (1 << gpio); #include <linux/bitops.h> ar2315_gpio_intmask |= BIT(gpio); > + ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE); > +} > +static struct irq_chip ar2315_gpio_irq_chip = { > + .name = DRIVER_NAME, > + .irq_unmask = ar2315_gpio_irq_unmask, > + .irq_mask = ar2315_gpio_irq_mask, > +}; So why is .set_type() not implemented and instead hard-coded into the unmask function? Please fix this. It will be called by the core eventually no matter what. > +static void ar2315_gpio_irq_init(unsigned irq) > +{ > + unsigned i; > + > + ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI); > + for (i = 0; i < AR2315_GPIO_NUM; i++) { > + unsigned _irq = ar2315_gpio_irq_base + i; > + > + irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip, > + handle_level_irq); > + } > + irq_set_chained_handler(irq, ar2315_gpio_irq_handler); > +} No, use the gpiolib irqchip helpers. > +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio) > +{ > + return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1; > +} Do this instead: return !!(ar2315_gpio_reg_read(AR2315_GPIO_DI) & BIT(offset)); > +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio) > +{ > + return ar2315_gpio_irq_base + gpio; > +} You do not implement this at all when using the gpiolib irqchip helpers. > +static int ar2315_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct resource *res; > + unsigned irq; > + int ret; > + > + if (ar2315_mem) > + return -EBUSY; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, DRIVER_NAME); > + if (!res) { > + dev_err(dev, "not found IRQ number\n"); > + return -ENXIO; > + } > + irq = res->start; Use irq = platform_get_irq_byname(pdev, DRIVER_NAME); if (irq < 0)... > + ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0); > + if (ret < 0) { > + dev_err(dev, "failed to allocate IRQ numbers\n"); > + return ret; > + } > + ar2315_gpio_irq_base = ret; > + > + ar2315_gpio_irq_init(irq); No, let GPIOLIB_IRQCHIP handle this. > +static int __init ar2315_gpio_init(void) > +{ > + return platform_driver_register(&ar2315_gpio_driver); > +} > +subsys_initcall(ar2315_gpio_init); Why are you using subsys_initcall()? This should not be necessary. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, thank you for so detailed review! I have one more generic question: could you merge driver without GPIOLIB_IRQCHIP usage? Currently no one driver for the AR231x SoCs uses irq_domain and I do not like to enable IRQ_DOMAIN just for one driver. I plan to convert drivers to make them irq_domain aware a bit later. Please, find my comments below. 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>: > On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> > wrote: > >> Atheros AR2315 SoC have a builtin GPIO controller, which could be >> accessed via memory mapped registers. This patch adds new driver >> for them. >> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Alexandre Courbot <gnurou@gmail.com> >> Cc: linux-gpio@vger.kernel.org > >> +config GPIO_AR2315 >> + bool "AR2315 SoC GPIO support" >> + default y if SOC_AR2315 >> + depends on SOC_AR2315 >> + help >> + Say yes here to enable GPIO support for Atheros AR2315+ SoCs. > > select GPIOLIB_IRQCHIP > > As far as I can see this driver should use the gpiolib irqchip helpers > and create a cascading irqchip. The code uses some copy/pasting > from earlier drivers which is not a good idea. Look at one of the drivers > using GPIOLIB_IRQCHIP and be inspired, check e.g. gpio-pl061.c > or gpio-zynq.c > Yes, this driver is pretty old, I updated it with newer API except IRQ_DOMAIN, which I left for stage 2. Thank you for your hint about reference realization. >> +static u32 ar2315_gpio_intmask; >> +static u32 ar2315_gpio_intval; >> +static unsigned ar2315_gpio_irq_base; >> +static void __iomem *ar2315_mem; > > No static locals. Allocate and use a state container, see > Documentation/driver-model/design-patterns.txt > Is that rule mandatory for drivers, which serve only one device? >> +static inline u32 ar2315_gpio_reg_read(unsigned reg) >> +{ >> + return __raw_readl(ar2315_mem + reg); >> +} > > Use readl_relaxed() instead. > readl_relaxed() converts the bit ordering and seems inapplicable in this case. >> +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val) >> +{ >> + __raw_writel(val, ar2315_mem + reg); >> +} > > Use writel_relaxed() instead. > > When you use the state container, you need to do a > state dereference like that: > > mystate->base + reg > > So I don't think these inlines buy you anything. Just use > readl/writel_relaxed directly in the code. > These helpers make code shorter and clearer. I can use macros if you do preferred. >> +static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val) >> +{ >> + ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) | >> val); >> +} > > Too simple helper IMO, if you wanna do this in some organized fashion, > use regmap. > >> +static void ar2315_gpio_int_setup(unsigned gpio, int trig) >> +{ >> + u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT); >> + >> + reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M); >> + reg |= gpio | AR2315_GPIO_INT_TRIG(trig); >> + ar2315_gpio_reg_write(AR2315_GPIO_INT, reg); >> +} > > Trig? Isn't this supposed to be done in the .set_type() > callback? > Yep. I tried to cheat kernel and made as if driver does not supports any other trigger types :) >> +static void ar2315_gpio_irq_unmask(struct irq_data *d) >> +{ >> + unsigned gpio = d->irq - ar2315_gpio_irq_base; > > This kind of weird calculations go away with irqdomain and that > is in turn used by GPIOLIB_IRQCHIP. > > After that you will just use d->hwirq. And name the variable > "offset" while you're at it. > >> + u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR); >> + >> + /* Enable interrupt with edge detection */ >> + if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio)) >> + return; >> + >> + ar2315_gpio_intmask |= (1 << gpio); > > #include <linux/bitops.h> > > ar2315_gpio_intmask |= BIT(gpio); > >> + ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE); >> +} > >> +static struct irq_chip ar2315_gpio_irq_chip = { >> + .name = DRIVER_NAME, >> + .irq_unmask = ar2315_gpio_irq_unmask, >> + .irq_mask = ar2315_gpio_irq_mask, >> +}; > > So why is .set_type() not implemented and instead hard-coded into > the unmask function? Please fix this. It will be called by the > core eventually no matter what. > The interrupt configuration is a bit complex. This controller could be configured to generate interrupts only for two lines at once. Or in other words: user could select any two lines to generate interrupt. >> +static void ar2315_gpio_irq_init(unsigned irq) >> +{ >> + unsigned i; >> + >> + ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI); >> + for (i = 0; i < AR2315_GPIO_NUM; i++) { >> + unsigned _irq = ar2315_gpio_irq_base + i; >> + >> + irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip, >> + handle_level_irq); >> + } >> + irq_set_chained_handler(irq, ar2315_gpio_irq_handler); >> +} > > No, use the gpiolib irqchip helpers. > >> +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio) >> +{ >> + return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1; >> +} > > Do this instead: > > return !!(ar2315_gpio_reg_read(AR2315_GPIO_DI) & BIT(offset)); > >> +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio) >> +{ >> + return ar2315_gpio_irq_base + gpio; >> +} > > You do not implement this at all when using the gpiolib irqchip helpers. > >> +static int ar2315_gpio_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + unsigned irq; >> + int ret; >> + >> + if (ar2315_mem) >> + return -EBUSY; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, >> DRIVER_NAME); >> + if (!res) { >> + dev_err(dev, "not found IRQ number\n"); >> + return -ENXIO; >> + } >> + irq = res->start; > > Use > > irq = platform_get_irq_byname(pdev, DRIVER_NAME); > if (irq < 0)... > >> + ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0); >> + if (ret < 0) { >> + dev_err(dev, "failed to allocate IRQ numbers\n"); >> + return ret; >> + } >> + ar2315_gpio_irq_base = ret; >> + >> + ar2315_gpio_irq_init(irq); > > No, let GPIOLIB_IRQCHIP handle this. > >> +static int __init ar2315_gpio_init(void) >> +{ >> + return platform_driver_register(&ar2315_gpio_driver); >> +} >> +subsys_initcall(ar2315_gpio_init); > > Why are you using subsys_initcall()? > > This should not be necessary. > I have users of GPIO in arch code, what called earlier than the devices initcall. > Yours, > Linus Walleij >
On Wed, Oct 15, 2014 at 1:12 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>: >> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> > I have one more generic question: could you merge driver without > GPIOLIB_IRQCHIP usage? No. > Currently no one driver for the AR231x SoCs > uses irq_domain and I do not like to enable IRQ_DOMAIN just for one > driver. I plan to convert drivers to make them irq_domain aware a bit > later. I don't believe any such promises. It's nothing personal, just I've been burned too many times by people promising to "fix later". >>> +static u32 ar2315_gpio_intmask; >>> +static u32 ar2315_gpio_intval; >>> +static unsigned ar2315_gpio_irq_base; >>> +static void __iomem *ar2315_mem; >> >> No static locals. Allocate and use a state container, see >> Documentation/driver-model/design-patterns.txt >> > Is that rule mandatory for drivers, which serve only one device? There is no central authority which decides what is mandatory or not. It is mandatory to get a driver past the GPIO maintainer. >>> +static inline u32 ar2315_gpio_reg_read(unsigned reg) >>> +{ >>> + return __raw_readl(ar2315_mem + reg); >>> +} >> >> Use readl_relaxed() instead. >> > readl_relaxed() converts the bit ordering and seems inapplicable in this case. It assumes the peripherals IO memory is little endian. If the IO memory for this device is little endian, please stay with [readl|writel]_relaxed so it looks familiar. Or is this machine really using big endian hardware registers? In that case I understand your comment... >> When you use the state container, you need to do a >> state dereference like that: >> >> mystate->base + reg >> >> So I don't think these inlines buy you anything. Just use >> readl/writel_relaxed directly in the code. >> > These helpers make code shorter and clearer. I can use macros if you > do preferred. No big deal. Keep it if you like it this way. >> So why is .set_type() not implemented and instead hard-coded into >> the unmask function? Please fix this. It will be called by the >> core eventually no matter what. >> > The interrupt configuration is a bit complex. This controller could be > configured to generate interrupts only for two lines at once. Or in > other words: user could select any two lines to generate interrupt. Oh well, better just handle it I guess... >>> +static int __init ar2315_gpio_init(void) >>> +{ >>> + return platform_driver_register(&ar2315_gpio_driver); >>> +} >>> +subsys_initcall(ar2315_gpio_init); >> >> Why are you using subsys_initcall()? >> >> This should not be necessary. >> > I have users of GPIO in arch code, what called earlier than the > devices initcall. OK? Why are there such users that early and what do they use the GPIOs for? Any reason they cannot be device_initcall()s? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-10-28 17:37 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > On Wed, Oct 15, 2014 at 1:12 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: >> 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>: >>> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> > >> I have one more generic question: could you merge driver without >> GPIOLIB_IRQCHIP usage? > > No. > Ok. >> Currently no one driver for the AR231x SoCs >> uses irq_domain and I do not like to enable IRQ_DOMAIN just for one >> driver. I plan to convert drivers to make them irq_domain aware a bit >> later. > > I don't believe any such promises. It's nothing personal, just I've > been burned too many times by people promising to "fix later". > Now I drop the driver from the series and return to the development a bit later, when finished the basic code for the MIPS architecture. In that case I will have a time to write the driver that does not require further fixes. >>>> +static u32 ar2315_gpio_intmask; >>>> +static u32 ar2315_gpio_intval; >>>> +static unsigned ar2315_gpio_irq_base; >>>> +static void __iomem *ar2315_mem; >>> >>> No static locals. Allocate and use a state container, see >>> Documentation/driver-model/design-patterns.txt >>> >> Is that rule mandatory for drivers, which serve only one device? > > There is no central authority which decides what is mandatory > or not. It is mandatory to get a driver past the GPIO maintainer. > Nice point :) >>>> +static inline u32 ar2315_gpio_reg_read(unsigned reg) >>>> +{ >>>> + return __raw_readl(ar2315_mem + reg); >>>> +} >>> >>> Use readl_relaxed() instead. >>> >> readl_relaxed() converts the bit ordering and seems inapplicable in this case. > > It assumes the peripherals IO memory is little endian. > > If the IO memory for this device is little endian, please stay with > [readl|writel]_relaxed so it looks familiar. > > Or is this machine really using big endian hardware registers? > In that case I understand your comment... > Yes, AR5312 and AR2315 SoCs are big endian machines with big endian registers. >>> When you use the state container, you need to do a >>> state dereference like that: >>> >>> mystate->base + reg >>> >>> So I don't think these inlines buy you anything. Just use >>> readl/writel_relaxed directly in the code. >>> >> These helpers make code shorter and clearer. I can use macros if you >> do preferred. > > No big deal. Keep it if you like it this way. > >>> So why is .set_type() not implemented and instead hard-coded into >>> the unmask function? Please fix this. It will be called by the >>> core eventually no matter what. >>> >> The interrupt configuration is a bit complex. This controller could be >> configured to generate interrupts only for two lines at once. Or in >> other words: user could select any two lines to generate interrupt. > > Oh well, better just handle it I guess... > Will do in v2. >>>> +static int __init ar2315_gpio_init(void) >>>> +{ >>>> + return platform_driver_register(&ar2315_gpio_driver); >>>> +} >>>> +subsys_initcall(ar2315_gpio_init); >>> >>> Why are you using subsys_initcall()? >>> >>> This should not be necessary. >>> >> I have users of GPIO in arch code, what called earlier than the >> devices initcall. > > OK? Why are there such users that early and what do they > use the GPIOs for? Any reason they cannot be device_initcall()s? > One GPIO line is used in reset handler to be able to reliably reset the chip. This is a workaround from vendor's reference design to eliminate a hw bug in the reset circuit of the AR2315 SoC. So I prefer to have GPIO controller in ready state as soon as possible. > Yours, > Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 7ce411b..0ceb4ba 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -112,6 +112,13 @@ config GPIO_MAX730X comment "Memory mapped GPIO drivers:" +config GPIO_AR2315 + bool "AR2315 SoC GPIO support" + default y if SOC_AR2315 + depends on SOC_AR2315 + help + Say yes here to enable GPIO support for Atheros AR2315+ SoCs. + config GPIO_AR5312 bool "AR5312 SoC GPIO support" default y if SOC_AR5312 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fae00f4..9a3a136 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o +obj-$(CONFIG_GPIO_AR2315) += gpio-ar2315.o obj-$(CONFIG_GPIO_AR5312) += gpio-ar5312.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o diff --git a/drivers/gpio/gpio-ar2315.c b/drivers/gpio/gpio-ar2315.c new file mode 100644 index 0000000..2a6caaf --- /dev/null +++ b/drivers/gpio/gpio-ar2315.c @@ -0,0 +1,232 @@ +/* + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + * + * Copyright (C) 2003 Atheros Communications, Inc., All Rights Reserved. + * Copyright (C) 2006 FON Technology, SL. + * Copyright (C) 2006 Imre Kaloz <kaloz@openwrt.org> + * Copyright (C) 2006 Felix Fietkau <nbd@openwrt.org> + * Copyright (C) 2012 Alexandros C. Couloumbis <alex@ozo.com> + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> +#include <linux/irq.h> + +#define DRIVER_NAME "ar2315-gpio" + +#define AR2315_GPIO_DI 0x0000 +#define AR2315_GPIO_DO 0x0008 +#define AR2315_GPIO_DIR 0x0010 +#define AR2315_GPIO_INT 0x0018 + +#define AR2315_GPIO_DIR_M(x) (1 << (x)) /* mask for i/o */ +#define AR2315_GPIO_DIR_O(x) (1 << (x)) /* output */ +#define AR2315_GPIO_DIR_I(x) (0) /* input */ + +#define AR2315_GPIO_INT_NUM_M 0x3F /* mask for GPIO num */ +#define AR2315_GPIO_INT_TRIG(x) ((x) << 6) /* interrupt trigger */ +#define AR2315_GPIO_INT_TRIG_M (0x3 << 6) /* mask for int trig */ + +#define AR2315_GPIO_INT_TRIG_OFF 0 /* Triggerring off */ +#define AR2315_GPIO_INT_TRIG_LOW 1 /* Low Level Triggered */ +#define AR2315_GPIO_INT_TRIG_HIGH 2 /* High Level Triggered */ +#define AR2315_GPIO_INT_TRIG_EDGE 3 /* Edge Triggered */ + +#define AR2315_GPIO_NUM 22 + +static u32 ar2315_gpio_intmask; +static u32 ar2315_gpio_intval; +static unsigned ar2315_gpio_irq_base; +static void __iomem *ar2315_mem; + +static inline u32 ar2315_gpio_reg_read(unsigned reg) +{ + return __raw_readl(ar2315_mem + reg); +} + +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val) +{ + __raw_writel(val, ar2315_mem + reg); +} + +static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val) +{ + ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) | val); +} + +static void ar2315_gpio_irq_handler(unsigned irq, struct irq_desc *desc) +{ + u32 pend; + int bit = -1; + + /* only do one gpio interrupt at a time */ + pend = ar2315_gpio_reg_read(AR2315_GPIO_DI); + pend ^= ar2315_gpio_intval; + pend &= ar2315_gpio_intmask; + + if (pend) { + bit = fls(pend) - 1; + pend &= ~(1 << bit); + ar2315_gpio_intval ^= (1 << bit); + } + + /* Enable interrupt with edge detection */ + if ((ar2315_gpio_reg_read(AR2315_GPIO_DIR) & AR2315_GPIO_DIR_M(bit)) != + AR2315_GPIO_DIR_I(bit)) + return; + + if (bit >= 0) + generic_handle_irq(ar2315_gpio_irq_base + bit); +} + +static void ar2315_gpio_int_setup(unsigned gpio, int trig) +{ + u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT); + + reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M); + reg |= gpio | AR2315_GPIO_INT_TRIG(trig); + ar2315_gpio_reg_write(AR2315_GPIO_INT, reg); +} + +static void ar2315_gpio_irq_unmask(struct irq_data *d) +{ + unsigned gpio = d->irq - ar2315_gpio_irq_base; + u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR); + + /* Enable interrupt with edge detection */ + if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio)) + return; + + ar2315_gpio_intmask |= (1 << gpio); + ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE); +} + +static void ar2315_gpio_irq_mask(struct irq_data *d) +{ + unsigned gpio = d->irq - ar2315_gpio_irq_base; + + /* Disable interrupt */ + ar2315_gpio_intmask &= ~(1 << gpio); + ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_OFF); +} + +static struct irq_chip ar2315_gpio_irq_chip = { + .name = DRIVER_NAME, + .irq_unmask = ar2315_gpio_irq_unmask, + .irq_mask = ar2315_gpio_irq_mask, +}; + +static void ar2315_gpio_irq_init(unsigned irq) +{ + unsigned i; + + ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI); + for (i = 0; i < AR2315_GPIO_NUM; i++) { + unsigned _irq = ar2315_gpio_irq_base + i; + + irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip, + handle_level_irq); + } + irq_set_chained_handler(irq, ar2315_gpio_irq_handler); +} + +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio) +{ + return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1; +} + +static void ar2315_gpio_set_val(struct gpio_chip *chip, unsigned gpio, int val) +{ + u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_DO); + + reg = val ? reg | (1 << gpio) : reg & ~(1 << gpio); + ar2315_gpio_reg_write(AR2315_GPIO_DO, reg); +} + +static int ar2315_gpio_dir_in(struct gpio_chip *chip, unsigned gpio) +{ + ar2315_gpio_reg_mask(AR2315_GPIO_DIR, 1 << gpio, 0); + return 0; +} + +static int ar2315_gpio_dir_out(struct gpio_chip *chip, unsigned gpio, int val) +{ + ar2315_gpio_reg_mask(AR2315_GPIO_DIR, 0, 1 << gpio); + ar2315_gpio_set_val(chip, gpio, val); + return 0; +} + +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio) +{ + return ar2315_gpio_irq_base + gpio; +} + +static struct gpio_chip ar2315_gpio_chip = { + .label = DRIVER_NAME, + .direction_input = ar2315_gpio_dir_in, + .direction_output = ar2315_gpio_dir_out, + .set = ar2315_gpio_set_val, + .get = ar2315_gpio_get_val, + .to_irq = ar2315_gpio_to_irq, + .base = 0, + .ngpio = AR2315_GPIO_NUM, +}; + +static int ar2315_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct resource *res; + unsigned irq; + int ret; + + if (ar2315_mem) + return -EBUSY; + + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, DRIVER_NAME); + if (!res) { + dev_err(dev, "not found IRQ number\n"); + return -ENXIO; + } + irq = res->start; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, DRIVER_NAME); + ar2315_mem = devm_ioremap_resource(dev, res); + if (IS_ERR(ar2315_mem)) + return PTR_ERR(ar2315_mem); + + ar2315_gpio_chip.dev = dev; + ret = gpiochip_add(&ar2315_gpio_chip); + if (ret) { + dev_err(dev, "failed to add gpiochip\n"); + return ret; + } + + ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0); + if (ret < 0) { + dev_err(dev, "failed to allocate IRQ numbers\n"); + return ret; + } + ar2315_gpio_irq_base = ret; + + ar2315_gpio_irq_init(irq); + + return 0; +} + +static struct platform_driver ar2315_gpio_driver = { + .probe = ar2315_gpio_probe, + .driver = { + .name = DRIVER_NAME, + .owner = THIS_MODULE, + } +}; + +static int __init ar2315_gpio_init(void) +{ + return platform_driver_register(&ar2315_gpio_driver); +} +subsys_initcall(ar2315_gpio_init);
Atheros AR2315 SoC have a builtin GPIO controller, which could be accessed via memory mapped registers. This patch adds new driver for them. Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Cc: linux-gpio@vger.kernel.org --- Changes since RFC: - fix chip name, this patch adds AR2315 GPIO controller driver - use dynamic IRQ numbers allocation - move device registration to separate patch drivers/gpio/Kconfig | 7 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-ar2315.c | 232 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+) create mode 100644 drivers/gpio/gpio-ar2315.c