Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Feb 2022 10:54:05 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Prasad Kumpatla <quic_pkumpatl@quicinc.com>
CC:     Mark Brown <broonie@kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        'Linux Samsung SOC' <linux-samsung-soc@vger.kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Subject: Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of
 regmap_write
Message-ID: <20220215105405.GD38351@ediswmail.ad.cirrus.com>
References: <20220119142953.1804-1-quic_pkumpatl@quicinc.com>
 <CGME20220208122955eucas1p2d4e32f51224242e9ebd0bce58b9c04ca@eucas1p2.samsung.com>
 <39f1b598-58ca-1e3d-3065-8dd692ee7c9f@samsung.com>
 <20220208133957.GC112838@ediswmail.ad.cirrus.com>
 <20220208135036.GD112838@ediswmail.ad.cirrus.com>
 <YgJ2hG2vwUclA/zF@sirena.org.uk>
 <20220208143316.GE112838@ediswmail.ad.cirrus.com>
 <YgKBEIapL8MAQAoI@sirena.org.uk>
 <22d9bf04-e069-c6a3-4d59-974a9402b0f7@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <22d9bf04-e069-c6a3-4d59-974a9402b0f7@quicinc.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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