Add level active IRQ support to regmap-irq irqchip. Change breaks
existing regmap-irq type setting. Convert the existing drivers which
use regmap-irq with trigger type setting (gpio-max77620) to work
with this new approach. So we do not magically support level-active
IRQs on gpio-max77620 - but add support to the regmap-irq for chips
which support them =)
We do not support distinguishing situation where HW supports rising
and falling edge detection but not both. Separating this would require
inventing yet another flags for IRQ types.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Version 3 of this patch is intended to be functionally identical to v2.
This patch is rebased on top of a tree which contains changes:
"regmap: irq: handle HW using separate rising/falling edge interrupts"
from Bartosz Golaszewski and the change
"regmap: regmap-irq: Remove default irq type setting from core"
(proposed here):
https://lore.kernel.org/lkml/[email protected]/
There should not be direct dependency to "regmap: regmap-irq: Remove
default irq type setting from core" though. Patch was also tested to
apply cleany on regmap-tree.
Same statement regarding testing applies - gpio-max77620 are only
tested to compile. All real testing would be _HIGHLY_ appreciated.
drivers/base/regmap/regmap-irq.c | 35 ++++++++++-----
drivers/gpio/gpio-max77620.c | 96 ++++++++++++++++++++++++++--------------
include/linux/regmap.h | 27 ++++++++---
3 files changed, 110 insertions(+), 48 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 8b216b2e2c19..31d23c9a5ae7 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -199,7 +199,7 @@ static void regmap_irq_enable(struct irq_data *data)
const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
unsigned int mask, type;
- type = irq_data->type_falling_mask | irq_data->type_rising_mask;
+ type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
/*
* The type_in_mask flag means that the underlying hardware uses
@@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
struct regmap *map = d->map;
const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
- int reg = irq_data->type_reg_offset / map->reg_stride;
+ int reg;
+ const struct regmap_irq_type *t = &irq_data->type;
- if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
- return 0;
+ if ((t->types_supported & type) != type)
+ return -ENOTSUPP;
+
+ reg = t->type_reg_offset / map->reg_stride;
- d->type_buf[reg] &= ~(irq_data->type_falling_mask |
- irq_data->type_rising_mask);
+ if (t->type_reg_mask)
+ d->type_buf[reg] &= ~t->type_reg_mask;
+ else
+ d->type_buf[reg] &= ~(t->type_falling_val |
+ t->type_rising_val |
+ t->type_level_low_val |
+ t->type_level_high_val);
switch (type) {
case IRQ_TYPE_EDGE_FALLING:
- d->type_buf[reg] |= irq_data->type_falling_mask;
+ d->type_buf[reg] |= t->type_falling_val;
break;
case IRQ_TYPE_EDGE_RISING:
- d->type_buf[reg] |= irq_data->type_rising_mask;
+ d->type_buf[reg] |= t->type_rising_val;
break;
case IRQ_TYPE_EDGE_BOTH:
- d->type_buf[reg] |= (irq_data->type_falling_mask |
- irq_data->type_rising_mask);
+ d->type_buf[reg] |= (t->type_falling_val |
+ t->type_rising_val);
break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ d->type_buf[reg] |= t->type_level_high_val;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ d->type_buf[reg] |= t->type_level_low_val;
+ break;
default:
return -EINVAL;
}
diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 538bce4b5b42..65fa3a198ebd 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -25,60 +25,92 @@ struct max77620_gpio {
static const struct regmap_irq max77620_gpio_irqs[] = {
[0] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE0,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 0,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE0,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 0,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[1] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE1,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 1,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE1,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 1,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[2] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE2,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 2,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE2,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 2,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[3] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE3,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 3,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE3,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 3,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[4] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE4,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 4,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE4,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 4,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[5] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE5,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 5,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE5,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 5,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[6] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE6,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 6,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE6,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 6,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
[7] = {
- .mask = MAX77620_IRQ_LVL2_GPIO_EDGE7,
- .type_rising_mask = MAX77620_CNFG_GPIO_INT_RISING,
- .type_falling_mask = MAX77620_CNFG_GPIO_INT_FALLING,
.reg_offset = 0,
- .type_reg_offset = 7,
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE7,
+ .type = {
+ .type_rising_val = MAX77620_CNFG_GPIO_INT_RISING,
+ .type_falling_val = MAX77620_CNFG_GPIO_INT_FALLING,
+ .type_reg_mask = MAX77620_CNFG_GPIO_INT_MASK,
+ .type_reg_offset = 7,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
},
};
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index b7aa50cfb306..a904f87151e8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1089,22 +1089,37 @@ int regmap_fields_read(struct regmap_field *field, unsigned int id,
int regmap_fields_update_bits_base(struct regmap_field *field, unsigned int id,
unsigned int mask, unsigned int val,
bool *change, bool async, bool force);
+/**
+ * struct regmap_irq_type - IRQ type definitions.
+ *
+ * @type_reg_offset: Offset register for the irq type setting.
+ * @type_rising_val: Register value to configure RISING type irq.
+ * @type_falling_val: Register value to configure FALLING type irq.
+ * @type_level_low_val: Register value to configure LEVEL_LOW type irq.
+ * @type_level_high_val: Register value to configure LEVEL_HIGH type irq.
+ * @types_supported: logical OR of IRQ_TYPE_* flags indicating supported types.
+ */
+struct regmap_irq_type {
+ unsigned int type_reg_offset;
+ unsigned int type_reg_mask;
+ unsigned int type_rising_val;
+ unsigned int type_falling_val;
+ unsigned int type_level_low_val;
+ unsigned int type_level_high_val;
+ unsigned int types_supported;
+};
/**
* struct regmap_irq - Description of an IRQ for the generic regmap irq_chip.
*
* @reg_offset: Offset of the status/mask register within the bank
* @mask: Mask used to flag/control the register.
- * @type_reg_offset: Offset register for the irq type setting.
- * @type_rising_mask: Mask bit to configure RISING type irq.
- * @type_falling_mask: Mask bit to configure FALLING type irq.
+ * @type: IRQ trigger type setting details if supported.
*/
struct regmap_irq {
unsigned int reg_offset;
unsigned int mask;
- unsigned int type_reg_offset;
- unsigned int type_rising_mask;
- unsigned int type_falling_mask;
+ struct regmap_irq_type type;
};
#define REGMAP_IRQ_REG(_irq, _off, _mask) \
--
2.14.3
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Hi Matti,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regmap/for-next]
[also build test ERROR on next-20181218]
[cannot apply to v4.20-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Matti-Vaittinen/regmap-regmap-irq-gpio-max77620-add-level-irq-support/20181218-225844
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
config: x86_64-randconfig-x006-201850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/base/regmap/regmap-irq.c: In function 'regmap_add_irq_chip':
>> drivers/base/regmap/regmap-irq.c:644:24: error: 'const struct regmap_irq' has no member named 'type_reg_offset'; did you mean 'reg_offset'?
reg = chip->irqs[i].type_reg_offset / map->reg_stride;
^~~~~~~~~~~~~~~
reg_offset
>> drivers/base/regmap/regmap-irq.c:645:41: error: 'const struct regmap_irq' has no member named 'type_rising_mask'
d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
^
>> drivers/base/regmap/regmap-irq.c:646:19: error: 'const struct regmap_irq' has no member named 'type_falling_mask'
chip->irqs[i].type_falling_mask;
^
vim +644 drivers/base/regmap/regmap-irq.c
4af8be67f Mark Brown 2012-05-13 446
f8beab2bb Mark Brown 2011-10-28 447 /**
2cf8e2dfd Charles Keepax 2017-01-12 448 * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
f8beab2bb Mark Brown 2011-10-28 449 *
2cf8e2dfd Charles Keepax 2017-01-12 450 * @map: The regmap for the device.
2cf8e2dfd Charles Keepax 2017-01-12 451 * @irq: The IRQ the device uses to signal interrupts.
2cf8e2dfd Charles Keepax 2017-01-12 452 * @irq_flags: The IRQF_ flags to use for the primary interrupt.
2cf8e2dfd Charles Keepax 2017-01-12 453 * @irq_base: Allocate at specific IRQ number if irq_base > 0.
2cf8e2dfd Charles Keepax 2017-01-12 454 * @chip: Configuration for the interrupt controller.
2cf8e2dfd Charles Keepax 2017-01-12 455 * @data: Runtime data structure for the controller, allocated on success.
f8beab2bb Mark Brown 2011-10-28 456 *
f8beab2bb Mark Brown 2011-10-28 457 * Returns 0 on success or an errno on failure.
f8beab2bb Mark Brown 2011-10-28 458 *
f8beab2bb Mark Brown 2011-10-28 459 * In order for this to be efficient the chip really should use a
f8beab2bb Mark Brown 2011-10-28 460 * register cache. The chip driver is responsible for restoring the
f8beab2bb Mark Brown 2011-10-28 461 * register values used by the IRQ controller over suspend and resume.
f8beab2bb Mark Brown 2011-10-28 462 */
f8beab2bb Mark Brown 2011-10-28 463 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
b026ddbbd Mark Brown 2012-05-31 464 int irq_base, const struct regmap_irq_chip *chip,
f8beab2bb Mark Brown 2011-10-28 465 struct regmap_irq_chip_data **data)
f8beab2bb Mark Brown 2011-10-28 466 {
f8beab2bb Mark Brown 2011-10-28 467 struct regmap_irq_chip_data *d;
4af8be67f Mark Brown 2012-05-13 468 int i;
f8beab2bb Mark Brown 2011-10-28 469 int ret = -ENOMEM;
bc998a730 Bartosz Golaszewski 2018-12-07 470 int num_type_reg;
16032624f Stephen Warren 2012-07-27 471 u32 reg;
7b7d1968e Guo Zeng 2015-09-17 472 u32 unmask_offset;
f8beab2bb Mark Brown 2011-10-28 473
e12892070 Xiubo Li 2014-05-19 474 if (chip->num_regs <= 0)
e12892070 Xiubo Li 2014-05-19 475 return -EINVAL;
e12892070 Xiubo Li 2014-05-19 476
f01ee60ff Stephen Warren 2012-04-09 477 for (i = 0; i < chip->num_irqs; i++) {
f01ee60ff Stephen Warren 2012-04-09 478 if (chip->irqs[i].reg_offset % map->reg_stride)
f01ee60ff Stephen Warren 2012-04-09 479 return -EINVAL;
f01ee60ff Stephen Warren 2012-04-09 480 if (chip->irqs[i].reg_offset / map->reg_stride >=
f01ee60ff Stephen Warren 2012-04-09 481 chip->num_regs)
f01ee60ff Stephen Warren 2012-04-09 482 return -EINVAL;
f01ee60ff Stephen Warren 2012-04-09 483 }
f01ee60ff Stephen Warren 2012-04-09 484
4af8be67f Mark Brown 2012-05-13 485 if (irq_base) {
f8beab2bb Mark Brown 2011-10-28 486 irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0);
f8beab2bb Mark Brown 2011-10-28 487 if (irq_base < 0) {
f8beab2bb Mark Brown 2011-10-28 488 dev_warn(map->dev, "Failed to allocate IRQs: %d\n",
f8beab2bb Mark Brown 2011-10-28 489 irq_base);
f8beab2bb Mark Brown 2011-10-28 490 return irq_base;
f8beab2bb Mark Brown 2011-10-28 491 }
4af8be67f Mark Brown 2012-05-13 492 }
f8beab2bb Mark Brown 2011-10-28 493
f8beab2bb Mark Brown 2011-10-28 494 d = kzalloc(sizeof(*d), GFP_KERNEL);
f8beab2bb Mark Brown 2011-10-28 495 if (!d)
f8beab2bb Mark Brown 2011-10-28 496 return -ENOMEM;
f8beab2bb Mark Brown 2011-10-28 497
eeda1bd69 lixiubo 2015-11-20 498 d->status_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
f8beab2bb Mark Brown 2011-10-28 499 GFP_KERNEL);
f8beab2bb Mark Brown 2011-10-28 500 if (!d->status_buf)
f8beab2bb Mark Brown 2011-10-28 501 goto err_alloc;
f8beab2bb Mark Brown 2011-10-28 502
eeda1bd69 lixiubo 2015-11-20 503 d->mask_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
f8beab2bb Mark Brown 2011-10-28 504 GFP_KERNEL);
f8beab2bb Mark Brown 2011-10-28 505 if (!d->mask_buf)
f8beab2bb Mark Brown 2011-10-28 506 goto err_alloc;
f8beab2bb Mark Brown 2011-10-28 507
eeda1bd69 lixiubo 2015-11-20 508 d->mask_buf_def = kcalloc(chip->num_regs, sizeof(unsigned int),
f8beab2bb Mark Brown 2011-10-28 509 GFP_KERNEL);
f8beab2bb Mark Brown 2011-10-28 510 if (!d->mask_buf_def)
f8beab2bb Mark Brown 2011-10-28 511 goto err_alloc;
f8beab2bb Mark Brown 2011-10-28 512
a43fd50dc Mark Brown 2012-06-05 513 if (chip->wake_base) {
eeda1bd69 lixiubo 2015-11-20 514 d->wake_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
a43fd50dc Mark Brown 2012-06-05 515 GFP_KERNEL);
a43fd50dc Mark Brown 2012-06-05 516 if (!d->wake_buf)
a43fd50dc Mark Brown 2012-06-05 517 goto err_alloc;
a43fd50dc Mark Brown 2012-06-05 518 }
a43fd50dc Mark Brown 2012-06-05 519
bc998a730 Bartosz Golaszewski 2018-12-07 520 num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg;
bc998a730 Bartosz Golaszewski 2018-12-07 521 if (num_type_reg) {
bc998a730 Bartosz Golaszewski 2018-12-07 522 d->type_buf_def = kcalloc(num_type_reg,
7a78479fd Laxman Dewangan 2015-12-22 523 sizeof(unsigned int), GFP_KERNEL);
7a78479fd Laxman Dewangan 2015-12-22 524 if (!d->type_buf_def)
7a78479fd Laxman Dewangan 2015-12-22 525 goto err_alloc;
7a78479fd Laxman Dewangan 2015-12-22 526
bc998a730 Bartosz Golaszewski 2018-12-07 527 d->type_buf = kcalloc(num_type_reg, sizeof(unsigned int),
7a78479fd Laxman Dewangan 2015-12-22 528 GFP_KERNEL);
7a78479fd Laxman Dewangan 2015-12-22 529 if (!d->type_buf)
7a78479fd Laxman Dewangan 2015-12-22 530 goto err_alloc;
7a78479fd Laxman Dewangan 2015-12-22 531 }
7a78479fd Laxman Dewangan 2015-12-22 532
7ac140ec4 Stephen Warren 2012-08-01 533 d->irq_chip = regmap_irq_chip;
ca142750f Stephen Warren 2012-08-01 534 d->irq_chip.name = chip->name;
a43fd50dc Mark Brown 2012-06-05 535 d->irq = irq;
f8beab2bb Mark Brown 2011-10-28 536 d->map = map;
f8beab2bb Mark Brown 2011-10-28 537 d->chip = chip;
f8beab2bb Mark Brown 2011-10-28 538 d->irq_base = irq_base;
022f926a2 Graeme Gregory 2012-05-14 539
022f926a2 Graeme Gregory 2012-05-14 540 if (chip->irq_reg_stride)
022f926a2 Graeme Gregory 2012-05-14 541 d->irq_reg_stride = chip->irq_reg_stride;
022f926a2 Graeme Gregory 2012-05-14 542 else
022f926a2 Graeme Gregory 2012-05-14 543 d->irq_reg_stride = 1;
022f926a2 Graeme Gregory 2012-05-14 544
7a78479fd Laxman Dewangan 2015-12-22 545 if (chip->type_reg_stride)
7a78479fd Laxman Dewangan 2015-12-22 546 d->type_reg_stride = chip->type_reg_stride;
7a78479fd Laxman Dewangan 2015-12-22 547 else
7a78479fd Laxman Dewangan 2015-12-22 548 d->type_reg_stride = 1;
7a78479fd Laxman Dewangan 2015-12-22 549
67921a1a6 Markus Pargmann 2015-08-21 550 if (!map->use_single_read && map->reg_stride == 1 &&
a7440eaa9 Mark Brown 2013-01-03 551 d->irq_reg_stride == 1) {
549e08a0a lixiubo 2015-11-20 552 d->status_reg_buf = kmalloc_array(chip->num_regs,
549e08a0a lixiubo 2015-11-20 553 map->format.val_bytes,
549e08a0a lixiubo 2015-11-20 554 GFP_KERNEL);
a7440eaa9 Mark Brown 2013-01-03 555 if (!d->status_reg_buf)
a7440eaa9 Mark Brown 2013-01-03 556 goto err_alloc;
a7440eaa9 Mark Brown 2013-01-03 557 }
a7440eaa9 Mark Brown 2013-01-03 558
f8beab2bb Mark Brown 2011-10-28 559 mutex_init(&d->lock);
f8beab2bb Mark Brown 2011-10-28 560
f8beab2bb Mark Brown 2011-10-28 561 for (i = 0; i < chip->num_irqs; i++)
f01ee60ff Stephen Warren 2012-04-09 562 d->mask_buf_def[chip->irqs[i].reg_offset / map->reg_stride]
f8beab2bb Mark Brown 2011-10-28 563 |= chip->irqs[i].mask;
f8beab2bb Mark Brown 2011-10-28 564
f8beab2bb Mark Brown 2011-10-28 565 /* Mask all the interrupts by default */
f8beab2bb Mark Brown 2011-10-28 566 for (i = 0; i < chip->num_regs; i++) {
f8beab2bb Mark Brown 2011-10-28 567 d->mask_buf[i] = d->mask_buf_def[i];
16032624f Stephen Warren 2012-07-27 568 reg = chip->mask_base +
16032624f Stephen Warren 2012-07-27 569 (i * map->reg_stride * d->irq_reg_stride);
36ac914ba Xiaofan Tian 2012-08-30 570 if (chip->mask_invert)
a71411dbf Michael Grzeschik 2017-06-23 571 ret = regmap_irq_update_bits(d, reg,
36ac914ba Xiaofan Tian 2012-08-30 572 d->mask_buf[i], ~d->mask_buf[i]);
7b7d1968e Guo Zeng 2015-09-17 573 else if (d->chip->unmask_base) {
7b7d1968e Guo Zeng 2015-09-17 574 unmask_offset = d->chip->unmask_base -
7b7d1968e Guo Zeng 2015-09-17 575 d->chip->mask_base;
a71411dbf Michael Grzeschik 2017-06-23 576 ret = regmap_irq_update_bits(d,
7b7d1968e Guo Zeng 2015-09-17 577 reg + unmask_offset,
7b7d1968e Guo Zeng 2015-09-17 578 d->mask_buf[i],
7b7d1968e Guo Zeng 2015-09-17 579 d->mask_buf[i]);
7b7d1968e Guo Zeng 2015-09-17 580 } else
a71411dbf Michael Grzeschik 2017-06-23 581 ret = regmap_irq_update_bits(d, reg,
0eb46ad0c Mark Brown 2012-08-01 582 d->mask_buf[i], d->mask_buf[i]);
f8beab2bb Mark Brown 2011-10-28 583 if (ret != 0) {
f8beab2bb Mark Brown 2011-10-28 584 dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
16032624f Stephen Warren 2012-07-27 585 reg, ret);
f8beab2bb Mark Brown 2011-10-28 586 goto err_alloc;
f8beab2bb Mark Brown 2011-10-28 587 }
2753e6f82 Philipp Zabel 2013-07-22 588
2753e6f82 Philipp Zabel 2013-07-22 589 if (!chip->init_ack_masked)
2753e6f82 Philipp Zabel 2013-07-22 590 continue;
2753e6f82 Philipp Zabel 2013-07-22 591
2753e6f82 Philipp Zabel 2013-07-22 592 /* Ack masked but set interrupts */
2753e6f82 Philipp Zabel 2013-07-22 593 reg = chip->status_base +
2753e6f82 Philipp Zabel 2013-07-22 594 (i * map->reg_stride * d->irq_reg_stride);
2753e6f82 Philipp Zabel 2013-07-22 595 ret = regmap_read(map, reg, &d->status_buf[i]);
2753e6f82 Philipp Zabel 2013-07-22 596 if (ret != 0) {
2753e6f82 Philipp Zabel 2013-07-22 597 dev_err(map->dev, "Failed to read IRQ status: %d\n",
2753e6f82 Philipp Zabel 2013-07-22 598 ret);
2753e6f82 Philipp Zabel 2013-07-22 599 goto err_alloc;
2753e6f82 Philipp Zabel 2013-07-22 600 }
2753e6f82 Philipp Zabel 2013-07-22 601
d32334333 Alexander Shiyan 2013-12-15 602 if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
2753e6f82 Philipp Zabel 2013-07-22 603 reg = chip->ack_base +
2753e6f82 Philipp Zabel 2013-07-22 604 (i * map->reg_stride * d->irq_reg_stride);
a650fdd94 Guo Zeng 2015-09-17 605 if (chip->ack_invert)
a650fdd94 Guo Zeng 2015-09-17 606 ret = regmap_write(map, reg,
a650fdd94 Guo Zeng 2015-09-17 607 ~(d->status_buf[i] & d->mask_buf[i]));
a650fdd94 Guo Zeng 2015-09-17 608 else
2753e6f82 Philipp Zabel 2013-07-22 609 ret = regmap_write(map, reg,
2753e6f82 Philipp Zabel 2013-07-22 610 d->status_buf[i] & d->mask_buf[i]);
2753e6f82 Philipp Zabel 2013-07-22 611 if (ret != 0) {
2753e6f82 Philipp Zabel 2013-07-22 612 dev_err(map->dev, "Failed to ack 0x%x: %d\n",
2753e6f82 Philipp Zabel 2013-07-22 613 reg, ret);
2753e6f82 Philipp Zabel 2013-07-22 614 goto err_alloc;
2753e6f82 Philipp Zabel 2013-07-22 615 }
2753e6f82 Philipp Zabel 2013-07-22 616 }
f8beab2bb Mark Brown 2011-10-28 617 }
f8beab2bb Mark Brown 2011-10-28 618
40052ca0c Stephen Warren 2012-08-01 619 /* Wake is disabled by default */
40052ca0c Stephen Warren 2012-08-01 620 if (d->wake_buf) {
40052ca0c Stephen Warren 2012-08-01 621 for (i = 0; i < chip->num_regs; i++) {
40052ca0c Stephen Warren 2012-08-01 622 d->wake_buf[i] = d->mask_buf_def[i];
40052ca0c Stephen Warren 2012-08-01 623 reg = chip->wake_base +
40052ca0c Stephen Warren 2012-08-01 624 (i * map->reg_stride * d->irq_reg_stride);
9442490a0 Mark Brown 2013-01-04 625
9442490a0 Mark Brown 2013-01-04 626 if (chip->wake_invert)
a71411dbf Michael Grzeschik 2017-06-23 627 ret = regmap_irq_update_bits(d, reg,
9442490a0 Mark Brown 2013-01-04 628 d->mask_buf_def[i],
9442490a0 Mark Brown 2013-01-04 629 0);
9442490a0 Mark Brown 2013-01-04 630 else
a71411dbf Michael Grzeschik 2017-06-23 631 ret = regmap_irq_update_bits(d, reg,
9442490a0 Mark Brown 2013-01-04 632 d->mask_buf_def[i],
40052ca0c Stephen Warren 2012-08-01 633 d->wake_buf[i]);
40052ca0c Stephen Warren 2012-08-01 634 if (ret != 0) {
40052ca0c Stephen Warren 2012-08-01 635 dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
40052ca0c Stephen Warren 2012-08-01 636 reg, ret);
40052ca0c Stephen Warren 2012-08-01 637 goto err_alloc;
40052ca0c Stephen Warren 2012-08-01 638 }
40052ca0c Stephen Warren 2012-08-01 639 }
40052ca0c Stephen Warren 2012-08-01 640 }
40052ca0c Stephen Warren 2012-08-01 641
bc998a730 Bartosz Golaszewski 2018-12-07 642 if (chip->num_type_reg && !chip->type_in_mask) {
7a78479fd Laxman Dewangan 2015-12-22 643 for (i = 0; i < chip->num_irqs; i++) {
7a78479fd Laxman Dewangan 2015-12-22 @644 reg = chip->irqs[i].type_reg_offset / map->reg_stride;
7a78479fd Laxman Dewangan 2015-12-22 @645 d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
7a78479fd Laxman Dewangan 2015-12-22 @646 chip->irqs[i].type_falling_mask;
7a78479fd Laxman Dewangan 2015-12-22 647 }
7a78479fd Laxman Dewangan 2015-12-22 648 for (i = 0; i < chip->num_type_reg; ++i) {
7a78479fd Laxman Dewangan 2015-12-22 649 if (!d->type_buf_def[i])
7a78479fd Laxman Dewangan 2015-12-22 650 continue;
7a78479fd Laxman Dewangan 2015-12-22 651
7a78479fd Laxman Dewangan 2015-12-22 652 reg = chip->type_base +
7a78479fd Laxman Dewangan 2015-12-22 653 (i * map->reg_stride * d->type_reg_stride);
7a78479fd Laxman Dewangan 2015-12-22 654 if (chip->type_invert)
a71411dbf Michael Grzeschik 2017-06-23 655 ret = regmap_irq_update_bits(d, reg,
7a78479fd Laxman Dewangan 2015-12-22 656 d->type_buf_def[i], 0xFF);
7a78479fd Laxman Dewangan 2015-12-22 657 else
a71411dbf Michael Grzeschik 2017-06-23 658 ret = regmap_irq_update_bits(d, reg,
7a78479fd Laxman Dewangan 2015-12-22 659 d->type_buf_def[i], 0x0);
7a78479fd Laxman Dewangan 2015-12-22 660 if (ret != 0) {
7a78479fd Laxman Dewangan 2015-12-22 661 dev_err(map->dev,
7a78479fd Laxman Dewangan 2015-12-22 662 "Failed to set type in 0x%x: %x\n",
7a78479fd Laxman Dewangan 2015-12-22 663 reg, ret);
7a78479fd Laxman Dewangan 2015-12-22 664 goto err_alloc;
7a78479fd Laxman Dewangan 2015-12-22 665 }
7a78479fd Laxman Dewangan 2015-12-22 666 }
7a78479fd Laxman Dewangan 2015-12-22 667 }
7a78479fd Laxman Dewangan 2015-12-22 668
4af8be67f Mark Brown 2012-05-13 669 if (irq_base)
4af8be67f Mark Brown 2012-05-13 670 d->domain = irq_domain_add_legacy(map->dev->of_node,
4af8be67f Mark Brown 2012-05-13 671 chip->num_irqs, irq_base, 0,
4af8be67f Mark Brown 2012-05-13 672 ®map_domain_ops, d);
4af8be67f Mark Brown 2012-05-13 673 else
4af8be67f Mark Brown 2012-05-13 674 d->domain = irq_domain_add_linear(map->dev->of_node,
4af8be67f Mark Brown 2012-05-13 675 chip->num_irqs,
4af8be67f Mark Brown 2012-05-13 676 ®map_domain_ops, d);
4af8be67f Mark Brown 2012-05-13 677 if (!d->domain) {
4af8be67f Mark Brown 2012-05-13 678 dev_err(map->dev, "Failed to create IRQ domain\n");
4af8be67f Mark Brown 2012-05-13 679 ret = -ENOMEM;
4af8be67f Mark Brown 2012-05-13 680 goto err_alloc;
f8beab2bb Mark Brown 2011-10-28 681 }
f8beab2bb Mark Brown 2011-10-28 682
09cadf6e0 Valentin Rothberg 2015-02-11 683 ret = request_threaded_irq(irq, NULL, regmap_irq_thread,
09cadf6e0 Valentin Rothberg 2015-02-11 684 irq_flags | IRQF_ONESHOT,
f8beab2bb Mark Brown 2011-10-28 685 chip->name, d);
f8beab2bb Mark Brown 2011-10-28 686 if (ret != 0) {
eed456f93 Mark Brown 2013-03-19 687 dev_err(map->dev, "Failed to request IRQ %d for %s: %d\n",
eed456f93 Mark Brown 2013-03-19 688 irq, chip->name, ret);
4af8be67f Mark Brown 2012-05-13 689 goto err_domain;
f8beab2bb Mark Brown 2011-10-28 690 }
f8beab2bb Mark Brown 2011-10-28 691
72a6a5df2 Krzysztof Kozlowski 2014-03-13 692 *data = d;
72a6a5df2 Krzysztof Kozlowski 2014-03-13 693
f8beab2bb Mark Brown 2011-10-28 694 return 0;
f8beab2bb Mark Brown 2011-10-28 695
4af8be67f Mark Brown 2012-05-13 696 err_domain:
4af8be67f Mark Brown 2012-05-13 697 /* Should really dispose of the domain but... */
f8beab2bb Mark Brown 2011-10-28 698 err_alloc:
7a78479fd Laxman Dewangan 2015-12-22 699 kfree(d->type_buf);
7a78479fd Laxman Dewangan 2015-12-22 700 kfree(d->type_buf_def);
a43fd50dc Mark Brown 2012-06-05 701 kfree(d->wake_buf);
f8beab2bb Mark Brown 2011-10-28 702 kfree(d->mask_buf_def);
f8beab2bb Mark Brown 2011-10-28 703 kfree(d->mask_buf);
f8beab2bb Mark Brown 2011-10-28 704 kfree(d->status_buf);
a7440eaa9 Mark Brown 2013-01-03 705 kfree(d->status_reg_buf);
f8beab2bb Mark Brown 2011-10-28 706 kfree(d);
f8beab2bb Mark Brown 2011-10-28 707 return ret;
f8beab2bb Mark Brown 2011-10-28 708 }
f8beab2bb Mark Brown 2011-10-28 709 EXPORT_SYMBOL_GPL(regmap_add_irq_chip);
f8beab2bb Mark Brown 2011-10-28 710
:::::: The code at line 644 was first introduced by commit
:::::: 7a78479fd2acd25db7ecd1744d76f6841ec8a257 regmap: irq: add support for configuration of trigger type
:::::: TO: Laxman Dewangan <[email protected]>
:::::: CC: Mark Brown <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Dec 18, 2018 at 11:36:22PM +0800, kbuild test robot wrote:
> Hi Matti,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on regmap/for-next]
> [also build test ERROR on next-20181218]
> [cannot apply to v4.20-rc7]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Matti-Vaittinen/regmap-regmap-irq-gpio-max77620-add-level-irq-support/20181218-225844
> base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
> config: x86_64-randconfig-x006-201850 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> drivers/base/regmap/regmap-irq.c: In function 'regmap_add_irq_chip':
> >> drivers/base/regmap/regmap-irq.c:644:24: error: 'const struct regmap_irq' has no member named 'type_reg_offset'; did you mean 'reg_offset'?
> reg = chip->irqs[i].type_reg_offset / map->reg_stride;
> ^~~~~~~~~~~~~~~
> reg_offset
> >> drivers/base/regmap/regmap-irq.c:645:41: error: 'const struct regmap_irq' has no member named 'type_rising_mask'
> d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
> ^
> >> drivers/base/regmap/regmap-irq.c:646:19: error: 'const struct regmap_irq' has no member named 'type_falling_mask'
> chip->irqs[i].type_falling_mask;
---
> > Version 3 of this patch is intended to be functionally identical to v2.
> > This patch is rebased on top of a tree which contains changes:
> > "regmap: irq: handle HW using separate rising/falling edge interrupts"
> > from Bartosz Golaszewski and the change
> > "regmap: regmap-irq: Remove default irq type setting from core"
> > (proposed here):
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > There should not be direct dependency to "regmap: regmap-irq: Remove
> > default irq type setting from core" though. Patch was also tested to
> > apply cleany on regmap-tree.
I forgot that the block of code the commit "regmap: regmap-irq: Remove
default irq type setting from core" removes do use the old type specifiers
whcih this patch changes. So even though this patch applies cleanly on tree
which does not include "regmap: regmap-irq: Remove default irq type setting
from core" - it does not mean there is no dependency. This was my brain fart.
There is dependency. "regmap: regmap-irq: Remove default irq type setting
from core" should be applied prior this patch.
Should I combine these two patches as a series (and resend them) or what is
the correct way to note the dependency?
Br,
Matti Vaittinen
On Wed, Dec 19, 2018 at 08:48:42AM +0200, Matti Vaittinen wrote:
> I forgot that the block of code the commit "regmap: regmap-irq: Remove
> default irq type setting from core" removes do use the old type specifiers
> whcih this patch changes. So even though this patch applies cleanly on tree
> which does not include "regmap: regmap-irq: Remove default irq type setting
> from core" - it does not mean there is no dependency. This was my brain fart.
> There is dependency. "regmap: regmap-irq: Remove default irq type setting
> from core" should be applied prior this patch.
> Should I combine these two patches as a series (and resend them) or what is
> the correct way to note the dependency?
It's fine as-is.
Hi Matti,
On Tue, Dec 18, 2018 at 1:00 PM Matti Vaittinen
<[email protected]> wrote:
> Add level active IRQ support to regmap-irq irqchip. Change breaks
> existing regmap-irq type setting. Convert the existing drivers which
Indeed it does.
> use regmap-irq with trigger type setting (gpio-max77620) to work
> with this new approach. So we do not magically support level-active
> IRQs on gpio-max77620 - but add support to the regmap-irq for chips
> which support them =)
>
> We do not support distinguishing situation where HW supports rising
> and falling edge detection but not both. Separating this would require
> inventing yet another flags for IRQ types.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
This is now upstream as commit 1c2928e3e3212252 ("regmap:
regmap-irq/gpio-max77620: add level-irq support"), and breaks da9063-rtc
on the Renesas Koelsch board:
genirq: Setting trigger mode 8 for irq 157 failed
(regmap_irq_set_type+0x0/0x140)
da9063-rtc da9063-rtc: Failed to request ALARM IRQ 157: -524
da9063-rtc: probe of da9063-rtc failed with error -524
> ---
>
> Version 3 of this patch is intended to be functionally identical to v2.
> This patch is rebased on top of a tree which contains changes:
> "regmap: irq: handle HW using separate rising/falling edge interrupts"
> from Bartosz Golaszewski and the change
> "regmap: regmap-irq: Remove default irq type setting from core"
> (proposed here):
> https://lore.kernel.org/lkml/[email protected]/
>
> There should not be direct dependency to "regmap: regmap-irq: Remove
> default irq type setting from core" though. Patch was also tested to
> apply cleany on regmap-tree.
>
> Same statement regarding testing applies - gpio-max77620 are only
> tested to compile. All real testing would be _HIGHLY_ appreciated.
>
> drivers/base/regmap/regmap-irq.c | 35 ++++++++++-----
> drivers/gpio/gpio-max77620.c | 96 ++++++++++++++++++++++++++--------------
> include/linux/regmap.h | 27 ++++++++---
> 3 files changed, 110 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 8b216b2e2c19..31d23c9a5ae7 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -199,7 +199,7 @@ static void regmap_irq_enable(struct irq_data *data)
> const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> unsigned int mask, type;
>
> - type = irq_data->type_falling_mask | irq_data->type_rising_mask;
> + type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
>
> /*
> * The type_in_mask flag means that the underlying hardware uses
> @@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
> struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
> struct regmap *map = d->map;
> const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> - int reg = irq_data->type_reg_offset / map->reg_stride;
> + int reg;
> + const struct regmap_irq_type *t = &irq_data->type;
>
> - if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> - return 0;
> + if ((t->types_supported & type) != type)
> + return -ENOTSUPP;
Given types_supported defaults to zero, I think this breaks every existing
setup using REGMAP_IRQ_REG().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hello Geert,
Sorry for waiting - I just opened my computer after the holidays.
On Wed, Dec 26, 2018 at 12:39:17PM +0100, Geert Uytterhoeven wrote:
> Hi Matti,
>
> On Tue, Dec 18, 2018 at 1:00 PM Matti Vaittinen
> <[email protected]> wrote:
> > Add level active IRQ support to regmap-irq irqchip. Change breaks
> > existing regmap-irq type setting. Convert the existing drivers which
>
> Indeed it does.
>
> > use regmap-irq with trigger type setting (gpio-max77620) to work
> > with this new approach. So we do not magically support level-active
> > IRQs on gpio-max77620 - but add support to the regmap-irq for chips
> > which support them =)
> >
> > We do not support distinguishing situation where HW supports rising
> > and falling edge detection but not both. Separating this would require
> > inventing yet another flags for IRQ types.
> >
> > Signed-off-by: Matti Vaittinen <[email protected]>
>
> This is now upstream as commit 1c2928e3e3212252 ("regmap:
> regmap-irq/gpio-max77620: add level-irq support"), and breaks da9063-rtc
> on the Renesas Koelsch board:
>
> genirq: Setting trigger mode 8 for irq 157 failed
> (regmap_irq_set_type+0x0/0x140)
> da9063-rtc da9063-rtc: Failed to request ALARM IRQ 157: -524
> da9063-rtc: probe of da9063-rtc failed with error -524
This is strange as I do not see any type setting support code in
drivers/mfd/da9063-irq.c. The type setting registers are neither
specified in static const struct regmap_irq_chip da9063l_irq_chip nor
in static const struct regmap_irq_chip da9063_irq_chip. Hence I don't
understand how the da9063 could have been supporting IRQ type setting in
first place.
> > ---
> >
> > Version 3 of this patch is intended to be functionally identical to v2.
> > This patch is rebased on top of a tree which contains changes:
> > "regmap: irq: handle HW using separate rising/falling edge interrupts"
> > from Bartosz Golaszewski and the change
> > "regmap: regmap-irq: Remove default irq type setting from core"
> > (proposed here):
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > There should not be direct dependency to "regmap: regmap-irq: Remove
> > default irq type setting from core" though. Patch was also tested to
> > apply cleany on regmap-tree.
> >
> > Same statement regarding testing applies - gpio-max77620 are only
> > tested to compile. All real testing would be _HIGHLY_ appreciated.
> >
> > drivers/base/regmap/regmap-irq.c | 35 ++++++++++-----
> > drivers/gpio/gpio-max77620.c | 96 ++++++++++++++++++++++++++--------------
> > include/linux/regmap.h | 27 ++++++++---
> > 3 files changed, 110 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> > index 8b216b2e2c19..31d23c9a5ae7 100644
> > --- a/drivers/base/regmap/regmap-irq.c
> > +++ b/drivers/base/regmap/regmap-irq.c
> > @@ -199,7 +199,7 @@ static void regmap_irq_enable(struct irq_data *data)
> > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > unsigned int mask, type;
> >
> > - type = irq_data->type_falling_mask | irq_data->type_rising_mask;
> > + type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
> >
> > /*
> > * The type_in_mask flag means that the underlying hardware uses
> > @@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
> > struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
> > struct regmap *map = d->map;
> > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > - int reg = irq_data->type_reg_offset / map->reg_stride;
> > + int reg;
> > + const struct regmap_irq_type *t = &irq_data->type;
> >
> > - if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> > - return 0;
> > + if ((t->types_supported & type) != type)
> > + return -ENOTSUPP;
>
> Given types_supported defaults to zero, I think this breaks every existing
> setup using REGMAP_IRQ_REG().
Not really. The type setting support is not too old or widely used in
regmap_irq. If the type_base and num_type_reg in struct regmap_irq_chip
have not been given the type setting has not been supported. And the
macro REGMAP_IRQ_REG() has not been initializing those fields - they
must have been explicitly set by the driver. Only in-tree driver which
really used the regmap_irq type-setting was gpio-max77620 (if my
grepping did not go totally wrong). Is your version of
drivers/mfd/da9063-irq.c same I can see in
https://elixir.bootlin.com/linux/v4.20/source/drivers/mfd/da9063-irq.c ?
I will try to see if the regmap_irq code has just silently ignored irq
type setting requests if type setting information has not been populated
by driver. May that has been changed by my patch. I wonder what would be
the correct action if there is no type-setting information in
struct regmap_irq_chip - and if type setting irq callbacks are called.
Br,
Matti Vaittinen
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Hello All,
On Thu, Dec 27, 2018 at 09:35:31AM +0200, Matti Vaittinen wrote:
> On Wed, Dec 26, 2018 at 12:39:17PM +0100, Geert Uytterhoeven wrote:
> > Hi Matti,
> >
> > On Tue, Dec 18, 2018 at 1:00 PM Matti Vaittinen
> > <[email protected]> wrote:
> > > Add level active IRQ support to regmap-irq irqchip. Change breaks
> > > existing regmap-irq type setting. Convert the existing drivers which
> >
> > Indeed it does.
> >
> > > use regmap-irq with trigger type setting (gpio-max77620) to work
> > > with this new approach. So we do not magically support level-active
> > > IRQs on gpio-max77620 - but add support to the regmap-irq for chips
> > > which support them =)
> > >
> > > We do not support distinguishing situation where HW supports rising
> > > and falling edge detection but not both. Separating this would require
> > > inventing yet another flags for IRQ types.
> > >
> > > Signed-off-by: Matti Vaittinen <[email protected]>
> >
> > This is now upstream as commit 1c2928e3e3212252 ("regmap:
> > regmap-irq/gpio-max77620: add level-irq support"), and breaks da9063-rtc
> > on the Renesas Koelsch board:
> >
> > genirq: Setting trigger mode 8 for irq 157 failed
> > (regmap_irq_set_type+0x0/0x140)
> > da9063-rtc da9063-rtc: Failed to request ALARM IRQ 157: -524
> > da9063-rtc: probe of da9063-rtc failed with error -524
>
> This is strange as I do not see any type setting support code in
> drivers/mfd/da9063-irq.c. The type setting registers are neither
> specified in static const struct regmap_irq_chip da9063l_irq_chip nor
> in static const struct regmap_irq_chip da9063_irq_chip. Hence I don't
> understand how the da9063 could have been supporting IRQ type setting in
> first place.
>
> > > ---
> > >
> > > Version 3 of this patch is intended to be functionally identical to v2.
> > > This patch is rebased on top of a tree which contains changes:
> > > "regmap: irq: handle HW using separate rising/falling edge interrupts"
> > > from Bartosz Golaszewski and the change
> > > "regmap: regmap-irq: Remove default irq type setting from core"
> > > (proposed here):
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > There should not be direct dependency to "regmap: regmap-irq: Remove
> > > default irq type setting from core" though. Patch was also tested to
> > > apply cleany on regmap-tree.
> > >
> > > Same statement regarding testing applies - gpio-max77620 are only
> > > tested to compile. All real testing would be _HIGHLY_ appreciated.
> > >
> > > drivers/base/regmap/regmap-irq.c | 35 ++++++++++-----
> > > drivers/gpio/gpio-max77620.c | 96 ++++++++++++++++++++++++++--------------
> > > include/linux/regmap.h | 27 ++++++++---
> > > 3 files changed, 110 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> > > index 8b216b2e2c19..31d23c9a5ae7 100644
> > > --- a/drivers/base/regmap/regmap-irq.c
> > > +++ b/drivers/base/regmap/regmap-irq.c
> > > @@ -199,7 +199,7 @@ static void regmap_irq_enable(struct irq_data *data)
> > > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > > unsigned int mask, type;
> > >
> > > - type = irq_data->type_falling_mask | irq_data->type_rising_mask;
> > > + type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
> > >
> > > /*
> > > * The type_in_mask flag means that the underlying hardware uses
> > > @@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
> > > struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
> > > struct regmap *map = d->map;
> > > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > > - int reg = irq_data->type_reg_offset / map->reg_stride;
> > > + int reg;
> > > + const struct regmap_irq_type *t = &irq_data->type;
> > >
> > > - if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> > > - return 0;
> > > + if ((t->types_supported & type) != type)
> > > + return -ENOTSUPP;
> >
> > Given types_supported defaults to zero, I think this breaks every existing
> > setup using REGMAP_IRQ_REG().
Right. Now I see what you mean. Original code did:
if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
return 0;
Eg, even when the driver was not able to perform the type-setting this
failure was silently ignored, right. So doing:
if ((t->types_supported & type) != type)
return 0;
would be functionally equal. It feels like utterly wrong thing to do
(to me) - if driver is written to work with edge or level active
interrupts - and if the irq controller is not supporting this - then we
should warn the user. Just silently ignoring this sounds like asking for
irq storm or missed interrupts - but maybe I just don't get this =)
I'll send a patch with
if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
return 0;
in order to not break existing functionality - but it feels plain wrong
to me.
> Br,
> Matti Vaittinen
>
> --
> Matti Vaittinen
> ROHM Semiconductors
>
> ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
On Thu, Dec 27, 2018 at 09:56:48AM +0200, Matti Vaittinen wrote:
> Hello All,
>
> On Thu, Dec 27, 2018 at 09:35:31AM +0200, Matti Vaittinen wrote:
> > On Wed, Dec 26, 2018 at 12:39:17PM +0100, Geert Uytterhoeven wrote:
> > > Hi Matti,
> > >
> > > On Tue, Dec 18, 2018 at 1:00 PM Matti Vaittinen
> > > <[email protected]> wrote:
> > > > Add level active IRQ support to regmap-irq irqchip. Change breaks
> > > > existing regmap-irq type setting. Convert the existing drivers which
> > >
> > > Indeed it does.
> > >
> > > This is now upstream as commit 1c2928e3e3212252 ("regmap:
> > > regmap-irq/gpio-max77620: add level-irq support"), and breaks da9063-rtc
> > > on the Renesas Koelsch board:
> > >
> > > genirq: Setting trigger mode 8 for irq 157 failed
> > > (regmap_irq_set_type+0x0/0x140)
> > > da9063-rtc da9063-rtc: Failed to request ALARM IRQ 157: -524
> > > da9063-rtc: probe of da9063-rtc failed with error -524
> >
> > This is strange as I do not see any type setting support code in
> > drivers/mfd/da9063-irq.c. The type setting registers are neither
> > specified in static const struct regmap_irq_chip da9063l_irq_chip nor
> > in static const struct regmap_irq_chip da9063_irq_chip. Hence I don't
> > understand how the da9063 could have been supporting IRQ type setting in
> > first place.
> >
> > > > @@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
> > > > struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
> > > > struct regmap *map = d->map;
> > > > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > > > - int reg = irq_data->type_reg_offset / map->reg_stride;
> > > > + int reg;
> > > > + const struct regmap_irq_type *t = &irq_data->type;
> > > >
> > > > - if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> > > > - return 0;
> > > > + if ((t->types_supported & type) != type)
> > > > + return -ENOTSUPP;
> > >
> > > Given types_supported defaults to zero, I think this breaks every existing
> > > setup using REGMAP_IRQ_REG().
>
> Right. Now I see what you mean. Original code did:
>
> if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> return 0;
>
> Eg, even when the driver was not able to perform the type-setting this
> failure was silently ignored, right. So doing:
> if ((t->types_supported & type) != type)
> return 0;
> would be functionally equal. It feels like utterly wrong thing to do
> (to me) - if driver is written to work with edge or level active
> interrupts - and if the irq controller is not supporting this - then we
> should warn the user. Just silently ignoring this sounds like asking for
> irq storm or missed interrupts - but maybe I just don't get this =)
>
> I'll send a patch with
> if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> return 0;
> in order to not break existing functionality - but it feels plain wrong
> to me.
Geert, I did send this patch yesterday. It's here if you wish to try it:
https://lore.kernel.org/patchwork/patch/1027963/
Last night - just when I was about to get some sleep - it stroke me. I
think the correct thing to do would be leaving the irq_set_type to NULL
for those IRQ chips which do not support type setting. If we do that,
then the irq core will take care of situations where user requests type
setting but the chip does not support it. Which means the regmap-irq
would be no different from any other irq chip where type setting is not
supported.
/// irrelevant ///
I guess you know the moment of "Hypnagogia" when you are comfortably at
bed your head feels all dizzy and world is starting to faint away...
And just a second later you are fully awake thinking of a possible
solution - and definitely not able to sleep anymore :/
https://www.reddit.com/r/ProgrammerHumor/comments/93eq0e/everytime/
/// irrelevant ends ///
I just took a peek in
kernel/irq/manage.c - and found following:
int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
{
struct irq_chip *chip = desc->irq_data.chip;
int ret, unmask = 0;
if (!chip || !chip->irq_set_type) {
/*
* IRQF_TRIGGER_* but the PIC does not support multiple
* flow-types?
*/
pr_debug("No set_type function for IRQ %d (%s)\n",
irq_desc_get_irq(desc),
chip ? (chip->name ? : "unknown") : "unknown");
return 0;
}
...
so at the moment the IRQ core is also just spilling out the warning and
then returning zero. But at least we would get the warning from core if
the irq-chip does not support type config.
So at the cost of removing "const" from regmap_irq_chip we could do:
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 2a031743f31f..b6aef50ab378 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -298,12 +298,11 @@ static int regmap_irq_set_wake(struct irq_data *data, unsigned int on)
return 0;
}
-static const struct irq_chip regmap_irq_chip = {
+static struct irq_chip regmap_irq_chip = {
.irq_bus_lock = regmap_irq_lock,
.irq_bus_sync_unlock = regmap_irq_sync_unlock,
.irq_disable = regmap_irq_disable,
.irq_enable = regmap_irq_enable,
- .irq_set_type = regmap_irq_set_type,
.irq_set_wake = regmap_irq_set_wake,
};
@@ -610,6 +609,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg;
if (num_type_reg) {
+ regmap_irq_chip.irq_set_type = regmap_irq_set_type;
d->type_buf_def = kcalloc(num_type_reg,
sizeof(unsigned int), GFP_KERNEL);
if (!d->type_buf_def)
Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
but I did omit this as I have never looked at the wake functionality
before).
Br,
Matti Vaittinen
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
On Fri, Dec 28, 2018 at 10:05:33AM +0200, Matti Vaittinen wrote:
> Last night - just when I was about to get some sleep - it stroke me. I
> think the correct thing to do would be leaving the irq_set_type to NULL
> for those IRQ chips which do not support type setting. If we do that,
> then the irq core will take care of situations where user requests type
> setting but the chip does not support it. Which means the regmap-irq
> would be no different from any other irq chip where type setting is not
> supported.
Yes, this is the best fix - let the framework handle things properly.
We'll need a second set of operations and to select which to use based
on having type information but that's fine.
> So at the cost of removing "const" from regmap_irq_chip we could do:
...
> Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
> but I did omit this as I have never looked at the wake functionality
> before).
We need a separate struct as otherwise if there's multiple devices with
regmap irq_chip implementations then they'll collide with each other but
otherwise I like this approach (or we could copy the irq_chip struct
when registering and then modify which is going to scale a bit better -
you're probably right that we need to do the same thing for the wake
configuration. I'll still look at applying your patch as a temporary
fix though.
On Mon, Dec 31, 2018 at 07:11:27PM +0000, Mark Brown wrote:
> On Fri, Dec 28, 2018 at 10:05:33AM +0200, Matti Vaittinen wrote:
>
> > Last night - just when I was about to get some sleep - it stroke me. I
> > think the correct thing to do would be leaving the irq_set_type to NULL
> > for those IRQ chips which do not support type setting. If we do that,
> > then the irq core will take care of situations where user requests type
> > setting but the chip does not support it. Which means the regmap-irq
> > would be no different from any other irq chip where type setting is not
> > supported.
>
> Yes, this is the best fix - let the framework handle things properly.
> We'll need a second set of operations and to select which to use based
> on having type information but that's fine.
>
> > So at the cost of removing "const" from regmap_irq_chip we could do:
>
> ...
>
> > Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
> > but I did omit this as I have never looked at the wake functionality
> > before).
>
> We need a separate struct as otherwise if there's multiple devices with
> regmap irq_chip implementations then they'll collide with each other
Right. I must admit I didn't notice this! I was about to make a nasty
error there...
> otherwise I like this approach (or we could copy the irq_chip struct
> when registering and then modify which is going to scale a bit better -
I am really not a fan of dynamic allocation - I'd rather had static
structs with different set of operations. But I admit I can't think of a
sane system where we would have more than few regmap_irq controllers so
memory consumption of allocating new structs is hardly an issue here.
> you're probably right that we need to do the same thing for the wake
> configuration. I'll still look at applying your patch as a temporary
> fix though.
Thanks Mark. I try to cook a patch with copying of struct irq_chip still
at this week but I wont rush it (I have some other topics under work) as
the regression should be fixed by the other patch.
Br,
Matti Vaittinen
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
On Wed, Jan 02, 2019 at 09:42:51AM +0200, Matti Vaittinen wrote:
> On Mon, Dec 31, 2018 at 07:11:27PM +0000, Mark Brown wrote:
> > On Fri, Dec 28, 2018 at 10:05:33AM +0200, Matti Vaittinen wrote:
> >
> > > Last night - just when I was about to get some sleep - it stroke me. I
> > > think the correct thing to do would be leaving the irq_set_type to NULL
> > > for those IRQ chips which do not support type setting. If we do that,
> > > then the irq core will take care of situations where user requests type
> > > setting but the chip does not support it. Which means the regmap-irq
> > > would be no different from any other irq chip where type setting is not
> > > supported.
> >
> > Yes, this is the best fix - let the framework handle things properly.
> > We'll need a second set of operations and to select which to use based
> > on having type information but that's fine.
> >
> > > So at the cost of removing "const" from regmap_irq_chip we could do:
> >
> > ...
> >
> > > Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
> > > but I did omit this as I have never looked at the wake functionality
> > > before).
> >
> > We need a separate struct as otherwise if there's multiple devices with
> > regmap irq_chip implementations then they'll collide with each other
>
> Right. I must admit I didn't notice this! I was about to make a nasty
> error there...
>
Looking at the code I think it just copies the struct anyway,
basically using it as a template so I think this should be fine.
> > you're probably right that we need to do the same thing for the wake
> > configuration. I'll still look at applying your patch as a temporary
> > fix though.
>
> Thanks Mark. I try to cook a patch with copying of struct irq_chip still
> at this week but I wont rush it (I have some other topics under work) as
> the regression should be fixed by the other patch.
>
Just to check that is this patch here:
https://lore.kernel.org/lkml/[email protected]/
Just want to check what will be applied so I know it will fix the
regression I am seeing as well.
Thanks,
Charles
On Thu, Jan 03, 2019 at 05:20:08PM +0000, Charles Keepax wrote:
> On Wed, Jan 02, 2019 at 09:42:51AM +0200, Matti Vaittinen wrote:
> > On Mon, Dec 31, 2018 at 07:11:27PM +0000, Mark Brown wrote:
> > > On Fri, Dec 28, 2018 at 10:05:33AM +0200, Matti Vaittinen wrote:
> > >
> > > > Last night - just when I was about to get some sleep - it stroke me. I
> > > > think the correct thing to do would be leaving the irq_set_type to NULL
> > > > for those IRQ chips which do not support type setting. If we do that,
> > > > then the irq core will take care of situations where user requests type
> > > > setting but the chip does not support it. Which means the regmap-irq
> > > > would be no different from any other irq chip where type setting is not
> > > > supported.
> > >
> > > Yes, this is the best fix - let the framework handle things properly.
> > > We'll need a second set of operations and to select which to use based
> > > on having type information but that's fine.
> > >
> > > > So at the cost of removing "const" from regmap_irq_chip we could do:
> > >
> > > ...
> > >
> > > > Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
> > > > but I did omit this as I have never looked at the wake functionality
> > > > before).
> > >
> > > We need a separate struct as otherwise if there's multiple devices with
> > > regmap irq_chip implementations then they'll collide with each other
> >
> > Right. I must admit I didn't notice this! I was about to make a nasty
> > error there...
> >
>
> Looking at the code I think it just copies the struct anyway,
> basically using it as a template so I think this should be fine.
Rigth. It seems to be:
d->irq_chip = regmap_irq_chip;
not:
d->irq_chip = ®map_irq_chip;
and
struct regmap_irq_chip_data {
struct mutex lock;
struct irq_chip irq_chip;
struct regmap *map;
...
};
not
struct regmap_irq_chip_data {
struct mutex lock;
struct irq_chip *irq_chip;
struct regmap *map;
...
};
> > > you're probably right that we need to do the same thing for the wake
> > > configuration. I'll still look at applying your patch as a temporary
> > > fix though.
I won't touch the wake thing (at least not yet) as I am not familiar
with it. Is it Ok to change it with another patch later?
> >
> > Thanks Mark. I try to cook a patch with copying of struct irq_chip still
> > at this week but I wont rush it (I have some other topics under work) as
> > the regression should be fixed by the other patch.
> >
>
> Just to check that is this patch here:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Just want to check what will be applied so I know it will fix the
> regression I am seeing as well.
Yep. That's the patch Mark did apply.
>
> Thanks,
> Charles
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~