[PATCH 0/3] rtc: pcf2127: add alarm support

[PATCH 0/3] rtc: pcf2127: add alarm support

[liambeguin]

From: Liam Beguin <lvb@xiphos.com>

The board used to test this series has the interrupt line of the RTC
connected to a circuit controlling the power of the board.
An event on the interrupt line while the board is off will power it on.

This was tested on a PCA2129, with:

	$ date "2010-10-10 10:10"
	Sun Oct 10 10:10:00 UTC 2010
	$ /usr/sbin/rtcwake -u -d /dev/rtc0  -s10 --mode off
	[ ... ]
	$ # power on after 10 seconds

Liam Beguin (3):
  rtc: pcf2127: add pca2129 device id
  rtc: pcf2127: add alarm support
  rtc: pcf2127: reset alarm interrupt at power on

 drivers/rtc/rtc-pcf2127.c | 131 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 3 deletions(-)

-- 
2.27.0

[PATCH 1/3] rtc: pcf2127: add pca2129 device id

[liambeguin]

From: Liam Beguin <lvb@xiphos.com>

From: Liam Beguin <lvb@xiphos.com>

The PCA2129 is the automotive grade version of the PCF2129.
add it to the list of compatibles.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/rtc/rtc-pcf2127.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 4e50d6768f13..396a1144a213 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -546,6 +546,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 static const struct of_device_id pcf2127_of_match[] = {
 	{ .compatible = "nxp,pcf2127" },
 	{ .compatible = "nxp,pcf2129" },
+	{ .compatible = "nxp,pca2129" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pcf2127_of_match);
@@ -656,6 +657,7 @@ static int pcf2127_i2c_probe(struct i2c_client *client,
 static const struct i2c_device_id pcf2127_i2c_id[] = {
 	{ "pcf2127", 1 },
 	{ "pcf2129", 0 },
+	{ "pca2129", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pcf2127_i2c_id);
@@ -720,6 +722,7 @@ static int pcf2127_spi_probe(struct spi_device *spi)
 static const struct spi_device_id pcf2127_spi_id[] = {
 	{ "pcf2127", 1 },
 	{ "pcf2129", 0 },
+	{ "pca2129", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, pcf2127_spi_id);
-- 
2.27.0

[PATCH 2/3] rtc: pcf2127: add alarm support

[liambeguin]

From: Liam Beguin <lvb@xiphos.com>

From: Liam Beguin <lvb@xiphos.com>

Add alarm support for the pcf2127 RTC chip family.
Tested on pca2129.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/rtc/rtc-pcf2127.c | 120 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 396a1144a213..3eeb085a7c72 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -28,7 +28,9 @@
 #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
 /* Control register 2 */
 #define PCF2127_REG_CTRL2		0x01
+#define PCF2127_BIT_CTRL2_AIE			BIT(1)
 #define PCF2127_BIT_CTRL2_TSIE			BIT(2)
+#define PCF2127_BIT_CTRL2_AF			BIT(4)
 #define PCF2127_BIT_CTRL2_TSF2			BIT(5)
 /* Control register 3 */
 #define PCF2127_REG_CTRL3		0x02
@@ -46,6 +48,12 @@
 #define PCF2127_REG_DW			0x07
 #define PCF2127_REG_MO			0x08
 #define PCF2127_REG_YR			0x09
+/* Alarm registers */
+#define PCF2127_REG_ALARM_SC		0x0A
+#define PCF2127_REG_ALARM_MN		0x0B
+#define PCF2127_REG_ALARM_HR		0x0C
+#define PCF2127_REG_ALARM_DM		0x0D
+#define PCF2127_REG_ALARM_DW		0x0E
 /* Watchdog registers */
 #define PCF2127_REG_WD_CTL		0x10
 #define PCF2127_BIT_WD_CTL_TF0			BIT(0)
@@ -185,6 +193,107 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+	unsigned int buf[5], ctrl2;
+	int ret;
+
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
+	if (ret) {
+		dev_err(dev, "%s: ctrl2 read error\n", __func__);
+		return ret;
+	}
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);
+	if (ret) {
+		dev_err(dev, "%s: alarm read error\n", __func__);
+		return ret;
+	}
+
+	alrm->enabled = ctrl2 & PCF2127_BIT_CTRL2_AIE;
+	alrm->pending = ctrl2 & PCF2127_BIT_CTRL2_AF;
+
+	alrm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
+	alrm->time.tm_min = bcd2bin(buf[1] & 0x7F);
+	alrm->time.tm_hour = bcd2bin(buf[2] & 0x3F);
+	alrm->time.tm_mday = bcd2bin(buf[3] & 0x3F);
+	alrm->time.tm_wday = buf[4] & 0x07;
+
+	dev_dbg(dev, "%s: alarm is %d:%d:%d, mday=%d, wday=%d\n", __func__,
+		alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec,
+		alrm->time.tm_mday, alrm->time.tm_wday);
+
+	return 0;
+}
+
+static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
+{
+	struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+	unsigned int ctrl2;
+	int ret;
+
+	dev_dbg(dev, "%s: %s\n", __func__, enable ? "enable" : "disable");
+
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
+	if (ret) {
+		dev_err(dev, "%s: ctrl2 read error\n", __func__);
+		return ret;
+	}
+
+	if (enable)
+		ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
+				   ctrl2 | PCF2127_BIT_CTRL2_AIE);
+	else
+		ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
+				   ctrl2 & ~PCF2127_BIT_CTRL2_AIE);
+
+	if (ret) {
+		dev_err(dev, "%s: failed to enable alarm (%d)\n", __func__,
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+	unsigned int ctrl2;
+	uint8_t buf[5];
+	int ret;
+
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+				 PCF2127_BIT_CTRL2_AF,
+				 (unsigned int)~PCF2127_BIT_CTRL2_AF);
+	if (ret) {
+		dev_err(dev, "%s: failed to clear alarm interrupt flag (%d)",
+			__func__, ret);
+		return ret;
+	}
+
+	buf[0] = bin2bcd(alrm->time.tm_sec);
+	buf[1] = bin2bcd(alrm->time.tm_min);
+	buf[2] = bin2bcd(alrm->time.tm_hour);
+	buf[3] = bin2bcd(alrm->time.tm_mday);
+	buf[4] = (alrm->time.tm_wday & 0x07);
+
+	dev_dbg(dev, "%s: alarm set for: %d:%d:%d, mday=%d, wday=%d\n",
+		__func__, alrm->time.tm_hour, alrm->time.tm_min,
+		alrm->time.tm_sec, alrm->time.tm_mday, alrm->time.tm_wday);
+
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);
+	if (ret) {
+		dev_err(dev, "%s: failed to write alarm registers (%d)",
+			__func__, ret);
+		return ret;
+	}
+
+	pcf2127_rtc_alarm_irq_enable(dev, alrm->enabled);
+
+	return 0;
+}
+
 #ifdef CONFIG_RTC_INTF_DEV
 static int pcf2127_rtc_ioctl(struct device *dev,
 				unsigned int cmd, unsigned long arg)
