[PATCH 1/2] Port rtc-pcf2123 to regmap

[PATCH 1/2] Port rtc-pcf2123 to regmap

[Howey, Dylan]

Replace all calls to pcf2123_read and _write with regmap.

Also remove pcf2123_delay_trec. This claimed to add a 30ns delay to SPI
writes, but I could not see any reference to this requirement in the
datasheet. Closest thing I could find to a requirement are timings for the
SPI chip enable line, which cannot be controlled by this driver (the ndelay
came after the call to spi_write_then_read, which means it would sleep
after CE has already gone inactive). Things seem to work fine without it.

Signed-off-by: Dylan Howey <Dylan.Howey@tennantco.com>
---
 drivers/rtc/rtc-pcf2123.c | 156 ++++++++++++++++++--------------------
 1 file changed, 75 insertions(+), 81 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 39da8b214275..4bfcb1e67c9b 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -45,6 +45,7 @@
 #include <linux/spi/spi.h>
 #include <linux/module.h>
 #include <linux/sysfs.h>
+#include <linux/regmap.h>
 
 /* REGISTERS */
 #define PCF2123_REG_CTRL1	(0x00)	/* Control Register 1 */
@@ -114,58 +115,29 @@ struct pcf2123_sysfs_reg {
 
 struct pcf2123_plat_data {
 	struct rtc_device *rtc;
+	struct regmap *map;
 	struct pcf2123_sysfs_reg regs[16];
 };
 
-/*
- * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
- * is released properly after an SPI write.  This function should be
- * called after EVERY read/write call over SPI.
- */
-static inline void pcf2123_delay_trec(void)
-{
-	ndelay(30);
-}
-
-static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size)
-{
-	struct spi_device *spi = to_spi_device(dev);
-	int ret;
-
-	reg |= PCF2123_READ;
-	ret = spi_write_then_read(spi, &reg, 1, rxbuf, size);
-	pcf2123_delay_trec();
-
-	return ret;
-}
-
-static int pcf2123_write(struct device *dev, u8 *txbuf, size_t size)
-{
-	struct spi_device *spi = to_spi_device(dev);
-	int ret;
-
-	txbuf[0] |= PCF2123_WRITE;
-	ret = spi_write(spi, txbuf, size);
-	pcf2123_delay_trec();
-
-	return ret;
-}
-
-static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
-{
-	u8 txbuf[2];
+static const struct regmap_range pcf2123_ranges[] = {
+	{
+		.range_min = PCF2123_REG_CTRL1,
+		.range_max = PCF2123_REG_CTDWN_TMR,
+	},
+};
 
-	txbuf[0] = reg;
-	txbuf[1] = val;
-	return pcf2123_write(dev, txbuf, sizeof(txbuf));
-}
+static const struct regmap_access_table pcf2123_access_table = {
+	.yes_ranges = pcf2123_ranges,
+	.n_yes_ranges = ARRAY_SIZE(pcf2123_ranges),
+};
 
 static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 			    char *buffer)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
 	struct pcf2123_sysfs_reg *r;
-	u8 rxbuf[1];
 	unsigned long reg;
+	unsigned int val = 0;
 	int ret;
 
 	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
@@ -174,16 +146,17 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	ret = pcf2123_read(dev, reg, rxbuf, 1);
-	if (ret < 0)
+	ret = regmap_read(pdata->map, reg, &val);
+	if (ret)
 		return -EIO;
 
-	return sprintf(buffer, "0x%x\n", rxbuf[0]);
+	return sprintf(buffer, "0x%x\n", val);
 }
 
 static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 			     const char *buffer, size_t count)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
 	struct pcf2123_sysfs_reg *r;
 	unsigned long reg;
 	unsigned long val;
@@ -200,19 +173,20 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	ret = pcf2123_write_reg(dev, reg, val);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, reg, val);
+	if (ret)
 		return -EIO;
 	return count;
 }
 
 static int pcf2123_read_offset(struct device *dev, long *offset)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
 	int ret;
 	s8 reg;
 
-	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
-	if (ret < 0)
+	ret = regmap_raw_read(pdata->map, PCF2123_REG_OFFSET, &reg, 1);
+	if (ret)
 		return ret;
 
 	if (reg & OFFSET_COARSE)
@@ -237,6 +211,8 @@ static int pcf2123_read_offset(struct device *dev, long *offset)
  */
 static int pcf2123_set_offset(struct device *dev, long offset)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
+	int ret;
 	s8 reg;
 
 	if (offset > OFFSET_STEP * 127)
@@ -256,16 +232,18 @@ static int pcf2123_set_offset(struct device *dev, long offset)
 		reg |= OFFSET_COARSE;
 	}
 
