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

From: Philippe De Muyter
Date: Sat Mar 14 2015 - 08:44:52 EST

Next message: Ameen Ali: "[PATCH 14/14] drivers/tty/vt/vt.c : access out-of-bounds"
Previous message: Jeff Layton: "Re: [PATCH v3 0/2] Use blocked_lock_lock only to protect blocked_hash"
In reply to: Alexandre Belloni: "[PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc"
Next in thread: Alexandre Belloni: "Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
>
> 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@xxxxxxxx: abx805 depends on i2c]
> Signed-off-by: Philippe De Muyter <phdm@xxxxxxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> + * Author: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxx>");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>");
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Ameen Ali: "[PATCH 14/14] drivers/tty/vt/vt.c : access out-of-bounds"
Previous message: Jeff Layton: "Re: [PATCH v3 0/2] Use blocked_lock_lock only to protect blocked_hash"
In reply to: Alexandre Belloni: "[PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc"
Next in thread: Alexandre Belloni: "Re: [PATCH 2/2] rtc: add rtc-abx80x, a driver for the Abracon AB x80x i2c rtc"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]