Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Tue, Feb 15, 2022 at 03:06:24PM +0530, Prasad Kumpatla wrote:
> On 2/8/2022 8:11 PM, Mark Brown wrote:
> >On Tue, Feb 08, 2022 at 02:33:16PM +0000, Charles Keepax wrote:
> >>On Tue, Feb 08, 2022 at 01:56:20PM +0000, Mark Brown wrote:
> ISR = Interrupt status register
> ICR = Interrupt clear register
> 
> When the Invert_ack is
>  *
>    *FALSE*: For ICR,writing 1 will clear , writing 0 has no effect.
>  *
>    *TRUE* : For ICR, writing 0 will clear , writing 1 has no effect
> and clear_ack
> 
>  *
>    *TRUE* :  Some hardware doesn’t support auto clear for ICR,  Need
>    explicitly clear the ICR
>  *
>    *FALSE *:  No need to clear ICR, as HW will reset.
> 
> Consider the following scenario,  where Invert_ack is false and
> clear_ack is true.
> 
> Say Default ISR=0x00 & ICR=0x00 and ISR is triggered with 2
> interrupts making ISR = 0x11.
> 
> *Step1:* Say ISR is set 0x11 (store status_buff = ISR). ISR needs to
> be cleared with the help of ICR once the Interrupt is processed.
> 
> *Step 2:* Write ICR = 0x11 (status_buff), this will clear the ISR to 0x00.
> 
> *Step 3:* Issue - In the existing code, ICR is written with ICR =
> ~(status_buff) i.e ICR = 0xEE à This will block all the interrupts
> from raising except for interrupts 0 and 4.
> 

Ok yes I the issue here now. I hadn't quite realised there was
hardware where you actually had to manually clear the ACK bits.

> Code snippet for step 3
> *if*(chip->clear_ack) {
> *if*(chip->ack_invert && !ret)
> ret = regmap_write(map, reg,
> data->status_buf[i]);
> *else**if*(!ret)
> ret = regmap_write(map, reg,
> ~data->status_buf[i]);
> 
> Solution: Since ICR does not clear automatically, writing 0x00 into
> ICR explicitly will clear the ICR (ICR = 0x00) allowing all the
> interrupts to raise.
> 
> So I’m thinking to implement as below.
> if (chip->clear_ack) {
> if (chip->ack_invert && !ret)
> - ret = regmap_write(map, reg,
> - data->status_buf[i]);
> + ret = regmap_write(map, reg, 0xff);
> else if (!ret)
> - ret = regmap_write(map, reg,
> - ~data->status_buf[i]);
> + ret = regmap_write(map, reg, 0x00);
> }

I think this looks much safer, the values you are writing should
have no effect, other than clearing the ACKs you just set.

Thanks,
Charles