-	return pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
+	ret = regmap_raw_write(pdata->map, PCF2123_REG_OFFSET, &reg, 1);
+	return ret;
 }
 
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
 	u8 rxbuf[7];
 	int ret;
 
-	ret = pcf2123_read(dev, PCF2123_REG_SC, rxbuf, sizeof(rxbuf));
-	if (ret < 0)
+	ret = regmap_bulk_read(pdata->map, PCF2123_REG_SC, rxbuf, 7);
+	if (ret)
 		return ret;
 
 	if (rxbuf[0] & OSC_HAS_STOPPED) {
@@ -294,7 +272,8 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
-	u8 txbuf[8];
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
+	u8 txbuf[7];
 	int ret;
 
 	dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
@@ -304,27 +283,26 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 			tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
 
 	/* Stop the counter first */
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_STOP);
+	if (ret)
 		return ret;
 
 	/* Set the new time */
-	txbuf[0] = PCF2123_REG_SC;
-	txbuf[1] = bin2bcd(tm->tm_sec & 0x7F);
-	txbuf[2] = bin2bcd(tm->tm_min & 0x7F);
-	txbuf[3] = bin2bcd(tm->tm_hour & 0x3F);
-	txbuf[4] = bin2bcd(tm->tm_mday & 0x3F);
-	txbuf[5] = tm->tm_wday & 0x07;
-	txbuf[6] = bin2bcd((tm->tm_mon + 1) & 0x1F); /* rtc mn 1-12 */
-	txbuf[7] = bin2bcd(tm->tm_year < 100 ? tm->tm_year : tm->tm_year - 100);
-
-	ret = pcf2123_write(dev, txbuf, sizeof(txbuf));
-	if (ret < 0)
+	txbuf[0] = bin2bcd(tm->tm_sec & 0x7F);
+	txbuf[1] = bin2bcd(tm->tm_min & 0x7F);
+	txbuf[2] = bin2bcd(tm->tm_hour & 0x3F);
+	txbuf[3] = bin2bcd(tm->tm_mday & 0x3F);
+	txbuf[4] = tm->tm_wday & 0x07;
+	txbuf[5] = bin2bcd((tm->tm_mon + 1) & 0x1F); /* rtc mn 1-12 */
+	txbuf[6] = bin2bcd(tm->tm_year < 100 ? tm->tm_year : tm->tm_year - 100);
+
+	ret = regmap_bulk_write(pdata->map, PCF2123_REG_SC, txbuf, 7);
+	if (ret)
 		return ret;
 
 	/* Start the counter */
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_CLEAR);
+	if (ret)
 		return ret;
 
 	return 0;
@@ -332,33 +310,33 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 static int pcf2123_reset(struct device *dev)
 {
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
 	int ret;
-	u8  rxbuf[2];
+	unsigned int val = 0;
 
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
+	if (ret)
 		return ret;
 
 	/* Stop the counter */
 	dev_dbg(dev, "stopping RTC\n");
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_STOP);
+	if (ret)
 		return ret;
 
 	/* See if the counter was actually stopped */
 	dev_dbg(dev, "checking for presence of RTC\n");
-	ret = pcf2123_read(dev, PCF2123_REG_CTRL1, rxbuf, sizeof(rxbuf));
-	if (ret < 0)
+	ret = regmap_read(pdata->map, PCF2123_REG_CTRL1, &val);
+	if (ret)
 		return ret;
 
-	dev_dbg(dev, "received data from RTC (0x%02X 0x%02X)\n",
-		rxbuf[0], rxbuf[1]);
-	if (!(rxbuf[0] & CTRL1_STOP))
+	dev_dbg(dev, "received data from RTC (0x%08X)\n", val);
+	if (!(val & CTRL1_STOP))
 		return -ENODEV;
 
 	/* Start the counter */
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
-	if (ret < 0)
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_CLEAR);
+	if (ret)
 		return ret;
 
 	return 0;
@@ -377,7 +355,8 @@ static int pcf2123_probe(struct spi_device *spi)
 	struct rtc_device *rtc;
 	struct rtc_time tm;
 	struct pcf2123_plat_data *pdata;
-	int ret, i;
+	struct regmap_config config;
+	int ret = 0, i;
 
 	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
 				GFP_KERNEL);
@@ -385,6 +364,21 @@ static int pcf2123_probe(struct spi_device *spi)
 		return -ENOMEM;
 	spi->dev.platform_data = pdata;
 
