[PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def

[PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def

[Kim, Milo]

Default mask value is used when updating irq registers.
Rather than using mask_buf_def[], use mask and value explicitly.

(a) remove default mask buffer : mask_buf_def[]

(b) add 'val_buf' for storing value of registers
  : val_buf is updated only when the irq is enabled or disabled

(c) In irq_sync_unlock, use 'mask_buf' and 'val_buf'
    on updating interrupt registers

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/base/regmap/regmap-irq.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index a897346..ed36ea2 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -34,7 +34,7 @@ struct regmap_irq_chip_data {
 
 	unsigned int *status_buf;
 	unsigned int *mask_buf;
-	unsigned int *mask_buf_def;
+	unsigned int *val_buf;
 	unsigned int *wake_buf;
 
 	unsigned int irq_reg_stride;
@@ -69,7 +69,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		ret = regmap_update_bits(d->map, d->chip->mask_base +
 						(i * map->reg_stride *
 						d->irq_reg_stride),
-					 d->mask_buf_def[i], d->mask_buf[i]);
+					 d->mask_buf[i], d->val_buf[i]);
 		if (ret != 0)
 			dev_err(d->map->dev, "Failed to sync masks in %x\n",
 				d->chip->mask_base + (i * map->reg_stride));
@@ -94,7 +94,7 @@ static void regmap_irq_enable(struct irq_data *data)
 	struct regmap *map = d->map;
 	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
 
-	d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~irq_data->mask;
+	d->val_buf[irq_data->reg_offset / map->reg_stride] &= ~irq_data->mask;
 }
 
 static void regmap_irq_disable(struct irq_data *data)
@@ -103,7 +103,7 @@ static void regmap_irq_disable(struct irq_data *data)
 	struct regmap *map = d->map;
 	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
 
-	d->mask_buf[irq_data->reg_offset / map->reg_stride] |= irq_data->mask;
+	d->val_buf[irq_data->reg_offset / map->reg_stride] |= irq_data->mask;
 }
 
 static int regmap_irq_set_wake(struct irq_data *data, unsigned int on)
@@ -272,9 +272,9 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	if (!d->mask_buf)
 		goto err_alloc;
 
