[rtc-linux] [PATCH v2 0/5] rtc: s35390a: allow shutdown of NAS after alarm triggered

[rtc-linux] [PATCH v2 0/5] rtc: s35390a: allow shutdown of NAS after alarm triggered

[Uwe Kleine-König]

Hello,

this is v2 of the series fixing https://bugs.debian.org/794266.

The only change compared to (implicit) v1 is that reading out a disabled
alarm is fixed. When the alarm is disabled the respective register is
shadowed with the result is that the reported alarm time is 45:85:0
because both hour and minute byte are read as 0xff and bcd2bin(0x3f) and
bcd2bin(0x7f) are 45 and 85 respectively.

The new behaviour is to only set .enabled =3D 0 and return. Not 100% sure
this is the right thing, though. The only alternative is to enable alarm
and read out a time, but that might have the side effect to trigger an
unwanted alarm, so I didn't chose this way.

Best regards
Uwe

Uwe Kleine-K=C3=B6nig (5):
  rtc: s35390a: fix reading out alarm
  rtc: s35390a: implement reset routine as suggested by the reference
  rtc: s35390a: improve irq handling
  rtc: s35390a: improve two comments in .set_alarm
  rtc: fix a typo and reduce three empty lines to one

 drivers/rtc/interface.c   |   4 +-
 drivers/rtc/rtc-s35390a.c | 157 +++++++++++++++++++++++++++++++++++-------=
----
 2 files changed, 120 insertions(+), 41 deletions(-)

--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH v2 1/5] rtc: s35390a: fix reading out alarm

[Uwe Kleine-König]

There are several issues fixed in this patch:

 - When alarm isn't enabled, set .enabled to zero instead of returning
   -EINVAL.
 - Ignore how IRQ1 is configured when determining if IRQ2 is on.
 - The three alarm registers have an enable flag which must be
   evaluated.
 - The chip always triggers when the seconds register gets 0.

