| Message ID | 20200924192455.2484005-6-j.neuschaefer@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Netronix embedded controller driver for Kobo and Tolino ebook readers | expand |
On Thu, 24 Sep 2020 21:24:53 +0200 Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > With this driver, mainline Linux can keep its time and date in sync with > the vendor kernel. > > Advanced functionality like alarm and automatic power-on is not yet > supported. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > > v3: > - Add email address to copyright line > - Remove OF compatible string and don't include linux/of_device.h > - Don't use a comma after sentinels > - Avoid ret |= ... pattern > - Move 8-bit register conversion to ntxec.h > - Relicense as GPLv2 or later > > v2: > - https://lore.kernel.org/lkml/20200905133230.1014581-7-j.neuschaefer@gmx.net/ > - Rework top-of-file comment [Lee Jones] > - Sort the #include lines [Alexandre Belloni] > - don't align = signs in struct initializers [Uwe Kleine-König] > - Switch to regmap > - Fix register number used to read minutes and seconds > - Prefix registers with NTXEC_REG_ > - Add help text to the Kconfig option > - Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max > --- > drivers/rtc/Kconfig | 8 +++ > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 141 insertions(+) > create mode 100644 drivers/rtc/rtc-ntxec.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 48c536acd777f..ae8f3dc36c9a3 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC > This driver can also be built as a module. If so, the module > will be called rtc-cros-ec. > > +config RTC_DRV_NTXEC > + tristate "Netronix embedded controller RTC driver" > + depends on MFD_NTXEC > + help > + Say yes here if you want to support the RTC functionality of the > + embedded controller found in certain e-book readers designed by the > + ODM Netronix. > + > comment "on-CPU RTC drivers" > > config RTC_DRV_ASM9260 > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index 880e08a409c3d..733479db18896 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o > obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o > obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o > +obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o > diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c > new file mode 100644 > index 0000000000000..af23c7cc76544 > --- /dev/null > +++ b/drivers/rtc/rtc-ntxec.c > @@ -0,0 +1,132 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * The Netronix embedded controller is a microcontroller found in some > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, > + * battery monitoring, system power management, and PWM functionality. > + * > + * This driver implements access to the RTC time and date. > + * > + * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net> > + */ > + > +#include <linux/mfd/ntxec.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/rtc.h> > +#include <linux/types.h> > + > +struct ntxec_rtc { > + struct device *dev; > + struct ntxec *ec; > +}; > + > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 > + > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year = (value >> 8) + 100; > + tm->tm_mon = (value & 0xff) - 1; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday = value >> 8; > + tm->tm_hour = value & 0xff; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min = value >> 8; > + tm->tm_sec = value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + int res = 0; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); > +} > + > +static const struct rtc_class_ops ntxec_rtc_ops = { > + .read_time = ntxec_read_time, > + .set_time = ntxec_set_time, > +}; > + > +static int ntxec_rtc_probe(struct platform_device *pdev) > +{ > + struct rtc_device *dev; > + struct ntxec_rtc *rtc; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + rtc->dev = &pdev->dev; > + rtc->ec = dev_get_drvdata(pdev->dev.parent); > + platform_set_drvdata(pdev, rtc); > + > + dev = devm_rtc_allocate_device(&pdev->dev); > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + > + dev->ops = &ntxec_rtc_ops; > + dev->range_min = RTC_TIMESTAMP_BEGIN_2000; > + dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */ > + > + return rtc_register_device(dev); > +} > + > +static struct platform_driver ntxec_rtc_driver = { > + .driver = { > + .name = "ntxec-rtc", > + }, > + .probe = ntxec_rtc_probe, > +}; > +module_platform_driver(ntxec_rtc_driver); > + I think module autoloading will not work without MODULE_ALIAS("platform:ntxec-rtc"); Same for the pwm device. > +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); > -- > 2.28.0 > > Regards, Andreas
Hello Jonathan, On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote: > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 Is this an official naming? I think at least ..._MS is a poor name. Maybe consider ..._MINSEC instead and make the other two names a bit longer for consistency? > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year = (value >> 8) + 100; > + tm->tm_mon = (value & 0xff) - 1; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday = value >> 8; > + tm->tm_hour = value & 0xff; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min = value >> 8; > + tm->tm_sec = value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + int res = 0; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); I wonder: Is this racy? If you write minute, does the seconds reset to zero or something like that? Or can it happen, that after writing the minute register and before writing the second register the seconds overflow and you end up with the time set to a minute later than intended? If so it might be worth to set the seconds to 0 at the start of the function (with an explaining comment). .read_time has a similar race. What happens if minutes overflow between reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > +static struct platform_driver ntxec_rtc_driver = { > + .driver = { > + .name = "ntxec-rtc", > + }, > + .probe = ntxec_rtc_probe, No .remove function? > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); Best regards Uwe
Hi, On 24/09/2020 21:24:53+0200, Jonathan Neuschäfer wrote: > With this driver, mainline Linux can keep its time and date in sync with > the vendor kernel. > > Advanced functionality like alarm and automatic power-on is not yet > supported. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > > v3: > - Add email address to copyright line > - Remove OF compatible string and don't include linux/of_device.h > - Don't use a comma after sentinels > - Avoid ret |= ... pattern > - Move 8-bit register conversion to ntxec.h > - Relicense as GPLv2 or later I don't think you had to relicense. The kernel is GPL 2 only, you are free to license your code under GPL 2 only if that is what you desire. > > v2: > - https://lore.kernel.org/lkml/20200905133230.1014581-7-j.neuschaefer@gmx.net/ > - Rework top-of-file comment [Lee Jones] > - Sort the #include lines [Alexandre Belloni] > - don't align = signs in struct initializers [Uwe Kleine-König] > - Switch to regmap > - Fix register number used to read minutes and seconds > - Prefix registers with NTXEC_REG_ > - Add help text to the Kconfig option > - Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max > --- > drivers/rtc/Kconfig | 8 +++ > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 141 insertions(+) > create mode 100644 drivers/rtc/rtc-ntxec.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 48c536acd777f..ae8f3dc36c9a3 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC > This driver can also be built as a module. If so, the module > will be called rtc-cros-ec. > > +config RTC_DRV_NTXEC > + tristate "Netronix embedded controller RTC driver" > + depends on MFD_NTXEC > + help > + Say yes here if you want to support the RTC functionality of the > + embedded controller found in certain e-book readers designed by the > + ODM Netronix. > + > comment "on-CPU RTC drivers" > > config RTC_DRV_ASM9260 > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index 880e08a409c3d..733479db18896 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o > obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o > obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o > +obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o > diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c > new file mode 100644 > index 0000000000000..af23c7cc76544 > --- /dev/null > +++ b/drivers/rtc/rtc-ntxec.c > @@ -0,0 +1,132 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * The Netronix embedded controller is a microcontroller found in some > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, > + * battery monitoring, system power management, and PWM functionality. > + * > + * This driver implements access to the RTC time and date. > + * > + * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net> > + */ > + > +#include <linux/mfd/ntxec.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/rtc.h> > +#include <linux/types.h> > + > +struct ntxec_rtc { > + struct device *dev; > + struct ntxec *ec; > +}; > + > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 > + > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year = (value >> 8) + 100; > + tm->tm_mon = (value & 0xff) - 1; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday = value >> 8; > + tm->tm_hour = value & 0xff; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min = value >> 8; > + tm->tm_sec = value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + int res = 0; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); > +} > + > +static const struct rtc_class_ops ntxec_rtc_ops = { > + .read_time = ntxec_read_time, > + .set_time = ntxec_set_time, > +}; > + > +static int ntxec_rtc_probe(struct platform_device *pdev) > +{ > + struct rtc_device *dev; > + struct ntxec_rtc *rtc; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + rtc->dev = &pdev->dev; > + rtc->ec = dev_get_drvdata(pdev->dev.parent); > + platform_set_drvdata(pdev, rtc); > + > + dev = devm_rtc_allocate_device(&pdev->dev); > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + > + dev->ops = &ntxec_rtc_ops; > + dev->range_min = RTC_TIMESTAMP_BEGIN_2000; > + dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */ > + > + return rtc_register_device(dev); > +} > + > +static struct platform_driver ntxec_rtc_driver = { > + .driver = { > + .name = "ntxec-rtc", > + }, > + .probe = ntxec_rtc_probe, > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); > -- > 2.28.0 >
On Fri, Sep 25, 2020 at 11:36:14AM +0200, Alexandre Belloni wrote: > Hi, > > On 24/09/2020 21:24:53+0200, Jonathan Neuschäfer wrote: ... > > v3: ... > > - Relicense as GPLv2 or later > > I don't think you had to relicense. The kernel is GPL 2 only, you are > free to license your code under GPL 2 only if that is what you desire. I don't mind in this case.
On Thu, Sep 24, 2020 at 10:40:11PM +0200, Andreas Kemnade wrote: > On Thu, 24 Sep 2020 21:24:53 +0200 > Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: [...] > > +static struct platform_driver ntxec_rtc_driver = { > > + .driver = { > > + .name = "ntxec-rtc", > > + }, > > + .probe = ntxec_rtc_probe, > > +}; > > +module_platform_driver(ntxec_rtc_driver); > > + > I think module autoloading will not work without > > MODULE_ALIAS("platform:ntxec-rtc"); > > Same for the pwm device. Right, I forgot that. Thanks for the reminder! Jonathan
On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-König wrote: > Hello Jonathan, > > On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote: > > +#define NTXEC_REG_WRITE_YEAR 0x10 > > +#define NTXEC_REG_WRITE_MONTH 0x11 > > +#define NTXEC_REG_WRITE_DAY 0x12 > > +#define NTXEC_REG_WRITE_HOUR 0x13 > > +#define NTXEC_REG_WRITE_MINUTE 0x14 > > +#define NTXEC_REG_WRITE_SECOND 0x15 > > + > > +#define NTXEC_REG_READ_YM 0x20 > > +#define NTXEC_REG_READ_DH 0x21 > > +#define NTXEC_REG_READ_MS 0x23 > > Is this an official naming? I think at least ..._MS is a poor name. > Maybe consider ..._MINSEC instead and make the other two names a bit longer > for consistency? It's inofficial (the vendor kernel uses 0x10 etc. directly). I'll pick longer names. > > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > > + unsigned int value; > > + int res; > > + > > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_year = (value >> 8) + 100; > > + tm->tm_mon = (value & 0xff) - 1; > > + > > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_mday = value >> 8; > > + tm->tm_hour = value & 0xff; > > + > > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_min = value >> 8; > > + tm->tm_sec = value & 0xff; > > + > > + return 0; > > +} > > + > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > > + int res = 0; > > + > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > > + if (res) > > + return res; > > + > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > > + if (res) > > + return res; > > + > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > > + if (res) > > + return res; > > + > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > > + if (res) > > + return res; > > + > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > > + if (res) > > + return res; > > + > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); > > I wonder: Is this racy? If you write minute, does the seconds reset to > zero or something like that? Or can it happen, that after writing the > minute register and before writing the second register the seconds > overflow and you end up with the time set to a minute later than > intended? If so it might be worth to set the seconds to 0 at the start > of the function (with an explaining comment). The setting the minutes does not reset the seconds, so I think this race condition is possible. I'll add the workaround. > .read_time has a similar race. What happens if minutes overflow between > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? Yes, we get read tearing in that case. It could even propagate all the way to the year/month field, for example when the following time rolls over: A | B | C 2020-10-31 23:59:59 2020-11-01 00:00:00 - If the increment happens after reading C, we get 2020-10-31 23:59:59 - If the increment happens between reading B and C, we get 2020-10-31 23:00:00 - If the increment happens between reading A and B, we get 2020-10-01 00:00:00 - If the increment happens before reading A, we get 2020-11-01 00:00:00 ... both of which are far from correct. To mitigate this issue, I think something like the following is needed: - Read year/month - Read day/hour - Read minute/second - Read day/hour, compare with previously read value, restart on mismatch - Read year/month, compare with previously read value, restart on mismatch The order of the last two steps doesn't matter, as far as I can see, but if I remove one of them, I can't catch all cases of read tearing. > > +static struct platform_driver ntxec_rtc_driver = { > > + .driver = { > > + .name = "ntxec-rtc", > > + }, > > + .probe = ntxec_rtc_probe, > > No .remove function? I don't think it would serve a purpose in this driver. There are no device-specific resources to release (no clocks to unprepare, for example). Thanks, Jonathan Neuschäfer
On 04/10/2020 03:43:23+0200, Jonathan Neuschäfer wrote: > > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > > > +{ > > > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > > > + int res = 0; > > > + > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > > > + if (res) > > > + return res; > > > + > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > > > + if (res) > > > + return res; > > > + > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > > > + if (res) > > > + return res; > > > + > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > > > + if (res) > > > + return res; > > > + > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > > > + if (res) > > > + return res; > > > + > > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); > > > > I wonder: Is this racy? If you write minute, does the seconds reset to > > zero or something like that? Or can it happen, that after writing the > > minute register and before writing the second register the seconds > > overflow and you end up with the time set to a minute later than > > intended? If so it might be worth to set the seconds to 0 at the start > > of the function (with an explaining comment). > > The setting the minutes does not reset the seconds, so I think this race > condition is possible. I'll add the workaround. > Are you sure this happens? Usually, the seconds are not reset but the internal 32768kHz counter is so you have a full second to write all the registers. > > .read_time has a similar race. What happens if minutes overflow between > > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > > Yes, we get read tearing in that case. It could even propagate all the > way to the year/month field, for example when the following time rolls > over: > A | B | C > 2020-10-31 23:59:59 > 2020-11-01 00:00:00 > > - If the increment happens after reading C, we get 2020-10-31 23:59:59 > - If the increment happens between reading B and C, we get 2020-10-31 23:00:00 > - If the increment happens between reading A and B, we get 2020-10-01 00:00:00 > - If the increment happens before reading A, we get 2020-11-01 00:00:00 > > ... both of which are far from correct. > > To mitigate this issue, I think something like the following is needed: > > - Read year/month > - Read day/hour > - Read minute/second > - Read day/hour, compare with previously read value, restart on mismatch > - Read year/month, compare with previously read value, restart on mismatch > > The order of the last two steps doesn't matter, as far as I can see, but > if I remove one of them, I can't catch all cases of read tearing. > Are you also sure this happens? Only one comparison is necessary, the correct order would be: - Read minute/second - Read day/hour - Read year/month - Read minute/second, compare If day/hour changes but not minute/second, it would mean that it took at least an hour to read all the registers. At this point, I think you have other problems and the exact time doesn't matter anymore.
Hello Jonathan, On Sun, Oct 04, 2020 at 03:43:23AM +0200, Jonathan Neuschäfer wrote: > On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-König wrote: > > > +static struct platform_driver ntxec_rtc_driver = { > > > + .driver = { > > > + .name = "ntxec-rtc", > > > + }, > > > + .probe = ntxec_rtc_probe, > > > > No .remove function? > > I don't think it would serve a purpose in this driver. There are no > device-specific resources to release (no clocks to unprepare, for > example). I had in mind that without a .remove callback the driver cannot detach. but looking in the code (drivers/base/platform.c) this seems wrong. So my concern can be considered void. Best regards Uwe
On Sun, Oct 04, 2020 at 10:42:09AM +0200, Alexandre Belloni wrote: > On 04/10/2020 03:43:23+0200, Jonathan Neuschäfer wrote: > > > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) .... > > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > > > > + if (res) > > > > + return res; > > > > + > > > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); > > > > > > I wonder: Is this racy? If you write minute, does the seconds reset to > > > zero or something like that? Or can it happen, that after writing the > > > minute register and before writing the second register the seconds > > > overflow and you end up with the time set to a minute later than > > > intended? If so it might be worth to set the seconds to 0 at the start > > > of the function (with an explaining comment). > > > > The setting the minutes does not reset the seconds, so I think this race > > condition is possible. I'll add the workaround. > > > > Are you sure this happens? Usually, the seconds are not reset but the > internal 32768kHz counter is so you have a full second to write all the > registers. I just checked it, and on this RTC, the phase / sub-second part is not reset when the time is set. > > > .read_time has a similar race. What happens if minutes overflow between > > > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > > > > Yes, we get read tearing in that case. It could even propagate all the > > way to the year/month field, for example when the following time rolls > > over: > > A | B | C > > 2020-10-31 23:59:59 > > 2020-11-01 00:00:00 > > > > - If the increment happens after reading C, we get 2020-10-31 23:59:59 > > - If the increment happens between reading B and C, we get 2020-10-31 23:00:00 > > - If the increment happens between reading A and B, we get 2020-10-01 00:00:00 > > - If the increment happens before reading A, we get 2020-11-01 00:00:00 > > > > ... both of which are far from correct. > > > > To mitigate this issue, I think something like the following is needed: > > > > - Read year/month > > - Read day/hour > > - Read minute/second > > - Read day/hour, compare with previously read value, restart on mismatch > > - Read year/month, compare with previously read value, restart on mismatch > > > > The order of the last two steps doesn't matter, as far as I can see, but > > if I remove one of them, I can't catch all cases of read tearing. > > > > Are you also sure this happens? > > Only one comparison is necessary, the correct order would be: > > - Read minute/second > - Read day/hour > - Read year/month > - Read minute/second, compare With this order, every one-second increment is detected, which I previously tried to avoid. But I suppose it's fine because it simplifies the logic and the window from first to last read should be short enough anyway to be relatively unlikely to hit, and thus not cause a lot of retries. > If day/hour changes but not minute/second, it would mean that it took at > least an hour to read all the registers. At this point, I think you have > other problems and the exact time doesn't matter anymore. Indeed. Thanks, Jonathan Neuschäfer
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 48c536acd777f..ae8f3dc36c9a3 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC This driver can also be built as a module. If so, the module will be called rtc-cros-ec. +config RTC_DRV_NTXEC + tristate "Netronix embedded controller RTC driver" + depends on MFD_NTXEC + help + Say yes here if you want to support the RTC functionality of the + embedded controller found in certain e-book readers designed by the + ODM Netronix. + comment "on-CPU RTC drivers" config RTC_DRV_ASM9260 diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index 880e08a409c3d..733479db18896 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o +obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c new file mode 100644 index 0000000000000..af23c7cc76544 --- /dev/null +++ b/drivers/rtc/rtc-ntxec.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * The Netronix embedded controller is a microcontroller found in some + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, + * battery monitoring, system power management, and PWM functionality. + * + * This driver implements access to the RTC time and date. + * + * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net> + */ + +#include <linux/mfd/ntxec.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/rtc.h> +#include <linux/types.h> + +struct ntxec_rtc { + struct device *dev; + struct ntxec *ec; +}; + +#define NTXEC_REG_WRITE_YEAR 0x10 +#define NTXEC_REG_WRITE_MONTH 0x11 +#define NTXEC_REG_WRITE_DAY 0x12 +#define NTXEC_REG_WRITE_HOUR 0x13 +#define NTXEC_REG_WRITE_MINUTE 0x14 +#define NTXEC_REG_WRITE_SECOND 0x15 + +#define NTXEC_REG_READ_YM 0x20 +#define NTXEC_REG_READ_DH 0x21 +#define NTXEC_REG_READ_MS 0x23 + +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) +{ + struct ntxec_rtc *rtc = dev_get_drvdata(dev); + unsigned int value; + int res; + + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); + if (res < 0) + return res; + + tm->tm_year = (value >> 8) + 100; + tm->tm_mon = (value & 0xff) - 1; + + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); + if (res < 0) + return res; + + tm->tm_mday = value >> 8; + tm->tm_hour = value & 0xff; + + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); + if (res < 0) + return res; + + tm->tm_min = value >> 8; + tm->tm_sec = value & 0xff; + + return 0; +} + +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) +{ + struct ntxec_rtc *rtc = dev_get_drvdata(dev); + int res = 0; + + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); + if (res) + return res; + + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); + if (res) + return res; + + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); + if (res) + return res; + + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); + if (res) + return res; + + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); + if (res) + return res; + + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); +} + +static const struct rtc_class_ops ntxec_rtc_ops = { + .read_time = ntxec_read_time, + .set_time = ntxec_set_time, +}; + +static int ntxec_rtc_probe(struct platform_device *pdev) +{ + struct rtc_device *dev; + struct ntxec_rtc *rtc; + + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); + if (!rtc) + return -ENOMEM; + + rtc->dev = &pdev->dev; + rtc->ec = dev_get_drvdata(pdev->dev.parent); + platform_set_drvdata(pdev, rtc); + + dev = devm_rtc_allocate_device(&pdev->dev); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + dev->ops = &ntxec_rtc_ops; + dev->range_min = RTC_TIMESTAMP_BEGIN_2000; + dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */ + + return rtc_register_device(dev); +} + +static struct platform_driver ntxec_rtc_driver = { + .driver = { + .name = "ntxec-rtc", + }, + .probe = ntxec_rtc_probe, +}; +module_platform_driver(ntxec_rtc_driver); + +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); +MODULE_DESCRIPTION("RTC driver for Netronix EC"); +MODULE_LICENSE("GPL");
With this driver, mainline Linux can keep its time and date in sync with the vendor kernel. Advanced functionality like alarm and automatic power-on is not yet supported. Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- v3: - Add email address to copyright line - Remove OF compatible string and don't include linux/of_device.h - Don't use a comma after sentinels - Avoid ret |= ... pattern - Move 8-bit register conversion to ntxec.h - Relicense as GPLv2 or later v2: - https://lore.kernel.org/lkml/20200905133230.1014581-7-j.neuschaefer@gmx.net/ - Rework top-of-file comment [Lee Jones] - Sort the #include lines [Alexandre Belloni] - don't align = signs in struct initializers [Uwe Kleine-König] - Switch to regmap - Fix register number used to read minutes and seconds - Prefix registers with NTXEC_REG_ - Add help text to the Kconfig option - Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max --- drivers/rtc/Kconfig | 8 +++ drivers/rtc/Makefile | 1 + drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) create mode 100644 drivers/rtc/rtc-ntxec.c -- 2.28.0