[PATCH 0/2] rtc: add Abracon ABx80x

[PATCH 0/2] rtc: add Abracon ABx80x

[Alexandre Belloni]

Hi,

As discussed, I reworked the abracon abx80x driver, merging Philippe's and mine.

I have added the Device Tree properties to enable the trickle charger and the
driver will detect the partnumber before doing that so that it won't try to
enable the charger when it is not available.

Alexandre Belloni (1):
  Documentation: bindings: add abracon,abx80x

Philippe De Muyter (1):
  rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

 .../devicetree/bindings/rtc/abracon,abx80x.txt     |  32 ++
 drivers/rtc/Kconfig                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-abx80x.c                           | 330 +++++++++++++++++++++
 4 files changed, 373 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
 create mode 100644 drivers/rtc/rtc-abx80x.c

-- 
2.1.0

[PATCH 1/2] Documentation: bindings: add abracon,abx80x

[Alexandre Belloni]

Document the bindings for abracon,abx80x and related compatibles.

Cc: devicetree-discuss@lists.ozlabs.org
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 .../devicetree/bindings/rtc/abracon,abx80x.txt     | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/abracon,abx80x.txt

diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
new file mode 100644
index 000000000000..6f0d3b2cdd89
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
@@ -0,0 +1,32 @@
+Abracon ABX80X I2C ultra low power RTC/Alarm chip
+
+The Abracon ABX80X family consist of the ab0801, ab0803, ab0804, ab0805, ab1801,
+ab1803, ab1804 and ab1805. The ab0805 is the superset of ab080x and the ab1805
+is the superset of ab180x.
+
+Required properties:
+
+ - "compatible": should one of:
+        "abracon,abx80x"
+        "abracon,ab0801"
+        "abracon,ab0803"
+        "abracon,ab0804"
+        "abracon,ab0805"
+        "abracon,ab1801"
+        "abracon,ab1803"
+        "abracon,ab1804"
+        "abracon,ab1805"
+	Using "abracon,abx80x" will enable chip autodetection.
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+The abx804 and abx805 have a trickle charger that is able to charge the
+connected battery or supercap. Both the following properties have to be defined
+and valid to enable charging:
+
+ - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V)
+ - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output
+                          resistor, the other values are in ohm.
+
+
-- 
2.1.0

[PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

[Alexandre Belloni]

From: Philippe De Muyter <phdm@macqel.be>

This is a basic driver for the ultra-low-power Abracon AB x80x series of RTC
chips. It supports in particular, the supersets AB0805 and AB1805.
It allows reading and writing the time, and enables the supercapacitor/
battery charger.

[arnd@arndb.de: abx805 depends on i2c]
Signed-off-by: Philippe De Muyter <phdm@macqel.be>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/rtc/Kconfig      |  10 ++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-abx80x.c | 325 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/rtc/rtc-abx80x.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b5b5c3d485d6..89507c1858ec 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -164,6 +164,16 @@ config RTC_DRV_ABB5ZES3
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ab-b5ze-s3.
 
+config RTC_DRV_ABX80X
+	tristate "Abracon ABx80x"
+	help
+	  If you say yes here you get support for Abracon AB080X and AB180X
+	  families of ultra-low-power  battery- and capacitor-backed real-time
+	  clock chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-abx80x.
+
 config RTC_DRV_AS3722
 	tristate "ams AS3722 RTC driver"
 	depends on MFD_AS3722
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 69c87062b098..7b20b0462cb6 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
 obj-$(CONFIG_RTC_DRV_AB3100)	+= rtc-ab3100.o
 obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
 obj-$(CONFIG_RTC_DRV_ABB5ZES3)	+= rtc-ab-b5ze-s3.o
+obj-$(CONFIG_RTC_DRV_ABX80X)	+= rtc-abx80x.o
 obj-$(CONFIG_RTC_DRV_ARMADA38X)	+= rtc-armada38x.o
 obj-$(CONFIG_RTC_DRV_AS3722)	+= rtc-as3722.o
 obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
new file mode 100644
index 000000000000..8424e500f1da
--- /dev/null
+++ b/drivers/rtc/rtc-abx80x.c
@@ -0,0 +1,325 @@
+/*
+ * A driver for the I2C members of the Abracon AB x8xx RTC family,
+ * and compatible: AB 1805 and AB 0805
+ *
+ * Copyright 2014-2015 Macq S.A.
+ *
+ * Author: Philippe De Muyter <phdm@macqel.be>
+ * Author: Alexandre Belloni <alexandre.belloni@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/bcd.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/rtc.h>
+
+#define ABX8XX_REG_HTH		0x00
+#define ABX8XX_REG_SC		0x01
+#define ABX8XX_REG_MN		0x02
+#define ABX8XX_REG_HR		0x03
+#define ABX8XX_REG_DA		0x04
+#define ABX8XX_REG_MO		0x05
+#define ABX8XX_REG_YR		0x06
+#define ABX8XX_REG_WD		0x07
+
+#define ABX8XX_REG_CTRL1	0x10
+#define ABX8XX_CTRL_WRITE	BIT(1)
+#define ABX8XX_CTRL_12_24	BIT(6)
+
+#define ABX8XX_REG_CFG_KEY	0x1f
+#define ABX8XX_CFG_KEY_MISC	0x9d
+
+#define ABX8XX_REG_ID0		0x28
+
+#define ABX8XX_REG_TRICKLE	0x20
+#define ABX8XX_TRICKLE_CHARGE_ENABLE	0xa0
+#define ABX8XX_TRICKLE_STANDARD_DIODE	0x8
+#define ABX8XX_TRICKLE_SCHOTTKY_DIODE	0x4
+
+static u8 trickle_resistors[] = {0, 3, 6, 11};
+
+enum abx80x_chip {AB0801, AB0803, AB0804, AB0805,
+	AB1801, AB1803, AB1804, AB1805, ABX80X};
+
+struct abx80x_cap {
+	u16 pn;
+	bool has_tc;
+};
+
+static struct abx80x_cap abx80x_caps[] = {
+	[AB0801] = {.pn = 0x0801},
+	[AB0803] = {.pn = 0x0803},
+	[AB0804] = {.pn = 0x0804, .has_tc = true},
+	[AB0805] = {.pn = 0x0805, .has_tc = true},
+	[AB1801] = {.pn = 0x1801},
+	[AB1803] = {.pn = 0x1803},
+	[AB1804] = {.pn = 0x1804, .has_tc = true},
+	[AB1805] = {.pn = 0x1805, .has_tc = true},
+	[ABX80X] = {.pn = 0}
+};
+
+static struct i2c_driver abx80x_driver;
+
+static int abx80x_enable_trickle_charger(struct i2c_client *client,
+					 u8 trickle_cfg)
+{
+	int err;
+
+	/*
+	 * Write the configuration key register to enable access to the Trickle
+	 * register
+	 */
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY,
+					ABX8XX_CFG_KEY_MISC);
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write configuration key\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_TRICKLE,
+					ABX8XX_TRICKLE_CHARGE_ENABLE |
+					trickle_cfg);
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write trickle register\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned char date[8];
+	int err;
+
+	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_HTH,
+					    sizeof(date), date);
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to read date\n");
+		return -EIO;
+	}
+
+	tm->tm_sec = bcd2bin(date[ABX8XX_REG_SC] & 0x7F);
+	tm->tm_min = bcd2bin(date[ABX8XX_REG_MN] & 0x7F);
+	tm->tm_hour = bcd2bin(date[ABX8XX_REG_HR] & 0x3F);
+	tm->tm_wday = date[ABX8XX_REG_WD] & 0x7;
+	tm->tm_mday = bcd2bin(date[ABX8XX_REG_DA] & 0x3F);
+	tm->tm_mon = bcd2bin(date[ABX8XX_REG_MO] & 0x1F) - 1;
+	tm->tm_year = bcd2bin(date[ABX8XX_REG_YR]);
+	if (tm->tm_year < 70)
+		tm->tm_year += 100;
+
+	err = rtc_valid_tm(tm);
+	if (err < 0)
+		dev_err(&client->dev, "retrieved date/time is not valid.\n");
+
+	return err;
+}
+
+static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int data, err;
+	unsigned char buf[8];
+
+	buf[ABX8XX_REG_SC] = bin2bcd(tm->tm_sec);
+	buf[ABX8XX_REG_MN] = bin2bcd(tm->tm_min);
+	buf[ABX8XX_REG_HR] = bin2bcd(tm->tm_hour);
+	buf[ABX8XX_REG_DA] = bin2bcd(tm->tm_mday);
+	buf[ABX8XX_REG_MO] = bin2bcd(tm->tm_mon + 1);
+	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year % 100);
+	buf[ABX8XX_REG_WD] = (0x1 << tm->tm_wday);
+
+	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_CTRL1);
+	if (data < 0) {
+		dev_err(&client->dev, "Unable to read control register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
+					(data | ABX8XX_CTRL_WRITE));
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write control register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_SC, 7,
+					     &buf[ABX8XX_REG_SC]);
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write to date registers\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
+					(data & ~ABX8XX_CTRL_WRITE));
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write control register\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static const struct rtc_class_ops abx80x_rtc_ops = {
+	.read_time	= abx80x_rtc_read_time,
+	.set_time	= abx80x_rtc_set_time,
+};
+
+
+static int abx80x_dt_trickle_cfg(struct device_node *np)
+{
+	const char *diode;
+	int trickle_cfg = 0;
+	int i, ret;
+	u32 tmp;
+
+	ret = of_property_read_string(np, "abracon,tc-diode", &diode);
+	if (ret)
+		return ret;
+
+	if (!strcmp(diode, "standard"))
+		trickle_cfg |= ABX8XX_TRICKLE_STANDARD_DIODE;
+	else if (!strcmp(diode, "schottky"))
+		trickle_cfg |= ABX8XX_TRICKLE_SCHOTTKY_DIODE;
+	else
+		return -EINVAL;
+
+	ret = of_property_read_u32(np, "abracon,tc-resistor", &tmp);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < sizeof(trickle_resistors); i++)
+		if (trickle_resistors[i] == tmp)
+			break;
+
+	if (i == sizeof(trickle_resistors))
+		return -EINVAL;
+
+	return (trickle_cfg | i);
+}
+
+static int abx80x_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device_node *np = client->dev.of_node;
+	struct rtc_device *rtc;
+	int i, data, err, trickle_cfg = -EINVAL;
+	char buf[7];
+	unsigned int part = id->driver_data;
+	unsigned int partnumber;
+	unsigned int majrev, minrev;
+	unsigned int lot;
+	unsigned int wafer;
+	unsigned int uid;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ID0,
+					    sizeof(buf), buf);
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to read date\n");
+		return -EIO;
+	}
+
+	partnumber = (buf[0] << 8) | buf[1];
+	majrev = buf[2] >> 3;
+	minrev = buf[2] & 0x7;
+	lot = ((buf[4] & 0x80) << 2) | ((buf[6] & 0x80) << 1) | buf[3];
+	uid = ((buf[4] & 0x7f) << 8) | buf[5];
+	wafer = (buf[6] & 0x7c) >> 2;
+	dev_info(&client->dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n",
+			partnumber, majrev, minrev, lot, wafer, uid);
+
+	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_CTRL1);
+	if (data < 0) {
+		dev_err(&client->dev, "Unable to read control register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
+					(data & ~ABX8XX_CTRL_12_24));
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write control register\n");
+		return -EIO;
+	}
+
+	/* part autodetection */
+	if (part == ABX80X) {
+		for (i = 0; abx80x_caps[i].pn; i++)
+			if (partnumber == abx80x_caps[i].pn)
+				break;
+		if (abx80x_caps[i].pn == 0) {
+			dev_err(&client->dev, "Unknown part: %04x\n",
+				partnumber);
+			return -EINVAL;
+		}
+		part = i;
+	}
+
+	if (partnumber != abx80x_caps[part].pn) {
+		dev_err(&client->dev, "partnumber mismatch %04x != %04x\n",
+			partnumber, abx80x_caps[part].pn);
+		return -EINVAL;
+	}
+
+	if (np && abx80x_caps[part].has_tc)
+		trickle_cfg = abx80x_dt_trickle_cfg(np);
+
+	if (trickle_cfg > 0) {
+		dev_info(&client->dev, "Enabling trickle charger: %02x\n",
+			 trickle_cfg);
+		abx80x_enable_trickle_charger(client, trickle_cfg);
+	}
+
+	rtc = devm_rtc_device_register(&client->dev, abx80x_driver.driver.name,
+				       &abx80x_rtc_ops, THIS_MODULE);
+
+	if (IS_ERR(rtc))
+		return PTR_ERR(rtc);
+
+	i2c_set_clientdata(client, rtc);
+
+	return 0;
+}
+
+static int abx80x_remove(struct i2c_client *client)
+{
+	return 0;
+}
+
+static const struct i2c_device_id abx80x_id[] = {
+	{ "abx80x", ABX80X },
+	{ "ab0801", AB0801 },
+	{ "ab0803", AB0803 },
+	{ "ab0804", AB0804 },
+	{ "ab0805", AB0805 },
+	{ "ab1801", AB1801 },
+	{ "ab1803", AB1803 },
+	{ "ab1804", AB1804 },
+	{ "ab1805", AB1805 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, abx80x_id);
+
+static struct i2c_driver abx80x_driver = {
+	.driver		= {
+		.name	= "rtc-abx80x",
+	},
+	.probe		= abx80x_probe,
+	.remove		= abx80x_remove,
+	.id_table	= abx80x_id,
+};
+
+module_i2c_driver(abx80x_driver);
+
+MODULE_AUTHOR("Philippe De Muyter <phdm@macqel.be>");
+MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
+MODULE_DESCRIPTION("Abracon ABX80X RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.1.0

Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

[Philippe De Muyter]

Hi Alexandre,

Thanks for the merge.  I have however some comments.  They are not meant
to be exhaustive.

Philippe

On Sat, Mar 14, 2015 at 11:05:46AM +0100, Alexandre Belloni wrote:
> From: Philippe De Muyter <phdm@macqel.be>
> 
> This is a basic driver for the ultra-low-power Abracon AB x80x series of RTC
> chips. It supports in particular, the supersets AB0805 and AB1805.
> It allows reading and writing the time, and enables the supercapacitor/
> battery charger.
> 
> [arnd@arndb.de: abx805 depends on i2c]
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>  drivers/rtc/Kconfig      |  10 ++
>  drivers/rtc/Makefile     |   1 +
>  drivers/rtc/rtc-abx80x.c | 325 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/rtc/rtc-abx80x.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index b5b5c3d485d6..89507c1858ec 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -164,6 +164,16 @@ config RTC_DRV_ABB5ZES3
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-ab-b5ze-s3.
>  
> +config RTC_DRV_ABX80X
> +	tristate "Abracon ABx80x"
> +	help
> +	  If you say yes here you get support for Abracon AB080X and AB180X
> +	  families of ultra-low-power  battery- and capacitor-backed real-time
> +	  clock chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-abx80x.
> +
>  config RTC_DRV_AS3722
>  	tristate "ams AS3722 RTC driver"
>  	depends on MFD_AS3722
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 69c87062b098..7b20b0462cb6 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
>  obj-$(CONFIG_RTC_DRV_AB3100)	+= rtc-ab3100.o
>  obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
>  obj-$(CONFIG_RTC_DRV_ABB5ZES3)	+= rtc-ab-b5ze-s3.o
> +obj-$(CONFIG_RTC_DRV_ABX80X)	+= rtc-abx80x.o
>  obj-$(CONFIG_RTC_DRV_ARMADA38X)	+= rtc-armada38x.o
>  obj-$(CONFIG_RTC_DRV_AS3722)	+= rtc-as3722.o
>  obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> new file mode 100644
> index 000000000000..8424e500f1da
> --- /dev/null
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -0,0 +1,325 @@
> +/*
> + * A driver for the I2C members of the Abracon AB x8xx RTC family,
> + * and compatible: AB 1805 and AB 0805
> + *
> + * Copyright 2014-2015 Macq S.A.
> + *
> + * Author: Philippe De Muyter <phdm@macqel.be>
> + * Author: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/bcd.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/rtc.h>
> +
> +#define ABX8XX_REG_HTH		0x00
> +#define ABX8XX_REG_SC		0x01
> +#define ABX8XX_REG_MN		0x02
> +#define ABX8XX_REG_HR		0x03
> +#define ABX8XX_REG_DA		0x04
> +#define ABX8XX_REG_MO		0x05
> +#define ABX8XX_REG_YR		0x06
> +#define ABX8XX_REG_WD		0x07
> +
> +#define ABX8XX_REG_CTRL1	0x10
> +#define ABX8XX_CTRL_WRITE	BIT(1)
> +#define ABX8XX_CTRL_12_24	BIT(6)
> +
> +#define ABX8XX_REG_CFG_KEY	0x1f
> +#define ABX8XX_CFG_KEY_MISC	0x9d
> +
> +#define ABX8XX_REG_ID0		0x28
> +
> +#define ABX8XX_REG_TRICKLE	0x20
> +#define ABX8XX_TRICKLE_CHARGE_ENABLE	0xa0
> +#define ABX8XX_TRICKLE_STANDARD_DIODE	0x8
> +#define ABX8XX_TRICKLE_SCHOTTKY_DIODE	0x4
> +
> +static u8 trickle_resistors[] = {0, 3, 6, 11};
> +
> +enum abx80x_chip {AB0801, AB0803, AB0804, AB0805,
> +	AB1801, AB1803, AB1804, AB1805, ABX80X};
> +
> +struct abx80x_cap {
> +	u16 pn;
> +	bool has_tc;
> +};
> +
> +static struct abx80x_cap abx80x_caps[] = {
> +	[AB0801] = {.pn = 0x0801},
> +	[AB0803] = {.pn = 0x0803},
> +	[AB0804] = {.pn = 0x0804, .has_tc = true},
> +	[AB0805] = {.pn = 0x0805, .has_tc = true},
> +	[AB1801] = {.pn = 0x1801},
> +	[AB1803] = {.pn = 0x1803},
> +	[AB1804] = {.pn = 0x1804, .has_tc = true},
> +	[AB1805] = {.pn = 0x1805, .has_tc = true},
> +	[ABX80X] = {.pn = 0}
> +};
> +
> +static struct i2c_driver abx80x_driver;
> +
> +static int abx80x_enable_trickle_charger(struct i2c_client *client,
> +					 u8 trickle_cfg)
> +{
> +	int err;
> +
> +	/*
> +	 * Write the configuration key register to enable access to the Trickle
> +	 * register
> +	 */
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY,
> +					ABX8XX_CFG_KEY_MISC);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write configuration key\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_TRICKLE,
> +					ABX8XX_TRICKLE_CHARGE_ENABLE |
> +					trickle_cfg);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write trickle register\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned char date[8];
> +	int err;
> +
> +	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_HTH,
> +					    sizeof(date), date);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to read date\n");
> +		return -EIO;
> +	}
> +
> +	tm->tm_sec = bcd2bin(date[ABX8XX_REG_SC] & 0x7F);
> +	tm->tm_min = bcd2bin(date[ABX8XX_REG_MN] & 0x7F);
> +	tm->tm_hour = bcd2bin(date[ABX8XX_REG_HR] & 0x3F);
> +	tm->tm_wday = date[ABX8XX_REG_WD] & 0x7;
> +	tm->tm_mday = bcd2bin(date[ABX8XX_REG_DA] & 0x3F);
> +	tm->tm_mon = bcd2bin(date[ABX8XX_REG_MO] & 0x1F) - 1;
> +	tm->tm_year = bcd2bin(date[ABX8XX_REG_YR]);
> +	if (tm->tm_year < 70)

