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
