Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 17 Feb 2022 13:17:33 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Prasad Kumpatla <quic_pkumpatl@quicinc.com>
CC:     Mark Brown <broonie@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v3] regmap-irq: Update interrupt clear register for
 proper reset
Message-ID: <20220217131733.GH38351@ediswmail.ad.cirrus.com>
References: <20220217085007.30218-1-quic_pkumpatl@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220217085007.30218-1-quic_pkumpatl@quicinc.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Thu, Feb 17, 2022 at 02:20:07PM +0530, Prasad Kumpatla wrote:

+ Marek Szyprowski, as the old version of the patch was causing
problems on his system.

> With the existing logic where clear_ack is true (HW doesn’t support
> auto clear for ICR), interrupt clear register reset is not handled
> properly. Due to this only the first interrupts get processed properly
> and further interrupts are blocked due to not resetting interrupt
> clear register.
> 
> Example for issue case 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.
> 
>     Step 1: 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. So expectation here is to
>             reset ICR, which will unblock all the interrupts.
> 
>             if (chip->clear_ack) {
>                  if (chip->ack_invert && !ret)
>                   ........
>                  else if (!ret)
>                      ret = regmap_write(map, reg,
>                             ~data->status_buf[i]);
> 
> So writing 0 and 0xff (when ack_invert is true) should have no effect, other
> than clearing the ACKs just set.
> 
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> ---

I think this version looks like it should work to me.

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

>  drivers/base/regmap/regmap-irq.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index d2656581a608..4a446259a184 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -189,11 +189,9 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>  				ret = regmap_write(map, reg, 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_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -							   ~d->mask_buf[i]);
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0)
>  				dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
> @@ -556,11 +554,9 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  						data->status_buf[i]);
>  			if (chip->clear_ack) {
>  				if (chip->ack_invert && !ret)
> -					ret = regmap_write(map, reg,
> -							data->status_buf[i]);
> +					ret = regmap_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -							~data->status_buf[i]);
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0)
>  				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
> @@ -817,13 +813,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>  					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_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -						~(d->status_buf[i] &
> -						  d->mask_buf[i]));
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0) {
>  				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
> -- 
> 2.17.1
>