Is that still useful for a driver written in 2015 ?

> +		tm->tm_year += 100;
> +
> +	err = rtc_valid_tm(tm);
> +	if (err < 0)
> +		dev_err(&client->dev, "retrieved date/time is not valid.\n");
> +
> +	return err;
> +}
> +
> +static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	int data, err;
> +	unsigned char buf[8];

The link is not clear between 8 above, the symbolic constants below, and 7
in the call to i2c_smbus_write_i2c_block_data.

> +
> +	buf[ABX8XX_REG_SC] = bin2bcd(tm->tm_sec);
> +	buf[ABX8XX_REG_MN] = bin2bcd(tm->tm_min);
> +	buf[ABX8XX_REG_HR] = bin2bcd(tm->tm_hour);
> +	buf[ABX8XX_REG_DA] = bin2bcd(tm->tm_mday);
> +	buf[ABX8XX_REG_MO] = bin2bcd(tm->tm_mon + 1);
> +	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year % 100);
> +	buf[ABX8XX_REG_WD] = (0x1 << tm->tm_wday);

That should be :

	buf[ABX8XX_REG_WD] = tm->tm_wday;

> +
> +	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_CTRL1);
> +	if (data < 0) {
> +		dev_err(&client->dev, "Unable to read control register\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
> +					(data | ABX8XX_CTRL_WRITE));
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write control register\n");
> +		return -EIO;
> +	}

