All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6] rtc: isl12026: Add driver.
@ 2018-02-22 20:04 David Daney
  2018-02-28 15:17 ` Alexandre Belloni
  0 siblings, 1 reply; 4+ messages in thread
From: David Daney @ 2018-02-22 20:04 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	linux-rtc, devicetree
  Cc: linux-kernel, Andy Shevchenko, 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: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---

Changes from v5:

o removed obsolete call to rtc_valid_tm().

Changes from v4:

o Rebased to rtc-next

o Improvements suggested by Alexandre Belloni

o Added Reviewed-by Rob Herring

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                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-isl12026.c                         | 505 +++++++++++++++++++++
 4 files changed, 544 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 f6d7e490e714..3bf649cd7b83 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -407,6 +407,16 @@ 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"
+	depends on OF || COMPILE_TEST
+	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..ada9849d8d97
--- /dev/null
+++ b/drivers/rtc/rtc-isl12026.c
@@ -0,0 +1,505 @@
+// 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;
+};
+
+static int isl12026_read_reg(struct i2c_client *client, int reg)
+{
+	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
+		}
+	};
+
+	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;
+	}
+
+	return ret;
+}
+
+static int isl12026_arm_write(struct i2c_client *client)
+{
+	int ret;
+	u8 op[3];
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= 1,
+		.buf	= op
+	};
+
+	/* 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;
+	} else {
+		ret = 0;
+	}
+out:
+	return ret;
+}
+
+static int isl12026_disarm_write(struct i2c_client *client)
+{
+	int ret;
+	u8 op[3] = {0, ISL12026_REG_SR, 0};
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= sizeof(op),
+		.buf	= op
+	};
+
+	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;
+	} else {
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
+{
+	int ret;
+	u8 op[3] = {0, reg, val};
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= sizeof(op),
+		.buf	= op
+	};
+
+	ret = isl12026_arm_write(client);
+	if (ret)
+		return ret;
+
+	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);
+
+	ret = isl12026_disarm_write(client);
+out:
+	return ret;
+}
+
+static int isl12026_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+	u8 op[10];
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= sizeof(op),
+		.buf	= op
+	};
+
+	ret = isl12026_arm_write(client);
+	if (ret)
+		return ret;
+
+	/* Set the CCR registers */
+	op[0] = 0;
+	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 */
+	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;
+	}
+
+	ret = isl12026_disarm_write(client);
+out:
+	return ret;
+}
+
+static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	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,
+		}
+	};
+
+	/* 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 = 0;
+out:
+	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
+		}
+	};
+
+	/*
+	 * offset and bytes checked and limited by nvmem core, so
+	 * proceed without further checks.
+	 */
+	ret = mutex_lock_interruptible(&priv->rtc->ops_lock);
+	if (ret)
+		return ret;
+
+	/* 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->rtc->ops_lock);
+
+	if (ret != ARRAY_SIZE(msgs)) {
+		dev_err(&priv->nvm_client->dev,
+			"nvmem read error, ret=%d\n", ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
+}
+
+static int isl12026_nvm_write(void *p, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct isl12026 *priv = p;
+	int ret;
+	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
+		}
+	};
+
+	/*
+	 * offset and bytes checked and limited by nvmem core, so
+	 * proceed without further checks.
+	 */
+	ret = mutex_lock_interruptible(&priv->rtc->ops_lock);
+	if (ret)
+		return ret;
+
+	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_client->dev,
+				"nvmem write error, ret=%d\n", ret);
+			ret = ret < 0 ? ret : -EIO;
+			break;
+		}
+		ret = 0;
+		bytes -= chunk_size;
+		offset += chunk_size;
+		num_written += chunk_size;
+		msleep(ISL12026_NVMEM_WRITE_TIME);
+	}
+
+	mutex_unlock(&priv->rtc->ops_lock);
+
+	return 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_dbg(&client->dev, "PWR: %02x\n", pwr);
+		dev_dbg(&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;
+	struct nvmem_config nvm_cfg;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	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->rtc->nvram_old_abi = false;
+	ret = rtc_register_device(priv->rtc);
+	if (ret)
+		return ret;
+
+	memset(&nvm_cfg, 0, sizeof(nvm_cfg));
+	nvm_cfg.name = "eeprom";
+	nvm_cfg.read_only = false;
+	nvm_cfg.root_only = true;
+	nvm_cfg.base_dev = &client->dev;
+	nvm_cfg.priv = priv;
+	nvm_cfg.stride = 1;
+	nvm_cfg.word_size = 1;
+	nvm_cfg.size = 512;
+	nvm_cfg.reg_read = isl12026_nvm_read;
+	nvm_cfg.reg_write = isl12026_nvm_write;
+
+	return rtc_nvmem_register(priv->rtc, &nvm_cfg);
+}
+
+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 = 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

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

* Re: [PATCH v6] rtc: isl12026: Add driver.
  2018-02-22 20:04 [PATCH v6] rtc: isl12026: Add driver David Daney
@ 2018-02-28 15:17 ` Alexandre Belloni
  2018-03-02 16:22   ` David Daney
  0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Belloni @ 2018-02-28 15:17 UTC (permalink / raw)
  To: David Daney
  Cc: Alessandro Zummo, Rob Herring, Mark Rutland, linux-rtc,
	devicetree, linux-kernel, Andy Shevchenko

Hi,

That's mostly good

On 22/02/2018 at 12:04:32 -0800, David Daney wrote:
> +	priv->rtc->ops = &isl12026_rtc_ops;
> +	priv->rtc->nvram_old_abi = false;

This allocation is not necessary and I would refer not having t so when
the ABI goes away, it is not necessary to change this driver.

> +	ret = rtc_register_device(priv->rtc);
> +	if (ret)
> +		return ret;
> +
> +	memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> +	nvm_cfg.name = "eeprom";

You probably need something more descriptive, usually the rtc model name
else, it is difficult (but not impossible) to find where this nvmem is
actually located when using /sys/bus/nvmem/devices
isl12026- would be a good choice.

> +	nvm_cfg.read_only = false;
> +	nvm_cfg.root_only = true;

any reason to have it root only?

> +	nvm_cfg.base_dev = &client->dev;
> +	nvm_cfg.priv = priv;
> +	nvm_cfg.stride = 1;
> +	nvm_cfg.word_size = 1;
> +	nvm_cfg.size = 512;
> +	nvm_cfg.reg_read = isl12026_nvm_read;
> +	nvm_cfg.reg_write = isl12026_nvm_write;
> +
> +	return rtc_nvmem_register(priv->rtc, &nvm_cfg);

The probe function must not fail after rtc_register_device has been
called so you must return 0 here.

It is not currently possible to call rtc_nvmem_register before
rtc_register_device unless we move the nvmem to the parent device:

diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c
index 3a02357eb783..17ec4c8d0fad 100644
--- a/drivers/rtc/nvmem.c
+++ b/drivers/rtc/nvmem.c
@@ -91,7 +91,7 @@ int rtc_nvmem_register(struct rtc_device *rtc,
        if (!nvmem_config)
                return -ENODEV;
 
-       nvmem_config->dev = &rtc->dev;
+       nvmem_config->dev = rtc->dev.parent;
        nvmem_config->owner = rtc->owner;
        rtc->nvmem = nvmem_register(nvmem_config);
        if (IS_ERR_OR_NULL(rtc->nvmem))

I'll submit this patch for this cycle before there are many users of the
interface (converted drivers are still exposing the old ABI).

If you prefer, you can depend on it.

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

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

* Re: [PATCH v6] rtc: isl12026: Add driver.
  2018-02-28 15:17 ` Alexandre Belloni
