diff mbox series

[5.10.y-cip,12/13] rtc: isl1208: Add a delay for clearing alarm

Message ID 20240729153504.510443-13-biju.das.jz@bp.renesas.com (mailing list archive)
State New
Headers show
Series RZ/G2L enhancements | expand

Commit Message

Biju Das July 29, 2024, 3:34 p.m. UTC 
commit 0dbd610c426ed695eef5d26584259d96b6250c76 upstream.

As per the latest HW manual[1], the INT# output is pulled low after the
alarm is triggered. After the INT# output is pulled low, it is low for at
least 250ms, even if the correct action is taken to clear it. It is
impossible to clear ALM if it is still active. The host must wait for the
RTC to progress past the alarm time plus the 250ms delay before clearing
ALM.

[1]https://www.renesas.com/us/en/document/dst/raa215300-datasheet?r=1506351

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Link: https://lore.kernel.org/r/20240618152635.48956-2-biju.das.jz@bp.renesas.com
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/rtc/rtc-isl1208.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Pavel Machek July 30, 2024, 12:30 p.m. UTC | #1 
Hi!

> commit 0dbd610c426ed695eef5d26584259d96b6250c76 upstream.
> 
> As per the latest HW manual[1], the INT# output is pulled low after the
> alarm is triggered. After the INT# output is pulled low, it is low for at
> least 250ms, even if the correct action is taken to clear it. It is
> impossible to clear ALM if it is still active. The host must wait for the
> RTC to progress past the alarm time plus the 250ms delay before clearing
> ALM.

> [1]https://www.renesas.com/us/en/document/dst/raa215300-datasheet?r=1506351

Ok, so this is kind of interesting. Yes, Renesas hw manual documents
250ms delay, but is that true for other chips? This seems to be
generic driver.

> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/bcd.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -628,6 +629,18 @@ isl1208_rtc_interrupt(int irq, void *data)
>  	struct isl1208_state *isl1208 = i2c_get_clientdata(client);
>  	int handled = 0, sr, err;
>  
> +	if (!isl1208->config->has_tamper) {
> +		/*
> +		 * The INT# output is pulled low 250ms after the alarm is
> +		 * triggered. After the INT# output is pulled low, it is low for
> +		 * at least 250ms, even if the correct action is taken to clear
> +		 * it. It is impossible to clear ALM if it is still active. The
> +		 * host must wait for the RTC to progress past the alarm time
> +		 * plus the 250ms delay before clearing ALM.
> +		 */
> +		msleep(250);
> +	}

Plus 250 msec in interrupt handler is strange. I guess this is i2c so
you can sleep, but... Plus I'd expect it to be conditional on we
having the alarm interrupt?

Best regards,
								Pavel
Biju Das July 30, 2024, 12:34 p.m. UTC | #2 
Hi Pavel Machek,

Thanks for the feedback.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: Tuesday, July 30, 2024 1:31 PM
> Subject: Re: [PATCH 5.10.y-cip 12/13] rtc: isl1208: Add a delay for clearing alarm
> 
> Hi!
> 
> > commit 0dbd610c426ed695eef5d26584259d96b6250c76 upstream.
> >
> > As per the latest HW manual[1], the INT# output is pulled low after
> > the alarm is triggered. After the INT# output is pulled low, it is low
> > for at least 250ms, even if the correct action is taken to clear it.
> > It is impossible to clear ALM if it is still active. The host must
> > wait for the RTC to progress past the alarm time plus the 250ms delay
> > before clearing ALM.
> 
> > [1]https://www.renesas.com/us/en/document/dst/raa215300-datasheet?r=15
> > 06351
> 
> Ok, so this is kind of interesting. Yes, Renesas hw manual documents 250ms delay, but is that true for
> other chips? This seems to be generic driver.

I believe so, that is the reason there is a work around in interrupt handler.

> 
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/bcd.h>
> >  #include <linux/clk.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > @@ -628,6 +629,18 @@ isl1208_rtc_interrupt(int irq, void *data)
> >  	struct isl1208_state *isl1208 = i2c_get_clientdata(client);
> >  	int handled = 0, sr, err;
> >
> > +	if (!isl1208->config->has_tamper) {
> > +		/*
> > +		 * The INT# output is pulled low 250ms after the alarm is
> > +		 * triggered. After the INT# output is pulled low, it is low for
> > +		 * at least 250ms, even if the correct action is taken to clear
> > +		 * it. It is impossible to clear ALM if it is still active. The
> > +		 * host must wait for the RTC to progress past the alarm time
> > +		 * plus the 250ms delay before clearing ALM.
> > +		 */
> > +		msleep(250);
> > +	}
> 
> Plus 250 msec in interrupt handler is strange. I guess this is i2c so you can sleep, but... Plus I'd
> expect it to be conditional on we having the alarm interrupt?

It should be fine as it is threaded irq.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 5b0a161f9552..c1824a76bc1c 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -7,6 +7,7 @@ 
 
 #include <linux/bcd.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -628,6 +629,18 @@  isl1208_rtc_interrupt(int irq, void *data)
 	struct isl1208_state *isl1208 = i2c_get_clientdata(client);
 	int handled = 0, sr, err;
 
+	if (!isl1208->config->has_tamper) {
+		/*
+		 * The INT# output is pulled low 250ms after the alarm is
+		 * triggered. After the INT# output is pulled low, it is low for
+		 * at least 250ms, even if the correct action is taken to clear
+		 * it. It is impossible to clear ALM if it is still active. The
+		 * host must wait for the RTC to progress past the alarm time
+		 * plus the 250ms delay before clearing ALM.
+		 */
+		msleep(250);
+	}
+
 	/*
 	 * I2C reads get NAK'ed if we read straight away after an interrupt?
 	 * Using a mdelay/msleep didn't seem to help either, so we work around