-	d->mask_buf_def = kzalloc(sizeof(unsigned int) * chip->num_regs,
+	d->val_buf = kzalloc(sizeof(unsigned int) * chip->num_regs,
 				  GFP_KERNEL);
-	if (!d->mask_buf_def)
+	if (!d->val_buf)
 		goto err_alloc;
 
 	if (chip->wake_base) {
@@ -297,12 +297,11 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	mutex_init(&d->lock);
 
 	for (i = 0; i < chip->num_irqs; i++)
-		d->mask_buf_def[chip->irqs[i].reg_offset / map->reg_stride]
+		d->mask_buf[chip->irqs[i].reg_offset / map->reg_stride]
 			|= chip->irqs[i].mask;
 
 	/* Mask all the interrupts by default */
 	for (i = 0; i < chip->num_regs; i++) {
-		d->mask_buf[i] = d->mask_buf_def[i];
 		ret = regmap_write(map, chip->mask_base + (i * map->reg_stride
 				   * d->irq_reg_stride),
 				   d->mask_buf[i]);
@@ -340,7 +339,7 @@ err_domain:
 	/* Should really dispose of the domain but... */
 err_alloc:
 	kfree(d->wake_buf);
-	kfree(d->mask_buf_def);
+	kfree(d->val_buf);
 	kfree(d->mask_buf);
 	kfree(d->status_buf);
 	kfree(d);
@@ -362,7 +361,7 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 	free_irq(irq, d);
 	/* We should unmap the domain but... */
 	kfree(d->wake_buf);
-	kfree(d->mask_buf_def);
+	kfree(d->val_buf);
 	kfree(d->mask_buf);
 	kfree(d->status_buf);
 	kfree(d);
-- 
1.7.2.5


Best Regards,
Milo

Re: [PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 274 bytes --]

On Thu, Jul 19, 2012 at 09:04:39AM +0000, Kim, Milo wrote:
> Default mask value is used when updating irq registers.
> Rather than using mask_buf_def[], use mask and value explicitly.

Why?  What is the problem you are seeing and what is the intended effect
of this change?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def

[Kim, Milo]

> On Thu, Jul 19, 2012 at 09:04:39AM +0000, Kim, Milo wrote:
> > Default mask value is used when updating irq registers.
> > Rather than using mask_buf_def[], use mask and value explicitly.
> 
> Why?  What is the problem you are seeing and what is the intended
> effect
> of this change?

There are two reasons for this patch.
One is for better understanding about masking and updated bits.
The other is related with supporting interrupt-unmasked device.

(a) 'mask_buf & val_buf' VS 'mask_buf_def & mask_buf'
In my opinion, the mask_buf_def[] is 'masking bit' and the mask_buf[] is the 'value' to be updated when the IRQ is enabled/disabled.
The mask_buf_def[] means which bit is updated - that is exactly *mask bit*.
The value of mask_buf_def[] is copied from mask_buf[] when regmap_add_irq_chip() is called.
And then mask_buf_def[] is fixed value.
On the other hand, the mask_buf[] is variable. The value is updated whenever the IRQ is enabled/disabled.
Which is better understandable terminology ? 'mask and value' or 'default mask and updated mask'
I think 'mask & value' is more clear.

(b) supporting interrupt-unmasked device
There is different interrupt concept from interrupt-masked device.
To enable the IRQ, the register bit should be 1.
To update this value, the bit of IRQ value should be separated from the mask bit.

< Example >
Let me assume that enabling the IRQ0 and IRQ1 ... (8 bits register based)

(Interrupt mask device)
IRQ7 | IRQ6 | IRQ5 | IRQ4 | IRQ3 | IRQ2 | IRQ1 | IRQ0
 1      1      1      1      1      1      0      0
Mask  = 0xFC
Value = 0xFC (same as mask bit)

(Interrupt unmask device)
IRQ7 | IRQ6 | IRQ5 | IRQ4 | IRQ3 | IRQ2 | IRQ1 | IRQ0
 0      0      0      0      0      0      1      1
Mask  = 0xFC
Value = 0x03 (different from mask bit)

Let's move to regmap_irq data structure with two types.

(Interrupt mask device)
static struct regmap_irq foo_irqs[] = {
	[FOO_IRQ0] = {
		.reg_offset =  0;
		.mask = BIT(0), /* 0x01 */
	},
	[FOO_IRQ1] = {
		.reg_offset =  0;
		.mask = BIT(1), /* 0x02 */
	},
};

                   Initial time      irq_enable()  irq_enable()
 Value        (regmap_add_irq_chip)   with IRQ0     with IRQ1
_______________________________________________________________
mask_buf_def       0x03                0x03          0x03
mask_buf           0x03                0xFE          0xFD
updated bits    0000 0011           xxxx xxx0      xxxx xx0x
            (IRQ0,1 disabled)     (IRQ0 enabled)  (IRQ1 enabled)

Result: Works well.
        Only one bit is cleared when the IRQ is enabled.

(Interrupt unmask device)
Case 1)
static struct regmap_irq foo_irqs[] = {
	[FOO_IRQ0] = {
		.reg_offset =  0;
		.mask = BIT(0), /* 0x01 */
	},
	[FOO_IRQ1] = {
		.reg_offset =  0;
		.mask = BIT(1), /* 0x02 */
	},
};

                   Initial time      irq_enable()  irq_enable()
 Value        (regmap_add_irq_chip)   with IRQ0     with IRQ1
_______________________________________________________________
mask_buf_def       0x03                0x03          0x03
mask_buf           0x03                0xFE          0xFD
updated bits    0000 0011           xxxx xxx0      xxxx xx0x
            (IRQ0,1 enabled)     (IRQ0 disabled) (IRQ1 disabled)

Result: Not working.
        Each IRQ is disabled. Other bits can be enabled not intentionally.

Case 2) What if changing the 'mask' to inverted bit as following.
static struct regmap_irq foo_irqs[] = {
	[FOO_IRQ0] = {
		.reg_offset =  0;
		.mask = ~BIT(0), /* 0xFE */
	},
	[FOO_IRQ1] = {
		.reg_offset =  0;
		.mask = ~BIT(1), /* 0xFD */
	},
};

                   Initial time      irq_enable()  irq_enable()
 Value        (regmap_add_irq_chip)   with IRQ0     with IRQ1
_______________________________________________________________
mask_buf_def       0xFF                0xFF          0xFF
mask_buf           0xFF                0x01          0x02
updated bits    1111 1111           0000 0001      0000 0010
         (All IRQs are enabled)  (IRQ0 enabled,  (IRQ1 enabled,
                                  others are      but others are disabled)
                                  disabled)

Result: Not working.
        Other bits are cleared except flagged IRQ.
        Only one IRQ exists. Others are ignored. 

In both cases, it doesn't work with interrupt-unmasked-register scheme.
To fix this issue, two patches were sent.
The mask and value should be separated -- (1) and different bit operation is required -- (2).
(1) [PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def
(2) [PATCH 2/2] regmap-irq: support different type of irq register

And then the result will be as following.

(Interrupt mask device)

                   Initial time      irq_enable()  irq_enable()
 Value        (regmap_add_irq_chip)   with IRQ0     with IRQ1
_______________________________________________________________
mask_buf           0x03                0x03          0x03
val_buf             -                  0xFE          0xFD
updated bits     0000 0011           xxxx xxx0     xxxx xx0x
             (IRQ0,1 disabled)    (IRQ0 enabled)  (IRQ1 enabled)

(Interrupt unmask device)

                   Initial time      irq_enable()  irq_enable()
 Value        (regmap_add_irq_chip)   with IRQ0     with IRQ1
_______________________________________________________________
mask_buf           0x03                0x03          0x03
val_buf             -                  0x01          0x02
updated bits     1111 1100           xxxx xxx1     xxxx xx1x
             (IRQ0,1 disabled)    (IRQ0 enabled)  (IRQ1 enabled)


I'm not sure there is another device which supports the interrupt-unmasked register.
As far as I know, LP8788 is the only device :-(

Thanks !

Best Regards,
Milo

Re: [PATCH 1/2] regmap-irq: use mask and val rather than using mask_buf_def

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Fri, Jul 20, 2012 at 07:08:15AM +0000, Kim, Milo wrote:

Fix your mailer to word wrap within paragraphs, I have reformatted your
mail for legibility.

> Which is better understandable terminology ? 'mask and value' or
> 'default mask and updated mask' I think 'mask & value' is more clear.

But you need to say which value we're talking about - there's multiple
values we're working with here, the interrupt mask is only one of them.
We also have the wake masks and the actual interrupt status values for
example.

> (b) supporting interrupt-unmasked device
> There is different interrupt concept from interrupt-masked device.
> To enable the IRQ, the register bit should be 1.
> To update this value, the bit of IRQ value should be separated from the mask bit.

The variable you're changing isn't the interrupt value, it's the virtual
copy of the mask registers.  The interrupt status value is managed
separately.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]