| Message ID | 20200614040409.30302-3-liambeguin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] rtc: pcf2127: add pca2129 device id | expand |
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 >
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);
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.
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
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 > >
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).
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;