@@ -211,9 +320,12 @@ static int pcf2127_rtc_ioctl(struct device *dev,
 #endif
 
 static const struct rtc_class_ops pcf2127_rtc_ops = {
-	.ioctl		= pcf2127_rtc_ioctl,
-	.read_time	= pcf2127_rtc_read_time,
-	.set_time	= pcf2127_rtc_set_time,
+	.ioctl            = pcf2127_rtc_ioctl,
+	.read_time        = pcf2127_rtc_read_time,
+	.set_time         = pcf2127_rtc_set_time,
+	.read_alarm       = pcf2127_rtc_read_alarm,
+	.set_alarm        = pcf2127_rtc_set_alarm,
+	.alarm_irq_enable = pcf2127_rtc_alarm_irq_enable,
 };
 
 static int pcf2127_nvmem_read(void *priv, unsigned int offset,
@@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	pcf2127->rtc->ops = &pcf2127_rtc_ops;
 
+	device_init_wakeup(dev, true);
+
 	pcf2127->wdd.parent = dev;
 	pcf2127->wdd.info = &pcf2127_wdt_info;
 	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
-- 
2.27.0

[PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on

[liambeguin]

From: Liam Beguin <lvb@xiphos.com>

From: Liam Beguin <lvb@xiphos.com>

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/rtc/rtc-pcf2127.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 3eeb085a7c72..f004a4030970 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -546,6 +546,14 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	pcf2127->rtc->ops = &pcf2127_rtc_ops;
 
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+				 PCF2127_BIT_CTRL2_AIE, 0);
+	if (ret) {
+		dev_err(dev, "%s: failed to clear interrupt enable bit (%d)",
+			__func__, ret);
+		return ret;
+	}
+
 	device_init_wakeup(dev, true);
 
 	pcf2127->wdd.parent = dev;
-- 
2.27.0

Re: [PATCH 2/3] rtc: pcf2127: add alarm support

[Bruno Thomsen]

Hi Liam,

See comments below.

Den søn. 7. jun. 2020 kl. 19.06 skrev <liambeguin@gmail.com>:
>
> From: Liam Beguin <lvb@xiphos.com>
>
> From: Liam Beguin <lvb@xiphos.com>
>
> Add alarm support for the pcf2127 RTC chip family.
> Tested on pca2129.
>
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/rtc/rtc-pcf2127.c | 120 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 117 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 396a1144a213..3eeb085a7c72 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -28,7 +28,9 @@
>  #define PCF2127_BIT_CTRL1_TSF1                 BIT(4)
>  /* Control register 2 */
>  #define PCF2127_REG_CTRL2              0x01
> +#define PCF2127_BIT_CTRL2_AIE                  BIT(1)
>  #define PCF2127_BIT_CTRL2_TSIE                 BIT(2)
> +#define PCF2127_BIT_CTRL2_AF                   BIT(4)
>  #define PCF2127_BIT_CTRL2_TSF2                 BIT(5)
>  /* Control register 3 */
>  #define PCF2127_REG_CTRL3              0x02
> @@ -46,6 +48,12 @@
>  #define PCF2127_REG_DW                 0x07
>  #define PCF2127_REG_MO                 0x08
>  #define PCF2127_REG_YR                 0x09
> +/* Alarm registers */
> +#define PCF2127_REG_ALARM_SC           0x0A
> +#define PCF2127_REG_ALARM_MN           0x0B
> +#define PCF2127_REG_ALARM_HR           0x0C
> +#define PCF2127_REG_ALARM_DM           0x0D
> +#define PCF2127_REG_ALARM_DW           0x0E
>  /* Watchdog registers */
>  #define PCF2127_REG_WD_CTL             0x10
>  #define PCF2127_BIT_WD_CTL_TF0                 BIT(0)
> @@ -185,6 +193,107 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
>         return 0;
>  }
>
> +static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> +       unsigned int buf[5], ctrl2;
> +       int ret;
> +
> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> +       if (ret) {
> +               dev_err(dev, "%s: ctrl2 read error\n", __func__);
> +               return ret;
> +       }

Reading CTRL2 register causes watchdog to stop.

Aways call pcf2127_wdt_active_ping() after CTRL2 access to ensure the watchdog
is running if enabled.

> +       ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);