+	memset(&config, 0, sizeof(config));
+	config.reg_bits = 8;
+	config.val_bits = 8;
+	config.read_flag_mask = PCF2123_READ;
+	config.write_flag_mask = PCF2123_WRITE;
+	config.max_register = PCF2123_REG_CTDWN_TMR;
+	config.wr_table = &pcf2123_access_table;
+
+	pdata->map = devm_regmap_init_spi(spi, &config);
+
+	if (IS_ERR(pdata->map)) {
+		dev_err(&spi->dev, "regmap init failed.\n");
+		goto kfree_exit;
+	}
+
 	ret = pcf2123_rtc_read_time(&spi->dev, &tm);
 	if (ret < 0) {
 		ret = pcf2123_reset(&spi->dev);
-- 
2.17.1

[PATCH 2/2] Add alarm support to rtc-pcf2123

[Howey, Dylan]

Allows alarm to be set using the wakealarm sysfs interface.

Signed-off-by: Dylan Howey <Dylan.Howey@tennantco.com>
---
 drivers/rtc/rtc-pcf2123.c | 115 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 4bfcb1e67c9b..8c16a313980d 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -308,6 +308,107 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static int pcf2123_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
+	u8 rxbuf[4];
+	int ret;
+	unsigned int val = 0;
+
+	ret = regmap_bulk_read(pdata->map, PCF2123_REG_ALRM_MN, rxbuf, 4);
+	if (ret)
+		return ret;
+
+	alm->time.tm_min = bcd2bin(rxbuf[0] & 0x7F);
+	alm->time.tm_hour = bcd2bin(rxbuf[1] & 0x3F);
+	alm->time.tm_mday = bcd2bin(rxbuf[2] & 0x3F);
+	alm->time.tm_wday = bcd2bin(rxbuf[3] & 0x07);
+
+	dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d mday=%d, mon=%d, year=%d, wday=%d\n",
+			__func__,
+			alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
+			alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
+			alm->time.tm_wday);
+
+	ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
+	if (ret)
+		return ret;
+
+	alm->enabled = !!(val & CTRL2_AIE);
+
+	return 0;
+}
+
+static int pcf2123_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
+	u8 txbuf[4];
+	int ret;
+	unsigned int val = 0;
+
+	dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
+			__func__,
+			alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
+			alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
+			alm->time.tm_wday);
+
+	/* Disable alarm */
+	ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
+	if (ret)
+		return ret;
+
+	val &= ~(CTRL2_AIE);
+	ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
+
+	/* Set new alarm */
+	txbuf[0] = bin2bcd(alm->time.tm_min & 0x7F);
+	txbuf[1] = bin2bcd(alm->time.tm_hour & 0x3F);
+	txbuf[2] = bin2bcd(alm->time.tm_mday & 0x3F);
+	txbuf[3] = bin2bcd(alm->time.tm_wday & 0x07);
+
+	ret = regmap_bulk_write(pdata->map, PCF2123_REG_ALRM_MN, txbuf, 4);
+	if (ret)
+		return ret;
+
+	if (alm->enabled)	{
+		ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
+		if (ret)
+			return ret;
+
+		val |= CTRL2_AIE;
+		ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static irqreturn_t pcf2123_rtc_irq(int irq, void *dev)
+{
+	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
+	struct mutex *lock = &pdata->rtc->ops_lock;
+	unsigned int val = 0;
+	int ret = IRQ_NONE;
+
+	mutex_lock(lock);
+	regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
+
+	/* Alarm? */
+	if (val & CTRL2_AF) {
+		ret = IRQ_HANDLED;
+
+		val &= ~(CTRL2_AF);
+		regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
+
+		rtc_update_irq(pdata->rtc, 1, RTC_IRQF | RTC_AF);
+	}
+
+	mutex_unlock(lock);
+
+	return ret;
+}
+
 static int pcf2123_reset(struct device *dev)
 {
 	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
@@ -347,6 +448,8 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.set_time	= pcf2123_rtc_set_time,
 	.read_offset	= pcf2123_read_offset,
 	.set_offset	= pcf2123_set_offset,
+	.read_alarm = pcf2123_rtc_read_alarm,
+	.set_alarm = pcf2123_rtc_set_alarm,
 
 };
 
@@ -391,6 +494,8 @@ static int pcf2123_probe(struct spi_device *spi)
 	dev_info(&spi->dev, "spiclk %u KHz.\n",
 			(spi->max_speed_hz + 500) / 1000);
 
+	device_init_wakeup(&spi->dev, true);
+
 	/* Finalize the initialization */
 	rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
 			&pcf2123_rtc_ops, THIS_MODULE);
@@ -418,6 +523,16 @@ static int pcf2123_probe(struct spi_device *spi)
 		}
 	}
 
+	/* Register alarm irq */
+	if (spi->irq >= 0)	{
+		ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+				pcf2123_rtc_irq,
+				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+				pcf2123_driver.driver.name, &spi->dev);
+		if (ret)
+			dev_err(&spi->dev, "could not request irq.\n");
+	}
+
 	return 0;
 
 sysfs_exit:
-- 
2.17.1

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Alexandre Belloni]

Hello,

