[PATCH v4 0/2] rtc-pm8xxx: fix rtc alarm which fired before driver probe not be cleard

[PATCH v4 0/2] rtc-pm8xxx: fix rtc alarm which fired before driver probe not be cleard

[jianbin zhang]

Hi maintainers,

In this v4 patch series include following:
- Fix the issue that rtc alarm interrupt triggered befor driver probe is not cleared
- Correct the value written into PM8xxx_RTC_ALARM_CLEAR register in trigger function

Regarding the issue "Fix the issue that rtc alarm interrupt triggered befor driver probe is not cleared":
- Issue describtion as following
  If the alarm is triggered before the driver gets probed, the alarm interrupt
  will be missed and it won't be detected, and the stale alarm settings will
  be still retained because of not being cleared.
- Issue reproduce step:
  (1) set the alarm and poweroff the device
  (2) alarm happens and the device boots
  (3) poweroff the device again
  (4) alarm irq not be cleard, device boots again

Regarding "Correct the value written into PM8xxx_RTC_ALARM_CLEAR"
- Writing 1 to the register is expected to clear the fired alarm
 register. In patch v2, the value written to the register in the
 trigger function is incorrectly written as 0. So correct the value
 to 1.

Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
---
jianbin zhang (2):
      rtc-pm8xxx: clear the triggered alarm interrupt during driver probe
      rtc-pm8xxx: Correct the value written into the PM8xxx_RTC_ALARM_CLEAR

 drivers/rtc/rtc-pm8xxx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
---
base-commit: 317c7bc0ef035d8ebfc3e55c5dde0566fd5fb171
change-id: 20240329-fix-rtc-alarm-which-fired-before-driver-probe-not-be-cleard-07c2180e5507

Best regards,
-- 
jianbin zhang <quic_jianbinz@quicinc.com>

[PATCH V4 1/2] rtc-pm8xxx: clear the triggered alarm interrupt during driver probe

[jianbin zhang]

If the alarm is triggered before the driver gets probed, the alarm interrupt
will be missed and it won't be detected, and the stale alarm settings will
be still retained because of not being cleared.

Issue reproduce step:
(1) set the alarm and poweroff the device
(2) alarm happens and the device boots
(3) poweroff the device again
(4) alarm irq not be cleard, device boots again

the fixing here is clear the interrupt during the step(3) unconditionally.

Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
---
Changes in v4:
- add the cover letter
- modify the patch to conform to the specification

Changes in v3:
- clear the interrupt in driver probe unconditionally
- link: https://lore.kernel.org/linux-rtc/20240319191037.GA3796206@hu-bjorande-lv.qualcomm.com/T/#t

Changes in v2:
- Adapt the V1 patch according to the newest rtc-pm8xxx
- link: https://lore.kernel.org/linux-rtc/20240124024023df15ef6e@mail.local/

Changes in v1:
- fixing is as below logic, During driver probe: read ALARM_EN, read ALARM_DATA, read RTC_RDATA,
  if (ALARM_DATA < RTC_DATA), Trigger the alarm event and clear the alarm settins
- link: https://lore.kernel.org/linux-rtc/20220321090514.4523-1-quic_jianbinz@quicinc.com/

Changes in original:
- link to original: https://lore.kernel.org/linux-rtc/YTusgJlMUdXOKQaL@piout.net/
---
 drivers/rtc/rtc-pm8xxx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index f6b779c12ca7..e4e2307445cf 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -527,6 +527,11 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
+	rc = regmap_update_bits(rtc_dd->regmap, rtc_dd->regs->alarm_ctrl2,
+					  PM8xxx_RTC_ALARM_CLEAR, 1);
+	if (rc)
+		return rc;
+
 	return 0;
 }
 

-- 
2.43.2

[PATCH V4 2/2] rtc-pm8xxx: Correct the value written into the PM8xxx_RTC_ALARM_CLEAR

[jianbin zhang]