Replace 5 with sizeof(buf).

> +       if (ret) {
> +               dev_err(dev, "%s: alarm read error\n", __func__);
> +               return ret;
> +       }
> +
> +       alrm->enabled = ctrl2 & PCF2127_BIT_CTRL2_AIE;
> +       alrm->pending = ctrl2 & PCF2127_BIT_CTRL2_AF;
> +
> +       alrm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> +       alrm->time.tm_min = bcd2bin(buf[1] & 0x7F);
> +       alrm->time.tm_hour = bcd2bin(buf[2] & 0x3F);
> +       alrm->time.tm_mday = bcd2bin(buf[3] & 0x3F);
> +       alrm->time.tm_wday = buf[4] & 0x07;
> +
> +       dev_dbg(dev, "%s: alarm is %d:%d:%d, mday=%d, wday=%d\n", __func__,
> +               alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec,
> +               alrm->time.tm_mday, alrm->time.tm_wday);
> +
> +       return 0;
> +}
> +
> +static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
> +{
> +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> +       unsigned int ctrl2;
> +       int ret;
> +
> +       dev_dbg(dev, "%s: %s\n", __func__, enable ? "enable" : "disable");

Delete debug trace.

> +
> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> +       if (ret) {
> +               dev_err(dev, "%s: ctrl2 read error\n", __func__);
> +               return ret;
> +       }
> +
> +       if (enable)
> +               ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> +                                  ctrl2 | PCF2127_BIT_CTRL2_AIE);
> +       else
> +               ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> +                                  ctrl2 & ~PCF2127_BIT_CTRL2_AIE);
> +
> +       if (ret) {
> +               dev_err(dev, "%s: failed to enable alarm (%d)\n", __func__,
> +                       ret);
> +               return ret;
> +       }

