[PATCH v4] rtc: isl12026: Add driver.

[PATCH v4] rtc: isl12026: Add driver.

[David Daney]

The ISL12026 is a combination RTC and EEPROM device with I2C
interface.  The standard RTC driver interface is provided.  The EEPROM
is accessed via the NVMEM interface via the "eeprom0" directory in the
sysfs entry for the device.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
Changes from v3:

o Add Reviewed-by

o s/dev_err/dev_warn/ in one place

o Remove redundant ','

Changes from v2:

o More code cleanups suggested by reviewers.

Changes from v1:

o Fixed device tree bindings document example.

o Use RTC_NVMEM facility for eeprom support.

o Small code cleanups suggested by reviewers.

 .../devicetree/bindings/rtc/isil,isl12026.txt      |  28 ++
 drivers/rtc/Kconfig                                |   9 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-isl12026.c                         | 529 +++++++++++++++++++++
 4 files changed, 567 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12026.txt
 create mode 100644 drivers/rtc/rtc-isl12026.c

diff --git a/Documentation/devicetree/bindings/rtc/isil,isl12026.txt b/Documentation/devicetree/bindings/rtc/isil,isl12026.txt
new file mode 100644
index 000000000000..2e0be45193bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/isil,isl12026.txt
@@ -0,0 +1,28 @@
+ISL12026 I2C RTC/EEPROM
+
+ISL12026 is an I2C RTC/EEPROM combination device.  The RTC and control
+registers respond at bus address 0x6f, and the EEPROM array responds
+at bus address 0x57.  The canonical "reg" value will be for the RTC portion.
+
+Required properties supported by the device:
+
+ - "compatible": must be "isil,isl12026"
+ - "reg": I2C bus address of the device (always 0x6f)
+
+Optional properties:
+
+ - "isil,pwr-bsw": If present PWR.BSW bit must be set to the specified
+                   value for proper operation.
+
+ - "isil,pwr-sbib": If present PWR.SBIB bit must be set to the specified
+                    value for proper operation.
+
+
+Example:
+
+	rtc@6f {
+		compatible = "isil,isl12026";
+		reg = <0x6f>;
+		isil,pwr-bsw = <0>;
+		isil,pwr-sbib = <1>;
+	}
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8ab5f0a5d323..85171e9e3ada 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -407,6 +407,15 @@ config RTC_DRV_ISL12022
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-isl12022.
 
+config RTC_DRV_ISL12026
+	tristate "Intersil ISL12026"
+	help
+	  If you say yes here you get support for the
+	  Intersil ISL12026 RTC chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-isl12026.
+
 config RTC_DRV_X1205
 	tristate "Xicor/Intersil X1205"
 	help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 4fbf87e45a7c..f481661a6eae 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o
 obj-$(CONFIG_RTC_DRV_HYM8563)	+= rtc-hym8563.o
 obj-$(CONFIG_RTC_DRV_IMXDI)	+= rtc-imxdi.o
 obj-$(CONFIG_RTC_DRV_ISL12022)	+= rtc-isl12022.o
+obj-$(CONFIG_RTC_DRV_ISL12026)	+= rtc-isl12026.o
 obj-$(CONFIG_RTC_DRV_ISL1208)	+= rtc-isl1208.o
 obj-$(CONFIG_RTC_DRV_JZ4740)	+= rtc-jz4740.o
 obj-$(CONFIG_RTC_DRV_LP8788)	+= rtc-lp8788.o
diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
new file mode 100644
index 000000000000..29e5bdf96c67
--- /dev/null
+++ b/drivers/rtc/rtc-isl12026.c
@@ -0,0 +1,529 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * An I2C driver for the Intersil ISL 12026
+ *
+ * Copyright (c) 2018 Cavium, Inc.
+ */
+#include <linux/bcd.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+
+/* register offsets */
+#define ISL12026_REG_PWR	0x14
+# define ISL12026_REG_PWR_BSW	BIT(6)
+# define ISL12026_REG_PWR_SBIB	BIT(7)
+#define ISL12026_REG_SC		0x30
+#define ISL12026_REG_HR		0x32
+# define ISL12026_REG_HR_MIL	BIT(7)	/* military or 24 hour time */
+#define ISL12026_REG_SR		0x3f
+# define ISL12026_REG_SR_RTCF	BIT(0)
+# define ISL12026_REG_SR_WEL	BIT(1)
+# define ISL12026_REG_SR_RWEL	BIT(2)
+# define ISL12026_REG_SR_MBZ	BIT(3)
+# define ISL12026_REG_SR_OSCF	BIT(4)
+
+/* The EEPROM array responds at i2c address 0x57 */
+#define ISL12026_EEPROM_ADDR	0x57
+
+#define ISL12026_PAGESIZE 16
+#define ISL12026_NVMEM_WRITE_TIME 20
+
+struct isl12026 {
+	struct rtc_device *rtc;
+	struct i2c_client *nvm_client;
+	struct nvmem_config nvm_cfg;
+	/*
+	 * RTC write operations require that multiple messages be
+	 * transmitted, we hold the lock for all accesses to the
+	 * device so that these sequences cannot be disrupted.  Also,
+	 * the write cycle to the nvmem takes many ms during which the
+	 * device does not respond to commands, so holding the lock
+	 * also prevents access during these times.
+	 */
+	struct mutex lock;
+};
+
+static int isl12026_read_reg(struct i2c_client *client, int reg)
+{
+	struct isl12026 *priv = i2c_get_clientdata(client);
+	u8 addr[] = {0, reg};
+	u8 val;
+	int ret;
+
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags	= 0,
+			.len	= sizeof(addr),
+			.buf	= addr
+		}, {
+			.addr	= client->addr,
+			.flags	= I2C_M_RD,
+			.len	= 1,
+			.buf	= &val
+		}
+	};
+
+	mutex_lock(&priv->lock);
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs)) {
+		dev_err(&client->dev, "read reg error, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+	} else {
+		ret = val;
+	}
+
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
+{
+	struct isl12026 *priv = i2c_get_clientdata(client);
+	int ret;
+	u8 op[3];
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= 1,
+		.buf	= op
+	};
+
+	mutex_lock(&priv->lock);
+
+	/* Set SR.WEL */
+	op[0] = 0;
+	op[1] = ISL12026_REG_SR;
+	op[2] = ISL12026_REG_SR_WEL;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	/* Set SR.WEL and SR.RWEL */
+	op[2] = ISL12026_REG_SR_WEL | ISL12026_REG_SR_RWEL;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev,
+			"write error SR.WEL|SR.RWEL, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	op[1] = reg;
+	op[2] = val;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error CCR, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	msleep(ISL12026_NVMEM_WRITE_TIME);
+
+	/* Clear SR.WEL and SR.RWEL */
+	op[1] = ISL12026_REG_SR;
+	op[2] = 0;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error SR, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+	ret = 0;
+out:
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int isl12026_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct isl12026 *priv = i2c_get_clientdata(client);
+	int ret;
+	u8 op[10];
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= 1,
+		.buf	= op
+	};
+
+	mutex_lock(&priv->lock);
+
+	/* Set SR.WEL */
+	op[0] = 0;
+	op[1] = ISL12026_REG_SR;
+	op[2] = ISL12026_REG_SR_WEL;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	/* Set SR.WEL and SR.RWEL */
+	op[2] = ISL12026_REG_SR_WEL | ISL12026_REG_SR_RWEL;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev,
+			"write error SR.WEL|SR.RWEL, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	/* Set the CCR registers */
+	op[1] = ISL12026_REG_SC;
+	op[2] = bin2bcd(tm->tm_sec); /* SC */
+	op[3] = bin2bcd(tm->tm_min); /* MN */
+	op[4] = bin2bcd(tm->tm_hour) | ISL12026_REG_HR_MIL; /* HR */
+	op[5] = bin2bcd(tm->tm_mday); /* DT */
+	op[6] = bin2bcd(tm->tm_mon + 1); /* MO */
+	op[7] = bin2bcd(tm->tm_year % 100); /* YR */
+	op[8] = bin2bcd(tm->tm_wday & 7); /* DW */
+	op[9] = bin2bcd(tm->tm_year >= 100 ? 20 : 19); /* Y2K */
+	msg.len = 10;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error CCR, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	/* Clear SR.WEL and SR.RWEL */
+	op[1] = ISL12026_REG_SR;
+	op[2] = 0;
+	msg.len = 3;
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "write error SR, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+	ret = 0;
+out:
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct isl12026 *priv = i2c_get_clientdata(client);
+	u8 ccr[8];
+	u8 addr[2];
+	u8 sr;
+	int ret;
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags	= 0,
+			.len	= sizeof(addr),
+			.buf	= addr
+		}, {
+			.addr	= client->addr,
+			.flags	= I2C_M_RD,
+		}
+	};
+
+	mutex_lock(&priv->lock);
+
+	/* First, read SR */
+	addr[0] = 0;
+	addr[1] = ISL12026_REG_SR;
+	msgs[1].len = 1;
+	msgs[1].buf = &sr;
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs)) {
+		dev_err(&client->dev, "read error, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	if (sr & ISL12026_REG_SR_RTCF)
+		dev_warn(&client->dev, "Real-Time Clock Failure on read\n");
+	if (sr & ISL12026_REG_SR_OSCF)
+		dev_warn(&client->dev, "Oscillator Failure on read\n");
+
+	/* Second, CCR regs */
+	addr[0] = 0;
+	addr[1] = ISL12026_REG_SC;
+	msgs[1].len = sizeof(ccr);
+	msgs[1].buf = ccr;
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs)) {
+		dev_err(&client->dev, "read error, ret=%d\n", ret);
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	tm->tm_sec = bcd2bin(ccr[0] & 0x7F);
+	tm->tm_min = bcd2bin(ccr[1] & 0x7F);
+	if (ccr[2] & ISL12026_REG_HR_MIL)
+		tm->tm_hour = bcd2bin(ccr[2] & 0x3F);
+	else
+		tm->tm_hour = bcd2bin(ccr[2] & 0x1F) +
+			((ccr[2] & 0x20) ? 12 : 0);
+	tm->tm_mday = bcd2bin(ccr[3] & 0x3F);
+	tm->tm_mon = bcd2bin(ccr[4] & 0x1F) - 1;
+	tm->tm_year = bcd2bin(ccr[5]);
+	if (bcd2bin(ccr[7]) == 20)
+		tm->tm_year += 100;
+	tm->tm_wday = ccr[6] & 0x07;
+
+	ret = rtc_valid_tm(tm);
+out:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static const struct rtc_class_ops isl12026_rtc_ops = {
+	.read_time	= isl12026_rtc_read_time,
+	.set_time	= isl12026_rtc_set_time,
+};
+
+static int isl12026_nvm_read(void *p, unsigned int offset,
+			     void *val, size_t bytes)
+{
+	struct isl12026 *priv = p;
+	int ret;
+	u8 addr[2];
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= priv->nvm_client->addr,
+			.flags	= 0,
+			.len	= sizeof(addr),
+			.buf	= addr
+		}, {
+			.addr	= priv->nvm_client->addr,
+			.flags	= I2C_M_RD,
+			.buf	= val
+		}
+	};
+
+	if (offset >= priv->nvm_cfg.size)
+		return 0; /* End-of-file */
+	if (offset + bytes > priv->nvm_cfg.size)
+		bytes = priv->nvm_cfg.size - offset;
+
+	mutex_lock(&priv->lock);
+
+	/* 2 bytes of address, most significant first */
+	addr[0] = offset >> 8;
+	addr[1] = offset;
+	msgs[1].len = bytes;
+	ret = i2c_transfer(priv->nvm_client->adapter, msgs, ARRAY_SIZE(msgs));
+
+	mutex_unlock(&priv->lock);
+
+	if (ret != ARRAY_SIZE(msgs)) {
+		dev_err(priv->nvm_cfg.dev, "nvmem read error, ret=%d\n", ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return bytes;
+}
+
+static int isl12026_nvm_write(void *p, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct isl12026 *priv = p;
+	int ret = -EIO;
+	u8 *v = val;
+	size_t chunk_size, num_written;
+	u8 payload[ISL12026_PAGESIZE + 2]; /* page + 2 address bytes */
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= priv->nvm_client->addr,
+			.flags	= 0,
+			.buf	= payload
+		}
+	};
+
+	if (offset >= priv->nvm_cfg.size)
+		return 0; /* End-of-file */
+	if (offset + bytes > priv->nvm_cfg.size)
+		bytes = priv->nvm_cfg.size - offset;
+
+	mutex_lock(&priv->lock);
+
+	num_written = 0;
+	while (bytes) {
+		chunk_size = round_down(offset, ISL12026_PAGESIZE) +
+			ISL12026_PAGESIZE - offset;
+		chunk_size = min(bytes, chunk_size);
+		/*
+		 * 2 bytes of address, most significant first, followed
+		 * by page data bytes
+		 */
+		memcpy(payload + 2, v + num_written, chunk_size);
+		payload[0] = offset >> 8;
+		payload[1] = offset;
+		msgs[0].len = chunk_size + 2;
+		ret = i2c_transfer(priv->nvm_client->adapter,
+				   msgs, ARRAY_SIZE(msgs));
+		if (ret != ARRAY_SIZE(msgs)) {
+			dev_err(priv->nvm_cfg.dev,
+				"nvmem write error, ret=%d\n", ret);
+			ret = ret < 0 ? ret : -EIO;
+			break;
+		}
+		bytes -= chunk_size;
+		offset += chunk_size;
+		num_written += chunk_size;
+		msleep(ISL12026_NVMEM_WRITE_TIME);
+	}
+
+	mutex_unlock(&priv->lock);
+
+	return num_written >= 0 ? num_written : ret;
+}
+
+static void isl12026_force_power_modes(struct i2c_client *client)
+{
+	int ret;
+	int pwr, requested_pwr;
+	u32 bsw_val, sbib_val;
+	bool set_bsw, set_sbib;
+
+	/*
+	 * If we can read the of_property, set the specified value.
+	 * If there is an error reading the of_property (likely
+	 * because it does not exist), keep the current value.
+	 */
+	ret = of_property_read_u32(client->dev.of_node,
+				   "isil,pwr-bsw", &bsw_val);
+	set_bsw = (ret == 0);
+
+	ret = of_property_read_u32(client->dev.of_node,
+				   "isil,pwr-sbib", &sbib_val);
+	set_sbib = (ret == 0);
+
+	/* Check if PWR.BSW and/or PWR.SBIB need specified values */
+	if (!set_bsw && !set_sbib)
+		return;
+
+	pwr = isl12026_read_reg(client, ISL12026_REG_PWR);
+	if (pwr < 0) {
+		dev_warn(&client->dev, "Error: Failed to read PWR %d\n", pwr);
+		return;
+	}
+
+	requested_pwr = pwr;
+
+	if (set_bsw) {
+		if (bsw_val)
+			requested_pwr |= ISL12026_REG_PWR_BSW;
+		else
+			requested_pwr &= ~ISL12026_REG_PWR_BSW;
+	} /* else keep current BSW */
+
+	if (set_sbib) {
+		if (sbib_val)
+			requested_pwr |= ISL12026_REG_PWR_SBIB;
+		else
+			requested_pwr &= ~ISL12026_REG_PWR_SBIB;
+	} /* else keep current SBIB */
+
+	if (pwr >= 0 && pwr != requested_pwr) {
+		dev_info(&client->dev, "PWR: %02x\n", pwr);
+		dev_info(&client->dev,
+			 "Updating PWR to: %02x\n", requested_pwr);
+		isl12026_write_reg(client, ISL12026_REG_PWR, requested_pwr);
+	}
+}
+
+static int isl12026_probe_new(struct i2c_client *client)
+{
+	struct isl12026 *priv;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->lock);
+
+	i2c_set_clientdata(client, priv);
+
+	isl12026_force_power_modes(client);
+
+	priv->nvm_client = i2c_new_dummy(client->adapter, ISL12026_EEPROM_ADDR);
+	if (!priv->nvm_client)
+		return -ENOMEM;
+
+	priv->rtc = devm_rtc_allocate_device(&client->dev);
+	ret = PTR_ERR_OR_ZERO(priv->rtc);
+	if (ret)
+		return ret;
+
+	priv->rtc->ops = &isl12026_rtc_ops;
+
+	priv->nvm_cfg.name = "eeprom";
+	priv->nvm_cfg.read_only = false;
+	priv->nvm_cfg.root_only = true;
+	priv->nvm_cfg.base_dev = &client->dev;
+	priv->nvm_cfg.priv = priv;
+	priv->nvm_cfg.stride = 1;
+	priv->nvm_cfg.word_size = 1;
+	priv->nvm_cfg.size = 512;
+	priv->nvm_cfg.reg_read = isl12026_nvm_read;
+	priv->nvm_cfg.reg_write = isl12026_nvm_write;
+
+	priv->rtc->nvmem_config = &priv->nvm_cfg;
+	priv->rtc->nvram_old_abi = false;
+
+	return rtc_register_device(priv->rtc);
+}
+
+static int isl12026_remove(struct i2c_client *client)
+{
+	struct isl12026 *priv = i2c_get_clientdata(client);
+
+	i2c_unregister_device(priv->nvm_client);
+	return 0;
+}
+
+static const struct of_device_id isl12026_dt_match[] = {
+	{ .compatible = "isil,isl12026" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, isl12026_dt_match);
+
+static struct i2c_driver isl12026_driver = {
+	.driver		= {
+		.name	= "rtc-isl12026",
+		.of_match_table = of_match_ptr(isl12026_dt_match),
+	},
+	.probe_new	= isl12026_probe_new,
+	.remove		= isl12026_remove,
+};
+
+module_i2c_driver(isl12026_driver);
+
+MODULE_DESCRIPTION("ISL 12026 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.14.3

Re: [PATCH v4] rtc: isl12026: Add driver.

[Andy Shevchenko]

On Fri, Feb 16, 2018 at 9:44 PM, David Daney <david.daney@cavium.com> wrote:
> The ISL12026 is a combination RTC and EEPROM device with I2C
> interface.  The standard RTC driver interface is provided.  The EEPROM
> is accessed via the NVMEM interface via the "eeprom0" directory in the
> sysfs entry for the device.

> +config RTC_DRV_ISL12026
> +       tristate "Intersil ISL12026"

depends on OF

> +static struct i2c_driver isl12026_driver = {
> +       .driver         = {
> +               .name   = "rtc-isl12026",
> +               .of_match_table = of_match_ptr(isl12026_dt_match),

/of_match_ptr//

> +       },
> +       .probe_new      = isl12026_probe_new,
> +       .remove         = isl12026_remove,
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] rtc: isl12026: Add driver.

[David Daney]

On 02/16/2018 12:13 PM, Andy Shevchenko wrote:
> On Fri, Feb 16, 2018 at 9:44 PM, David Daney <david.daney@cavium.com> wrote:
>> The ISL12026 is a combination RTC and EEPROM device with I2C
>> interface.  The standard RTC driver interface is provided.  The EEPROM
>> is accessed via the NVMEM interface via the "eeprom0" directory in the
>> sysfs entry for the device.
> 
>> +config RTC_DRV_ISL12026
>> +       tristate "Intersil ISL12026"
> 
> depends on OF

It doesn't depend on CONFIG_OF, it builds just fine without it.

> 
>> +static struct i2c_driver isl12026_driver = {
>> +       .driver         = {
>> +               .name   = "rtc-isl12026",
>> +               .of_match_table = of_match_ptr(isl12026_dt_match),
> 
> /of_match_ptr//


of_match_ptr() needed if we build without CONFIG_OF

> 
>> +       },
>> +       .probe_new      = isl12026_probe_new,
>> +       .remove         = isl12026_remove,
>> +};
> 

Re: [PATCH v4] rtc: isl12026: Add driver.

[Andy Shevchenko]

On Fri, Feb 16, 2018 at 11:19 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 02/16/2018 12:13 PM, Andy Shevchenko wrote:
>> On Fri, Feb 16, 2018 at 9:44 PM, David Daney <david.daney@cavium.com>
>> wrote:

>>> +config RTC_DRV_ISL12026
>>> +       tristate "Intersil ISL12026"

>> depends on OF

> It doesn't depend on CONFIG_OF, it builds just fine without it.

OK.
But then it's useless.

depends on OF || COMPILE_TEST

>>> +static struct i2c_driver isl12026_driver = {
>>> +       .driver         = {
>>> +               .name   = "rtc-isl12026",
>>> +               .of_match_table = of_match_ptr(isl12026_dt_match),

>> /of_match_ptr//

> of_match_ptr() needed if we build without CONFIG_OF

OK.
Have you tried to enable warnings?
You must get `defined but not used` one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] rtc: isl12026: Add driver.

[Philippe Ombredanne]

Hi David,

On Fri, Feb 16, 2018 at 8:44 PM, David Daney <david.daney@cavium.com> wrote:
> The ISL12026 is a combination RTC and EEPROM device with I2C
> interface.  The standard RTC driver interface is provided.  The EEPROM
> is accessed via the NVMEM interface via the "eeprom0" directory in the
> sysfs entry for the device.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: David Daney <david.daney@cavium.com>

<snip>

> --- /dev/null
> +++ b/drivers/rtc/rtc-isl12026.c
> @@ -0,0 +1,529 @@
> +// SPDX-License-Identifier: GPL-2.0

<snip>

> +MODULE_LICENSE("GPL");

Your MODULE_LICENSE does not match your SPDX tag.
Per module.h, GPL would mean GPL-2.0+ not GPL-2.0
It would be best if you can sync the two.

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v4] rtc: isl12026: Add driver.

[Rob Herring]

On Fri, Feb 16, 2018 at 11:44:15AM -0800, David Daney wrote:
> The ISL12026 is a combination RTC and EEPROM device with I2C
> interface.  The standard RTC driver interface is provided.  The EEPROM
> is accessed via the NVMEM interface via the "eeprom0" directory in the
> sysfs entry for the device.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
> Changes from v3:
> 
> o Add Reviewed-by
> 
> o s/dev_err/dev_warn/ in one place
> 
> o Remove redundant ','
> 
> Changes from v2:
> 
> o More code cleanups suggested by reviewers.
> 
> Changes from v1:
> 
> o Fixed device tree bindings document example.
> 
> o Use RTC_NVMEM facility for eeprom support.
> 
> o Small code cleanups suggested by reviewers.
> 
>  .../devicetree/bindings/rtc/isil,isl12026.txt      |  28 ++

Reviewed-by: Rob Herring <robh@kernel.org>

>  drivers/rtc/Kconfig                                |   9 +
>  drivers/rtc/Makefile                               |   1 +
>  drivers/rtc/rtc-isl12026.c                         | 529 +++++++++++++++++++++
>  4 files changed, 567 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12026.txt
>  create mode 100644 drivers/rtc/rtc-isl12026.c

Re: [PATCH v4] rtc: isl12026: Add driver.

[Alexandre Belloni]

Hi,

On 16/02/2018 at 11:44:15 -0800, David Daney wrote:
> The ISL12026 is a combination RTC and EEPROM device with I2C
> interface.  The standard RTC driver interface is provided.  The EEPROM
> is accessed via the NVMEM interface via the "eeprom0" directory in the
> sysfs entry for the device.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
> Changes from v3:
> 
> o Add Reviewed-by
> 
> o s/dev_err/dev_warn/ in one place
> 
> o Remove redundant ','
> 
> Changes from v2:
> 
> o More code cleanups suggested by reviewers.
> 
> Changes from v1:
> 
> o Fixed device tree bindings document example.
> 
> o Use RTC_NVMEM facility for eeprom support.
> 
> o Small code cleanups suggested by reviewers.
> 
>  .../devicetree/bindings/rtc/isil,isl12026.txt      |  28 ++
>  drivers/rtc/Kconfig                                |   9 +
>  drivers/rtc/Makefile                               |   1 +
>  drivers/rtc/rtc-isl12026.c                         | 529 +++++++++++++++++++++
>  4 files changed, 567 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12026.txt
>  create mode 100644 drivers/rtc/rtc-isl12026.c
> 
> diff --git a/Documentation/devicetree/bindings/rtc/isil,isl12026.txt b/Documentation/devicetree/bindings/rtc/isil,isl12026.txt
> new file mode 100644
> index 000000000000..2e0be45193bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl12026.txt
> @@ -0,0 +1,28 @@
> +ISL12026 I2C RTC/EEPROM
> +
> +ISL12026 is an I2C RTC/EEPROM combination device.  The RTC and control
> +registers respond at bus address 0x6f, and the EEPROM array responds
> +at bus address 0x57.  The canonical "reg" value will be for the RTC portion.
> +
> +Required properties supported by the device:
> +
> + - "compatible": must be "isil,isl12026"
> + - "reg": I2C bus address of the device (always 0x6f)
> +
> +Optional properties:
> +
> + - "isil,pwr-bsw": If present PWR.BSW bit must be set to the specified
> +                   value for proper operation.
> +
> + - "isil,pwr-sbib": If present PWR.SBIB bit must be set to the specified
> +                    value for proper operation.
> +
> +
> +Example:
> +
> +	rtc@6f {
> +		compatible = "isil,isl12026";
> +		reg = <0x6f>;
> +		isil,pwr-bsw = <0>;
> +		isil,pwr-sbib = <1>;
> +	}
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 8ab5f0a5d323..85171e9e3ada 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -407,6 +407,15 @@ config RTC_DRV_ISL12022
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-isl12022.
>  
> +config RTC_DRV_ISL12026
> +	tristate "Intersil ISL12026"
> +	help
> +	  If you say yes here you get support for the
> +	  Intersil ISL12026 RTC chip.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-isl12026.
> +
>  config RTC_DRV_X1205
>  	tristate "Xicor/Intersil X1205"
>  	help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 4fbf87e45a7c..f481661a6eae 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o
>  obj-$(CONFIG_RTC_DRV_HYM8563)	+= rtc-hym8563.o
>  obj-$(CONFIG_RTC_DRV_IMXDI)	+= rtc-imxdi.o
>  obj-$(CONFIG_RTC_DRV_ISL12022)	+= rtc-isl12022.o
> +obj-$(CONFIG_RTC_DRV_ISL12026)	+= rtc-isl12026.o
>  obj-$(CONFIG_RTC_DRV_ISL1208)	+= rtc-isl1208.o
>  obj-$(CONFIG_RTC_DRV_JZ4740)	+= rtc-jz4740.o
>  obj-$(CONFIG_RTC_DRV_LP8788)	+= rtc-lp8788.o
> diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> new file mode 100644
> index 000000000000..29e5bdf96c67
> --- /dev/null
> +++ b/drivers/rtc/rtc-isl12026.c
> @@ -0,0 +1,529 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * An I2C driver for the Intersil ISL 12026
> + *
> + * Copyright (c) 2018 Cavium, Inc.
> + */
> +#include <linux/bcd.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/rtc.h>
> +#include <linux/slab.h>
> +
> +/* register offsets */
> +#define ISL12026_REG_PWR	0x14
> +# define ISL12026_REG_PWR_BSW	BIT(6)
> +# define ISL12026_REG_PWR_SBIB	BIT(7)
> +#define ISL12026_REG_SC		0x30
> +#define ISL12026_REG_HR		0x32
> +# define ISL12026_REG_HR_MIL	BIT(7)	/* military or 24 hour time */
> +#define ISL12026_REG_SR		0x3f
> +# define ISL12026_REG_SR_RTCF	BIT(0)
> +# define ISL12026_REG_SR_WEL	BIT(1)
> +# define ISL12026_REG_SR_RWEL	BIT(2)
> +# define ISL12026_REG_SR_MBZ	BIT(3)
> +# define ISL12026_REG_SR_OSCF	BIT(4)
> +
> +/* The EEPROM array responds at i2c address 0x57 */
> +#define ISL12026_EEPROM_ADDR	0x57
> +
> +#define ISL12026_PAGESIZE 16
> +#define ISL12026_NVMEM_WRITE_TIME 20
> +
> +struct isl12026 {
> +	struct rtc_device *rtc;
> +	struct i2c_client *nvm_client;
> +	struct nvmem_config nvm_cfg;
> +	/*
> +	 * RTC write operations require that multiple messages be
> +	 * transmitted, we hold the lock for all accesses to the
> +	 * device so that these sequences cannot be disrupted.  Also,
> +	 * the write cycle to the nvmem takes many ms during which the
> +	 * device does not respond to commands, so holding the lock
> +	 * also prevents access during these times.
> +	 */
> +	struct mutex lock;
> +};
> +
> +static int isl12026_read_reg(struct i2c_client *client, int reg)
> +{
> +	struct isl12026 *priv = i2c_get_clientdata(client);
> +	u8 addr[] = {0, reg};
> +	u8 val;
> +	int ret;
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= sizeof(addr),
> +			.buf	= addr
> +		}, {
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= 1,
> +			.buf	= &val
> +		}
> +	};

I'm pretty sure you can use regmap instead of open coding all the i2c
transfers, did you try?

> +
> +	mutex_lock(&priv->lock);
> +

Also, regmap will remove the need for that lock.

> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret != ARRAY_SIZE(msgs)) {
> +		dev_err(&client->dev, "read reg error, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +	} else {
> +		ret = val;
> +	}
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
> +{
> +	struct isl12026 *priv = i2c_get_clientdata(client);
> +	int ret;
> +	u8 op[3];
> +	struct i2c_msg msg = {
> +		.addr	= client->addr,
> +		.flags	= 0,
> +		.len	= 1,
> +		.buf	= op
> +	};
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* Set SR.WEL */
> +	op[0] = 0;
> +	op[1] = ISL12026_REG_SR;
> +	op[2] = ISL12026_REG_SR_WEL;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}

If you don't clear SR.WEL, I don't think you need to set it each time
you write to the RTC. I would just set SR.WEL at probe time and let it
there. That removes two i2c writes for each write operation.

> +
> +	/* Set SR.WEL and SR.RWEL */
> +	op[2] = ISL12026_REG_SR_WEL | ISL12026_REG_SR_RWEL;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev,
> +			"write error SR.WEL|SR.RWEL, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	op[1] = reg;
> +	op[2] = val;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error CCR, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	msleep(ISL12026_NVMEM_WRITE_TIME);
> +
> +	/* Clear SR.WEL and SR.RWEL */
> +	op[1] = ISL12026_REG_SR;
> +	op[2] = 0;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error SR, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +	ret = 0;
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int isl12026_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct isl12026 *priv = i2c_get_clientdata(client);
> +	int ret;
> +	u8 op[10];
> +	struct i2c_msg msg = {
> +		.addr	= client->addr,
> +		.flags	= 0,
> +		.len	= 1,
> +		.buf	= op
> +	};
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* Set SR.WEL */
> +	op[0] = 0;
> +	op[1] = ISL12026_REG_SR;
> +	op[2] = ISL12026_REG_SR_WEL;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	/* Set SR.WEL and SR.RWEL */
> +	op[2] = ISL12026_REG_SR_WEL | ISL12026_REG_SR_RWEL;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev,
> +			"write error SR.WEL|SR.RWEL, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	/* Set the CCR registers */
> +	op[1] = ISL12026_REG_SC;
> +	op[2] = bin2bcd(tm->tm_sec); /* SC */
> +	op[3] = bin2bcd(tm->tm_min); /* MN */
> +	op[4] = bin2bcd(tm->tm_hour) | ISL12026_REG_HR_MIL; /* HR */
> +	op[5] = bin2bcd(tm->tm_mday); /* DT */
> +	op[6] = bin2bcd(tm->tm_mon + 1); /* MO */
> +	op[7] = bin2bcd(tm->tm_year % 100); /* YR */
> +	op[8] = bin2bcd(tm->tm_wday & 7); /* DW */
> +	op[9] = bin2bcd(tm->tm_year >= 100 ? 20 : 19); /* Y2K */
> +	msg.len = 10;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error CCR, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	/* Clear SR.WEL and SR.RWEL */
> +	op[1] = ISL12026_REG_SR;
> +	op[2] = 0;
> +	msg.len = 3;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "write error SR, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +	ret = 0;
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct isl12026 *priv = i2c_get_clientdata(client);
> +	u8 ccr[8];
> +	u8 addr[2];
> +	u8 sr;
> +	int ret;
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= sizeof(addr),
> +			.buf	= addr
> +		}, {
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +		}
> +	};
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* First, read SR */
> +	addr[0] = 0;
> +	addr[1] = ISL12026_REG_SR;
> +	msgs[1].len = 1;
> +	msgs[1].buf = &sr;
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret != ARRAY_SIZE(msgs)) {
> +		dev_err(&client->dev, "read error, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	if (sr & ISL12026_REG_SR_RTCF)
> +		dev_warn(&client->dev, "Real-Time Clock Failure on read\n");
> +	if (sr & ISL12026_REG_SR_OSCF)
> +		dev_warn(&client->dev, "Oscillator Failure on read\n");
> +
> +	/* Second, CCR regs */
> +	addr[0] = 0;
> +	addr[1] = ISL12026_REG_SC;
> +	msgs[1].len = sizeof(ccr);
> +	msgs[1].buf = ccr;
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret != ARRAY_SIZE(msgs)) {
> +		dev_err(&client->dev, "read error, ret=%d\n", ret);
> +		ret = ret < 0 ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	tm->tm_sec = bcd2bin(ccr[0] & 0x7F);
> +	tm->tm_min = bcd2bin(ccr[1] & 0x7F);
> +	if (ccr[2] & ISL12026_REG_HR_MIL)
> +		tm->tm_hour = bcd2bin(ccr[2] & 0x3F);
> +	else
> +		tm->tm_hour = bcd2bin(ccr[2] & 0x1F) +
> +			((ccr[2] & 0x20) ? 12 : 0);
> +	tm->tm_mday = bcd2bin(ccr[3] & 0x3F);
> +	tm->tm_mon = bcd2bin(ccr[4] & 0x1F) - 1;
> +	tm->tm_year = bcd2bin(ccr[5]);
> +	if (bcd2bin(ccr[7]) == 20)
> +		tm->tm_year += 100;
> +	tm->tm_wday = ccr[6] & 0x07;
> +
> +	ret = rtc_valid_tm(tm);

This rtc_valid_tm is unnecessary, you can simply return 0.

> +out:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static const struct rtc_class_ops isl12026_rtc_ops = {
> +	.read_time	= isl12026_rtc_read_time,
> +	.set_time	= isl12026_rtc_set_time,
> +};
> +
> +static int isl12026_nvm_read(void *p, unsigned int offset,
> +			     void *val, size_t bytes)
> +{
> +	struct isl12026 *priv = p;
> +	int ret;
> +	u8 addr[2];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= priv->nvm_client->addr,
> +			.flags	= 0,
> +			.len	= sizeof(addr),
> +			.buf	= addr
> +		}, {
> +			.addr	= priv->nvm_client->addr,
> +			.flags	= I2C_M_RD,
> +			.buf	= val
> +		}
> +	};
> +
> +	if (offset >= priv->nvm_cfg.size)
> +		return 0; /* End-of-file */
> +	if (offset + bytes > priv->nvm_cfg.size)
> +		bytes = priv->nvm_cfg.size - offset;
> +
> +	mutex_lock(&priv->lock);

You can completely remove the need for that lock by taking
priv->rtc->ops_lock here.

> +
> +	/* 2 bytes of address, most significant first */
> +	addr[0] = offset >> 8;
> +	addr[1] = offset;
> +	msgs[1].len = bytes;
> +	ret = i2c_transfer(priv->nvm_client->adapter, msgs, ARRAY_SIZE(msgs));
> +
> +	mutex_unlock(&priv->lock);
> +
> +	if (ret != ARRAY_SIZE(msgs)) {
> +		dev_err(priv->nvm_cfg.dev, "nvmem read error, ret=%d\n", ret);
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	return bytes;
> +}
> +
> +static int isl12026_nvm_write(void *p, unsigned int offset,
> +			      void *val, size_t bytes)
> +{
> +	struct isl12026 *priv = p;
> +	int ret = -EIO;
> +	u8 *v = val;
> +	size_t chunk_size, num_written;
> +	u8 payload[ISL12026_PAGESIZE + 2]; /* page + 2 address bytes */
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= priv->nvm_client->addr,
> +			.flags	= 0,
> +			.buf	= payload
> +		}
> +	};
> +
> +	if (offset >= priv->nvm_cfg.size)
> +		return 0; /* End-of-file */
> +	if (offset + bytes > priv->nvm_cfg.size)
> +		bytes = priv->nvm_cfg.size - offset;
> +
> +	mutex_lock(&priv->lock);
> +
> +	num_written = 0;
> +	while (bytes) {
> +		chunk_size = round_down(offset, ISL12026_PAGESIZE) +
> +			ISL12026_PAGESIZE - offset;
> +		chunk_size = min(bytes, chunk_size);
> +		/*
> +		 * 2 bytes of address, most significant first, followed
> +		 * by page data bytes
> +		 */
> +		memcpy(payload + 2, v + num_written, chunk_size);
> +		payload[0] = offset >> 8;
> +		payload[1] = offset;
> +		msgs[0].len = chunk_size + 2;
> +		ret = i2c_transfer(priv->nvm_client->adapter,
> +				   msgs, ARRAY_SIZE(msgs));
> +		if (ret != ARRAY_SIZE(msgs)) {
> +			dev_err(priv->nvm_cfg.dev,
> +				"nvmem write error, ret=%d\n", ret);
> +			ret = ret < 0 ? ret : -EIO;
> +			break;
> +		}
> +		bytes -= chunk_size;
> +		offset += chunk_size;
> +		num_written += chunk_size;
> +		msleep(ISL12026_NVMEM_WRITE_TIME);
> +	}
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return num_written >= 0 ? num_written : ret;