@ 2018-03-02 16:22   ` David Daney
  2018-03-02 23:43     ` Alexandre Belloni
  0 siblings, 1 reply; 4+ messages in thread
From: David Daney @ 2018-03-02 16:22 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: David Daney, Alessandro Zummo, Rob Herring, Mark Rutland,
	linux-rtc, devicetree, Linux Kernel Mailing List,
	Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

I am no longer able to test this patch as I lost access to the hardware.

If someone else wants to take charge of the patch, that would be great.

Sorry I couldn't drive this to completion,
David Daney

On Wed, Feb 28, 2018 at 7:17 AM, Alexandre Belloni <
alexandre.belloni@free-electrons.com> wrote:

> Hi,
>
> That's mostly good
>
> On 22/02/2018 at 12:04:32 -0800, David Daney wrote:
> > +     priv->rtc->ops = &isl12026_rtc_ops;
> > +     priv->rtc->nvram_old_abi = false;
>
> This allocation is not necessary and I would refer not having t so when
> the ABI goes away, it is not necessary to change this driver.
>
> > +     ret = rtc_register_device(priv->rtc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> > +     nvm_cfg.name = "eeprom";
>
> You probably need something more descriptive, usually the rtc model name
> else, it is difficult (but not impossible) to find where this nvmem is
> actually located when using /sys/bus/nvmem/devices
> isl12026- would be a good choice.
>
> > +     nvm_cfg.read_only = false;
> > +     nvm_cfg.root_only = true;
>
> any reason to have it root only?
>
> > +     nvm_cfg.base_dev = &client->dev;
> > +     nvm_cfg.priv = priv;
> > +     nvm_cfg.stride = 1;
> > +     nvm_cfg.word_size = 1;
> > +     nvm_cfg.size = 512;
> > +     nvm_cfg.reg_read = isl12026_nvm_read;
> > +     nvm_cfg.reg_write = isl12026_nvm_write;
> > +
> > +     return rtc_nvmem_register(priv->rtc, &nvm_cfg);
>
> The probe function must not fail after rtc_register_device has been
> called so you must return 0 here.
>
> It is not currently possible to call rtc_nvmem_register before
> rtc_register_device unless we move the nvmem to the parent device:
>
> diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c
> index 3a02357eb783..17ec4c8d0fad 100644
> --- a/drivers/rtc/nvmem.c
> +++ b/drivers/rtc/nvmem.c
> @@ -91,7 +91,7 @@ int rtc_nvmem_register(struct rtc_device *rtc,
>         if (!nvmem_config)
>                 return -ENODEV;
>
> -       nvmem_config->dev = &rtc->dev;
> +       nvmem_config->dev = rtc->dev.parent;
>         nvmem_config->owner = rtc->owner;
>         rtc->nvmem = nvmem_register(nvmem_config);
>         if (IS_ERR_OR_NULL(rtc->nvmem))
>
> I'll submit this patch for this cycle before there are many users of the
> interface (converted drivers are still exposing the old ABI).
>
> If you prefer, you can depend on it.
>
> --
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: text/html, Size: 4067 bytes --]

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

* Re: [PATCH v6] rtc: isl12026: Add driver.
  2018-03-02 16:22   ` David Daney
@ 2018-03-02 23:43     ` Alexandre Belloni
  0 siblings, 0 replies; 4+ messages in thread
From: Alexandre Belloni @ 2018-03-02 23:43 UTC (permalink / raw)
  To: David Daney
  Cc: David Daney, Alessandro Zummo, Rob Herring, Mark Rutland,
	linux-rtc, devicetree, Linux Kernel Mailing List,
	Andy Shevchenko

On 02/03/2018 at 08:22:22 -0800, David Daney wrote:
> I am no longer able to test this patch as I lost access to the hardware.
> 

Ah, too bad. I will take your patch as-is and then add a small patch to
fixup the nvmem registration.

> If someone else wants to take charge of the patch, that would be great.
> 
> Sorry I couldn't drive this to completion,
> David Daney
> 
> On Wed, Feb 28, 2018 at 7:17 AM, Alexandre Belloni <
> alexandre.belloni@free-electrons.com> wrote:
> 
> > Hi,
> >
> > That's mostly good
> >
> > On 22/02/2018 at 12:04:32 -0800, David Daney wrote:
> > > +     priv->rtc->ops = &isl12026_rtc_ops;
> > > +     priv->rtc->nvram_old_abi = false;
> >
> > This allocation is not necessary and I would refer not having t so when
> > the ABI goes away, it is not necessary to change this driver.
> >
> > > +     ret = rtc_register_device(priv->rtc);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> > > +     nvm_cfg.name = "eeprom";
> >
> > You probably need something more descriptive, usually the rtc model name
> > else, it is difficult (but not impossible) to find where this nvmem is
> > actually located when using /sys/bus/nvmem/devices
> > isl12026- would be a good choice.
> >
> > > +     nvm_cfg.read_only = false;
> > > +     nvm_cfg.root_only = true;
> >
> > any reason to have it root only?
> >
> > > +     nvm_cfg.base_dev = &client->dev;
> > > +     nvm_cfg.priv = priv;
> > > +     nvm_cfg.stride = 1;
> > > +     nvm_cfg.word_size = 1;
> > > +     nvm_cfg.size = 512;
> > > +     nvm_cfg.reg_read = isl12026_nvm_read;
> > > +     nvm_cfg.reg_write = isl12026_nvm_write;
> > > +
> > > +     return rtc_nvmem_register(priv->rtc, &nvm_cfg);
> >
> > The probe function must not fail after rtc_register_device has been
> > called so you must return 0 here.
> >
> > It is not currently possible to call rtc_nvmem_register before
> > rtc_register_device unless we move the nvmem to the parent device:
> >
> > diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c
> > index 3a02357eb783..17ec4c8d0fad 100644
> > --- a/drivers/rtc/nvmem.c
> > +++ b/drivers/rtc/nvmem.c
> > @@ -91,7 +91,7 @@ int rtc_nvmem_register(struct rtc_device *rtc,
> >         if (!nvmem_config)
> >                 return -ENODEV;
> >
> > -       nvmem_config->dev = &rtc->dev;
> > +       nvmem_config->dev = rtc->dev.parent;
> >         nvmem_config->owner = rtc->owner;
> >         rtc->nvmem = nvmem_register(nvmem_config);
> >         if (IS_ERR_OR_NULL(rtc->nvmem))
> >
> > I'll submit this patch for this cycle before there are many users of the
> > interface (converted drivers are still exposing the old ABI).
> >
> > If you prefer, you can depend on it.
> >
> > --
> > Alexandre Belloni, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

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

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

end of thread, other threads:[~2018-03-02 23:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22 20:04 [PATCH v6] rtc: isl12026: Add driver David Daney
2018-02-28 15:17 ` Alexandre Belloni
2018-03-02 16:22   ` David Daney
2018-03-02 23:43     ` Alexandre Belloni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.