Note that the rtc framework however doesn't handle the result correctly
because it doesn't check wday being initialized and so interprets an
alarm being set for 10:00 AM in three days as 10:00 AM tomorrow (or
today if that's not over yet).

Signed-off-by: Uwe Kleine-K=C3=B6nig <uwe@kleine-koenig.org>
---
 drivers/rtc/rtc-s35390a.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index f40afdd0e5f5..6507a01cf9ad 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -242,6 +242,8 @@ static int s35390a_set_alarm(struct i2c_client *client,=
 struct rtc_wkalrm *alm)
=20
 	if (alm->time.tm_wday !=3D -1)
 		buf[S35390A_ALRM_BYTE_WDAY] =3D bin2bcd(alm->time.tm_wday) | 0x80;
+	else
+		buf[S35390A_ALRM_BYTE_WDAY] =3D 0;
=20
 	buf[S35390A_ALRM_BYTE_HOURS] =3D s35390a_hr2reg(s35390a,
 			alm->time.tm_hour) | 0x80;
@@ -269,23 +271,43 @@ static int s35390a_read_alarm(struct i2c_client *clie=
nt, struct rtc_wkalrm *alm)
 	if (err < 0)
 		return err;
=20
-	if (bitrev8(sts) !=3D S35390A_INT2_MODE_ALARM)
-		return -EINVAL;
+	if ((bitrev8(sts) & S35390A_INT2_MODE_MASK) !=3D S35390A_INT2_MODE_ALARM)=
 {
+		/*
+		 * When the alarm isn't enabled, the register to configure
+		 * the alarm time isn't accessible.
+		 */
+		alm->enabled =3D 0;
+		return 0;
+	} else {
+		alm->enabled =3D 1;
+	}
=20
 	err =3D s35390a_get_reg(s35390a, S35390A_CMD_INT2_REG1, buf, sizeof(buf))=
;
 	if (err < 0)
 		return err;
=20
 	/* This chip returns the bits of each byte in reverse order */
-	for (i =3D 0; i < 3; ++i) {
+	for (i =3D 0; i < 3; ++i)
 		buf[i] =3D bitrev8(buf[i]);
-		buf[i] &=3D ~0x80;
-	}
=20
-	alm->time.tm_wday =3D bcd2bin(buf[S35390A_ALRM_BYTE_WDAY]);
-	alm->time.tm_hour =3D s35390a_reg2hr(s35390a,
-						buf[S35390A_ALRM_BYTE_HOURS]);
-	alm->time.tm_min =3D bcd2bin(buf[S35390A_ALRM_BYTE_MINS]);
+	/*
+	 * B0 of the three matching registers is an enable flag. Iff it is set
+	 * the configured value is used for matching.
+	 */
+	if (buf[S35390A_ALRM_BYTE_WDAY] & 0x80)
+		alm->time.tm_wday =3D
+			bcd2bin(buf[S35390A_ALRM_BYTE_WDAY] & ~0x80);
+
+	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
+		alm->time.tm_hour =3D
+			s35390a_reg2hr(s35390a,
+				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
+
+	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
+		alm->time.tm_min =3D bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
+
+	/* alarm triggers always at s=3D0 */
+	alm->time.tm_sec =3D 0;
=20
 	dev_dbg(&client->dev, "%s: alm is mins=3D%d, hours=3D%d, wday=3D%d\n",
 			__func__, alm->time.tm_min, alm->time.tm_hour,
--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH v2 2/5] rtc: s35390a: implement reset routine as suggested by the reference

[Uwe Kleine-König]

There were two deviations from the reference manual: you have to wait
half a second when POC is active and you might have to repeat
initialization when POC or BLD are still set after the sequence.

Note however that as POC and BLD are cleared by read the driver might
not be able to detect that a reset is necessary. I don't have a good
idea how to fix this.

Additionally report the value read from STATUS1 to the caller. This
prepares the next patch.

Signed-off-by: Uwe Kleine-K=C3=B6nig <uwe@kleine-koenig.org>
---
 drivers/rtc/rtc-s35390a.c | 65 +++++++++++++++++++++++++++++++++++++++----=
----
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 6507a01cf9ad..6c90c9f48cab 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -15,6 +15,7 @@
 #include <linux/bitrev.h>
 #include <linux/bcd.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
=20
 #define S35390A_CMD_STATUS1	0
 #define S35390A_CMD_STATUS2	1
@@ -94,19 +95,63 @@ static int s35390a_get_reg(struct s35390a *s35390a, int=
 reg, char *buf, int len)
 	return 0;
 }
=20
-static int s35390a_reset(struct s35390a *s35390a)
+/*
+ * Returns <0 on error, 0 if rtc is setup fine and 1 if the chip was reset=
.
+ * To keep the information if an irq is pending, pass the value read from
+ * STATUS1 to the caller.
+ */
+static int s35390a_reset(struct s35390a *s35390a, char *status1)
 {
-	char buf[1];
+	char buf;
+	int ret;
+	unsigned initcount =3D 0;
=20
-	if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf)) < 0)
-		return -EIO;
+	ret =3D s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, status1, 1);
+	if (ret < 0)
+		return ret;
=20
-	if (!(buf[0] & (S35390A_FLAG_POC | S35390A_FLAG_BLD)))
+	if (*status1 & S35390A_FLAG_POC)
+		/*
+		 * Do not communicate for 0.5 seconds since the power-on
+		 * detection circuit is in operation.
+		 */
+		msleep(500);
+	else if (!(*status1 & S35390A_FLAG_BLD))
+		/*
+		 * If both POC and BLD are unset everything is fine.
+		 */
 		return 0;
=20
-	buf[0] |=3D (S35390A_FLAG_RESET | S35390A_FLAG_24H);
-	buf[0] &=3D 0xf0;
-	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
+	/*
+	 * At least one of POC and BLD are set, so reinitialise chip. Keeping
+	 * this information in the hardware to know later that the time isn't
+	 * valid is unfortunately not possible because POC and BLD are cleared
+	 * on read. So the reset is best done now.
+	 *
+	 * The 24H bit is kept over reset, so set it already here.
+	 */
+initialize:
+	*status1 =3D S35390A_FLAG_24H;
+	buf =3D S35390A_FLAG_RESET | S35390A_FLAG_24H;
+	ret =3D s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+
+	if (ret < 0)
+		return ret;
+
+	ret =3D s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+	if (ret < 0)
+		return ret;
+
+	if (buf & (S35390A_FLAG_POC | S35390A_FLAG_BLD)) {
+		/* Try up to five times to reset the chip */
+		if (initcount < 5) {
+			++initcount;
+			goto initialize;
+		} else
+			return -EIO;
+	}
+
+	return 1;
 }
