[PATCHv1 0/3] Alarm support for ISL12057 + RFC dt property

[PATCHv1 0/3] Alarm support for ISL12057 + RFC dt property

[Arnaud Ebalard]

Hi,

Changes since v0:

 - Basic alarm support is now the first patch of the series with nothing
   fancy in it: as per Uwe's request, all the "Chip IRQ#2 is not routed
   to the SoC but device can still be woken up with the alarm" is in the
   second patch. Uwe, can you review that one, so that Andrew can then
   merge it if it is ok?
 - Second patch has simple changes going on top of first patch to
   support the use case for current in-tree users, i.e. ReadyNAS 102,
   104 and 2120. It's a simple property with complete documentation
   about what it does and does not.
 - Third and last patch add that property to 102, 104 nd 2120 .dts
   files (in a single patch as suggested by Jason)

Comments welcome,

Cheers,

a+

Arnaud Ebalard (3):
  rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver
  rtc: rtc-isl12057: add isil,irq2-can-wakeup-machine property for in-tree users
  ARM: mvebu: ISL12057 rtc chip can now wake up RN102, RN104 and RN2120

 .../devicetree/bindings/rtc/isil,isl12057.txt      |  78 +++++
 arch/arm/boot/dts/armada-370-netgear-rn102.dts     |   1 +
 arch/arm/boot/dts/armada-370-netgear-rn104.dts     |   1 +
 arch/arm/boot/dts/armada-xp-netgear-rn2120.dts     |   1 +
 drivers/rtc/rtc-isl12057.c                         | 346 ++++++++++++++++++++-
 5 files changed, 419 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12057.txt

-- 
2.1.1

[PATCHv1 1/3] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[Arnaud Ebalard]

This patch adds alarm support to Intersil ISL12057 driver. The chip
supports two separate alarms:

 - Alarm1: works up to one month in the future and accurate to the
           second, associated w/ IRQ#2 pin
 - Alarm2: works up to one month in the future and accurate to the
           minute, associated w/ IRQ#1 or IRQ#2 pin (configuable).

This patch only adds support for Alarm1 which allows to configure
the chip to generate an interrupt on IRQ#2 pin when current time
matches the alarm.

This 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). The test was performed using
a modified .dts file reflecting this change (see below) and rtc-test.c
program available in Documentation/rtc.txt. This test program ran as
expected, which validates alarm supports, including interrupt support.

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 works as expected (if the primary function of your
interrupt pin is not GPIO, you will need some additional pinctrl
properties):

          isl12057: isl12057@68 {
                  compatible = "isil,isl12057";
                  interrupt-parent = <&gpio0>;
                  interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
                  reg = <0x68>;
          };

FWIW, if someone is looking for a way to test alarm support this can
be done in the following way:

    # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
    # shutdown -h now

With the commands above, after a minute, the system comes back to life.

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 drivers/rtc/rtc-isl12057.c | 313 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 305 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
index 6e1fcfb5d7e6..3ec73ad7f2d8 100644
--- a/drivers/rtc/rtc-isl12057.c
+++ b/drivers/rtc/rtc-isl12057.c
@@ -79,8 +79,10 @@
 #define ISL12057_MEM_MAP_LEN	0x10
 
 struct isl12057_rtc_data {
+	struct rtc_device *rtc;
 	struct regmap *regmap;
 	struct mutex lock;
+	int irq;
 };
 
 static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
@@ -160,14 +162,47 @@ 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 (%d)\n", __func__, ret);
+
+	return ret;
+}
+
+static int _isl12057_rtc_update_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: changing alarm interrupt flag failed (%d)\n",
+			__func__, ret);
+
+	return ret;
+}
+
+/*
+ * Note: as we only read from device and do not perform any update, there is
+ * no need for an equivalent function which would try and get driver's main
+ * lock. Here, it is safe for everyone if we just use regmap internal lock
+ * on the device when reading.
+ */
+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];
 	unsigned int sr;
 	int ret;
 
-	mutex_lock(&data->lock);
 	ret = regmap_read(data->regmap, ISL12057_REG_SR, &sr);
 	if (ret) {
 		dev_err(dev, "%s: unable to read oscillator status flag (%d)\n",
@@ -187,8 +222,6 @@ static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm)
 			__func__, ret);
 
 out:
