The regmap API supports IO port accessors so we can take advantage of
regmap abstractions rather than handling access to the device registers
directly in the driver.
For the 104-dio-48e we have the following IRQ registers (0xB and 0xF):
Base Address +B (Write): Enable Interrupt
Base Address +B (Read): Disable Interrupt
Base Address +F (Read/Write): Clear Interrupt
Any write to 0xB will enable interrupts, while any read will disable
interrupts. Interrupts are cleared by a read or any write to 0xF.
There's no IRQ status register, so software has to assume that if an
interrupt is raised then it was for the 104-DIO-48E device.
Cc: Mark Brown <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/gpio/Kconfig | 1 +
drivers/gpio/gpio-104-dio-48e.c | 274 ++++++++++++++++----------------
2 files changed, 135 insertions(+), 140 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ec7cfd4f52b1..b62bef4e563d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -845,6 +845,7 @@ config GPIO_104_DIO_48E
tristate "ACCES 104-DIO-48E GPIO support"
depends on PC104
select ISA_BUS_API
+ select REGMAP_IRQ
select GPIOLIB_IRQCHIP
select GPIO_I8255
help
diff --git a/drivers/gpio/gpio-104-dio-48e.c b/drivers/gpio/gpio-104-dio-48e.c
index 7b8829c8e423..fcee3dc81902 100644
--- a/drivers/gpio/gpio-104-dio-48e.c
+++ b/drivers/gpio/gpio-104-dio-48e.c
@@ -8,17 +8,15 @@
*/
#include <linux/bits.h>
#include <linux/device.h>
-#include <linux/errno.h>
+#include <linux/err.h>
#include <linux/gpio/driver.h>
-#include <linux/io.h>
#include <linux/ioport.h>
-#include <linux/interrupt.h>
-#include <linux/irqdesc.h>
+#include <linux/irq.h>
#include <linux/isa.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
-#include <linux/spinlock.h>
+#include <linux/regmap.h>
#include <linux/types.h>
#include "gpio-i8255.h"
@@ -38,46 +36,30 @@ static unsigned int num_irq;
module_param_hw_array(irq, uint, irq, &num_irq, 0);
MODULE_PARM_DESC(irq, "ACCES 104-DIO-48E interrupt line numbers");
+#define DIO48E_ENABLE_INTERRUPT 0xB
+#define DIO48E_DISABLE_INTERRUPT DIO48E_ENABLE_INTERRUPT
+#define DIO48E_CLEAR_INTERRUPT 0xF
+
#define DIO48E_NUM_PPI 2
/**
* struct dio48e_reg - device register structure
* @ppi: Programmable Peripheral Interface groups
- * @enable_buffer: Enable/Disable Buffer groups
- * @unused1: Unused
- * @enable_interrupt: Write: Enable Interrupt
- * Read: Disable Interrupt
- * @unused2: Unused
- * @enable_counter: Write: Enable Counter/Timer Addressing
- * Read: Disable Counter/Timer Addressing
- * @unused3: Unused
- * @clear_interrupt: Clear Interrupt
*/
struct dio48e_reg {
struct i8255 ppi[DIO48E_NUM_PPI];
- u8 enable_buffer[DIO48E_NUM_PPI];
- u8 unused1;
- u8 enable_interrupt;
- u8 unused2;
- u8 enable_counter;
- u8 unused3;
- u8 clear_interrupt;
};
/**
* struct dio48e_gpio - GPIO device private data structure
* @chip: instance of the gpio_chip
* @ppi_state: PPI device states
- * @lock: synchronization lock to prevent I/O race conditions
* @reg: I/O address offset for the device registers
- * @irq_mask: I/O bits affected by interrupts
*/
struct dio48e_gpio {
struct gpio_chip chip;
struct i8255_state ppi_state[DIO48E_NUM_PPI];
- raw_spinlock_t lock;
struct dio48e_reg __iomem *reg;
- unsigned char irq_mask;
};
static int dio48e_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -144,106 +126,95 @@ static void dio48e_gpio_set_multiple(struct gpio_chip *chip,
bits, chip->ngpio);
}
-static void dio48e_irq_ack(struct irq_data *data)
-{
-}
-
-static void dio48e_irq_mask(struct irq_data *data)
-{
- struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
- const unsigned long offset = irqd_to_hwirq(data);
- unsigned long flags;
-
- /* only bit 3 on each respective Port C supports interrupts */
- if (offset != 19 && offset != 43)
- return;
-
- raw_spin_lock_irqsave(&dio48egpio->lock, flags);
-
- if (offset == 19)
- dio48egpio->irq_mask &= ~BIT(0);
- else
- dio48egpio->irq_mask &= ~BIT(1);
- gpiochip_disable_irq(chip, offset);
-
- if (!dio48egpio->irq_mask)
- /* disable interrupts */
- ioread8(&dio48egpio->reg->enable_interrupt);
-
- raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-}
-
-static void dio48e_irq_unmask(struct irq_data *data)
-{
- struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
- const unsigned long offset = irqd_to_hwirq(data);
- unsigned long flags;
-
- /* only bit 3 on each respective Port C supports interrupts */
- if (offset != 19 && offset != 43)
- return;
-
- raw_spin_lock_irqsave(&dio48egpio->lock, flags);
+static const struct regmap_range dio48e_wr_ranges[] = {
+ regmap_reg_range(0x0, 0x9), regmap_reg_range(0xB, 0xB),
+ regmap_reg_range(0xD, 0xD), regmap_reg_range(0xF, 0xF),
+};
+static const struct regmap_range dio48e_rd_ranges[] = {
+ regmap_reg_range(0x0, 0x2), regmap_reg_range(0x4, 0x6),
+ regmap_reg_range(0xB, 0xB), regmap_reg_range(0xD, 0xD),
+ regmap_reg_range(0xF, 0xF),
+};
+static const struct regmap_range dio48e_volatile_ranges[] = {
+ i8255_volatile_regmap_range(0x0), i8255_volatile_regmap_range(0x4),
+ regmap_reg_range(0xB, 0xB), regmap_reg_range(0xD, 0xD),
+ regmap_reg_range(0xF, 0xF),
+};
+static const struct regmap_range dio48e_precious_ranges[] = {
+ regmap_reg_range(0xB, 0xB), regmap_reg_range(0xD, 0xD),
+ regmap_reg_range(0xF, 0xF),
+};
+static const struct regmap_access_table dio48e_wr_table = {
+ .yes_ranges = dio48e_wr_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dio48e_wr_ranges),
+};
+static const struct regmap_access_table dio48e_rd_table = {
+ .yes_ranges = dio48e_rd_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dio48e_rd_ranges),
+};
+static const struct regmap_access_table dio48e_volatile_table = {
+ .yes_ranges = dio48e_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dio48e_volatile_ranges),
+};
+static const struct regmap_access_table dio48e_precious_table = {
+ .yes_ranges = dio48e_precious_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dio48e_precious_ranges),
+};
+static const struct regmap_config dio48e_regmap_config = {
+ .reg_bits = 8,
+ .reg_stride = 1,
+ .val_bits = 8,
+ .io_port = true,
+ .max_register = 0xF,
+ .wr_table = &dio48e_wr_table,
+ .rd_table = &dio48e_rd_table,
+ .volatile_table = &dio48e_volatile_table,
+ .precious_table = &dio48e_precious_table,
+ .cache_type = REGCACHE_FLAT,
+};
- if (!dio48egpio->irq_mask) {
- /* enable interrupts */
- iowrite8(0x00, &dio48egpio->reg->clear_interrupt);
- iowrite8(0x00, &dio48egpio->reg->enable_interrupt);
+/* only bit 3 on each respective Port C supports interrupts */
+#define DIO48E_REGMAP_IRQ(_ppi) \
+ [19 + (_ppi) * 24] = { \
+ .mask = BIT(_ppi), \
+ .type = { .types_supported = IRQ_TYPE_EDGE_RISING, }, \
}
- gpiochip_enable_irq(chip, offset);
- if (offset == 19)
- dio48egpio->irq_mask |= BIT(0);
- else
- dio48egpio->irq_mask |= BIT(1);
-
- raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-}
-
-static int dio48e_irq_set_type(struct irq_data *data, unsigned int flow_type)
-{
- const unsigned long offset = irqd_to_hwirq(data);
-
- /* only bit 3 on each respective Port C supports interrupts */
- if (offset != 19 && offset != 43)
- return -EINVAL;
-
- if (flow_type != IRQ_TYPE_NONE && flow_type != IRQ_TYPE_EDGE_RISING)
- return -EINVAL;
-
- return 0;
-}
-
-static const struct irq_chip dio48e_irqchip = {
- .name = "104-dio-48e",
- .irq_ack = dio48e_irq_ack,
- .irq_mask = dio48e_irq_mask,
- .irq_unmask = dio48e_irq_unmask,
- .irq_set_type = dio48e_irq_set_type,
- .flags = IRQCHIP_IMMUTABLE,
- GPIOCHIP_IRQ_RESOURCE_HELPERS,
+static const struct regmap_irq dio48e_regmap_irqs[] = {
+ DIO48E_REGMAP_IRQ(0), DIO48E_REGMAP_IRQ(1),
};
-static irqreturn_t dio48e_irq_handler(int irq, void *dev_id)
+static int dio48e_handle_mask_sync(struct regmap *const map, const int index,
+ const unsigned int mask_buf_def,
+ const unsigned int mask_buf,
+ void *const irq_drv_data)
{
- struct dio48e_gpio *const dio48egpio = dev_id;
- struct gpio_chip *const chip = &dio48egpio->chip;
- const unsigned long irq_mask = dio48egpio->irq_mask;
- unsigned long gpio;
-
- for_each_set_bit(gpio, &irq_mask, 2)
- generic_handle_domain_irq(chip->irq.domain,
- 19 + gpio*24);
-
- raw_spin_lock(&dio48egpio->lock);
-
- iowrite8(0x00, &dio48egpio->reg->clear_interrupt);
+ unsigned int *const irq_mask = irq_drv_data;
+ const unsigned int prev_mask = *irq_mask;
+ const unsigned int all_masked = 0x3;
+ unsigned int val;
+ int err;
- raw_spin_unlock(&dio48egpio->lock);
+ /* exit early if no change since the previous mask */
+ if (mask_buf == prev_mask)
+ return 0;
+
+ /* remember the current mask for the next mask sync */
+ *irq_mask = mask_buf;
+
+ /* if all previously masked, enable interrupts when unmasking */
+ if (prev_mask == all_masked) {
+ err = regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00);
+ if (err)
+ return err;
+ /* if all are currently masked, disable interrupts */
+ } else if (mask_buf == all_masked) {
+ err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
+ if (err)
+ return err;
+ }
- return IRQ_HANDLED;
+ return 0;
}
#define DIO48E_NGPIO 48
@@ -295,8 +266,13 @@ static int dio48e_probe(struct device *dev, unsigned int id)
{
struct dio48e_gpio *dio48egpio;
const char *const name = dev_name(dev);
- struct gpio_irq_chip *girq;
+ void __iomem *regs;
+ struct regmap *map;
+ unsigned int val;
int err;
+ struct regmap_irq_chip *chip;
+ unsigned int irq_mask;
+ struct regmap_irq_chip_data *chip_data;
dio48egpio = devm_kzalloc(dev, sizeof(*dio48egpio), GFP_KERNEL);
if (!dio48egpio)
@@ -308,10 +284,46 @@ static int dio48e_probe(struct device *dev, unsigned int id)
return -EBUSY;
}
- dio48egpio->reg = devm_ioport_map(dev, base[id], DIO48E_EXTENT);
- if (!dio48egpio->reg)
+ regs = devm_ioport_map(dev, base[id], DIO48E_EXTENT);
+ if (!regs)
+ return -ENOMEM;
+ dio48egpio->reg = regs;
+
+ map = devm_regmap_init_mmio(dev, regs, &dio48e_regmap_config);
+ if (IS_ERR(map))
+ return PTR_ERR(map);
+
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
return -ENOMEM;
+ chip->irq_drv_data = devm_kzalloc(dev, sizeof(irq_mask), GFP_KERNEL);
+ if (!chip->irq_drv_data)
+ return -ENOMEM;
+
+ chip->name = name;
+ /* No IRQ status register so use CLEAR_INTERRUPT register instead */
+ chip->status_base = DIO48E_CLEAR_INTERRUPT;
+ chip->mask_base = DIO48E_ENABLE_INTERRUPT;
+ chip->clear_on_unmask = true;
+ chip->status_invert = true;
+ chip->num_regs = 1;
+ chip->irqs = dio48e_regmap_irqs;
+ chip->num_irqs = ARRAY_SIZE(dio48e_regmap_irqs);
+ chip->handle_mask_sync = dio48e_handle_mask_sync;
+
+ /* Initialize device interrupt state */
+ err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
+ if (err)
+ return err;
+
+ err = devm_regmap_add_irq_chip(dev, map, irq[id], 0, 0, chip,
+ &chip_data);
+ if (err) {
+ dev_err(dev, "IRQ registration failed (%d)\n", err);
+ return err;
+ }
+
dio48egpio->chip.label = name;
dio48egpio->chip.parent = dev;
dio48egpio->chip.owner = THIS_MODULE;
@@ -326,18 +338,6 @@ static int dio48e_probe(struct device *dev, unsigned int id)
dio48egpio->chip.set = dio48e_gpio_set;
dio48egpio->chip.set_multiple = dio48e_gpio_set_multiple;
- girq = &dio48egpio->chip.irq;
- gpio_irq_chip_set_chip(girq, &dio48e_irqchip);
- /* This will let us handle the parent IRQ in the driver */
- girq->parent_handler = NULL;
- girq->num_parents = 0;
- girq->parents = NULL;
- girq->default_type = IRQ_TYPE_NONE;
- girq->handler = handle_edge_irq;
- girq->init_hw = dio48e_irq_init_hw;
-
- raw_spin_lock_init(&dio48egpio->lock);
-
i8255_state_init(dio48egpio->ppi_state, DIO48E_NUM_PPI);
dio48e_init_ppi(dio48egpio->reg->ppi, dio48egpio->ppi_state);
@@ -347,14 +347,8 @@ static int dio48e_probe(struct device *dev, unsigned int id)
return err;
}
- err = devm_request_irq(dev, irq[id], dio48e_irq_handler, 0, name,
- dio48egpio);
- if (err) {
- dev_err(dev, "IRQ handler registering failed (%d)\n", err);
- return err;
- }
-
- return 0;
+ return gpiochip_irqchip_add_domain(&dio48egpio->chip,
+ regmap_irq_get_domain(chip_data));
}
static struct isa_driver dio48e_driver = {
--
2.38.1
On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray wrote:
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver.
>
> For the 104-dio-48e we have the following IRQ registers (0xB and 0xF):
>
> Base Address +B (Write): Enable Interrupt
> Base Address +B (Read): Disable Interrupt
> Base Address +F (Read/Write): Clear Interrupt
>
> Any write to 0xB will enable interrupts, while any read will disable
> interrupts. Interrupts are cleared by a read or any write to 0xF.
> There's no IRQ status register, so software has to assume that if an
> interrupt is raised then it was for the 104-DIO-48E device.
...
> +/* only bit 3 on each respective Port C supports interrupts */
> +#define DIO48E_REGMAP_IRQ(_ppi) \
> + [19 + (_ppi) * 24] = { \
> + .mask = BIT(_ppi), \
> + .type = { .types_supported = IRQ_TYPE_EDGE_RISING, }, \
When {} on a single line, the trailing comma is not needed.
.type = { .types_supported = IRQ_TYPE_EDGE_RISING }, \
would work as well.
A nit: I would put \ on the same column by using TABs before each of them.
> }
...
> + /* if all previously masked, enable interrupts when unmasking */
> + if (prev_mask == all_masked) {
> + err = regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00);
> + if (err)
> + return err;
> + /* if all are currently masked, disable interrupts */
> + } else if (mask_buf == all_masked) {
> + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
> + if (err)
> + return err;
> + }
Haven't looked at the rest of the series, but if there is nothing with this
code piece, the above can be optimized to
if (prev_mask == all_masked)
return regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00);
if (mask_buf == all_masked)
return regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
...
> + /* Initialize device interrupt state */
> + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
> + if (err)
> + return err;
Use ->init_hw() callback for this.
...
> + err = devm_regmap_add_irq_chip(dev, map, irq[id], 0, 0, chip,
> + &chip_data);
I would leave this on one line. It's only 82.
> + if (err) {
> + dev_err(dev, "IRQ registration failed (%d)\n", err);
> + return err;
> + }
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 23, 2022 at 05:01:53PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray wrote:
> > + /* Initialize device interrupt state */
> > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
> > + if (err)
> > + return err;
>
> Use ->init_hw() callback for this.
In a subsequent patch 7/9 we remove direct gpio_chip registration in
favor of the i8255 library registration via gpio_regmap. It doesn't look
like gpio_regmap_register() sets the init_hw() callback.
Michael, do you see any issues if I introduce init_hw() to
gpio_regmap_config? Or do you think this IRQ initialization belongs
somewhere else?
William Breathitt Gray
Hi,
[sorry this mail was just delivered now, although it seems
to be sent last Tuesday.]
Am 2022-11-22 11:29, schrieb William Breathitt Gray:
> On Wed, Nov 23, 2022 at 05:01:53PM +0200, Andy Shevchenko wrote:
>> On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray
>> wrote:
>> > + /* Initialize device interrupt state */
>> > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
>> > + if (err)
>> > + return err;
>>
>> Use ->init_hw() callback for this.
>
> In a subsequent patch 7/9 we remove direct gpio_chip registration in
> favor of the i8255 library registration via gpio_regmap. It doesn't
> look
> like gpio_regmap_register() sets the init_hw() callback.
>
> Michael, do you see any issues if I introduce init_hw() to
> gpio_regmap_config? Or do you think this IRQ initialization belongs
> somewhere else?
Something like the following?
gpiochip->init_hw = config.irq_init_hw;
gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
seems wrong.
Maybe make gpio-regmap call it on its own? But really we just connect
the regmap-irq to the gpiochip irqdomain. What is the purpose of the
.init_hw callback? I've looked at other drivers which use regmap-irq
and they all seem to just initialize the hardware in their _probe().
-michael
On Sun, Nov 27, 2022 at 07:31:48PM +0100, Michael Walle wrote:
> Hi,
>
> [sorry this mail was just delivered now, although it seems
> to be sent last Tuesday.]
Actually I just sent it today, but you're right that it seems to have a
date of last Tuesday, so I must have configured something wrong on my
end before I sent it (sorry).
>
> Am 2022-11-22 11:29, schrieb William Breathitt Gray:
> > On Wed, Nov 23, 2022 at 05:01:53PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray
> > > wrote:
> > > > + /* Initialize device interrupt state */
> > > > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
> > > > + if (err)
> > > > + return err;
> > >
> > > Use ->init_hw() callback for this.
> >
> > In a subsequent patch 7/9 we remove direct gpio_chip registration in
> > favor of the i8255 library registration via gpio_regmap. It doesn't look
> > like gpio_regmap_register() sets the init_hw() callback.
> >
> > Michael, do you see any issues if I introduce init_hw() to
> > gpio_regmap_config? Or do you think this IRQ initialization belongs
> > somewhere else?
>
> Something like the following?
> gpiochip->init_hw = config.irq_init_hw;
>
> gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
> is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
> seems wrong.
>
> Maybe make gpio-regmap call it on its own? But really we just connect
> the regmap-irq to the gpiochip irqdomain.
I think you're right, it feels strange to handle IRQ initialization via
the GPIO framework. Maybe somewhere in regmap_irq might be more
appropriate?
> What is the purpose of the
> .init_hw callback? I've looked at other drivers which use regmap-irq
> and they all seem to just initialize the hardware in their _probe().
>
> -michael
I'm not opposed to initializing the hardware in _probe(), although I can
see merit in pushing that operation instead closer to the framework
where the initialization is actually relevant.
Andy, maybe you can shed some light about .init_hw; I think you
introduced it to gpiolib in commit 9411e3aaa6342.
William Breathitt Gray
On Sun, Nov 27, 2022 at 05:00:40PM -0500, William Breathitt Gray wrote:
> On Sun, Nov 27, 2022 at 07:31:48PM +0100, Michael Walle wrote:
> > Am 2022-11-22 11:29, schrieb William Breathitt Gray:
...
> > gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
> > is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
> > seems wrong.
> >
> > Maybe make gpio-regmap call it on its own? But really we just connect
> > the regmap-irq to the gpiochip irqdomain.
>
> I think you're right, it feels strange to handle IRQ initialization via
> the GPIO framework. Maybe somewhere in regmap_irq might be more
> appropriate?
The problem that that callback solves is possible interrupt storm, spurious
interrupts, and Use Before Initialized.
If you can guarantee that in your case it never happens, add a comment
and go on.
(It might be useful to tweak code a bit and try CONFIG_DEBUG_SHIRQ=y)
> > What is the purpose of the
> > .init_hw callback? I've looked at other drivers which use regmap-irq
> > and they all seem to just initialize the hardware in their _probe().
> >
> > -michael
>
> I'm not opposed to initializing the hardware in _probe(), although I can
> see merit in pushing that operation instead closer to the framework
> where the initialization is actually relevant.
>
> Andy, maybe you can shed some light about .init_hw; I think you
> introduced it to gpiolib in commit 9411e3aaa6342.
It seems that commit message doesn't fully explain the situation behind
that change. But it was observed in real life, see above.
--
With Best Regards,
Andy Shevchenko
On Sun, Nov 27, 2022 at 07:31:48PM +0100, Michael Walle wrote:
> Am 2022-11-22 11:29, schrieb William Breathitt Gray:
> > On Wed, Nov 23, 2022 at 05:01:53PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray
> > > wrote:
> > > > + /* Initialize device interrupt state */
> > > > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
> > > > + if (err)
> > > > + return err;
> > >
> > > Use ->init_hw() callback for this.
> >
> > In a subsequent patch 7/9 we remove direct gpio_chip registration in
> > favor of the i8255 library registration via gpio_regmap. It doesn't look
> > like gpio_regmap_register() sets the init_hw() callback.
> >
> > Michael, do you see any issues if I introduce init_hw() to
> > gpio_regmap_config? Or do you think this IRQ initialization belongs
> > somewhere else?
>
> Something like the following?
> gpiochip->init_hw = config.irq_init_hw;
>
> gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
> is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
> seems wrong.
>
> Maybe make gpio-regmap call it on its own? But really we just connect
> the regmap-irq to the gpiochip irqdomain. What is the purpose of the
> .init_hw callback? I've looked at other drivers which use regmap-irq
> and they all seem to just initialize the hardware in their _probe().
The purpose of that callback is to initialize IRQ part of the GPIO hardware
in the appropriate point of time. Of course there are drivers that are using
it and it's not in their ->probe():s, however you can tell that in the same
flow, because it's called synchronously.
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 28, 2022 at 11:51:10AM +0200, Andy Shevchenko wrote:
> On Sun, Nov 27, 2022 at 05:00:40PM -0500, William Breathitt Gray wrote:
> > On Sun, Nov 27, 2022 at 07:31:48PM +0100, Michael Walle wrote:
> > > Am 2022-11-22 11:29, schrieb William Breathitt Gray:
...
> > > gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
> > > is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
> > > seems wrong.
> > >
> > > Maybe make gpio-regmap call it on its own? But really we just connect
> > > the regmap-irq to the gpiochip irqdomain.
> >
> > I think you're right, it feels strange to handle IRQ initialization via
> > the GPIO framework. Maybe somewhere in regmap_irq might be more
> > appropriate?
>
> The problem that that callback solves is possible interrupt storm, spurious
> interrupts, and Use Before Initialized.
>
> If you can guarantee that in your case it never happens, add a comment
> and go on.
>
> (It might be useful to tweak code a bit and try CONFIG_DEBUG_SHIRQ=y)
>
> > > What is the purpose of the
> > > .init_hw callback? I've looked at other drivers which use regmap-irq
> > > and they all seem to just initialize the hardware in their _probe().
> > >
> > > -michael
> >
> > I'm not opposed to initializing the hardware in _probe(), although I can
> > see merit in pushing that operation instead closer to the framework
> > where the initialization is actually relevant.
> >
> > Andy, maybe you can shed some light about .init_hw; I think you
> > introduced it to gpiolib in commit 9411e3aaa6342.
>
> It seems that commit message doesn't fully explain the situation behind
> that change. But it was observed in real life, see above.
FWIW, real life example:
e986f0e602f1 ("pinctrl: intel: fix unexpected interrupt")
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 28, 2022 at 11:56:15AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 11:51:10AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 27, 2022 at 05:00:40PM -0500, William Breathitt Gray wrote:
> > > On Sun, Nov 27, 2022 at 07:31:48PM +0100, Michael Walle wrote:
> > > > Am 2022-11-22 11:29, schrieb William Breathitt Gray:
...
> > > > gpiochip doesn't seem to be the correct place, gpiochip_add_irqchip()
> > > > is a noop for gpio-regmap, right? So using gpiochip_irqchip_init_hw()
> > > > seems wrong.
> > > >
> > > > Maybe make gpio-regmap call it on its own? But really we just connect
> > > > the regmap-irq to the gpiochip irqdomain.
> > >
> > > I think you're right, it feels strange to handle IRQ initialization via
> > > the GPIO framework. Maybe somewhere in regmap_irq might be more
> > > appropriate?
> >
> > The problem that that callback solves is possible interrupt storm, spurious
> > interrupts, and Use Before Initialized.
> >
> > If you can guarantee that in your case it never happens, add a comment
> > and go on.
> >
> > (It might be useful to tweak code a bit and try CONFIG_DEBUG_SHIRQ=y)
> >
> > > > What is the purpose of the
> > > > .init_hw callback? I've looked at other drivers which use regmap-irq
> > > > and they all seem to just initialize the hardware in their _probe().
> > > >
> > > > -michael
> > >
> > > I'm not opposed to initializing the hardware in _probe(), although I can
> > > see merit in pushing that operation instead closer to the framework
> > > where the initialization is actually relevant.
> > >
> > > Andy, maybe you can shed some light about .init_hw; I think you
> > > introduced it to gpiolib in commit 9411e3aaa6342.
> >
> > It seems that commit message doesn't fully explain the situation behind
> > that change. But it was observed in real life, see above.
>
> FWIW, real life example:
> e986f0e602f1 ("pinctrl: intel: fix unexpected interrupt")
And another one (seems found from SW perspective):
a33912061607 ("gpio: lynxpoint: Move hardware initialization to callback")
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 28, 2022 at 10:56:06AM +0100, Michael Walle wrote:
> Am 2022-11-28 10:41, schrieb Andy Shevchenko:
> > Of course there are drivers that are using it and it's not in
> > their ->probe():s
>
> I was speaking of gpio drivers which use the regmap-irq stuff. I
> couldn't find any which are using {devm_,}regmap_add_irq_chip*()
> and gpiochip.init_hw().
Ah, that's true.
--
With Best Regards,
Andy Shevchenko
Am 2022-11-28 10:41, schrieb Andy Shevchenko:
> Of course there are drivers that are using it and it's not in
> their ->probe():s
I was speaking of gpio drivers which use the regmap-irq stuff. I
couldn't find any which are using {devm_,}regmap_add_irq_chip*()
and gpiochip.init_hw().
-michael