There is no need to enable/disable write access for each write.

> +
> +	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_SC, 7,
> +					     &buf[ABX8XX_REG_SC]);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write to date registers\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
> +					(data & ~ABX8XX_CTRL_WRITE));
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write control register\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops abx80x_rtc_ops = {
> +	.read_time	= abx80x_rtc_read_time,
> +	.set_time	= abx80x_rtc_set_time,
> +};
> +
> +
> +static int abx80x_dt_trickle_cfg(struct device_node *np)
> +{
> +	const char *diode;
> +	int trickle_cfg = 0;
> +	int i, ret;
> +	u32 tmp;
> +
> +	ret = of_property_read_string(np, "abracon,tc-diode", &diode);
> +	if (ret)
> +		return ret;
> +
> +	if (!strcmp(diode, "standard"))
> +		trickle_cfg |= ABX8XX_TRICKLE_STANDARD_DIODE;
> +	else if (!strcmp(diode, "schottky"))
> +		trickle_cfg |= ABX8XX_TRICKLE_SCHOTTKY_DIODE;
> +	else
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(np, "abracon,tc-resistor", &tmp);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < sizeof(trickle_resistors); i++)
> +		if (trickle_resistors[i] == tmp)
> +			break;
> +
> +	if (i == sizeof(trickle_resistors))
> +		return -EINVAL;
> +
> +	return (trickle_cfg | i);
> +}
> +
> +static int abx80x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct rtc_device *rtc;
> +	int i, data, err, trickle_cfg = -EINVAL;
> +	char buf[7];
> +	unsigned int part = id->driver_data;
> +	unsigned int partnumber;
> +	unsigned int majrev, minrev;
> +	unsigned int lot;
> +	unsigned int wafer;
> +	unsigned int uid;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ID0,
> +					    sizeof(buf), buf);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to read date\n");

