| Message ID | 20151030000406.GA8699@sophia |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 30, 2015 at 1:04 AM, William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > The ACCES 104-IDIO-16 series offers Change-of-State detection interrupt > functionality; if Change-of-State detection is enabled, an interrupt is > fired off if any input line changes state (i.e. goes from low to high, > or from high to low). This patch adds support to handle these interrupts > and allows the user to mask which GPIO lines are affected. The interrupt > line number for the device may be set via the idio_16_irq module > parameter. > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> Overall this is looking nice. > config GPIO_104_IDIO_16 > tristate "ACCES 104-IDIO-16 GPIO support" > depends on X86 > + select GPIOLIB_IRQCHIP Thanks for using this helper library. > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdesc.h> > +#include <linux/irqdomain.h> With GPIOLIB_IRQCHIP the <linux/gpio/driver.h> take in so many headers that all of these are seldom needed. Try to trim it down to what is actually needed, I think only <linux/interrupt.h> should be needed. > +static unsigned idio_16_irq; > +module_param(idio_16_irq, uint, 0); > +MODULE_PARM_DESC(idio_16_irq, "ACCES 104-IDIO-16 interrupt line number"); This with specifying IRQ as module parameter feels very, very retro. Is there no way to get the portbase and IRQ from the system *at all*? Isn't things like ACPI supposed to do this... I've seen this before so I just need to ask twice. I understand if this is how you have to do it, but I wanna make sure. > +static void idio_16_irq_mask(struct irq_data *data) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + struct idio_16_gpio *const idio16gpio = to_idio16gpio(chip); > + const unsigned long GPIO_MASK = 1UL << irqd_to_hwirq(data); Why const? Why uppercase GPIO_MASK, just lowercase "mask" works fine. unsigned long mask = BIT(irqd_to_hwirq(data)); Should be equal. > + unsigned long flags; > + > + idio16gpio->irq_mask &= ~GPIO_MASK; > + > + if (!idio16gpio->irq_mask) { > + spin_lock_irqsave(&idio16gpio->lock, flags); > + > + outb(0, idio16gpio->base + 2); > + > + spin_unlock_irqrestore(&idio16gpio->lock, flags); > + } > +} > + > +static void idio_16_irq_unmask(struct irq_data *data) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + struct idio_16_gpio *const idio16gpio = to_idio16gpio(chip); > + const unsigned long GPIO_MASK = 1UL << irqd_to_hwirq(data); > + const unsigned long PREV_IRQ_MASK = idio16gpio->irq_mask; > + unsigned long flags; > + > + idio16gpio->irq_mask |= GPIO_MASK; > + > + if (!PREV_IRQ_MASK) { > + spin_lock_irqsave(&idio16gpio->lock, flags); > + > + outb(0, idio16gpio->base + 1); > + inb(idio16gpio->base + 2); > + > + spin_unlock_irqrestore(&idio16gpio->lock, flags); > + } > +} Looks more like you should prefer to just update the shadow registers idio16gpio->irq_mask etc in the mask/unmask calls, and write to the ports in the .irq_bus_lock() and .irq_bus_sync_unlock() callbacks. Thoughts on this? > +static int idio_16_irq_set_type(struct irq_data *data, unsigned flow_type) > +{ > + if (flow_type && (flow_type & IRQ_TYPE_EDGE_BOTH) != IRQ_TYPE_EDGE_BOTH) Please do not use implicit conventions. if (flow_type != IRQ_TYPE_NODE && (... is better. Then I understand what is going on. What is clear is that either no type is specified or both edges, or it doesn't work. You could add a comment on this, that is helpful. > +static irqreturn_t idio_16_irq_handler(int irq, void *dev_id) > +{ > + struct idio_16_gpio *const idio16gpio = dev_id; > + struct gpio_chip *const chip = &idio16gpio->chip; > + int gpio; > + > + for_each_set_bit(gpio, &idio16gpio->irq_mask, chip->ngpio) > + generic_handle_irq(irq_find_mapping(chip->irqdomain, gpio)); To me it looks very strange when you start handling all IRQs without actually reading any status register. Now you fire off *every* unmasked IRQ meaning their IRQ handlers will be called unconditionally. Care to explain how this works? > + > + spin_lock(&idio16gpio->lock); > + > + outb(0, idio16gpio->base + 1); What happens here? > + err = gpiochip_irqchip_add(&idio16gpio->chip, &idio_16_irqchip, 0, > + handle_simple_irq, IRQ_TYPE_NONE); I think you want to specify handle_edge_irq() and for that to work you also need to implement .irq_ack() on the irqchip. You just specified above that this chip only handles edge IRQs, so it makes no sense that you do not use handle_edge_irq. 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 10/30/2015 10:36 AM, Linus Walleij wrote: > With GPIOLIB_IRQCHIP the <linux/gpio/driver.h> take in so many headers that > all of these are seldom needed. Try to trim it down to what is actually needed, > I think only <linux/interrupt.h> should be needed. I checked out <linux/gpio/driver.h> and it looks like it unconditionally includes <linux/irq.h> and <linux/irqdomain.h>. I'll remove those includes from my patch and leave <linux/interrupt.h> and <linux/irqdesc.h>. > This with specifying IRQ as module parameter feels very, very retro. You are absolutely correct: it is very retro. PC/104 was released in the late '80s and lacks the functionality to probe for device configurations. Both the base address and IRQ line are configured via physical jumpers on the ACCES 104-IDIO-16 board. It's essentially a living relic, but it's still popular in the embedded computing industry so I'd like to support it. >> +static void idio_16_irq_mask(struct irq_data *data) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); >> + struct idio_16_gpio *const idio16gpio = to_idio16gpio(chip); >> + const unsigned long GPIO_MASK = 1UL << irqd_to_hwirq(data); > > Why const? Why uppercase GPIO_MASK, just lowercase "mask" > works fine. I'll rename the symbol to lowercase "mask" since I can see how uppercase can lead to confusion between variables and preprocessor macros. I'm hesitant to remove the const. Upon initialization, this variable is constant for the entirety of its life, and the const qualifier shows that intent to both compilers and future readers. What is your justification for removing it? > Looks more like you should prefer to just update the shadow registers > idio16gpio->irq_mask etc in the mask/unmask calls, and write to the > ports in the .irq_bus_lock() and .irq_bus_sync_unlock() callbacks. I'm somewhat new to the irqchip API so correct me if I'm wrong. The irq_bus_lock and irq_bus_sync_unlock callbacks are used for locking/unlocking access to slow bus (i2c) chips. The port I/O operations I perform in the irq_mask and irq_unmask callbacks are to respectively disable and enable interrupts. It appears more appropriate to leave those port operations in irq_mask/irq_unmask since I want to disable/enable interrupts, rather than to lock out access to the device. > Please do not use implicit conventions. I'll make the comparison explicit and add a comment. > To me it looks very strange when you start handling all IRQs without > actually reading any status register. Now you fire off *every* unmasked > IRQ meaning their IRQ handlers will be called unconditionally. > > Care to explain how this works? When enabled, the change-of-state detection interrupt provided by the ACCES 104-IDIO-16 will fire off if *any* input line goes from low to high or high to low. Unfortunately, the device provides no hardware means to differentiate which input caused the interrupt to fire. My software solution is to maintain a mask (irq_mask) which tracks which GPIO lines have been selected by the user to be affected the interrupt (via the set_type callback) -- furthermore, disabling interrupts entirely when no GPIO are any longer selected by the user. The downside to this approach is that if multiple GPIO lines are selected, then all of those GPIO IRQ handlers are called; I don't believe this can be avoided in software since there is no hardware differentiation. What are your thoughts? >> + spin_lock(&idio16gpio->lock); >> + >> + outb(0, idio16gpio->base + 1); > > What happens here? This is an IRQ acknowledgment sent to the device in order to clear the pending interrupt. I'll move it to the more appropriate irq_ack callback. >> + err = gpiochip_irqchip_add(&idio16gpio->chip, &idio_16_irqchip, 0, >> + handle_simple_irq, IRQ_TYPE_NONE); > > I think you want to specify handle_edge_irq() and for that to work you also > need to implement .irq_ack() on the irqchip. > > You just specified above that this chip only handles edge IRQs, so it makes > no sense that you do not use handle_edge_irq. Agreed. I'll update my patch to provide an implementation for irq_ack and to pass handle_edge_irq. Sincerely, William Breathitt Gray -- 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/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index dbb171d..8635f1a 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -694,8 +694,12 @@ menu "ISA GPIO drivers" config GPIO_104_IDIO_16 tristate "ACCES 104-IDIO-16 GPIO support" depends on X86 + select GPIOLIB_IRQCHIP help - Enables GPIO support for the ACCES 104-IDIO-16 family. + Enables GPIO support for the ACCES 104-IDIO-16 family. The base port + address for the device may be set via the idio_16_base module + parameter. The interrupt line number for the device may be set via the + idio_16_irq module parameter. endmenu diff --git a/drivers/gpio/gpio-104-idio-16.c b/drivers/gpio/gpio-104-idio-16.c index 5400d7d..d1298dc 100644 --- a/drivers/gpio/gpio-104-idio-16.c +++ b/drivers/gpio/gpio-104-idio-16.c @@ -11,11 +11,16 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. */ +#include <linux/bitops.h> #include <linux/device.h> #include <linux/errno.h> #include <linux/gpio/driver.h> #include <linux/io.h> #include <linux/ioport.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdesc.h> +#include <linux/irqdomain.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/moduleparam.h> @@ -25,20 +30,27 @@ static unsigned idio_16_base; module_param(idio_16_base, uint, 0); MODULE_PARM_DESC(idio_16_base, "ACCES 104-IDIO-16 base address"); +static unsigned idio_16_irq; +module_param(idio_16_irq, uint, 0); +MODULE_PARM_DESC(idio_16_irq, "ACCES 104-IDIO-16 interrupt line number"); /** * struct idio_16_gpio - GPIO device private data structure * @chip: instance of the gpio_chip - * @lock: synchronization lock to prevent gpio_set race conditions + * @lock: synchronization lock to prevent I/O race conditions + * @irq_mask: I/O bits affected by interrupts * @base: base port address of the GPIO device * @extent: extent of port address region of the GPIO device + * @irq: Interrupt line number * @out_state: output bits state */ struct idio_16_gpio { struct gpio_chip chip; spinlock_t lock; + unsigned long irq_mask; unsigned base; unsigned extent; + unsigned irq; unsigned out_state; }; @@ -105,6 +117,77 @@ static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) spin_unlock_irqrestore(&idio16gpio->lock, flags); } +static void idio_16_irq_mask(struct irq_data *data) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + struct idio_16_gpio *const idio16gpio = to_idio16gpio(chip); + const unsigned long GPIO_MASK = 1UL << irqd_to_hwirq(data); + unsigned long flags; + + idio16gpio->irq_mask &= ~GPIO_MASK; + + if (!idio16gpio->irq_mask) { + spin_lock_irqsave(&idio16gpio->lock, flags); + + outb(0, idio16gpio->base + 2); + + spin_unlock_irqrestore(&idio16gpio->lock, flags); + } +} + +static void idio_16_irq_unmask(struct irq_data *data) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + struct idio_16_gpio *const idio16gpio = to_idio16gpio(chip); + const unsigned long GPIO_MASK = 1UL << irqd_to_hwirq(data); + const unsigned long PREV_IRQ_MASK = idio16gpio->irq_mask; + unsigned long flags; + + idio16gpio->irq_mask |= GPIO_MASK; + + if (!PREV_IRQ_MASK) { + spin_lock_irqsave(&idio16gpio->lock, flags); + + outb(0, idio16gpio->base + 1); + inb(idio16gpio->base + 2); + + spin_unlock_irqrestore(&idio16gpio->lock, flags); + } +} + +static int idio_16_irq_set_type(struct irq_data *data, unsigned flow_type) +{ + if (flow_type && (flow_type & IRQ_TYPE_EDGE_BOTH) != IRQ_TYPE_EDGE_BOTH) + return -EINVAL; + + return 0; +} + +static struct irq_chip idio_16_irqchip = { + .name = "104-idio-16", + .irq_mask = idio_16_irq_mask, + .irq_unmask = idio_16_irq_unmask, + .irq_set_type = idio_16_irq_set_type +}; + +static irqreturn_t idio_16_irq_handler(int irq, void *dev_id) +{ + struct idio_16_gpio *const idio16gpio = dev_id; + struct gpio_chip *const chip = &idio16gpio->chip; + int gpio; + + for_each_set_bit(gpio, &idio16gpio->irq_mask, chip->ngpio) + generic_handle_irq(irq_find_mapping(chip->irqdomain, gpio)); + + spin_lock(&idio16gpio->lock); + + outb(0, idio16gpio->base + 1); + + spin_unlock(&idio16gpio->lock); + + return IRQ_HANDLED; +} + static int __init idio_16_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -113,6 +196,7 @@ static int __init idio_16_probe(struct platform_device *pdev) const unsigned BASE = idio_16_base; const unsigned EXTENT = 8; + const unsigned IRQ = idio_16_irq; const char *const NAME = dev_name(dev); idio16gpio = devm_kzalloc(dev, sizeof(*idio16gpio), GFP_KERNEL); @@ -138,6 +222,7 @@ static int __init idio_16_probe(struct platform_device *pdev) idio16gpio->chip.set = idio_16_gpio_set; idio16gpio->base = BASE; idio16gpio->extent = EXTENT; + idio16gpio->irq = IRQ; idio16gpio->out_state = 0xFFFF; spin_lock_init(&idio16gpio->lock); @@ -150,8 +235,24 @@ static int __init idio_16_probe(struct platform_device *pdev) goto err_gpio_register; } + err = gpiochip_irqchip_add(&idio16gpio->chip, &idio_16_irqchip, 0, + handle_simple_irq, IRQ_TYPE_NONE); + if (err) { + dev_err(dev, "Could not add irqchip (%d)\n", err); + goto err_gpiochip_irqchip_add; + } + + err = request_irq(IRQ, idio_16_irq_handler, 0, NAME, idio16gpio); + if (err) { + dev_err(dev, "IRQ handler registering failed (%d)\n", err); + goto err_request_irq; + } + return 0; +err_request_irq: +err_gpiochip_irqchip_add: + gpiochip_remove(&idio16gpio->chip); err_gpio_register: release_region(BASE, EXTENT); err_lock_io_port: @@ -162,6 +263,7 @@ static int idio_16_remove(struct platform_device *pdev) { struct idio_16_gpio *const idio16gpio = platform_get_drvdata(pdev); + free_irq(idio16gpio->irq, idio16gpio); gpiochip_remove(&idio16gpio->chip); release_region(idio16gpio->base, idio16gpio->extent);
The ACCES 104-IDIO-16 series offers Change-of-State detection interrupt functionality; if Change-of-State detection is enabled, an interrupt is fired off if any input line changes state (i.e. goes from low to high, or from high to low). This patch adds support to handle these interrupts and allows the user to mask which GPIO lines are affected. The interrupt line number for the device may be set via the idio_16_irq module parameter. Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- drivers/gpio/Kconfig | 6 ++- drivers/gpio/gpio-104-idio-16.c | 104 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-)