From mboxrd@z Thu Jan 1 00:00:00 1970 From: arno@natisbad.org (Arnaud Ebalard) Date: Mon, 01 Dec 2014 21:11:21 +0100 Subject: [PATCHv1 6/6] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver In-Reply-To: <5476ED9F.2070600@kleine-koenig.org> ("Uwe \=\?utf-8\?Q\?Kleine-K\?\= \=\?utf-8\?Q\?\=C3\=B6nig\=22's\?\= message of "Thu, 27 Nov 2014 10:23:43 +0100") References: <986ee8d9d3e6862007f398ffddc2a4bb2368933e.1416006090.git.arno@natisbad.org> <5476ED9F.2070600@kleine-koenig.org> Message-ID: <87tx1fnn9y.fsf@natisbad.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Uwe, Uwe Kleine-K?nig writes: > finally I managed to test this series on my (unmodified) rn104. > > For patch 1: Maybe point out that the issue with the century bit isn't > that critical, because this bit is not expected to be set before year > 2100. It has: This was tested by putting a device 100 years in the future (using a specific kernel due to the inability of userland tools such as date or hwclock to pass year 2038), rebooting on a kernel w/ this patch applied and verifying the device was still 100 years in the future. > For patch 3: This patch adds a few dev_err calls that get later amended > in patch 5 to also include an error code. IMHO these should already be > added in patch 3. Patch 5 should only add it to the already existing > strings (if applicable). will do that next time. > For patch 4: Maybe > s/obsolete/for backwards compatibility, don't use in new code/. > > > Some further comments inline ... > > On 11/15/2014 12:07 AM, Arnaud Ebalard wrote: >> The latter is the one found on current 3 kernel users of the chip >> for which support was initially developed (Netgear ReadyNAS 102, >> 104 and 2120 NAS). On those devices, the IRQ#2 pin of the chip is not >> connected to the SoC but to a PMIC. This allows setting an alarm, >> powering off the device and have it wake up when the alarm rings. >> To support that configuration the driver does the following: >> >> 1. it has alarm_irq_enable() function returns -ENOTTY when no IRQ >> is passed to the driver. >> 2. it marks the device as a wakeup source in all cases (whether an >> IRQ is passed to the driver or not) to have 'wakealarm' sysfs >> entry created. > This is not pretty, but I don't know if there is a nicer alternative. > Maybe this should only be done in the presence of a flag in the device > tree (say: can-wakeup-machine, a prefix would be nice, but "isil," feels > wrong). I can prepare a v0 patch for a "can-wakeup-machine" property to mark the device as a wakeup source when the IRQ is absent. Will fix the prefix in a v1. >> [...] >> FWIW, if someone is looking for a way to test alarm support on a system >> on which the chip IRQ line has the ability to boot the system (e.g. >> ReadyNAS 102, 104, etc): >> >> # echo 0 > /sys/class/rtc/rtc0/wakealarm > Why is this needed? It disable the alarm. It's not required. >> # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm >> # shutdown -h now >> >> With the commandes above, after a minute, the system comes back to life. > s/commandes/commands/ useless french letter. >> + 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: changing alarm interrupt flag failed (%d)\n", >> + __func__, ret); >> + >> + return ret; >> +} > Maybe point out in the commit log that the first alarm (of two) is used, > and the event is signaled on pin $Ididntlookitup. > (IIRC the 2nd alarm register set doesn't support seconds, but in return > has year and month field.) I can add that in the documentation. >> [...] >> + /* >> + * This is needed to have 'wakealarm' sysfs entry available. One >> + * would expect the device to be marked as a wakeup source only >> + * when an IRQ pin of the RTC is routed to an interrupt line of the >> + * CPU. In practice, such an IRQ pin can be connected to a PMIC and >> + * this allows the device to be powered up when RTC alarm rings. > Maybe add the machines you know of that have this setup to the > comment. ditto. >> + */ >> + device_init_wakeup(dev, true); >> + >> + data->rtc = devm_rtc_device_register(dev, DRV_NAME, &rtc_ops, >> + THIS_MODULE); >> + ret = PTR_ERR_OR_ZERO(data->rtc); >> + if (ret) { >> + dev_err(dev, "%s: unable to register RTC device (%d)\n", >> + __func__, ret); >> + goto err; >> + } >> + >> + /* We cannot support UIE mode if we do not have an IRQ line */ >> + if (!data->irq) >> + data->rtc->uie_unsupported = 1; >> + >> +err: >> + return ret; >> } >> >> +static int isl12057_remove(struct i2c_client *client) >> +{ >> + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev); >> + >> + if (rtc_data->irq > 0) >> + device_init_wakeup(&client->dev, false); > I didn't check, but I wonder it that could be/is done by the device > core already? > >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int isl12057_rtc_suspend(struct device *dev) >> +{ >> + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev); >> + >> + if (device_may_wakeup(dev)) >> + return enable_irq_wake(rtc_data->irq); > Does this need a check for data->irq? > >> + return 0; >> +} >> + >> +static int isl12057_rtc_resume(struct device *dev) >> +{ >> + struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev); >> + >> + if (device_may_wakeup(dev)) >> + return disable_irq_wake(rtc_data->irq); > ditto. Hum, I will take a look at those irq vs wakeup cpabilities when adding support for "can-wakeup-machine" property to consolidate the behavior. Cheers, a+ From mboxrd@z Thu Jan 1 00:00:00 1970 From: arno@natisbad.org (Arnaud Ebalard) Subject: Re: [PATCHv1 6/6] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver Date: Mon, 01 Dec 2014 21:11:21 +0100 Message-ID: <87tx1fnn9y.fsf@natisbad.org> References: <986ee8d9d3e6862007f398ffddc2a4bb2368933e.1416006090.git.arno@natisbad.org> <5476ED9F.2070600@kleine-koenig.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <5476ED9F.2070600@kleine-koenig.org> ("Uwe \=\?utf-8\?Q\?Kleine-K\?\= \=\?utf-8\?Q\?\=C3\=B6nig\=22's\?\= message of "Thu, 27 Nov 2014 10:23:43 +0100") Sender: linux-doc-owner@vger.kernel.org To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Cc: Mark Rutland , Alessandro Zummo , Peter Huewe , Linus Walleij , Thierry Reding , Mark Brown , Rob Herring , Pawel Moll , Stephen Warren , Ian Campbell , Grant Likely , devicetree@vger.kernel.org, linux-doc@vger.kernel.org, Rob Landley , rtc-linux@googlegroups.com, Jason Cooper , Guenter Roeck , Jason Gunthorpe , Kumar Gala , linux-arm-kernel@lists.infradead.org, Andrew Morton List-Id: devicetree@vger.kernel.org Hi Uwe, Uwe Kleine-K=C3=B6nig writes: > finally I managed to test this series on my (unmodified) rn104. > > For patch 1: Maybe point out that the issue with the century bit isn'= t > that critical, because this bit is not expected to be set before year > 2100. It has: This was tested by putting a device 100 years in the future (using = a specific kernel due to the inability of userland tools such as date= or hwclock to pass year 2038), rebooting on a kernel w/ this patch app= lied and verifying the device was still 100 years in the future. > For patch 3: This patch adds a few dev_err calls that get later amend= ed > in patch 5 to also include an error code. IMHO these should already b= e > added in patch 3. Patch 5 should only add it to the already existing > strings (if applicable). will do that next time. > For patch 4: Maybe > s/obsolete/for backwards compatibility, don't use in new code/. > > > Some further comments inline ... > > On 11/15/2014 12:07 AM, Arnaud Ebalard wrote: >> The latter is the one found on current 3 kernel users of the chip >> for which support was initially developed (Netgear ReadyNAS 102, >> 104 and 2120 NAS). On those devices, the IRQ#2 pin of the chip is no= t >> connected to the SoC but to a PMIC. This allows setting an alarm, >> powering off the device and have it wake up when the alarm rings. >> To support that configuration the driver does the following: >>=20 >> 1. it has alarm_irq_enable() function returns -ENOTTY when no IRQ >> is passed to the driver. >> 2. it marks the device as a wakeup source in all cases (whether an >> IRQ is passed to the driver or not) to have 'wakealarm' sysfs >> entry created. > This is not pretty, but I don't know if there is a nicer alternative. > Maybe this should only be done in the presence of a flag in the devic= e > tree (say: can-wakeup-machine, a prefix would be nice, but "isil," fe= els > wrong). I can prepare a v0 patch for a "can-wakeup-machine" property to mark th= e device as a wakeup source when the IRQ is absent. Will fix the prefix in a v1. >> [...] >> FWIW, if someone is looking for a way to test alarm support on a sys= tem >> on which the chip IRQ line has the ability to boot the system (e.g. >> ReadyNAS 102, 104, etc): >>=20 >> # echo 0 > /sys/class/rtc/rtc0/wakealarm > Why is this needed? It disable the alarm. It's not required. >> # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakea= larm >> # shutdown -h now >>=20 >> With the commandes above, after a minute, the system comes back to l= ife. > s/commandes/commands/ useless french letter. >> + ret =3D regmap_update_bits(data->regmap, ISL12057_REG_INT, >> + ISL12057_REG_INT_A1IE, >> + enable ? ISL12057_REG_INT_A1IE : 0); >> + if (ret) >> + dev_err(dev, "%s: changing alarm interrupt flag failed (%d)\n", >> + __func__, ret); >> + >> + return ret; >> +} > Maybe point out in the commit log that the first alarm (of two) is us= ed, > and the event is signaled on pin $Ididntlookitup. > (IIRC the 2nd alarm register set doesn't support seconds, but in retu= rn > has year and month field.) I can add that in the documentation. >> [...] >> + /* >> + * This is needed to have 'wakealarm' sysfs entry available. One >> + * would expect the device to be marked as a wakeup source only >> + * when an IRQ pin of the RTC is routed to an interrupt line of th= e >> + * CPU. In practice, such an IRQ pin can be connected to a PMIC an= d >> + * this allows the device to be powered up when RTC alarm rings. > Maybe add the machines you know of that have this setup to the > comment. ditto. >> + */ >> + device_init_wakeup(dev, true); >> + >> + data->rtc =3D devm_rtc_device_register(dev, DRV_NAME, &rtc_ops, >> + THIS_MODULE); >> + ret =3D PTR_ERR_OR_ZERO(data->rtc); >> + if (ret) { >> + dev_err(dev, "%s: unable to register RTC device (%d)\n", >> + __func__, ret); >> + goto err; >> + } >> + >> + /* We cannot support UIE mode if we do not have an IRQ line */ >> + if (!data->irq) >> + data->rtc->uie_unsupported =3D 1; >> + >> +err: >> + return ret; >> } >> =20 >> +static int isl12057_remove(struct i2c_client *client) >> +{ >> + struct isl12057_rtc_data *rtc_data =3D dev_get_drvdata(&client->de= v); >> + >> + if (rtc_data->irq > 0) >> + device_init_wakeup(&client->dev, false); > I didn't check, but I wonder it that could be/is done by the device > core already? > >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int isl12057_rtc_suspend(struct device *dev) >> +{ >> + struct isl12057_rtc_data *rtc_data =3D dev_get_drvdata(dev); >> + >> + if (device_may_wakeup(dev)) >> + return enable_irq_wake(rtc_data->irq); > Does this need a check for data->irq? > >> + return 0; >> +} >> + >> +static int isl12057_rtc_resume(struct device *dev) >> +{ >> + struct isl12057_rtc_data *rtc_data =3D dev_get_drvdata(dev); >> + >> + if (device_may_wakeup(dev)) >> + return disable_irq_wake(rtc_data->irq); > ditto. Hum, I will take a look at those irq vs wakeup cpabilities when adding support for "can-wakeup-machine" property to consolidate the behavior. Cheers, a+