| Message ID | 1419409345-8297-3-git-send-email-thloh@altera.com |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-12-24 at 00:22 -0800, thloh@altera.com wrote: > Adds a new driver for Altera soft GPIO IP. The driver is able to > do read/write and allows GPIO to be a interrupt controller. Some trivial comments, some not quite so trivial. > diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c [] > +static int altera_gpio_irq_set_type(struct irq_data *d, > + unsigned int type) > +{ > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > + > + altera_gc = irq_data_get_irq_chip_data(d); I presume this multiple initialization of altera_gc is unnecessary duplication. > +static void altera_gpio_irq_edge_handler(unsigned int irq, > + struct irq_desc *desc) > +{ > + struct altera_gpio_chip *altera_gc; > + struct irq_chip *chip; > + struct of_mm_gpio_chip *mm_gc; > + unsigned long status; > + int i; > + > + altera_gc = irq_desc_get_handler_data(desc); > + chip = irq_desc_get_chip(desc); > + mm_gc = &altera_gc->mmchip; > + > + chained_irq_enter(chip, desc); > + > + while ((status = > + (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) & > + readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) { > + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP); > + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { > + generic_handle_irq( > + irq_find_mapping(altera_gc->mmchip.gc.irqdomain > + , i)); That's kind of unpleasant to read. It might be better to use a separate these statements and use a temporary for irq_find_mapping() > +static void altera_gpio_irq_leveL_high_handler(unsigned int irq, > + struct irq_desc *desc) > +{ > + struct altera_gpio_chip *altera_gc; > + struct irq_chip *chip; > + struct of_mm_gpio_chip *mm_gc; > + unsigned long status; > + int i; > + > + altera_gc = irq_desc_get_handler_data(desc); > + chip = irq_desc_get_chip(desc); > + mm_gc = &altera_gc->mmchip; > + > + chained_irq_enter(chip, desc); > + > + status = readl(mm_gc->regs + ALTERA_GPIO_DATA); > + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + > + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { > + generic_handle_irq( > + irq_find_mapping(altera_gc->mmchip.gc.irqdomain, i)); Maybe a temporary here too > +int altera_gpio_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + int id, reg, ret; > + struct altera_gpio_chip *altera_gc; > + > + altera_gc = devm_kzalloc(&pdev->dev, sizeof(*altera_gc), GFP_KERNEL); > + > + if (altera_gc == NULL) > + return -ENOMEM; No blank line after the devm_kzalloc please and if (!altera_gc) is more common. > + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, > + &altera_irq_chip, > + altera_gc->mapped_irq, > + altera_gpio_irq_leveL_high_handler); > + } else { > + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, > + &altera_irq_chip, > + altera_gc->mapped_irq, > + altera_gpio_irq_edge_handler); > + } Sometimes using a ?: ternary is smaller code gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, &altera_irq_chip, altera_gc->mapped_irq, altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH ? altera_gpio_irq_leveL_high_handler : altera_gpio_irq_edge_handler); > -- 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
On Wed, Dec 24, 2014 at 9:22 AM, <thloh@altera.com> wrote: > From: Tien Hock Loh <thloh@altera.com> > > Adds a new driver for Altera soft GPIO IP. The driver is able to > do read/write and allows GPIO to be a interrupt controller. > > Tested on Altera GHRD on interrupt handling and IO. > > Signed-off-by: Tien Hock Loh <thloh@altera.com> (...) > +config GPIO_ALTERA > + tristate "Altera GPIO" > + depends on OF_GPIO select GPIOLIB_IRQCHIP Also, I think (see below) select GENERIC_GPIO > +#include <linux/gpio.h> #include <linux/gpio/driver.h> should be just fine instead of this old header. > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/irq.h> Should not be needed. > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> None of these should be needed. > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> #include <linux/basic_mmio_gpio.h> with GENERIC_GPIO (see below). > + > +#define ALTERA_GPIO_MAX_NGPIO 32 > +#define ALTERA_GPIO_DATA 0x0 > +#define ALTERA_GPIO_DIR 0x4 > +#define ALTERA_GPIO_IRQ_MASK 0x8 > +#define ALTERA_GPIO_EDGE_CAP 0xc > + > +/** > +* struct altera_gpio_chip > +* @mmchip : memory mapped chip structure. > +* @gpio_lock : synchronization lock so that new irq/set/get requests > + will be blocked until the current one completes. > +* @interrupt_trigger : specifies the hardware configured IRQ trigger type > + (rising, falling, both, high) > +* @mapped_irq : kernel mapped irq number. > +*/ > +struct altera_gpio_chip { > + struct of_mm_gpio_chip mmchip; > + spinlock_t gpio_lock; > + int interrupt_trigger; > + int mapped_irq; > +}; > + > +static void altera_gpio_irq_unmask(struct irq_data *d) > +{ > + struct altera_gpio_chip *altera_gc; > + struct of_mm_gpio_chip *mm_gc; > + unsigned long flags; > + u32 intmask; > + > + altera_gc = irq_data_get_irq_chip_data(d); > + mm_gc = &altera_gc->mmchip; > + > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */ > + intmask |= BIT(irqd_to_hwirq(d)); > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > +} > + > +static void altera_gpio_irq_mask(struct irq_data *d) > +{ > + struct altera_gpio_chip *altera_gc; > + struct of_mm_gpio_chip *mm_gc; > + unsigned long flags; > + u32 intmask; > + > + altera_gc = irq_data_get_irq_chip_data(d); > + mm_gc = &altera_gc->mmchip; > + > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */ > + intmask &= ~BIT(irqd_to_hwirq(d)); > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > +} > + > +static int altera_gpio_irq_set_type(struct irq_data *d, > + unsigned int type) > +{ > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > + > + altera_gc = irq_data_get_irq_chip_data(d); > + > + if (type == IRQ_TYPE_NONE) > + return 0; > + if (type == IRQ_TYPE_LEVEL_HIGH && > + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) > + return 0; > + if (type == IRQ_TYPE_EDGE_RISING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) > + return 0; > + if (type == IRQ_TYPE_EDGE_FALLING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) > + return 0; > + if (type == IRQ_TYPE_EDGE_BOTH && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) > + return 0; > + > + return -EINVAL; > +} It took me a while to understand this. Write in a comment that this is a hardware that is synthesized for a specific trigger type, and that there is no way to software-configure the trigger type. > + > +static unsigned int altera_gpio_irq_startup(struct irq_data *d) > +{ > + altera_gpio_irq_unmask(d); > + > + return 0; > +} > + > +static void altera_gpio_irq_shutdown(struct irq_data *d) > +{ > + altera_gpio_irq_mask(d); > +} Instead of those pointless functions just assign the unmask/mask functions in the vtable right below. > + > +static struct irq_chip altera_irq_chip = { > + .name = "altera-gpio", > + .irq_mask = altera_gpio_irq_mask, > + .irq_unmask = altera_gpio_irq_unmask, > + .irq_set_type = altera_gpio_irq_set_type, > + .irq_startup = altera_gpio_irq_startup, > + .irq_shutdown = altera_gpio_irq_shutdown, i.e. here. > +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) > +{ > + struct of_mm_gpio_chip *mm_gc; > + > + mm_gc = to_of_mm_gpio_chip(gc); > + > + return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset); > +} > + > +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value) > +{ > + struct of_mm_gpio_chip *mm_gc; > + struct altera_gpio_chip *chip; > + unsigned long flags; > + unsigned int data_reg; > + > + mm_gc = to_of_mm_gpio_chip(gc); > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > + > + spin_lock_irqsave(&chip->gpio_lock, flags); > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > + if (value) > + data_reg |= BIT(offset); > + else > + data_reg &= ~BIT(offset); > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > +} > + > +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset) > +{ > + struct of_mm_gpio_chip *mm_gc; > + struct altera_gpio_chip *chip; > + unsigned long flags; > + unsigned int gpio_ddr; > + > + mm_gc = to_of_mm_gpio_chip(gc); > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > + > + spin_lock_irqsave(&chip->gpio_lock, flags); > + /* Set pin as input, assumes software controlled IP */ > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > + gpio_ddr &= ~BIT(offset); > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > + > + return 0; > +} > + > +static int altera_gpio_direction_output(struct gpio_chip *gc, > + unsigned offset, int value) > +{ > + struct of_mm_gpio_chip *mm_gc; > + struct altera_gpio_chip *chip; > + unsigned long flags; > + unsigned int data_reg, gpio_ddr; > + > + mm_gc = to_of_mm_gpio_chip(gc); > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > + > + spin_lock_irqsave(&chip->gpio_lock, flags); > + /* Sets the GPIO value */ > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > + if (value) > + data_reg |= BIT(offset); > + else > + data_reg &= ~BIT(offset); > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > + > + /* Set pin as output, assumes software controlled IP */ > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > + gpio_ddr |= BIT(offset); > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > + > + return 0; > +} These calls seem like pretty vanilla generic GPIO functions. Use GENERIC_GPIO with bgpio_init() and override the default functions only when really needed. See e.g. drivers/gpio/gpio-74xx-mmio.c > +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, > + irq_hw_number_t hw_irq_num) > +{ > + irq_set_chip_data(irq, h->host_data); > + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops altera_gpio_irq_ops = { > + .map = altera_gpio_irq_map, > + .xlate = irq_domain_xlate_onecell, > +}; This looks like some leftover garbage. You don't need them with GPIOLIB_IRQCHIP so delete these two. > +static int altera_gpio_remove(struct platform_device *pdev) > +{ > + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev); > + > + gpiochip_remove(&altera_gc->mmchip.gc); > + > + irq_set_handler_data(altera_gc->mapped_irq, NULL); > + irq_set_chained_handler(altera_gc->mapped_irq, NULL); These two calls should not be needed either. 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
On Wed, 2014-12-24 at 03:04 -0800, Joe Perches wrote: > On Wed, 2014-12-24 at 00:22 -0800, thloh@altera.com wrote: > > Adds a new driver for Altera soft GPIO IP. The driver is able to > > do read/write and allows GPIO to be a interrupt controller. > > Some trivial comments, some not quite so trivial. > > > diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c > [] > > +static int altera_gpio_irq_set_type(struct irq_data *d, > > + unsigned int type) > > +{ > > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > > + > > + altera_gc = irq_data_get_irq_chip_data(d); > > I presume this multiple initialization of altera_gc > is unnecessary duplication. I'll remove this. > > > +static void altera_gpio_irq_edge_handler(unsigned int irq, > > + struct irq_desc *desc) > > +{ > > + struct altera_gpio_chip *altera_gc; > > + struct irq_chip *chip; > > + struct of_mm_gpio_chip *mm_gc; > > + unsigned long status; > > + int i; > > + > > + altera_gc = irq_desc_get_handler_data(desc); > > + chip = irq_desc_get_chip(desc); > > + mm_gc = &altera_gc->mmchip; > > + > > + chained_irq_enter(chip, desc); > > + > > + while ((status = > > + (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) & > > + readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) { > > + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP); > > + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { > > + generic_handle_irq( > > + irq_find_mapping(altera_gc->mmchip.gc.irqdomain > > + , i)); > > That's kind of unpleasant to read. > It might be better to use a separate these statements > and use a temporary for irq_find_mapping() > OK I'll fix it. > > +static void altera_gpio_irq_leveL_high_handler(unsigned int irq, > > + struct irq_desc *desc) > > +{ > > + struct altera_gpio_chip *altera_gc; > > + struct irq_chip *chip; > > + struct of_mm_gpio_chip *mm_gc; > > + unsigned long status; > > + int i; > > + > > + altera_gc = irq_desc_get_handler_data(desc); > > + chip = irq_desc_get_chip(desc); > > + mm_gc = &altera_gc->mmchip; > > + > > + chained_irq_enter(chip, desc); > > + > > + status = readl(mm_gc->regs + ALTERA_GPIO_DATA); > > + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > + > > + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { > > + generic_handle_irq( > > + irq_find_mapping(altera_gc->mmchip.gc.irqdomain, i)); > > Maybe a temporary here too > Yup > > +int altera_gpio_probe(struct platform_device *pdev) > > +{ > > + struct device_node *node = pdev->dev.of_node; > > + int id, reg, ret; > > + struct altera_gpio_chip *altera_gc; > > + > > + altera_gc = devm_kzalloc(&pdev->dev, sizeof(*altera_gc), GFP_KERNEL); > > + > > + if (altera_gc == NULL) > > + return -ENOMEM; > > No blank line after the devm_kzalloc please and > if (!altera_gc) > is more common. > > OK noted. > > + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > > + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, > > + &altera_irq_chip, > > + altera_gc->mapped_irq, > > + altera_gpio_irq_leveL_high_handler); > > + } else { > > + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, > > + &altera_irq_chip, > > + altera_gc->mapped_irq, > > + altera_gpio_irq_edge_handler); > > + } > > Sometimes using a ?: ternary is smaller code > > gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, > &altera_irq_chip, > altera_gc->mapped_irq, > altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH ? > altera_gpio_irq_leveL_high_handler : > altera_gpio_irq_edge_handler); > > > Yup I'll fix it. Regards, Tien Hock Loh -- 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
On Wed, 2015-01-14 at 10:58 +0100, Linus Walleij wrote: > On Wed, Dec 24, 2014 at 9:22 AM, <thloh@altera.com> wrote: > > > From: Tien Hock Loh <thloh@altera.com> > > > > Adds a new driver for Altera soft GPIO IP. The driver is able to > > do read/write and allows GPIO to be a interrupt controller. > > > > Tested on Altera GHRD on interrupt handling and IO. > > > > Signed-off-by: Tien Hock Loh <thloh@altera.com> > > (...) > > +config GPIO_ALTERA > > + tristate "Altera GPIO" > > + depends on OF_GPIO > > select GPIOLIB_IRQCHIP > > Also, I think (see below) > > select GENERIC_GPIO > > > +#include <linux/gpio.h> > > #include <linux/gpio/driver.h> > > should be just fine instead of this old header. > > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/irq.h> > > Should not be needed. > > > +#include <linux/irqchip/chained_irq.h> > > +#include <linux/irqdomain.h> > > None of these should be needed. > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_gpio.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > #include <linux/basic_mmio_gpio.h> > > with GENERIC_GPIO (see below). OK > > + > > +#define ALTERA_GPIO_MAX_NGPIO 32 > > +#define ALTERA_GPIO_DATA 0x0 > > +#define ALTERA_GPIO_DIR 0x4 > > +#define ALTERA_GPIO_IRQ_MASK 0x8 > > +#define ALTERA_GPIO_EDGE_CAP 0xc > > + > > +/** > > +* struct altera_gpio_chip > > +* @mmchip : memory mapped chip structure. > > +* @gpio_lock : synchronization lock so that new irq/set/get requests > > + will be blocked until the current one completes. > > +* @interrupt_trigger : specifies the hardware configured IRQ trigger type > > + (rising, falling, both, high) > > +* @mapped_irq : kernel mapped irq number. > > +*/ > > +struct altera_gpio_chip { > > + struct of_mm_gpio_chip mmchip; > > + spinlock_t gpio_lock; > > + int interrupt_trigger; > > + int mapped_irq; > > +}; > > + > > +static void altera_gpio_irq_unmask(struct irq_data *d) > > +{ > > + struct altera_gpio_chip *altera_gc; > > + struct of_mm_gpio_chip *mm_gc; > > + unsigned long flags; > > + u32 intmask; > > + > > + altera_gc = irq_data_get_irq_chip_data(d); > > + mm_gc = &altera_gc->mmchip; > > + > > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */ > > + intmask |= BIT(irqd_to_hwirq(d)); > > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > > +} > > + > > +static void altera_gpio_irq_mask(struct irq_data *d) > > +{ > > + struct altera_gpio_chip *altera_gc; > > + struct of_mm_gpio_chip *mm_gc; > > + unsigned long flags; > > + u32 intmask; > > + > > + altera_gc = irq_data_get_irq_chip_data(d); > > + mm_gc = &altera_gc->mmchip; > > + > > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */ > > + intmask &= ~BIT(irqd_to_hwirq(d)); > > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > > +} > > + > > +static int altera_gpio_irq_set_type(struct irq_data *d, > > + unsigned int type) > > +{ > > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > > + > > + altera_gc = irq_data_get_irq_chip_data(d); > > + > > + if (type == IRQ_TYPE_NONE) > > + return 0; > > + if (type == IRQ_TYPE_LEVEL_HIGH && > > + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) > > + return 0; > > + if (type == IRQ_TYPE_EDGE_RISING && > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) > > + return 0; > > + if (type == IRQ_TYPE_EDGE_FALLING && > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) > > + return 0; > > + if (type == IRQ_TYPE_EDGE_BOTH && > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) > > + return 0; > > + > > + return -EINVAL; > > +} > > It took me a while to understand this. Write in a comment that > this is a hardware that is synthesized for a specific trigger > type, and that there is no way to software-configure the > trigger type. > OK noted. > > + > > +static unsigned int altera_gpio_irq_startup(struct irq_data *d) > > +{ > > + altera_gpio_irq_unmask(d); > > + > > + return 0; > > +} > > + > > +static void altera_gpio_irq_shutdown(struct irq_data *d) > > +{ > > + altera_gpio_irq_mask(d); > > +} > > Instead of those pointless functions just assign > the unmask/mask functions in the vtable right below. > OK > > + > > +static struct irq_chip altera_irq_chip = { > > + .name = "altera-gpio", > > + .irq_mask = altera_gpio_irq_mask, > > + .irq_unmask = altera_gpio_irq_unmask, > > + .irq_set_type = altera_gpio_irq_set_type, > > + .irq_startup = altera_gpio_irq_startup, > > + .irq_shutdown = altera_gpio_irq_shutdown, > > i.e. here. > > > +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) > > +{ > > + struct of_mm_gpio_chip *mm_gc; > > + > > + mm_gc = to_of_mm_gpio_chip(gc); > > + > > + return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset); > > +} > > + > > +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value) > > +{ > > + struct of_mm_gpio_chip *mm_gc; > > + struct altera_gpio_chip *chip; > > + unsigned long flags; > > + unsigned int data_reg; > > + > > + mm_gc = to_of_mm_gpio_chip(gc); > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > + > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > > + if (value) > > + data_reg |= BIT(offset); > > + else > > + data_reg &= ~BIT(offset); > > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > +} > > + > > +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset) > > +{ > > + struct of_mm_gpio_chip *mm_gc; > > + struct altera_gpio_chip *chip; > > + unsigned long flags; > > + unsigned int gpio_ddr; > > + > > + mm_gc = to_of_mm_gpio_chip(gc); > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > + > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > + /* Set pin as input, assumes software controlled IP */ > > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > > + gpio_ddr &= ~BIT(offset); > > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > + > > + return 0; > > +} > > + > > +static int altera_gpio_direction_output(struct gpio_chip *gc, > > + unsigned offset, int value) > > +{ > > + struct of_mm_gpio_chip *mm_gc; > > + struct altera_gpio_chip *chip; > > + unsigned long flags; > > + unsigned int data_reg, gpio_ddr; > > + > > + mm_gc = to_of_mm_gpio_chip(gc); > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > + > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > + /* Sets the GPIO value */ > > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > > + if (value) > > + data_reg |= BIT(offset); > > + else > > + data_reg &= ~BIT(offset); > > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > > + > > + /* Set pin as output, assumes software controlled IP */ > > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > > + gpio_ddr |= BIT(offset); > > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > + > > + return 0; > > +} > > These calls seem like pretty vanilla generic GPIO functions. > Use GENERIC_GPIO with bgpio_init() and override the default > functions only when really needed. > > See e.g. drivers/gpio/gpio-74xx-mmio.c > OK, I'll update this. > > +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, > > + irq_hw_number_t hw_irq_num) > > +{ > > + irq_set_chip_data(irq, h->host_data); > > + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops altera_gpio_irq_ops = { > > + .map = altera_gpio_irq_map, > > + .xlate = irq_domain_xlate_onecell, > > +}; > > This looks like some leftover garbage. You don't need them with > GPIOLIB_IRQCHIP so delete these two. > OK > > +static int altera_gpio_remove(struct platform_device *pdev) > > +{ > > + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev); > > + > > + gpiochip_remove(&altera_gc->mmchip.gc); > > + > > + irq_set_handler_data(altera_gc->mapped_irq, NULL); > > + irq_set_chained_handler(altera_gc->mapped_irq, NULL); > > These two calls should not be needed either. > OK > Yours, > Linus Walleij Regards, Tien Hock Loh -- 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
On Thu, 2015-02-05 at 18:54 -0800, Tien Hock Loh wrote: > On Wed, 2015-01-14 at 10:58 +0100, Linus Walleij wrote: > > On Wed, Dec 24, 2014 at 9:22 AM, <thloh@altera.com> wrote: > > > > > From: Tien Hock Loh <thloh@altera.com> > > > > > > Adds a new driver for Altera soft GPIO IP. The driver is able to > > > do read/write and allows GPIO to be a interrupt controller. > > > > > > Tested on Altera GHRD on interrupt handling and IO. > > > > > > Signed-off-by: Tien Hock Loh <thloh@altera.com> > > > > (...) > > > +config GPIO_ALTERA > > > + tristate "Altera GPIO" > > > + depends on OF_GPIO > > > > select GPIOLIB_IRQCHIP > > > > Also, I think (see below) > > > > select GENERIC_GPIO > > > > > +#include <linux/gpio.h> > > > > #include <linux/gpio/driver.h> > > > > should be just fine instead of this old header. > > > > > +#include <linux/init.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/io.h> > > > +#include <linux/irq.h> > > > > Should not be needed. > > > > > +#include <linux/irqchip/chained_irq.h> > > > +#include <linux/irqdomain.h> > > > > None of these should be needed. > > > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/of_irq.h> > > > +#include <linux/of_gpio.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/slab.h> > > > > #include <linux/basic_mmio_gpio.h> > > > > with GENERIC_GPIO (see below). > > OK > > > > + > > > +#define ALTERA_GPIO_MAX_NGPIO 32 > > > +#define ALTERA_GPIO_DATA 0x0 > > > +#define ALTERA_GPIO_DIR 0x4 > > > +#define ALTERA_GPIO_IRQ_MASK 0x8 > > > +#define ALTERA_GPIO_EDGE_CAP 0xc > > > + > > > +/** > > > +* struct altera_gpio_chip > > > +* @mmchip : memory mapped chip structure. > > > +* @gpio_lock : synchronization lock so that new irq/set/get requests > > > + will be blocked until the current one completes. > > > +* @interrupt_trigger : specifies the hardware configured IRQ trigger type > > > + (rising, falling, both, high) > > > +* @mapped_irq : kernel mapped irq number. > > > +*/ > > > +struct altera_gpio_chip { > > > + struct of_mm_gpio_chip mmchip; > > > + spinlock_t gpio_lock; > > > + int interrupt_trigger; > > > + int mapped_irq; > > > +}; > > > + > > > +static void altera_gpio_irq_unmask(struct irq_data *d) > > > +{ > > > + struct altera_gpio_chip *altera_gc; > > > + struct of_mm_gpio_chip *mm_gc; > > > + unsigned long flags; > > > + u32 intmask; > > > + > > > + altera_gc = irq_data_get_irq_chip_data(d); > > > + mm_gc = &altera_gc->mmchip; > > > + > > > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > > > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > > + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */ > > > + intmask |= BIT(irqd_to_hwirq(d)); > > > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > > > +} > > > + > > > +static void altera_gpio_irq_mask(struct irq_data *d) > > > +{ > > > + struct altera_gpio_chip *altera_gc; > > > + struct of_mm_gpio_chip *mm_gc; > > > + unsigned long flags; > > > + u32 intmask; > > > + > > > + altera_gc = irq_data_get_irq_chip_data(d); > > > + mm_gc = &altera_gc->mmchip; > > > + > > > + spin_lock_irqsave(&altera_gc->gpio_lock, flags); > > > + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > > + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */ > > > + intmask &= ~BIT(irqd_to_hwirq(d)); > > > + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > > > + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); > > > +} > > > + > > > +static int altera_gpio_irq_set_type(struct irq_data *d, > > > + unsigned int type) > > > +{ > > > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > > > + > > > + altera_gc = irq_data_get_irq_chip_data(d); > > > + > > > + if (type == IRQ_TYPE_NONE) > > > + return 0; > > > + if (type == IRQ_TYPE_LEVEL_HIGH && > > > + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) > > > + return 0; > > > + if (type == IRQ_TYPE_EDGE_RISING && > > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) > > > + return 0; > > > + if (type == IRQ_TYPE_EDGE_FALLING && > > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) > > > + return 0; > > > + if (type == IRQ_TYPE_EDGE_BOTH && > > > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) > > > + return 0; > > > + > > > + return -EINVAL; > > > +} > > > > It took me a while to understand this. Write in a comment that > > this is a hardware that is synthesized for a specific trigger > > type, and that there is no way to software-configure the > > trigger type. > > > OK noted. > > > > + > > > +static unsigned int altera_gpio_irq_startup(struct irq_data *d) > > > +{ > > > + altera_gpio_irq_unmask(d); > > > + > > > + return 0; > > > +} > > > + > > > +static void altera_gpio_irq_shutdown(struct irq_data *d) > > > +{ > > > + altera_gpio_irq_mask(d); > > > +} > > > > Instead of those pointless functions just assign > > the unmask/mask functions in the vtable right below. > > > OK > > > > + > > > +static struct irq_chip altera_irq_chip = { > > > + .name = "altera-gpio", > > > + .irq_mask = altera_gpio_irq_mask, > > > + .irq_unmask = altera_gpio_irq_unmask, > > > + .irq_set_type = altera_gpio_irq_set_type, > > > + .irq_startup = altera_gpio_irq_startup, > > > + .irq_shutdown = altera_gpio_irq_shutdown, > > > > i.e. here. > > > > > +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) > > > +{ > > > + struct of_mm_gpio_chip *mm_gc; > > > + > > > + mm_gc = to_of_mm_gpio_chip(gc); > > > + > > > + return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset); > > > +} > > > + > > > +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value) > > > +{ > > > + struct of_mm_gpio_chip *mm_gc; > > > + struct altera_gpio_chip *chip; > > > + unsigned long flags; > > > + unsigned int data_reg; > > > + > > > + mm_gc = to_of_mm_gpio_chip(gc); > > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > > + > > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > > > + if (value) > > > + data_reg |= BIT(offset); > > > + else > > > + data_reg &= ~BIT(offset); > > > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > > +} > > > + > > > +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset) > > > +{ > > > + struct of_mm_gpio_chip *mm_gc; > > > + struct altera_gpio_chip *chip; > > > + unsigned long flags; > > > + unsigned int gpio_ddr; > > > + > > > + mm_gc = to_of_mm_gpio_chip(gc); > > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > > + > > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > > + /* Set pin as input, assumes software controlled IP */ > > > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > > > + gpio_ddr &= ~BIT(offset); > > > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > > + > > > + return 0; > > > +} > > > + > > > +static int altera_gpio_direction_output(struct gpio_chip *gc, > > > + unsigned offset, int value) > > > +{ > > > + struct of_mm_gpio_chip *mm_gc; > > > + struct altera_gpio_chip *chip; > > > + unsigned long flags; > > > + unsigned int data_reg, gpio_ddr; > > > + > > > + mm_gc = to_of_mm_gpio_chip(gc); > > > + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); > > > + > > > + spin_lock_irqsave(&chip->gpio_lock, flags); > > > + /* Sets the GPIO value */ > > > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > > > + if (value) > > > + data_reg |= BIT(offset); > > > + else > > > + data_reg &= ~BIT(offset); > > > + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); > > > + > > > + /* Set pin as output, assumes software controlled IP */ > > > + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); > > > + gpio_ddr |= BIT(offset); > > > + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); > > > + spin_unlock_irqrestore(&chip->gpio_lock, flags); > > > + > > > + return 0; > > > +} > > > > These calls seem like pretty vanilla generic GPIO functions. > > Use GENERIC_GPIO with bgpio_init() and override the default > > functions only when really needed. > > > > See e.g. drivers/gpio/gpio-74xx-mmio.c > > > OK, I'll update this. I just recall that I couldn't use bgpio because the number of gpio pins is configurable and may not be multiple of 8, thus I'll not be updating this to use bgpio. > > > > +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, > > > + irq_hw_number_t hw_irq_num) > > > +{ > > > + irq_set_chip_data(irq, h->host_data); > > > + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct irq_domain_ops altera_gpio_irq_ops = { > > > + .map = altera_gpio_irq_map, > > > + .xlate = irq_domain_xlate_onecell, > > > +}; > > > > This looks like some leftover garbage. You don't need them with > > GPIOLIB_IRQCHIP so delete these two. > > > OK > > > > +static int altera_gpio_remove(struct platform_device *pdev) > > > +{ > > > + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev); > > > + > > > + gpiochip_remove(&altera_gc->mmchip.gc); > > > + > > > + irq_set_handler_data(altera_gc->mapped_irq, NULL); > > > + irq_set_chained_handler(altera_gc->mapped_irq, NULL); > > > > These two calls should not be needed either. > > > OK > > > Yours, > > Linus Walleij > > Regards, > Tien Hock Loh Regards, Tien Hock Loh -- 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
On Wed, Feb 11, 2015 at 9:20 AM, Tien Hock Loh <thloh@altera.com> wrote: >> > These calls seem like pretty vanilla generic GPIO functions. >> > Use GENERIC_GPIO with bgpio_init() and override the default >> > functions only when really needed. >> > >> > See e.g. drivers/gpio/gpio-74xx-mmio.c >> > >> OK, I'll update this. > > I just recall that I couldn't use bgpio because the number of gpio pins > is configurable and may not be multiple of 8, thus I'll not be updating > this to use bgpio. It doesn't matter. ngpio will be respected by gpio_request() so you will not be able to request any of the GPIOs above the lower bits anyway. 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
diff --git a/MAINTAINERS b/MAINTAINERS index ddb9ac8..62936d5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -563,6 +563,12 @@ S: Odd Fixes L: linux-alpha@vger.kernel.org F: arch/alpha/ +ALTERA PIO DRIVER +M: Tien Hock Loh <thloh@altera.com> +L: linux-gpio@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-altera.c + ALTERA TRIPLE SPEED ETHERNET DRIVER M: Vince Bridgers <vbridger@opensource.altera.com> L: netdev@vger.kernel.org diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 633ec21..e38beff 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -156,6 +156,14 @@ config GPIO_DWAPB Say Y or M here to build support for the Synopsys DesignWare APB GPIO block. +config GPIO_ALTERA + tristate "Altera GPIO" + depends on OF_GPIO + help + Say Y or M here to build support for the Altera PIO device. + + If driver is built as a module it will be called gpio-altera. + config GPIO_IT8761E tristate "IT8761E GPIO support" depends on X86 # unconditional access to IO space. diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 81755f1..239b28b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o +obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c new file mode 100644 index 0000000..15ac8c2 --- /dev/null +++ b/drivers/gpio/gpio-altera.c @@ -0,0 +1,410 @@ +/* + * Copyright (C) 2013 Altera Corporation + * Based on gpio-mpc8xxx.c + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/gpio.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#define ALTERA_GPIO_MAX_NGPIO 32 +#define ALTERA_GPIO_DATA 0x0 +#define ALTERA_GPIO_DIR 0x4 +#define ALTERA_GPIO_IRQ_MASK 0x8 +#define ALTERA_GPIO_EDGE_CAP 0xc + +/** +* struct altera_gpio_chip +* @mmchip : memory mapped chip structure. +* @gpio_lock : synchronization lock so that new irq/set/get requests + will be blocked until the current one completes. +* @interrupt_trigger : specifies the hardware configured IRQ trigger type + (rising, falling, both, high) +* @mapped_irq : kernel mapped irq number. +*/ +struct altera_gpio_chip { + struct of_mm_gpio_chip mmchip; + spinlock_t gpio_lock; + int interrupt_trigger; + int mapped_irq; +}; + +static void altera_gpio_irq_unmask(struct irq_data *d) +{ + struct altera_gpio_chip *altera_gc; + struct of_mm_gpio_chip *mm_gc; + unsigned long flags; + u32 intmask; + + altera_gc = irq_data_get_irq_chip_data(d); + mm_gc = &altera_gc->mmchip; + + spin_lock_irqsave(&altera_gc->gpio_lock, flags); + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + /* Set ALTERA_GPIO_IRQ_MASK bit to unmask */ + intmask |= BIT(irqd_to_hwirq(d)); + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); +} + +static void altera_gpio_irq_mask(struct irq_data *d) +{ + struct altera_gpio_chip *altera_gc; + struct of_mm_gpio_chip *mm_gc; + unsigned long flags; + u32 intmask; + + altera_gc = irq_data_get_irq_chip_data(d); + mm_gc = &altera_gc->mmchip; + + spin_lock_irqsave(&altera_gc->gpio_lock, flags); + intmask = readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + /* Clear ALTERA_GPIO_IRQ_MASK bit to mask */ + intmask &= ~BIT(irqd_to_hwirq(d)); + writel(intmask, mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + spin_unlock_irqrestore(&altera_gc->gpio_lock, flags); +} + +static int altera_gpio_irq_set_type(struct irq_data *d, + unsigned int type) +{ + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); + + altera_gc = irq_data_get_irq_chip_data(d); + + if (type == IRQ_TYPE_NONE) + return 0; + if (type == IRQ_TYPE_LEVEL_HIGH && + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) + return 0; + if (type == IRQ_TYPE_EDGE_RISING && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) + return 0; + if (type == IRQ_TYPE_EDGE_FALLING && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) + return 0; + if (type == IRQ_TYPE_EDGE_BOTH && + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) + return 0; + + return -EINVAL; +} + +static unsigned int altera_gpio_irq_startup(struct irq_data *d) +{ + altera_gpio_irq_unmask(d); + + return 0; +} + +static void altera_gpio_irq_shutdown(struct irq_data *d) +{ + altera_gpio_irq_mask(d); +} + +static struct irq_chip altera_irq_chip = { + .name = "altera-gpio", + .irq_mask = altera_gpio_irq_mask, + .irq_unmask = altera_gpio_irq_unmask, + .irq_set_type = altera_gpio_irq_set_type, + .irq_startup = altera_gpio_irq_startup, + .irq_shutdown = altera_gpio_irq_shutdown, +}; + +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) +{ + struct of_mm_gpio_chip *mm_gc; + + mm_gc = to_of_mm_gpio_chip(gc); + + return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset); +} + +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value) +{ + struct of_mm_gpio_chip *mm_gc; + struct altera_gpio_chip *chip; + unsigned long flags; + unsigned int data_reg; + + mm_gc = to_of_mm_gpio_chip(gc); + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); + + spin_lock_irqsave(&chip->gpio_lock, flags); + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); + if (value) + data_reg |= BIT(offset); + else + data_reg &= ~BIT(offset); + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); + spin_unlock_irqrestore(&chip->gpio_lock, flags); +} + +static int altera_gpio_direction_input(struct gpio_chip *gc, unsigned offset) +{ + struct of_mm_gpio_chip *mm_gc; + struct altera_gpio_chip *chip; + unsigned long flags; + unsigned int gpio_ddr; + + mm_gc = to_of_mm_gpio_chip(gc); + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); + + spin_lock_irqsave(&chip->gpio_lock, flags); + /* Set pin as input, assumes software controlled IP */ + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); + gpio_ddr &= ~BIT(offset); + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); + spin_unlock_irqrestore(&chip->gpio_lock, flags); + + return 0; +} + +static int altera_gpio_direction_output(struct gpio_chip *gc, + unsigned offset, int value) +{ + struct of_mm_gpio_chip *mm_gc; + struct altera_gpio_chip *chip; + unsigned long flags; + unsigned int data_reg, gpio_ddr; + + mm_gc = to_of_mm_gpio_chip(gc); + chip = container_of(mm_gc, struct altera_gpio_chip, mmchip); + + spin_lock_irqsave(&chip->gpio_lock, flags); + /* Sets the GPIO value */ + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); + if (value) + data_reg |= BIT(offset); + else + data_reg &= ~BIT(offset); + writel(data_reg, mm_gc->regs + ALTERA_GPIO_DATA); + + /* Set pin as output, assumes software controlled IP */ + gpio_ddr = readl(mm_gc->regs + ALTERA_GPIO_DIR); + gpio_ddr |= BIT(offset); + writel(gpio_ddr, mm_gc->regs + ALTERA_GPIO_DIR); + spin_unlock_irqrestore(&chip->gpio_lock, flags); + + return 0; +} + +static void altera_gpio_irq_edge_handler(unsigned int irq, + struct irq_desc *desc) +{ + struct altera_gpio_chip *altera_gc; + struct irq_chip *chip; + struct of_mm_gpio_chip *mm_gc; + unsigned long status; + int i; + + altera_gc = irq_desc_get_handler_data(desc); + chip = irq_desc_get_chip(desc); + mm_gc = &altera_gc->mmchip; + + chained_irq_enter(chip, desc); + + while ((status = + (readl(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) & + readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) { + writel(status, mm_gc->regs + ALTERA_GPIO_EDGE_CAP); + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { + generic_handle_irq( + irq_find_mapping(altera_gc->mmchip.gc.irqdomain + , i)); + } + } + + chained_irq_exit(chip, desc); +} + + +static void altera_gpio_irq_leveL_high_handler(unsigned int irq, + struct irq_desc *desc) +{ + struct altera_gpio_chip *altera_gc; + struct irq_chip *chip; + struct of_mm_gpio_chip *mm_gc; + unsigned long status; + int i; + + altera_gc = irq_desc_get_handler_data(desc); + chip = irq_desc_get_chip(desc); + mm_gc = &altera_gc->mmchip; + + chained_irq_enter(chip, desc); + + status = readl(mm_gc->regs + ALTERA_GPIO_DATA); + status &= readl(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); + + for_each_set_bit(i, &status, mm_gc->gc.ngpio) { + generic_handle_irq( + irq_find_mapping(altera_gc->mmchip.gc.irqdomain, i)); + } + chained_irq_exit(chip, desc); +} + +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int irq, + irq_hw_number_t hw_irq_num) +{ + irq_set_chip_data(irq, h->host_data); + irq_set_chip_and_handler(irq, &altera_irq_chip, handle_level_irq); + + return 0; +} + +static const struct irq_domain_ops altera_gpio_irq_ops = { + .map = altera_gpio_irq_map, + .xlate = irq_domain_xlate_onecell, +}; + +int altera_gpio_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + int id, reg, ret; + struct altera_gpio_chip *altera_gc; + + altera_gc = devm_kzalloc(&pdev->dev, sizeof(*altera_gc), GFP_KERNEL); + + if (altera_gc == NULL) + return -ENOMEM; + + spin_lock_init(&altera_gc->gpio_lock); + + id = pdev->id; + + if (of_property_read_u32(node, "altr,ngpio", ®)) + /*By default assume full GPIO controller*/ + altera_gc->mmchip.gc.ngpio = ALTERA_GPIO_MAX_NGPIO; + else + altera_gc->mmchip.gc.ngpio = reg; + + if (altera_gc->mmchip.gc.ngpio > 32) { + dev_warn(&pdev->dev, + "ngpio is greater than %d, defaulting to %d\n", + ALTERA_GPIO_MAX_NGPIO, ALTERA_GPIO_MAX_NGPIO); + altera_gc->mmchip.gc.ngpio = ALTERA_GPIO_MAX_NGPIO; + } + + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input; + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output; + altera_gc->mmchip.gc.get = altera_gpio_get; + altera_gc->mmchip.gc.set = altera_gpio_set; + altera_gc->mmchip.gc.owner = THIS_MODULE; + altera_gc->mmchip.gc.dev = &pdev->dev; + + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip); + if (ret) { + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n"); + return ret; + } + + platform_set_drvdata(pdev, altera_gc); + + altera_gc->mapped_irq = platform_get_irq(pdev, 0); + + if (altera_gc->mapped_irq < 0) + goto skip_irq; + + if (of_property_read_u32(node, "altr,interrupt-type", ®)) { + ret = -EINVAL; + dev_err(&pdev->dev, + "altr,interrupt-type value not set in device tree\n"); + goto teardown; + } + altera_gc->interrupt_trigger = reg; + + ret = gpiochip_irqchip_add(&altera_gc->mmchip.gc, &altera_irq_chip, 0, + handle_simple_irq, IRQ_TYPE_NONE); + + if (ret) { + dev_info(&pdev->dev, "could not add irqchip\n"); + return ret; + } + + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, + &altera_irq_chip, + altera_gc->mapped_irq, + altera_gpio_irq_leveL_high_handler); + } else { + gpiochip_set_chained_irqchip(&altera_gc->mmchip.gc, + &altera_irq_chip, + altera_gc->mapped_irq, + altera_gpio_irq_edge_handler); + } + +skip_irq: + return 0; +teardown: + pr_err("%s: registration failed with status %d\n", + node->full_name, ret); + + return ret; +} + +static int altera_gpio_remove(struct platform_device *pdev) +{ + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev); + + gpiochip_remove(&altera_gc->mmchip.gc); + + irq_set_handler_data(altera_gc->mapped_irq, NULL); + irq_set_chained_handler(altera_gc->mapped_irq, NULL); + return -EIO; +} + +static const struct of_device_id altera_gpio_of_match[] = { + { .compatible = "altr,pio-1.0", }, + {}, +}; +MODULE_DEVICE_TABLE(of, altera_gpio_of_match); + +static struct platform_driver altera_gpio_driver = { + .driver = { + .name = "altera_gpio", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(altera_gpio_of_match), + }, + .probe = altera_gpio_probe, + .remove = altera_gpio_remove, +}; + +static int __init altera_gpio_init(void) +{ + return platform_driver_register(&altera_gpio_driver); +} +subsys_initcall(altera_gpio_init); + +static void __exit altera_gpio_exit(void) +{ + platform_driver_unregister(&altera_gpio_driver); +} +module_exit(altera_gpio_exit); + +MODULE_AUTHOR("Tien Hock Loh <thloh@altera.com>"); +MODULE_DESCRIPTION("Altera GPIO driver"); +MODULE_LICENSE("GPL");