Message ID | 20170221061650.12636-1-sre@kernel.org |
---|---|
State | Superseded |
Headers | show |
Hi, The patch has a few checkpatch issues. Some of those should really be fixed. Can you do that? Else, it is mostly fine, a few comments below. On 21/02/2017 at 07:16:50 +0100, Sebastian Reichel wrote: > +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct cpcap_rtc *rtc; > + struct cpcap_time cpcap_tm; > + int ret = 0; > + > + rtc = dev_get_drvdata(dev); > + > + rtc2cpcap_time(&cpcap_tm, tm); > + > + if (rtc->alarm_enabled) > + disable_irq(rtc->alarm_irq); > + if (rtc->update_enabled) > + disable_irq(rtc->update_irq); > + > + if (rtc->vendor == CPCAP_VENDOR_ST) { > + /* The TOD1 and TOD2 registers MUST be written in this order > + * for the change to properly set. */ Does this mean there is a race condition? > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > + TOD1_MASK, cpcap_tm.tod1); > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, > + TOD2_MASK, cpcap_tm.tod2); > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, > + DAY_MASK, cpcap_tm.day); > + } else { > + /* Clearing the upper lower 8 bits of the TOD guarantees that > + * the upper half of TOD (TOD2) will not increment for 0xFF RTC > + * ticks (255 seconds). During this time we can safely write > + * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be > + * synchronized to the exact time requested upon the final write > + * to TOD1. */ > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > + TOD1_MASK, 0); > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, > + DAY_MASK, cpcap_tm.day); > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, > + TOD2_MASK, cpcap_tm.tod2); > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > + TOD1_MASK, cpcap_tm.tod1); > + } > + > + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor); I think this means it depends on the mfd tree. > + if (err) > + return err; > + > + rtc->alarm_irq= platform_get_irq(pdev, 0); > + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL, > + cpcap_rtc_alarm_irq, IRQ_NONE, > + "rtc_alarm", rtc); > + if (err) { > + dev_err(dev, "Could not request alarm irq: %d\n", err); > + return err; > + } > + disable_irq(rtc->alarm_irq); > + > + rtc->update_irq= platform_get_irq(pdev, 1); > + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL, > + cpcap_rtc_update_irq, IRQ_NONE, > + "rtc_1hz", rtc); I don't think this IRQ is actually useful. It doesn't really harm but the tests should pass without it. > + if (err) { > + dev_err(dev, "Could not request update irq: %d\n", err); > + return err; > + } > + disable_irq(rtc->update_irq); > + > + err = device_init_wakeup(dev, 1); If you use device_init_wakeup, I think it needs to be called before devm_rtc_device_register() to properly work.
Hi Alexandre, On Wed, Feb 22, 2017 at 12:52:12AM +0100, Alexandre Belloni wrote: > The patch has a few checkpatch issues. Some of those should really be > fixed. Can you do that? of course. > Else, it is mostly fine, a few comments below. > > On 21/02/2017 at 07:16:50 +0100, Sebastian Reichel wrote: > > +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct cpcap_rtc *rtc; > > + struct cpcap_time cpcap_tm; > > + int ret = 0; > > + > > + rtc = dev_get_drvdata(dev); > > + > > + rtc2cpcap_time(&cpcap_tm, tm); > > + > > + if (rtc->alarm_enabled) > > + disable_irq(rtc->alarm_irq); > > + if (rtc->update_enabled) > > + disable_irq(rtc->update_irq); > > + > > + if (rtc->vendor == CPCAP_VENDOR_ST) { > > + /* The TOD1 and TOD2 registers MUST be written in this order > > + * for the change to properly set. */ > > Does this mean there is a race condition? The logic (incl. comments) in this section are from the vendor kernel driver and there is no documentation for CPCAP as far as I know. I don't know if the hardware has logic to prevent a race condition for the cpcap_tm.tod1 == 255 case. > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > > + TOD1_MASK, cpcap_tm.tod1); > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, > > + TOD2_MASK, cpcap_tm.tod2); > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, > > + DAY_MASK, cpcap_tm.day); > > + } else { > > + /* Clearing the upper lower 8 bits of the TOD guarantees that > > + * the upper half of TOD (TOD2) will not increment for 0xFF RTC > > + * ticks (255 seconds). During this time we can safely write > > + * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be > > + * synchronized to the exact time requested upon the final write > > + * to TOD1. */ > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > > + TOD1_MASK, 0); > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, > > + DAY_MASK, cpcap_tm.day); > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, > > + TOD2_MASK, cpcap_tm.tod2); > > + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, > > + TOD1_MASK, cpcap_tm.tod1); > > + } > > + > > > + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor); > I think this means it depends on the mfd tree. Yes, but cpcap_get_vendor should get into mainline with the 4.11 mfd pull request. So if you base your 4.12 for-next tree on 4.11-rc1 everything should be fine. > > + if (err) > > + return err; > > + > > + rtc->alarm_irq= platform_get_irq(pdev, 0); > > + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL, > > + cpcap_rtc_alarm_irq, IRQ_NONE, > > + "rtc_alarm", rtc); > > + if (err) { > > + dev_err(dev, "Could not request alarm irq: %d\n", err); > > + return err; > > + } > > + disable_irq(rtc->alarm_irq); > > + > > + rtc->update_irq= platform_get_irq(pdev, 1); > > + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL, > > + cpcap_rtc_update_irq, IRQ_NONE, > > + "rtc_1hz", rtc); > I don't think this IRQ is actually useful. It doesn't really harm but > the tests should pass without it. Yes. RTC works perfectly fine with just the alarm irq. It also works perfectly fine with just the 1 Hz irq (except for wakeup). I would like to keep the irq in the driver, so that it's explicitly disabled. On Droid 4 mainline kernel is booted via kexec from Android (AKA bootloader) and Motorola's Android kernel uses the 1 Hz IRQ for some proprietary "secure clock daemon". I will add a comment. > > + if (err) { > > + dev_err(dev, "Could not request update irq: %d\n", err); > > + return err; > > + } > > + disable_irq(rtc->update_irq); > > + > > + err = device_init_wakeup(dev, 1); > > If you use device_init_wakeup, I think it needs to be called before > devm_rtc_device_register() to properly work. I successfully tested wakeup before sending this. But in case your prefer it to be called before registering the RTC I can move the call accordingly. -- Sebastian
On 22/02/2017 at 02:56:34 +0100, Sebastian Reichel wrote: > > Does this mean there is a race condition? > > The logic (incl. comments) in this section are from the vendor > kernel driver and there is no documentation for CPCAP as far as > I know. I don't know if the hardware has logic to prevent a race > condition for the cpcap_tm.tod1 == 255 case. > That's fine, I was just curious :) > > > + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor); > > I think this means it depends on the mfd tree. > > Yes, but cpcap_get_vendor should get into mainline with the > 4.11 mfd pull request. So if you base your 4.12 for-next tree > on 4.11-rc1 everything should be fine. > OK, I'll take it for 4.12 then > > > + if (err) > > > + return err; > > > + > > > + rtc->alarm_irq= platform_get_irq(pdev, 0); > > > + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL, > > > + cpcap_rtc_alarm_irq, IRQ_NONE, > > > + "rtc_alarm", rtc); > > > + if (err) { > > > + dev_err(dev, "Could not request alarm irq: %d\n", err); > > > + return err; > > > + } > > > + disable_irq(rtc->alarm_irq); > > > + > > > + rtc->update_irq= platform_get_irq(pdev, 1); > > > + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL, > > > + cpcap_rtc_update_irq, IRQ_NONE, > > > + "rtc_1hz", rtc); > > > I don't think this IRQ is actually useful. It doesn't really harm but > > the tests should pass without it. > > Yes. RTC works perfectly fine with just the alarm irq. It also > works perfectly fine with just the 1 Hz irq (except for wakeup). > > I would like to keep the irq in the driver, so that it's explicitly > disabled. On Droid 4 mainline kernel is booted via kexec from > Android (AKA bootloader) and Motorola's Android kernel uses the > 1 Hz IRQ for some proprietary "secure clock daemon". > > I will add a comment. > OK, my plan was to remove all the RTC_UF users. I'll give it more thoughts. > > > + if (err) { > > > + dev_err(dev, "Could not request update irq: %d\n", err); > > > + return err; > > > + } > > > + disable_irq(rtc->update_irq); > > > + > > > + err = device_init_wakeup(dev, 1); > > > > If you use device_init_wakeup, I think it needs to be called before > > devm_rtc_device_register() to properly work. > > I successfully tested wakeup before sending this. But in case your > prefer it to be called before registering the RTC I can move the > call accordingly. > Then it is fine where it is.
diff --git a/Documentation/devicetree/bindings/rtc/cpcap-rtc.txt b/Documentation/devicetree/bindings/rtc/cpcap-rtc.txt new file mode 100644 index 000000000000..2709c32baf2c --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/cpcap-rtc.txt @@ -0,0 +1,13 @@ +Motorola CPCAP PMIC RTC +------------------------------------ + +Requires node properties: +- compatible: should contain "motorola,cpcap-rtc" +- interrupts: An interrupt specifier for alarm and 1 Hz irq + +Example: + +cpcap_rtc: rtc { + compatible = "motorola,cpcap-rtc"; + interrupts = <39 IRQ_TYPE_NONE>, <26 IRQ_TYPE_NONE>; +}; diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index ee1b0e9dde79..050bec749fae 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1731,6 +1731,13 @@ config RTC_DRV_STM32 This driver can also be built as a module, if so, the module will be called "rtc-stm32". +config RTC_DRV_CPCAP + depends on MFD_CPCAP + tristate "Motorola CPCAP RTC" + help + Say y here for CPCAP rtc found on some Motorola phones + and tablets such as Droid 4. + comment "HID Sensor RTC drivers" config RTC_DRV_HID_SENSOR_TIME diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index f07297b1460a..13857d2fce09 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -40,6 +40,7 @@ obj-$(CONFIG_RTC_DRV_BQ32K) += rtc-bq32k.o obj-$(CONFIG_RTC_DRV_BQ4802) += rtc-bq4802.o obj-$(CONFIG_RTC_DRV_CMOS) += rtc-cmos.o obj-$(CONFIG_RTC_DRV_COH901331) += rtc-coh901331.o +obj-$(CONFIG_RTC_DRV_CPCAP) += rtc-cpcap.o obj-$(CONFIG_RTC_DRV_DA9052) += rtc-da9052.o obj-$(CONFIG_RTC_DRV_DA9055) += rtc-da9055.o obj-$(CONFIG_RTC_DRV_DA9063) += rtc-da9063.o diff --git a/drivers/rtc/rtc-cpcap.c b/drivers/rtc/rtc-cpcap.c new file mode 100644 index 000000000000..d24c205b70e8 --- /dev/null +++ b/drivers/rtc/rtc-cpcap.c @@ -0,0 +1,324 @@ +/* + * Motorola CPCAP PMIC RTC driver + * + * Based on cpcap-regulator.c from Motorola Linux kernel tree + * Copyright (C) 2009 Motorola, Inc. + * + * Rewritten for mainline kernel + * - use DT + * - use regmap + * - use standard interrupt framework + * - use managed device resources + * - remove custom "secure clock daemon" helpers + * + * Copyright (C) 2017 Sebastian Reichel <sre@kernel.org> + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/rtc.h> +#include <linux/err.h> +#include <linux/regmap.h> +#include <linux/mfd/motorola-cpcap.h> +#include <linux/slab.h> +#include <linux/sched.h> + +#define SECS_PER_DAY 86400 +#define DAY_MASK 0x7FFF +#define TOD1_MASK 0x00FF +#define TOD2_MASK 0x01FF + +struct cpcap_time { + int day; + int tod1; + int tod2; +}; + +struct cpcap_rtc { + struct regmap *regmap; + struct rtc_device *rtc_dev; + u16 vendor; + int alarm_irq; + bool alarm_enabled; + int update_irq; + bool update_enabled; +}; + +static void cpcap2rtc_time(struct rtc_time *rtc, struct cpcap_time *cpcap) +{ + unsigned long int tod; + unsigned long int time; + + tod = (cpcap->tod1 & TOD1_MASK) | ((cpcap->tod2 & TOD2_MASK) << 8); + time = tod + ((cpcap->day & DAY_MASK) * SECS_PER_DAY); + + rtc_time_to_tm(time, rtc); +} + +static void rtc2cpcap_time(struct cpcap_time *cpcap, struct rtc_time *rtc) +{ + unsigned long time; + + rtc_tm_to_time(rtc, &time); + + cpcap->day = time / SECS_PER_DAY; + time %= SECS_PER_DAY; + cpcap->tod2 = (time >> 8) & TOD2_MASK; + cpcap->tod1 = time & TOD1_MASK; +} + +static int cpcap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) +{ + struct cpcap_rtc *rtc = dev_get_drvdata(dev); + + if (rtc->alarm_enabled == enabled) + return 0; + + if (enabled) + enable_irq(rtc->alarm_irq); + else + disable_irq(rtc->alarm_irq); + + rtc->alarm_enabled = !!enabled; + + return 0; +} + +static int cpcap_rtc_read_time(struct device *dev, struct rtc_time *tm) +{ + struct cpcap_rtc *rtc; + struct cpcap_time cpcap_tm; + int temp_tod2; + int ret; + + rtc = dev_get_drvdata(dev); + + ret = regmap_read(rtc->regmap, CPCAP_REG_TOD2, &temp_tod2); + ret |= regmap_read(rtc->regmap, CPCAP_REG_DAY, &cpcap_tm.day); + ret |= regmap_read(rtc->regmap, CPCAP_REG_TOD1, &cpcap_tm.tod1); + ret |= regmap_read(rtc->regmap, CPCAP_REG_TOD2, &cpcap_tm.tod2); + + if (temp_tod2 > cpcap_tm.tod2) + ret |= regmap_read(rtc->regmap, CPCAP_REG_DAY, &cpcap_tm.day); + + if (ret) { + dev_err(dev, "Failed to read time\n"); + return -EIO; + } + + cpcap2rtc_time(tm, &cpcap_tm); + + return rtc_valid_tm(tm); +} + +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm) +{ + struct cpcap_rtc *rtc; + struct cpcap_time cpcap_tm; + int ret = 0; + + rtc = dev_get_drvdata(dev); + + rtc2cpcap_time(&cpcap_tm, tm); + + if (rtc->alarm_enabled) + disable_irq(rtc->alarm_irq); + if (rtc->update_enabled) + disable_irq(rtc->update_irq); + + if (rtc->vendor == CPCAP_VENDOR_ST) { + /* The TOD1 and TOD2 registers MUST be written in this order + * for the change to properly set. */ + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, + TOD1_MASK, cpcap_tm.tod1); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, + TOD2_MASK, cpcap_tm.tod2); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, + DAY_MASK, cpcap_tm.day); + } else { + /* Clearing the upper lower 8 bits of the TOD guarantees that + * the upper half of TOD (TOD2) will not increment for 0xFF RTC + * ticks (255 seconds). During this time we can safely write + * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be + * synchronized to the exact time requested upon the final write + * to TOD1. */ + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, + TOD1_MASK, 0); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY, + DAY_MASK, cpcap_tm.day); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2, + TOD2_MASK, cpcap_tm.tod2); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1, + TOD1_MASK, cpcap_tm.tod1); + } + + if (rtc->update_enabled) + enable_irq(rtc->update_irq); + if (rtc->alarm_enabled) + enable_irq(rtc->alarm_irq); + + return ret; +} + +static int cpcap_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct cpcap_rtc *rtc; + struct cpcap_time cpcap_tm; + int ret; + + rtc = dev_get_drvdata(dev); + + alrm->enabled = rtc->alarm_enabled; + + ret = regmap_read(rtc->regmap, CPCAP_REG_DAYA, &cpcap_tm.day); + ret |= regmap_read(rtc->regmap, CPCAP_REG_TODA2, &cpcap_tm.tod2); + ret |= regmap_read(rtc->regmap, CPCAP_REG_TODA1, &cpcap_tm.tod1); + + if (ret) { + dev_err(dev, "Failed to read time\n"); + return -EIO; + } + + cpcap2rtc_time(&alrm->time, &cpcap_tm); + return rtc_valid_tm(&alrm->time); +} + +static int cpcap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct cpcap_rtc *rtc; + struct cpcap_time cpcap_tm; + int ret; + + rtc = dev_get_drvdata(dev); + + rtc2cpcap_time(&cpcap_tm, &alrm->time); + + if (rtc->alarm_enabled) + disable_irq(rtc->alarm_irq); + + ret = regmap_update_bits(rtc->regmap, CPCAP_REG_DAYA, DAY_MASK, + cpcap_tm.day); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TODA2, TOD2_MASK, + cpcap_tm.tod2); + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TODA1, TOD1_MASK, + cpcap_tm.tod1); + + if (!ret) { + enable_irq(rtc->alarm_irq); + rtc->alarm_enabled = true; + } + + return ret; +} + +static struct rtc_class_ops cpcap_rtc_ops = { + .read_time = cpcap_rtc_read_time, + .set_time = cpcap_rtc_set_time, + .read_alarm = cpcap_rtc_read_alarm, + .set_alarm = cpcap_rtc_set_alarm, + .alarm_irq_enable = cpcap_rtc_alarm_irq_enable, +}; + +static irqreturn_t cpcap_rtc_alarm_irq(int irq, void *data) +{ + struct cpcap_rtc *rtc = data; + + rtc_update_irq(rtc->rtc_dev, 1, RTC_AF | RTC_IRQF); + return IRQ_HANDLED; +} + +static irqreturn_t cpcap_rtc_update_irq(int irq, void *data) +{ + struct cpcap_rtc *rtc = data; + + rtc_update_irq(rtc->rtc_dev, 1, RTC_UF | RTC_IRQF); + return IRQ_HANDLED; +} + +static int cpcap_rtc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct cpcap_rtc *rtc; + int err; + + rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL); + if (!rtc) + return -ENOMEM; + + rtc->regmap = dev_get_regmap(dev->parent, NULL); + if (!rtc->regmap) + return -ENODEV; + + platform_set_drvdata(pdev, rtc); + rtc->rtc_dev = devm_rtc_device_register(dev, "cpcap_rtc", + &cpcap_rtc_ops, THIS_MODULE); + + if (IS_ERR(rtc->rtc_dev)) { + kfree(rtc); + return PTR_ERR(rtc->rtc_dev); + } + + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor); + if (err) + return err; + + rtc->alarm_irq= platform_get_irq(pdev, 0); + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL, + cpcap_rtc_alarm_irq, IRQ_NONE, + "rtc_alarm", rtc); + if (err) { + dev_err(dev, "Could not request alarm irq: %d\n", err); + return err; + } + disable_irq(rtc->alarm_irq); + + rtc->update_irq= platform_get_irq(pdev, 1); + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL, + cpcap_rtc_update_irq, IRQ_NONE, + "rtc_1hz", rtc); + if (err) { + dev_err(dev, "Could not request update irq: %d\n", err); + return err; + } + disable_irq(rtc->update_irq); + + err = device_init_wakeup(dev, 1); + if (err) { + dev_err(dev, "wakeup initialization failed (%d)\n", err); + /* ignore error and continue without wakeup support */ + } + + return 0; +} + +static const struct of_device_id cpcap_rtc_of_match[] = { + { .compatible = "motorola,cpcap-rtc", }, + {}, +}; +MODULE_DEVICE_TABLE(of, cpcap_rtc_of_match); + +static struct platform_driver cpcap_rtc_driver = { + .probe = cpcap_rtc_probe, + .driver = { + .name = "cpcap-rtc", + .of_match_table = cpcap_rtc_of_match, + }, +}; + +module_platform_driver(cpcap_rtc_driver); + +MODULE_ALIAS("platform:cpcap-rtc"); +MODULE_DESCRIPTION("CPCAP RTC driver"); +MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>"); +MODULE_LICENSE("GPL");