Please use a subject consistent with the subsystem style (here rtc:
pcf2123: ... ).

On 26/04/2019 19:37:11+0000, Howey, Dylan wrote:
> Replace all calls to pcf2123_read and _write with regmap.
> 
> Also remove pcf2123_delay_trec. This claimed to add a 30ns delay to SPI
> writes, but I could not see any reference to this requirement in the
> datasheet. Closest thing I could find to a requirement are timings for the
> SPI chip enable line, which cannot be controlled by this driver (the ndelay
> came after the call to spi_write_then_read, which means it would sleep
> after CE has already gone inactive). Things seem to work fine without it.
> 
> Signed-off-by: Dylan Howey <Dylan.Howey@tennantco.com>

This has to match the From: of the email.

> ---
>  drivers/rtc/rtc-pcf2123.c | 156 ++++++++++++++++++--------------------
>  1 file changed, 75 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> index 39da8b214275..4bfcb1e67c9b 100644
> --- a/drivers/rtc/rtc-pcf2123.c
> +++ b/drivers/rtc/rtc-pcf2123.c
> @@ -45,6 +45,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/module.h>
>  #include <linux/sysfs.h>
> +#include <linux/regmap.h>
>  
>  /* REGISTERS */
>  #define PCF2123_REG_CTRL1	(0x00)	/* Control Register 1 */
> @@ -114,58 +115,29 @@ struct pcf2123_sysfs_reg {
>  
>  struct pcf2123_plat_data {
>  	struct rtc_device *rtc;
> +	struct regmap *map;
>  	struct pcf2123_sysfs_reg regs[16];
>  };
>  
> -/*
> - * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
> - * is released properly after an SPI write.  This function should be
> - * called after EVERY read/write call over SPI.
> - */
> -static inline void pcf2123_delay_trec(void)
> -{
> -	ndelay(30);
> -}
> -
> -static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size)
> -{
> -	struct spi_device *spi = to_spi_device(dev);
> -	int ret;
> -
> -	reg |= PCF2123_READ;
> -	ret = spi_write_then_read(spi, &reg, 1, rxbuf, size);
> -	pcf2123_delay_trec();
> -
> -	return ret;
> -}
> -
> -static int pcf2123_write(struct device *dev, u8 *txbuf, size_t size)
> -{
> -	struct spi_device *spi = to_spi_device(dev);
> -	int ret;
> -
> -	txbuf[0] |= PCF2123_WRITE;
> -	ret = spi_write(spi, txbuf, size);
> -	pcf2123_delay_trec();
> -
> -	return ret;
> -}
> -
> -static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
> -{
> -	u8 txbuf[2];
> +static const struct regmap_range pcf2123_ranges[] = {
> +	{
> +		.range_min = PCF2123_REG_CTRL1,
> +		.range_max = PCF2123_REG_CTDWN_TMR,
> +	},
> +};
>  
> -	txbuf[0] = reg;
> -	txbuf[1] = val;
> -	return pcf2123_write(dev, txbuf, sizeof(txbuf));
> -}
> +static const struct regmap_access_table pcf2123_access_table = {
> +	.yes_ranges = pcf2123_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(pcf2123_ranges),
> +};

This covers all the registers, I don't think this is necessary.

>  
>  static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
>  			    char *buffer)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
>  	struct pcf2123_sysfs_reg *r;
> -	u8 rxbuf[1];
>  	unsigned long reg;
> +	unsigned int val = 0;
>  	int ret;
>  
>  	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> @@ -174,16 +146,17 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
>  	if (ret)
>  		return ret;
>  
> -	ret = pcf2123_read(dev, reg, rxbuf, 1);
> -	if (ret < 0)
> +	ret = regmap_read(pdata->map, reg, &val);
> +	if (ret)
>  		return -EIO;
>  
> -	return sprintf(buffer, "0x%x\n", rxbuf[0]);
> +	return sprintf(buffer, "0x%x\n", val);
>  }
>  
>  static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
>  			     const char *buffer, size_t count)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
>  	struct pcf2123_sysfs_reg *r;
>  	unsigned long reg;
>  	unsigned long val;
> @@ -200,19 +173,20 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
>  	if (ret)
>  		return ret;
>  
> -	ret = pcf2123_write_reg(dev, reg, val);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, reg, val);
> +	if (ret)
>  		return -EIO;
>  	return count;
>  }
>  

You should add a preliminary patch removing that sysfs interface
exposing all the registers. regmap will anyway provide you a debugfs
interface.

>  static int pcf2123_read_offset(struct device *dev, long *offset)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
>  	int ret;
>  	s8 reg;
>  
> -	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
> -	if (ret < 0)
> +	ret = regmap_raw_read(pdata->map, PCF2123_REG_OFFSET, &reg, 1);

Do you really need the raw version of regmap_read?