=20
 static int s35390a_disable_test_mode(struct s35390a *s35390a)
@@ -353,7 +398,7 @@ static int s35390a_probe(struct i2c_client *client,
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_time tm;
-	char buf[1];
+	char buf[1], status1;
=20
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		err =3D -ENODEV;
@@ -382,7 +427,7 @@ static int s35390a_probe(struct i2c_client *client,
 		}
 	}
=20
-	err =3D s35390a_reset(s35390a);
+	err =3D s35390a_reset(s35390a, &status1);
 	if (err < 0) {
 		dev_err(&client->dev, "error resetting chip\n");
 		goto exit_dummy;
--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH v2 3/5] rtc: s35390a: improve irq handling

[Uwe Kleine-König]

On some QNAP NAS devices the rtc can wake the machine. Several people
noticed that once the machine was woken this way it fails to shut down.
That's because the driver fails to acknowledge the interrupt and so it
keeps active and restarts the machine immediatly after shutdown. See
https://bugs.debian.org/794266 for a bug report.

Doing this correctly requires to interpret the INT2 flag of the first read
of the STATUS1 register because this bit is cleared by read.

Note this is not maximally robust though because a pending irq isn't
detected when the STATUS1 register was already read (and so INT2 is not
set) but the irq was not disabled. But that is a hardware imposed problem
that cannot easily be fixed by software.

Signed-off-by: Uwe Kleine-K=C3=B6nig <uwe@kleine-koenig.org>
---
 drivers/rtc/rtc-s35390a.c | 48 ++++++++++++++++++++++++++++++-------------=
----
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 6c90c9f48cab..d5b572266a72 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -35,10 +35,14 @@
 #define S35390A_ALRM_BYTE_HOURS	1
 #define S35390A_ALRM_BYTE_MINS	2
=20
+/* flags for STATUS1 */
 #define S35390A_FLAG_POC	0x01
 #define S35390A_FLAG_BLD	0x02
+#define S35390A_FLAG_INT2	0x04
 #define S35390A_FLAG_24H	0x40
 #define S35390A_FLAG_RESET	0x80
+
+/* flag for STATUS2 */
 #define S35390A_FLAG_TEST	0x01
=20
 #define S35390A_INT2_MODE_MASK		0xF0
@@ -394,11 +398,11 @@ static struct i2c_driver s35390a_driver;
 static int s35390a_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
-	int err;
+	int err, err_reset;
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_time tm;
-	char buf[1], status1;
+	char buf, status1;
=20
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		err =3D -ENODEV;
@@ -427,29 +431,35 @@ static int s35390a_probe(struct i2c_client *client,
 		}
 	}
=20
-	err =3D s35390a_reset(s35390a, &status1);
-	if (err < 0) {
+	err_reset =3D s35390a_reset(s35390a, &status1);
+	if (err_reset < 0) {
+		err =3D err_reset;
 		dev_err(&client->dev, "error resetting chip\n");
 		goto exit_dummy;
 	}
=20
-	err =3D s35390a_disable_test_mode(s35390a);
-	if (err < 0) {
-		dev_err(&client->dev, "error disabling test mode\n");
-		goto exit_dummy;
-	}
-
-	err =3D s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
-	if (err < 0) {
-		dev_err(&client->dev, "error checking 12/24 hour mode\n");
-		goto exit_dummy;
-	}
-	if (buf[0] & S35390A_FLAG_24H)
+	if (status1 & S35390A_FLAG_24H)
 		s35390a->twentyfourhour =3D 1;
 	else
 		s35390a->twentyfourhour =3D 0;
=20
-	if (s35390a_get_datetime(client, &tm) < 0)
+	if (status1 & S35390A_FLAG_INT2) {
+		/* disable alarm (and maybe test mode) */
+		buf =3D 0;
+		err =3D s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
+		if (err < 0) {
+			dev_err(&client->dev, "error disabling alarm");
+			goto exit_dummy;
+		}
+	} else {
+		err =3D s35390a_disable_test_mode(s35390a);
+		if (err < 0) {
+			dev_err(&client->dev, "error disabling test mode\n");
+			goto exit_dummy;
+		}
+	}
+
+	if (err_reset > 0 || s35390a_get_datetime(client, &tm) < 0)
 		dev_warn(&client->dev, "clock needs to be set\n");
=20
 	device_set_wakeup_capable(&client->dev, 1);
@@ -462,6 +472,10 @@ static int s35390a_probe(struct i2c_client *client,
 		err =3D PTR_ERR(s35390a->rtc);
 		goto exit_dummy;
 	}
+
+	if (status1 & S35390A_FLAG_INT2)
+		rtc_update_irq(s35390a->rtc, 1, RTC_AF);
+
 	return 0;
=20
 exit_dummy:
--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH v2 4/5] rtc: s35390a: improve two comments in .set_alarm

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K=C3=B6nig <uwe@kleine-koenig.org>
---
 drivers/rtc/rtc-s35390a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index d5b572266a72..5dab4665ca3b 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -266,12 +266,12 @@ static int s35390a_set_alarm(struct i2c_client *clien=
t, struct rtc_wkalrm *alm)
 		alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
 		alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