-	mutex_unlock(&data->lock);
-
 	if (ret)
 		return ret;
 
@@ -197,6 +230,168 @@ out:
 	return rtc_valid_tm(tm);
 }
 
+static int isl12057_rtc_update_alarm(struct device *dev, int enable)
+{
+	struct isl12057_rtc_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = _isl12057_rtc_update_alarm(dev, enable);
+	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 (%d)\n",
+			__func__, ret);
+		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 alarm interrupt flag failed (%d)\n",
+			__func__, ret);
+		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 for alarm is one month (%d)\n",
+				__func__, ret);
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+	}
+
+	/* Disable the alarm before modifying it */
+	ret = _isl12057_rtc_update_alarm(dev, 0);
+	if (ret < 0) {
+		dev_err(dev, "%s: unable to disable the alarm (%d)\n",
+			__func__, ret);
+		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 (%d)\n",
+			__func__, ret);
+		goto err_unlock;
+	}
+
+	/* Enable or disable alarm */
+	ret = _isl12057_rtc_update_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);
@@ -262,9 +457,48 @@ 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)
+{
+	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev);
+	int ret = -ENOTTY;
+
+	if (rtc_data->irq)
+		ret = isl12057_rtc_update_alarm(dev, enable);
+
+	return ret;
+}
+
+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_update_alarm(&client->dev, 0);
+
+		handled = IRQ_HANDLED;
+	}
+
+	return handled;
+}
+
 static const struct rtc_class_ops rtc_ops = {
-	.read_time = isl12057_rtc_read_time,
+	.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 = {
@@ -277,7 +511,6 @@ static int isl12057_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct isl12057_rtc_data *data;
-	struct rtc_device *rtc;
 	struct regmap *regmap;
 	int ret;
 
@@ -310,10 +543,72 @@ static int isl12057_probe(struct i2c_client *client,
 	data->regmap = regmap;
 	dev_set_drvdata(dev, data);
 
-	rtc = devm_rtc_device_register(dev, DRV_NAME, &rtc_ops, THIS_MODULE);
-	return PTR_ERR_OR_ZERO(rtc);
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						isl12057_rtc_interrupt,
+						IRQF_SHARED|IRQF_ONESHOT,
+						DRV_NAME, client);
+		if (!ret)
+			data->irq = client->irq;
+		else
+			dev_err(dev, "%s: irq %d unavailable (%d)\n", __func__,
+				client->irq, ret);
+	}
+
+	device_init_wakeup(dev, !!data->irq);
+
+	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)
+		device_init_wakeup(&client->dev, false);
+
+	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);
+
+	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);
+
+	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 = "isl,isl12057" },
@@ -331,9 +626,11 @@ 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);
-- 
2.1.1

Re: [PATCHv1 1/3] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[Uwe Kleine-König]

Hello Arnaud,

On 12/16/2014 12:54 AM, Arnaud Ebalard wrote:
> 
> This patch adds alarm support to Intersil ISL12057 driver. The chip
> supports two separate alarms:
> 
>  - Alarm1: works up to one month in the future and accurate to the
>            second, associated w/ IRQ#2 pin
>  - Alarm2: works up to one month in the future and accurate to the
>            minute, associated w/ IRQ#1 or IRQ#2 pin (configuable).
> 
> This patch only adds support for Alarm1 which allows to configure
> the chip to generate an interrupt on IRQ#2 pin when current time
> matches the alarm.
> 
> This 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). The test was performed using
> a modified .dts file reflecting this change (see below) and rtc-test.c
> program available in Documentation/rtc.txt. This test program ran as
> expected, which validates alarm supports, including interrupt support.
> 
> 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 works as expected (if the primary function of your
> interrupt pin is not GPIO, you will need some additional pinctrl
> properties):
> 
>           isl12057: isl12057@68 {
>                   compatible = "isil,isl12057";
>                   interrupt-parent = <&gpio0>;
>                   interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
>                   reg = <0x68>;
>           };
> 
> FWIW, if someone is looking for a way to test alarm support this can
> be done in the following way:
> 
>     # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
>     # shutdown -h now
> 
> With the commands above, after a minute, the system comes back to life.
This paragraph belongs into the 2nd patch, right?

> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
> index 6e1fcfb5d7e6..3ec73ad7f2d8 100644
> --- a/drivers/rtc/rtc-isl12057.c
> +++ b/drivers/rtc/rtc-isl12057.c
> @@ -79,8 +79,10 @@
>  #define ISL12057_MEM_MAP_LEN	0x10
>  
>  struct isl12057_rtc_data {
> +	struct rtc_device *rtc;
>  	struct regmap *regmap;
>  	struct mutex lock;
> +	int irq;
interrupts are usually unsigned values. Hmm, I see that it simplifies a
bit here, as the function to determine the irq returns the actual value
or a negative error. As there would be problems anyhow for irq values >
INT_MAX probably this comment isn't that important.

> +/*
> + * Note: as we only read from device and do not perform any update, there is
> + * no need for an equivalent function which would try and get driver's main
> + * lock. Here, it is safe for everyone if we just use regmap internal lock
> + * on the device when reading.
> + */
> +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];
>  	unsigned int sr;
>  	int ret;
>  
> -	mutex_lock(&data->lock);
>  	ret = regmap_read(data->regmap, ISL12057_REG_SR, &sr);
>  	if (ret) {
>  		dev_err(dev, "%s: unable to read oscillator status flag (%d)\n",
> @@ -187,8 +222,6 @@ static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  			__func__, ret);
>  
>  out:
> -	mutex_unlock(&data->lock);
> -
>  	if (ret)
>  		return ret;
>  

Is this locking update worth a separate change?

Best regards
Uwe

Re: [PATCHv1 1/3] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[Arnaud Ebalard]

Hi Uwe,

Uwe Kleine-König <uwe@kleine-koenig.org> writes:

> On 12/16/2014 12:54 AM, Arnaud Ebalard wrote:
>> 
>> This patch adds alarm support to Intersil ISL12057 driver. The chip
>> supports two separate alarms:
>> 
>>  - Alarm1: works up to one month in the future and accurate to the
>>            second, associated w/ IRQ#2 pin
>>  - Alarm2: works up to one month in the future and accurate to the
>>            minute, associated w/ IRQ#1 or IRQ#2 pin (configuable).
>> 
>> This patch only adds support for Alarm1 which allows to configure
>> the chip to generate an interrupt on IRQ#2 pin when current time
>> matches the alarm.
>> 
>> This 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). The test was performed using
>> a modified .dts file reflecting this change (see below) and rtc-test.c
>> program available in Documentation/rtc.txt. This test program ran as
>> expected, which validates alarm supports, including interrupt support.
>> 
>> 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 works as expected (if the primary function of your
>> interrupt pin is not GPIO, you will need some additional pinctrl
>> properties):
>> 
>>           isl12057: isl12057@68 {
>>                   compatible = "isil,isl12057";
>>                   interrupt-parent = <&gpio0>;
>>                   interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
>>                   reg = <0x68>;
>>           };
>> 
>> FWIW, if someone is looking for a way to test alarm support this can
>> be done in the following way:
>> 
>>     # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
>>     # shutdown -h now
>> 
>> With the commands above, after a minute, the system comes back to life.
> This paragraph belongs into the 2nd patch, right?

no, it is for this patch. I did a specific commit message for the second
patch. IIRC, it also has the example if it was the meaning of your
question.


>> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
>> index 6e1fcfb5d7e6..3ec73ad7f2d8 100644
>> --- a/drivers/rtc/rtc-isl12057.c
>> +++ b/drivers/rtc/rtc-isl12057.c
>> @@ -79,8 +79,10 @@
>>  #define ISL12057_MEM_MAP_LEN	0x10
>>  
>>  struct isl12057_rtc_data {
>> +	struct rtc_device *rtc;
>>  	struct regmap *regmap;
>>  	struct mutex lock;
>> +	int irq;
> interrupts are usually unsigned values. Hmm, I see that it simplifies a
> bit here, as the function to determine the irq returns the actual value
> or a negative error. As there would be problems anyhow for irq values >
> INT_MAX probably this comment isn't that important.

ok.

>> +/*
>> + * Note: as we only read from device and do not perform any update, there is
>> + * no need for an equivalent function which would try and get driver's main
>> + * lock. Here, it is safe for everyone if we just use regmap internal lock
>> + * on the device when reading.
>> + */
>> +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];
>>  	unsigned int sr;
>>  	int ret;
>>  
>> -	mutex_lock(&data->lock);
>>  	ret = regmap_read(data->regmap, ISL12057_REG_SR, &sr);
>>  	if (ret) {
>>  		dev_err(dev, "%s: unable to read oscillator status flag (%d)\n",
>> @@ -187,8 +222,6 @@ static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>  			__func__, ret);
>>  
>>  out:
>> -	mutex_unlock(&data->lock);
>> -
>>  	if (ret)
>>  		return ret;
>>  
>
> Is this locking update worth a separate change?

I do not think it needs a separate change (to simplify bisect for
instance) because the mutex protection can safely be removed here
around the two tests because the first read (to check oscillator bit)
and second read (to get time if oscillator bit is ok) we are doing on
the device do not need to be globally protected: oscillator bit is not
something we will update in such a way it would out of sync with the
current state of the time value kept by the device. More precisely, in
isl12057_rtc_set_time(), we do update the oscillator but only *after*
the time has been set to a valid value.

Cheers,

a+

Re: [PATCHv1 1/3] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[Uwe Kleine-König]

Hello Arnaud,

On 12/16/2014 01:07 PM, Arnaud Ebalard wrote:
>>> FWIW, if someone is looking for a way to test alarm support this can
>>> be done in the following way:
>>>
>>>     # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
>>>     # shutdown -h now
>>>
>>> With the commands above, after a minute, the system comes back to life.
>> This paragraph belongs into the 2nd patch, right?
> 
> no, it is for this patch. I did a specific commit message for the second
> patch. IIRC, it also has the example if it was the meaning of your
> question.
Is it usual that an rtc can wake up a machine from shutdown state? I
thought it isn't.

Best regards
Uwe

Re: [PATCHv1 1/3] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Tue, Dec 16, 2014 at 10:47:31PM +0100, Uwe Kleine-König wrote:
> On 12/16/2014 01:07 PM, Arnaud Ebalard wrote:

> > no, it is for this patch. I did a specific commit message for the second
> > patch. IIRC, it also has the example if it was the meaning of your
> > question.

> Is it usual that an rtc can wake up a machine from shutdown state? I
> thought it isn't.

It's fairly normal when the RTC is integrated into the PMIC - the PMIC
is typically the component initiating power on so it's easy for it to do
so.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCHv1 2/3] rtc: rtc-isl12057: add isil,irq2-can-wakeup-machine property for in-tree users

[Arnaud Ebalard]

Current in-tree users of ISL12057 RTC chip (NETGEAR ReadyNAS 102, 104
and 2120) do not have the IRQ#2 pin of the chip (associated w/ the
Alarm1 mechanism) connected to their SoC, but to a PMIC (TPS65251
FWIW). This specific hardware configuration allows the NAS to wake up
when the alarms rings.

Recently introduced alarm support for ISL12057 relies on the provision
of an "interrupts" property in system .dts file, which previous three
users will never get. For that reason, alarm support on those devices
is not function. To support this use case, this patch adds a new DT
property for ISL12057 (isil,irq2-can-wakeup-machine) to indicate that
the chip is capable of waking up the device using its IRQ#2 pin (even
though it does not have its IRQ#2 pin connected directly to the SoC).

This specific configuration was tested on a ReadyNAS 102 by setting
an alarm, powering off the device and see it reboot as expected
when the alarm rang w/:

  # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
  # shutdown -h now

As a side note, the ISL12057 remains in the list of trivial devices,
because the property is not per se required by the device to work
but can help handle system w/ specific requirements. In exchange, the
new feature is described in details in a specific documentation file.

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 .../devicetree/bindings/rtc/isil,isl12057.txt      | 78 ++++++++++++++++++++++
 drivers/rtc/rtc-isl12057.c                         | 45 +++++++++++--
 2 files changed, 117 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12057.txt