> +	if (ret)
>  		return ret;
>  
>  	if (reg & OFFSET_COARSE)
> @@ -237,6 +211,8 @@ static int pcf2123_read_offset(struct device *dev, long *offset)
>   */
>  static int pcf2123_set_offset(struct device *dev, long offset)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> +	int ret;
>  	s8 reg;
>  
>  	if (offset > OFFSET_STEP * 127)
> @@ -256,16 +232,18 @@ static int pcf2123_set_offset(struct device *dev, long offset)
>  		reg |= OFFSET_COARSE;
>  	}
>  
> -	return pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
> +	ret = regmap_raw_write(pdata->map, PCF2123_REG_OFFSET, &reg, 1);
> +	return ret;

ret is not necessary here.

>  }
>  
>  static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
>  	u8 rxbuf[7];
>  	int ret;
>  
> -	ret = pcf2123_read(dev, PCF2123_REG_SC, rxbuf, sizeof(rxbuf));
> -	if (ret < 0)
> +	ret = regmap_bulk_read(pdata->map, PCF2123_REG_SC, rxbuf, 7);

you should keep using sizeof(rxbuf).

> +	if (ret)
>  		return ret;
>  
>  	if (rxbuf[0] & OSC_HAS_STOPPED) {
> @@ -294,7 +272,8 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  
>  static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
> -	u8 txbuf[8];
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> +	u8 txbuf[7];
>  	int ret;
>  
>  	dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> @@ -304,27 +283,26 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  			tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
>  
>  	/* Stop the counter first */
> -	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_STOP);
> +	if (ret)
>  		return ret;
>  
>  	/* Set the new time */
> -	txbuf[0] = PCF2123_REG_SC;
> -	txbuf[1] = bin2bcd(tm->tm_sec & 0x7F);
> -	txbuf[2] = bin2bcd(tm->tm_min & 0x7F);
> -	txbuf[3] = bin2bcd(tm->tm_hour & 0x3F);
> -	txbuf[4] = bin2bcd(tm->tm_mday & 0x3F);
> -	txbuf[5] = tm->tm_wday & 0x07;
> -	txbuf[6] = bin2bcd((tm->tm_mon + 1) & 0x1F); /* rtc mn 1-12 */
> -	txbuf[7] = bin2bcd(tm->tm_year < 100 ? tm->tm_year : tm->tm_year - 100);
> -
> -	ret = pcf2123_write(dev, txbuf, sizeof(txbuf));
> -	if (ret < 0)
> +	txbuf[0] = bin2bcd(tm->tm_sec & 0x7F);
> +	txbuf[1] = bin2bcd(tm->tm_min & 0x7F);
> +	txbuf[2] = bin2bcd(tm->tm_hour & 0x3F);
> +	txbuf[3] = bin2bcd(tm->tm_mday & 0x3F);
> +	txbuf[4] = tm->tm_wday & 0x07;
> +	txbuf[5] = bin2bcd((tm->tm_mon + 1) & 0x1F); /* rtc mn 1-12 */
> +	txbuf[6] = bin2bcd(tm->tm_year < 100 ? tm->tm_year : tm->tm_year - 100);
> +
> +	ret = regmap_bulk_write(pdata->map, PCF2123_REG_SC, txbuf, 7);

Ditto for txbuf

> +	if (ret)
>  		return ret;
>  
>  	/* Start the counter */
> -	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_CLEAR);
> +	if (ret)
>  		return ret;
>  
>  	return 0;
> @@ -332,33 +310,33 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  static int pcf2123_reset(struct device *dev)
>  {
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
>  	int ret;
> -	u8  rxbuf[2];
> +	unsigned int val = 0;
>  
> -	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
> +	if (ret)
>  		return ret;
>  
>  	/* Stop the counter */
>  	dev_dbg(dev, "stopping RTC\n");
> -	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_STOP);
> +	if (ret)
>  		return ret;
>  
>  	/* See if the counter was actually stopped */
>  	dev_dbg(dev, "checking for presence of RTC\n");
> -	ret = pcf2123_read(dev, PCF2123_REG_CTRL1, rxbuf, sizeof(rxbuf));
> -	if (ret < 0)
> +	ret = regmap_read(pdata->map, PCF2123_REG_CTRL1, &val);
> +	if (ret)
>  		return ret;
>  
> -	dev_dbg(dev, "received data from RTC (0x%02X 0x%02X)\n",
> -		rxbuf[0], rxbuf[1]);
> -	if (!(rxbuf[0] & CTRL1_STOP))
> +	dev_dbg(dev, "received data from RTC (0x%08X)\n", val);
> +	if (!(val & CTRL1_STOP))
>  		return -ENODEV;
>  
>  	/* Start the counter */
> -	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
> -	if (ret < 0)
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL1, CTRL1_CLEAR);
> +	if (ret)
>  		return ret;
>  
>  	return 0;
> @@ -377,7 +355,8 @@ static int pcf2123_probe(struct spi_device *spi)
>  	struct rtc_device *rtc;
>  	struct rtc_time tm;
>  	struct pcf2123_plat_data *pdata;
> -	int ret, i;
> +	struct regmap_config config;
> +	int ret = 0, i;
>  
>  	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
>  				GFP_KERNEL);
> @@ -385,6 +364,21 @@ static int pcf2123_probe(struct spi_device *spi)
>  		return -ENOMEM;
>  	spi->dev.platform_data = pdata;
>  
> +	memset(&config, 0, sizeof(config));
> +	config.reg_bits = 8;
> +	config.val_bits = 8;
> +	config.read_flag_mask = PCF2123_READ;
> +	config.write_flag_mask = PCF2123_WRITE;
> +	config.max_register = PCF2123_REG_CTDWN_TMR;
> +	config.wr_table = &pcf2123_access_table;
> +

