[PATCHv0 0/3] rtc: rtc-isl12057: fixes and alarm support

[PATCHv0 0/3] rtc: rtc-isl12057: fixes and alarm support

[Arnaud Ebalard]

Hi,

This series includes three patches for Intersil ISL12057 driver.

 - first patch is a fix for masking issues which dates back to driver
   inclusion. Even if those issues are not critical per se, the patch is
   a good candidate for stable down to 3.14.

 - second patch fixes remaining places where 'isl' is used instead of the
   expected NASDAQ symbol for Intersil (i.e. isil) after commit
   7a6540ca856ae ("ARM: mvebu: Change vendor prefix for Intersil 
   Corporation to isil"). 

 - third patch provides alarm support for Intersil ISL12057. This
   support was not added when the driver was initially pushed in 3.14
   kernel due to the inability to check interrupt support. After some
   soldering, this tests have been performed.

Comments are welcome,

Cheers,

a+

Arnaud Ebalard (3):
  rtc: rtc-isl12057: fix masking of register values
  rtc: rtc-isl12057: fix isil vs isl naming for intersil
  rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

 .../devicetree/bindings/i2c/trivial-devices.txt    |   2 +-
 .../devicetree/bindings/vendor-prefixes.txt        |   2 +-
 drivers/rtc/rtc-isl12057.c                         | 319 ++++++++++++++++++++-
 3 files changed, 309 insertions(+), 14 deletions(-)

-- 
2.1.1

[PATCHv0 1/3] rtc: rtc-isl12057: fix masking of register values

[Arnaud Ebalard]

When Intersil ISL12057 support was added by commit 70e123373c05 ("rtc:
Add support for Intersil ISL12057 I2C RTC chip"), two masks for time
registers values imported from the device were either wrong or
omitted, leading to additional bits from those registers to impact
read values:

 - mask for hour register value when reading it in AM/PM mode. As
   AM/PM mode is not the usual mode used by the driver, this error
   would only have an impact on an externally configured RTC hour
   later read by the driver.
 - mask for month value. The lack of masking would provide an
   erroneous value if century bit is set.

This patch fixes those two masks.

Fixes: 70e123373c05 ("rtc: Add support for Intersil ISL12057 I2C RTC chip")
Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 drivers/rtc/rtc-isl12057.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
index 455b601d731d..8132fbc7e10a 100644
--- a/drivers/rtc/rtc-isl12057.c
+++ b/drivers/rtc/rtc-isl12057.c
@@ -88,7 +88,7 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
 	tm->tm_min = bcd2bin(regs[ISL12057_REG_RTC_MN]);
 
 	if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_MIL) { /* AM/PM */
-		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x0f);
+		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x1f);
 		if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_PM)
 			tm->tm_hour += 12;
 	} else {					    /* 24 hour mode */
@@ -96,8 +96,8 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
 	}
 
 	tm->tm_mday = bcd2bin(regs[ISL12057_REG_RTC_DT]);
-	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts at 1 */
-	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO]) - 1; /* starts at 1 */
+	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1;  /* starts at 1 */
+	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO] & 0x1f) - 1; /* ditto */
 	tm->tm_year = bcd2bin(regs[ISL12057_REG_RTC_YR]) + 100;
 }
 
-- 
2.1.1

[PATCHv0 1/3] rtc: rtc-isl12057: fix masking of register values

[Uwe Kleine-König]

Hello Arnaud,