Writing 1 to the PM8xxx_RTC_ALARM_CLEAR register is expected to clear
the triggered alarm status. In patch v2, the value written to the
PM8xxx_RTC_ALARM_CLEAR register in the trigger function is incorrectly
written as 0. So correct the value written to PM8xxx_RTC_ALARM_CLEAR
register into 1.

Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
---
Changess in v4:
- add the cover letter
- modify the patch to conform to the specification

Changes in v3:
- Correct the value written into the PM8xxx_RTC_ALARM_CLEAR to 1.
- link: https://lore.kernel.org/linux-rtc/20240319191216.GB3796206@hu-bjorande-lv.qualcomm.com/T/#t

Changes in v2:
- Switch to using regmap_update_bits() instead of open coding
  read-modify-write accesses.
- link: https://lore.kernel.org/lkml/20230202155448.6715-4-johan+linaro@kernel.org/

Changes in v1:
-link: https://lore.kernel.org/linux-rtc/20230126142057.25715-4-johan+linaro@kernel.org/
---
 drivers/rtc/rtc-pm8xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index e4e2307445cf..806c99cdac9a 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -391,7 +391,7 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 
 	/* Clear alarm status */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
-				PM8xxx_RTC_ALARM_CLEAR, 0);
+				PM8xxx_RTC_ALARM_CLEAR, 1);
 	if (rc)
 		return IRQ_NONE;
 

-- 
2.43.2

Re: [PATCH V4 1/2] rtc-pm8xxx: clear the triggered alarm interrupt during driver probe

[Bjorn Andersson]

On Mon, Apr 01, 2024 at 09:56:29AM +0800, jianbin zhang wrote:

Sorry, I think the patch looks good now, but the subject prefix
(rtc-pm8xxx) does not match other changes to this file.

> If the alarm is triggered before the driver gets probed, the alarm interrupt
> will be missed and it won't be detected, and the stale alarm settings will
> be still retained because of not being cleared.
> 
> Issue reproduce step:
> (1) set the alarm and poweroff the device
> (2) alarm happens and the device boots
> (3) poweroff the device again
> (4) alarm irq not be cleard, device boots again
> 
> the fixing here is clear the interrupt during the step(3) unconditionally.
> 
> Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
> ---
> Changes in v4:
> - add the cover letter
> - modify the patch to conform to the specification
> 
> Changes in v3:
> - clear the interrupt in driver probe unconditionally
> - link: https://lore.kernel.org/linux-rtc/20240319191037.GA3796206@hu-bjorande-lv.qualcomm.com/T/#t

These are expected to be links to the previous revisions of your patch,
not people's answers.

Please consult go/upstream and switch to b4 for the future, which does
this automatically for you. Please also use the internal review list!

Regards,
Bjorn

> 
> Changes in v2:
> - Adapt the V1 patch according to the newest rtc-pm8xxx
> - link: https://lore.kernel.org/linux-rtc/20240124024023df15ef6e@mail.local/
> 
> Changes in v1:
> - fixing is as below logic, During driver probe: read ALARM_EN, read ALARM_DATA, read RTC_RDATA,
>   if (ALARM_DATA < RTC_DATA), Trigger the alarm event and clear the alarm settins
> - link: https://lore.kernel.org/linux-rtc/20220321090514.4523-1-quic_jianbinz@quicinc.com/
> 
> Changes in original:
> - link to original: https://lore.kernel.org/linux-rtc/YTusgJlMUdXOKQaL@piout.net/
> ---
>  drivers/rtc/rtc-pm8xxx.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index f6b779c12ca7..e4e2307445cf 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -527,6 +527,11 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> +	rc = regmap_update_bits(rtc_dd->regmap, rtc_dd->regs->alarm_ctrl2,
> +					  PM8xxx_RTC_ALARM_CLEAR, 1);
> +	if (rc)
> +		return rc;
> +
>  	return 0;
>  }
>  
> 
> -- 
> 2.43.2
> 

