| Message ID | 20150302155337.7876cfb631552427187c4c0a@linux-foundation.org |
|---|---|
| State | Superseded |
| Headers | show |
On 02/03/2015 at 15:53:37 -0800, Andrew Morton wrote : > On Sun, 1 Mar 2015 11:27:15 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > Add support for the i2c RTC from Abracon. > > What is the relationship between this patch and > http://ozlabs.org/~akpm/mmots/broken-out/rtc-add-rtc-abx805-a-driver-for-the-abracon-ab-1805-i2c-rtc.patch? > I'd say drop mine, I couldn't find the other one before writing it... I'll try to build on Philippe's driver.
On Tue, 3 Mar 2015 02:11:16 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 02/03/2015 at 15:53:37 -0800, Andrew Morton wrote : > > On Sun, 1 Mar 2015 11:27:15 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > Add support for the i2c RTC from Abracon. > > > > What is the relationship between this patch and > > http://ozlabs.org/~akpm/mmots/broken-out/rtc-add-rtc-abx805-a-driver-for-the-abracon-ab-1805-i2c-rtc.patch? > > > > I'd say drop mine, I couldn't find the other one before writing it... > > I'll try to build on Philippe's driver. Which driver supports the most devices? Your driver has +static const struct i2c_device_id abx80x_id[] = { + { "abx80x", ABX80X }, + { "ab0801", AB0801 }, + { "ab0802", AB0802 }, + { "ab0803", AB0803 }, + { "ab0804", AB0804 }, + { "ab0805", AB0805 }, + { "ab1801", AB1801 }, + { "ab1802", AB1802 }, + { "ab1803", AB1803 }, + { "ab1804", AB1804 }, + { "ab1805", AB1805 }, + { } +}; And Philippe's has +static struct i2c_device_id abx805_id[] = { + { "abx805-rtc", 0 }, + { } +}; And is the naming in Philippe's driver appropriate? If it supports the AB1801 (for example) then why is it described as an "abx805" driver?
On 03/03/2015 at 12:20:12 -0800, Andrew Morton wrote : > On Tue, 3 Mar 2015 02:11:16 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > On 02/03/2015 at 15:53:37 -0800, Andrew Morton wrote : > > > On Sun, 1 Mar 2015 11:27:15 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > > > Add support for the i2c RTC from Abracon. > > > > > > What is the relationship between this patch and > > > http://ozlabs.org/~akpm/mmots/broken-out/rtc-add-rtc-abx805-a-driver-for-the-abracon-ab-1805-i2c-rtc.patch? > > > > > > > I'd say drop mine, I couldn't find the other one before writing it... > > > > I'll try to build on Philippe's driver. > > Which driver supports the most devices? Your driver has > > +static const struct i2c_device_id abx80x_id[] = { > + { "abx80x", ABX80X }, > + { "ab0801", AB0801 }, > + { "ab0802", AB0802 }, > + { "ab0803", AB0803 }, > + { "ab0804", AB0804 }, > + { "ab0805", AB0805 }, > + { "ab1801", AB1801 }, > + { "ab1802", AB1802 }, > + { "ab1803", AB1803 }, > + { "ab1804", AB1804 }, > + { "ab1805", AB1805 }, > + { } > +}; > > And Philippe's has > > +static struct i2c_device_id abx805_id[] = { > + { "abx805-rtc", 0 }, > + { } > +}; > > > And is the naming in Philippe's driver appropriate? If it supports the > AB1801 (for example) then why is it described as an "abx805" driver? The real naming is in the form ABx8yz. With: x: 0 or 1, indicate the presence of the reset management y: 0 or 1: 0 is i2c, 1 is spi z: [1-5]: different amount of on chip SRAM. From what I understand, only ABx8y5 are actually recommended for new designs. They all share the same register set, apart from the reset management that is only available on AB18yz. From my point of view, but I'm obviously biased, I'd take my patch because it declares and supports all the available rtc and it also uses the i2c_smbus_xxx API that is more robust than the raw i2c_transfer. I also take care of the 12/24 mode bit and the write RTC bit which is necessary to be able to write to the RTC. What I like in Philippe's driver is the info printed at probe time and the support for trickle charging. However, I wouldn't enable it unconditionally. To move forward, I can either send follow up patches based on what Philippe did. Or I can merge features from Philippe in my driver in a new patch, keeping his authorship. Regards,
On Tue, Mar 03, 2015 at 09:50:32PM +0100, Alexandre Belloni wrote: > On 03/03/2015 at 12:20:12 -0800, Andrew Morton wrote : > > On Tue, 3 Mar 2015 02:11:16 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > On 02/03/2015 at 15:53:37 -0800, Andrew Morton wrote : > > > > On Sun, 1 Mar 2015 11:27:15 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > > > > > Add support for the i2c RTC from Abracon. > > > > > > > > What is the relationship between this patch and > > > > http://ozlabs.org/~akpm/mmots/broken-out/rtc-add-rtc-abx805-a-driver-for-the-abracon-ab-1805-i2c-rtc.patch? > > > > > > > > > > I'd say drop mine, I couldn't find the other one before writing it... > > > > > > I'll try to build on Philippe's driver. > > > > Which driver supports the most devices? Your driver has > > > > +static const struct i2c_device_id abx80x_id[] = { > > + { "abx80x", ABX80X }, > > + { "ab0801", AB0801 }, > > + { "ab0802", AB0802 }, > > + { "ab0803", AB0803 }, > > + { "ab0804", AB0804 }, > > + { "ab0805", AB0805 }, > > + { "ab1801", AB1801 }, > > + { "ab1802", AB1802 }, > > + { "ab1803", AB1803 }, > > + { "ab1804", AB1804 }, > > + { "ab1805", AB1805 }, > > + { } > > +}; > > > > And Philippe's has > > > > +static struct i2c_device_id abx805_id[] = { > > + { "abx805-rtc", 0 }, The only part my driver is actually tested with, is the 'AB1805', but see below. > > + { } > > +}; > > > > > > And is the naming in Philippe's driver appropriate? If it supports the > > AB1801 (for example) then why is it described as an "abx805" driver? > > The real naming is in the form ABx8yz. With: > > x: 0 or 1, indicate the presence of the reset management > y: 0 or 1: 0 is i2c, 1 is spi > z: [1-5]: different amount of on chip SRAM. From what I understand, only > ABx8y5 are actually recommended for new designs. My driver is based on the datashet called 'AB18X5 Real-Time Clock with Power Management Family', which calls the parts 'AB18X5 family' with X being '0' for I2c parts and '1' for SPI parts. As the part I have and the driver I wrote are I2C, not SPI, I fixed the 'X' as 0 in the name of the driver. The datasheet lists only one part as being 'software and pin compatible' with the 'AB1805': the 'AB0805', hence the 'x' in the name I choose : abx805. > > They all share the same register set, apart from the reset management > that is only available on AB18yz. > > >From my point of view, but I'm obviously biased, I'd take my patch > because it declares and supports all the available rtc and it also uses > the i2c_smbus_xxx API that is more robust than the raw i2c_transfer. I have no opinion on that. The driver I started from uses 'raw i2c_transfer'. > > I also take care of the 12/24 mode bit and the write RTC bit which is > necessary to be able to write to the RTC. Actually, my driver is used in production and works fine, because the default(reset) value of the 12/24 mode bit is '24 hour mode' and the default value of the 'write RTC bit' enables writing. There is no real need to play with them. > > What I like in Philippe's driver is the info printed at probe time and > the support for trickle charging. However, I wouldn't enable it > unconditionally. My hardware colleagues told me that the only way to enable the 'ultra low-power' functionality is enabling the trickle charger. And the 'ultra low-power' functionality is the reason we choose that chip, so I would at least keep that as the default behaviour. Regards Philippe
On 04/03/2015 at 09:52:42 +0100, Philippe De Muyter wrote : > > > And is the naming in Philippe's driver appropriate? If it supports the > > > AB1801 (for example) then why is it described as an "abx805" driver? > > > > The real naming is in the form ABx8yz. With: > > > > x: 0 or 1, indicate the presence of the reset management > > y: 0 or 1: 0 is i2c, 1 is spi > > z: [1-5]: different amount of on chip SRAM. From what I understand, only > > ABx8y5 are actually recommended for new designs. > > My driver is based on the datashet called 'AB18X5 Real-Time Clock with > Power Management Family', which calls the parts 'AB18X5 family' with X > being '0' for I2c parts and '1' for SPI parts. > > As the part I have and the driver I wrote are I2C, not SPI, I fixed the > 'X' as 0 in the name of the driver. > > The datasheet lists only one part as being 'software and pin compatible' > with the 'AB1805': the 'AB0805', hence the 'x' in the name I choose : abx805. > The "AB08XX Real-Time Clock Family" document states that they are all software and pin compatible (including the AB18xx). > Actually, my driver is used in production and works fine, because the > default(reset) value of the 12/24 mode bit is '24 hour mode' and the > default value of the 'write RTC bit' enables writing. There is > no real need to play with them. > I know this is unlikely to happen but what if someone messes with the RTC in the bootloader? > > > > What I like in Philippe's driver is the info printed at probe time and > > the support for trickle charging. However, I wouldn't enable it > > unconditionally. > > My hardware colleagues told me that the only way to enable the 'ultra low-power' > functionality is enabling the trickle charger. And the 'ultra low-power' > functionality is the reason we choose that chip, so I would at least > keep that as the default behaviour. > My concern is that you have a static configuration. I would expose a sysfs interface to configure the diode, resistor and enable/disable the trickle charger. Would that work for you?
Hi Alexandre, On Wed, Mar 04, 2015 at 10:06:01AM +0100, Alexandre Belloni wrote: > On 04/03/2015 at 09:52:42 +0100, Philippe De Muyter wrote : > > > > And is the naming in Philippe's driver appropriate? If it supports the > > > > AB1801 (for example) then why is it described as an "abx805" driver? > > > > > > The real naming is in the form ABx8yz. With: > > > > > > x: 0 or 1, indicate the presence of the reset management > > > y: 0 or 1: 0 is i2c, 1 is spi > > > z: [1-5]: different amount of on chip SRAM. From what I understand, only > > > ABx8y5 are actually recommended for new designs. > > > > My driver is based on the datashet called 'AB18X5 Real-Time Clock with > > Power Management Family', which calls the parts 'AB18X5 family' with X > > being '0' for I2c parts and '1' for SPI parts. > > > > As the part I have and the driver I wrote are I2C, not SPI, I fixed the > > 'X' as 0 in the name of the driver. > > > > The datasheet lists only one part as being 'software and pin compatible' > > with the 'AB1805': the 'AB0805', hence the 'x' in the name I choose : abx805. > > > > The "AB08XX Real-Time Clock Family" document states that they are all > software and pin compatible (including the AB18xx). Which part(s) do you really have ? Mine is a 1805. Do all the parts have the trickle charger ? As I don't know, I prefer not to pretend that the driver supports those chips. > > > Actually, my driver is used in production and works fine, because the > > default(reset) value of the 12/24 mode bit is '24 hour mode' and the > > default value of the 'write RTC bit' enables writing. There is > > no real need to play with them. > > > > I know this is unlikely to happen but what if someone messes with the > RTC in the bootloader? Yes, you may unconditionnaly rewrite this register if you want. > > > > > > > What I like in Philippe's driver is the info printed at probe time and > > > the support for trickle charging. However, I wouldn't enable it > > > unconditionally. > > > > My hardware colleagues told me that the only way to enable the 'ultra low-power' > > functionality is enabling the trickle charger. And the 'ultra low-power' > > functionality is the reason we choose that chip, so I would at least > > keep that as the default behaviour. > > > > My concern is that you have a static configuration. I would expose a > sysfs interface to configure the diode, resistor and enable/disable the > trickle charger. Would that work for you? I think a 'of_xxx' dts/dtb description is the way to go, but I did not want to introduce unnecessaty complexity without knowing other usages. My hardware colleagues followed the recommended design from Abracon. Philippe
On 04/03/2015 at 10:55:56 +0100, Philippe De Muyter wrote : > > The "AB08XX Real-Time Clock Family" document states that they are all > > software and pin compatible (including the AB18xx). > > Which part(s) do you really have ? Mine is a 1805. Do all the parts have > the trickle charger ? As I don't know, I prefer not to pretend that the > driver supports those chips. > I have the AB0805. The trickle charger is not available on ABx8x1 and ABx8x3. > > > My hardware colleagues told me that the only way to enable the 'ultra low-power' > > > functionality is enabling the trickle charger. And the 'ultra low-power' > > > functionality is the reason we choose that chip, so I would at least > > > keep that as the default behaviour. > > > > > > > My concern is that you have a static configuration. I would expose a > > sysfs interface to configure the diode, resistor and enable/disable the > > trickle charger. Would that work for you? > > I think a 'of_xxx' dts/dtb description is the way to go, but I did not want to > introduce unnecessaty complexity without knowing other usages. My hardware > colleagues followed the recommended design from Abracon. > Yeah, I'm not sure about the DT description, some may argue this is configuration. But I guess it actually depends on the battery you are using so that could count as hardware description. I'll use: abracon,tc-diode = "(standard|schottky)" abracon,tc-resistor = <(0|3|6|11)> If both are defined and valid, I'll enable the trickle charger with the provided configuration in probe().
Hi Alexandre, On Wed, Mar 04, 2015 at 11:38:14AM +0100, Alexandre Belloni wrote: > On 04/03/2015 at 10:55:56 +0100, Philippe De Muyter wrote : > > > The "AB08XX Real-Time Clock Family" document states that they are all > > > software and pin compatible (including the AB18xx). > > > > Which part(s) do you really have ? Mine is a 1805. Do all the parts have > > the trickle charger ? As I don't know, I prefer not to pretend that the > > driver supports those chips. > > > > I have the AB0805. The trickle charger is not available on ABx8x1 and > ABx8x3. > > > > > My hardware colleagues told me that the only way to enable the 'ultra low-power' > > > > functionality is enabling the trickle charger. And the 'ultra low-power' > > > > functionality is the reason we choose that chip, so I would at least > > > > keep that as the default behaviour. > > > > > > > > > > My concern is that you have a static configuration. I would expose a > > > sysfs interface to configure the diode, resistor and enable/disable the > > > trickle charger. Would that work for you? > > > > I think a 'of_xxx' dts/dtb description is the way to go, but I did not want to > > introduce unnecessaty complexity without knowing other usages. My hardware > > colleagues followed the recommended design from Abracon. > > > > Yeah, I'm not sure about the DT description, some may argue this is > configuration. But I guess it actually depends on the battery you are > using so that could count as hardware description. As far as I know, it's purely hardware related. > > I'll use: > abracon,tc-diode = "(standard|schottky)" > abracon,tc-resistor = <(0|3|6|11)> > > If both are defined and valid, I'll enable the trickle charger with the > provided configuration in probe(). My current entry is : &i2c2 { ... ab1805@69 { compatible = "abracon,abx805-rtc"; reg = <0x69>; position = <3>; }; }; It would then become : &i2c2 { ... ab1805@69 { compatible = "abracon,abx805-rtc"; reg = <0x69>; position = <3>; abracon,tc-diode = "schottky"; abracon,tc-resistor = <3>; }; }; or if you change the driver's name to "abx80x-rtc" compatible = "abracon,abx80x-rtc"; That's fine for me. Thanks ! Philippe
diff -puN drivers/rtc/Kconfig~rtc-add-abracon-abx80x-driver drivers/rtc/Kconfig --- a/drivers/rtc/Kconfig~rtc-add-abracon-abx80x-driver +++ a/drivers/rtc/Kconfig @@ -164,6 +164,15 @@ 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 + 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 -puN drivers/rtc/Makefile~rtc-add-abracon-abx80x-driver drivers/rtc/Makefile --- a/drivers/rtc/Makefile~rtc-add-abracon-abx80x-driver +++ a/drivers/rtc/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_RTC_DRV_AB3100) += rtc-ab31 obj-$(CONFIG_RTC_DRV_AB8500) += rtc-ab8500.o obj-$(CONFIG_RTC_DRV_ABB5ZES3) += rtc-ab-b5ze-s3.o obj-$(CONFIG_RTC_DRV_ABX805) += rtc-abx805.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 -puN /dev/null drivers/rtc/rtc-abx80x.c --- /dev/null +++ a/drivers/rtc/rtc-abx80x.c @@ -0,0 +1,193 @@ +/* + * An I2C driver for the Abracon AB08xx + * + * 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/module.h> +#include <linux/i2c.h> +#include <linux/bcd.h> +#include <linux/rtc.h> +#include <linux/log2.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_REG_PART0 0x28 +#define ABX8XX_REG_PART1 0x29 + +#define ABX8XX_CTRL_WRITE BIT(1) +#define ABX8XX_CTRL_12_24 BIT(6) + +enum abx80x_chip {ABX80X, AB0801, AB0802, AB0803, AB0804, AB0805, + AB1801, AB1802, AB1803, AB1804, AB1805}; + +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, + 8, 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_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct rtc_device *rtc; + int data, err, part0, part1; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) + return -ENODEV; + + part0 = i2c_smbus_read_byte_data(client, ABX8XX_REG_PART0); + part1 = i2c_smbus_read_byte_data(client, ABX8XX_REG_PART1); + if ((part0 < 0) || (part1 < 0)) { + dev_err(&client->dev, "Unable to read part number\n"); + return -EIO; + } + dev_info(&client->dev, "chip found %02x%02x\n", part0, part1); + + 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; + } + + rtc = devm_rtc_device_register(&client->dev, "rtc-abx80x", + &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 }, + { "ab0802", AB0802 }, + { "ab0803", AB0803 }, + { "ab0804", AB0804 }, + { "ab0805", AB0805 }, + { "ab1801", AB1801 }, + { "ab1802", AB1802 }, + { "ab1803", AB1803 }, + { "ab1804", AB1804 }, + { "ab1805", AB1805 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, abx80x_id); + +static struct i2c_driver abx80x_driver = { + .driver = { + .name = "rtc-abx80x", + .owner = THIS_MODULE, + }, + .probe = abx80x_probe, + .remove = abx80x_remove, + .id_table = abx80x_id, +}; + +module_i2c_driver(abx80x_driver); + +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>"); +MODULE_DESCRIPTION("Abracon ABX80X RTC driver"); +MODULE_LICENSE("GPL");