On Wed, Nov 05, 2014 at 10:42:25PM +0100, Arnaud Ebalard wrote:
> When Intersil ISL12057 support was added by commit 70e123373c05 ("rtc:
> Add support for Intersil ISL12057 I2C RTC chip"), two masks for time
> registers values imported from the device were either wrong or
> omitted, leading to additional bits from those registers to impact
> read values:
> 
>  - mask for hour register value when reading it in AM/PM mode. As
>    AM/PM mode is not the usual mode used by the driver, this error
>    would only have an impact on an externally configured RTC hour
>    later read by the driver.
>  - mask for month value. The lack of masking would provide an
>    erroneous value if century bit is set.
> 
> This patch fixes those two masks.
> 
> Fixes: 70e123373c05 ("rtc: Add support for Intersil ISL12057 I2C RTC chip")
> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> ---
>  drivers/rtc/rtc-isl12057.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
> index 455b601d731d..8132fbc7e10a 100644
> --- a/drivers/rtc/rtc-isl12057.c
> +++ b/drivers/rtc/rtc-isl12057.c
> @@ -88,7 +88,7 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
>  	tm->tm_min = bcd2bin(regs[ISL12057_REG_RTC_MN]);
>  
>  	if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_MIL) { /* AM/PM */
> -		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x0f);
> +		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x1f);
>  		if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_PM)
>  			tm->tm_hour += 12;
>  	} else {					    /* 24 hour mode */
> @@ -96,8 +96,8 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
>  	}
>  
>  	tm->tm_mday = bcd2bin(regs[ISL12057_REG_RTC_DT]);
> -	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts at 1 */
> +	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1;  /* starts at 1 */
unrelated change?

> -	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO]) - 1; /* starts at 1 */
> +	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO] & 0x1f) - 1; /* ditto */
>  	tm->tm_year = bcd2bin(regs[ISL12057_REG_RTC_YR]) + 100;
>  }
This patch definitly improves the driver, still I see room for more
improvement (all but the first issue are unrelated to this patch):

 - There is a century bit in the RTC_MO driver that is now correctly
   masked to determine the month. Shouldn't it be used to flag if year
   is >= 2100? That seems to match the hardware leap year handling.
   (I.e. it considers RTC_YR=0 not to be a leap year iff RTC_MO_CENTURY
   is set.)

 - I wonder how tm_wday should be handled. It seems to be hardly used by
   callers of rtc_read_time. Is it better to report the hardware state
   here or the weekday matching year/month/day?

 - IMHO the probe function shouldn't clear the OSCILLATOR FAILURE BIT
   (OSF). It's an indicator that the stored time is wrong. Instead this
   bit should be evaluated in the read_time callback. (If it's set,
   return -ENODATA.) And only clear the bit in .set_time when the
   registers were updated. I think most drivers get that wrong.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCHv0 1/3] rtc: rtc-isl12057: fix masking of register values

[Arnaud Ebalard]

Hi,

Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> writes:

> Hello Arnaud,
>
> On Wed, Nov 05, 2014 at 10:42:25PM +0100, Arnaud Ebalard wrote:
>> When Intersil ISL12057 support was added by commit 70e123373c05 ("rtc:
>> Add support for Intersil ISL12057 I2C RTC chip"), two masks for time
>> registers values imported from the device were either wrong or
>> omitted, leading to additional bits from those registers to impact
>> read values:
>> 
>>  - mask for hour register value when reading it in AM/PM mode. As
>>    AM/PM mode is not the usual mode used by the driver, this error
>>    would only have an impact on an externally configured RTC hour
>>    later read by the driver.
>>  - mask for month value. The lack of masking would provide an
>>    erroneous value if century bit is set.
>> 
>> This patch fixes those two masks.
>> 
>> Fixes: 70e123373c05 ("rtc: Add support for Intersil ISL12057 I2C RTC chip")
>> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
>> ---
>>  drivers/rtc/rtc-isl12057.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
>> index 455b601d731d..8132fbc7e10a 100644
>> --- a/drivers/rtc/rtc-isl12057.c
>> +++ b/drivers/rtc/rtc-isl12057.c
>> @@ -88,7 +88,7 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
>>  	tm->tm_min = bcd2bin(regs[ISL12057_REG_RTC_MN]);
>>  
>>  	if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_MIL) { /* AM/PM */
>> -		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x0f);
>> +		tm->tm_hour = bcd2bin(regs[ISL12057_REG_RTC_HR] & 0x1f);
>>  		if (regs[ISL12057_REG_RTC_HR] & ISL12057_REG_RTC_HR_PM)
>>  			tm->tm_hour += 12;
>>  	} else {					    /* 24 hour mode */
>> @@ -96,8 +96,8 @@ static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs)
>>  	}
>>  
>>  	tm->tm_mday = bcd2bin(regs[ISL12057_REG_RTC_DT]);
>> -	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1; /* starts at 1 */
>> +	tm->tm_wday = bcd2bin(regs[ISL12057_REG_RTC_DW]) - 1;  /* starts at 1 */
> unrelated change?

