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;
Message-ID: <585d09d8-b2ba-97e3-992e-404364fa6bb5@samsung.com>
Date:   Thu, 17 Feb 2022 15:09:49 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.6.0
Subject: Re: [PATCH v3] regmap-irq: Update interrupt clear register for
 proper reset
Content-Language: en-US
To:     Charles Keepax <ckeepax@opensource.cirrus.com>,
        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>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20220217131733.GH38351@ediswmail.ad.cirrus.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20220217085007.30218-1-quic_pkumpatl@quicinc.com>
        <CGME20220217131745eucas1p16489e2c6a7c9e11dccd2a9c5dc322c6d@eucas1p1.samsung.com>
        <20220217131733.GH38351@ediswmail.ad.cirrus.com>
Precedence: bulk

On 17.02.2022 14:17, Charles Keepax wrote:
> 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.

Both devices affected by the previous version of the patch work fine 
with this version.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> 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
>>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland