| Message ID | 20191015173026.9962-4-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add GPIO support for RDA8810PL SoC | expand |
On Tue, Oct 15, 2019 at 11:00:25PM +0530, Manivannan Sadhasivam wrote: > Add support for GPIO controller from RDA Micro. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/gpio/Kconfig | 8 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-rda.c | 315 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 324 insertions(+) > create mode 100644 drivers/gpio/gpio-rda.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 38e096e6925f..71826e61fdb3 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -435,6 +435,14 @@ config GPIO_RCAR > help > Say yes here to support GPIO on Renesas R-Car SoCs. > > +config GPIO_RDA > + bool "RDA Micro GPIO controller support" > + depends on ARCH_RDA || COMPILE_TEST > + depends on OF_GPIO > + select GPIOLIB_IRQCHIP > + help > + Say Y here to support RDA Micro GPIO controller. > + > config GPIO_REG > bool > help > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index d2fd19c15bae..5c68c9a48fa3 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -115,6 +115,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o > obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o > obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o > obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o > +obj-$(CONFIG_GPIO_RDA) += gpio-rda.o > obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o > obj-$(CONFIG_GPIO_REG) += gpio-reg.o > obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o > diff --git a/drivers/gpio/gpio-rda.c b/drivers/gpio/gpio-rda.c > new file mode 100644 > index 000000000000..1e6ffe27897f > --- /dev/null > +++ b/drivers/gpio/gpio-rda.c > @@ -0,0 +1,315 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * RDA Micro GPIO driver > + * > + * Copyright (C) 2012 RDA Micro Inc. > + * Copyright (C) 2019 Manivannan Sadhasivam > + */ > + > +#include <linux/bitops.h> > +#include <linux/gpio/driver.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> Ah, just realised that I forgot to remove this unused include. Should I send a new version? Thanks, Mani > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> > + > +#define RDA_GPIO_OEN_VAL 0x00 > +#define RDA_GPIO_OEN_SET_OUT 0x04 > +#define RDA_GPIO_OEN_SET_IN 0x08 > +#define RDA_GPIO_VAL 0x0c > +#define RDA_GPIO_SET 0x10 > +#define RDA_GPIO_CLR 0x14 > +#define RDA_GPIO_INT_CTRL_SET 0x18 > +#define RDA_GPIO_INT_CTRL_CLR 0x1c > +#define RDA_GPIO_INT_CLR 0x20 > +#define RDA_GPIO_INT_STATUS 0x24 > + > +#define RDA_GPIO_IRQ_RISE_SHIFT 0 > +#define RDA_GPIO_IRQ_FALL_SHIFT 8 > +#define RDA_GPIO_DEBOUCE_SHIFT 16 > +#define RDA_GPIO_LEVEL_SHIFT 24 > + > +#define RDA_GPIO_IRQ_MASK 0xff > + > +/* Each bank consists of 32 GPIOs */ > +#define RDA_GPIO_BANK_NR 32 > + > +struct rda_gpio { > + struct gpio_chip chip; > + void __iomem *base; > + spinlock_t lock; > + struct irq_chip irq_chip; > + int irq; > +}; > + > +static void rda_gpio_update(struct gpio_chip *chip, unsigned int offset, > + u16 reg, int val) > +{ > + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); > + void __iomem *base = rda_gpio->base; > + unsigned long flags; > + u32 tmp; > + > + spin_lock_irqsave(&rda_gpio->lock, flags); > + tmp = readl_relaxed(base + reg); > + > + if (val) > + tmp |= BIT(offset); > + else > + tmp &= ~BIT(offset); > + > + writel_relaxed(tmp, base + reg); > + spin_unlock_irqrestore(&rda_gpio->lock, flags); > +} > + > +static int rda_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) > +{ > + rda_gpio_update(chip, offset, RDA_GPIO_OEN_SET_IN, 1); > + > + return 0; > +} > + > +static int rda_gpio_direction_output(struct gpio_chip *chip, > + unsigned int offset, int value) > +{ > + rda_gpio_update(chip, offset, RDA_GPIO_OEN_SET_OUT, 1); > + > + return 0; > +} > + > +static int rda_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); > + void __iomem *base = rda_gpio->base; > + > + if (readl_relaxed(base + RDA_GPIO_OEN_VAL) & BIT(offset)) > + return !!(readl_relaxed(base + RDA_GPIO_VAL) & BIT(offset)); > + else > + return !!(readl_relaxed(base + RDA_GPIO_SET) & BIT(offset)); > +} > + > +static void rda_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) > +{ > + if (value) > + rda_gpio_update(chip, offset, RDA_GPIO_SET, 1); > + else > + rda_gpio_update(chip, offset, RDA_GPIO_CLR, 1); > +} > + > +static void rda_gpio_irq_mask(struct irq_data *data) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); > + void __iomem *base = rda_gpio->base; > + u32 offset = irqd_to_hwirq(data); > + u32 value; > + > + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; > + value |= BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; > + > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); > +} > + > +static void rda_gpio_irq_ack(struct irq_data *data) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + u32 offset = irqd_to_hwirq(data); > + > + rda_gpio_update(chip, offset, RDA_GPIO_INT_CLR, 1); > +} > + > +static int rda_gpio_set_irq(struct gpio_chip *chip, u32 offset, > + unsigned int flow_type) > +{ > + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); > + void __iomem *base = rda_gpio->base; > + u32 value; > + > + switch (flow_type) { > + case IRQ_TYPE_EDGE_RISING: > + /* Set rising edge trigger */ > + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); > + > + /* Switch to edge trigger interrupt */ > + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + /* Set falling edge trigger */ > + value = BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); > + > + /* Switch to edge trigger interrupt */ > + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + /* Set both edge trigger */ > + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; > + value |= BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); > + > + /* Switch to edge trigger interrupt */ > + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); > + break; > + > + case IRQ_TYPE_LEVEL_HIGH: > + /* Set high level trigger */ > + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; > + > + /* Switch to level trigger interrupt */ > + value |= BIT(offset) << RDA_GPIO_LEVEL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); > + break; > + > + case IRQ_TYPE_LEVEL_LOW: > + /* Set low level trigger */ > + value = BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; > + > + /* Switch to level trigger interrupt */ > + value |= BIT(offset) << RDA_GPIO_LEVEL_SHIFT; > + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void rda_gpio_irq_unmask(struct irq_data *data) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + u32 offset = irqd_to_hwirq(data); > + u32 trigger = irqd_get_trigger_type(data); > + > + rda_gpio_set_irq(chip, offset, trigger); > +} > + > +static int rda_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + u32 offset = irqd_to_hwirq(data); > + int ret; > + > + ret = rda_gpio_set_irq(chip, offset, flow_type); > + if (ret) > + return ret; > + > + if (flow_type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) > + irq_set_handler_locked(data, handle_level_irq); > + else if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) > + irq_set_handler_locked(data, handle_edge_irq); > + > + return 0; > +} > + > +static void rda_gpio_irq_handler(struct irq_desc *desc) > +{ > + struct gpio_chip *chip = irq_desc_get_handler_data(desc); > + struct irq_chip *ic = irq_desc_get_chip(desc); > + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); > + unsigned long status; > + u32 n, girq; > + > + chained_irq_enter(ic, desc); > + > + status = readl_relaxed(rda_gpio->base + RDA_GPIO_INT_STATUS); > + /* Only lower 8 bits are capable of generating interrupts */ > + status &= RDA_GPIO_IRQ_MASK; > + > + for_each_set_bit(n, &status, RDA_GPIO_BANK_NR) { > + girq = irq_find_mapping(chip->irq.domain, n); > + generic_handle_irq(girq); > + } > + > + chained_irq_exit(ic, desc); > +} > + > +static int rda_gpio_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct gpio_irq_chip *irq_chip; > + struct rda_gpio *rda_gpio; > + u32 ngpios; > + int ret; > + > + rda_gpio = devm_kzalloc(&pdev->dev, sizeof(*rda_gpio), GFP_KERNEL); > + if (!rda_gpio) > + return -ENOMEM; > + > + ret = device_property_read_u32(&pdev->dev, "ngpios", &ngpios); > + if (ret < 0) > + return ret; > + > + /* > + * Not all ports have interrupt capability. For instance, on > + * RDA8810PL, GPIOC doesn't support interrupt. So we must handle > + * those also. > + */ > + rda_gpio->irq = platform_get_irq(pdev, 0); > + > + rda_gpio->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(rda_gpio->base)) > + return PTR_ERR(rda_gpio->base); > + > + spin_lock_init(&rda_gpio->lock); > + > + rda_gpio->chip.label = dev_name(&pdev->dev); > + rda_gpio->chip.ngpio = ngpios; > + rda_gpio->chip.base = -1; > + rda_gpio->chip.parent = &pdev->dev; > + rda_gpio->chip.of_node = np; > + rda_gpio->chip.get = rda_gpio_get; > + rda_gpio->chip.set = rda_gpio_set; > + rda_gpio->chip.direction_input = rda_gpio_direction_input; > + rda_gpio->chip.direction_output = rda_gpio_direction_output; > + > + if (rda_gpio->irq >= 0) { > + rda_gpio->irq_chip.name = "rda-gpio", > + rda_gpio->irq_chip.irq_ack = rda_gpio_irq_ack, > + rda_gpio->irq_chip.irq_mask = rda_gpio_irq_mask, > + rda_gpio->irq_chip.irq_unmask = rda_gpio_irq_unmask, > + rda_gpio->irq_chip.irq_set_type = rda_gpio_irq_set_type, > + rda_gpio->irq_chip.flags = IRQCHIP_SKIP_SET_WAKE, > + > + irq_chip = &rda_gpio->chip.irq; > + irq_chip->chip = &rda_gpio->irq_chip; > + irq_chip->handler = handle_bad_irq; > + irq_chip->default_type = IRQ_TYPE_NONE; > + irq_chip->parent_handler = rda_gpio_irq_handler; > + irq_chip->parent_handler_data = rda_gpio; > + irq_chip->num_parents = 1; > + irq_chip->parents = &rda_gpio->irq; > + } > + > + platform_set_drvdata(pdev, rda_gpio); > + > + return devm_gpiochip_add_data(&pdev->dev, &rda_gpio->chip, rda_gpio); > +} > + > +static const struct of_device_id rda_gpio_of_match[] = { > + { .compatible = "rda,8810pl-gpio", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, rda_gpio_of_match); > + > +static struct platform_driver rda_gpio_driver = { > + .probe = rda_gpio_probe, > + .driver = { > + .name = "rda-gpio", > + .of_match_table = rda_gpio_of_match, > + }, > +}; > + > +module_platform_driver_probe(rda_gpio_driver, rda_gpio_probe); > + > +MODULE_DESCRIPTION("RDA Micro GPIO driver"); > +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 >
Hi Manivannan! Thanks for your patch! On Tue, Oct 15, 2019 at 7:30 PM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > Add support for GPIO controller from RDA Micro. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Please use a little bit more verbose commit message, who made this hardware and what is it for. If you know! > +config GPIO_RDA > + bool "RDA Micro GPIO controller support" > + depends on ARCH_RDA || COMPILE_TEST > + depends on OF_GPIO > + select GPIOLIB_IRQCHIP select GPIO_GENERIC > +#include <linux/bitops.h> Do you need this or just <linux/bits.h>? > +#define RDA_GPIO_OEN_VAL 0x00 > +#define RDA_GPIO_OEN_SET_OUT 0x04 > +#define RDA_GPIO_OEN_SET_IN 0x08 > +#define RDA_GPIO_VAL 0x0c > +#define RDA_GPIO_SET 0x10 > +#define RDA_GPIO_CLR 0x14 > +#define RDA_GPIO_INT_CTRL_SET 0x18 > +#define RDA_GPIO_INT_CTRL_CLR 0x1c > +#define RDA_GPIO_INT_CLR 0x20 > +#define RDA_GPIO_INT_STATUS 0x24 This is a very clear cut MMIO GPIO so use GPIO_GENERIC with this hardware. > +static void rda_gpio_update(struct gpio_chip *chip, unsigned int offset, > + u16 reg, int val) Maybe keep this if it saves code from the IRQ callbacks, inline it to register writes if it doesn't get called much. > +static int rda_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) > +static int rda_gpio_direction_output(struct gpio_chip *chip, > + unsigned int offset, int value) > +static int rda_gpio_get(struct gpio_chip *chip, unsigned int offset) > +static void rda_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) This can all be replaces by select GPIO_GENERIC and passing the right offsets into bgpio_init(). Look at for example gpio-ftgpio010.c and the documentation for bgpio_init() in gpio-mmio.c for help. This will also implement get/set_multiple for you for free! > +static void rda_gpio_irq_mask(struct irq_data *data) > +static void rda_gpio_irq_ack(struct irq_data *data) Looks good > +static int rda_gpio_set_irq(struct gpio_chip *chip, u32 offset, > + unsigned int flow_type) Maybe _setup_irq()? Not sure, just that the name doesn't obviously imply how it is used as it is called from two places. The rest of the IRQ code looks good! > +static int rda_gpio_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct gpio_irq_chip *irq_chip; Since irq_chip is the name of a struct in the kernel I usually just call this "girq" as in "GPIO irq chip". > + struct rda_gpio *rda_gpio; > + u32 ngpios; > + int ret; Create a struct device *dev = &pdev->dev; helper variable to make the following code easier to read. (The pointer &pdev->dev is used in many places...) > + /* > + * Not all ports have interrupt capability. For instance, on > + * RDA8810PL, GPIOC doesn't support interrupt. So we must handle > + * those also. > + */ > + rda_gpio->irq = platform_get_irq(pdev, 0); > + > + rda_gpio->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(rda_gpio->base)) > + return PTR_ERR(rda_gpio->base); > + > + spin_lock_init(&rda_gpio->lock); > + > + rda_gpio->chip.label = dev_name(&pdev->dev); > + rda_gpio->chip.ngpio = ngpios; > + rda_gpio->chip.base = -1; > + rda_gpio->chip.parent = &pdev->dev; > + rda_gpio->chip.of_node = np; > + rda_gpio->chip.get = rda_gpio_get; > + rda_gpio->chip.set = rda_gpio_set; > + rda_gpio->chip.direction_input = rda_gpio_direction_input; > + rda_gpio->chip.direction_output = rda_gpio_direction_output; > + > + if (rda_gpio->irq >= 0) { > + rda_gpio->irq_chip.name = "rda-gpio", > + rda_gpio->irq_chip.irq_ack = rda_gpio_irq_ack, > + rda_gpio->irq_chip.irq_mask = rda_gpio_irq_mask, > + rda_gpio->irq_chip.irq_unmask = rda_gpio_irq_unmask, > + rda_gpio->irq_chip.irq_set_type = rda_gpio_irq_set_type, > + rda_gpio->irq_chip.flags = IRQCHIP_SKIP_SET_WAKE, > + > + irq_chip = &rda_gpio->chip.irq; > + irq_chip->chip = &rda_gpio->irq_chip; > + irq_chip->handler = handle_bad_irq; > + irq_chip->default_type = IRQ_TYPE_NONE; > + irq_chip->parent_handler = rda_gpio_irq_handler; > + irq_chip->parent_handler_data = rda_gpio; > + irq_chip->num_parents = 1; > + irq_chip->parents = &rda_gpio->irq; That works but ... please devm_kzalloc() like the other drivers do: girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), GFP_KERNEL); if (!girq->parents) { ret = -ENOMEM; (...) Unless you have a real good reason to optimize it. I just want it to follow the pattern since I want to minimize cognitive stress for the maintainers. (Me.) Yours, Linus Walleij
Hi Linus, Thanks for the review! Please see comments inline. On Wed, Oct 16, 2019 at 02:41:32PM +0200, Linus Walleij wrote: > Hi Manivannan! > > Thanks for your patch! > > On Tue, Oct 15, 2019 at 7:30 PM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > Add support for GPIO controller from RDA Micro. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Please use a little bit more verbose commit message, who > made this hardware and what is it for. If you know! > okay. > > +config GPIO_RDA > > + bool "RDA Micro GPIO controller support" > > + depends on ARCH_RDA || COMPILE_TEST > > + depends on OF_GPIO > > + select GPIOLIB_IRQCHIP > > select GPIO_GENERIC > hmm.. I don't think this driver can use it. Please see the justification below. > > +#include <linux/bitops.h> > > Do you need this or just <linux/bits.h>? > I need this for for_each_set_bit() macro. > > +#define RDA_GPIO_OEN_VAL 0x00 > > +#define RDA_GPIO_OEN_SET_OUT 0x04 > > +#define RDA_GPIO_OEN_SET_IN 0x08 > > +#define RDA_GPIO_VAL 0x0c > > +#define RDA_GPIO_SET 0x10 > > +#define RDA_GPIO_CLR 0x14 > > +#define RDA_GPIO_INT_CTRL_SET 0x18 > > +#define RDA_GPIO_INT_CTRL_CLR 0x1c > > +#define RDA_GPIO_INT_CLR 0x20 > > +#define RDA_GPIO_INT_STATUS 0x24 > > This is a very clear cut MMIO GPIO so use GPIO_GENERIC with this > hardware. > So, I'd be happy to use gpio-mmio driver if applicable. In fact, I looked into that while starting to write this driver since most of the `set*` APIs are like dups. But one thing which blocked me was, `gpio_get` API. As you can see in this driver, there are 2 separate registers needs to be read in order to get the value. RDA_GPIO_VAL needs to be read when the pin is in input state and RDA_GPIO_SET needs to be read when the pin is in output state. The MMIO driver relies on a single `dat` register to read the GPIO state and this won't fit for this driver and hence my justification for not using it. > > +static void rda_gpio_update(struct gpio_chip *chip, unsigned int offset, > > + u16 reg, int val) > > Maybe keep this if it saves code from the IRQ callbacks, > inline it to register writes if it doesn't get called much. > It is being called from multiple places, so I'd like to keep it as a normal function. > > +static int rda_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) > > +static int rda_gpio_direction_output(struct gpio_chip *chip, > > + unsigned int offset, int value) > > +static int rda_gpio_get(struct gpio_chip *chip, unsigned int offset) > > +static void rda_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) > > This can all be replaces by select GPIO_GENERIC and passing > the right offsets into bgpio_init(). Look at for example > gpio-ftgpio010.c and the documentation for bgpio_init() > in gpio-mmio.c for help. > > This will also implement get/set_multiple for you for > free! > > > +static void rda_gpio_irq_mask(struct irq_data *data) > > +static void rda_gpio_irq_ack(struct irq_data *data) > > Looks good > > > +static int rda_gpio_set_irq(struct gpio_chip *chip, u32 offset, > > + unsigned int flow_type) > > Maybe _setup_irq()? Not sure, just that the name doesn't > obviously imply how it is used as it is called from two > places. > Well, this routine sets the irq_type. But it has multiple usecase. Like, it is being used to unmask as irq and also to set irq type. So to be in a equillibrium state, I went for rda_gpio_set_irq(). > The rest of the IRQ code looks good! > > > +static int rda_gpio_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct gpio_irq_chip *irq_chip; > > Since irq_chip is the name of a struct in the kernel I usually > just call this "girq" as in "GPIO irq chip". > Ah, a name change again... will do ;-) > > + struct rda_gpio *rda_gpio; > > + u32 ngpios; > > + int ret; > > Create a struct device *dev = &pdev->dev; helper variable > to make the following code easier to read. (The pointer > &pdev->dev is used in many places...) > okay. > > + /* > > + * Not all ports have interrupt capability. For instance, on > > + * RDA8810PL, GPIOC doesn't support interrupt. So we must handle > > + * those also. > > + */ > > + rda_gpio->irq = platform_get_irq(pdev, 0); > > + > > + rda_gpio->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(rda_gpio->base)) > > + return PTR_ERR(rda_gpio->base); > > + > > + spin_lock_init(&rda_gpio->lock); > > + > > + rda_gpio->chip.label = dev_name(&pdev->dev); > > + rda_gpio->chip.ngpio = ngpios; > > + rda_gpio->chip.base = -1; > > + rda_gpio->chip.parent = &pdev->dev; > > + rda_gpio->chip.of_node = np; > > + rda_gpio->chip.get = rda_gpio_get; > > + rda_gpio->chip.set = rda_gpio_set; > > + rda_gpio->chip.direction_input = rda_gpio_direction_input; > > + rda_gpio->chip.direction_output = rda_gpio_direction_output; > > + > > + if (rda_gpio->irq >= 0) { > > + rda_gpio->irq_chip.name = "rda-gpio", > > + rda_gpio->irq_chip.irq_ack = rda_gpio_irq_ack, > > + rda_gpio->irq_chip.irq_mask = rda_gpio_irq_mask, > > + rda_gpio->irq_chip.irq_unmask = rda_gpio_irq_unmask, > > + rda_gpio->irq_chip.irq_set_type = rda_gpio_irq_set_type, > > + rda_gpio->irq_chip.flags = IRQCHIP_SKIP_SET_WAKE, > > + > > + irq_chip = &rda_gpio->chip.irq; > > + irq_chip->chip = &rda_gpio->irq_chip; > > + irq_chip->handler = handle_bad_irq; > > + irq_chip->default_type = IRQ_TYPE_NONE; > > + irq_chip->parent_handler = rda_gpio_irq_handler; > > + irq_chip->parent_handler_data = rda_gpio; > > + irq_chip->num_parents = 1; > > + irq_chip->parents = &rda_gpio->irq; > > That works but ... please devm_kzalloc() like the other drivers > do: > > girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), > GFP_KERNEL); > if (!girq->parents) { > ret = -ENOMEM; > (...) > > Unless you have a real good reason to optimize it. I just > want it to follow the pattern since I want to minimize > cognitive stress for the maintainers. (Me.) > no issues for me, will do. Thanks, Mani > Yours, > Linus Walleij
On Sat, Oct 19, 2019 at 6:05 PM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > On Wed, Oct 16, 2019 at 02:41:32PM +0200, Linus Walleij wrote: > > select GPIO_GENERIC > > hmm.. I don't think this driver can use it. Please see the justification > below. (...) > As you can see in this driver, there are 2 separate registers needs to be > read in order to get the value. RDA_GPIO_VAL needs to be read when the pin > is in input state and RDA_GPIO_SET needs to be read when the pin is in output > state. > > The MMIO driver relies on a single `dat` register to read the GPIO state and > this won't fit for this driver and hence my justification for not using it. Use RDA_GPIO_VAL for dat, then set BGPIOF_READ_OUTPUT_REG_SET and the mmio core will do what you want I think? That's what the flag is for IIUC. Maybe we should document it better :/ Yours, Linus Walleij
Hi Linus, On Mon, Oct 21, 2019 at 02:57:31AM +0200, Linus Walleij wrote: > On Sat, Oct 19, 2019 at 6:05 PM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > On Wed, Oct 16, 2019 at 02:41:32PM +0200, Linus Walleij wrote: > > > > select GPIO_GENERIC > > > > hmm.. I don't think this driver can use it. Please see the justification > > below. > (...) > > As you can see in this driver, there are 2 separate registers needs to be > > read in order to get the value. RDA_GPIO_VAL needs to be read when the pin > > is in input state and RDA_GPIO_SET needs to be read when the pin is in output > > state. > > > > The MMIO driver relies on a single `dat` register to read the GPIO state and > > this won't fit for this driver and hence my justification for not using it. > > Use RDA_GPIO_VAL for dat, then set BGPIOF_READ_OUTPUT_REG_SET > and the mmio core will do what you want I think? That's what the flag is > for IIUC. > Ah, this should work. Sorry for missing this earlier. > Maybe we should document it better :/ > That's how everything is in kernel for me... If you don't look closer, you'll not get it. Thanks, Mani > Yours, > Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 38e096e6925f..71826e61fdb3 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -435,6 +435,14 @@ config GPIO_RCAR help Say yes here to support GPIO on Renesas R-Car SoCs. +config GPIO_RDA + bool "RDA Micro GPIO controller support" + depends on ARCH_RDA || COMPILE_TEST + depends on OF_GPIO + select GPIOLIB_IRQCHIP + help + Say Y here to support RDA Micro GPIO controller. + config GPIO_REG bool help diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index d2fd19c15bae..5c68c9a48fa3 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -115,6 +115,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o +obj-$(CONFIG_GPIO_RDA) += gpio-rda.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o obj-$(CONFIG_GPIO_REG) += gpio-reg.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o diff --git a/drivers/gpio/gpio-rda.c b/drivers/gpio/gpio-rda.c new file mode 100644 index 000000000000..1e6ffe27897f --- /dev/null +++ b/drivers/gpio/gpio-rda.c @@ -0,0 +1,315 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * RDA Micro GPIO driver + * + * Copyright (C) 2012 RDA Micro Inc. + * Copyright (C) 2019 Manivannan Sadhasivam + */ + +#include <linux/bitops.h> +#include <linux/gpio/driver.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +#define RDA_GPIO_OEN_VAL 0x00 +#define RDA_GPIO_OEN_SET_OUT 0x04 +#define RDA_GPIO_OEN_SET_IN 0x08 +#define RDA_GPIO_VAL 0x0c +#define RDA_GPIO_SET 0x10 +#define RDA_GPIO_CLR 0x14 +#define RDA_GPIO_INT_CTRL_SET 0x18 +#define RDA_GPIO_INT_CTRL_CLR 0x1c +#define RDA_GPIO_INT_CLR 0x20 +#define RDA_GPIO_INT_STATUS 0x24 + +#define RDA_GPIO_IRQ_RISE_SHIFT 0 +#define RDA_GPIO_IRQ_FALL_SHIFT 8 +#define RDA_GPIO_DEBOUCE_SHIFT 16 +#define RDA_GPIO_LEVEL_SHIFT 24 + +#define RDA_GPIO_IRQ_MASK 0xff + +/* Each bank consists of 32 GPIOs */ +#define RDA_GPIO_BANK_NR 32 + +struct rda_gpio { + struct gpio_chip chip; + void __iomem *base; + spinlock_t lock; + struct irq_chip irq_chip; + int irq; +}; + +static void rda_gpio_update(struct gpio_chip *chip, unsigned int offset, + u16 reg, int val) +{ + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); + void __iomem *base = rda_gpio->base; + unsigned long flags; + u32 tmp; + + spin_lock_irqsave(&rda_gpio->lock, flags); + tmp = readl_relaxed(base + reg); + + if (val) + tmp |= BIT(offset); + else + tmp &= ~BIT(offset); + + writel_relaxed(tmp, base + reg); + spin_unlock_irqrestore(&rda_gpio->lock, flags); +} + +static int rda_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) +{ + rda_gpio_update(chip, offset, RDA_GPIO_OEN_SET_IN, 1); + + return 0; +} + +static int rda_gpio_direction_output(struct gpio_chip *chip, + unsigned int offset, int value) +{ + rda_gpio_update(chip, offset, RDA_GPIO_OEN_SET_OUT, 1); + + return 0; +} + +static int rda_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); + void __iomem *base = rda_gpio->base; + + if (readl_relaxed(base + RDA_GPIO_OEN_VAL) & BIT(offset)) + return !!(readl_relaxed(base + RDA_GPIO_VAL) & BIT(offset)); + else + return !!(readl_relaxed(base + RDA_GPIO_SET) & BIT(offset)); +} + +static void rda_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) +{ + if (value) + rda_gpio_update(chip, offset, RDA_GPIO_SET, 1); + else + rda_gpio_update(chip, offset, RDA_GPIO_CLR, 1); +} + +static void rda_gpio_irq_mask(struct irq_data *data) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); + void __iomem *base = rda_gpio->base; + u32 offset = irqd_to_hwirq(data); + u32 value; + + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; + value |= BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; + + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); +} + +static void rda_gpio_irq_ack(struct irq_data *data) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + u32 offset = irqd_to_hwirq(data); + + rda_gpio_update(chip, offset, RDA_GPIO_INT_CLR, 1); +} + +static int rda_gpio_set_irq(struct gpio_chip *chip, u32 offset, + unsigned int flow_type) +{ + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); + void __iomem *base = rda_gpio->base; + u32 value; + + switch (flow_type) { + case IRQ_TYPE_EDGE_RISING: + /* Set rising edge trigger */ + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); + + /* Switch to edge trigger interrupt */ + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); + break; + + case IRQ_TYPE_EDGE_FALLING: + /* Set falling edge trigger */ + value = BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); + + /* Switch to edge trigger interrupt */ + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); + break; + + case IRQ_TYPE_EDGE_BOTH: + /* Set both edge trigger */ + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; + value |= BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); + + /* Switch to edge trigger interrupt */ + value = BIT(offset) << RDA_GPIO_LEVEL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_CLR); + break; + + case IRQ_TYPE_LEVEL_HIGH: + /* Set high level trigger */ + value = BIT(offset) << RDA_GPIO_IRQ_RISE_SHIFT; + + /* Switch to level trigger interrupt */ + value |= BIT(offset) << RDA_GPIO_LEVEL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); + break; + + case IRQ_TYPE_LEVEL_LOW: + /* Set low level trigger */ + value = BIT(offset) << RDA_GPIO_IRQ_FALL_SHIFT; + + /* Switch to level trigger interrupt */ + value |= BIT(offset) << RDA_GPIO_LEVEL_SHIFT; + writel_relaxed(value, base + RDA_GPIO_INT_CTRL_SET); + break; + + default: + return -EINVAL; + } + + return 0; +} + +static void rda_gpio_irq_unmask(struct irq_data *data) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + u32 offset = irqd_to_hwirq(data); + u32 trigger = irqd_get_trigger_type(data); + + rda_gpio_set_irq(chip, offset, trigger); +} + +static int rda_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + u32 offset = irqd_to_hwirq(data); + int ret; + + ret = rda_gpio_set_irq(chip, offset, flow_type); + if (ret) + return ret; + + if (flow_type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) + irq_set_handler_locked(data, handle_level_irq); + else if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) + irq_set_handler_locked(data, handle_edge_irq); + + return 0; +} + +static void rda_gpio_irq_handler(struct irq_desc *desc) +{ + struct gpio_chip *chip = irq_desc_get_handler_data(desc); + struct irq_chip *ic = irq_desc_get_chip(desc); + struct rda_gpio *rda_gpio = gpiochip_get_data(chip); + unsigned long status; + u32 n, girq; + + chained_irq_enter(ic, desc); + + status = readl_relaxed(rda_gpio->base + RDA_GPIO_INT_STATUS); + /* Only lower 8 bits are capable of generating interrupts */ + status &= RDA_GPIO_IRQ_MASK; + + for_each_set_bit(n, &status, RDA_GPIO_BANK_NR) { + girq = irq_find_mapping(chip->irq.domain, n); + generic_handle_irq(girq); + } + + chained_irq_exit(ic, desc); +} + +static int rda_gpio_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct gpio_irq_chip *irq_chip; + struct rda_gpio *rda_gpio; + u32 ngpios; + int ret; + + rda_gpio = devm_kzalloc(&pdev->dev, sizeof(*rda_gpio), GFP_KERNEL); + if (!rda_gpio) + return -ENOMEM; + + ret = device_property_read_u32(&pdev->dev, "ngpios", &ngpios); + if (ret < 0) + return ret; + + /* + * Not all ports have interrupt capability. For instance, on + * RDA8810PL, GPIOC doesn't support interrupt. So we must handle + * those also. + */ + rda_gpio->irq = platform_get_irq(pdev, 0); + + rda_gpio->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(rda_gpio->base)) + return PTR_ERR(rda_gpio->base); + + spin_lock_init(&rda_gpio->lock); + + rda_gpio->chip.label = dev_name(&pdev->dev); + rda_gpio->chip.ngpio = ngpios; + rda_gpio->chip.base = -1; + rda_gpio->chip.parent = &pdev->dev; + rda_gpio->chip.of_node = np; + rda_gpio->chip.get = rda_gpio_get; + rda_gpio->chip.set = rda_gpio_set; + rda_gpio->chip.direction_input = rda_gpio_direction_input; + rda_gpio->chip.direction_output = rda_gpio_direction_output; + + if (rda_gpio->irq >= 0) { + rda_gpio->irq_chip.name = "rda-gpio", + rda_gpio->irq_chip.irq_ack = rda_gpio_irq_ack, + rda_gpio->irq_chip.irq_mask = rda_gpio_irq_mask, + rda_gpio->irq_chip.irq_unmask = rda_gpio_irq_unmask, + rda_gpio->irq_chip.irq_set_type = rda_gpio_irq_set_type, + rda_gpio->irq_chip.flags = IRQCHIP_SKIP_SET_WAKE, + + irq_chip = &rda_gpio->chip.irq; + irq_chip->chip = &rda_gpio->irq_chip; + irq_chip->handler = handle_bad_irq; + irq_chip->default_type = IRQ_TYPE_NONE; + irq_chip->parent_handler = rda_gpio_irq_handler; + irq_chip->parent_handler_data = rda_gpio; + irq_chip->num_parents = 1; + irq_chip->parents = &rda_gpio->irq; + } + + platform_set_drvdata(pdev, rda_gpio); + + return devm_gpiochip_add_data(&pdev->dev, &rda_gpio->chip, rda_gpio); +} + +static const struct of_device_id rda_gpio_of_match[] = { + { .compatible = "rda,8810pl-gpio", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rda_gpio_of_match); + +static struct platform_driver rda_gpio_driver = { + .probe = rda_gpio_probe, + .driver = { + .name = "rda-gpio", + .of_match_table = rda_gpio_of_match, + }, +}; + +module_platform_driver_probe(rda_gpio_driver, rda_gpio_probe); + +MODULE_DESCRIPTION("RDA Micro GPIO driver"); +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"); +MODULE_LICENSE("GPL v2");
Add support for GPIO controller from RDA Micro. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-rda.c | 315 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 324 insertions(+) create mode 100644 drivers/gpio/gpio-rda.c