diff --git a/Documentation/devicetree/bindings/rtc/isil,isl12057.txt b/Documentation/devicetree/bindings/rtc/isil,isl12057.txt
new file mode 100644
index 000000000000..501c39ceae79
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/isil,isl12057.txt
@@ -0,0 +1,78 @@
+Intersil ISL12057 I2C RTC/Alarm chip
+
+ISL12057 is a trivial I2C device (it has simple device tree bindings,
+consisting of a compatible field, an address and possibly an interrupt
+line).
+
+Nonetheless, it also supports an option boolean property
+("isil,irq2-can-wakeup-machine") to handle the specific use-case found
+on at least three in-tree users of the chip (NETGEAR ReadyNAS 102, 104
+and 2120 ARM-based NAS); On those devices, the IRQ#2 pin of the chip
+(associated with the alarm supported by the driver) is not connected
+to the SoC but to a PMIC. It allows the device to be powered up when
+RTC alarm rings. In order to mark the device has a wakeup source and
+get access to the 'wakealarm' sysfs entry, this specific property can
+be set when the IRQ#2 pin of the chip is not connected to the SoC but
+can wake up the device.
+
+Required properties supported by the device:
+
+ - "compatible": must be "isil,isl12057"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+ - "isil,irq2-can-wakeup-machine": mark the chip as a wakeup source,
+   independently of the availability of an IRQ line connected to the
+   SoC.
+
+ - "interrupt-parent", "interrupts": for passing the interrupt line
+   of the SoC connected to IRQ#2 of the RTC chip.
+
+
+Example isl12057 node without IRQ#2 pin connected (no alarm support):
+
+	isl12057: isl12057@68 {
+		compatible = "isil,isl12057";
+		reg = <0x68>;
+	};
+
+
+Example isl12057 node with IRQ#2 pin connected to main SoC via MPP6 (note
+that the pinctrl-related properties below are given for completeness and
+may not be required or may be different depending on your system or
+SoC, and the main function of the MPP used as IRQ line, i.e.
+"interrupt-parent" and "interrupts" are usually sufficient):
+
+		    pinctrl {
+				...
+
+				rtc_alarm_pin: rtc_alarm_pin {
+					marvell,pins = "mpp6";
+					marvell,function = "gpio";
+				};
+
+				...
+
+		    };
+
+	...
+
+	isl12057: isl12057@68 {
+		compatible = "isil,isl12057";
+		reg = <0x68>;
+		pinctrl-0 = <&rtc_alarm_pin>;
+		pinctrl-names = "default";
+		interrupt-parent = <&gpio0>;
+		interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+	};
+
+
+Example isl12057 node without IRQ#2 pin connected to the SoC but to a
+PMIC, allowing the device to be started based on configured alarm:
+
+	isl12057: isl12057@68 {
+		compatible = "isil,isl12057";
+		reg = <0x68>;
+		isil,irq2-can-wakeup-machine;
+	};
diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
index 3ec73ad7f2d8..bd76c5e35fa5 100644
--- a/drivers/rtc/rtc-isl12057.c
+++ b/drivers/rtc/rtc-isl12057.c
@@ -457,6 +457,40 @@ static int isl12057_check_rtc_status(struct device *dev, struct regmap *regmap)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+/*
+ * 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. This
+ * is for instance the case on ReadyNAS 102, 104 and 2120. On those
+ * devices with no IRQ driectly connected to the SoC, the RTC chip
+ * can be forced as a wakeup source by stating that explicitly in
+ * the device's .dts file using the "isil,irq2-can-wakeup-machine"
+ * boolean property. This will guarantee 'wakealarm' sysfs entry is
+ * available on the device.
+ *
+ * The function below returns 1, i.e. the capability of the chip to
+ * wakeup the device, based on IRQ availability or if the boolean
+ * property has been set in the .dts file. Otherwise, it returns 0.
+ */
+
+static bool isl12057_can_wakeup_machine(struct device *dev)
+{
+	struct isl12057_rtc_data *data = dev_get_drvdata(dev);
+
+	return (data->irq || of_property_read_bool(dev->of_node,
+					      "isil,irq2-can-wakeup-machine"));
+}
+#else
+static bool isl12057_can_wakeup_machine(struct device *dev)
+{
+	struct isl12057_rtc_data *data = dev_get_drvdata(dev);
+
+	return !!data->irq;
+}
+#endif
+
 static int isl12057_rtc_alarm_irq_enable(struct device *dev,
 					 unsigned int enable)
 {
@@ -555,7 +589,8 @@ static int isl12057_probe(struct i2c_client *client,
 				client->irq, ret);
 	}
 
