| Message ID | ff236f6dab7f3408551f8f4b0345e62b37fd99c5.1410020459.git.stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote: > Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the > Vybrid's GPIO and PORT module. The driver is instanced once per > each GPIO/PORT module pair and handles 32 GPIO's. > > Signed-off-by: Stefan Agner <stefan@agner.ch> (...) > obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o > +obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o Some like to keep GPIOs tightly associated with a pin controller in a file next to the pin controller. I.e. in drivers/pinctrl/freescale/gpio-vf610.c But this works too. Any preference? > +#define GPIO_PER_PORT 32 Very generic define. VF610_GPIOS_PER_PORT? > +struct vf610_gpio_port { > + struct gpio_chip gc; > + void __iomem *base; > + void __iomem *gpio_base; > + u8 irqc[GPIO_PER_PORT]; > + int irq; irq? Why do you need to keep this around? > +static const struct of_device_id vf610_gpio_dt_ids[] = { > + { .compatible = "fsl,vf610-gpio" }, > + { /* sentinel */ } > +}; > + > +static inline void vf610_gpio_writel(u32 val, void __iomem *reg) > +{ > + __raw_writel(val, reg); Use writel_relaxed() instead unless you can explain why you want this. (Same for all occurences.) > +static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct vf610_gpio_port *port = > + container_of(gc, struct vf610_gpio_port, gc); > + > + return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio); #include <linux/bitops.h> ... & BIT(gpio) > +static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > +{ > + struct vf610_gpio_port *port = > + container_of(gc, struct vf610_gpio_port, gc); > + unsigned long mask = 1 << gpio; = BIT(gpio); > +static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc) > +{ > + struct vf610_gpio_port *port = irq_get_handler_data(irq); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + int pin; > + unsigned long irq_isfr; > + > + chained_irq_enter(chip, desc); > + > + irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR); > + > + for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) { > + vf610_gpio_writel(1 << pin, port->base + PORT_ISFR); BIT(pin) (etc) > + port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (port->irq == NO_IRQ) > + return -ENODEV; Can't you just use a local int irq; variable for this? > +static int __init gpio_vf610_init(void) > +{ > + return platform_driver_register(&vf610_gpio_driver); > +} > +postcore_initcall(gpio_vf610_init); postcore again. I don't like this, can you get rid of it? Overall the driver looks very nice except for these nitty gritty details. Yours, Linus Walleij
Hi Linus, Thanks for your review! Am 2014-09-23 11:58, schrieb Linus Walleij: > On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote: > >> Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the >> Vybrid's GPIO and PORT module. The driver is instanced once per >> each GPIO/PORT module pair and handles 32 GPIO's. >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> > (...) >> obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o >> +obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o > > Some like to keep GPIOs tightly associated with a pin controller > in a file next to the pin controller. > > I.e. in drivers/pinctrl/freescale/gpio-vf610.c > > But this works too. Any preference? The other Freescale GPIO drivers (e.g. gpio-mxs.c/gpio-mxc.c) are located under drivers/gpio/ hence I would prefer to leave them there, even we use pinctrl here. Unless someone at Freescale has another opinion on this? > >> +#define GPIO_PER_PORT 32 > > Very generic define. VF610_GPIOS_PER_PORT? Agreed > >> +struct vf610_gpio_port { >> + struct gpio_chip gc; >> + void __iomem *base; >> + void __iomem *gpio_base; >> + u8 irqc[GPIO_PER_PORT]; >> + int irq; > > irq? Why do you need to keep this around? > >> +static const struct of_device_id vf610_gpio_dt_ids[] = { >> + { .compatible = "fsl,vf610-gpio" }, >> + { /* sentinel */ } >> +}; >> + >> +static inline void vf610_gpio_writel(u32 val, void __iomem *reg) >> +{ >> + __raw_writel(val, reg); > > Use writel_relaxed() instead unless you can explain why you want this. > > (Same for all occurences.) Agreed, I have don't know why I used the __raw variant here. I think its because copied this two stubs from gpio-tegra.c. > >> +static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) >> +{ >> + struct vf610_gpio_port *port = >> + container_of(gc, struct vf610_gpio_port, gc); >> + >> + return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio); > > #include <linux/bitops.h> > > ... & BIT(gpio) > >> +static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) >> +{ >> + struct vf610_gpio_port *port = >> + container_of(gc, struct vf610_gpio_port, gc); >> + unsigned long mask = 1 << gpio; > > = BIT(gpio); > >> +static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc) >> +{ >> + struct vf610_gpio_port *port = irq_get_handler_data(irq); >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + int pin; >> + unsigned long irq_isfr; >> + >> + chained_irq_enter(chip, desc); >> + >> + irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR); >> + >> + for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) { >> + vf610_gpio_writel(1 << pin, port->base + PORT_ISFR); > > BIT(pin) > > (etc) Ok, will replace those bit operations. > >> + port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); >> + if (port->irq == NO_IRQ) >> + return -ENODEV; > > Can't you just use a local int irq; variable for this? > >> +static int __init gpio_vf610_init(void) >> +{ >> + return platform_driver_register(&vf610_gpio_driver); >> +} >> +postcore_initcall(gpio_vf610_init); > > postcore again. I don't like this, can you get rid of it? I guess we could load this driver easily a bit later. IMHO, since lots of other driver use GPIO's, we should it load before all the drivers gets loaded (before device_initcall). Most GPIO driver do this, some statistic again: $ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n -r 33 subsys_initcall 14 postcore_initcall 2 device_initcall 2 arch_initcall 1 late_initcall 1 core_initcall My proposal: Use subsys_initcall (which is called after arch_initcall) in this GPIO driver and leave the pinctrl driver as arch_initcall. This way we are absolutely sure that the GPIO driver gets loaded after the pinctrl and also leave the pinctrl at its current value. -- Stefan > > Overall the driver looks very nice except for these nitty gritty details. > > Yours, > Linus Walleij
On Tue, Sep 23, 2014 at 1:51 PM, Stefan Agner <stefan@agner.ch> wrote: > [Me] >> postcore again. I don't like this, can you get rid of it? > > I guess we could load this driver easily a bit later. IMHO, since lots > of other driver use GPIO's, we should it load before all the drivers > gets loaded (before device_initcall). Nope. We use deferred probing to control that today. Ideally all drivers should be device_initcall() and deferred probe be used to order things, not by playing around with initcalls. > Most GPIO driver do this, some statistic again: > $ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n > -r > 33 subsys_initcall > 14 postcore_initcall > 2 device_initcall > 2 arch_initcall > 1 late_initcall > 1 core_initcall Yeah old legacy. There are patch attacks to get rid of this. The reason we can't just change them is because sometimes dependent drivers do not handle the errorpath very well can can't defer cleanly. With a new driver I expect deferred probe to be used. Yours, Linus Walleij
Am 2014-09-24 13:10, schrieb Linus Walleij: > On Tue, Sep 23, 2014 at 1:51 PM, Stefan Agner <stefan@agner.ch> wrote: >> [Me] >>> postcore again. I don't like this, can you get rid of it? >> >> I guess we could load this driver easily a bit later. IMHO, since lots >> of other driver use GPIO's, we should it load before all the drivers >> gets loaded (before device_initcall). > > Nope. We use deferred probing to control that today. Ideally > all drivers should be device_initcall() and deferred probe be used > to order things, not by playing around with initcalls. > You can not "nope" my humble opinion, this is not possible, its write protected! :-) I fully understand the deferred probe approach, and I also think its really a good approach for almost all drivers. The system by itself makes sure the drivers are loaded in correct order. But if all drivers use pinctrl, and the pinctrl happens to be the last initialized driver, I first get 30 messages with probe deferred before the pinctrl driver finally gets initialized. IMHO, this is a waste of resources... Giving the system some hint what we need early (e.g. pinctrl or even GPIO drivers) is just sensible. But maybe I miss something here... Fact is, currently, without touching GPIO infrastructure code, the GPIO driver can not be deferred when the pinctrl driver is missing. The of_gpiochip_add_pin_range function does not handle the missing pinctrl driver. Hence as of now, the pinctrl still needs an earlier initcall. But anyway, currently the pinctrl driver is already using arch_initcall, this patch set is no longer touching that. >> Most GPIO driver do this, some statistic again: >> $ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n >> -r >> 33 subsys_initcall >> 14 postcore_initcall >> 2 device_initcall >> 2 arch_initcall >> 1 late_initcall >> 1 core_initcall > > Yeah old legacy. There are patch attacks to get rid of this. > > The reason we can't just change them is because sometimes > dependent drivers do not handle the errorpath very well can can't > defer cleanly. > > With a new driver I expect deferred probe to be used. > Actually, the esdhc driver gets the cd-gpio directly and does not handle EPROBE_DEFER properly, hence Vybrid would be affected too. But I understand that we need to start somewhere. I will change the GPIO driver to device_initcall and going to fix esdhc driver. -- Stefan
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 9de1515..82b38f5 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -334,6 +334,13 @@ config GPIO_TZ1090_PDC help Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs. +config GPIO_VF610 + def_bool y + depends on ARCH_MXC && SOC_VF610 + select GPIOLIB_IRQCHIP + help + Say yes here to support Vybrid vf610 GPIOs. + config GPIO_XILINX bool "Xilinx GPIO support" depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 5d024e3..9893d4c 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -95,6 +95,7 @@ obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o obj-$(CONFIG_GPIO_TZ1090) += gpio-tz1090.o obj-$(CONFIG_GPIO_TZ1090_PDC) += gpio-tz1090-pdc.o obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o +obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c new file mode 100644 index 0000000..5f59424 --- /dev/null +++ b/drivers/gpio/gpio-vf610.c @@ -0,0 +1,285 @@ +/* + * vf610 GPIO support through PORT and GPIO module + * + * Copyright (c) 2014 Toradex AG. + * + * Author: Stefan Agner <stefan@agner.ch>. + * + * 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. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/module.h> +#include <asm-generic/bug.h> + + +#define GPIO_PER_PORT 32 + +struct vf610_gpio_port { + struct gpio_chip gc; + void __iomem *base; + void __iomem *gpio_base; + u8 irqc[GPIO_PER_PORT]; + int irq; +}; + +#define GPIO_PDOR 0x00 +#define GPIO_PSOR 0x04 +#define GPIO_PCOR 0x08 +#define GPIO_PTOR 0x0c +#define GPIO_PDIR 0x10 + +#define PORT_PCR(n) (n * 0x4) +#define PORT_PCR_IRQC_OFFSET 16 + +#define PORT_ISFR 0xa0 +#define PORT_DFER 0xc0 +#define PORT_DFCR 0xc4 +#define PORT_DFWR 0xc8 + +#define PORT_INT_OFF 0x0 +#define PORT_INT_LOGIC_ZERO 0x8 +#define PORT_INT_RISING_EDGE 0x9 +#define PORT_INT_FALLING_EDGE 0xa +#define PORT_INT_EITHER_EDGE 0xb +#define PORT_INT_LOGIC_ONE 0xc + + +static const struct of_device_id vf610_gpio_dt_ids[] = { + { .compatible = "fsl,vf610-gpio" }, + { /* sentinel */ } +}; + +static inline void vf610_gpio_writel(u32 val, void __iomem *reg) +{ + __raw_writel(val, reg); +} + +static inline u32 vf610_gpio_readl(void __iomem *reg) +{ + return __raw_readl(reg); +} + +static int vf610_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + return pinctrl_request_gpio(chip->base + offset); +} + +static void vf610_gpio_free(struct gpio_chip *chip, unsigned offset) +{ + pinctrl_free_gpio(chip->base + offset); +} + +static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct vf610_gpio_port *port = + container_of(gc, struct vf610_gpio_port, gc); + + return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio); +} + +static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) +{ + struct vf610_gpio_port *port = + container_of(gc, struct vf610_gpio_port, gc); + unsigned long mask = 1 << gpio; + + if (val) + vf610_gpio_writel(mask, port->gpio_base + GPIO_PSOR); + else + vf610_gpio_writel(mask, port->gpio_base + GPIO_PCOR); +} + +static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned gpio) +{ + return pinctrl_gpio_direction_input(chip->base + gpio); +} + +static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, + int value) +{ + vf610_gpio_set(chip, gpio, value); + + return pinctrl_gpio_direction_output(chip->base + gpio); +} + +static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc) +{ + struct vf610_gpio_port *port = irq_get_handler_data(irq); + struct irq_chip *chip = irq_desc_get_chip(desc); + int pin; + unsigned long irq_isfr; + + chained_irq_enter(chip, desc); + + irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR); + + for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) { + vf610_gpio_writel(1 << pin, port->base + PORT_ISFR); + + generic_handle_irq(irq_find_mapping(port->gc.irqdomain, pin)); + } + + chained_irq_exit(chip, desc); +} + + +static void vf610_gpio_irq_ack(struct irq_data *d) +{ + struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d); + int gpio = d->hwirq; + + vf610_gpio_writel(1 << gpio, port->base + PORT_ISFR); +} + +static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type) +{ + struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d); + u8 irqc; + + switch (type) { + case IRQ_TYPE_EDGE_RISING: + irqc = PORT_INT_RISING_EDGE; + break; + case IRQ_TYPE_EDGE_FALLING: + irqc = PORT_INT_FALLING_EDGE; + break; + case IRQ_TYPE_EDGE_BOTH: + irqc = PORT_INT_EITHER_EDGE; + break; + case IRQ_TYPE_LEVEL_LOW: + irqc = PORT_INT_LOGIC_ZERO; + break; + case IRQ_TYPE_LEVEL_HIGH: + irqc = PORT_INT_LOGIC_ONE; + break; + default: + return -EINVAL; + } + + port->irqc[d->hwirq] = irqc; + + return 0; +} + +static void vf610_gpio_irq_mask(struct irq_data *d) +{ + struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d); + void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq); + + vf610_gpio_writel(0, pcr_base); +} + +static void vf610_gpio_irq_unmask(struct irq_data *d) +{ + struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d); + void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq); + + vf610_gpio_writel(port->irqc[d->hwirq] << PORT_PCR_IRQC_OFFSET, + pcr_base); +} + +static struct irq_chip vf610_gpio_irq_chip = { + .name = "gpio-vf610", + .irq_ack = vf610_gpio_irq_ack, + .irq_mask = vf610_gpio_irq_mask, + .irq_unmask = vf610_gpio_irq_unmask, + .irq_set_type = vf610_gpio_irq_set_type, +}; + +static int vf610_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct vf610_gpio_port *port; + struct resource *iores; + struct gpio_chip *gc; + int ret; + + port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL); + if (!port) + return -ENOMEM; + + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); + port->base = devm_ioremap_resource(dev, iores); + if (IS_ERR(port->base)) + return PTR_ERR(port->base); + + iores = platform_get_resource(pdev, IORESOURCE_MEM, 1); + port->gpio_base = devm_ioremap_resource(dev, iores); + if (IS_ERR(port->gpio_base)) + return PTR_ERR(port->gpio_base); + + port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (port->irq == NO_IRQ) + return -ENODEV; + + gc = &port->gc; + gc->of_node = np; + gc->dev = dev; + gc->label = "vf610-gpio", + gc->ngpio = GPIO_PER_PORT, + gc->base = of_alias_get_id(np, "gpio") * GPIO_PER_PORT; + + gc->request = vf610_gpio_request, + gc->free = vf610_gpio_free, + gc->direction_input = vf610_gpio_direction_input, + gc->get = vf610_gpio_get, + gc->direction_output = vf610_gpio_direction_output, + gc->set = vf610_gpio_set, + + ret = gpiochip_add(gc); + if (ret < 0) + return ret; + + /* Clear the interrupt status register for all GPIO's */ + vf610_gpio_writel(~0, port->base + PORT_ISFR); + + ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0, + handle_simple_irq, IRQ_TYPE_NONE); + if (ret) { + dev_err(dev, "failed to add irqchip\n"); + gpiochip_remove(gc); + return ret; + } + gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, port->irq, + vf610_gpio_irq_handler); + + return 0; +} + +static struct platform_driver vf610_gpio_driver = { + .driver = { + .name = "gpio-vf610", + .owner = THIS_MODULE, + .of_match_table = vf610_gpio_dt_ids, + }, + .probe = vf610_gpio_probe, +}; + +static int __init gpio_vf610_init(void) +{ + return platform_driver_register(&vf610_gpio_driver); +} +postcore_initcall(gpio_vf610_init); + +MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>"); +MODULE_DESCRIPTION("Freescale VF610 GPIO"); +MODULE_LICENSE("GPL v2");
Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the Vybrid's GPIO and PORT module. The driver is instanced once per each GPIO/PORT module pair and handles 32 GPIO's. Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/gpio/Kconfig | 7 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-vf610.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+) create mode 100644 drivers/gpio/gpio-vf610.c