Replace regmap_read() and regmap_write() with a regmap_update_bits().

So something like:

ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
                                          enabled ? PCF2127_REG_CTRL2 : 0);

And remember to call pcf2127_wdt_active_ping().

> +
> +       return 0;
> +}
> +
> +static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> +       unsigned int ctrl2;
> +       uint8_t buf[5];
> +       int ret;
> +
> +       ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> +                                PCF2127_BIT_CTRL2_AF,
> +                                (unsigned int)~PCF2127_BIT_CTRL2_AF);

If you just want to clear the AF bit in CTRL2, just do:

ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
                                           PCF2127_BIT_CTRL2_AF, 0);

> +       if (ret) {
> +               dev_err(dev, "%s: failed to clear alarm interrupt flag (%d)",
> +                       __func__, ret);
> +               return ret;
> +       }
> +
> +       buf[0] = bin2bcd(alrm->time.tm_sec);
> +       buf[1] = bin2bcd(alrm->time.tm_min);
> +       buf[2] = bin2bcd(alrm->time.tm_hour);
> +       buf[3] = bin2bcd(alrm->time.tm_mday);
> +       buf[4] = (alrm->time.tm_wday & 0x07);
> +
> +       dev_dbg(dev, "%s: alarm set for: %d:%d:%d, mday=%d, wday=%d\n",
> +               __func__, alrm->time.tm_hour, alrm->time.tm_min,
> +               alrm->time.tm_sec, alrm->time.tm_mday, alrm->time.tm_wday);
> +
> +       ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);

Replace 5 with sizeof(buf).

/Bruno