=20
-	/* disable interrupt */
+	/* disable interrupt (which deasserts the irq line) */
 	err =3D s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
=20
-	/* clear pending interrupt, if any */
+	/* clear pending interrupt (in STATUS1 only), if any */
 	err =3D s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH v2 5/5] rtc: fix a typo and reduce three empty lines to one

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K=C3=B6nig <uwe@kleine-koenig.org>
---
 drivers/rtc/interface.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 99475908e556..c803364ad4ce 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -393,7 +393,7 @@ int rtc_initialize_alarm(struct rtc_device *rtc, struct=
 rtc_wkalrm *alarm)
 	rtc->aie_timer.node.expires =3D rtc_tm_to_ktime(alarm->time);
 	rtc->aie_timer.period =3D ktime_set(0, 0);
=20
-	/* Alarm has to be enabled & in the futrure for us to enqueue it */
+	/* Alarm has to be enabled & in the future for us to enqueue it */
 	if (alarm->enabled && (rtc_tm_to_ktime(now).tv64 <
 			 rtc->aie_timer.node.expires.tv64)) {
=20
@@ -405,8 +405,6 @@ int rtc_initialize_alarm(struct rtc_device *rtc, struct=
 rtc_wkalrm *alarm)
 }
 EXPORT_SYMBOL_GPL(rtc_initialize_alarm);
=20
-
-
 int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
 {
 	int err =3D mutex_lock_interruptible(&rtc->ops_lock);
--=20
2.8.1

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH v2 0/5] rtc: s35390a: allow shutdown of NAS after alarm triggered

[Alexandre Belloni]

On 02/07/2016 at 17:28:07 +0200, Uwe Kleine-K=C3=B6nig wrote :
> Hello,
>=20
> this is v2 of the series fixing https://bugs.debian.org/794266.
>=20
> The only change compared to (implicit) v1 is that reading out a disabled
> alarm is fixed. When the alarm is disabled the respective register is
> shadowed with the result is that the reported alarm time is 45:85:0
> because both hour and minute byte are read as 0xff and bcd2bin(0x3f) and
> bcd2bin(0x7f) are 45 and 85 respectively.
>=20
> The new behaviour is to only set .enabled =3D 0 and return. Not 100% sure
> this is the right thing, though. The only alternative is to enable alarm
> and read out a time, but that might have the side effect to trigger an
> unwanted alarm, so I didn't chose this way.
>=20
> Best regards
> Uwe
>=20
> Uwe Kleine-K=C3=B6nig (5):
>   rtc: s35390a: fix reading out alarm
>   rtc: s35390a: implement reset routine as suggested by the reference
>   rtc: s35390a: improve irq handling
>   rtc: s35390a: improve two comments in .set_alarm
>   rtc: fix a typo and reduce three empty lines to one
>=20
>  drivers/rtc/interface.c   |   4 +-
>  drivers/rtc/rtc-s35390a.c | 157 +++++++++++++++++++++++++++++++++++-----=
------
>  2 files changed, 120 insertions(+), 41 deletions(-)
>=20

All applied, thanks!


I added a quick commit message in 4/5.

--=20
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.