* [RFC] regmap-irq: add "main register" and level-irq support
@ 2018-11-30 8:59 Matti Vaittinen
2018-12-04 17:21 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Matti Vaittinen @ 2018-11-30 8:59 UTC (permalink / raw)
To: broonie, gregkh, rafael, linux-kernel
Hello All.
Sorry for lenghty email. But I tried to explain this in details.
Surprizingly I didn't find an example how to handle this properly/easily.
Thus I am wondering if following changes to regmap-irq make any sense
or is there some better way (which I just didn't spot) to handle this.
I am working with another PMIC from ROHM. The PMIC
has several 'sub blocks' like RTC, GPIO, charger, regulators, ...
each of which can generate interrupts to processor. PMIC irq is
connected via single GPIO line to the processor irq controller
and registers are accessed via i2c.
IRQ handling is implemented so that each sub block has own
status/ack/mask registers and then there is one main status
register. When any of the sub blocks gets unmasked IRQ,
corresponding 'main status register' bit is set in main status
register.
Main
+---------------+ inactive
|0|0|0|0|0|0|0|0| ------------- IRQ_L to processor
+---------------+
Sub status
+---------------+ +---------------+ +---------------+ +---------------+
|0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0|
+---------------+ +---------------+ +---------------+ +---------------+
Sub mask
+---------------+ +---------------+ +---------------+ +---------------+
|0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0|
+---------------+ +---------------+ +---------------+ +---------------+
Main
+---------------+ active
|0|0|0|0|0|0|1|0| ------------- IRQ_L to processor
+---------------+
Sub status/ack X
+---------------+ +---------------+ +---------------+ +---------------+
|0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|1|0|0|0| |0|0|0|0|0|0|0|0|
+---------------+ +---------------+ +---------------+ +---------------+
Sub mask
+---------------+ +---------------+ +---------------+ +---------------+
|0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0| |0|0|0|0|0|0|0|0|
+---------------+ +---------------+ +---------------+ +---------------+
Usually I would model this by using chained/nested IRQ chips for this
setup. One chip would handle the main register and be parent for
the secondary chip(s) which would handle the sub-irqs. I really liked to
use regmap-irq for this as it already provides all the code. Problem I
am struggling with is that the GPIO sub-block has 4 GPIO pins for other
devices to use for IRQs. And I would like to provide access to them via
the pmic device-tree node by marking the pmic node as
interrupt-controller. Eg, I would like to do:
pmic: bd70528@4b {
compatible = "rohm,bd70528";
interrupt-parent = <&gpio1>;
interrupts = <29 8>;
interrupt-controller;
#interrupt-cells = <2>;
/* Snip... */
};
deviceX {
interrupt-parent = <&pmic>;
interrupts = <GPIO_1 2>;
/* ... */
};
deviceY {
interrupt-parent = <&pmic>;
interrupts = <GPIO_2 2>;
/* ... */
};
Problem I faced is that I don't know how to create two regmap-irq chips for
the same i2c decvice - because the regmap-irq gets the DT node from regmnap
- and thus both of the irq chips would have same DT node. My assumption is
that the DT - xlate functionality for this setup would not be quite correct,
right? I guess the "main irq" interrupts would be hookable via DT? But
the devices are interested in "sub IRQs". So I thought that the correct
solution would be to model the interrupts from this PMIC as just one
irq-chip. So I wonder if addin this "main IRQ register" support for
regmap-irq makes sense? Additionally this ROHM PMIC GPIO supports
configuring IRQ pins as level triggered. So I thought we could add
support for setting the type to level_low or level_high. This change
would require adding 'supported_types' information to all drivers which
are currently using regmap-irq for supporting type-setting - but quick
grep for drivers allow me to think that it is currently only the
gpio-max77620 which uses the type setting in-tree.
So the patch below is not properly tested but it should explain what I
am plannig. Can you plese tell me if this makes sense or not? If this
sounds like acceptable patch - then I will test this a bit further and
create a real patch out of it.
Best Regards:
Matti Vaittinen
There is bunch of devices with multiple logical blocks which
can generate interrupts. It's not a rare case that the interrupt
reason registers are arranged so that there is own status/ack/mask
register for each logical block. In soem devices there is also a
'main interrupt register(s)' which can indicate what sub blocks
have interrupts pending.
When sucn a device is connected via slow bus like i2c the main
part of interrupt handling latency can be caused by bus accesses.
On systems where it is expected that only on (or few) sub blocks
have active interrupts we can reduce the latency by only reading
the main register and those sub registers which have active
interrupts. Support this with regmap-irq for simple cases where
main register does not require acking or masking.
additionally support level active interrupts with regmap-irq
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/base/regmap/regmap-irq.c | 118 +++++++++++++++++++++++++++++++++++++--
include/linux/regmap.h | 34 +++++++++++
2 files changed, 146 insertions(+), 6 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 429ca8ed7e51..b6971123268d 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -35,6 +35,7 @@ struct regmap_irq_chip_data {
int wake_count;
void *status_reg_buf;
+ unsigned int *main_status_buf;
unsigned int *status_buf;
unsigned int *mask_buf;
unsigned int *mask_buf_def;
@@ -214,11 +215,17 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
int reg = irq_data->type_reg_offset / map->reg_stride;
- if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
+ if ((irq_data->types_supported & type) != type)
+ return -ENOTSUPP;
+
+ if (!(irq_data->type_rising_mask | irq_data->type_falling_mask |
+ irq_data->type_level_high_mask | irq_data->type_level_low_mask))
return 0;
d->type_buf[reg] &= ~(irq_data->type_falling_mask |
- irq_data->type_rising_mask);
+ irq_data->type_rising_mask |
+ irq_data->type_level_low_mask |
+ irq_data->type_level_high_mask);
switch (type) {
case IRQ_TYPE_EDGE_FALLING:
d->type_buf[reg] |= irq_data->type_falling_mask;
@@ -233,6 +240,13 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
irq_data->type_rising_mask);
break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ d->type_buf[reg] |= irq_data->type_level_high_mask;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ d->type_buf[reg] |= irq_data->type_level_low_mask;
+ break;
default:
return -EINVAL;
}
@@ -269,6 +283,33 @@ static const struct irq_chip regmap_irq_chip = {
.irq_set_wake = regmap_irq_set_wake,
};
+static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
+ unsigned int b)
+{
+ const struct regmap_irq_chip *chip = data->chip;
+ struct regmap *map = data->map;
+ struct regmap_irq_sub_irq_map *subreg;
+ int i, ret = 0;
+
+ if (!chip->sub_reg_offsets) {
+ /* Assume linear mapping */
+ ret = regmap_read(map, chip->status_base +
+ (b * map->reg_stride * data->irq_reg_stride),
+ &data->status_buf[b]);
+ } else {
+ subreg = &chip->sub_reg_offsets[b];
+ for (i = 0; i < subreg->num_regs; i++) {
+ unsigned int offset = subreg->offset[i];
+
+ ret = regmap_read(map, chip->status_base + offset,
+ &data->status_buf[offset]);
+ if (ret)
+ break;
+ }
+ }
+ return ret;
+}
+
static irqreturn_t regmap_irq_thread(int irq, void *d)
{
struct regmap_irq_chip_data *data = d;
@@ -292,11 +333,65 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
}
/*
- * Read in the statuses, using a single bulk read if possible
- * in order to reduce the I/O overheads.
+ * Read only registers with active IRQs if the chip has 'main status
+ * register'. Else read in the statuses, using a single bulk read if
+ * possible in order to reduce the I/O overheads.
*/
- if (!map->use_single_read && map->reg_stride == 1 &&
- data->irq_reg_stride == 1) {
+
+ if (chip->num_main_regs) {
+ unsigned int max_main_bits;
+ unsigned long size;
+
+ size = chip->num_regs * sizeof(unsigned int);
+
+ max_main_bits = (chip->num_main_status_bits) ?
+ chip->num_main_status_bits : chip->num_regs;
+ /* Clear the status buf as we don't read all status regs */
+ memset(data->status_buf, 0, size);
+
+ /* We could support bulk read for main status registers
+ * but I don't expect to see devices with really many main
+ * status registers so let's only support single reads for the
+ * sake of simplicity. and add bulk reads only if needed
+ */
+ for (i = 0; i < chip->num_main_regs; i++) {
+ ret = regmap_read(map, chip->main_status +
+ (i * map->reg_stride
+ * data->irq_reg_stride),
+ &data->main_status_buf[i]);
+ if (ret) {
+ dev_err(map->dev,
+ "Failed to read IRQ status %d\n",
+ ret);
+ goto exit;
+ }
+ }
+
+ /* Read sub registers with active IRQs */
+ for (i = 0; i < chip->num_main_regs; i++) {
+ unsigned int b;
+ const unsigned long mreg = data->main_status_buf[i];
+
+ for_each_set_bit(b, &mreg, map->format.val_bytes * 8) {
+ if (i * map->format.val_bytes * 8 + b >
+ max_main_bits)
+ break;
+ ret = read_sub_irq_data(data, b);
+
+ if (ret != 0) {
+ dev_err(map->dev,
+ "Failed to read IRQ status %d\n",
+ ret);
+ if (chip->runtime_pm)
+ pm_runtime_put(map->dev);
+ goto exit;
+ }
+ }
+
+ }
+ } else if (!map->use_single_read && map->reg_stride == 1 &&
+ data->irq_reg_stride == 1) {
+
u8 *buf8 = data->status_reg_buf;
u16 *buf16 = data->status_reg_buf;
u32 *buf32 = data->status_reg_buf;
@@ -457,6 +552,15 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
if (!d)
return -ENOMEM;
+ if (chip->num_main_regs) {
+ d->main_status_buf = kcalloc(chip->num_main_regs,
+ sizeof(unsigned int),
+ GFP_KERNEL);
+
+ if (!d->main_status_buf)
+ goto err_alloc;
+ }
+
d->status_buf = kcalloc(chip->num_regs, sizeof(unsigned int),
GFP_KERNEL);
if (!d->status_buf)
@@ -602,6 +706,8 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
if (chip->num_type_reg) {
for (i = 0; i < chip->num_irqs; i++) {
+ if (!chip->irqs[i].types_supported)
+ continue;
reg = chip->irqs[i].type_reg_offset / map->reg_stride;
d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
chip->irqs[i].type_falling_mask;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a367d59c301d..57dfd3e462db 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1098,6 +1098,7 @@ int regmap_fields_update_bits_base(struct regmap_field *field, unsigned int id,
* @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.
+ * @types_supported: logical OR of IRQ_TYPE_* flags indicating supported types
*/
struct regmap_irq {
unsigned int reg_offset;
@@ -1105,16 +1106,44 @@ struct regmap_irq {
unsigned int type_reg_offset;
unsigned int type_rising_mask;
unsigned int type_falling_mask;
+ unsigned int type_level_low_mask;
+ unsigned int type_level_high_mask;
+ unsigned int types_supported;
};
#define REGMAP_IRQ_REG(_irq, _off, _mask) \
[_irq] = { .reg_offset = (_off), .mask = (_mask) }
+struct regmap_irq_sub_irq_map {
+ unsigned int num_regs;
+ unsigned int *offset;
+};
+
+#define REGMAP_IRQ_MAIN_REG_OFFSET(arr) \
+ { .num_regs = ARRAY_SIZE((arr)), .offset = &(arr)[0] }
+
/**
* struct regmap_irq_chip - Description of a generic regmap irq_chip.
*
* @name: Descriptive name for IRQ controller.
*
+ * @main_status: Base main status register address. For chips which have
+ * interrupts arranged in separate sub-irq blocks with own IRQ
+ * registers and which have a main IRQ registers indicating
+ * sub-irq blocks with unhandled interrupts. For such chips fill
+ * sub-irq register information in status_base, mask_base and
+ * ack_base.
+ * @sub_reg_offsets: arrays of mappings from main register bits to sub irq
+ * registers. First item in array describes the registers
+ * for first main status bit. Second array for second bit etc.
+ * Offset is given as sub register status offset to status_base.
+ * Should contain num_regs arrays. Can be provided for chips with
+ * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to
+ * 2.nd sub-reg, ...
+ * @num_main_regs: Number of 'main' irq registers.
+ * @num_main_status_bits: Should be given to chips where number of meaningfull
+ * main status bits differs from num_regs.
+ *
* @status_base: Base status register address.
* @mask_base: Base mask register address.
* @mask_writeonly: Base mask register is write only.
@@ -1154,6 +1183,10 @@ struct regmap_irq {
struct regmap_irq_chip {
const char *name;
+ unsigned int main_status;
+ unsigned int num_main_status_bits;
+ struct regmap_irq_sub_irq_map *sub_reg_offsets;
+
unsigned int status_base;
unsigned int mask_base;
unsigned int unmask_base;
@@ -1170,6 +1203,7 @@ struct regmap_irq_chip {
bool runtime_pm:1;
bool type_invert:1;
+ int num_main_regs;
int num_regs;
const struct regmap_irq *irqs;
--
2.14.3
--
Matti Vaittinen
ROHM Semiconductors
~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-11-30 8:59 [RFC] regmap-irq: add "main register" and level-irq support Matti Vaittinen @ 2018-12-04 17:21 ` Mark Brown 2018-12-05 8:22 ` Matti Vaittinen 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2018-12-04 17:21 UTC (permalink / raw) To: Matti Vaittinen; +Cc: gregkh, rafael, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3312 bytes --] On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote: > IRQ handling is implemented so that each sub block has own > status/ack/mask registers and then there is one main status > register. When any of the sub blocks gets unmasked IRQ, > corresponding 'main status register' bit is set in main status > register. This sounds exactly like the wm831x which uses cascaded irqchips for this, though not regmap-irq. > Problem I faced is that I don't know how to create two regmap-irq chips for > the same i2c decvice - because the regmap-irq gets the DT node from regmnap > - and thus both of the irq chips would have same DT node. My assumption is > that the DT - xlate functionality for this setup would not be quite correct, > right? I guess the "main irq" interrupts would be hookable via DT? But > the devices are interested in "sub IRQs". So I thought that the correct > solution would be to model the interrupts from this PMIC as just one > irq-chip. So I wonder if addin this "main IRQ register" support for I suspect the idiomatic way to handle this in DT is to make DT nodes (and devices) for the subfunction interrupt controllers and expose the internal structure of the chip to the DT. This does make some sense as the individual interrupt controllers within the chip are clearly reusable IP blocks though it means that the chip starts looking a bit weird externally so perhaps that's not ideal and your suggestion makes sense. > configuring IRQ pins as level triggered. So I thought we could add > support for setting the type to level_low or level_high. This change > would require adding 'supported_types' information to all drivers which > are currently using regmap-irq for supporting type-setting - but quick > grep for drivers allow me to think that it is currently only the > gpio-max77620 which uses the type setting in-tree. That seems pluasible but definitely seems like a separate change. > + * @main_status: Base main status register address. For chips which have > + * interrupts arranged in separate sub-irq blocks with own IRQ > + * registers and which have a main IRQ registers indicating > + * sub-irq blocks with unhandled interrupts. For such chips fill > + * sub-irq register information in status_base, mask_base and > + * ack_base. > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq > + * registers. First item in array describes the registers > + * for first main status bit. Second array for second bit etc. > + * Offset is given as sub register status offset to status_base. > + * Should contain num_regs arrays. Can be provided for chips with > + * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to > + * 2.nd sub-reg, ... This interface feels a little awkward in that we define the sub interrupts in this separate array and require them all to be in separate single registers which probably won't be true in general (IIRC it wasn't how the wm831x worked). How about instead we have the individual interrupts mark which main status bits flag them, then build up a mapping in the other direction during init of subregisters to read? I've not tried to implement that so there might be annoying difficulties during implementation though. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-04 17:21 ` Mark Brown @ 2018-12-05 8:22 ` Matti Vaittinen 2018-12-05 17:27 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Matti Vaittinen @ 2018-12-05 8:22 UTC (permalink / raw) To: Mark Brown; +Cc: gregkh, rafael, linux-kernel Hello Mark, Thank you for taking the time and checking this! Highly appreciated! On Tue, Dec 04, 2018 at 05:21:37PM +0000, Mark Brown wrote: > On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote: > > > IRQ handling is implemented so that each sub block has own > > status/ack/mask registers and then there is one main status > > register. When any of the sub blocks gets unmasked IRQ, > > corresponding 'main status register' bit is set in main status > > register. > > This sounds exactly like the wm831x which uses cascaded irqchips for > this, though not regmap-irq. Yep. I guess I could do cascaded chips - but I would like to use the regmap-irq for this driver. Maybe we could change regmap-irq so that it would be possible to not give the DT node from regmap for this chip? This was actually my first thought - but then I felt that having two irq chips for one device is a bit hacky - and hence I decided to see how regmap-irq could be changed to support the 'main irq status' register usage. I think this 'main register' setup is pretty common design. > > Problem I faced is that I don't know how to create two regmap-irq chips for > > the same i2c decvice - because the regmap-irq gets the DT node from regmnap > > - and thus both of the irq chips would have same DT node. My assumption is > > that the DT - xlate functionality for this setup would not be quite correct, > > right? I guess the "main irq" interrupts would be hookable via DT? But > > the devices are interested in "sub IRQs". So I thought that the correct > > solution would be to model the interrupts from this PMIC as just one > > irq-chip. So I wonder if addin this "main IRQ register" support for > > I suspect the idiomatic way to handle this in DT is to make DT nodes > (and devices) for the subfunction interrupt controllers and expose the > internal structure of the chip to the DT. Yes. This would be one approach - but I am not sure how DT guys see this. I may be wrong but I have a feeling Rob prefers having only one DT node describing single device. But for me it would make sense to have own node for GPIO - especially because the GPIO is only IRQ controller which really is visible outside the chip. But here we still hit the same problem if we want to use regmap-irq. Regmap-irq still knows only the i2c device DT node. Currently there is no way to tell regmap-irq to use the sub-node. > This does make some sense as > the individual interrupt controllers within the chip are clearly > reusable IP blocks though it means that the chip starts looking a bit > weird externally so perhaps that's not ideal and your suggestion makes > sense. So I take this as a suggestion to continue tinkering with the 'main irq register support'. Or how do you see my suggestion of making iot possible to instruct the regmap-irq to not use DT for 'main irq controller'? Then we could probably do cascaded controllers where only the controller handling the externally visible 'sub-irqs' would be bound to DT? The pitfall here is case where more than one sub-irq controllers needs to be exposed to other devices. In that sense the 'main irq thing' would have better 'case coverage' =) (Wow, what a fancy words, maybe I am turning into a manager here :p ) > > configuring IRQ pins as level triggered. So I thought we could add > > support for setting the type to level_low or level_high. This change > > would require adding 'supported_types' information to all drivers which > > are currently using regmap-irq for supporting type-setting - but quick > > grep for drivers allow me to think that it is currently only the > > gpio-max77620 which uses the type setting in-tree. > > That seems pluasible but definitely seems like a separate change. All right. I will create a separate patch (set) out of this. > > > + * @main_status: Base main status register address. For chips which have > > + * interrupts arranged in separate sub-irq blocks with own IRQ > > + * registers and which have a main IRQ registers indicating > > + * sub-irq blocks with unhandled interrupts. For such chips fill > > + * sub-irq register information in status_base, mask_base and > > + * ack_base. > > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq > > + * registers. First item in array describes the registers > > + * for first main status bit. Second array for second bit etc. > > + * Offset is given as sub register status offset to status_base. > > + * Should contain num_regs arrays. Can be provided for chips with > > + * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to > > + * 2.nd sub-reg, ... > > This interface feels a little awkward in that we define the sub > interrupts in this separate array and require them all to be in separate > single registers which probably won't be true in general (IIRC it wasn't > how the wm831x worked). So my implementation here was just the simplest solution to 'glue' the main status register on top of existing implementation. If we do it this way, then the 'main status register' is completely optional - chip would work just fine even without the 'main register' - main register only saves some reads as subregisters with no irqs can be left unread. But you are correct - this only supports limited amount of HWs. Many chips I have seen (prior to the one I am now working with) actually require acking also the main status register. They also may support masking the main status register. But I guess trying to support all such cases would make the regmap-irq really complex and hard to understand. > How about instead we have the individual > interrupts mark which main status bits flag them, then build up a > mapping in the other direction during init of subregisters to read? I am not sure if I understand your idea correctly - but for me it feels that the 'main status register' is only benefical when we can do direct mapping from main register bit/value to sub-register(s). But as we must really read whole sub register at once, it really does not matter which interrupts inside the sub register did activate the main status - we go through all bits in the sub register anyways. Hence it may not be needed to have mapping from individual interrupts to the main status register value? I guess looping through all the sub register bits is negligible source of latency compared to the time the register access takes. So identifying the sub register(s) based on main status is the thing - not mapping the individual interrupts in sub register(s), right? I think that having to specify the main status register value for each interrupt would be quite a overkill. So what do you think, would changing the > > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq > > + * registers. First item in array describes the registers > > + * for first main status bit. Second array for second bit etc. > > + * Offset is given as sub register status offset to status_base. > > + * Should contain num_regs arrays. Can be provided for chips with > > + * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to > > + * 2.nd sub-reg, ... to @sub_reg_offsets: arrays of mappings from main register _value_ to to sub irq registers ... be the (simplest) way to go? And maybe changing the description for main_status to: * @main_status: Optional base main status register address. Some chips have * "main status register(s)" which indicate actual irq registers * with unhandled interrupts. Provide main status register * address here to avoid reading irq registers with no * active interrupts. Mapping from main status value to * interrupt register can be provided in sub_reg_offsets. Perhaps this would clarify that the status_base, mask_base, ack_base, etc. should still be used normally? Best Regards Matti Vaittinen -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-05 8:22 ` Matti Vaittinen @ 2018-12-05 17:27 ` Mark Brown 2018-12-07 7:58 ` Matti Vaittinen 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2018-12-05 17:27 UTC (permalink / raw) To: Matti Vaittinen; +Cc: gregkh, rafael, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7512 bytes --] On Wed, Dec 05, 2018 at 10:22:51AM +0200, Matti Vaittinen wrote: > On Tue, Dec 04, 2018 at 05:21:37PM +0000, Mark Brown wrote: > > On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote: > > This sounds exactly like the wm831x which uses cascaded irqchips for > > this, though not regmap-irq. > Yep. I guess I could do cascaded chips - but I would like to use the > regmap-irq for this driver. Maybe we could change regmap-irq so that it > would be possible to not give the DT node from regmap for this chip? > This was actually my first thought - but then I felt that having two irq > chips for one device is a bit hacky - and hence I decided to see how > regmap-irq could be changed to support the 'main irq status' register > usage. I think this 'main register' setup is pretty common design. I'm not sure I see it as particularly hacky - it's using features that the frameworks provide to do the fan out, that just seems like making use of existing framework features. > > I suspect the idiomatic way to handle this in DT is to make DT nodes > > (and devices) for the subfunction interrupt controllers and expose the > > internal structure of the chip to the DT. > Yes. This would be one approach - but I am not sure how DT guys see > this. I may be wrong but I have a feeling Rob prefers having only one DT > node describing single device. But for me it would make sense to have > own node for GPIO - especially because the GPIO is only IRQ controller Well, it kind of depends if you see the sub-interrupt controllers as independent devices or not (at the hardware level they will be). > which really is visible outside the chip. But here we still hit the same > problem if we want to use regmap-irq. Regmap-irq still knows only the > i2c device DT node. Currently there is no way to tell regmap-irq to use > the sub-node. If they're full subdevices you'd be pointing at the relevant platform device anyway. > > This does make some sense as > > the individual interrupt controllers within the chip are clearly > > reusable IP blocks though it means that the chip starts looking a bit > > weird externally so perhaps that's not ideal and your suggestion makes > > sense. > So I take this as a suggestion to continue tinkering with the 'main irq > register support'. Or how do you see my suggestion of making iot > possible to instruct the regmap-irq to not use DT for 'main irq > controller'? Then we could probably do cascaded controllers where only > the controller handling the externally visible 'sub-irqs' would be > bound to DT? The pitfall here is case where more than one sub-irq > controllers needs to be exposed to other devices. In that sense the > 'main irq thing' would have better 'case coverage' =) (Wow, what a fancy > words, maybe I am turning into a manager here :p ) I'm on the fence about the main controller idea. > > > + * @main_status: Base main status register address. For chips which have > > > + * interrupts arranged in separate sub-irq blocks with own IRQ > > > + * registers and which have a main IRQ registers indicating > > > + * sub-irq blocks with unhandled interrupts. For such chips fill > > > + * sub-irq register information in status_base, mask_base and > > > + * ack_base. > > > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq > > > + * registers. First item in array describes the registers > > > + * for first main status bit. Second array for second bit etc. > > > + * Offset is given as sub register status offset to status_base. > > > + * Should contain num_regs arrays. Can be provided for chips with > > > + * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to > > > + * 2.nd sub-reg, ... > > This interface feels a little awkward in that we define the sub > > interrupts in this separate array and require them all to be in separate > > single registers which probably won't be true in general (IIRC it wasn't > > how the wm831x worked). > So my implementation here was just the simplest solution to 'glue' the > main status register on top of existing implementation. If we do it this > way, then the 'main status register' is completely optional - chip would > work just fine even without the 'main register' - main register only > saves some reads as subregisters with no irqs can be left unread. But > you are correct - this only supports limited amount of HWs. Many chips I > have seen (prior to the one I am now working with) actually require > acking also the main status register. They also may support masking the > main status register. But I guess trying to support all such cases would > make the regmap-irq really complex and hard to understand. Right, it's that sort of thing that makes me want to just cascade the interrupt controllers - at some point you just end up with as fully featured an interrupt controller distributing to other fully featured interrupt controllers. > > How about instead we have the individual > > interrupts mark which main status bits flag them, then build up a > > mapping in the other direction during init of subregisters to read? > I am not sure if I understand your idea correctly - but for me it feels > that the 'main status register' is only benefical when we can do direct > mapping from main register bit/value to sub-register(s). The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware gets structured is that the central interrupt controller is saying which IP block is flagging an interrupt rather than which register has an interrupt set in it. You can then have either more than one detailed status register for that IP or several smaller IPs reporting through a single detailed status register. I've also seen some interrupts represented directly in the main status if there's some need for a fast path. > But as we must really read whole sub register at once, it really does > not matter which interrupts inside the sub register did activate the > main status - we go through all bits in the sub register anyways. Hence > it may not be needed to have mapping from individual interrupts to the > main status register value? I guess looping through all the sub > register bits is negligible source of latency compared to the time the > register access takes. So identifying the sub register(s) based on main > status is the thing - not mapping the individual interrupts in sub > register(s), right? I think that having to specify the main status > register value for each interrupt would be quite a overkill. Right, it's about working out which subregister to read - the point is you can do this by adding a link in either direction, I'm suggesting doing it from the individual interrupt to the main register since we already have individual data structures for those and it feels less likely to run into hard to represent corner cases. > * @main_status: Optional base main status register address. Some chips have > * "main status register(s)" which indicate actual irq registers > * with unhandled interrupts. Provide main status register > * address here to avoid reading irq registers with no > * active interrupts. Mapping from main status value to > * interrupt register can be provided in sub_reg_offsets. > Perhaps this would clarify that the status_base, mask_base, ack_base, > etc. should still be used normally? Probably. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-05 17:27 ` Mark Brown @ 2018-12-07 7:58 ` Matti Vaittinen 2018-12-07 13:14 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Matti Vaittinen @ 2018-12-07 7:58 UTC (permalink / raw) To: Mark Brown; +Cc: gregkh, rafael, linux-kernel Hello Again Mark, On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote: > On Wed, Dec 05, 2018 at 10:22:51AM +0200, Matti Vaittinen wrote: > > On Tue, Dec 04, 2018 at 05:21:37PM +0000, Mark Brown wrote: > > > On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote: > > > > This sounds exactly like the wm831x which uses cascaded irqchips for > > > this, though not regmap-irq. > > > Yep. I guess I could do cascaded chips - but I would like to use the > > regmap-irq for this driver. Maybe we could change regmap-irq so that it > > would be possible to not give the DT node from regmap for this chip? > > This was actually my first thought - but then I felt that having two irq > > chips for one device is a bit hacky - and hence I decided to see how > > regmap-irq could be changed to support the 'main irq status' register > > usage. I think this 'main register' setup is pretty common design. > > I'm not sure I see it as particularly hacky - it's using features that > the frameworks provide to do the fan out, that just seems like making > use of existing framework features. > > > > I suspect the idiomatic way to handle this in DT is to make DT nodes > > > (and devices) for the subfunction interrupt controllers and expose the > > > internal structure of the chip to the DT. > > > Yes. This would be one approach - but I am not sure how DT guys see > > this. I may be wrong but I have a feeling Rob prefers having only one DT > > node describing single device. But for me it would make sense to have > > own node for GPIO - especially because the GPIO is only IRQ controller > > Well, it kind of depends if you see the sub-interrupt controllers as > independent devices or not (at the hardware level they will be). > > > which really is visible outside the chip. But here we still hit the same > > problem if we want to use regmap-irq. Regmap-irq still knows only the > > i2c device DT node. Currently there is no way to tell regmap-irq to use > > the sub-node. > > If they're full subdevices you'd be pointing at the relevant platform > device anyway. I am not sure if I have explained me well enough =) When we look at regmap_add_irq_chip we see that it creates the IRQ domains. And we further see: d->domain = irq_domain_add_linear(map->dev->of_node, chip->num_irqs, ®map_domain_ops, d); where map->dev->of_node points the node added to regmap. And as we really want to use the i2c to access registers, we should have done the regmap using: devm_regmap_init_i2c(i2c, ®map); where the i2c is the struct i2c_client *. The dev in i2c_client is the i2c device - which has of_node pointing at the "main i2c node" - not the sub block nodes. Hence all irq chips created by regmap_add_irq_chip for this physical i2c slave device will point to the same DT node. > > > This does make some sense as > > > the individual interrupt controllers within the chip are clearly > > > reusable IP blocks though it means that the chip starts looking a bit > > > weird externally so perhaps that's not ideal and your suggestion makes > > > sense. > > > So I take this as a suggestion to continue tinkering with the 'main irq > > register support'. Or how do you see my suggestion of making iot > > possible to instruct the regmap-irq to not use DT for 'main irq > > controller'? Then we could probably do cascaded controllers where only > > the controller handling the externally visible 'sub-irqs' would be > > bound to DT? The pitfall here is case where more than one sub-irq > > controllers needs to be exposed to other devices. In that sense the > > 'main irq thing' would have better 'case coverage' =) (Wow, what a fancy > > words, maybe I am turning into a manager here :p ) > > I'm on the fence about the main controller idea So am I. But I still would like to use regmap-irq no matter if I create one or many irq chips. So please allow me to present the other idea: Would it work if we add someting like struct regmap_irq_chip { const char *name; unsigned int status_base; unsigned int mask_base; unsigned int unmask_base; unsigned int ack_base; unsigned int wake_base; unsigned int type_base; unsigned int irq_reg_stride; bool mask_writeonly:1; + bool no_of:1; and in the regmap_add_irq_chip do: node = (chip->no_of) ? NULL : map->dev->of_node; d->domain = irq_domain_add_linear(node, chip->num_irqs, ®map_domain_ops, d); Then we could have chip->no_of set for the 'main irq chip' and for those chips we don't wan't to expose via DT. In my case I would leave no_of unset only for the irq-chip which I created for the GPIO? Is this a silly idea? > > > > + * @main_status: Base main status register address. For chips which have > > > > + * interrupts arranged in separate sub-irq blocks with own IRQ > > > > + * registers and which have a main IRQ registers indicating > > > > + * sub-irq blocks with unhandled interrupts. For such chips fill > > > > + * sub-irq register information in status_base, mask_base and > > > > + * ack_base. > > > > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq > > > > + * registers. First item in array describes the registers > > > > + * for first main status bit. Second array for second bit etc. > > > > + * Offset is given as sub register status offset to status_base. > > > > + * Should contain num_regs arrays. Can be provided for chips with > > > > + * more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to > > > > + * 2.nd sub-reg, ... > > > > This interface feels a little awkward in that we define the sub > > > interrupts in this separate array and require them all to be in separate > > > single registers which probably won't be true in general (IIRC it wasn't > > > how the wm831x worked). > > > So my implementation here was just the simplest solution to 'glue' the > > main status register on top of existing implementation. If we do it this > > way, then the 'main status register' is completely optional - chip would > > work just fine even without the 'main register' - main register only > > saves some reads as subregisters with no irqs can be left unread. But > > you are correct - this only supports limited amount of HWs. Many chips I > > have seen (prior to the one I am now working with) actually require > > acking also the main status register. They also may support masking the > > main status register. But I guess trying to support all such cases would > > make the regmap-irq really complex and hard to understand. > > Right, it's that sort of thing that makes me want to just cascade the > interrupt controllers - at some point you just end up with as fully > featured an interrupt controller distributing to other fully featured > interrupt controllers. Yes. Question is whether we should support getting rid of extra irq register reads if HW provides the 'main status' register. I would not support main irq acking/masking (at least not for now). > > > How about instead we have the individual > > > interrupts mark which main status bits flag them, then build up a > > > mapping in the other direction during init of subregisters to read? > > > I am not sure if I understand your idea correctly - but for me it feels > > that the 'main status register' is only benefical when we can do direct > > mapping from main register bit/value to sub-register(s). > > The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware > gets structured is that the central interrupt controller is saying which > IP block is flagging an interrupt rather than which register has an > interrupt set in it. You can then have either more than one detailed > status register for that IP Correct. But I guess the IP blocks often have limited set of registers for the "sub interrupts". For such cases my idea would work, right. My RFC handled case where there is many 'sub registers' to read for one 'main bit. > or several smaller IPs reporting through a > single detailed status register. I think that if we have more than two layers of irqs (main and sub) - then we should do cascaded controllers. > I've also seen some interrupts > represented directly in the main status if there's some need for a fast > path. Right. This is not covered by my 'RFC'. And here the flagging from individual irq to main status bit would be usefull. But I am a bit afraid of complexity supporting this adds - as well as the overhead of describing 'main status bits' for all interrupts in cases where we don't have such fast-path bits. Besides, adding the main status handling does not force one to use it if existing mechanisms work well =) > > But as we must really read whole sub register at once, it really does > > not matter which interrupts inside the sub register did activate the > > main status - we go through all bits in the sub register anyways. Hence > > it may not be needed to have mapping from individual interrupts to the > > main status register value? I guess looping through all the sub > > register bits is negligible source of latency compared to the time the > > register access takes. So identifying the sub register(s) based on main > > status is the thing - not mapping the individual interrupts in sub > > register(s), right? I think that having to specify the main status > > register value for each interrupt would be quite a overkill. > > Right, it's about working out which subregister to read - the point is > you can do this by adding a link in either direction, I'm suggesting > doing it from the individual interrupt to the main register since we > already have individual data structures for those and it feels less > likely to run into hard to represent corner cases. I see your point now. But as I said, I am not sure we should add the overhead of 'main irq bit description' for simple cases just to cover the corner cases. Yet I can try seeing what I can come up with if you think this is the way to go. > > * @main_status: Optional base main status register address. Some chips have > > * "main status register(s)" which indicate actual irq registers > > * with unhandled interrupts. Provide main status register > > * address here to avoid reading irq registers with no > > * active interrupts. Mapping from main status value to > > * interrupt register can be provided in sub_reg_offsets. > > > Perhaps this would clarify that the status_base, mask_base, ack_base, > > etc. should still be used normally? > > Probably. I am not sure this means I was able to convince you or if this is the polite way to tell me not to bother =) I am Finnish guy who does not understand small talk or polite hints ;) Best Regards Matti Vaittinen -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-07 7:58 ` Matti Vaittinen @ 2018-12-07 13:14 ` Mark Brown 2018-12-14 13:58 ` Matti Vaittinen 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2018-12-07 13:14 UTC (permalink / raw) To: Matti Vaittinen; +Cc: gregkh, rafael, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4354 bytes --] On Fri, Dec 07, 2018 at 09:58:29AM +0200, Matti Vaittinen wrote: > On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote: > > If they're full subdevices you'd be pointing at the relevant platform > > device anyway. > I am not sure if I have explained me well enough =) When we look at > regmap_add_irq_chip we see that it creates the IRQ domains. And we > further see: > > d->domain = irq_domain_add_linear(map->dev->of_node, > chip->num_irqs, > ®map_domain_ops, d); > where map->dev->of_node points the node added to regmap. And as we > really want to use the i2c to access registers, we should have done the > regmap using: > devm_regmap_init_i2c(i2c, ®map); > where the i2c is the struct i2c_client *. The dev in i2c_client is the > i2c device - which has of_node pointing at the "main i2c node" - not the > sub block nodes. Hence all irq chips created by regmap_add_irq_chip for > this physical i2c slave device will point to the same DT node. Hrm, right. You'd need to have a proxy regmap for the child devices for that. Not ideal. > bool mask_writeonly:1; > + bool no_of:1; > > and in the regmap_add_irq_chip do: > node = (chip->no_of) ? NULL : map->dev->of_node; > d->domain = irq_domain_add_linear(node, chip->num_irqs, > ®map_domain_ops, d); > Then we could have chip->no_of set for the 'main irq chip' and for those > chips we don't wan't to expose via DT. In my case I would leave no_of > unset only for the irq-chip which I created for the GPIO? Is this a > silly idea? That's worth a shot, yes - I'd need to see it fully fleshed out I think but it looks sensible (no ternery operator please). > > The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware > > gets structured is that the central interrupt controller is saying which > > IP block is flagging an interrupt rather than which register has an > > interrupt set in it. You can then have either more than one detailed > > status register for that IP > Correct. But I guess the IP blocks often have limited set of registers for the > "sub interrupts". For such cases my idea would work, right. My RFC > handled case where there is many 'sub registers' to read for one 'main > bit. Your idea definitely works for the current case, yes - I'm just thinking about future edge and extension cases. > > or several smaller IPs reporting through a > > single detailed status register. > I think that if we have more than two layers of irqs (main and sub) - > then we should do cascaded controllers. Yeah, and my first thought here is that we should just be using cascaded controllers all the time (but like I say I'm not 100% certain on that). > > Right, it's about working out which subregister to read - the point is > > you can do this by adding a link in either direction, I'm suggesting > > doing it from the individual interrupt to the main register since we > > already have individual data structures for those and it feels less > > likely to run into hard to represent corner cases. > I see your point now. But as I said, I am not sure we should add the > overhead of 'main irq bit description' for simple cases just to cover > the corner cases. Yet I can try seeing what I can come up with if you > think this is the way to go. If you could take a look that'd be great. > > > * @main_status: Optional base main status register address. Some chips have > > > * "main status register(s)" which indicate actual irq registers > > > * with unhandled interrupts. Provide main status register > > > * address here to avoid reading irq registers with no > > > * active interrupts. Mapping from main status value to > > > * interrupt register can be provided in sub_reg_offsets. > > > Perhaps this would clarify that the status_base, mask_base, ack_base, > > > etc. should still be used normally? > > Probably. > I am not sure this means I was able to convince you or if this is the > polite way to tell me not to bother =) I am Finnish guy who does not > understand small talk or polite hints ;) I mean that probably if you add clarifications like you suggest that might do it. It's one of these things where you can't 100% tell how things will look until you see the actual thing. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-07 13:14 ` Mark Brown @ 2018-12-14 13:58 ` Matti Vaittinen 2018-12-14 17:26 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Matti Vaittinen @ 2018-12-14 13:58 UTC (permalink / raw) To: Mark Brown; +Cc: gregkh, rafael, linux-kernel On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote: > On Fri, Dec 07, 2018 at 09:58:29AM +0200, Matti Vaittinen wrote: > > On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote: > > > d->domain = irq_domain_add_linear(map->dev->of_node, > > chip->num_irqs, > > ®map_domain_ops, d); > > > where map->dev->of_node points the node added to regmap. And as we > > really want to use the i2c to access registers, we should have done the > > regmap using: > > > devm_regmap_init_i2c(i2c, ®map); > > > where the i2c is the struct i2c_client *. The dev in i2c_client is the > > i2c device - which has of_node pointing at the "main i2c node" - not the > > sub block nodes. Hence all irq chips created by regmap_add_irq_chip for > > this physical i2c slave device will point to the same DT node. > > Hrm, right. You'd need to have a proxy regmap for the child devices for > that. Not ideal. > > > bool mask_writeonly:1; > > + bool no_of:1; > > > > and in the regmap_add_irq_chip do: > > > node = (chip->no_of) ? NULL : map->dev->of_node; > > d->domain = irq_domain_add_linear(node, chip->num_irqs, > > ®map_domain_ops, d); > > > Then we could have chip->no_of set for the 'main irq chip' and for those > > chips we don't wan't to expose via DT. In my case I would leave no_of > > unset only for the irq-chip which I created for the GPIO? Is this a > > silly idea? > > That's worth a shot, yes - I'd need to see it fully fleshed out I think > but it looks sensible (no ternery operator please). Do you think this would be benefical even if we add the 'main irq support'? If so, I can generate a patch out of this. I think this would really suffice for my current need - but this stops wokrking as soon as more than one sub-irq-chip want's to expose interrupts via DT. > > > The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware > > > gets structured is that the central interrupt controller is saying which > > > IP block is flagging an interrupt rather than which register has an > > > interrupt set in it. You can then have either more than one detailed > > > status register for that IP > > > Correct. But I guess the IP blocks often have limited set of registers for the > > "sub interrupts". For such cases my idea would work, right. My RFC > > handled case where there is many 'sub registers' to read for one 'main > > bit. > > Your idea definitely works for the current case, yes - I'm just thinking > about future edge and extension cases. I could send an example on how the driver utilizing the original RFC interface would look like. I am starting to think it was not *that* bad after all... > > > or several smaller IPs reporting through a > > > single detailed status register. > > > I think that if we have more than two layers of irqs (main and sub) - > > then we should do cascaded controllers. > > Yeah, and my first thought here is that we should just be using cascaded > controllers all the time (but like I say I'm not 100% certain on that). > > > > Right, it's about working out which subregister to read - the point is > > > you can do this by adding a link in either direction, I'm suggesting > > > doing it from the individual interrupt to the main register since we > > > already have individual data structures for those and it feels less > > > likely to run into hard to represent corner cases. > > > I see your point now. But as I said, I am not sure we should add the > > overhead of 'main irq bit description' for simple cases just to cover > > the corner cases. Yet I can try seeing what I can come up with if you > > think this is the way to go. > > If you could take a look that'd be great. I did some experiment. I will post this as another RFC - but I am really not terribly happy about it. It's complex (well, in my opinion) and I am not sure the driver interface is much easier. But you can see it yourself. -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-14 13:58 ` Matti Vaittinen @ 2018-12-14 17:26 ` Mark Brown 2018-12-17 8:19 ` Matti Vaittinen 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2018-12-14 17:26 UTC (permalink / raw) To: Matti Vaittinen; +Cc: gregkh, rafael, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1833 bytes --] On Fri, Dec 14, 2018 at 03:58:19PM +0200, Matti Vaittinen wrote: > On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote: > > > Then we could have chip->no_of set for the 'main irq chip' and for those > > > chips we don't wan't to expose via DT. In my case I would leave no_of > > > unset only for the irq-chip which I created for the GPIO? Is this a > > > silly idea? > > That's worth a shot, yes - I'd need to see it fully fleshed out I think > > but it looks sensible (no ternery operator please). > Do you think this would be benefical even if we add the 'main irq > support'? If so, I can generate a patch out of this. I think this would > really suffice for my current need - but this stops wokrking as soon as > more than one sub-irq-chip want's to expose interrupts via DT. Hrm, yeah. That's a thing. I think I'd misvisualized the DT change as being the other way around for some reason. > > Your idea definitely works for the current case, yes - I'm just thinking > > about future edge and extension cases. > I could send an example on how the driver utilizing the original RFC > interface would look like. I am starting to think it was not *that* bad > after all... That might help, yes. > > > I see your point now. But as I said, I am not sure we should add the > > > overhead of 'main irq bit description' for simple cases just to cover > > > the corner cases. Yet I can try seeing what I can come up with if you > > > think this is the way to go. > > If you could take a look that'd be great. > I did some experiment. I will post this as another RFC - but I am really > not terribly happy about it. It's complex (well, in my opinion) and I am > not sure the driver interface is much easier. But you can see it > yourself. Great, I'll take a proper look on Monday. Thanks for putting the time in here! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-14 17:26 ` Mark Brown @ 2018-12-17 8:19 ` Matti Vaittinen 2018-12-17 8:30 ` Matti Vaittinen 2018-12-17 18:50 ` Mark Brown 0 siblings, 2 replies; 11+ messages in thread From: Matti Vaittinen @ 2018-12-17 8:19 UTC (permalink / raw) To: Mark Brown; +Cc: gregkh, rafael, linux-kernel, heikki.haikola, mikko.mutanen Hello Mark & All, On Fri, Dec 14, 2018 at 05:26:19PM +0000, Mark Brown wrote: > On Fri, Dec 14, 2018 at 03:58:19PM +0200, Matti Vaittinen wrote: > > On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote: > > > I could send an example on how the driver utilizing the original RFC > > interface would look like. I am starting to think it was not *that* bad > > after all... > > That might help, yes. > This is what I came up with. I think having the simple mapping arrays (bitX_offsets) and REGMAP_IRQ_MAIN_REG_OFFSET macro makes this not-so-bad :) I will also send an example on how this would look like with the RFC v2 interface. #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/mfd/core.h> #include <linux/mfd/rohm-bd70528.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/regmap.h> #include <linux/types.h> static struct mfd_cell bd70528_mfd_cells[] = { { .name = "bd70528-pmic", }, { .name = "bd70528-rtc", }, { .name = "bd70528-gpio", }, { .name = "bd70528-led", }, { .name = "bd70528-power", }, { .name = "bd70528-clk", }, { .name = "bd70528-pwrkey", }, }; static const struct regmap_range bd70528_irq_reg_range = { .range_min = BD70528_REG_INT_MAIN, .range_max = BD70528_REG_INT_OP_FAIL, }; static struct regmap_access_table volatile_regs = { .yes_ranges = &bd70528_irq_reg_range, .n_yes_ranges = 1, }; static struct regmap_config bd70528_regmap = { .reg_bits = 8, .val_bits = 8, .volatile_table = &volatile_regs, .max_register = BD70528_MAX_REGISTER, .cache_type = REGCACHE_RBTREE, }; /* bit [0] - Shutdown register */ unsigned int bit0_offsets[] = {0}; /* bit [1] - Power failure register */ unsigned int bit1_offsets[] = {1}; /* bit [2] - VR FAULT register */ unsigned int bit2_offsets[] = {2}; /* bit [3] - PMU register interrupts */ unsigned int bit3_offsets[] = {3}; /* bit [4] - Charger 1 and Charger 2 registers */ unsigned int bit4_offsets[] = {4, 5}; /* bit [5] - RTC register */ unsigned int bit5_offsets[] = {6}; /* bit [6] - GPIO register */ unsigned int bit6_offsets[] = {7}; /* bit [7] - Invalid operation register */ unsigned int bit7_offsets[] = {8}; static struct regmap_irq_sub_irq_map bd70528_sub_irq_offsets[] = { REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets), }; static struct regmap_irq bd70528_irqs[] = { REGMAP_IRQ_REG(BD70528_INT_LONGPUSH, 0, BD70528_INT_LONGPUSH_MASK), REGMAP_IRQ_REG(BD70528_INT_WDT, 0, BD70528_INT_WDT_MASK), REGMAP_IRQ_REG(BD70528_INT_HWRESET, 0, BD70528_INT_HWRESET_MASK), REGMAP_IRQ_REG(BD70528_INT_RSTB_FAULT, 0, BD70528_INT_RSTB_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_VBAT_UVLO, 0, BD70528_INT_VBAT_UVLO_MASK), REGMAP_IRQ_REG(BD70528_INT_TSD, 0, BD70528_INT_TSD_MASK), REGMAP_IRQ_REG(BD70528_INT_RSTIN, 0, BD70528_INT_RSTIN_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK1_FAULT, 1, BD70528_INT_BUCK1_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK2_FAULT, 1, BD70528_INT_BUCK2_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK3_FAULT, 1, BD70528_INT_BUCK3_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_LDO1_FAULT, 1, BD70528_INT_LDO1_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_LDO2_FAULT, 1, BD70528_INT_LDO2_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_LDO3_FAULT, 1, BD70528_INT_LDO3_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_LED1_FAULT, 1, BD70528_INT_LED1_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_LED2_FAULT, 1, BD70528_INT_LED2_FAULT_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK1_OCP, 2, BD70528_INT_BUCK1_OCP_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK2_OCP, 2, BD70528_INT_BUCK2_OCP_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK3_OCP, 2, BD70528_INT_BUCK3_OCP_MASK), REGMAP_IRQ_REG(BD70528_INT_LED1_OCP, 2, BD70528_INT_LED1_OCP_MASK), REGMAP_IRQ_REG(BD70528_INT_LED2_OCP, 2, BD70528_INT_LED2_OCP_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK1_FULLON, 2, BD70528_INT_BUCK1_FULLON_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK2_FULLON, 2, BD70528_INT_BUCK2_FULLON_MASK), REGMAP_IRQ_REG(BD70528_INT_SHORTPUSH, 3, BD70528_INT_SHORTPUSH_MASK), REGMAP_IRQ_REG(BD70528_INT_AUTO_WAKEUP, 3, BD70528_INT_AUTO_WAKEUP_MASK), REGMAP_IRQ_REG(BD70528_INT_STATE_CHANGE, 3, BD70528_INT_STATE_CHANGE_MASK), REGMAP_IRQ_REG(BD70528_INT_BAT_OV_RES, 4, BD70528_INT_BAT_OV_RES_MASK), REGMAP_IRQ_REG(BD70528_INT_BAT_OV_DET, 4, BD70528_INT_BAT_OV_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_DBAT_DET, 4, BD70528_INT_DBAT_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_RES, 4, BD70528_INT_BATTSD_COLD_RES_MASK), REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_DET, 4, BD70528_INT_BATTSD_COLD_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_RES, 4, BD70528_INT_BATTSD_HOT_RES_MASK), REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_DET, 4, BD70528_INT_BATTSD_HOT_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_CHG_TSD, 4, BD70528_INT_CHG_TSD_MASK), REGMAP_IRQ_REG(BD70528_INT_BAT_RMV, 5, BD70528_INT_BAT_RMV_MASK), REGMAP_IRQ_REG(BD70528_INT_BAT_DET, 5, BD70528_INT_BAT_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_RES, 5, BD70528_INT_DCIN2_OV_RES_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_DET, 5, BD70528_INT_DCIN2_OV_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN2_RMV, 5, BD70528_INT_DCIN2_RMV_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN2_DET, 5, BD70528_INT_DCIN2_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN1_RMV, 5, BD70528_INT_DCIN1_RMV_MASK), REGMAP_IRQ_REG(BD70528_INT_DCIN1_DET, 5, BD70528_INT_DCIN1_DET_MASK), REGMAP_IRQ_REG(BD70528_INT_RTC_ALARM, 6, BD70528_INT_RTC_ALARM_MASK), REGMAP_IRQ_REG(BD70528_INT_ELPS_TIM, 6, BD70528_INT_ELPS_TIM_MASK), REGMAP_IRQ_REG(BD70528_INT_GPIO0, 7, BD70528_INT_GPIO0_MASK), REGMAP_IRQ_REG(BD70528_INT_GPIO1, 7, BD70528_INT_GPIO1_MASK), REGMAP_IRQ_REG(BD70528_INT_GPIO2, 7, BD70528_INT_GPIO2_MASK), REGMAP_IRQ_REG(BD70528_INT_GPIO3, 7, BD70528_INT_GPIO3_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK1_DVS_OPFAIL, 8, BD70528_INT_BUCK1_DVS_OPFAIL_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK2_DVS_OPFAIL, 8, BD70528_INT_BUCK2_DVS_OPFAIL_MASK), REGMAP_IRQ_REG(BD70528_INT_BUCK3_DVS_OPFAIL, 8, BD70528_INT_BUCK3_DVS_OPFAIL_MASK), REGMAP_IRQ_REG(BD70528_INT_LED1_VOLT_OPFAIL, 8, BD70528_INT_LED1_VOLT_OPFAIL_MASK), REGMAP_IRQ_REG(BD70528_INT_LED2_VOLT_OPFAIL, 8, BD70528_INT_LED2_VOLT_OPFAIL_MASK), }; static struct regmap_irq_chip bd70528_irq_chip = { .name = "bd70528_irq", .main_status = BD70528_REG_INT_MAIN, .irqs = &bd70528_irqs[0], .num_irqs = ARRAY_SIZE(bd70528_irqs), .status_base = BD70528_REG_INT_SHDN, .mask_base = BD70528_REG_INT_SHDN_MASK, .ack_base = BD70528_REG_INT_SHDN, .init_ack_masked = true, .num_regs = 9, .num_main_regs = 1, .sub_reg_offsets = &bd70528_sub_irq_offsets[0], .num_main_status_bits = 8, .irq_reg_stride = 1, }; static int bd70528_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct bd70528 *bd70528; struct regmap_irq_chip_data *irq_data; int ret, i; if (!i2c->irq) { dev_err(&i2c->dev, "No IRQ configured\n"); return -EINVAL; } bd70528 = devm_kzalloc(&i2c->dev, sizeof(*bd70528), GFP_KERNEL); if (!bd70528) return -ENOMEM; dev_set_drvdata(&i2c->dev, bd70528); bd70528->regmap = devm_regmap_init_i2c(i2c, &bd70528_regmap); if (IS_ERR(bd70528->regmap)) { dev_err(&i2c->dev, "regmap initialization failed\n"); return PTR_ERR(bd70528->regmap); } ret = devm_regmap_add_irq_chip(&i2c->dev, bd70528->regmap, i2c->irq, IRQF_ONESHOT, 0, &bd70528_irq_chip, &irq_data); if (ret) { dev_err(&i2c->dev, "Failed to add irq_chip\n"); return ret; } else pr_info("Registered %d irqs for chip\n", bd70528_irq_chip.num_irqs); ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bd70528_mfd_cells, ARRAY_SIZE(bd70528_mfd_cells), NULL, 0, regmap_irq_get_domain(irq_data)); if (ret) dev_err(&i2c->dev, "Failed to create subdevices\n"); return ret; } -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-17 8:19 ` Matti Vaittinen @ 2018-12-17 8:30 ` Matti Vaittinen 2018-12-17 18:50 ` Mark Brown 1 sibling, 0 replies; 11+ messages in thread From: Matti Vaittinen @ 2018-12-17 8:30 UTC (permalink / raw) To: Mark Brown Cc: gregkh, rafael, linux-kernel, heikki.haikola, mikko.mutanen, mazziesaccount, matti.vaittinen On Mon, Dec 17, 2018 at 10:19:12AM +0200, Matti Vaittinen wrote: > Hello Mark & All, > > On Fri, Dec 14, 2018 at 05:26:19PM +0000, Mark Brown wrote: > > On Fri, Dec 14, 2018 at 03:58:19PM +0200, Matti Vaittinen wrote: > > > On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote: > > > > > I could send an example on how the driver utilizing the original RFC > > > interface would look like. I am starting to think it was not *that* bad > > > after all... > > > > That might help, yes. > > > This is what I came up with. I think having the simple mapping arrays > (bitX_offsets) and REGMAP_IRQ_MAIN_REG_OFFSET macro makes this > not-so-bad :) I will also send an example on how this would look like > with the RFC v2 interface. I decided to go with diff between RFC v1 and RFC v2 driver interfaces: diff --git a/drivers/mfd/rohm-bd70528.c b/drivers/mfd/rohm-bd70528.c index b270cdad1db6..abb0cff8d1c2 100644 --- a/drivers/mfd/rohm-bd70528.c +++ b/drivers/mfd/rohm-bd70528.c @@ -41,92 +41,114 @@ static struct regmap_config bd70528_regmap = { .max_register = BD70528_MAX_REGISTER, .cache_type = REGCACHE_RBTREE, }; -/* bit [0] - Shutdown register */ -unsigned int bit0_offsets[] = {0}; -/* bit [1] - Power failure register */ -unsigned int bit1_offsets[] = {1}; -/* bit [2] - VR FAULT register */ -unsigned int bit2_offsets[] = {2}; -/* bit [3] - PMU register interrupts */ -unsigned int bit3_offsets[] = {3}; -/* bit [4] - Charger 1 and Charger 2 registers */ -unsigned int bit4_offsets[] = {4,5}; -/* bit [5] - RTC register */ -unsigned int bit5_offsets[] = {6}; -/* bit [6] - GPIO register */ -unsigned int bit6_offsets[] = {7}; -/* bit [7] - Invalid operation register */ -unsigned int bit7_offsets[] = {8}; - -static struct regmap_irq_sub_irq_map bd70528_sub_irq_offsets[] = { - REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets), - REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets), -}; static struct regmap_irq bd70528_irqs[] = { - REGMAP_IRQ_REG(BD70528_INT_LONGPUSH, 0, BD70528_INT_LONGPUSH_MASK), - REGMAP_IRQ_REG(BD70528_INT_WDT, 0, BD70528_INT_WDT_MASK), - REGMAP_IRQ_REG(BD70528_INT_HWRESET, 0, BD70528_INT_HWRESET_MASK), - REGMAP_IRQ_REG(BD70528_INT_RSTB_FAULT, 0, BD70528_INT_RSTB_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_VBAT_UVLO, 0, BD70528_INT_VBAT_UVLO_MASK), - REGMAP_IRQ_REG(BD70528_INT_TSD, 0, BD70528_INT_TSD_MASK), - REGMAP_IRQ_REG(BD70528_INT_RSTIN, 0, BD70528_INT_RSTIN_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK1_FAULT, 1, BD70528_INT_BUCK1_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK2_FAULT, 1, BD70528_INT_BUCK2_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK3_FAULT, 1, BD70528_INT_BUCK3_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_LDO1_FAULT, 1, BD70528_INT_LDO1_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_LDO2_FAULT, 1, BD70528_INT_LDO2_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_LDO3_FAULT, 1, BD70528_INT_LDO3_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED1_FAULT, 1, BD70528_INT_LED1_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED2_FAULT, 1, BD70528_INT_LED2_FAULT_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK1_OCP, 2, BD70528_INT_BUCK1_OCP_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK2_OCP, 2, BD70528_INT_BUCK2_OCP_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK3_OCP, 2, BD70528_INT_BUCK3_OCP_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED1_OCP, 2, BD70528_INT_LED1_OCP_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED2_OCP, 2, BD70528_INT_LED2_OCP_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK1_FULLON, 2, BD70528_INT_BUCK1_FULLON_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK2_FULLON, 2, BD70528_INT_BUCK2_FULLON_MASK), - REGMAP_IRQ_REG(BD70528_INT_SHORTPUSH, 3, BD70528_INT_SHORTPUSH_MASK), - REGMAP_IRQ_REG(BD70528_INT_AUTO_WAKEUP, 3, BD70528_INT_AUTO_WAKEUP_MASK), - REGMAP_IRQ_REG(BD70528_INT_STATE_CHANGE, 3, BD70528_INT_STATE_CHANGE_MASK), - REGMAP_IRQ_REG(BD70528_INT_BAT_OV_RES, 4, BD70528_INT_BAT_OV_RES_MASK), - REGMAP_IRQ_REG(BD70528_INT_BAT_OV_DET, 4, BD70528_INT_BAT_OV_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_DBAT_DET, 4, BD70528_INT_DBAT_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_RES, 4, BD70528_INT_BATTSD_COLD_RES_MASK), - REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_DET, 4, BD70528_INT_BATTSD_COLD_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_RES, 4, BD70528_INT_BATTSD_HOT_RES_MASK), - REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_DET, 4, BD70528_INT_BATTSD_HOT_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_CHG_TSD, 4, BD70528_INT_CHG_TSD_MASK), - REGMAP_IRQ_REG(BD70528_INT_BAT_RMV, 5, BD70528_INT_BAT_RMV_MASK), - REGMAP_IRQ_REG(BD70528_INT_BAT_DET, 5, BD70528_INT_BAT_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_RES, 5, BD70528_INT_DCIN2_OV_RES_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_DET, 5, BD70528_INT_DCIN2_OV_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN2_RMV, 5, BD70528_INT_DCIN2_RMV_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN2_DET, 5, BD70528_INT_DCIN2_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN1_RMV, 5, BD70528_INT_DCIN1_RMV_MASK), - REGMAP_IRQ_REG(BD70528_INT_DCIN1_DET, 5, BD70528_INT_DCIN1_DET_MASK), - REGMAP_IRQ_REG(BD70528_INT_RTC_ALARM, 6, BD70528_INT_RTC_ALARM_MASK), - REGMAP_IRQ_REG(BD70528_INT_ELPS_TIM, 6, BD70528_INT_ELPS_TIM_MASK), - REGMAP_IRQ_REG(BD70528_INT_GPIO0, 7, BD70528_INT_GPIO0_MASK), - REGMAP_IRQ_REG(BD70528_INT_GPIO1, 7, BD70528_INT_GPIO1_MASK), - REGMAP_IRQ_REG(BD70528_INT_GPIO2, 7, BD70528_INT_GPIO2_MASK), - REGMAP_IRQ_REG(BD70528_INT_GPIO3, 7, BD70528_INT_GPIO3_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK1_DVS_OPFAIL, 8, BD70528_INT_BUCK1_DVS_OPFAIL_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK2_DVS_OPFAIL, 8, BD70528_INT_BUCK2_DVS_OPFAIL_MASK), - REGMAP_IRQ_REG(BD70528_INT_BUCK3_DVS_OPFAIL, 8, BD70528_INT_BUCK3_DVS_OPFAIL_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED1_VOLT_OPFAIL, 8, BD70528_INT_LED1_VOLT_OPFAIL_MASK), - REGMAP_IRQ_REG(BD70528_INT_LED2_VOLT_OPFAIL, 8, BD70528_INT_LED2_VOLT_OPFAIL_MASK), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LONGPUSH, 0, BD70528_INT_LONGPUSH_MASK, + 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_WDT, 0, BD70528_INT_WDT_MASK, 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_HWRESET, 0, BD70528_INT_HWRESET_MASK, 0, + 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_RSTB_FAULT, 0, + BD70528_INT_RSTB_FAULT_MASK, 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_VBAT_UVLO, 0, + BD70528_INT_VBAT_UVLO_MASK, 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_TSD, 0, BD70528_INT_TSD_MASK, 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_RSTIN, 0, BD70528_INT_RSTIN_MASK, 0, 1), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK1_FAULT, 1, + BD70528_INT_BUCK1_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK2_FAULT, 1, + BD70528_INT_BUCK2_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK3_FAULT, 1, + BD70528_INT_BUCK3_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LDO1_FAULT, 1, + BD70528_INT_LDO1_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LDO2_FAULT, 1, + BD70528_INT_LDO2_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LDO3_FAULT, 1, + BD70528_INT_LDO3_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED1_FAULT, 1, + BD70528_INT_LED1_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED2_FAULT, 1, + BD70528_INT_LED2_FAULT_MASK, 0, 2), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK1_OCP, 2, + BD70528_INT_BUCK1_OCP_MASK, 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK2_OCP, 2, + BD70528_INT_BUCK2_OCP_MASK, 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK3_OCP, 2, + BD70528_INT_BUCK3_OCP_MASK, 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED1_OCP, 2, BD70528_INT_LED1_OCP_MASK, + 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED2_OCP, 2, BD70528_INT_LED2_OCP_MASK, + 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK1_FULLON, 2, + BD70528_INT_BUCK1_FULLON_MASK, 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK2_FULLON, 2, + BD70528_INT_BUCK2_FULLON_MASK, 0, 3), + REGMAP_IRQ_REG_MAIN(BD70528_INT_SHORTPUSH, 3, + BD70528_INT_SHORTPUSH_MASK, 0, 4), + REGMAP_IRQ_REG_MAIN(BD70528_INT_AUTO_WAKEUP, 3, + BD70528_INT_AUTO_WAKEUP_MASK, 0, 4), + REGMAP_IRQ_REG_MAIN(BD70528_INT_STATE_CHANGE, 3, + BD70528_INT_STATE_CHANGE_MASK, 0, 4), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BAT_OV_RES, 4, + BD70528_INT_BAT_OV_RES_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BAT_OV_DET, 4, + BD70528_INT_BAT_OV_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DBAT_DET, 4, BD70528_INT_DBAT_DET_MASK, + 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BATTSD_COLD_RES, 4, + BD70528_INT_BATTSD_COLD_RES_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BATTSD_COLD_DET, 4, + BD70528_INT_BATTSD_COLD_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BATTSD_HOT_RES, 4, + BD70528_INT_BATTSD_HOT_RES_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BATTSD_HOT_DET, 4, + BD70528_INT_BATTSD_HOT_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_CHG_TSD, 4, + BD70528_INT_CHG_TSD_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BAT_RMV, 5, BD70528_INT_BAT_RMV_MASK, + 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BAT_DET, 5, BD70528_INT_BAT_DET_MASK, 0, + 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN2_OV_RES, 5, + BD70528_INT_DCIN2_OV_RES_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN2_OV_DET, 5, + BD70528_INT_DCIN2_OV_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN2_RMV, 5, + BD70528_INT_DCIN2_RMV_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN2_DET, 5, + BD70528_INT_DCIN2_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN1_RMV, 5, + BD70528_INT_DCIN1_RMV_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_DCIN1_DET, 5, + BD70528_INT_DCIN1_DET_MASK, 0, 5), + REGMAP_IRQ_REG_MAIN(BD70528_INT_RTC_ALARM, 6, + BD70528_INT_RTC_ALARM_MASK, 0, 6), + REGMAP_IRQ_REG_MAIN(BD70528_INT_ELPS_TIM, 6, BD70528_INT_ELPS_TIM_MASK, + 0, 6), + REGMAP_IRQ_REG_MAIN(BD70528_INT_GPIO0, 7, BD70528_INT_GPIO0_MASK, 0, 7), + REGMAP_IRQ_REG_MAIN(BD70528_INT_GPIO1, 7, BD70528_INT_GPIO1_MASK, 0, 7), + REGMAP_IRQ_REG_MAIN(BD70528_INT_GPIO2, 7, BD70528_INT_GPIO2_MASK, 0, 7), + REGMAP_IRQ_REG_MAIN(BD70528_INT_GPIO3, 7, BD70528_INT_GPIO3_MASK, 0, 7), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK1_DVS_OPFAIL, 8, + BD70528_INT_BUCK1_DVS_OPFAIL_MASK, 0, 8), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK2_DVS_OPFAIL, 8, + BD70528_INT_BUCK2_DVS_OPFAIL_MASK, 0, 8), + REGMAP_IRQ_REG_MAIN(BD70528_INT_BUCK3_DVS_OPFAIL, 8, + BD70528_INT_BUCK3_DVS_OPFAIL_MASK, 0, 8), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED1_VOLT_OPFAIL, 8, + BD70528_INT_LED1_VOLT_OPFAIL_MASK, 0, 8), + REGMAP_IRQ_REG_MAIN(BD70528_INT_LED2_VOLT_OPFAIL, 8, + BD70528_INT_LED2_VOLT_OPFAIL_MASK, 0, 8), +}; + +static struct regmap_main_status main_irq_reg = { + .main_status = BD70528_REG_INT_MAIN, + .num_main_regs = 1, }; static struct regmap_irq_chip bd70528_irq_chip = { .name = "bd70528_irq", - .main_status = BD70528_REG_INT_MAIN, .irqs = &bd70528_irqs[0], .num_irqs = ARRAY_SIZE(bd70528_irqs), .status_base = BD70528_REG_INT_SHDN, @@ -135,11 +157,9 @@ static struct regmap_irq_chip bd70528_irq_chip = { .type_base = BD70528_GPIO1_IN_REG, .init_ack_masked = true, .num_regs = 9, - .num_main_regs = 1, .num_type_reg = 4, - .sub_reg_offsets = &bd70528_sub_irq_offsets[0], - .num_main_status_bits = 8, .irq_reg_stride = 1, + .main_reg = &main_irq_reg, }; Br, Matti Vaittinen ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC] regmap-irq: add "main register" and level-irq support 2018-12-17 8:19 ` Matti Vaittinen 2018-12-17 8:30 ` Matti Vaittinen @ 2018-12-17 18:50 ` Mark Brown 1 sibling, 0 replies; 11+ messages in thread From: Mark Brown @ 2018-12-17 18:50 UTC (permalink / raw) To: Matti Vaittinen Cc: gregkh, rafael, linux-kernel, heikki.haikola, mikko.mutanen [-- Attachment #1: Type: text/plain, Size: 770 bytes --] On Mon, Dec 17, 2018 at 10:19:12AM +0200, Matti Vaittinen wrote: > On Fri, Dec 14, 2018 at 05:26:19PM +0000, Mark Brown wrote: > > On Fri, Dec 14, 2018 at 03:58:19PM +0200, Matti Vaittinen wrote: > > > On Fri, Dec 07, 2018 at 01:14:18PM +0000, Mark Brown wrote: > > > I could send an example on how the driver utilizing the original RFC > > > interface would look like. I am starting to think it was not *that* bad > > > after all... > > That might help, yes. > This is what I came up with. I think having the simple mapping arrays > (bitX_offsets) and REGMAP_IRQ_MAIN_REG_OFFSET macro makes this > not-so-bad :) I will also send an example on how this would look like > with the RFC v2 interface. I think your approach is fine - there's not much difference really. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-12-17 18:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-30 8:59 [RFC] regmap-irq: add "main register" and level-irq support Matti Vaittinen 2018-12-04 17:21 ` Mark Brown 2018-12-05 8:22 ` Matti Vaittinen 2018-12-05 17:27 ` Mark Brown 2018-12-07 7:58 ` Matti Vaittinen 2018-12-07 13:14 ` Mark Brown 2018-12-14 13:58 ` Matti Vaittinen 2018-12-14 17:26 ` Mark Brown 2018-12-17 8:19 ` Matti Vaittinen 2018-12-17 8:30 ` Matti Vaittinen 2018-12-17 18:50 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).