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

Biju Das
2024-07-30T12:34:40Z

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