You should probably have the regmap_config as a global static const.
This would make the initialization easier.

> +	pdata->map = devm_regmap_init_spi(spi, &config);
> +
> +	if (IS_ERR(pdata->map)) {
> +		dev_err(&spi->dev, "regmap init failed.\n");
> +		goto kfree_exit;
> +	}
> +
>  	ret = pcf2123_rtc_read_time(&spi->dev, &tm);
>  	if (ret < 0) {
>  		ret = pcf2123_reset(&spi->dev);
> -- 
> 2.17.1
> 

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

Re: [PATCH 2/2] Add alarm support to rtc-pcf2123

[Alexandre Belloni]

On 26/04/2019 19:37:15+0000, Howey, Dylan wrote:
> +static int pcf2123_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> +	u8 rxbuf[4];
> +	int ret;
> +	unsigned int val = 0;
> +
> +	ret = regmap_bulk_read(pdata->map, PCF2123_REG_ALRM_MN, rxbuf, 4);

sizeof(rxbuf) instead of 4.

> +	if (ret)
> +		return ret;
> +
> +	alm->time.tm_min = bcd2bin(rxbuf[0] & 0x7F);
> +	alm->time.tm_hour = bcd2bin(rxbuf[1] & 0x3F);
> +	alm->time.tm_mday = bcd2bin(rxbuf[2] & 0x3F);
> +	alm->time.tm_wday = bcd2bin(rxbuf[3] & 0x07);
> +
> +	dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d mday=%d, mon=%d, year=%d, wday=%d\n",
> +			__func__,
> +			alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
> +			alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
> +			alm->time.tm_wday);
> +

Please use %ptR.

> +	ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> +	if (ret)
> +		return ret;
> +
> +	alm->enabled = !!(val & CTRL2_AIE);
> +
> +	return 0;
> +}
> +
> +static int pcf2123_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> +	u8 txbuf[4];
> +	int ret;
> +	unsigned int val = 0;
> +
> +	dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
> +			__func__,
> +			alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
> +			alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
> +			alm->time.tm_wday);
> +

Ditto

> +	/* Disable alarm */
> +	ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~(CTRL2_AIE);
> +	ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> +
Please use regmap_update_bits

> +	/* Set new alarm */
> +	txbuf[0] = bin2bcd(alm->time.tm_min & 0x7F);
> +	txbuf[1] = bin2bcd(alm->time.tm_hour & 0x3F);
> +	txbuf[2] = bin2bcd(alm->time.tm_mday & 0x3F);
> +	txbuf[3] = bin2bcd(alm->time.tm_wday & 0x07);
> +
> +	ret = regmap_bulk_write(pdata->map, PCF2123_REG_ALRM_MN, txbuf, 4);

sizeof(txbuf)

