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 <p...@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.
> 
> [a...@arndb.de: abx805 depends on i2c]
> Signed-off-by: Philippe De Muyter <p...@macqel.be>
> Cc: Alessandro Zummo <a.zu...@towertech.it>
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> Signed-off-by: Andrew Morton <a...@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 <p...@macqel.be>
> + * Author: Alexandre Belloni <alexandre.bell...@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 <p...@macqel.be>");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.bell...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to