gpio: Add IRQ support to ACCES 104-IDIO-16 driver
Subsystems (2)
PATCH 1/1 gpio: Add IRQ support to ACCES 104-IDIO-16 driver
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.
--- drivers/gpio/Kconfig | 6 ++- drivers/gpio/gpio-104-idio-16.c | 104 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-)
drivers/gpio/Kconfig +5 -1
@@ -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
drivers/gpio/gpio-104-idio-16.c +103 -1
166 lines not on this page · raw message
Signature · 1 line
2.4.10
Quoted 9 lines · William Breathitt Gray
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_irqmodule parameter.Signed-off-by William Breathitt Gray <[email protected]>
Overall this is looking nice.
Quoted 4 lines
config
GPIO_104_IDIO_16
tristate "ACCES 104-IDIO-16 GPIO support"
depends on X86
+ selectGPIOLIB_IRQCHIP
Thanks for using this helper library.
drivers/gpio/gpio-104-idio-16.c line 23
Quoted 4 lines
drivers/gpio/gpio-104-idio-16.c +4
-0+#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.
drivers/gpio/gpio-104-idio-16.c line 35
Quoted 3 lines
drivers/gpio/gpio-104-idio-16.c +3
-0+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.
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 124
Quoted 5 lines
drivers/gpio/gpio-104-idio-16.c +5
-0+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.
Quoted 22 lines
drivers/gpio/gpio-104-idio-16.c +22
-0+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 ;
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 156
Quoted 10 lines
drivers/gpio/gpio-104-idio-16.c +10
-0+ +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?
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 160
Quoted 3 lines
drivers/gpio/gpio-104-idio-16.c +3
-0+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.
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 180
Quoted 8 lines
drivers/gpio/gpio-104-idio-16.c +8
-0+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?
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 184
Quoted 4 lines
drivers/gpio/gpio-104-idio-16.c +4
-0+ +spin_lock (&idio16gpio->lock); + +outb (0 , idio16gpio->base +1 );
What happens here?
drivers/gpio/gpio-104-idio-16.c static int __init idio_16_probe(struct platform_device *pdev) · line 239
Quoted 2 lines
drivers/gpio/gpio-104-idio-16.c +2
-0+ 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
Quoted 3 lines · Linus Walleij
With
GPIOLIB_IRQCHIPthe <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>.
Quoted 1 line
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.
Quoted 8 lines
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 124
Quoted 5 lines
drivers/gpio/gpio-104-idio-16.c +5
-0+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?
Quoted 3 lines
Looks more like you should prefer to just update the shadow registers
idio16gpio->irq_masketc 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.
Quoted 1 line
Please do not use implicit conventions.
I'll make the comparison explicit and add a comment.
Quoted 5 lines
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?
Quoted 5 lines
drivers/gpio/gpio-104-idio-16.c static void idio_16_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 184
Quoted 3 lines
drivers/gpio/gpio-104-idio-16.c +3
-0+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.
Quoted 8 lines
drivers/gpio/gpio-104-idio-16.c static int __init idio_16_probe(struct platform_device *pdev) · line 239
Quoted 2 lines
drivers/gpio/gpio-104-idio-16.c +2
-0+ 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