nvmem requires that you return 0 or an error, not the number of bytes
written. Also, in that case num_written >= 0 will always be true (size_t
is unsigned).

> +}
> +
> +static void isl12026_force_power_modes(struct i2c_client *client)
> +{
> +	int ret;
> +	int pwr, requested_pwr;
> +	u32 bsw_val, sbib_val;
> +	bool set_bsw, set_sbib;
> +
> +	/*
> +	 * If we can read the of_property, set the specified value.
> +	 * If there is an error reading the of_property (likely
> +	 * because it does not exist), keep the current value.
> +	 */
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "isil,pwr-bsw", &bsw_val);
> +	set_bsw = (ret == 0);
> +
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "isil,pwr-sbib", &sbib_val);
> +	set_sbib = (ret == 0);
> +
> +	/* Check if PWR.BSW and/or PWR.SBIB need specified values */
> +	if (!set_bsw && !set_sbib)
> +		return;
> +
> +	pwr = isl12026_read_reg(client, ISL12026_REG_PWR);
> +	if (pwr < 0) {
> +		dev_warn(&client->dev, "Error: Failed to read PWR %d\n", pwr);
> +		return;
> +	}
> +
> +	requested_pwr = pwr;
> +
> +	if (set_bsw) {
> +		if (bsw_val)
> +			requested_pwr |= ISL12026_REG_PWR_BSW;
> +		else
> +			requested_pwr &= ~ISL12026_REG_PWR_BSW;
> +	} /* else keep current BSW */
> +
> +	if (set_sbib) {
> +		if (sbib_val)
> +			requested_pwr |= ISL12026_REG_PWR_SBIB;
> +		else
> +			requested_pwr &= ~ISL12026_REG_PWR_SBIB;
> +	} /* else keep current SBIB */
> +
> +	if (pwr >= 0 && pwr != requested_pwr) {
> +		dev_info(&client->dev, "PWR: %02x\n", pwr);
> +		dev_info(&client->dev,
> +			 "Updating PWR to: %02x\n", requested_pwr);

I would use dev_dbg instead of dev_info.

> +		isl12026_write_reg(client, ISL12026_REG_PWR, requested_pwr);
> +	}
> +}
> +
> +static int isl12026_probe_new(struct i2c_client *client)
> +{
> +	struct isl12026 *priv;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->lock);
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	isl12026_force_power_modes(client);
> +
> +	priv->nvm_client = i2c_new_dummy(client->adapter, ISL12026_EEPROM_ADDR);
> +	if (!priv->nvm_client)
> +		return -ENOMEM;
> +
> +	priv->rtc = devm_rtc_allocate_device(&client->dev);
> +	ret = PTR_ERR_OR_ZERO(priv->rtc);
> +	if (ret)
> +		return ret;
> +
> +	priv->rtc->ops = &isl12026_rtc_ops;
> +
> +	priv->nvm_cfg.name = "eeprom";
> +	priv->nvm_cfg.read_only = false;
> +	priv->nvm_cfg.root_only = true;
> +	priv->nvm_cfg.base_dev = &client->dev;
> +	priv->nvm_cfg.priv = priv;
> +	priv->nvm_cfg.stride = 1;
> +	priv->nvm_cfg.word_size = 1;
> +	priv->nvm_cfg.size = 512;
> +	priv->nvm_cfg.reg_read = isl12026_nvm_read;
> +	priv->nvm_cfg.reg_write = isl12026_nvm_write;
> +
> +	priv->rtc->nvmem_config = &priv->nvm_cfg;
> +	priv->rtc->nvram_old_abi = false;