yep, will remove it.


>> -	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO]) - 1; /* starts at 1 */
>> +	tm->tm_mon  = bcd2bin(regs[ISL12057_REG_RTC_MO] & 0x1f) - 1; /* ditto */
>>  	tm->tm_year = bcd2bin(regs[ISL12057_REG_RTC_YR]) + 100;
>>  }
> This patch definitly improves the driver, still I see room for more
> improvement (all but the first issue are unrelated to this patch):
>
>  - There is a century bit in the RTC_MO driver that is now correctly
>    masked to determine the month. Shouldn't it be used to flag if year
>    is >= 2100? That seems to match the hardware leap year handling.
>    (I.e. it considers RTC_YR=0 not to be a leap year iff RTC_MO_CENTURY
>    is set.)
>
>  - I wonder how tm_wday should be handled. It seems to be hardly used by
>    callers of rtc_read_time. Is it better to report the hardware state
>    here or the weekday matching year/month/day?
>
>  - IMHO the probe function shouldn't clear the OSCILLATOR FAILURE BIT
>    (OSF). It's an indicator that the stored time is wrong. Instead this
>    bit should be evaluated in the read_time callback. (If it's set,
>    return -ENODATA.) And only clear the bit in .set_time when the
>    registers were updated. I think most drivers get that wrong.

Thanks for your feedback. I will take a look at those after alarm
support is ok.

Cheers,

a+

[PATCHv0 2/3] rtc: rtc-isl12057: fix isil vs isl naming for intersil

[Arnaud Ebalard]

When Intersil ISL12057 driver was introduced by commit 70e123373c05
(rtc: Add support for Intersil ISL12057 I2C RTC chip), the vendor
prefix 'isl' was used instead of the expected 'isil' (Intersil
NASDAQ symbol). Recently, a patch from Philip Zabel (7a6540ca856a,
ARM: mvebu: Change vendor prefix for Intersil Corporation to isil)
fixed that prefix in ReadyNAS devices .dts files (AFAICT, the only
kernel users at the moment).

That patch completes that work to change the remaining values in:

 - Intersil ISL12057 driver (compatible string) and also associated
   Documentation/devicetree/bindings/i2c/trivial-devices.txt file
   to reflect the changes introduced by previous commit from Philip

 - Documentation/devicetree/bindings/vendor-prefixes.txt files to
   avoid future mistakes

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 Documentation/devicetree/bindings/i2c/trivial-devices.txt | 2 +-
 Documentation/devicetree/bindings/vendor-prefixes.txt     | 2 +-
 drivers/rtc/rtc-isl12057.c                                | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index fbde415078e6..edac97c0f756 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)
-isl,isl12057		Intersil ISL12057 I2C RTC Chip
+isil,isl12057		Intersil ISL12057 I2C RTC 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/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 723999d73744..84193ecdc41c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -77,7 +77,7 @@ innolux	Innolux Corporation
 intel	Intel Corporation
 intercontrol	Inter Control Group
 isee	ISEE 2007 S.L.
-isl	Intersil
+isil	Intersil
 karo	Ka-Ro electronics GmbH
 keymile	Keymile GmbH
 lacie	LaCie
diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
index 8132fbc7e10a..adb0646236b1 100644
--- a/drivers/rtc/rtc-isl12057.c
+++ b/drivers/rtc/rtc-isl12057.c
@@ -279,7 +279,7 @@ static int isl12057_probe(struct i2c_client *client,
 
 #ifdef CONFIG_OF
 static const struct of_device_id isl12057_dt_match[] = {
-	{ .compatible = "isl,isl12057" },
+	{ .compatible = "isil,isl12057" },
 	{ },
 };
 #endif
-- 
2.1.1

[PATCHv0 2/3] rtc: rtc-isl12057: fix isil vs isl naming for intersil

[Mark Brown]

On Wed, Nov 05, 2014 at 10:42:41PM +0100, Arnaud Ebalard wrote:

> When Intersil ISL12057 driver was introduced by commit 70e123373c05
> (rtc: Add support for Intersil ISL12057 I2C RTC chip), the vendor
> prefix 'isl' was used instead of the expected 'isil' (Intersil
> NASDAQ symbol). Recently, a patch from Philip Zabel (7a6540ca856a,
> ARM: mvebu: Change vendor prefix for Intersil Corporation to isil)
> fixed that prefix in ReadyNAS devices .dts files (AFAICT, the only
> kernel users at the moment).

They may be the only in kernel users but someone with an out of tree DT
may be using the existing prefix, we shouldn't break compatibility with
them so we should support both compatible strings even if we want to
deprecate the isl, one.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141106/78ac7a76/attachment-0001.sig>

[PATCHv0 2/3] rtc: rtc-isl12057: fix isil vs isl naming for intersil

[Arnaud Ebalard]

Hi Mark,

Mark Brown <broonie@kernel.org> writes:

> On Wed, Nov 05, 2014 at 10:42:41PM +0100, Arnaud Ebalard wrote:
>
>> When Intersil ISL12057 driver was introduced by commit 70e123373c05
>> (rtc: Add support for Intersil ISL12057 I2C RTC chip), the vendor
>> prefix 'isl' was used instead of the expected 'isil' (Intersil
>> NASDAQ symbol). Recently, a patch from Philip Zabel (7a6540ca856a,
>> ARM: mvebu: Change vendor prefix for Intersil Corporation to isil)
>> fixed that prefix in ReadyNAS devices .dts files (AFAICT, the only
>> kernel users at the moment).
>
> They may be the only in kernel users but someone with an out of tree DT
> may be using the existing prefix, we shouldn't break compatibility with
> them so we should support both compatible strings even if we want to
> deprecate the isl, one.

Updating the patch in the following way should make it possible to
support out-of-tree users while avoiding additional uses of 'isl': 

- have two compatible entries in isl12057_dt_match struct instead of one
  i.e.:

    static const struct of_device_id isl12057_dt_match[] = {
   	{ .compatible = "isl,isl12057" },
   	{ .compatible = "isil,isl12057" },
    	{ },
    };

  I think it matches the situation we have.

- keeping the updates I had for trivial-devices.txt and
  vendor-prefixes.txt files.

Thoughts?

Cheers,

a+

[PATCHv0 2/3] rtc: rtc-isl12057: fix isil vs isl naming for intersil

[Uwe Kleine-König]

Hello Arnaud,

On Thu, Nov 06, 2014 at 11:46:21PM +0100, Arnaud Ebalard wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Wed, Nov 05, 2014 at 10:42:41PM +0100, Arnaud Ebalard wrote:
> >
> >> When Intersil ISL12057 driver was introduced by commit 70e123373c05
> >> (rtc: Add support for Intersil ISL12057 I2C RTC chip), the vendor
> >> prefix 'isl' was used instead of the expected 'isil' (Intersil
> >> NASDAQ symbol). Recently, a patch from Philip Zabel (7a6540ca856a,
> >> ARM: mvebu: Change vendor prefix for Intersil Corporation to isil)
> >> fixed that prefix in ReadyNAS devices .dts files (AFAICT, the only
> >> kernel users at the moment).
> >
> > They may be the only in kernel users but someone with an out of tree DT
> > may be using the existing prefix, we shouldn't break compatibility with
> > them so we should support both compatible strings even if we want to
> > deprecate the isl, one.
> 
> Updating the patch in the following way should make it possible to
> support out-of-tree users while avoiding additional uses of 'isl': 
> 
> - have two compatible entries in isl12057_dt_match struct instead of one
>   i.e.:
> 
>     static const struct of_device_id isl12057_dt_match[] = {
>    	{ .compatible = "isl,isl12057" },
>    	{ .compatible = "isil,isl12057" },
>     	{ },
>     };
> 
>   I think it matches the situation we have.
This should work. I suggest to add a comment to the obsolete one to
state not to use it for new boards.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Arnaud Ebalard]

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 at 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(-)

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");
-- 
2.1.1

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

[Mark Brown]

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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141106/4b2c7d8e/attachment.sig>

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

[Guenter Roeck]

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

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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141106/f4bbde01/attachment.sig>

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

[Arnaud Ebalard]

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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Arnaud Ebalard]

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+

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

[Mark Brown]

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/  |
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141107/ec400187/attachment.sig>

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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141107/f0d88edf/attachment.sig>

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Arnaud Ebalard]

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+