> +	if (ret)
> +		return ret;
> +
> +	if (alm->enabled)	{
> +		ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> +		if (ret)
> +			return ret;
> +
> +		val |= CTRL2_AIE;
> +		ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> +		if (ret)
> +			return ret;

Use regmap_update_bits

> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t pcf2123_rtc_irq(int irq, void *dev)
> +{
> +	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> +	struct mutex *lock = &pdata->rtc->ops_lock;
> +	unsigned int val = 0;
> +	int ret = IRQ_NONE;
> +
> +	mutex_lock(lock);
> +	regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> +
> +	/* Alarm? */
> +	if (val & CTRL2_AF) {
> +		ret = IRQ_HANDLED;
> +
> +		val &= ~(CTRL2_AF);
> +		regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> +
> +		rtc_update_irq(pdata->rtc, 1, RTC_IRQF | RTC_AF);
> +	}
> +
> +	mutex_unlock(lock);
> +
> +	return ret;
> +}
> +
>  static int pcf2123_reset(struct device *dev)
>  {
>  	struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> @@ -347,6 +448,8 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
>  	.set_time	= pcf2123_rtc_set_time,
>  	.read_offset	= pcf2123_read_offset,
>  	.set_offset	= pcf2123_set_offset,
> +	.read_alarm = pcf2123_rtc_read_alarm,
> +	.set_alarm = pcf2123_rtc_set_alarm,

Please keep that struct aligned

>  
>  };
>  
> @@ -391,6 +494,8 @@ static int pcf2123_probe(struct spi_device *spi)
>  	dev_info(&spi->dev, "spiclk %u KHz.\n",
>  			(spi->max_speed_hz + 500) / 1000);
>  
> +	device_init_wakeup(&spi->dev, true);
> +

This should be done only when you have an irq and the irq handler was
properly registered.

>  	/* Finalize the initialization */
>  	rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
>  			&pcf2123_rtc_ops, THIS_MODULE);
> @@ -418,6 +523,16 @@ static int pcf2123_probe(struct spi_device *spi)
>  		}
>  	}
>  
> +	/* Register alarm irq */
> +	if (spi->irq >= 0)	{
> +		ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
> +				pcf2123_rtc_irq,
> +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +				pcf2123_driver.driver.name, &spi->dev);
> +		if (ret)
> +			dev_err(&spi->dev, "could not request irq.\n");
> +	}
> +

As this RTC only has a minute granularity, you must set uie_unsupported
to get the UIE emulation working. You should probably run rtctest, this
would have uncovered the issue.

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

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Howey, Dylan]

Thanks for the quick response. This is actually the first time I've
submitted changes to this project.

The 04/27/2019 15:00, Alexandre Belloni wrote:
> > -static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
> > -{
> > -	u8 txbuf[2];
> > +static const struct regmap_range pcf2123_ranges[] = {
> > +	{
> > +		.range_min = PCF2123_REG_CTRL1,
> > +		.range_max = PCF2123_REG_CTDWN_TMR,
> > +	},
> > +};
> >  
> > -	txbuf[0] = reg;
> > -	txbuf[1] = val;
> > -	return pcf2123_write(dev, txbuf, sizeof(txbuf));
> > -}
> > +static const struct regmap_access_table pcf2123_access_table = {
> > +	.yes_ranges = pcf2123_ranges,
> > +	.n_yes_ranges = ARRAY_SIZE(pcf2123_ranges),
> > +};
> 
> This covers all the registers, I don't think this is necessary.
> 
This would cover the same registers that are exposed by the sysfs
interface. I can take out the timer registers since this driver does not
support the periodic timer.

I agree with the rest of your feedback. When I send a V2 patch should I
respond to this thread?


-- 
Dylan

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Alexandre Belloni]

On 29/04/2019 15:09:18+0000, Howey, Dylan wrote:
> Thanks for the quick response. This is actually the first time I've
> submitted changes to this project.
> 
> The 04/27/2019 15:00, Alexandre Belloni wrote:
> > > -static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
> > > -{
> > > -	u8 txbuf[2];
> > > +static const struct regmap_range pcf2123_ranges[] = {
> > > +	{
> > > +		.range_min = PCF2123_REG_CTRL1,
> > > +		.range_max = PCF2123_REG_CTDWN_TMR,
> > > +	},
> > > +};
> > >  
> > > -	txbuf[0] = reg;
> > > -	txbuf[1] = val;
> > > -	return pcf2123_write(dev, txbuf, sizeof(txbuf));
> > > -}
> > > +static const struct regmap_access_table pcf2123_access_table = {
> > > +	.yes_ranges = pcf2123_ranges,
> > > +	.n_yes_ranges = ARRAY_SIZE(pcf2123_ranges),
> > > +};
> > 
> > This covers all the registers, I don't think this is necessary.
> > 
> This would cover the same registers that are exposed by the sysfs
> interface. I can take out the timer registers since this driver does not
> support the periodic timer.
> 

What I meant is that you probably don't need the access table as the
whole range is already between 0 and max_register so there is no need to
double check.

> I agree with the rest of your feedback. When I send a V2 patch should I
> respond to this thread?
> 

This should be sent as a separate thred.


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

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Dylan Howey]

As I'm working on this I've run across some other issues:

* Driver does not do a software reset on init. Datasheet recommends doing
  this as this will clear any interrupts and alarm flags. The fix would
  presumably be to add a call to pcf2123_reset in the init, but...

* pcf2123_reset stops the RTC for no apparent reason. Result is that the
  time is invalid after a call to pcf2123_reset, which requires the time
  to be set again manually. The fix would be to delete the stop commands.