If you have a look at rtc-next, I've just changed the API so you don't
need to keep a copy of nvm_cfg. You will need to switch to that (at
least call rtc_nvmem_register from the driver).

> +
> +	return rtc_register_device(priv->rtc);
> +}
> +
> +static int isl12026_remove(struct i2c_client *client)
> +{
> +	struct isl12026 *priv = i2c_get_clientdata(client);
> +
> +	i2c_unregister_device(priv->nvm_client);
> +	return 0;
> +}
> +
> +static const struct of_device_id isl12026_dt_match[] = {
> +	{ .compatible = "isil,isl12026" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, isl12026_dt_match);
> +
> +static struct i2c_driver isl12026_driver = {
> +	.driver		= {
> +		.name	= "rtc-isl12026",
> +		.of_match_table = of_match_ptr(isl12026_dt_match),
> +	},
> +	.probe_new	= isl12026_probe_new,
> +	.remove		= isl12026_remove,
> +};
> +
> +module_i2c_driver(isl12026_driver);
> +
> +MODULE_DESCRIPTION("ISL 12026 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.14.3
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4] rtc: isl12026: Add driver.

[David Daney]

On 02/20/2018 03:03 AM, Alexandre Belloni wrote:
[...]


>> diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
>> new file mode 100644
>> index 000000000000..29e5bdf96c67
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-isl12026.c
>> @@ -0,0 +1,529 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * An I2C driver for the Intersil ISL 12026
>> + *
>> + * Copyright (c) 2018 Cavium, Inc.
>> + */
>> +#include <linux/bcd.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/rtc.h>
>> +#include <linux/slab.h>
>> +
>> +/* register offsets */
>> +#define ISL12026_REG_PWR	0x14
>> +# define ISL12026_REG_PWR_BSW	BIT(6)
>> +# define ISL12026_REG_PWR_SBIB	BIT(7)
>> +#define ISL12026_REG_SC		0x30
>> +#define ISL12026_REG_HR		0x32
>> +# define ISL12026_REG_HR_MIL	BIT(7)	/* military or 24 hour time */
>> +#define ISL12026_REG_SR		0x3f
>> +# define ISL12026_REG_SR_RTCF	BIT(0)
>> +# define ISL12026_REG_SR_WEL	BIT(1)
>> +# define ISL12026_REG_SR_RWEL	BIT(2)
>> +# define ISL12026_REG_SR_MBZ	BIT(3)
>> +# define ISL12026_REG_SR_OSCF	BIT(4)
>> +
>> +/* The EEPROM array responds at i2c address 0x57 */
>> +#define ISL12026_EEPROM_ADDR	0x57
>> +
>> +#define ISL12026_PAGESIZE 16
>> +#define ISL12026_NVMEM_WRITE_TIME 20
>> +
>> +struct isl12026 {
>> +	struct rtc_device *rtc;
>> +	struct i2c_client *nvm_client;
>> +	struct nvmem_config nvm_cfg;
>> +	/*
>> +	 * RTC write operations require that multiple messages be
>> +	 * transmitted, we hold the lock for all accesses to the
>> +	 * device so that these sequences cannot be disrupted.  Also,
>> +	 * the write cycle to the nvmem takes many ms during which the
>> +	 * device does not respond to commands, so holding the lock
>> +	 * also prevents access during these times.
>> +	 */
>> +	struct mutex lock;
>> +};
>> +
>> +static int isl12026_read_reg(struct i2c_client *client, int reg)
>> +{
>> +	struct isl12026 *priv = i2c_get_clientdata(client);
>> +	u8 addr[] = {0, reg};
>> +	u8 val;
>> +	int ret;
>> +
>> +	struct i2c_msg msgs[] = {
>> +		{
>> +			.addr	= client->addr,
>> +			.flags	= 0,
>> +			.len	= sizeof(addr),
>> +			.buf	= addr
>> +		}, {
>> +			.addr	= client->addr,
>> +			.flags	= I2C_M_RD,
>> +			.len	= 1,
>> +			.buf	= &val
>> +		}
>> +	};
> 
> I'm pretty sure you can use regmap instead of open coding all the i2c
> transfers, did you try?

I couldn't figure out how to make it do the device-atomic stores to 
SR.RWEL and SR.WEL that must precede certain register store operations. 
Also, dealing with locking across multiple i2c target addresses seems 
problematical with the regmap helpers.

The open coding doesn't clutter things up too much, and allows us to 
follow the isl12026 protocol without having to jump through too many hoops.

> 
>> +
>> +	mutex_lock(&priv->lock);
>> +
> 
> Also, regmap will remove the need for that lock.

Since


> 
>> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> +	if (ret != ARRAY_SIZE(msgs)) {
>> +		dev_err(&client->dev, "read reg error, ret=%d\n", ret);
>> +		ret = ret < 0 ? ret : -EIO;
>> +	} else {
>> +		ret = val;
>> +	}
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
>> +{
>> +	struct isl12026 *priv = i2c_get_clientdata(client);
>> +	int ret;
>> +	u8 op[3];
>> +	struct i2c_msg msg = {
>> +		.addr	= client->addr,
>> +		.flags	= 0,
>> +		.len	= 1,
>> +		.buf	= op
>> +	};
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	/* Set SR.WEL */
>> +	op[0] = 0;
>> +	op[1] = ISL12026_REG_SR;
>> +	op[2] = ISL12026_REG_SR_WEL;
>> +	msg.len = 3;
>> +	ret = i2c_transfer(client->adapter, &msg, 1);
>> +	if (ret != 1) {
>> +		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
>> +		ret = ret < 0 ? ret : -EIO;
>> +		goto out;
>> +	}
> 
> If you don't clear SR.WEL, I don't think you need to set it each time
> you write to the RTC. I would just set SR.WEL at probe time and let it
> there. That removes two i2c writes for each write operation.

I don't like the idea of leaving the thing partially armed when write 
operations should be rare.


[...]
>> +static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct isl12026 *priv = i2c_get_clientdata(client);
>> +	u8 ccr[8];
>> +	u8 addr[2];
>> +	u8 sr;
>> +	int ret;
>> +	struct i2c_msg msgs[] = {
>> +		{
>> +			.addr	= client->addr,
>> +			.flags	= 0,
>> +			.len	= sizeof(addr),
>> +			.buf	= addr
>> +		}, {
>> +			.addr	= client->addr,
>> +			.flags	= I2C_M_RD,
>> +		}
>> +	};
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	/* First, read SR */
>> +	addr[0] = 0;
>> +	addr[1] = ISL12026_REG_SR;
>> +	msgs[1].len = 1;
>> +	msgs[1].buf = &sr;
>> +
>> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> +	if (ret != ARRAY_SIZE(msgs)) {
>> +		dev_err(&client->dev, "read error, ret=%d\n", ret);
>> +		ret = ret < 0 ? ret : -EIO;
>> +		goto out;
>> +	}
>> +
>> +	if (sr & ISL12026_REG_SR_RTCF)
>> +		dev_warn(&client->dev, "Real-Time Clock Failure on read\n");
>> +	if (sr & ISL12026_REG_SR_OSCF)
>> +		dev_warn(&client->dev, "Oscillator Failure on read\n");
>> +
>> +	/* Second, CCR regs */
>> +	addr[0] = 0;
>> +	addr[1] = ISL12026_REG_SC;
>> +	msgs[1].len = sizeof(ccr);
>> +	msgs[1].buf = ccr;
>> +
>> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> +	if (ret != ARRAY_SIZE(msgs)) {
>> +		dev_err(&client->dev, "read error, ret=%d\n", ret);
>> +		ret = ret < 0 ? ret : -EIO;
>> +		goto out;
>> +	}
>> +
>> +	tm->tm_sec = bcd2bin(ccr[0] & 0x7F);
>> +	tm->tm_min = bcd2bin(ccr[1] & 0x7F);
>> +	if (ccr[2] & ISL12026_REG_HR_MIL)
>> +		tm->tm_hour = bcd2bin(ccr[2] & 0x3F);
>> +	else
>> +		tm->tm_hour = bcd2bin(ccr[2] & 0x1F) +
>> +			((ccr[2] & 0x20) ? 12 : 0);
>> +	tm->tm_mday = bcd2bin(ccr[3] & 0x3F);
>> +	tm->tm_mon = bcd2bin(ccr[4] & 0x1F) - 1;
>> +	tm->tm_year = bcd2bin(ccr[5]);
>> +	if (bcd2bin(ccr[7]) == 20)
>> +		tm->tm_year += 100;
>> +	tm->tm_wday = ccr[6] & 0x07;
>> +
>> +	ret = rtc_valid_tm(tm);
> 
> This rtc_valid_tm is unnecessary, you can simply return 0.

It may be possible for invalid values to be programmed into the RTC, 
this would catch that case.


> 
>> +out:
>> +	mutex_unlock(&priv->lock);
>> +	return ret;
>> +}
>> +
>> +static const struct rtc_class_ops isl12026_rtc_ops = {
>> +	.read_time	= isl12026_rtc_read_time,
>> +	.set_time	= isl12026_rtc_set_time,
>> +};
>> +
>> +static int isl12026_nvm_read(void *p, unsigned int offset,
>> +			     void *val, size_t bytes)
>> +{
>> +	struct isl12026 *priv = p;
>> +	int ret;
>> +	u8 addr[2];
>> +	struct i2c_msg msgs[] = {
>> +		{
>> +			.addr	= priv->nvm_client->addr,
>> +			.flags	= 0,
>> +			.len	= sizeof(addr),
>> +			.buf	= addr
>> +		}, {
>> +			.addr	= priv->nvm_client->addr,
>> +			.flags	= I2C_M_RD,
>> +			.buf	= val
>> +		}
>> +	};
>> +
>> +	if (offset >= priv->nvm_cfg.size)
>> +		return 0; /* End-of-file */
>> +	if (offset + bytes > priv->nvm_cfg.size)
>> +		bytes = priv->nvm_cfg.size - offset;
>> +
>> +	mutex_lock(&priv->lock);
> 
> You can completely remove the need for that lock by taking
> priv->rtc->ops_lock here.

Good point.  I will try doing that.


> 
>> +
>> +	/* 2 bytes of address, most significant first */
>> +	addr[0] = offset >> 8;
>> +	addr[1] = offset;
>> +	msgs[1].len = bytes;
>> +	ret = i2c_transfer(priv->nvm_client->adapter, msgs, ARRAY_SIZE(msgs));
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	if (ret != ARRAY_SIZE(msgs)) {
>> +		dev_err(priv->nvm_cfg.dev, "nvmem read error, ret=%d\n", ret);
>> +		return ret < 0 ? ret : -EIO;
>> +	}
>> +
>> +	return bytes;
>> +}
>> +
>> +static int isl12026_nvm_write(void *p, unsigned int offset,
>> +			      void *val, size_t bytes)
>> +{
>> +	struct isl12026 *priv = p;
>> +	int ret = -EIO;
>> +	u8 *v = val;
>> +	size_t chunk_size, num_written;
>> +	u8 payload[ISL12026_PAGESIZE + 2]; /* page + 2 address bytes */
>> +	struct i2c_msg msgs[] = {
>> +		{
>> +			.addr	= priv->nvm_client->addr,
>> +			.flags	= 0,
>> +			.buf	= payload
>> +		}
>> +	};
>> +
>> +	if (offset >= priv->nvm_cfg.size)
>> +		return 0; /* End-of-file */
>> +	if (offset + bytes > priv->nvm_cfg.size)
>> +		bytes = priv->nvm_cfg.size - offset;
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	num_written = 0;
>> +	while (bytes) {
>> +		chunk_size = round_down(offset, ISL12026_PAGESIZE) +
>> +			ISL12026_PAGESIZE - offset;
>> +		chunk_size = min(bytes, chunk_size);
>> +		/*
>> +		 * 2 bytes of address, most significant first, followed
>> +		 * by page data bytes
>> +		 */
>> +		memcpy(payload + 2, v + num_written, chunk_size);
>> +		payload[0] = offset >> 8;
>> +		payload[1] = offset;
>> +		msgs[0].len = chunk_size + 2;
>> +		ret = i2c_transfer(priv->nvm_client->adapter,
>> +				   msgs, ARRAY_SIZE(msgs));
>> +		if (ret != ARRAY_SIZE(msgs)) {
>> +			dev_err(priv->nvm_cfg.dev,
>> +				"nvmem write error, ret=%d\n", ret);
>> +			ret = ret < 0 ? ret : -EIO;
>> +			break;
>> +		}
>> +		bytes -= chunk_size;
>> +		offset += chunk_size;
>> +		num_written += chunk_size;
>> +		msleep(ISL12026_NVMEM_WRITE_TIME);
>> +	}
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	return num_written >= 0 ? num_written : ret;
> 
> nvmem requires that you return 0 or an error, not the number of bytes
> written. Also, in that case num_written >= 0 will always be true (size_t
> is unsigned).


Yes, the 0-day bot found these problems too.

I will fix this.


> 
>> +}
>> +
>> +static void isl12026_force_power_modes(struct i2c_client *client)
>> +{
>> +	int ret;
>> +	int pwr, requested_pwr;
>> +	u32 bsw_val, sbib_val;
>> +	bool set_bsw, set_sbib;
>> +
>> +	/*
>> +	 * If we can read the of_property, set the specified value.
>> +	 * If there is an error reading the of_property (likely
>> +	 * because it does not exist), keep the current value.
>> +	 */
>> +	ret = of_property_read_u32(client->dev.of_node,
>> +				   "isil,pwr-bsw", &bsw_val);
>> +	set_bsw = (ret == 0);
>> +
>> +	ret = of_property_read_u32(client->dev.of_node,
>> +				   "isil,pwr-sbib", &sbib_val);
>> +	set_sbib = (ret == 0);
>> +
>> +	/* Check if PWR.BSW and/or PWR.SBIB need specified values */
>> +	if (!set_bsw && !set_sbib)
>> +		return;
>> +
>> +	pwr = isl12026_read_reg(client, ISL12026_REG_PWR);
>> +	if (pwr < 0) {
>> +		dev_warn(&client->dev, "Error: Failed to read PWR %d\n", pwr);
>> +		return;
>> +	}
>> +
>> +	requested_pwr = pwr;
>> +
>> +	if (set_bsw) {
>> +		if (bsw_val)
>> +			requested_pwr |= ISL12026_REG_PWR_BSW;
>> +		else
>> +			requested_pwr &= ~ISL12026_REG_PWR_BSW;
>> +	} /* else keep current BSW */
>> +
>> +	if (set_sbib) {
>> +		if (sbib_val)
>> +			requested_pwr |= ISL12026_REG_PWR_SBIB;
>> +		else
>> +			requested_pwr &= ~ISL12026_REG_PWR_SBIB;
>> +	} /* else keep current SBIB */
>> +
>> +	if (pwr >= 0 && pwr != requested_pwr) {
>> +		dev_info(&client->dev, "PWR: %02x\n", pwr);
>> +		dev_info(&client->dev,
>> +			 "Updating PWR to: %02x\n", requested_pwr);
> 
> I would use dev_dbg instead of dev_info.
> 
>> +		isl12026_write_reg(client, ISL12026_REG_PWR, requested_pwr);
>> +	}
>> +}
>> +
>> +static int isl12026_probe_new(struct i2c_client *client)
>> +{
>> +	struct isl12026 *priv;
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>> +		return -ENODEV;
>> +
>> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&priv->lock);
>> +
>> +	i2c_set_clientdata(client, priv);
>> +
>> +	isl12026_force_power_modes(client);
>> +
>> +	priv->nvm_client = i2c_new_dummy(client->adapter, ISL12026_EEPROM_ADDR);
>> +	if (!priv->nvm_client)
>> +		return -ENOMEM;
>> +
>> +	priv->rtc = devm_rtc_allocate_device(&client->dev);
>> +	ret = PTR_ERR_OR_ZERO(priv->rtc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->rtc->ops = &isl12026_rtc_ops;
>> +
>> +	priv->nvm_cfg.name = "eeprom";
>> +	priv->nvm_cfg.read_only = false;
>> +	priv->nvm_cfg.root_only = true;
>> +	priv->nvm_cfg.base_dev = &client->dev;
>> +	priv->nvm_cfg.priv = priv;
>> +	priv->nvm_cfg.stride = 1;
>> +	priv->nvm_cfg.word_size = 1;
>> +	priv->nvm_cfg.size = 512;
>> +	priv->nvm_cfg.reg_read = isl12026_nvm_read;
>> +	priv->nvm_cfg.reg_write = isl12026_nvm_write;
>> +
>> +	priv->rtc->nvmem_config = &priv->nvm_cfg;
>> +	priv->rtc->nvram_old_abi = false;
> 
> If you have a look at rtc-next, I've just changed the API so you don't
> need to keep a copy of nvm_cfg. You will need to switch to that (at
> least call rtc_nvmem_register from the driver).


OK

> 
>> +
>> +	return rtc_register_device(priv->rtc);
>> +}
>> +
>> +static int isl12026_remove(struct i2c_client *client)
>> +{
>> +	struct isl12026 *priv = i2c_get_clientdata(client);
>> +
>> +	i2c_unregister_device(priv->nvm_client);
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id isl12026_dt_match[] = {
>> +	{ .compatible = "isil,isl12026" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, isl12026_dt_match);
>> +
>> +static struct i2c_driver isl12026_driver = {
>> +	.driver		= {
>> +		.name	= "rtc-isl12026",
>> +		.of_match_table = of_match_ptr(isl12026_dt_match),
>> +	},
>> +	.probe_new	= isl12026_probe_new,
>> +	.remove		= isl12026_remove,
>> +};
>> +
>> +module_i2c_driver(isl12026_driver);
>> +
>> +MODULE_DESCRIPTION("ISL 12026 RTC driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.14.3
>>
> 

Re: [PATCH v4] rtc: isl12026: Add driver.

[Alexandre Belloni]

On 20/02/2018 at 11:43:47 -0800, David Daney wrote:
> On 02/20/2018 03:03 AM, Alexandre Belloni wrote:
> [...]
> 
> 
> > > diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> > > new file mode 100644
> > > index 000000000000..29e5bdf96c67
> > > --- /dev/null
> > > +++ b/drivers/rtc/rtc-isl12026.c
> > > @@ -0,0 +1,529 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * An I2C driver for the Intersil ISL 12026
> > > + *
> > > + * Copyright (c) 2018 Cavium, Inc.
> > > + */
> > > +#include <linux/bcd.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/nvmem-provider.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/rtc.h>
> > > +#include <linux/slab.h>
> > > +
> > > +/* register offsets */
> > > +#define ISL12026_REG_PWR	0x14
> > > +# define ISL12026_REG_PWR_BSW	BIT(6)
> > > +# define ISL12026_REG_PWR_SBIB	BIT(7)
> > > +#define ISL12026_REG_SC		0x30
> > > +#define ISL12026_REG_HR		0x32
> > > +# define ISL12026_REG_HR_MIL	BIT(7)	/* military or 24 hour time */
> > > +#define ISL12026_REG_SR		0x3f
> > > +# define ISL12026_REG_SR_RTCF	BIT(0)
> > > +# define ISL12026_REG_SR_WEL	BIT(1)
> > > +# define ISL12026_REG_SR_RWEL	BIT(2)
> > > +# define ISL12026_REG_SR_MBZ	BIT(3)
> > > +# define ISL12026_REG_SR_OSCF	BIT(4)
> > > +
> > > +/* The EEPROM array responds at i2c address 0x57 */
> > > +#define ISL12026_EEPROM_ADDR	0x57
> > > +
> > > +#define ISL12026_PAGESIZE 16
> > > +#define ISL12026_NVMEM_WRITE_TIME 20
> > > +
> > > +struct isl12026 {
> > > +	struct rtc_device *rtc;
> > > +	struct i2c_client *nvm_client;
> > > +	struct nvmem_config nvm_cfg;
> > > +	/*
> > > +	 * RTC write operations require that multiple messages be
> > > +	 * transmitted, we hold the lock for all accesses to the
> > > +	 * device so that these sequences cannot be disrupted.  Also,
> > > +	 * the write cycle to the nvmem takes many ms during which the
> > > +	 * device does not respond to commands, so holding the lock
> > > +	 * also prevents access during these times.
> > > +	 */
> > > +	struct mutex lock;
> > > +};
> > > +
> > > +static int isl12026_read_reg(struct i2c_client *client, int reg)
> > > +{
> > > +	struct isl12026 *priv = i2c_get_clientdata(client);
> > > +	u8 addr[] = {0, reg};
> > > +	u8 val;
> > > +	int ret;
> > > +
> > > +	struct i2c_msg msgs[] = {
> > > +		{
> > > +			.addr	= client->addr,
> > > +			.flags	= 0,
> > > +			.len	= sizeof(addr),
> > > +			.buf	= addr
> > > +		}, {
> > > +			.addr	= client->addr,
> > > +			.flags	= I2C_M_RD,
> > > +			.len	= 1,
> > > +			.buf	= &val
> > > +		}
> > > +	};
> > 
> > I'm pretty sure you can use regmap instead of open coding all the i2c
> > transfers, did you try?
> 
> I couldn't figure out how to make it do the device-atomic stores to SR.RWEL
> and SR.WEL that must precede certain register store operations. Also,
> dealing with locking across multiple i2c target addresses seems
> problematical with the regmap helpers.
> 
> The open coding doesn't clutter things up too much, and allows us to follow
> the isl12026 protocol without having to jump through too many hoops.
> 
> > 
> > > +
> > > +	mutex_lock(&priv->lock);
> > > +
> > 
> > Also, regmap will remove the need for that lock.
> 
> Since
> 
> 
> > 
> > > +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > +	if (ret != ARRAY_SIZE(msgs)) {
> > > +		dev_err(&client->dev, "read reg error, ret=%d\n", ret);
> > > +		ret = ret < 0 ? ret : -EIO;
> > > +	} else {
> > > +		ret = val;
> > > +	}
> > > +
> > > +	mutex_unlock(&priv->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
> > > +{
> > > +	struct isl12026 *priv = i2c_get_clientdata(client);
> > > +	int ret;
> > > +	u8 op[3];
> > > +	struct i2c_msg msg = {
> > > +		.addr	= client->addr,
> > > +		.flags	= 0,
> > > +		.len	= 1,
> > > +		.buf	= op
> > > +	};
> > > +
> > > +	mutex_lock(&priv->lock);
> > > +
> > > +	/* Set SR.WEL */
> > > +	op[0] = 0;
> > > +	op[1] = ISL12026_REG_SR;
> > > +	op[2] = ISL12026_REG_SR_WEL;
> > > +	msg.len = 3;
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	if (ret != 1) {
> > > +		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
> > > +		ret = ret < 0 ? ret : -EIO;
> > > +		goto out;
> > > +	}
> > 
> > If you don't clear SR.WEL, I don't think you need to set it each time
> > you write to the RTC. I would just set SR.WEL at probe time and let it
> > there. That removes two i2c writes for each write operation.
> 
> I don't like the idea of leaving the thing partially armed when write
> operations should be rare.
> 

Ok, then, can you at least factorize the write enabling/disabling in two
functions. That would make the code smaller.

> > > +	ret = rtc_valid_tm(tm);
> > 
> > This rtc_valid_tm is unnecessary, you can simply return 0.
> 
> It may be possible for invalid values to be programmed into the RTC, this
> would catch that case.
> 

Which will be caught by the core anyway.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com