Re: [PATCH V4 2/2] rtc-pm8xxx: Correct the value written into the PM8xxx_RTC_ALARM_CLEAR

[Bjorn Andersson]

On Mon, Apr 01, 2024 at 09:56:30AM +0800, jianbin zhang wrote:
> Writing 1 to the PM8xxx_RTC_ALARM_CLEAR register is expected to clear
> the triggered alarm status. In patch v2, the value written to the

I'm not sure what "in patch v2" refers to here.

Also, as with patch 1, please fix your subject line and use internal
review list.

Regards,
Bjorn

> PM8xxx_RTC_ALARM_CLEAR register in the trigger function is incorrectly
> written as 0. So correct the value written to PM8xxx_RTC_ALARM_CLEAR
> register into 1.
> 
> Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
> ---
> Changess in v4:
> - add the cover letter
> - modify the patch to conform to the specification
> 
> Changes in v3:
> - Correct the value written into the PM8xxx_RTC_ALARM_CLEAR to 1.
> - link: https://lore.kernel.org/linux-rtc/20240319191216.GB3796206@hu-bjorande-lv.qualcomm.com/T/#t
> 
> Changes in v2:
> - Switch to using regmap_update_bits() instead of open coding
>   read-modify-write accesses.
> - link: https://lore.kernel.org/lkml/20230202155448.6715-4-johan+linaro@kernel.org/
> 
> Changes in v1:
> -link: https://lore.kernel.org/linux-rtc/20230126142057.25715-4-johan+linaro@kernel.org/
> ---
>  drivers/rtc/rtc-pm8xxx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index e4e2307445cf..806c99cdac9a 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -391,7 +391,7 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
>  
>  	/* Clear alarm status */
>  	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
> -				PM8xxx_RTC_ALARM_CLEAR, 0);
> +				PM8xxx_RTC_ALARM_CLEAR, 1);
>  	if (rc)
>  		return IRQ_NONE;
>  
> 
> -- 
> 2.43.2
> 

Re: [PATCH V4 1/2] rtc-pm8xxx: clear the triggered alarm interrupt during driver probe

[jianbin zhang]

On 4/5/24 10:45, Bjorn Andersson wrote:
> On Mon, Apr 01, 2024 at 09:56:29AM +0800, jianbin zhang wrote:
>
> Sorry, I think the patch looks good now, but the subject prefix
> (rtc-pm8xxx) does not match other changes to this file.
>
Sure, thanks for reviewing, I will update the subject prefix to "rtc: 
pm8xxx:" as other changes to this file.
>> If the alarm is triggered before the driver gets probed, the alarm interrupt
>> will be missed and it won't be detected, and the stale alarm settings will
>> be still retained because of not being cleared.
>>
>> Issue reproduce step:
>> (1) set the alarm and poweroff the device
>> (2) alarm happens and the device boots
>> (3) poweroff the device again
>> (4) alarm irq not be cleard, device boots again
>>
>> the fixing here is clear the interrupt during the step(3) unconditionally.
>>
>> Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
>> ---
>> Changes in v4:
>> - add the cover letter
>> - modify the patch to conform to the specification
>>
>> Changes in v3:
>> - clear the interrupt in driver probe unconditionally
>> - link: https://lore.kernel.org/linux-rtc/20240319191037.GA3796206@hu-bjorande-lv.qualcomm.com/T/#t
> These are expected to be links to the previous revisions of your patch,
> not people's answers.
>
> Please consult go/upstream and switch to b4 for the future, which does
> this automatically for you. Please also use the internal review list!
>
> Regards,
> Bjorn
Yeah, will use the internal review list in next patch version and will 
switch to b4 in the future.
>> Changes in v2:
>> - Adapt the V1 patch according to the newest rtc-pm8xxx
>> - link: https://lore.kernel.org/linux-rtc/20240124024023df15ef6e@mail.local/
>>
>> Changes in v1:
>> - fixing is as below logic, During driver probe: read ALARM_EN, read ALARM_DATA, read RTC_RDATA,
>>    if (ALARM_DATA < RTC_DATA), Trigger the alarm event and clear the alarm settins
>> - link: https://lore.kernel.org/linux-rtc/20220321090514.4523-1-quic_jianbinz@quicinc.com/
>>
>> Changes in original:
>> - link to original: https://lore.kernel.org/linux-rtc/YTusgJlMUdXOKQaL@piout.net/
>> ---
>>   drivers/rtc/rtc-pm8xxx.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
>> index f6b779c12ca7..e4e2307445cf 100644
>> --- a/drivers/rtc/rtc-pm8xxx.c
>> +++ b/drivers/rtc/rtc-pm8xxx.c
>> @@ -527,6 +527,11 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
>>   	if (rc)
>>   		return rc;
>>   
>> +	rc = regmap_update_bits(rtc_dd->regmap, rtc_dd->regs->alarm_ctrl2,
>> +					  PM8xxx_RTC_ALARM_CLEAR, 1);
>> +	if (rc)
>> +		return rc;
>> +
>>   	return 0;
>>   }
>>   
>>
>> -- 
>> 2.43.2
>>