> +       if (ret) {
> +               dev_err(dev, "%s: failed to write alarm registers (%d)",
> +                       __func__, ret);
> +               return ret;
> +       }
> +
> +       pcf2127_rtc_alarm_irq_enable(dev, alrm->enabled);
> +
> +       return 0;
> +}
> +
>  #ifdef CONFIG_RTC_INTF_DEV
>  static int pcf2127_rtc_ioctl(struct device *dev,
>                                 unsigned int cmd, unsigned long arg)
> @@ -211,9 +320,12 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>  #endif
>
>  static const struct rtc_class_ops pcf2127_rtc_ops = {
> -       .ioctl          = pcf2127_rtc_ioctl,
> -       .read_time      = pcf2127_rtc_read_time,
> -       .set_time       = pcf2127_rtc_set_time,
> +       .ioctl            = pcf2127_rtc_ioctl,
> +       .read_time        = pcf2127_rtc_read_time,
> +       .set_time         = pcf2127_rtc_set_time,
> +       .read_alarm       = pcf2127_rtc_read_alarm,
> +       .set_alarm        = pcf2127_rtc_set_alarm,
> +       .alarm_irq_enable = pcf2127_rtc_alarm_irq_enable,
>  };
>
>  static int pcf2127_nvmem_read(void *priv, unsigned int offset,
> @@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>
>         pcf2127->rtc->ops = &pcf2127_rtc_ops;
>
> +       device_init_wakeup(dev, true);
> +
>         pcf2127->wdd.parent = dev;
>         pcf2127->wdd.info = &pcf2127_wdt_info;
>         pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> --
> 2.27.0
>

Re: [PATCH 1/3] rtc: pcf2127: add pca2129 device id

[Bruno Thomsen]

Den søn. 7. jun. 2020 kl. 19.06 skrev <liambeguin@gmail.com>:
>
> From: Liam Beguin <lvb@xiphos.com>
>
> From: Liam Beguin <lvb@xiphos.com>
>
> The PCA2129 is the automotive grade version of the PCF2129.
> add it to the list of compatibles.
>
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/rtc/rtc-pcf2127.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 4e50d6768f13..396a1144a213 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -546,6 +546,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  static const struct of_device_id pcf2127_of_match[] = {
>         { .compatible = "nxp,pcf2127" },
>         { .compatible = "nxp,pcf2129" },
> +       { .compatible = "nxp,pca2129" },

Remember to document new compatible string here:
Documentation/devicetree/bindings/rtc/trivial-rtc.yaml

/Bruno

>         {}
>  };
>  MODULE_DEVICE_TABLE(of, pcf2127_of_match);
> @@ -656,6 +657,7 @@ static int pcf2127_i2c_probe(struct i2c_client *client,
>  static const struct i2c_device_id pcf2127_i2c_id[] = {
>         { "pcf2127", 1 },
>         { "pcf2129", 0 },
> +       { "pca2129", 0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, pcf2127_i2c_id);
> @@ -720,6 +722,7 @@ static int pcf2127_spi_probe(struct spi_device *spi)
>  static const struct spi_device_id pcf2127_spi_id[] = {
>         { "pcf2127", 1 },
>         { "pcf2129", 0 },
> +       { "pca2129", 0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(spi, pcf2127_spi_id);
> --
> 2.27.0
>

Re: [PATCH 2/3] rtc: pcf2127: add alarm support

[Alexandre Belloni]

On 07/06/2020 13:06:09-0400, liambeguin@gmail.com wrote:
>  static int pcf2127_nvmem_read(void *priv, unsigned int offset,
> @@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  
>  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
>  
> +	device_init_wakeup(dev, true);
> +

This can't be done unconditionally, You need to have been able to
request an interrupt or the wakeup-source property needs to be present.

The interrupt handler is also missing from the patch.


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

Re: [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on

[Alexandre Belloni]

On 07/06/2020 13:06:10-0400, liambeguin@gmail.com wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> From: Liam Beguin <lvb@xiphos.com>

Always include a commit message.

> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/rtc/rtc-pcf2127.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 3eeb085a7c72..f004a4030970 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -546,6 +546,14 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  
>  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
>  
> +	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> +				 PCF2127_BIT_CTRL2_AIE, 0);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to clear interrupt enable bit (%d)",
> +			__func__, ret);
> +		return ret;
> +	}
> +

The driver simply must not do that or alarms will be missed.

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

Re: [PATCH 2/3] rtc: pcf2127: add alarm support

[Liam Beguin]

Hi Bruno,

Thanks for your comments, I've updated the patch and will send a v2
soon.

On Tue Jun 9, 2020 at 10:42 PM Bruno Thomsen wrote:
> Hi Liam,
> 
> See comments below.
> 
> Den søn. 7. jun. 2020 kl. 19.06 skrev <liambeguin@gmail.com>:
> >
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > Add alarm support for the pcf2127 RTC chip family.
> > Tested on pca2129.
> >
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/rtc/rtc-pcf2127.c | 120 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 117 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 396a1144a213..3eeb085a7c72 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -28,7 +28,9 @@
> >  #define PCF2127_BIT_CTRL1_TSF1                 BIT(4)
> >  /* Control register 2 */
> >  #define PCF2127_REG_CTRL2              0x01
> > +#define PCF2127_BIT_CTRL2_AIE                  BIT(1)
> >  #define PCF2127_BIT_CTRL2_TSIE                 BIT(2)
> > +#define PCF2127_BIT_CTRL2_AF                   BIT(4)
> >  #define PCF2127_BIT_CTRL2_TSF2                 BIT(5)
> >  /* Control register 3 */
> >  #define PCF2127_REG_CTRL3              0x02
> > @@ -46,6 +48,12 @@
> >  #define PCF2127_REG_DW                 0x07
> >  #define PCF2127_REG_MO                 0x08
> >  #define PCF2127_REG_YR                 0x09
> > +/* Alarm registers */
> > +#define PCF2127_REG_ALARM_SC           0x0A
> > +#define PCF2127_REG_ALARM_MN           0x0B
> > +#define PCF2127_REG_ALARM_HR           0x0C
> > +#define PCF2127_REG_ALARM_DM           0x0D
> > +#define PCF2127_REG_ALARM_DW           0x0E
> >  /* Watchdog registers */
> >  #define PCF2127_REG_WD_CTL             0x10
> >  #define PCF2127_BIT_WD_CTL_TF0                 BIT(0)
> > @@ -185,6 +193,107 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >         return 0;
> >  }
> >
> > +static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > +       unsigned int buf[5], ctrl2;
> > +       int ret;
> > +
> > +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> > +       if (ret) {
> > +               dev_err(dev, "%s: ctrl2 read error\n", __func__);
> > +               return ret;
> > +       }
> 
> Reading CTRL2 register causes watchdog to stop.
> 
> Aways call pcf2127_wdt_active_ping() after CTRL2 access to ensure the watchdog
> is running if enabled.
> 
> > +       ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);
> 
> Replace 5 with sizeof(buf).
> 
> > +       if (ret) {
> > +               dev_err(dev, "%s: alarm read error\n", __func__);
> > +               return ret;
> > +       }
> > +
> > +       alrm->enabled = ctrl2 & PCF2127_BIT_CTRL2_AIE;
> > +       alrm->pending = ctrl2 & PCF2127_BIT_CTRL2_AF;
> > +
> > +       alrm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> > +       alrm->time.tm_min = bcd2bin(buf[1] & 0x7F);
> > +       alrm->time.tm_hour = bcd2bin(buf[2] & 0x3F);
> > +       alrm->time.tm_mday = bcd2bin(buf[3] & 0x3F);
> > +       alrm->time.tm_wday = buf[4] & 0x07;
> > +
> > +       dev_dbg(dev, "%s: alarm is %d:%d:%d, mday=%d, wday=%d\n", __func__,
> > +               alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec,
> > +               alrm->time.tm_mday, alrm->time.tm_wday);
> > +
> > +       return 0;
> > +}
> > +
> > +static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
> > +{
> > +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > +       unsigned int ctrl2;
> > +       int ret;
> > +
> > +       dev_dbg(dev, "%s: %s\n", __func__, enable ? "enable" : "disable");
> 
> Delete debug trace.
> 
> > +
> > +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> > +       if (ret) {
> > +               dev_err(dev, "%s: ctrl2 read error\n", __func__);
> > +               return ret;
> > +       }
> > +
> > +       if (enable)
> > +               ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> > +                                  ctrl2 | PCF2127_BIT_CTRL2_AIE);
> > +       else
> > +               ret = regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> > +                                  ctrl2 & ~PCF2127_BIT_CTRL2_AIE);
> > +
> > +       if (ret) {
> > +               dev_err(dev, "%s: failed to enable alarm (%d)\n", __func__,
> > +                       ret);
> > +               return ret;
> > +       }
> 
> Replace regmap_read() and regmap_write() with a regmap_update_bits().
> 
> So something like:
> 
> ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
>                                           enabled ? PCF2127_REG_CTRL2 : 0);
> 
> And remember to call pcf2127_wdt_active_ping().
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > +       struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > +       unsigned int ctrl2;
> > +       uint8_t buf[5];
> > +       int ret;
> > +
> > +       ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > +                                PCF2127_BIT_CTRL2_AF,
> > +                                (unsigned int)~PCF2127_BIT_CTRL2_AF);
> 
> If you just want to clear the AF bit in CTRL2, just do:
> 
> ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
>                                            PCF2127_BIT_CTRL2_AF, 0);
> 
> > +       if (ret) {
> > +               dev_err(dev, "%s: failed to clear alarm interrupt flag (%d)",
> > +                       __func__, ret);
> > +               return ret;
> > +       }
> > +
> > +       buf[0] = bin2bcd(alrm->time.tm_sec);
> > +       buf[1] = bin2bcd(alrm->time.tm_min);
> > +       buf[2] = bin2bcd(alrm->time.tm_hour);
> > +       buf[3] = bin2bcd(alrm->time.tm_mday);
> > +       buf[4] = (alrm->time.tm_wday & 0x07);
> > +
> > +       dev_dbg(dev, "%s: alarm set for: %d:%d:%d, mday=%d, wday=%d\n",
> > +               __func__, alrm->time.tm_hour, alrm->time.tm_min,
> > +               alrm->time.tm_sec, alrm->time.tm_mday, alrm->time.tm_wday);
> > +
> > +       ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);
> 
> Replace 5 with sizeof(buf).
> 
> /Bruno
> 