-	device_init_wakeup(dev, !!data->irq);
+	if (isl12057_can_wakeup_machine(dev))
+		device_init_wakeup(dev, true);
 
 	data->rtc = devm_rtc_device_register(dev, DRV_NAME, &rtc_ops,
 					     THIS_MODULE);
@@ -576,9 +611,7 @@ err:
 
 static int isl12057_remove(struct i2c_client *client)
 {
-	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev);
-
-	if (rtc_data->irq)
+	if (isl12057_can_wakeup_machine(&client->dev))
 		device_init_wakeup(&client->dev, false);
 
 	return 0;
@@ -589,7 +622,7 @@ static int isl12057_rtc_suspend(struct device *dev)
 {
 	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev);
 
-	if (device_may_wakeup(dev))
+	if (rtc_data->irq && device_may_wakeup(dev))
 		return enable_irq_wake(rtc_data->irq);
 
 	return 0;
@@ -599,7 +632,7 @@ static int isl12057_rtc_resume(struct device *dev)
 {
 	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev);
 
-	if (device_may_wakeup(dev))
+	if (rtc_data->irq && device_may_wakeup(dev))
 		return disable_irq_wake(rtc_data->irq);
 
 	return 0;
-- 
2.1.1

[PATCHv1 3/3] ARM: mvebu: ISL12057 rtc chip can now wake up RN102, RN102 and RN2120

[Arnaud Ebalard]

Now that alarm support for ISL12057 chip is available w/ the specific
"isil,irq2-can-wakeup-machine" property, let's use that feature of the
driver dedicated to NETGEAR ReadyNAS 102, 104 and 2120 specific
routing of RTC Alarm IRQ#2 pin; on those devices, this pin is not
connected to the SoC but to a PMIC, which allows the device to be
powered up when RTC alarm rings.

For that to work, the chip needs to be explicitly marked as a device
wakeup source using this "isil,irq2-can-wakeup-machine" boolean
property. This makes 'wakealarm' sysfs entry available to configure
the alarm.

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 arch/arm/boot/dts/armada-370-netgear-rn102.dts | 1 +
 arch/arm/boot/dts/armada-370-netgear-rn104.dts | 1 +
 arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-netgear-rn102.dts b/arch/arm/boot/dts/armada-370-netgear-rn102.dts
index 4e24932c6e30..1c83b7ce0982 100644
--- a/arch/arm/boot/dts/armada-370-netgear-rn102.dts
+++ b/arch/arm/boot/dts/armada-370-netgear-rn102.dts
@@ -87,6 +87,7 @@
 				isl12057: isl12057@68 {
 					compatible = "isil,isl12057";
 					reg = <0x68>;
+					isil,irq2-can-wakeup-machine;
 				};
 
 				g762: g762@3e {
diff --git a/arch/arm/boot/dts/armada-370-netgear-rn104.dts b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
index 30586e47986a..5fbfe02964dc 100644
--- a/arch/arm/boot/dts/armada-370-netgear-rn104.dts
+++ b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
@@ -93,6 +93,7 @@
 				isl12057: isl12057@68 {
 					compatible = "isil,isl12057";
 					reg = <0x68>;
+					isil,irq2-can-wakeup-machine;
 				};
 
 				g762: g762@3e {
diff --git a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
index d81430aa4ab3..fc8bdfcd2348 100644
--- a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
+++ b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
@@ -100,6 +100,7 @@
 				isl12057: isl12057@68 {
 					compatible = "isil,isl12057";
 					reg = <0x68>;
+					isil,irq2-can-wakeup-machine;
 				};
 
 				/* Controller for rear fan #1 of 3 (Protechnic
-- 
2.1.1