With the existing logic by using regmap_write() all the bits in
the register are being updated which is not expected. To update only the
interrupt raised bit and not tocuhing other bits, replace regmap_write()
with regmap_irq_update_bits().

This patch is to fix the issue observed in MBHC button press/release events.

Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
---
Changes Since V1:
-- Update commit message.

 drivers/base/regmap/regmap-irq.c | 52 ++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index d2656581a608..bb9d07960dd0 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -184,16 +184,18 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 			/* some chips ack by write 0 */
 			if (d->chip->ack_invert)
-				ret = regmap_write(map, reg, ~d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], 0x00);
 			else
-				ret = regmap_write(map, reg, d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], d->mask_buf[i]);
 			if (d->chip->clear_ack) {
 				if (d->chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-							   d->mask_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], d->mask_buf[i]);
 				else if (!ret)
-					ret = regmap_write(map, reg,
-							   ~d->mask_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], 0x00);
 			}
 			if (ret != 0)
 				dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
@@ -549,18 +551,20 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 			reg = sub_irq_reg(data, data->chip->ack_base, i);
 
 			if (chip->ack_invert)
-				ret = regmap_write(map, reg,
-						~data->status_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i], 0x00);
 			else
-				ret = regmap_write(map, reg,
+				ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i],
 						data->status_buf[i]);
 			if (chip->clear_ack) {
 				if (chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-							data->status_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i],
+						data->status_buf[i]);
 				else if (!ret)
-					ret = regmap_write(map, reg,
-							~data->status_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i], 0x00);
 			}
 			if (ret != 0)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
@@ -810,20 +814,22 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
 			reg = sub_irq_reg(d, d->chip->ack_base, i);
 			if (chip->ack_invert)
-				ret = regmap_write(map, reg,
-					~(d->status_buf[i] & d->mask_buf[i]));
+				ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					0x00);
 			else
-				ret = regmap_write(map, reg,
-					d->status_buf[i] & d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					(d->status_buf[i] & d->mask_buf[i]));
 			if (chip->clear_ack) {
 				if (chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-						(d->status_buf[i] &
-						 d->mask_buf[i]));
+					ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					(d->status_buf[i] & d->mask_buf[i]));
 				else if (!ret)
