rtc: rtc-isl12057: fixes and alarm support

This series includes six 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.

- As suggested by Uwe, second patch adds proper support for century bit provided by the driver. This will allow to use the chip until 2199 if we manage to pass 2038.

- Following another comment by Uwe, third patch corrects the handling of oscillator failure bit.

- fourth 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").

- As suggested by Mark, fifth patch improves failure reports by providing error code in dev_err() code.

- Sixth 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. More time was also spent on testing.

I wonder if you could take a look at this series I sent providing fixes (patches #1-5) and alarm support (patch #6) for ISL12057 RTC Chip. As all people do for RTC patches - due to lack of feedback from maintainer
- I am looking for someone to review the patches (Mark, patch 6 has a sticker w/ regmap-inside ;-)) and then push those upstream if they are ok (Jason, you handled that part for 70e123373c05: "rtc: Add support for Intersil ISL12057 I2C RTC chip" in 2013 for the same reason).

Note that I addressed previous comments and feedback received from Uwe and also spent time to do some soldering in order to support and test the last patch for both my use case (Alarm IRQ signal not routed to the SoC on ReadyNAS RN102, RN104 and RN2120) and the common case.

Do not hesitate to ask if you want me to resend!

akpm already applied the series, doesn't he?

Quoted 8 lines · Arnaud Ebalard

I wonder if you could take a look at this series I sent providing fixes (patches #1-5) and alarm support (patch #6) for ISL12057 RTC Chip. As all people do for RTC patches - due to lack of feedback from maintainer
- I am looking for someone to review the patches (Mark, patch 6 has a sticker w/ regmap-inside ;-)) and then push those upstream if they are ok (Jason, you handled that part for 70e123373c05: "rtc: Add support for Intersil ISL12057 I2C RTC chip" in 2013 for the same reason).

Quoted 6 lines

Note that I addressed previous comments and feedback received from Uwe and also spent time to do some soldering in order to support and test the last patch for both my use case (Alarm IRQ signal not routed to the SoC on ReadyNAS RN102, RN104 and RN2120) and the common case.

Do not hesitate to ask if you want me to resend!

akpm already applied the series, doesn't he?

I'm about to test the patches, just fighting with my NAS to boot my custom kernel.

Apart from that patches 1 - 5 can have my Ack. Didn't look deep enough into patch 6 yet. @akpm: No need to hurry adding them, I hope to be able to also provide Tested-by:-tags later today.

Hello Arnaud,

Quoted 8 lines · Arnaud Ebalard

I wonder if you could take a look at this series I sent providing fixes (patches #1-5) and alarm support (patch #6) for ISL12057 RTC Chip. As all people do for RTC patches - due to lack of feedback from maintainer
- I am looking for someone to review the patches (Mark, patch 6 has a sticker w/ regmap-inside ;-)) and then push those upstream if they are ok (Jason, you handled that part for 70e123373c05: "rtc: Add support for Intersil ISL12057 I2C RTC chip" in 2013 for the same reason).

Quoted 6 lines

Note that I addressed previous comments and feedback received from Uwe and also spent time to do some soldering in order to support and test the last patch for both my use case (Alarm IRQ signal not routed to the SoC on ReadyNAS RN102, RN104 and RN2120) and the common case.

Do not hesitate to ask if you want me to resend!

akpm already applied the series, doesn't he?

I'm about to test the patches, just fighting with my NAS to boot my custom kernel.

Apart from that patches 1 - 5 can have my Ack. Didn't look deep enough into patch 6 yet. @akpm: No need to hurry adding them, I hope to be able to also provide Tested-by:-tags later today.

Hi,

Quoted 14 lines · Uwe Kleine-K__nig

Quoted 13 lines · Arnaud Ebalard

I wonder if you could take a look at this series I sent providing fixes (patches #1-5) and alarm support (patch #6) for ISL12057 RTC Chip. As all people do for RTC patches - due to lack of feedback from maintainer
- I am looking for someone to review the patches (Mark, patch 6 has a sticker w/ regmap-inside ;-)) and then push those upstream if they are ok (Jason, you handled that part for 70e123373c05: "rtc: Add support for Intersil ISL12057 I2C RTC chip" in 2013 for the same reason).

Note that I addressed previous comments and feedback received from Uwe and also spent time to do some soldering in order to support and test the last patch for both my use case (Alarm IRQ signal not routed to the SoC on ReadyNAS RN102, RN104 and RN2120) and the common case.

Quoted 2 lines

Quoted 1 line

Do not hesitate to ask if you want me to resend!

akpm already applied the series, doesn't he?

I am not really familiar w/ -mm tree workflow. It was not clear to me whether it was sitting in -mm tree waiting for reviewed-by and acked-by.

Quoted 5 lines

Quoted 1 line

akpm already applied the series, doesn't he?

I am not really familiar w/ -mm tree workflow. It was not clear to me whether it was sitting in -mm tree waiting for reviewed-by and acked-by.

-mm is a collection of several hundred trees (most of which are empty at a particular time). The workflow depends on which tree we're talking about!

For RTC patches I attempt to review them myself but I'm by no means an expert on RTC, so I'm careful to cc various people who I think might help with review and test. Often people do this. Generally when they don't, the patches are sufficiently obvious for me to proceed under my own steam. If something breaks (this is rare), the number of people who are affected is usually quite small and they tend to be system developers who can fix it up for us anyway.

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.

[...]
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

    # 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.

drivers/rtc/rtc-isl12057.c +16 -0

+	 */
+	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;
+

Hello,

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.

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).

For patch 4: Maybe
s/obsolete/for backwards compatibility, don't use in new code/.

Some further comments inline ...

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.

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).