date ? Rather partnumber, or identification, or model, or ...

> +		return -EIO;
> +	}
> +
> +	partnumber = (buf[0] << 8) | buf[1];
> +	majrev = buf[2] >> 3;
> +	minrev = buf[2] & 0x7;
> +	lot = ((buf[4] & 0x80) << 2) | ((buf[6] & 0x80) << 1) | buf[3];
> +	uid = ((buf[4] & 0x7f) << 8) | buf[5];
> +	wafer = (buf[6] & 0x7c) >> 2;
> +	dev_info(&client->dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n",
> +			partnumber, majrev, minrev, lot, wafer, uid);
> +
> +	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_CTRL1);
> +	if (data < 0) {
> +		dev_err(&client->dev, "Unable to read control register\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CTRL1,
> +					(data & ~ABX8XX_CTRL_12_24));

Let's disable the write-protection here.

> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write control register\n");
> +		return -EIO;
> +	}
> +
> +	/* part autodetection */
> +	if (part == ABX80X) {
> +		for (i = 0; abx80x_caps[i].pn; i++)
> +			if (partnumber == abx80x_caps[i].pn)
> +				break;
> +		if (abx80x_caps[i].pn == 0) {
> +			dev_err(&client->dev, "Unknown part: %04x\n",
> +				partnumber);
> +			return -EINVAL;
> +		}
> +		part = i;
> +	}
> +
> +	if (partnumber != abx80x_caps[part].pn) {
> +		dev_err(&client->dev, "partnumber mismatch %04x != %04x\n",
> +			partnumber, abx80x_caps[part].pn);
> +		return -EINVAL;
> +	}
> +
> +	if (np && abx80x_caps[part].has_tc)
> +		trickle_cfg = abx80x_dt_trickle_cfg(np);
> +
> +	if (trickle_cfg > 0) {
> +		dev_info(&client->dev, "Enabling trickle charger: %02x\n",
> +			 trickle_cfg);
> +		abx80x_enable_trickle_charger(client, trickle_cfg);
> +	}
> +
> +	rtc = devm_rtc_device_register(&client->dev, abx80x_driver.driver.name,
> +				       &abx80x_rtc_ops, THIS_MODULE);
> +
> +	if (IS_ERR(rtc))
> +		return PTR_ERR(rtc);
> +
> +	i2c_set_clientdata(client, rtc);
> +
> +	return 0;
> +}
> +
> +static int abx80x_remove(struct i2c_client *client)
> +{
> +	return 0;
> +}
> +
> +static const struct i2c_device_id abx80x_id[] = {
> +	{ "abx80x", ABX80X },
> +	{ "ab0801", AB0801 },
> +	{ "ab0803", AB0803 },
> +	{ "ab0804", AB0804 },
> +	{ "ab0805", AB0805 },
> +	{ "ab1801", AB1801 },
> +	{ "ab1803", AB1803 },
> +	{ "ab1804", AB1804 },
> +	{ "ab1805", AB1805 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, abx80x_id);
> +
> +static struct i2c_driver abx80x_driver = {
> +	.driver		= {
> +		.name	= "rtc-abx80x",
> +	},
> +	.probe		= abx80x_probe,
> +	.remove		= abx80x_remove,
> +	.id_table	= abx80x_id,
> +};
> +
> +module_i2c_driver(abx80x_driver);
> +
> +MODULE_AUTHOR("Philippe De Muyter <phdm@macqel.be>");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
> +MODULE_DESCRIPTION("Abracon ABX80X RTC driver");
> +MODULE_LICENSE("GPL");

I'm fine with 'GPL', but someone suggested to change that to 'GPL v2' in my patch.
> -- 
> 2.1.0

Best regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

[Alexandre Belloni]

On 14/03/2015 at 13:44:41 +0100, Philippe De Muyter wrote :
> > +	tm->tm_sec = bcd2bin(date[ABX8XX_REG_SC] & 0x7F);
> > +	tm->tm_min = bcd2bin(date[ABX8XX_REG_MN] & 0x7F);
> > +	tm->tm_hour = bcd2bin(date[ABX8XX_REG_HR] & 0x3F);
> > +	tm->tm_wday = date[ABX8XX_REG_WD] & 0x7;
> > +	tm->tm_mday = bcd2bin(date[ABX8XX_REG_DA] & 0x3F);
> > +	tm->tm_mon = bcd2bin(date[ABX8XX_REG_MO] & 0x1F) - 1;
> > +	tm->tm_year = bcd2bin(date[ABX8XX_REG_YR]);
> > +	if (tm->tm_year < 70)
> 
> Is that still useful for a driver written in 2015 ?
> 

I'd say that this is actually the only correct way to do it. Only dates
before 01/01/1970 00:00 are considered invalid. So, unless adding a
check like:

if (tm->tm_year < 100)
	return -EINVAL;

in abx80x_rtc_set_time, setting and then reading a date before 2000 will
fail silently. I'm open to add that check.

> > +static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	int data, err;
> > +	unsigned char buf[8];
> 
> The link is not clear between 8 above, the symbolic constants below, and 7
> in the call to i2c_smbus_write_i2c_block_data.
> 

It is because I didn't bother writing the hundreth of seconds, I'll
change that and the other issues you pointed.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

[Philippe De Muyter]

On Sat, Mar 14, 2015 at 07:09:11PM +0100, Alexandre Belloni wrote:
> On 14/03/2015 at 13:44:41 +0100, Philippe De Muyter wrote :
> > > +	tm->tm_sec = bcd2bin(date[ABX8XX_REG_SC] & 0x7F);
> > > +	tm->tm_min = bcd2bin(date[ABX8XX_REG_MN] & 0x7F);
> > > +	tm->tm_hour = bcd2bin(date[ABX8XX_REG_HR] & 0x3F);
> > > +	tm->tm_wday = date[ABX8XX_REG_WD] & 0x7;
> > > +	tm->tm_mday = bcd2bin(date[ABX8XX_REG_DA] & 0x3F);
> > > +	tm->tm_mon = bcd2bin(date[ABX8XX_REG_MO] & 0x1F) - 1;
> > > +	tm->tm_year = bcd2bin(date[ABX8XX_REG_YR]);
> > > +	if (tm->tm_year < 70)
> > 
> > Is that still useful for a driver written in 2015 ?
> > 
> 
> I'd say that this is actually the only correct way to do it. Only dates
> before 01/01/1970 00:00 are considered invalid. So, unless adding a
> check like:
> 
> if (tm->tm_year < 100)
> 	return -EINVAL;
> 
> in abx80x_rtc_set_time, setting and then reading a date before 2000 will
> fail silently. I'm open to add that check.

You are right.  It is actually more a consistency problem between rtc
drivers and thus a question for the rtc-subsytem maintainer.

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc

[Paul Bolle]

On Sat, 2015-03-14 at 13:44 +0100, Philippe De Muyter wrote:
> On Sat, Mar 14, 2015 at 11:05:46AM +0100, Alexandre Belloni wrote:
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-abx80x.c
> > +/*
> > + * A driver for the I2C members of the Abracon AB x8xx RTC family,
> > + * and compatible: AB 1805 and AB 0805
> > + *
> > + * Copyright 2014-2015 Macq S.A.
> > + *
> > + * Author: Philippe De Muyter <phdm@macqel.be>
> > + * Author: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > 
> > +MODULE_LICENSE("GPL");
> 
> I'm fine with 'GPL', but someone suggested to change that to 'GPL v2' in my patch.

That someone was me. And my reasoning is that "GPL" is documented to
mean "GPL v2 or later", while "GPL v2" is documented to mean just that.
So "GPL v2" would match what is stated at the comment at the top of this
file.

Thanks,


Paul Bolle