Re: [PATCH V4 2/2] rtc-pm8xxx: Correct the value written into the PM8xxx_RTC_ALARM_CLEAR

[jianbin zhang]

On 4/5/24 10:47, Bjorn Andersson wrote:
> On Mon, Apr 01, 2024 at 09:56:30AM +0800, jianbin zhang wrote:
>> Writing 1 to the PM8xxx_RTC_ALARM_CLEAR register is expected to clear
>> the triggered alarm status. In patch v2, the value written to the
> I'm not sure what "in patch v2" refers to here.

Thanks for reviewing, "In patch v2" refers to https://lore.kernel.org/lkml/20230202155448.6715-4-johan+linaro@kernel.org/ which siwtch to use regmap_update_bits.

> Also, as with patch 1, please fix your subject line and use internal
> review list.
>
> Regards,
> Bjorn
Sure, Will fix the subject line and use internal review list in next 
patch version.
>> PM8xxx_RTC_ALARM_CLEAR register in the trigger function is incorrectly
>> written as 0. So correct the value written to PM8xxx_RTC_ALARM_CLEAR
>> register into 1.
>>
>> Signed-off-by: jianbin zhang <quic_jianbinz@quicinc.com>
>> ---
>> Changess in v4:
>> - add the cover letter
>> - modify the patch to conform to the specification
>>
>> Changes in v3:
>> - Correct the value written into the PM8xxx_RTC_ALARM_CLEAR to 1.
>> - link: https://lore.kernel.org/linux-rtc/20240319191216.GB3796206@hu-bjorande-lv.qualcomm.com/T/#t
>>
>> Changes in v2:
>> - Switch to using regmap_update_bits() instead of open coding
>>    read-modify-write accesses.
>> - link: https://lore.kernel.org/lkml/20230202155448.6715-4-johan+linaro@kernel.org/
>>
>> Changes in v1:
>> -link: https://lore.kernel.org/linux-rtc/20230126142057.25715-4-johan+linaro@kernel.org/
>> ---
>>   drivers/rtc/rtc-pm8xxx.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
>> index e4e2307445cf..806c99cdac9a 100644
>> --- a/drivers/rtc/rtc-pm8xxx.c
>> +++ b/drivers/rtc/rtc-pm8xxx.c
>> @@ -391,7 +391,7 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
>>   
>>   	/* Clear alarm status */
>>   	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
>> -				PM8xxx_RTC_ALARM_CLEAR, 0);
>> +				PM8xxx_RTC_ALARM_CLEAR, 1);
>>   	if (rc)
>>   		return IRQ_NONE;
>>   
>>
>> -- 
>> 2.43.2
>>