Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] rtc: pcf2127: add alarm support
@ 2020-06-07 17:06 liambeguin
  2020-06-07 17:06 ` [PATCH 1/3] rtc: pcf2127: add pca2129 device id liambeguin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: liambeguin @ 2020-06-07 17:06 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, linux-rtc

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] rtc: pcf2127: add pca2129 device id
  2020-06-07 17:06 [PATCH 0/3] rtc: pcf2127: add alarm support liambeguin
@ 2020-06-07 17:06 ` liambeguin
  2020-06-09 20:48   ` Bruno Thomsen
  2020-06-07 17:06 ` [PATCH 2/3] rtc: pcf2127: add alarm support liambeguin
  2020-06-07 17:06 ` [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on liambeguin
  2 siblings, 1 reply; 11+ messages in thread
From: liambeguin @ 2020-06-07 17:06 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, linux-rtc

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-07 17:06 [PATCH 0/3] rtc: pcf2127: add alarm support liambeguin
  2020-06-07 17:06 ` [PATCH 1/3] rtc: pcf2127: add pca2129 device id liambeguin
@ 2020-06-07 17:06 ` liambeguin
  2020-06-09 20:42   ` Bruno Thomsen
  2020-06-09 21:05   ` Alexandre Belloni
  2020-06-07 17:06 ` [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on liambeguin
  2 siblings, 2 replies; 11+ messages in thread
From: liambeguin @ 2020-06-07 17:06 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, linux-rtc

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on
  2020-06-07 17:06 [PATCH 0/3] rtc: pcf2127: add alarm support liambeguin
  2020-06-07 17:06 ` [PATCH 1/3] rtc: pcf2127: add pca2129 device id liambeguin
  2020-06-07 17:06 ` [PATCH 2/3] rtc: pcf2127: add alarm support liambeguin
@ 2020-06-07 17:06 ` liambeguin
  2020-06-09 21:07   ` Alexandre Belloni
  2 siblings, 1 reply; 11+ messages in thread
From: liambeguin @ 2020-06-07 17:06 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, linux-rtc

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-07 17:06 ` [PATCH 2/3] rtc: pcf2127: add alarm support liambeguin
@ 2020-06-09 20:42   ` Bruno Thomsen
  2020-06-10 15:38     ` Liam Beguin
  2020-06-09 21:05   ` Alexandre Belloni
  1 sibling, 1 reply; 11+ messages in thread
From: Bruno Thomsen @ 2020-06-09 20:42 UTC (permalink / raw)
  To: liambeguin; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] rtc: pcf2127: add pca2129 device id
  2020-06-07 17:06 ` [PATCH 1/3] rtc: pcf2127: add pca2129 device id liambeguin
@ 2020-06-09 20:48   ` Bruno Thomsen
  0 siblings, 0 replies; 11+ messages in thread
From: Bruno Thomsen @ 2020-06-09 20:48 UTC (permalink / raw)
  To: liambeguin; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-07 17:06 ` [PATCH 2/3] rtc: pcf2127: add alarm support liambeguin
  2020-06-09 20:42   ` Bruno Thomsen
@ 2020-06-09 21:05   ` Alexandre Belloni
  2020-06-10 15:49     ` Liam Beguin
  1 sibling, 1 reply; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-09 21:05 UTC (permalink / raw)
  To: liambeguin; +Cc: a.zummo, linux-rtc

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on
  2020-06-07 17:06 ` [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on liambeguin
@ 2020-06-09 21:07   ` Alexandre Belloni
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-09 21:07 UTC (permalink / raw)
  To: liambeguin; +Cc: a.zummo, linux-rtc

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-09 20:42   ` Bruno Thomsen
@ 2020-06-10 15:38     ` Liam Beguin
  0 siblings, 0 replies; 11+ messages in thread
From: Liam Beguin @ 2020-06-10 15:38 UTC (permalink / raw)
  To: Bruno Thomsen; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc

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
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-09 21:05   ` Alexandre Belloni
@ 2020-06-10 15:49     ` Liam Beguin
  2020-06-10 16:32       ` Alexandre Belloni
  0 siblings, 1 reply; 11+ messages in thread
From: Liam Beguin @ 2020-06-10 15:49 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: a.zummo, linux-rtc

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rtc: pcf2127: add alarm support
  2020-06-10 15:49     ` Liam Beguin
@ 2020-06-10 16:32       ` Alexandre Belloni
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-10 16:32 UTC (permalink / raw)
  To: Liam Beguin; +Cc: a.zummo, linux-rtc

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-07 17:06 [PATCH 0/3] rtc: pcf2127: add alarm support liambeguin
2020-06-07 17:06 ` [PATCH 1/3] rtc: pcf2127: add pca2129 device id liambeguin
2020-06-09 20:48   ` Bruno Thomsen
2020-06-07 17:06 ` [PATCH 2/3] rtc: pcf2127: add alarm support liambeguin
2020-06-09 20:42   ` Bruno Thomsen
2020-06-10 15:38     ` Liam Beguin
2020-06-09 21:05   ` Alexandre Belloni
2020-06-10 15:49     ` Liam Beguin
2020-06-10 16:32       ` Alexandre Belloni
2020-06-07 17:06 ` [PATCH 3/3] rtc: pcf2127: reset alarm interrupt at power on liambeguin
2020-06-09 21:07   ` Alexandre Belloni

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git