RE: [PATCH] rtc-pm8xxx: Clear Alarm register on resume

[maggarwa]

Hi,

Comments inline.


Thanks & Regards,
Mohit

-----Original Message-----
From: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> 
Sent: Monday, March 16, 2020 3:59 PM
To: Mohit Aggarwal <maggarwa@xxxxxxxxxxxxxx>
Cc: a.zummo@xxxxxxxxxxxx; linux-rtc@xxxxxxxxxxxxxxx;
linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] rtc-pm8xxx: Clear Alarm register on resume

Hi,

On 16/03/2020 12:41:28+0530, Mohit Aggarwal wrote:
> Currently, alarm register is not cleared on resume leading to reboot 
> during power off charging mode.
> 
> Change-Id: Ie2e6bbab8aa46e4e9b9cc984181ffab557cbbdae

No Change-Id upstream please.
[Mohit]: Will fix in next patch.

> Signed-off-by: Mohit Aggarwal <maggarwa@xxxxxxxxxxxxxx>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c index 
> bbe013f..96e7985 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2010-2011, 2019, The Linux Foundation. All rights 
> reserved. */
> +/* Copyright (c) 2010-2011, 2019-2020, The Linux Foundation. All 
> +rights reserved. */
>  
>  #include <linux/of.h>
>  #include <linux/module.h>
> @@ -301,6 +301,7 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device
*dev, unsigned int enable)
>  	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
>  	unsigned int ctrl_reg;
> +	u8 value[NUM_8_BIT_RTC_REGS] = {0};
>  
>  	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
>  
> @@ -319,6 +320,16 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device
*dev, unsigned int enable)
>  		goto rtc_rw_fail;
>  	}
>  
> +	/* Clear Alarm register */
> +	if (!enable) {
> +		rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw,
value,
> +					sizeof(value));

This is not properly aligned.
[Mohit]: I don't see any alignment issue at my end. I can see proper tabs
are present.

> +		if (rc) {
> +			dev_err(dev, "Write to RTC ALARM register
failed\n");

Is that error message necessary? What would be the user action after seeing
that in the logs? Will the logs actually be seen?
[Mohit]: In case issue in question reproduces even after this change then
for debugging purposes user can look out for this error log in kernel logs
which can help to triage the issue.

> +			goto rtc_rw_fail;
> +		}
> +	}
> +

--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com