* I don't think pcf2123_read_offset is working correctly. In the case of
  a coarse offset the value is not being sign extended. So a negative
  offset read will not be correct if the coarse bit is set (result would
  be a positive number being returned if this is true). I need to look
  into this some more. The fix would be to sign extend first, then if
  coarse bit is set multiply the result by 2.

Not sure if I'll fix these issues right now. Also this RTC does have the 
ability to do periodic interrupts but the feature has not been implemented
in this driver. So I'll leave uie_unsupported set for now.

-- 
Dylan

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Alexandre Belloni]

On 02/05/2019 17:45:24+0000, Dylan Howey wrote:
> As I'm working on this I've run across some other issues:
> 
> * Driver does not do a software reset on init. Datasheet recommends doing
>   this as this will clear any interrupts and alarm flags. The fix would
>   presumably be to add a call to pcf2123_reset in the init, but...
> 

It recommends doing a software reset after power-on in that case, it
refers to the power-on of the RTC, not the platform. You shouldn't do a
software reset as this will break time keeping, the offset and reading
alarms that may have been starting the platform.

> * pcf2123_reset stops the RTC for no apparent reason. Result is that the
>   time is invalid after a call to pcf2123_reset, which requires the time
>   to be set again manually. The fix would be to delete the stop commands.
> 

Using the software reset will render the time invalid anyway as this
will set the OS bit in the seconds register (see Table 7.
Register reset values).

> * I don't think pcf2123_read_offset is working correctly. In the case of
>   a coarse offset the value is not being sign extended. So a negative
>   offset read will not be correct if the coarse bit is set (result would
>   be a positive number being returned if this is true). I need to look
>   into this some more. The fix would be to sign extend first, then if
>   coarse bit is set multiply the result by 2.
> 

As the comment says, it is properly extended because after shifting,
bits [6:0] become bit [7:1].

> Not sure if I'll fix these issues right now. Also this RTC does have the 
> ability to do periodic interrupts but the feature has not been implemented
> in this driver. So I'll leave uie_unsupported set for now.
> 

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

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Dylan Howey]

The 05/02/2019 22:55, Alexandre Belloni wrote:
> > * I don't think pcf2123_read_offset is working correctly. In the case of
> >   a coarse offset the value is not being sign extended. So a negative
> >   offset read will not be correct if the coarse bit is set (result would
> >   be a positive number being returned if this is true). I need to look
> >   into this some more. The fix would be to sign extend first, then if
> >   coarse bit is set multiply the result by 2.
> > 
> 
> As the comment says, it is properly extended because after shifting,
> bits [6:0] become bit [7:1].
> 
But what about this:

# echo 2170 > /sys/class/rtc/rtc0/offset 
# cat /sys/class/rtc/rtc0/offset 
2170
# echo -2170 > /sys/class/rtc/rtc0/offset 
# cat /sys/class/rtc/rtc0/offset 
0
# echo 4340 > /sys/class/rtc/rtc0/offset 
# cat /sys/class/rtc/rtc0/offset 
4340
# echo -4340 > /sys/class/rtc/rtc0/offset 
# cat /sys/class/rtc/rtc0/offset 
-2170

Negative offset reads seem to be off by a factor of 2.


-- 
Dylan

Re: [PATCH 1/2] Port rtc-pcf2123 to regmap

[Alexandre Belloni]

Sorry for the very late reply !

On 03/05/2019 13:30:12+0000, Dylan Howey wrote:
> The 05/02/2019 22:55, Alexandre Belloni wrote:
> > > * I don't think pcf2123_read_offset is working correctly. In the case of
> > >   a coarse offset the value is not being sign extended. So a negative
> > >   offset read will not be correct if the coarse bit is set (result would
> > >   be a positive number being returned if this is true). I need to look
> > >   into this some more. The fix would be to sign extend first, then if
> > >   coarse bit is set multiply the result by 2.
> > > 
> > 
> > As the comment says, it is properly extended because after shifting,
> > bits [6:0] become bit [7:1].
> > 
> But what about this:
> 
> # echo 2170 > /sys/class/rtc/rtc0/offset 
> # cat /sys/class/rtc/rtc0/offset 
> 2170
> # echo -2170 > /sys/class/rtc/rtc0/offset 
> # cat /sys/class/rtc/rtc0/offset 
> 0
> # echo 4340 > /sys/class/rtc/rtc0/offset 
> # cat /sys/class/rtc/rtc0/offset 
> 4340
> # echo -4340 > /sys/class/rtc/rtc0/offset 
> # cat /sys/class/rtc/rtc0/offset 
> -2170
> 
> Negative offset reads seem to be off by a factor of 2.
> 

The issue is not the sign extension, it is the rounding function that
only works properly for positive numbers:

reg = (s8)((offset + (OFFSET_STEP >> 1)) / OFFSET_STEP);

I'm sending a patch.

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