For patch 4: Maybe
s/obsolete/for backwards compatibility, don't use in new code/.

Some further comments inline ...

Quoted 7 lines · Arnaud Ebalard

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:

Quoted 5 lines

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).

Quoted 6 lines

[...]
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?

Quoted 4 lines

    # 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/

Quoted 5 lines

drivers/rtc/rtc-isl12057.c +5 -0

+	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",

drivers/rtc/rtc-isl12057.c static int isl12057_i2c_validate_chip(struct regmap *regmap) · line 191

Quoted 4 lines

drivers/rtc/rtc-isl12057.c +4 -0

+			__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.)

drivers/rtc/rtc-isl12057.c static int isl12057_probe(struct i2c_client *client, · line 563

Quoted 7 lines

[...]

drivers/rtc/rtc-isl12057.c +6 -0

+	/*
+	 * 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.

Quoted 16 lines

drivers/rtc/rtc-isl12057.c +16 -0

+	 */
+	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;
+

drivers/rtc/rtc-isl12057.c static int isl12057_probe(struct i2c_client *client, · line 589

Quoted 10 lines

drivers/rtc/rtc-isl12057.c +8 -0

+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?

drivers/rtc/rtc-isl12057.c static int isl12057_probe(struct i2c_client *client, · line 600

Quoted 10 lines

drivers/rtc/rtc-isl12057.c +10 -0

+	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?

drivers/rtc/rtc-isl12057.c static int isl12057_probe(struct i2c_client *client, · line 610

Quoted 9 lines

drivers/rtc/rtc-isl12057.c +9 -0

+	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.

Quoted 1 line · Uwe Kleine-K?nig

For patch 1: Maybe point out that the issue with the century bit

isn't

Quoted 2 lines

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.

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 users).

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

...

Documentation/devicetree/bindings/i2c/trivial-devices.txt +1 -1

index fbde415078e6..edac97c0f756 100644
@@ -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

Documentation/devicetree/bindings/vendor-prefixes.txt +1 -1

index 723999d73744..84193ecdc41c 100644
@@ -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

drivers/rtc/rtc-isl12057.c +0 -0

index fe562820a54a..31eaa6a9c2b2 100644

Quoted 50 lines · On Sat, 15 Nov 2014 00:07:05 +0100 Arnaud Ebalard <[email protected]> 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 users).

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

...

Documentation/devicetree/bindings/i2c/trivial-devices.txt +1 -1

index fbde415078e6..edac97c0f756 100644
@@ -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

Documentation/devicetree/bindings/vendor-prefixes.txt +1 -1

index 723999d73744..84193ecdc41c 100644
@@ -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

drivers/rtc/rtc-isl12057.c +0 -0

index fe562820a54a..31eaa6a9c2b2 100644

drivers/rtc/rtc-isl12057.c static int isl12057_probe(struct i2c_client *client, · line 317

Quoted 9 lines

drivers/rtc/rtc-isl12057.c +2 -1

@@ -310,7 +310,8 @@ static int isl12057_probe(struct i2c_client *client,
 
 #ifdef CONFIG_OF
 static const struct of_device_id isl12057_dt_match[] = {
-	{ .compatible = "isl,isl12057" },
+	{ .compatible = "isl,isl12057" },  /* obsolete */
+	{ .compatible = "isil,isl12057" },
 	{ },
 };
 #endif

Your patch conflicts both textually and materially with the two below patches, which are now upstream.

I don't know what to do about this so I'll drop "rtc: rtc-isl12057: fix isil vs isl naming for intersil". Could you folks please sort this all out?

commit 7c75c1d5e72be6736aa558efd33ca2b31d77e425

Author:     Darshana Padmadas <[email protected]>
AuthorDate: Sat Nov 8 22:52:06 2014 +0530
Commit:     Arnd Bergmann <[email protected]>
CommitDate: Thu Nov 20 12:20:18 2014 +0100

dt-bindings: Document deprecated device vendor name to fix related warning

This patch documents deprecated vendor name "isil" to fix warning of undocumented string for device isl29028 as reported while running checkpatch.pl on drivers/staging/iio/light/isl29028.c. This is done to maintain compatibility with older kernels.

Signed-off-by Darshana Padmadas <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Acked-by Mark Rutland <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Documentation/devicetree/bindings/vendor-prefixes.txt +1 -0

index 723999d..bfbd93e 100644
@@ -77,6 +77,7 @@ innolux	Innolux Corporation
 intel	Intel Corporation
 intercontrol	Inter Control Group
 isee	ISEE 2007 S.L.
+isil    Intersil (deprecated, use isl)
 isl	Intersil
 karo	Ka-Ro electronics GmbH
 keymile	Keymile GmbH

commit b2ea3f82e7984d855c5e61fef18746771d90c73c

Author:     Darshana Padmadas <[email protected]>
AuthorDate: Sat Nov 8 22:52:05 2014 +0530
Commit:     Arnd Bergmann <[email protected]>
CommitDate: Thu Nov 20 12:19:59 2014 +0100

dt-bindings: Document correct and deprecated vendor-prefix with device isl29028

This patch documents the device isl29028 with its vendor-prefix. Undocumented deprecated vendor-prefix found by checkpatch also documented for compatibility reasons.

Signed-off-by Darshana Padmadas <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Acked-by Mark Rutland <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Documentation/devicetree/bindings/i2c/trivial-devices.txt +2 -0

index fbde415..605dcca 100644
@@ -56,6 +56,8 @@ gmt,g751		G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire In
 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,isl29028           (deprecated, use isl)
+isl,isl29028            Intersil ISL29028 Ambient Light and Proximity Sensor
 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