* [PATCH v2 0/2] rtc: pcf2127: add alarm support
@ 2020-06-14 4:04 Liam Beguin
2020-06-14 4:04 ` [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id Liam Beguin
2020-06-14 4:04 ` [PATCH v2 2/2] rtc: pcf2127: add alarm support Liam Beguin
0 siblings, 2 replies; 11+ messages in thread
From: Liam Beguin @ 2020-06-14 4:04 UTC (permalink / raw)
To: bruno.thomsen, a.zummo, alexandre.belloni, linux-rtc
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.
Because of these hardware limitations, the irq handler added in this
patch wasn't fully tested.
The alarm fuctionality 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
Changes since v1:
- Document new compatible string for the pca2129
- Add calls to pcf2127_wdt_active_ping after accessing CTRL2
- Use sizeof(buf) instead of hadcoding value
- Cleanup debug trace
- Add interrupt handler and wakeup-source devicetree option
Liam Beguin (2):
rtc: pcf2127: add pca2129 device id
rtc: pcf2127: add alarm support
.../devicetree/bindings/rtc/trivial-rtc.yaml | 2 +
drivers/rtc/rtc-pcf2127.c | 172 +++++++++++++++++-
2 files changed, 171 insertions(+), 3 deletions(-)
Interdiff against v1:
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index 18cb456752f6..c7d14de214c4 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -52,6 +52,8 @@ properties:
- nxp,pcf2127
# Real-time clock
- nxp,pcf2129
+ # Real-time clock
+ - nxp,pca2129
# Real-time Clock Module
- pericom,pt7c4338
# I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index f004a4030970..87ecb29247c6 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_irq.h>
#include <linux/regmap.h>
#include <linux/watchdog.h>
@@ -87,6 +88,8 @@
#define PCF2127_WD_VAL_MAX 255
#define PCF2127_WD_VAL_DEFAULT 60
+static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
+
struct pcf2127 {
struct rtc_device *rtc;
struct watchdog_device wdd;
@@ -204,7 +207,13 @@ static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
dev_err(dev, "%s: ctrl2 read error\n", __func__);
return ret;
}
- ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf, 5);
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ return ret;
+
+ ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
+ sizeof(buf));
if (ret) {
dev_err(dev, "%s: alarm read error\n", __func__);
return ret;
@@ -229,29 +238,21 @@ static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
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);
+ ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+ PCF2127_BIT_CTRL2_AIE,
+ enable ? PCF2127_BIT_CTRL2_AIE : 0);
if (ret) {
- dev_err(dev, "%s: ctrl2 read error\n", __func__);
+ dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
+ enable ? "enable" : "disable",
+ ret);
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);
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
return ret;
- }
return 0;
}
@@ -259,19 +260,21 @@ static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
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);
+ PCF2127_BIT_CTRL2_AF, 0);
if (ret) {
dev_err(dev, "%s: failed to clear alarm interrupt flag (%d)",
__func__, ret);
return ret;
}
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ return ret;
+
buf[0] = bin2bcd(alrm->time.tm_sec);
buf[1] = bin2bcd(alrm->time.tm_min);
buf[2] = bin2bcd(alrm->time.tm_hour);
@@ -282,7 +285,8 @@ static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
__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);
+ ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
+ sizeof(buf));
if (ret) {
dev_err(dev, "%s: failed to write alarm registers (%d)",
__func__, ret);
@@ -294,6 +298,38 @@ static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return 0;
}
+static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
+{
+ struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+ unsigned int ctrl2 = 0;
+ int ret = 0;
+
+ ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
+ if (ret) {
+ dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
+ goto irq_err;
+ }
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ goto irq_err;
+
+ if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
+ regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+ PCF2127_BIT_CTRL2_AF, 0);
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ goto irq_err;
+
+ rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
+ }
+
+ return IRQ_HANDLED;
+irq_err:
+ return IRQ_NONE;
+}
+
#ifdef CONFIG_RTC_INTF_DEV
static int pcf2127_rtc_ioctl(struct device *dev,
unsigned int cmd, unsigned long arg)
@@ -527,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
const char *name, bool has_nvmem)
{
struct pcf2127 *pcf2127;
+ int alarm_irq = 0;
u32 wdd_timeout;
int ret = 0;
@@ -546,15 +583,19 @@ 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;
+ alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
+ if (alarm_irq >= 0) {
+ ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ dev_name(dev), dev);
+ if (ret) {
+ dev_err(dev, "failed to request alarm irq\n");
+ return ret;
+ }
}
- device_init_wakeup(dev, true);
+ if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
+ device_init_wakeup(dev, true);
pcf2127->wdd.parent = dev;
pcf2127->wdd.info = &pcf2127_wdt_info;
base-commit: aaa2faab4ed8e5fe0111e04d6e168c028fe2987f
--
2.27.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id
2020-06-14 4:04 [PATCH v2 0/2] rtc: pcf2127: add alarm support Liam Beguin
@ 2020-06-14 4:04 ` Liam Beguin
2020-06-16 8:17 ` Bruno Thomsen
2020-06-14 4:04 ` [PATCH v2 2/2] rtc: pcf2127: add alarm support Liam Beguin
1 sibling, 1 reply; 11+ messages in thread
From: Liam Beguin @ 2020-06-14 4:04 UTC (permalink / raw)
To: bruno.thomsen, a.zummo, alexandre.belloni, linux-rtc
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>
---
Changes since v1:
- Document new compatible string for the pca2129
Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
drivers/rtc/rtc-pcf2127.c | 3 +++
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index 18cb456752f6..c7d14de214c4 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -52,6 +52,8 @@ properties:
- nxp,pcf2127
# Real-time clock
- nxp,pcf2129
+ # Real-time clock
+ - nxp,pca2129
# Real-time Clock Module
- pericom,pt7c4338
# I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
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 related [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id
2020-06-14 4:04 ` [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id Liam Beguin
@ 2020-06-16 8:17 ` Bruno Thomsen
2020-06-16 8:45 ` Alexandre Belloni
0 siblings, 1 reply; 11+ messages in thread
From: Bruno Thomsen @ 2020-06-16 8:17 UTC (permalink / raw)
To: Liam Beguin; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc
Den søn. 14. jun. 2020 kl. 06.04 skrev Liam Beguin <liambeguin@gmail.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>
> ---
> Changes since v1:
> - Document new compatible string for the pca2129
>
> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
> drivers/rtc/rtc-pcf2127.c | 3 +++
> 2 files changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> index 18cb456752f6..c7d14de214c4 100644
> --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> @@ -52,6 +52,8 @@ properties:
> - nxp,pcf2127
> # Real-time clock
> - nxp,pcf2129
> + # Real-time clock
> + - nxp,pca2129
> # Real-time Clock Module
> - pericom,pt7c4338
> # I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
Split device tree binding update into separate patch and
remember to add devicetree@vger.kernel.org mailing list
when you send the next patch series version.
/Bruno
> 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* Re: [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id
2020-06-16 8:17 ` Bruno Thomsen
@ 2020-06-16 8:45 ` Alexandre Belloni
0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-16 8:45 UTC (permalink / raw)
To: Bruno Thomsen; +Cc: Liam Beguin, Alessandro Zummo, linux-rtc
On 16/06/2020 10:17:21+0200, Bruno Thomsen wrote:
> Den søn. 14. jun. 2020 kl. 06.04 skrev Liam Beguin <liambeguin@gmail.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>
> > ---
> > Changes since v1:
> > - Document new compatible string for the pca2129
> >
> > Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
> > drivers/rtc/rtc-pcf2127.c | 3 +++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > index 18cb456752f6..c7d14de214c4 100644
> > --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > @@ -52,6 +52,8 @@ properties:
> > - nxp,pcf2127
> > # Real-time clock
> > - nxp,pcf2129
> > + # Real-time clock
> > + - nxp,pca2129
> > # Real-time Clock Module
> > - pericom,pt7c4338
> > # I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
>
> Split device tree binding update into separate patch and
> remember to add devicetree@vger.kernel.org mailing list
> when you send the next patch series version.
>
For trivial rtc, it is not really necessary to separate the binding doc,
especially when simply adding a compatible string.
> /Bruno
>
> > 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
> >
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] rtc: pcf2127: add alarm support
2020-06-14 4:04 [PATCH v2 0/2] rtc: pcf2127: add alarm support Liam Beguin
2020-06-14 4:04 ` [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id Liam Beguin
@ 2020-06-14 4:04 ` Liam Beguin
2020-06-16 8:38 ` Bruno Thomsen
2020-06-16 9:02 ` Alexandre Belloni
1 sibling, 2 replies; 11+ messages in thread
From: Liam Beguin @ 2020-06-14 4:04 UTC (permalink / raw)
To: bruno.thomsen, a.zummo, alexandre.belloni, linux-rtc
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>
---
Changes since v1:
- Add calls to pcf2127_wdt_active_ping after accessing CTRL2
- Cleanup calls to regmap_{read,write,update_bits}
- Cleanup debug trace
- Add interrupt handler, untested because of hardware limitations
- Add wakeup-source devicetree option
drivers/rtc/rtc-pcf2127.c | 169 +++++++++++++++++++++++++++++++++++++-
1 file changed, 166 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 396a1144a213..87ecb29247c6 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_irq.h>
#include <linux/regmap.h>
#include <linux/watchdog.h>
@@ -28,7 +29,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 +49,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)
@@ -79,6 +88,8 @@
#define PCF2127_WD_VAL_MAX 255
#define PCF2127_WD_VAL_DEFAULT 60
+static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
+
struct pcf2127 {
struct rtc_device *rtc;
struct watchdog_device wdd;
@@ -185,6 +196,140 @@ 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 = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ return ret;
+
+ ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
+ 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);
+ int ret;
+
+ ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+ PCF2127_BIT_CTRL2_AIE,
+ enable ? PCF2127_BIT_CTRL2_AIE : 0);
+ if (ret) {
+ dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
+ enable ? "enable" : "disable",
+ ret);
+ return ret;
+ }
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (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);
+ uint8_t buf[5];
+ int ret;
+
+ 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;
+ }
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (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,
+ sizeof(buf));
+ 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;
+}
+
+static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
+{
+ struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+ unsigned int ctrl2 = 0;
+ int ret = 0;
+
+ ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
+ if (ret) {
+ dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
+ goto irq_err;
+ }
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ goto irq_err;
+
+ if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
+ regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+ PCF2127_BIT_CTRL2_AF, 0);
+
+ ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+ if (ret)
+ goto irq_err;
+
+ rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
+ }
+
+ return IRQ_HANDLED;
+irq_err:
+ return IRQ_NONE;
+}
+
#ifdef CONFIG_RTC_INTF_DEV
static int pcf2127_rtc_ioctl(struct device *dev,
unsigned int cmd, unsigned long arg)
@@ -211,9 +356,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,
@@ -415,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
const char *name, bool has_nvmem)
{
struct pcf2127 *pcf2127;
+ int alarm_irq = 0;
u32 wdd_timeout;
int ret = 0;
@@ -434,6 +583,20 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
pcf2127->rtc->ops = &pcf2127_rtc_ops;
+ alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
+ if (alarm_irq >= 0) {
+ ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ dev_name(dev), dev);
+ if (ret) {
+ dev_err(dev, "failed to request alarm irq\n");
+ return ret;
+ }
+ }
+
+ if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
+ 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 related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] rtc: pcf2127: add alarm support
2020-06-14 4:04 ` [PATCH v2 2/2] rtc: pcf2127: add alarm support Liam Beguin
@ 2020-06-16 8:38 ` Bruno Thomsen
2020-06-16 8:47 ` Alexandre Belloni
2020-06-16 13:11 ` Liam Beguin
2020-06-16 9:02 ` Alexandre Belloni
1 sibling, 2 replies; 11+ messages in thread
From: Bruno Thomsen @ 2020-06-16 8:38 UTC (permalink / raw)
To: Liam Beguin; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc
Hi Liam
Good updates, but you need to rebase patches on top of the latest mainline tree.
Den søn. 14. jun. 2020 kl. 06.04 skrev Liam Beguin <liambeguin@gmail.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>
> ---
> Changes since v1:
> - Add calls to pcf2127_wdt_active_ping after accessing CTRL2
> - Cleanup calls to regmap_{read,write,update_bits}
> - Cleanup debug trace
> - Add interrupt handler, untested because of hardware limitations
> - Add wakeup-source devicetree option
>
> drivers/rtc/rtc-pcf2127.c | 169 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 166 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 396a1144a213..87ecb29247c6 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -20,6 +20,7 @@
> #include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
>
> @@ -28,7 +29,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 +49,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)
> @@ -79,6 +88,8 @@
> #define PCF2127_WD_VAL_MAX 255
> #define PCF2127_WD_VAL_DEFAULT 60
>
> +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
> +
> struct pcf2127 {
> struct rtc_device *rtc;
> struct watchdog_device wdd;
> @@ -185,6 +196,140 @@ 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 = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
> + 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);
> + int ret;
> +
> + ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> + PCF2127_BIT_CTRL2_AIE,
> + enable ? PCF2127_BIT_CTRL2_AIE : 0);
> + if (ret) {
> + dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
> + enable ? "enable" : "disable",
> + ret);
> + return ret;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + return ret;
> +
> + return 0;
Just do "return ret;" unconditional.
> +}
> +
> +static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> + uint8_t buf[5];
> + int ret;
> +
> + 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;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (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,
> + sizeof(buf));
> + 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;
> +}
> +
> +static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> +{
> + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> + unsigned int ctrl2 = 0;
> + int ret = 0;
> +
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> + if (ret) {
> + dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
> + goto irq_err;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + goto irq_err;
> +
> + if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
> + regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> + PCF2127_BIT_CTRL2_AF, 0);
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + goto irq_err;
> +
> + rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> + }
Do pcf2127_wdt_active_ping() here to avoid having the same code twice
in the function.
/Bruno
> +
> + return IRQ_HANDLED;
> +irq_err:
> + return IRQ_NONE;
> +}
> +
> #ifdef CONFIG_RTC_INTF_DEV
> static int pcf2127_rtc_ioctl(struct device *dev,
> unsigned int cmd, unsigned long arg)
> @@ -211,9 +356,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,
> @@ -415,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> const char *name, bool has_nvmem)
> {
> struct pcf2127 *pcf2127;
> + int alarm_irq = 0;
> u32 wdd_timeout;
> int ret = 0;
>
> @@ -434,6 +583,20 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>
> pcf2127->rtc->ops = &pcf2127_rtc_ops;
>
> + alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
> + if (alarm_irq >= 0) {
> + ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + dev_name(dev), dev);
> + if (ret) {
> + dev_err(dev, "failed to request alarm irq\n");
> + return ret;
> + }
> + }
> +
> + if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
> + 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 v2 2/2] rtc: pcf2127: add alarm support
2020-06-16 8:38 ` Bruno Thomsen
@ 2020-06-16 8:47 ` Alexandre Belloni
2020-06-16 13:11 ` Liam Beguin
1 sibling, 0 replies; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-16 8:47 UTC (permalink / raw)
To: Bruno Thomsen; +Cc: Liam Beguin, Alessandro Zummo, linux-rtc
On 16/06/2020 10:38:00+0200, Bruno Thomsen wrote:
> > +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 = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
> > + 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);
> > + int ret;
> > +
> > + ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + PCF2127_BIT_CTRL2_AIE,
> > + enable ? PCF2127_BIT_CTRL2_AIE : 0);
> > + if (ret) {
> > + dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
> > + enable ? "enable" : "disable",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Just do "return ret;" unconditional.
>
Just return pcf2127_wdt_active_ping(&pcf2127->wdd);
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] rtc: pcf2127: add alarm support
2020-06-16 8:38 ` Bruno Thomsen
2020-06-16 8:47 ` Alexandre Belloni
@ 2020-06-16 13:11 ` Liam Beguin
1 sibling, 0 replies; 11+ messages in thread
From: Liam Beguin @ 2020-06-16 13:11 UTC (permalink / raw)
To: Bruno Thomsen; +Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc
Hi Bruno,
On Tue Jun 16, 2020 at 10:38 AM Bruno Thomsen wrote:
> Hi Liam
>
> Good updates, but you need to rebase patches on top of the latest mainline tree.
Thanks, I'll rebase before sending v3.
>
> Den søn. 14. jun. 2020 kl. 06.04 skrev Liam Beguin <liambeguin@gmail.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>
> > ---
> > Changes since v1:
> > - Add calls to pcf2127_wdt_active_ping after accessing CTRL2
> > - Cleanup calls to regmap_{read,write,update_bits}
> > - Cleanup debug trace
> > - Add interrupt handler, untested because of hardware limitations
> > - Add wakeup-source devicetree option
> >
> > drivers/rtc/rtc-pcf2127.c | 169 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 166 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 396a1144a213..87ecb29247c6 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -20,6 +20,7 @@
> > #include <linux/slab.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > +#include <linux/of_irq.h>
> > #include <linux/regmap.h>
> > #include <linux/watchdog.h>
> >
> > @@ -28,7 +29,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 +49,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)
> > @@ -79,6 +88,8 @@
> > #define PCF2127_WD_VAL_MAX 255
> > #define PCF2127_WD_VAL_DEFAULT 60
> >
> > +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
> > +
> > struct pcf2127 {
> > struct rtc_device *rtc;
> > struct watchdog_device wdd;
> > @@ -185,6 +196,140 @@ 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 = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
> > + 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);
> > + int ret;
> > +
> > + ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + PCF2127_BIT_CTRL2_AIE,
> > + enable ? PCF2127_BIT_CTRL2_AIE : 0);
> > + if (ret) {
> > + dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
> > + enable ? "enable" : "disable",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Just do "return ret;" unconditional.
>
> > +}
> > +
> > +static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > + uint8_t buf[5];
> > + int ret;
> > +
> > + 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;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (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,
> > + sizeof(buf));
> > + 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;
> > +}
> > +
> > +static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> > +{
> > + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > + unsigned int ctrl2 = 0;
> > + int ret = 0;
> > +
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> > + if (ret) {
> > + dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
> > + goto irq_err;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + goto irq_err;
> > +
> > + if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
> > + regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + PCF2127_BIT_CTRL2_AF, 0);
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + goto irq_err;
> > +
> > + rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> > + }
>
> Do pcf2127_wdt_active_ping() here to avoid having the same code twice
> in the function.
That looks better, I'll update.
Thanks,
Liam
>
> /Bruno
>
> > +
> > + return IRQ_HANDLED;
> > +irq_err:
> > + return IRQ_NONE;
> > +}
> > +
> > #ifdef CONFIG_RTC_INTF_DEV
> > static int pcf2127_rtc_ioctl(struct device *dev,
> > unsigned int cmd, unsigned long arg)
> > @@ -211,9 +356,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,
> > @@ -415,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > const char *name, bool has_nvmem)
> > {
> > struct pcf2127 *pcf2127;
> > + int alarm_irq = 0;
> > u32 wdd_timeout;
> > int ret = 0;
> >
> > @@ -434,6 +583,20 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> >
> > pcf2127->rtc->ops = &pcf2127_rtc_ops;
> >
> > + alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
> > + if (alarm_irq >= 0) {
> > + ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + dev_name(dev), dev);
> > + if (ret) {
> > + dev_err(dev, "failed to request alarm irq\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
> > + 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 v2 2/2] rtc: pcf2127: add alarm support
2020-06-14 4:04 ` [PATCH v2 2/2] rtc: pcf2127: add alarm support Liam Beguin
2020-06-16 8:38 ` Bruno Thomsen
@ 2020-06-16 9:02 ` Alexandre Belloni
2020-06-16 13:07 ` Liam Beguin
1 sibling, 1 reply; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-16 9:02 UTC (permalink / raw)
To: Liam Beguin; +Cc: bruno.thomsen, a.zummo, linux-rtc
Hi,
On 14/06/2020 00:04:09-0400, Liam Beguin wrote:
> 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>
> ---
> Changes since v1:
> - Add calls to pcf2127_wdt_active_ping after accessing CTRL2
> - Cleanup calls to regmap_{read,write,update_bits}
> - Cleanup debug trace
> - Add interrupt handler, untested because of hardware limitations
> - Add wakeup-source devicetree option
>
> drivers/rtc/rtc-pcf2127.c | 169 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 166 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 396a1144a213..87ecb29247c6 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -20,6 +20,7 @@
> #include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
>
> @@ -28,7 +29,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 +49,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)
> @@ -79,6 +88,8 @@
> #define PCF2127_WD_VAL_MAX 255
> #define PCF2127_WD_VAL_DEFAULT 60
>
> +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
> +
This forward declaration should be avoided.
> struct pcf2127 {
> struct rtc_device *rtc;
> struct watchdog_device wdd;
> @@ -185,6 +196,140 @@ 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__);
Honestly, I would prefer avoiding adding so many strings in the driver.
The reality is that nobody will look into dmesg to know what was the
issue and even if somebody does, the solution would simply be to start
the operation again. Something that can already be deducted when
returning a simple error code. (This is valid for the subsequent
dev_err).
> + return ret;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
> + 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);
> +
It is generally not useful to have those debug strings anymore because
the core already provides tracepoints at the correct locations.
If you really want to keep it, then please use %ptR.
This is also valid for the other dev_dbg.
> + return 0;
> +}
> +
> +static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
> +{
> + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> + PCF2127_BIT_CTRL2_AIE,
> + enable ? PCF2127_BIT_CTRL2_AIE : 0);
> + if (ret) {
> + dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
> + enable ? "enable" : "disable",
> + ret);
> + return ret;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (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);
> + uint8_t buf[5];
> + int ret;
> +
> + 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;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (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,
> + sizeof(buf));
> + 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;
> +}
> +
> +static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> +{
> + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> + unsigned int ctrl2 = 0;
> + int ret = 0;
> +
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> + if (ret) {
> + dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
> + goto irq_err;
> + }
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + goto irq_err;
> +
> + if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
> + regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> + PCF2127_BIT_CTRL2_AF, 0);
In that case, I think it makes more sense to use regmap_write as this
would avoid another read of ctrl2.
> +
> + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> + if (ret)
> + goto irq_err;
> +
> + rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> + }
> +
> + return IRQ_HANDLED;
> +irq_err:
> + return IRQ_NONE;
> +}
> +
> #ifdef CONFIG_RTC_INTF_DEV
> static int pcf2127_rtc_ioctl(struct device *dev,
> unsigned int cmd, unsigned long arg)
> @@ -211,9 +356,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,
> @@ -415,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> const char *name, bool has_nvmem)
> {
> struct pcf2127 *pcf2127;
> + int alarm_irq = 0;
> u32 wdd_timeout;
> int ret = 0;
>
> @@ -434,6 +583,20 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>
> pcf2127->rtc->ops = &pcf2127_rtc_ops;
>
> + alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
> + if (alarm_irq >= 0) {
> + ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + dev_name(dev), dev);
> + if (ret) {
> + dev_err(dev, "failed to request alarm irq\n");
> + return ret;
> + }
> + }
> +
> + if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
> + device_init_wakeup(dev, true);
The last thing here is that you should have two different rtc_class_ops
struct, one with alarm and the other one without. at this point, you
know which one you should use. I know this is not convenient but I'm
working on a series to make things better. Until this is ready, this is
what we will have to live with.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] rtc: pcf2127: add alarm support
2020-06-16 9:02 ` Alexandre Belloni
@ 2020-06-16 13:07 ` Liam Beguin
2020-06-16 18:31 ` Alexandre Belloni
0 siblings, 1 reply; 11+ messages in thread
From: Liam Beguin @ 2020-06-16 13:07 UTC (permalink / raw)
To: Alexandre Belloni; +Cc: bruno.thomsen, a.zummo, linux-rtc
Hi Alexandre,
On Tue Jun 16, 2020 at 11:02 AM Alexandre Belloni wrote:
> Hi,
>
> On 14/06/2020 00:04:09-0400, Liam Beguin wrote:
> > 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>
> > ---
> > Changes since v1:
> > - Add calls to pcf2127_wdt_active_ping after accessing CTRL2
> > - Cleanup calls to regmap_{read,write,update_bits}
> > - Cleanup debug trace
> > - Add interrupt handler, untested because of hardware limitations
> > - Add wakeup-source devicetree option
> >
> > drivers/rtc/rtc-pcf2127.c | 169 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 166 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 396a1144a213..87ecb29247c6 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -20,6 +20,7 @@
> > #include <linux/slab.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > +#include <linux/of_irq.h>
> > #include <linux/regmap.h>
> > #include <linux/watchdog.h>
> >
> > @@ -28,7 +29,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 +49,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)
> > @@ -79,6 +88,8 @@
> > #define PCF2127_WD_VAL_MAX 255
> > #define PCF2127_WD_VAL_DEFAULT 60
> >
> > +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd);
> > +
>
> This forward declaration should be avoided.
>
I'll try to move things around to avoid this.
> > struct pcf2127 {
> > struct rtc_device *rtc;
> > struct watchdog_device wdd;
> > @@ -185,6 +196,140 @@ 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__);
>
> Honestly, I would prefer avoiding adding so many strings in the driver.
> The reality is that nobody will look into dmesg to know what was the
> issue and even if somebody does, the solution would simply be to start
> the operation again. Something that can already be deducted when
> returning a simple error code. (This is valid for the subsequent
> dev_err).
>
Understood, I'll get rid of these unnecessary strings.
> > + return ret;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_ALARM_SC, buf,
> > + 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);
> > +
>
> It is generally not useful to have those debug strings anymore because
> the core already provides tracepoints at the correct locations.
>
> If you really want to keep it, then please use %ptR.
>
> This is also valid for the other dev_dbg.
>
I'm not particularly attached to keeping these in. I just left them in
since it seemed to be common in other rtc drivers.
I'll update to use %ptR.
> > + return 0;
> > +}
> > +
> > +static int pcf2127_rtc_alarm_irq_enable(struct device *dev, u32 enable)
> > +{
> > + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + PCF2127_BIT_CTRL2_AIE,
> > + enable ? PCF2127_BIT_CTRL2_AIE : 0);
> > + if (ret) {
> > + dev_err(dev, "%s: failed to %s alarm (%d)\n", __func__,
> > + enable ? "enable" : "disable",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (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);
> > + uint8_t buf[5];
> > + int ret;
> > +
> > + 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;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (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,
> > + sizeof(buf));
> > + 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;
> > +}
> > +
> > +static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> > +{
> > + struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > + unsigned int ctrl2 = 0;
> > + int ret = 0;
> > +
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> > + if (ret) {
> > + dev_err(dev, "%s: ctrl2 read error (%d)\n", __func__, ret);
> > + goto irq_err;
> > + }
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + goto irq_err;
> > +
> > + if (ctrl2 & PCF2127_BIT_CTRL2_AF) {
> > + regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + PCF2127_BIT_CTRL2_AF, 0);
>
> In that case, I think it makes more sense to use regmap_write as this
> would avoid another read of ctrl2.
>
Thanks, will update that.
> > +
> > + ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
> > + if (ret)
> > + goto irq_err;
> > +
> > + rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +irq_err:
> > + return IRQ_NONE;
> > +}
> > +
> > #ifdef CONFIG_RTC_INTF_DEV
> > static int pcf2127_rtc_ioctl(struct device *dev,
> > unsigned int cmd, unsigned long arg)
> > @@ -211,9 +356,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,
> > @@ -415,6 +563,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > const char *name, bool has_nvmem)
> > {
> > struct pcf2127 *pcf2127;
> > + int alarm_irq = 0;
> > u32 wdd_timeout;
> > int ret = 0;
> >
> > @@ -434,6 +583,20 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> >
> > pcf2127->rtc->ops = &pcf2127_rtc_ops;
> >
> > + alarm_irq = of_irq_get_byname(dev->of_node, "alarm");
> > + if (alarm_irq >= 0) {
> > + ret = devm_request_irq(dev, alarm_irq, pcf2127_rtc_irq,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + dev_name(dev), dev);
> > + if (ret) {
> > + dev_err(dev, "failed to request alarm irq\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (alarm_irq >= 0 || device_property_read_bool(dev, "wakeup-source"))
> > + device_init_wakeup(dev, true);
>
> The last thing here is that you should have two different rtc_class_ops
> struct, one with alarm and the other one without. at this point, you
> know which one you should use. I know this is not convenient but I'm
> working on a series to make things better. Until this is ready, this is
> what we will have to live with.
>
Okay, will update that too.
Thanks for your time,
Liam
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] rtc: pcf2127: add alarm support
2020-06-16 13:07 ` Liam Beguin
@ 2020-06-16 18:31 ` Alexandre Belloni
0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Belloni @ 2020-06-16 18:31 UTC (permalink / raw)
To: Liam Beguin; +Cc: bruno.thomsen, a.zummo, linux-rtc
On 16/06/2020 09:07:59-0400, Liam Beguin wrote:
> > It is generally not useful to have those debug strings anymore because
> > the core already provides tracepoints at the correct locations.
> >
> > If you really want to keep it, then please use %ptR.
> >
> > This is also valid for the other dev_dbg.
> >
>
> I'm not particularly attached to keeping these in. I just left them in
> since it seemed to be common in other rtc drivers.
>
Yes, those predates the tracepoints (which are fairly recent).
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-16 18:31 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-14 4:04 [PATCH v2 0/2] rtc: pcf2127: add alarm support Liam Beguin
2020-06-14 4:04 ` [PATCH v2 1/2] rtc: pcf2127: add pca2129 device id Liam Beguin
2020-06-16 8:17 ` Bruno Thomsen
2020-06-16 8:45 ` Alexandre Belloni
2020-06-14 4:04 ` [PATCH v2 2/2] rtc: pcf2127: add alarm support Liam Beguin
2020-06-16 8:38 ` Bruno Thomsen
2020-06-16 8:47 ` Alexandre Belloni
2020-06-16 13:11 ` Liam Beguin
2020-06-16 9:02 ` Alexandre Belloni
2020-06-16 13:07 ` Liam Beguin
2020-06-16 18:31 ` Alexandre Belloni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).