Thanks again for your time,
Liam

> > +       if (ret) {
> > +               dev_err(dev, "%s: failed to write alarm registers (%d)",
> > +                       __func__, ret);
> > +               return ret;
> > +       }
> > +
> > +       pcf2127_rtc_alarm_irq_enable(dev, alrm->enabled);
> > +
> > +       return 0;
> > +}
> > +
> >  #ifdef CONFIG_RTC_INTF_DEV
> >  static int pcf2127_rtc_ioctl(struct device *dev,
> >                                 unsigned int cmd, unsigned long arg)
> > @@ -211,9 +320,12 @@ static int pcf2127_rtc_ioctl(struct device *dev,
> >  #endif
> >
> >  static const struct rtc_class_ops pcf2127_rtc_ops = {
> > -       .ioctl          = pcf2127_rtc_ioctl,
> > -       .read_time      = pcf2127_rtc_read_time,
> > -       .set_time       = pcf2127_rtc_set_time,
> > +       .ioctl            = pcf2127_rtc_ioctl,
> > +       .read_time        = pcf2127_rtc_read_time,
> > +       .set_time         = pcf2127_rtc_set_time,
> > +       .read_alarm       = pcf2127_rtc_read_alarm,
> > +       .set_alarm        = pcf2127_rtc_set_alarm,
> > +       .alarm_irq_enable = pcf2127_rtc_alarm_irq_enable,
> >  };
> >
> >  static int pcf2127_nvmem_read(void *priv, unsigned int offset,
> > @@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> >
> >         pcf2127->rtc->ops = &pcf2127_rtc_ops;
> >
> > +       device_init_wakeup(dev, true);
> > +
> >         pcf2127->wdd.parent = dev;
> >         pcf2127->wdd.info = &pcf2127_wdt_info;
> >         pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> > --
> > 2.27.0
> >

Re: [PATCH 2/3] rtc: pcf2127: add alarm support

[Liam Beguin]

Hi Alexandre,

On Tue Jun 9, 2020 at 11:05 PM Alexandre Belloni wrote:
> On 07/06/2020 13:06:09-0400, liambeguin@gmail.com wrote:
> >  static int pcf2127_nvmem_read(void *priv, unsigned int offset,
> > @@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> >  
> >  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
> >  
> > +	device_init_wakeup(dev, true);
> > +
> 
> This can't be done unconditionally, You need to have been able to
> request an interrupt or the wakeup-source property needs to be present.
> 
> The interrupt handler is also missing from the patch.

Like I tried to explain in the cover letter, the interrupt line isn't
connected to the CPU on the board I'm using.
I'd be glad to add the interrupt handler to this patch. Is there a way I
can make it conditional?
Thanks,

Liam

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

Re: [PATCH 2/3] rtc: pcf2127: add alarm support

[Alexandre Belloni]

On 10/06/2020 11:49:06-0400, Liam Beguin wrote:
> Hi Alexandre,
> 
> On Tue Jun 9, 2020 at 11:05 PM Alexandre Belloni wrote:
> > On 07/06/2020 13:06:09-0400, liambeguin@gmail.com wrote:
> > >  static int pcf2127_nvmem_read(void *priv, unsigned int offset,
> > > @@ -434,6 +546,8 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > >  
> > >  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
> > >  
> > > +	device_init_wakeup(dev, true);
> > > +
> > 
> > This can't be done unconditionally, You need to have been able to
> > request an interrupt or the wakeup-source property needs to be present.
> > 
> > The interrupt handler is also missing from the patch.
> 
> Like I tried to explain in the cover letter, the interrupt line isn't
> connected to the CPU on the board I'm using.
> I'd be glad to add the interrupt handler to this patch. Is there a way I
> can make it conditional?

It is necessarily conditional, as it won't be used if no interrupt is
provided.


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