Hi Alessandro, you did not reply to my patch submission. Why not? Tony Olech Dialog Semiconductor
> -----Original Message----- > From: Opensource [Anthony Olech] > [mailto:anthony.olech.opensou...@diasemi.com] > Sent: 02 April 2014 12:46 > To: Alessandro Zummo; Support Opensource > Cc: linux-kernel@vger.kernel.org; rtc-li...@googlegroups.com; David Dajun > Chen > Subject: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm > > Setting the alarm to a time not on a minute boundary results in repeated > interrupts being generated by the DA9052/3 PMIC device until the kernel RTC > core sees that the alarm has rung. Sometimes the number and frequency of > interrupts can cause the kernel to disable the IRQ line used by the > DA9052/3 PMIC with disasterous consequences. This patch fixes the > problem. > > Even though the DA9052/3 PMIC is capable generating periodic interrupts, ie > TICKS, the method used to distinguish RTC_AF from RTC_PF events was > flawed and can not work in conjunction with the regmap_irq kernel core. > Thus that flawed detection has also been removed by the DA9052/3 PMIC > RTC driver's irq handler, so that it no longer reports the wrong type of event > to the kernel RTC core. > > The internal static functions within the DA9052/3 PMIC RTC driver have been > changed to pass the 'da9052_rtc' structure instead of the 'da9052' > because there is no backwards pointer from the 'da9052' structure. > > Signed-off-by: Anthony Olech <anthony.olech.opensou...@diasemi.com> > --- > > This patch is relative to linux-next repository tag next-20140402 > > This patch fixes the three issues described above. The first is serious > because > usiing the RTC alarm set to a non minute boundary will eventually cause all > component drivers that depend on the interrupt line to fail. The solution > adopted is to round up to alarm time to the next highest minute. > > The second bug, reporting a RTC_PF event instead of an RTC_AF event turns > out to not matter with the current implementation of the kernel RTC core as > it seems to ignore the event type. However, should that change in the future > it is better to fix the issue now and not have 'problems waiting to happen' > > The third set of changes are to make the da9052_rtc structure available to all > the local internal functions in the driver. This was done during testing so > that > diagnostic data could be stored there. Should the solution to the first issue > be found not acceptable, then the alternative of using the TICKS interrupt at > the fixed one second interval in order to step to the exact second of the > requested alarm requires an extra (alarm time) piece of data to be stored. In > devices that use the alarm function to wake up from sleep, accuracy to the > second will result in the device being awake for up to nearly a minute longer > than expected. > > drivers/rtc/rtc-da9052.c | 122 +++++++++++++++++++++++++----------------- > ---- > 1 file changed, 66 insertions(+), 56 deletions(-) > > diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c index > a1cbf64..e5c9486 100644 > --- a/drivers/rtc/rtc-da9052.c > +++ b/drivers/rtc/rtc-da9052.c > @@ -20,28 +20,28 @@ > #include <linux/mfd/da9052/da9052.h> > #include <linux/mfd/da9052/reg.h> > > -#define rtc_err(da9052, fmt, ...) \ > - dev_err(da9052->dev, "%s: " fmt, __func__, > ##__VA_ARGS__) > +#define rtc_err(rtc, fmt, ...) \ > + dev_err(rtc->da9052->dev, "%s: " fmt, __func__, > ##__VA_ARGS__) > > struct da9052_rtc { > struct rtc_device *rtc; > struct da9052 *da9052; > }; > > -static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable) > +static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable) > { > int ret; > if (enable) { > - ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG, > - DA9052_ALARM_Y_ALARM_ON, > - DA9052_ALARM_Y_ALARM_ON); > + ret = da9052_reg_update(rtc->da9052, > DA9052_ALARM_Y_REG, > + > DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, > + DA9052_ALARM_Y_ALARM_ON); > if (ret != 0) > - rtc_err(da9052, "Failed to enable ALM: %d\n", ret); > + rtc_err(rtc, "Failed to enable ALM: %d\n", ret); > } else { > - ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG, > - DA9052_ALARM_Y_ALARM_ON, 0); > + ret = da9052_reg_update(rtc->da9052, > DA9052_ALARM_Y_REG, > + > DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0); > if (ret != 0) > - rtc_err(da9052, "Write error: %d\n", ret); > + rtc_err(rtc, "Write error: %d\n", ret); > } > return ret; > } > @@ -49,31 +49,20 @@ static int da9052_rtc_enable_alarm(struct da9052 > *da9052, bool enable) static irqreturn_t da9052_rtc_irq(int irq, void *data) > { > struct da9052_rtc *rtc = data; > - int ret; > > - ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_MI_REG); > - if (ret < 0) { > - rtc_err(rtc->da9052, "Read error: %d\n", ret); > - return IRQ_NONE; > - } > - > - if (ret & DA9052_ALARMMI_ALARMTYPE) { > - da9052_rtc_enable_alarm(rtc->da9052, 0); > - rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF); > - } else > - rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_PF); > + rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF); > > return IRQ_HANDLED; > } > > -static int da9052_read_alarm(struct da9052 *da9052, struct rtc_time > *rtc_tm) > +static int da9052_read_alarm(struct da9052_rtc *rtc, struct rtc_time > +*rtc_tm) > { > int ret; > uint8_t v[5]; > > - ret = da9052_group_read(da9052, DA9052_ALARM_MI_REG, 5, v); > + ret = da9052_group_read(rtc->da9052, DA9052_ALARM_MI_REG, 5, > v); > if (ret != 0) { > - rtc_err(da9052, "Failed to group read ALM: %d\n", ret); > + rtc_err(rtc, "Failed to group read ALM: %d\n", ret); > return ret; > } > > @@ -84,23 +73,33 @@ static int da9052_read_alarm(struct da9052 > *da9052, struct rtc_time *rtc_tm) > rtc_tm->tm_min = v[0] & DA9052_RTC_MIN; > > ret = rtc_valid_tm(rtc_tm); > - if (ret != 0) > - return ret; > return ret; > } > > -static int da9052_set_alarm(struct da9052 *da9052, struct rtc_time > *rtc_tm) > +static int da9052_set_alarm(struct da9052_rtc *rtc, struct rtc_time > +*rtc_tm) > { > + struct da9052 *da9052 = rtc->da9052; > + unsigned long alm_time; > int ret; > uint8_t v[3]; > > + ret = rtc_tm_to_time(rtc_tm, &alm_time); > + if (ret != 0) > + return ret; > + > + if (rtc_tm->tm_sec > 0) { > + alm_time += 60 - rtc_tm->tm_sec; > + rtc_time_to_tm(alm_time, rtc_tm); > + } > + BUG_ON(rtc_tm->tm_sec); /* it will cause repeated irqs if not zero > */ > + > rtc_tm->tm_year -= 100; > rtc_tm->tm_mon += 1; > > ret = da9052_reg_update(da9052, DA9052_ALARM_MI_REG, > DA9052_RTC_MIN, rtc_tm->tm_min); > if (ret != 0) { > - rtc_err(da9052, "Failed to write ALRM MIN: %d\n", ret); > + rtc_err(rtc, "Failed to write ALRM MIN: %d\n", ret); > return ret; > } > > @@ -115,22 +114,22 @@ static int da9052_set_alarm(struct da9052 > *da9052, struct rtc_time *rtc_tm) > ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG, > DA9052_RTC_YEAR, rtc_tm->tm_year); > if (ret != 0) > - rtc_err(da9052, "Failed to write ALRM YEAR: %d\n", ret); > + rtc_err(rtc, "Failed to write ALRM YEAR: %d\n", ret); > > return ret; > } > > -static int da9052_rtc_get_alarm_status(struct da9052 *da9052) > +static int da9052_rtc_get_alarm_status(struct da9052_rtc *rtc) > { > int ret; > > - ret = da9052_reg_read(da9052, DA9052_ALARM_Y_REG); > + ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_Y_REG); > if (ret < 0) { > - rtc_err(da9052, "Failed to read ALM: %d\n", ret); > + rtc_err(rtc, "Failed to read ALM: %d\n", ret); > return ret; > } > - ret &= DA9052_ALARM_Y_ALARM_ON; > - return (ret > 0) ? 1 : 0; > + > + return !!(ret&DA9052_ALARM_Y_ALARM_ON); > } > > static int da9052_rtc_read_time(struct device *dev, struct rtc_time *rtc_tm) > @@ -141,7 +140,7 @@ static int da9052_rtc_read_time(struct device *dev, > struct rtc_time *rtc_tm) > > ret = da9052_group_read(rtc->da9052, DA9052_COUNT_S_REG, 6, > v); > if (ret < 0) { > - rtc_err(rtc->da9052, "Failed to read RTC time : %d\n", ret); > + rtc_err(rtc, "Failed to read RTC time : %d\n", ret); > return ret; > } > > @@ -153,18 +152,14 @@ static int da9052_rtc_read_time(struct device > *dev, struct rtc_time *rtc_tm) > rtc_tm->tm_sec = v[0] & DA9052_RTC_SEC; > > ret = rtc_valid_tm(rtc_tm); > - if (ret != 0) { > - rtc_err(rtc->da9052, "rtc_valid_tm failed: %d\n", ret); > - return ret; > - } > - > - return 0; > + return ret; > } > > static int da9052_rtc_set_time(struct device *dev, struct rtc_time *tm) { > struct da9052_rtc *rtc; > uint8_t v[6]; > + int ret; > > rtc = dev_get_drvdata(dev); > > @@ -175,7 +170,10 @@ static int da9052_rtc_set_time(struct device *dev, > struct rtc_time *tm) > v[4] = tm->tm_mon + 1; > v[5] = tm->tm_year - 100; > > - return da9052_group_write(rtc->da9052, DA9052_COUNT_S_REG, 6, > v); > + ret = da9052_group_write(rtc->da9052, DA9052_COUNT_S_REG, 6, > v); > + if (ret < 0) > + rtc_err(rtc, "failed to set RTC time: %d\n", ret); > + return ret; > } > > static int da9052_rtc_read_alarm(struct device *dev, struct rtc_wkalrm > *alrm) @@ -184,13 +182,13 @@ static int da9052_rtc_read_alarm(struct > device *dev, struct rtc_wkalrm *alrm) > struct rtc_time *tm = &alrm->time; > struct da9052_rtc *rtc = dev_get_drvdata(dev); > > - ret = da9052_read_alarm(rtc->da9052, tm); > - > - if (ret) > + ret = da9052_read_alarm(rtc, tm); > + if (ret < 0) { > + rtc_err(rtc, "failed to read RTC alarm: %d\n", ret); > return ret; > + } > > - alrm->enabled = da9052_rtc_get_alarm_status(rtc->da9052); > - > + alrm->enabled = da9052_rtc_get_alarm_status(rtc); > return 0; > } > > @@ -200,16 +198,15 @@ static int da9052_rtc_set_alarm(struct device > *dev, struct rtc_wkalrm *alrm) > struct rtc_time *tm = &alrm->time; > struct da9052_rtc *rtc = dev_get_drvdata(dev); > > - ret = da9052_rtc_enable_alarm(rtc->da9052, 0); > + ret = da9052_rtc_enable_alarm(rtc, 0); > if (ret < 0) > return ret; > > - ret = da9052_set_alarm(rtc->da9052, tm); > - if (ret) > + ret = da9052_set_alarm(rtc, tm); > + if (ret < 0) > return ret; > > - ret = da9052_rtc_enable_alarm(rtc->da9052, 1); > - > + ret = da9052_rtc_enable_alarm(rtc, 1); > return ret; > } > > @@ -217,7 +214,7 @@ static int da9052_rtc_alarm_irq_enable(struct device > *dev, unsigned int enabled) { > struct da9052_rtc *rtc = dev_get_drvdata(dev); > > - return da9052_rtc_enable_alarm(rtc->da9052, enabled); > + return da9052_rtc_enable_alarm(rtc, enabled); > } > > static const struct rtc_class_ops da9052_rtc_ops = { @@ -239,10 +236,23 > @@ static int da9052_rtc_probe(struct platform_device *pdev) > > rtc->da9052 = dev_get_drvdata(pdev->dev.parent); > platform_set_drvdata(pdev, rtc); > + > + ret = da9052_reg_write(rtc->da9052, DA9052_BBAT_CONT_REG, > 0xFE); > + if (ret < 0) { > + rtc_err(rtc, > + "Failed to setup RTC battery charging: %d\n", ret); > + return ret; > + } > + > + ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG, > + DA9052_ALARM_Y_TICK_ON, 0); > + if (ret != 0) > + rtc_err(rtc, "Failed to disable TICKS: %d\n", ret); > + > ret = da9052_request_irq(rtc->da9052, DA9052_IRQ_ALARM, "ALM", > da9052_rtc_irq, rtc); > if (ret != 0) { > - rtc_err(rtc->da9052, "irq registration failed: %d\n", ret); > + rtc_err(rtc, "irq registration failed: %d\n", ret); > return ret; > } > > @@ -261,7 +271,7 @@ static struct platform_driver da9052_rtc_driver = { > > module_platform_driver(da9052_rtc_driver); > > -MODULE_AUTHOR("David Dajun Chen <dc...@diasemi.com>"); > +MODULE_AUTHOR("Anthony Olech <anthony.ol...@diasemi.com>"); > MODULE_DESCRIPTION("RTC driver for Dialog DA9052 PMIC"); > MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:da9052-rtc"); > -- > end-of-patch 1/1 for drivers/rtc: da9052: ALARM causes interrupt storm V1 -- 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/
