| Message ID | 148403c0ab1d556cbb99d9242c65f714a77843e5.1415222752.git.arno@natisbad.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: > +static int _isl12057_rtc_toggle_alarm(struct device *dev, int enable) > +{ > + struct isl12057_rtc_data *data = dev_get_drvdata(dev); I can't help but think that toggle is a confusing name for this. enable? > + ret = regmap_update_bits(data->regmap, ISL12057_REG_INT, > + ISL12057_REG_INT_A1IE, > + enable ? ISL12057_REG_INT_A1IE : 0); > + if (ret) > + dev_err(dev, "%s: writing INT failed\n", __func__); It's generally helpful to log the error code as well. > +static int _isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) > { > struct isl12057_rtc_data *data = dev_get_drvdata(dev); > u8 regs[ISL12057_RTC_SEC_LEN]; > int ret; > > - mutex_lock(&data->lock); > ret = regmap_bulk_read(data->regmap, ISL12057_REG_RTC_SC, regs, > ISL12057_RTC_SEC_LEN); > - mutex_unlock(&data->lock); > - This is a perfectly sensible change (there's no need for the lock, regmap locks the physical I/O and there's no other interaction) but it's not related to enabling alarm functionality so should be in a separate patch. > +static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct isl12057_rtc_data *data = dev_get_drvdata(dev); > + int ret; > + > + mutex_lock(&data->lock); > + ret = _isl12057_rtc_read_time(dev, tm); > + mutex_unlock(&data->lock); Why lock? I guess this is due to the above change but it seems better to just not lock since it's redundant. > + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); > + if (ret) > + goto err_unlock; > + > + ret = rtc_tm_to_time(alarm_tm, &alarm_secs); > + if (ret) > + goto err_unlock; > + > + /* If alarm time is before current time, disable the alarm */ > + if (!alarm->enabled || alarm_secs <= rtc_secs) { > + enable = 0; Shouldn't there be some margin for time rolling forwards when checking for alarm times in the past (or just refuse to set them, I've not checked if this is following existing practice for RTC drivers)? > + if (client->irq > 0) { > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + isl12057_rtc_interrupt, > + IRQF_SHARED|IRQF_ONESHOT, > + DRV_NAME, client); > + if (!ret) { > + device_init_wakeup(&client->dev, true); > + } else { > + dev_err(dev, "irq %d unavailable, no alarm support\n", > + client->irq); > + client->irq = 0; > + } > + } > + None of the alarm functionality checks to see if there's actually an IRQ - is that OK? I'd at least expect the alarm interrupt enable call to check if the interrupt is wired up. It's also bad form to overwrite client->irq - it's possible a future probe might work (eg, if the driver for the interrupt controller gets loaded). Ideally we'd handle deferred probe, though I don't think the interrupt core supports that yet. > +err: > + if (client->irq > 0) > + free_irq(client->irq, client); The whole point with devm_ is that you don't need to manually free anything, remove this (and the entire remove function).
On 11/05/2014 09:50 PM, Mark Brown wrote: > On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: > >> +static int _isl12057_rtc_toggle_alarm(struct device *dev, int enable) >> +{ >> + struct isl12057_rtc_data *data = dev_get_drvdata(dev); > > I can't help but think that toggle is a confusing name for this. > enable? > If I recall correctly we had this argument before. Problem is that the function can also _disable_ the alarm, so to name it enable is just as misleading or confusing. update_alarm, maybe ? Guenter
On Wed, Nov 05, 2014 at 09:59:47PM -0800, Guenter Roeck wrote: > On 11/05/2014 09:50 PM, Mark Brown wrote: > >I can't help but think that toggle is a confusing name for this. > >enable? > If I recall correctly we had this argument before. Problem is that the > function can also _disable_ the alarm, so to name it enable is just > as misleading or confusing. update_alarm, maybe ? update seems reasonable, yes. toggle I'd expect to invert the state.
Hello, On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: > +#ifdef CONFIG_PM_SLEEP > +static int isl12057_rtc_suspend(struct device *dev) > +{ > + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); drivers/rtc/rtc-isl12057.c: In function 'isl12057_rtc_suspend': drivers/rtc/rtc-isl12057.c:551:56: error: 'client' undeclared (first use in this function) > + > + if (device_may_wakeup(dev)) > + return enable_irq_wake(rtc_data->irq); > + > + return 0; > } Best regards Uwe
Hi Uwe, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: >> +#ifdef CONFIG_PM_SLEEP >> +static int isl12057_rtc_suspend(struct device *dev) >> +{ >> + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); > drivers/rtc/rtc-isl12057.c: In function 'isl12057_rtc_suspend': > drivers/rtc/rtc-isl12057.c:551:56: error: 'client' undeclared (first > use in this function) I guess I forgot to do a git commit after my last quilt refresh before pushing the patches. Sorry for that. Cheers, a+
Hi, Mark Brown <broonie@kernel.org> writes: > On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: > >> +static int _isl12057_rtc_toggle_alarm(struct device *dev, int enable) >> +{ >> + struct isl12057_rtc_data *data = dev_get_drvdata(dev); > > I can't help but think that toggle is a confusing name for this. > enable? I'll fix the name to use update_alarm, as proposed by Guenter. >> + ret = regmap_update_bits(data->regmap, ISL12057_REG_INT, >> + ISL12057_REG_INT_A1IE, >> + enable ? ISL12057_REG_INT_A1IE : 0); >> + if (ret) >> + dev_err(dev, "%s: writing INT failed\n", __func__); > > It's generally helpful to log the error code as well. WILCO >> +static int _isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) >> { >> struct isl12057_rtc_data *data = dev_get_drvdata(dev); >> u8 regs[ISL12057_RTC_SEC_LEN]; >> int ret; >> >> - mutex_lock(&data->lock); >> ret = regmap_bulk_read(data->regmap, ISL12057_REG_RTC_SC, regs, >> ISL12057_RTC_SEC_LEN); >> - mutex_unlock(&data->lock); >> - > > This is a perfectly sensible change (there's no need for the lock, > regmap locks the physical I/O and there's no other interaction) but it's > not related to enabling alarm functionality so should be in a separate > patch. > >> +static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) >> +{ >> + struct isl12057_rtc_data *data = dev_get_drvdata(dev); >> + int ret; >> + >> + mutex_lock(&data->lock); >> + ret = _isl12057_rtc_read_time(dev, tm); >> + mutex_unlock(&data->lock); > > Why lock? I guess this is due to the above change but it seems better > to just not lock since it's redundant. I am aware regmap has an inner lock to protect access. But later in the patch, there are functions (e.g. isl12057_rtc_set_alarm()) which need to do various consecutive accesses to the device; the main purpose of the mutex is to protect those multiple accesses. In those functions, I cannot call functions which do try and get the lock, hence the creation of an underscore version (_isl12057_rtc_read_time()) which does not lock. That being said, regarding isl12057_rtc_read_time() (the version I declare in rtc_ops structure), I agree that it does not need to get the lock as it only reads (and does not modify the device) and the regmap inner lock protection is ok in that context. As it is the only one to be in that case (read_alarm() is more complex and needs the lock). >> + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); >> + if (ret) >> + goto err_unlock; >> + >> + ret = rtc_tm_to_time(alarm_tm, &alarm_secs); >> + if (ret) >> + goto err_unlock; >> + >> + /* If alarm time is before current time, disable the alarm */ >> + if (!alarm->enabled || alarm_secs <= rtc_secs) { >> + enable = 0; > > Shouldn't there be some margin for time rolling forwards when checking > for alarm times in the past (or just refuse to set them, I've not > checked if this is following existing practice for RTC drivers)? No strong feeling on that point. ISL12008 on which this driver is based has a similar logic. >> + if (client->irq > 0) { >> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, >> + isl12057_rtc_interrupt, >> + IRQF_SHARED|IRQF_ONESHOT, >> + DRV_NAME, client); >> + if (!ret) { >> + device_init_wakeup(&client->dev, true); >> + } else { >> + dev_err(dev, "irq %d unavailable, no alarm support\n", >> + client->irq); >> + client->irq = 0; >> + } >> + } >> + > > None of the alarm functionality checks to see if there's actually an IRQ > - is that OK? I'd at least expect the alarm interrupt enable call to > check if the interrupt is wired up. I can add those check BUT I would like some directions in order to support the following use case too. Current three in-tree users of ISL12057 are NAS devices (Netgear ReadyNAS 102, 104 and 2120). All of them *do not have* the interrupt pin of the RTC chip connected to an interrupt line of the SoC. Nonetheless, the IRQ line of the chip being connected to a PMIC on the board (TI TPS65251 [1] on ReadyNAS 102 and 104, I do not know for ReadyNAS 2120). When the device is off and the alarm rings, the device gets powered on. AFAICT, the IRQ coming on the TPS65251 is not replicated on any line of the SoC. I think it's possible w/ some soldering to get somewhere where the RTC framework wants me to be and finish the alarm part to have it merged upstream but that's of limited interest if in-tree user cannot use it to fit their need, i.e. configure an alarm value and enable the associated interrupt which is routed externally, i.e. not visible by the SoC. FWIW, we had a similar discussion a while ago, during driver inclusion: http://marc.info/?l=devicetree&m=138109313118504&w=2 Uwe, any idea? > It's also bad form to overwrite client->irq - it's possible a future > probe might work (eg, if the driver for the interrupt controller gets > loaded). Ideally we'd handle deferred probe, though I don't think the > interrupt core supports that yet. I'll add a field in my private structure to reference the irq, modify it at will and leave client->irq alone. >> +err: >> + if (client->irq > 0) >> + free_irq(client->irq, client); > > The whole point with devm_ is that you don't need to manually free > anything, remove this (and the entire remove function). good point. I'll remove the free_irq() call. I'll keep remove function though for other reasons; I think I will need to have a conditional call to device_init_wakeup() in it. a+ [1]: http://natisbad.org/NAS/refs/tps65251.pdf
Hi, On Fri, Nov 07, 2014 at 12:30:48AM +0100, Arnaud Ebalard wrote: > Mark Brown <broonie@kernel.org> writes: > > On Wed, Nov 05, 2014 at 10:42:52PM +0100, Arnaud Ebalard wrote: > > >> + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); > >> + if (ret) > >> + goto err_unlock; > >> + > >> + ret = rtc_tm_to_time(alarm_tm, &alarm_secs); > >> + if (ret) > >> + goto err_unlock; > >> + > >> + /* If alarm time is before current time, disable the alarm */ > >> + if (!alarm->enabled || alarm_secs <= rtc_secs) { > >> + enable = 0; > > > > Shouldn't there be some margin for time rolling forwards when checking > > for alarm times in the past (or just refuse to set them, I've not > > checked if this is following existing practice for RTC drivers)? > > No strong feeling on that point. ISL12008 on which this driver is based > has a similar logic. Some time ago I already had the feeling that there is much "rank growth" in the rtc drivers. I guess this is because maintenance of the rtc subsystem isn't optimal and there are no guidelines (at least I'm not aware of any) about detail questions. > >> + if (client->irq > 0) { > >> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > >> + isl12057_rtc_interrupt, > >> + IRQF_SHARED|IRQF_ONESHOT, > >> + DRV_NAME, client); > >> + if (!ret) { > >> + device_init_wakeup(&client->dev, true); > >> + } else { > >> + dev_err(dev, "irq %d unavailable, no alarm support\n", > >> + client->irq); > >> + client->irq = 0; > >> + } > >> + } > >> + > > > > None of the alarm functionality checks to see if there's actually an IRQ > > - is that OK? I'd at least expect the alarm interrupt enable call to > > check if the interrupt is wired up. > > I can add those check BUT I would like some directions in order to > support the following use case too. > > Current three in-tree users of ISL12057 are NAS devices (Netgear > ReadyNAS 102, 104 and 2120). All of them *do not have* the interrupt pin I assume you don't have schematics of the devices, right? If so, do you think it might be worth to try to get them from Netgear? Just to make sure that there really is no pin connected to the SoC. > of the RTC chip connected to an interrupt line of the SoC. Nonetheless, > the IRQ line of the chip being connected to a PMIC on the board (TI > TPS65251 [1] on ReadyNAS 102 and 104, I do not know for ReadyNAS Looking over the manual it seems there is no way to let the PMIC forward the irq either. > 2120). When the device is off and the alarm rings, the device gets > powered on. AFAICT, the IRQ coming on the TPS65251 is not replicated on > any line of the SoC. > > I think it's possible w/ some soldering to get somewhere where the RTC > framework wants me to be and finish the alarm part to have it merged > upstream but that's of limited interest if in-tree user cannot use it to > fit their need, i.e. configure an alarm value and enable the associated > interrupt which is routed externally, i.e. not visible by the SoC. Do you need to enable the irq somewhere else (apart from in the RTC device)? I guess not. > FWIW, we had a similar discussion a while ago, during driver inclusion: > > http://marc.info/?l=devicetree&m=138109313118504&w=2 > > Uwe, any idea? What is the problem of a not-wired-up irq line? Does the rtc framework need to change to allow setting an alarm without the irq line being hooked up? Is it "only" that the alarm is missed? Irq polling probably isn't sensible? I assume it's not that unusual that an rtc irq doesn't trigger a system irq, so having that supported nicely would be great. Looking through the rtc's datasheet, the device is a tad more complicated than the current driver handles. There are two irq lines and three functions that can be routed through these (alarm1, alarm2 and clkout; not all combinations are possible). Best regards Uwe
Hi Uwe, Thanks for your support. Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: >> No strong feeling on that point. ISL12008 on which this driver is based >> has a similar logic. > Some time ago I already had the feeling that there is much "rank growth" > in the rtc drivers. I guess this is because maintenance of the rtc > subsystem isn't optimal and there are no guidelines (at least I'm not > aware of any) about detail questions. I had that feeling too when I pushed the ISL12057 driver. >> > None of the alarm functionality checks to see if there's actually an IRQ >> > - is that OK? I'd at least expect the alarm interrupt enable call to >> > check if the interrupt is wired up. >> >> I can add those check BUT I would like some directions in order to >> support the following use case too. >> >> Current three in-tree users of ISL12057 are NAS devices (Netgear >> ReadyNAS 102, 104 and 2120). All of them *do not have* the interrupt pin > I assume you don't have schematics of the devices, right? If so, do you > think it might be worth to try to get them from Netgear? Just to make > sure that there really is no pin connected to the SoC. I'll ask but I am skeptical: I already tested all the MPP of the SoC so unless I missed something, it's unlikely. >> of the RTC chip connected to an interrupt line of the SoC. Nonetheless, >> the IRQ line of the chip being connected to a PMIC on the board (TI >> TPS65251 [1] on ReadyNAS 102 and 104, I do not know for ReadyNAS > Looking over the manual it seems there is no way to let the PMIC forward > the irq either. > >> 2120). When the device is off and the alarm rings, the device gets >> powered on. AFAICT, the IRQ coming on the TPS65251 is not replicated on >> any line of the SoC. >> >> I think it's possible w/ some soldering to get somewhere where the RTC >> framework wants me to be and finish the alarm part to have it merged >> upstream but that's of limited interest if in-tree user cannot use it to >> fit their need, i.e. configure an alarm value and enable the associated >> interrupt which is routed externally, i.e. not visible by the SoC. > Do you need to enable the irq somewhere else (apart from in the RTC > device)? I guess not. This is the problem: I just need to enable the interrupt in the ISL12057 (flag in regs) but w/o dealing with SoC/system interrupt, i.e. w/o having a client->irq passed to the driver. In current RTC framework, alarm support requires a client->irq and a working interrupt line w/ the SoC. >> FWIW, we had a similar discussion a while ago, during driver inclusion: >> >> http://marc.info/?l=devicetree&m=138109313118504&w=2 >> >> Uwe, any idea? > What is the problem of a not-wired-up irq line? Does the rtc framework > need to change to allow setting an alarm without the irq line being > hooked up? yes. > Is it "only" that the alarm is missed? In practice, yes. But RTC framework does not support that. Or maybe there is some trick a maintainer would be aware of to support that scenario. Something involving: - returning some specific value in alarm_irq_enable handler - calling device_init_wakeup(dev, true); w/o having an IRQ line - extend workaround started in c9f5c7e7a84f and 4a649903f91232 and expose those via dt - ... > Irq polling probably isn't sensible? > > I assume it's not that unusual that an rtc irq doesn't trigger a system > irq, so having that supported nicely would be great. Now, we're two to think that way ;-) > Looking through the rtc's datasheet, the device is a tad more > complicated than the current driver handles. There are two irq lines and > three functions that can be routed through these (alarm1, alarm2 and > clkout; not all combinations are possible). Yes, but I wanted to handle the problem at hand before soldering IRQ#1 on my RN102 and add more feature. a+
On Fri, Nov 07, 2014 at 12:30:48AM +0100, Arnaud Ebalard wrote: > Mark Brown <broonie@kernel.org> writes: > > None of the alarm functionality checks to see if there's actually an IRQ > > - is that OK? I'd at least expect the alarm interrupt enable call to > > check if the interrupt is wired up. > I can add those check BUT I would like some directions in order to > support the following use case too. > Current three in-tree users of ISL12057 are NAS devices (Netgear > ReadyNAS 102, 104 and 2120). All of them *do not have* the interrupt pin > of the RTC chip connected to an interrupt line of the SoC. Nonetheless, > the IRQ line of the chip being connected to a PMIC on the board (TI > TPS65251 [1] on ReadyNAS 102 and 104, I do not know for ReadyNAS > 2120). When the device is off and the alarm rings, the device gets > powered on. AFAICT, the IRQ coming on the TPS65251 is not replicated on > any line of the SoC. It's OK to support not having the interrupt, the point is that if there isn't an interrupt the driver shouldn't support operations which rely on the interrupt to succeed - things like enabling the alarm IRQ for example.
On Fri, Nov 07, 2014 at 08:58:43AM +0100, Uwe Kleine-König wrote: > On Fri, Nov 07, 2014 at 12:30:48AM +0100, Arnaud Ebalard wrote: > > Mark Brown <broonie@kernel.org> writes: > > FWIW, we had a similar discussion a while ago, during driver inclusion: > > > > http://marc.info/?l=devicetree&m=138109313118504&w=2 > > > > Uwe, any idea? > What is the problem of a not-wired-up irq line? Does the rtc framework > need to change to allow setting an alarm without the irq line being > hooked up? Is it "only" that the alarm is missed? Irq polling probably > isn't sensible? The problem is that we've got operations like alarm_irq_enable() which probably shouldn't be reporting success if we don't have an interrupt line. Setting the alarm with no interrupt might still be useful but that at least ought to fail I'd expect. > > I assume it's not that unusual that an rtc irq doesn't trigger a system > irq, so having that supported nicely would be great. > > Looking through the rtc's datasheet, the device is a tad more > complicated than the current driver handles. There are two irq lines and > three functions that can be routed through these (alarm1, alarm2 and > clkout; not all combinations are possible). > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | >
diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt index edac97c0f756..f716e7da44a9 100644 --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt @@ -55,7 +55,7 @@ fsl,sgtl5000 SGTL5000: Ultra Low-Power Audio Codec gmt,g751 G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface infineon,slb9635tt Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz) infineon,slb9645tt Infineon SLB9645 I2C TPM (new protocol, max 400khz) -isil,isl12057 Intersil ISL12057 I2C RTC Chip +isil,isl12057 Intersil ISL12057 I2C RTC/Alarm Chip maxim,ds1050 5 Bit Programmable, Pulse-Width Modulator maxim,max1237 Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs maxim,max6625 9-Bit/12-Bit Temperature Sensors with I²C-Compatible Serial Interface diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c index adb0646236b1..56d222f31baa 100644 --- a/drivers/rtc/rtc-isl12057.c +++ b/drivers/rtc/rtc-isl12057.c @@ -1,5 +1,5 @@ /* - * rtc-isl12057 - Driver for Intersil ISL12057 I2C Real Time Clock + * rtc-isl12057 - Driver for Intersil ISL12057 I2C Real Time Clock / Alarm * * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org> * @@ -27,7 +27,6 @@ #include <linux/i2c.h> #include <linux/bcd.h> #include <linux/of.h> -#include <linux/of_device.h> #include <linux/regmap.h> #define DRV_NAME "rtc-isl12057" @@ -78,6 +77,7 @@ #define ISL12057_MEM_MAP_LEN 0x10 struct isl12057_rtc_data { + struct rtc_device *rtc; struct regmap *regmap; struct mutex lock; }; @@ -148,17 +148,42 @@ static int isl12057_i2c_validate_chip(struct regmap *regmap) return 0; } -static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) +static int _isl12057_rtc_clear_alarm(struct device *dev) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + int ret; + + ret = regmap_update_bits(data->regmap, ISL12057_REG_SR, + ISL12057_REG_SR_A1F, 0); + + if (ret) + dev_err(dev, "%s: clearing alarm failed\n", __func__); + + return ret; +} + +static int _isl12057_rtc_toggle_alarm(struct device *dev, int enable) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + int ret; + + ret = regmap_update_bits(data->regmap, ISL12057_REG_INT, + ISL12057_REG_INT_A1IE, + enable ? ISL12057_REG_INT_A1IE : 0); + if (ret) + dev_err(dev, "%s: writing INT failed\n", __func__); + + return ret; +} + +static int _isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) { struct isl12057_rtc_data *data = dev_get_drvdata(dev); u8 regs[ISL12057_RTC_SEC_LEN]; int ret; - mutex_lock(&data->lock); ret = regmap_bulk_read(data->regmap, ISL12057_REG_RTC_SC, regs, ISL12057_RTC_SEC_LEN); - mutex_unlock(&data->lock); - if (ret) { dev_err(dev, "%s: RTC read failed\n", __func__); return ret; @@ -169,6 +194,176 @@ static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) return rtc_valid_tm(tm); } +static int isl12057_rtc_toggle_alarm(struct device *dev, int enable) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + int ret; + + mutex_lock(&data->lock); + ret = _isl12057_rtc_toggle_alarm(dev, enable); + mutex_unlock(&data->lock); + + return ret; +} + +static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + int ret; + + mutex_lock(&data->lock); + ret = _isl12057_rtc_read_time(dev, tm); + mutex_unlock(&data->lock); + + return ret; +} + +static int isl12057_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + struct rtc_time rtc_tm, *alarm_tm = &alarm->time; + unsigned long rtc_secs, alarm_secs; + u8 regs[ISL12057_A1_SEC_LEN]; + unsigned int ir; + int ret; + + mutex_lock(&data->lock); + ret = regmap_bulk_read(data->regmap, ISL12057_REG_A1_SC, regs, + ISL12057_A1_SEC_LEN); + if (ret) { + dev_err(dev, "%s: reading alarm section failed\n", __func__); + goto err_unlock; + } + + alarm_tm->tm_sec = bcd2bin(regs[0] & 0x7f); + alarm_tm->tm_min = bcd2bin(regs[1] & 0x7f); + alarm_tm->tm_hour = bcd2bin(regs[2] & 0x3f); + alarm_tm->tm_mday = bcd2bin(regs[3] & 0x3f); + alarm_tm->tm_wday = -1; + + /* + * The alarm section does not store year/month. We use the ones in rtc + * section as a basis and increment month and then year if needed to get + * alarm after current time. + */ + ret = _isl12057_rtc_read_time(dev, &rtc_tm); + if (ret) + goto err_unlock; + + alarm_tm->tm_year = rtc_tm.tm_year; + alarm_tm->tm_mon = rtc_tm.tm_mon; + + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); + if (ret) + goto err_unlock; + + ret = rtc_tm_to_time(alarm_tm, &alarm_secs); + if (ret) + goto err_unlock; + + if (alarm_secs < rtc_secs) { + if (alarm_tm->tm_mon == 11) { + alarm_tm->tm_mon = 0; + alarm_tm->tm_year += 1; + } else { + alarm_tm->tm_mon += 1; + } + } + + ret = regmap_read(data->regmap, ISL12057_REG_INT, &ir); + if (ret) { + dev_err(dev, "%s: reading INT failed\n", __func__); + goto err_unlock; + } + + alarm->enabled = !!(ir & ISL12057_REG_INT_A1IE); + +err_unlock: + mutex_unlock(&data->lock); + + return ret; +} + +static int isl12057_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm) +{ + struct isl12057_rtc_data *data = dev_get_drvdata(dev); + struct rtc_time *alarm_tm = &alarm->time; + unsigned long rtc_secs, alarm_secs; + u8 regs[ISL12057_A1_SEC_LEN]; + struct rtc_time rtc_tm; + int ret, enable = 1; + + mutex_lock(&data->lock); + ret = _isl12057_rtc_read_time(dev, &rtc_tm); + if (ret) + goto err_unlock; + + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); + if (ret) + goto err_unlock; + + ret = rtc_tm_to_time(alarm_tm, &alarm_secs); + if (ret) + goto err_unlock; + + /* If alarm time is before current time, disable the alarm */ + if (!alarm->enabled || alarm_secs <= rtc_secs) { + enable = 0; + } else { + /* + * Chip only support alarms up to one month in the future. Let's + * return an error if we get something after that limit. + * Comparison is done by incrementing rtc_tm month field by one + * and checking alarm value is still below. + */ + if (rtc_tm.tm_mon == 11) { /* handle year wrapping */ + rtc_tm.tm_mon = 0; + rtc_tm.tm_year += 1; + } else { + rtc_tm.tm_mon += 1; + } + + ret = rtc_tm_to_time(&rtc_tm, &rtc_secs); + if (ret) + goto err_unlock; + + if (alarm_secs > rtc_secs) { + dev_err(dev, "%s: max one month in the future\n", + __func__); + ret = -EINVAL; + goto err_unlock; + } + } + + /* Disable the alarm before modifying it */ + ret = _isl12057_rtc_toggle_alarm(dev, 0); + if (ret < 0) { + dev_err(dev, "Unable to disable the alarm\n"); + goto err_unlock; + } + + /* Program alarm registers */ + regs[0] = bin2bcd(alarm_tm->tm_sec) & 0x7f; + regs[1] = bin2bcd(alarm_tm->tm_min) & 0x7f; + regs[2] = bin2bcd(alarm_tm->tm_hour) & 0x3f; + regs[3] = bin2bcd(alarm_tm->tm_mday) & 0x3f; + + ret = regmap_bulk_write(data->regmap, ISL12057_REG_A1_SC, regs, + ISL12057_A1_SEC_LEN); + if (ret < 0) { + dev_err(dev, "%s: writing ALARM section failed\n", __func__); + goto err_unlock; + } + + /* Enable or disable alarm */ + ret = _isl12057_rtc_toggle_alarm(dev, enable); + +err_unlock: + mutex_unlock(&data->lock); + + return ret; +} + static int isl12057_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct isl12057_rtc_data *data = dev_get_drvdata(dev); @@ -226,9 +421,42 @@ static int isl12057_check_rtc_status(struct device *dev, struct regmap *regmap) return 0; } +static int isl12057_rtc_alarm_irq_enable(struct device *dev, + unsigned int enable) +{ + return isl12057_rtc_toggle_alarm(dev, enable); +} + +static irqreturn_t isl12057_rtc_interrupt(int irq, void *data) +{ + struct i2c_client *client = data; + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); + struct rtc_device *rtc = rtc_data->rtc; + int ret, handled = IRQ_NONE; + unsigned int sr; + + ret = regmap_read(rtc_data->regmap, ISL12057_REG_SR, &sr); + if (!ret && (sr & ISL12057_REG_SR_A1F)) { + dev_dbg(&client->dev, "RTC alarm!\n"); + + rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF); + + /* Acknowledge and disable the alarm */ + _isl12057_rtc_clear_alarm(&client->dev); + _isl12057_rtc_toggle_alarm(&client->dev, 0); + + handled = IRQ_HANDLED; + } + + return handled; +} + static const struct rtc_class_ops rtc_ops = { .read_time = isl12057_rtc_read_time, .set_time = isl12057_rtc_set_time, + .read_alarm = isl12057_rtc_read_alarm, + .set_alarm = isl12057_rtc_set_alarm, + .alarm_irq_enable = isl12057_rtc_alarm_irq_enable, }; static struct regmap_config isl12057_rtc_regmap_config = { @@ -273,10 +501,75 @@ static int isl12057_probe(struct i2c_client *client, data->regmap = regmap; dev_set_drvdata(dev, data); + if (client->irq > 0) { + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + isl12057_rtc_interrupt, + IRQF_SHARED|IRQF_ONESHOT, + DRV_NAME, client); + if (!ret) { + device_init_wakeup(&client->dev, true); + } else { + dev_err(dev, "irq %d unavailable, no alarm support\n", + client->irq); + client->irq = 0; + } + } + rtc = devm_rtc_device_register(dev, DRV_NAME, &rtc_ops, THIS_MODULE); - return PTR_ERR_OR_ZERO(rtc); + ret = PTR_ERR_OR_ZERO(rtc); + if (ret) { + dev_err(dev, "unable to register RTC device\n"); + goto err; + + /* + * XXX we may additionally need to unregister wakeup source + * on error: see https://lkml.org/lkml/2014/10/9/425 + */ + } + + data->rtc = rtc; + + return ret; + +err: + if (client->irq > 0) + free_irq(client->irq, client); + + return ret; +} + +static int isl12057_remove(struct i2c_client *client) +{ + if (client->irq > 0) + free_irq(client->irq, client); + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int isl12057_rtc_suspend(struct device *dev) +{ + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); + + if (device_may_wakeup(dev)) + return enable_irq_wake(rtc_data->irq); + + return 0; } +static int isl12057_rtc_resume(struct device *dev) +{ + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); + + if (device_may_wakeup(dev)) + return disable_irq_wake(rtc_data->irq); + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(isl12057_rtc_pm_ops, isl12057_rtc_suspend, + isl12057_rtc_resume); + #ifdef CONFIG_OF static const struct of_device_id isl12057_dt_match[] = { { .compatible = "isil,isl12057" }, @@ -294,13 +587,15 @@ static struct i2c_driver isl12057_driver = { .driver = { .name = DRV_NAME, .owner = THIS_MODULE, + .pm = &isl12057_rtc_pm_ops, .of_match_table = of_match_ptr(isl12057_dt_match), }, .probe = isl12057_probe, + .remove = isl12057_remove, .id_table = isl12057_id, }; module_i2c_driver(isl12057_driver); MODULE_AUTHOR("Arnaud EBALARD <arno@natisbad.org>"); -MODULE_DESCRIPTION("Intersil ISL12057 RTC driver"); +MODULE_DESCRIPTION("Intersil ISL12057 RTC/Alarm driver"); MODULE_LICENSE("GPL");
This patch adds alarm support to Intersil ISL12057 driver. This allows to configure the chip to generate an interrupt when the alarm matches current time value. Alarm can be programmed up to one month in the future. The patch was tested on a Netgear ReadyNAS 102 after some soldering of the IRQ#2 pin of the RTC chip to a MPP line of the SoC (the one used usually handles the reset button). By default, on this device, the interrupt line of the RTC chip is not directly connected to the SoC but to a PMIC which handles powering of the device upon alarm when it is off. The test was performed using a modified .dts file reflecting this change and rtc-test.c program available in Documentation/rtc.txt. This test program ran as expected, which validates alarm supports, including interrupt support. Additional tests were performed on an unmodified ReadyNAS 102 to validate the ability to wake up the device based on a configured alarm value. As a side note, the ISL12057 remains in the list of trivial devices, i.e. no specific DT binding being added by this patch: i2c core automatically handles extraction of IRQ line info from .dts file. For instance, if one wants to reference the interrupt line for the alarm in its .dts file, adding interrupt and interrupt-parent properties should work as expected: isl12057: isl12057@68 { compatible = "isil,isl12057"; interrupt-parent = <&gpio0>; interrupts = <6 IRQ_TYPE_EDGE_FALLING>; reg = <0x68>; }; Signed-off-by: Arnaud Ebalard <arno@natisbad.org> --- .../devicetree/bindings/i2c/trivial-devices.txt | 2 +- drivers/rtc/rtc-isl12057.c | 311 ++++++++++++++++++++- 2 files changed, 304 insertions(+), 9 deletions(-)