Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

From: Vaibhav Hiremath
Date: Mon Aug 24 2015 - 11:20:21 EST

On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:

As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
(page 0) controls the method of clearing interrupt
status of 88pm800 family of devices;

0: clear on read
1: clear on write

If pdata is not coming from board file, then set the
default irq clear method to "irq clear on write"

Also, as suggested by "Lee Jones" renaming variable field
to appropriate name and removed unnecessary field
pm80x_chip.irq_mode, using platform_data.irq_clr_method.

Signed-off-by: Zhao Ye <zhaoy@xxxxxxxxxxx>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@xxxxxxxxxx>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
drivers/mfd/88pm800.c | 15 ++++++++++-----
include/linux/mfd/88pm80x.h | 9 +++++++--
2 files changed, 17 insertions(+), 7 deletions(-)


+#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)

Use BIT().

+/* Used by irq_clr_method */
+#define PM800_IRQ_CLR_ON_READ 0
+#define PM800_IRQ_CLR_ON_WRITE 1

- int irq_mode; /* Clear interrupt by read/write(0/1) */
+ bool irq_clr_method; /* Clear interrupt by read/write(0/1) */

+ irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
+ ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);

This is pretty convoluted.

For starters you're abusing the 'bool' type here. Bool is either
'true' or 'false', so at the very least you should rename
'irq_clr_method' to 'irq_clr_on_write'.

Then you can do:

irq_clr_mode = pdata->irq_clr_on_write ?

We have discussed on this, and went back-n-forth.
I think if I remember correctly, one of the version was using
true/false then we decided to rename it to relevant macro.

If I am not wrong V4 version of this series is exactly same as what you
are referring to.

However, what I suggest you really do is share
PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
the value through directly.

I think we discussed about this also, and the reason I recall here is,

we may need to control this from DT in the future so we decided to keep
it boolean in platform_data and have simple check before writing to

And I think that was also another reason we introduced

/* Used by irq_clr_method */
#define PM800_IRQ_CLR_ON_READ 0
#define PM800_IRQ_CLR_ON_WRITE 1

(Earlier it was true/false in V4)

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at