-					ret = regmap_write(map, reg,
-						~(d->status_buf[i] &
-						  d->mask_buf[i]));
+					ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					0x00);
 			}
 			if (ret != 0) {
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
-- 
2.17.1

On Wed, 19 Jan 2022 19:59:53 +0530, Prasad Kumpatla wrote:
> With the existing logic by using regmap_write() all the bits in
> the register are being updated which is not expected. To update only the
> interrupt raised bit and not tocuhing other bits, replace regmap_write()
> with regmap_irq_update_bits().
> 
> This patch is to fix the issue observed in MBHC button press/release events.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-linus

Thanks!

[1/1] regmap-irq: Use regmap_irq_update_bits instead of regmap_write
      commit: 8419f8e559a78a3d1050dd8132ef04727e8881c1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Hi Prasad,

On 19.01.2022 15:29, Prasad Kumpatla wrote:
> With the existing logic by using regmap_write() all the bits in
> the register are being updated which is not expected. To update only the
> interrupt raised bit and not tocuhing other bits, replace regmap_write()
> with regmap_irq_update_bits().
>
> This patch is to fix the issue observed in MBHC button press/release events.
>
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>

There is something wrong with this patch. Since it landed in linux-next 
(20220204) I get an interrupt storm on two of my test devices:

1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
error" message)

2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
sample" message)

Definitely the interrupts are not acknowledged properly. Once I find 
some spare time, I will check it further which regmap configuration 
triggers the issue, but it is definitely related to this patch. 
Reverting it on top of current linux-next fixes the issue.

> ---
> Changes Since V1:
> -- Update commit message.
>
>   drivers/base/regmap/regmap-irq.c | 52 ++++++++++++++++++--------------
>   1 file changed, 29 insertions(+), 23 deletions(-)
>
> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> Hi Prasad,
> 
> On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > With the existing logic by using regmap_write() all the bits in
> > the register are being updated which is not expected. To update only the
> > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > with regmap_irq_update_bits().
> >
> > This patch is to fix the issue observed in MBHC button press/release events.
> >
> > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> 
> There is something wrong with this patch. Since it landed in linux-next 
> (20220204) I get an interrupt storm on two of my test devices:
> 
> 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
> error" message)
> 
> 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
> sample" message)
> 
> Definitely the interrupts are not acknowledged properly. Once I find 
> some spare time, I will check it further which regmap configuration 
> triggers the issue, but it is definitely related to this patch. 
> Reverting it on top of current linux-next fixes the issue.
> 

Yeah I was just looking at this patch it looks a bit weird. Like
most IRQ acks are write 1 to clear, so doing an update_bits seems
unlikely to work, as it will ack every pending interrupt. As the
whole idea of doing a regmap_write was to only write 1 to the bits
that need ACKed.

I suspect what is needed here is the inverted case wants an
update bits but the normal case needs to stay as a regmap_write.

Thanks,
Charles

On Tue, Feb 08, 2022 at 01:39:57PM +0000, Charles Keepax wrote:
> On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> > Hi Prasad,
> > 
> > On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > > With the existing logic by using regmap_write() all the bits in
> > > the register are being updated which is not expected. To update only the
> > > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > > with regmap_irq_update_bits().
> > >
> > > This patch is to fix the issue observed in MBHC button press/release events.
> > >
> > > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> > 
> > There is something wrong with this patch. Since it landed in linux-next 
> > (20220204) I get an interrupt storm on two of my test devices:
> > 
> > 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
> > error" message)
> > 
> > 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
> > sample" message)
> > 
> > Definitely the interrupts are not acknowledged properly. Once I find 
> > some spare time, I will check it further which regmap configuration 
> > triggers the issue, but it is definitely related to this patch. 
> > Reverting it on top of current linux-next fixes the issue.
> > 
> 
> Yeah I was just looking at this patch it looks a bit weird. Like
> most IRQ acks are write 1 to clear, so doing an update_bits seems
> unlikely to work, as it will ack every pending interrupt. As the
> whole idea of doing a regmap_write was to only write 1 to the bits
> that need ACKed.
> 
> I suspect what is needed here is the inverted case wants an
> update bits but the normal case needs to stay as a regmap_write.
> 

Apologies for the multiple emails, yeah looking at this I think
need some more information on how the hardware that patch was
addressing works. I don't quite understand what was wrong with
the old code even in the inverted case, the old code wrote a 1 to
every bit except the interrupt being cleared which gets a 0. This
feels like how I would have thought a write 0 to clear IRQ would
work, you don't want to clear any other bits so you write 1 to
them.

The update_bits is really problematic as even in the write 0 to
clear case, if a new interrupt asserts between the regmap_read
and regmap_write that make up the update_bits, you will clear that
new interrupt without ever noticing it.

Thanks,
Charles

On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> Hi Prasad,
> 
> On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > With the existing logic by using regmap_write() all the bits in
> > the register are being updated which is not expected. To update only the
> > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > with regmap_irq_update_bits().
> >
> > This patch is to fix the issue observed in MBHC button press/release events.
> >
> > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> 
> There is something wrong with this patch. Since it landed in linux-next 
> (20220204) I get an interrupt storm on two of my test devices:

I'll just revert it for now.

On 2/8/2022 5:59 PM, Marek Szyprowski wrote:

> There is something wrong with this patch. Since it landed in linux-next
> (20220204) I get an interrupt storm on two of my test devices:
>
> 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO
> error" message)
>
> 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped
> sample" message)
>
> Definitely the interrupts are not acknowledged properly. Once I find
> some spare time, I will check it further which regmap configuration
> triggers the issue, but it is definitely related to this patch.
> Reverting it on top of current linux-next fixes the issue.

Change is needed to handle the interrupt ack properly to clear the ack.

I observed that the regmap_irq_update_bits() writes the register only if 
it finds a difference b/w existing reg value to update reg value.

This may be causing the interrupt storm issue mentioned above.

Setting the mask_writeonly flag to 1, to force write the register may 
resolve the interrupt storm issue.

--Prasad