| Message ID | 1584342688-14035-1-git-send-email-maggarwa@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rtc-pm8xxx: Clear Alarm register on resume | expand |
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. > Signed-off-by: Mohit Aggarwal <maggarwa@codeaurora.org> > --- > 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. > + 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? > + goto rtc_rw_fail; > + } > + } > +
Hi, Comments inline. Thanks & Regards, Mohit -----Original Message----- From: Alexandre Belloni <alexandre.belloni@bootlin.com> Sent: Monday, March 16, 2020 3:59 PM To: Mohit Aggarwal <maggarwa@codeaurora.org> Cc: a.zummo@towertech.it; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org 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@codeaurora.org> > --- > 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
On 17/03/2020 10:20:36+0530, maggarwa@codeaurora.org wrote: > Hi, > > Comments inline. > > > Thanks & Regards, > Mohit > > -----Original Message----- > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > Sent: Monday, March 16, 2020 3:59 PM > To: Mohit Aggarwal <maggarwa@codeaurora.org> > Cc: a.zummo@towertech.it; linux-rtc@vger.kernel.org; > linux-kernel@vger.kernel.org > 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@codeaurora.org> > > --- > > 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. > The alignment should match the opening parenthesis. > > + 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. > Who in the field on the final product will see this error message? For debugging purposes, regmap already provides plenty of tracing facilities.
Comments inline. -----Original Message----- From: Alexandre Belloni <alexandre.belloni@bootlin.com> Sent: Tuesday, March 17, 2020 5:25 PM To: maggarwa@codeaurora.org Cc: a.zummo@towertech.it; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] rtc-pm8xxx: Clear Alarm register on resume On 17/03/2020 10:20:36+0530, maggarwa@codeaurora.org wrote: > Hi, > > Comments inline. > > > Thanks & Regards, > Mohit > > -----Original Message----- > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > Sent: Monday, March 16, 2020 3:59 PM > To: Mohit Aggarwal <maggarwa@codeaurora.org> > Cc: a.zummo@towertech.it; linux-rtc@vger.kernel.org; > linux-kernel@vger.kernel.org > 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@codeaurora.org> > > --- > > 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. > The alignment should match the opening parenthesis. [Mohit]: Thanks for clarification. Uploaded new patch. > > + 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. > Who in the field on the final product will see this error message? For debugging purposes, regmap already provides plenty of tracing facilities. [Mohit]: This is mostly for debugging issues reported during internal tests using kernel logs. Also added the debug error log to remain in-line with other error logs in file. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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)); + if (rc) { + dev_err(dev, "Write to RTC ALARM register failed\n"); + goto rtc_rw_fail; + } + } + rtc_rw_fail: spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags); return rc;
Currently, alarm register is not cleared on resume leading to reboot during power off charging mode. Change-Id: Ie2e6bbab8aa46e4e9b9cc984181ffab557cbbdae Signed-off-by: Mohit Aggarwal <maggarwa@codeaurora.org> --- drivers/rtc/rtc-pm8xxx.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)