From: arno@natisbad•org (Arnaud Ebalard)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCHv1 6/6] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver
Date: Mon, 01 Dec 2014 21:11:21 +0100 [thread overview]
Message-ID: <87tx1fnn9y.fsf@natisbad.org> (raw)
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")
Hi Uwe,
Uwe Kleine-K?nig <uwe@kleine-koenig•org> 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+
next prev parent reply other threads:[~2014-12-01 20:11 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-14 23:06 [PATCHv1 0/6] rtc: rtc-isl12057: fixes and alarm support Arnaud Ebalard
2014-11-14 23:06 ` [PATCHv1 1/6] rtc: rtc-isl12057: fix masking of register values Arnaud Ebalard
2014-11-14 23:06 ` [PATCHv1 2/6] rtc: rtc-isl12057: add support for century bit Arnaud Ebalard
2014-11-14 23:06 ` [PATCHv1 3/6] rtc: rtc-isl12057: add proper handling of oscillator failure bit Arnaud Ebalard
2014-11-14 23:07 ` [PATCHv1 4/6] rtc: rtc-isl12057: fix isil vs isl naming for intersil Arnaud Ebalard
2014-12-10 21:30 ` [rtc-linux] " Andrew Morton
2014-12-11 19:59 ` Arnaud Ebalard
2014-11-14 23:07 ` [PATCHv1 5/6] rtc: rtc-isl12057: report error code upon failure in dev_err() calls Arnaud Ebalard
2014-11-14 23:07 ` [PATCHv1 6/6] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver Arnaud Ebalard
2014-11-27 9:23 ` Uwe Kleine-König
2014-12-01 8:07 ` Arnaud Ebalard
2014-12-01 20:11 ` Arnaud Ebalard [this message]
2014-12-02 8:26 ` Uwe Kleine-König
2014-11-26 18:35 ` [PATCHv1 0/6] rtc: rtc-isl12057: fixes and alarm support Arnaud Ebalard
2014-11-26 18:48 ` Uwe Kleine-König
2014-11-26 19:02 ` Mark Brown
2014-11-26 19:28 ` Arnaud Ebalard
2014-11-26 19:53 ` Andrew Morton
2014-11-26 20:10 ` Arnaud Ebalard
2014-11-26 19:46 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tx1fnn9y.fsf@natisbad.org \
--to=arno@natisbad$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox