rtc: rtc-isl12057: fixes and alarm support

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 <[email protected]>

---
 drivers/rtc/rtc-isl12057.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 455b601d731d..8132fbc7e10a 100644
@@ -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 */

Hello Arnaud,

Quoted 37 lines · 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 <[email protected]>

---
 drivers/rtc/rtc-isl12057.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 455b601d731d..8132fbc7e10a 100644
@@ -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)
 	}
 

drivers/rtc/rtc-isl12057.c static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs) · line 99

Quoted 3 lines

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

 	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?

drivers/rtc/rtc-isl12057.c static void isl12057_rtc_regs_to_tm(struct rtc_time *tm, u8 *regs) · line 102

Quoted 4 lines

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

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

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

Quoted 7 lines · 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).

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.

Quoted 10 lines · Mark Brown

Quoted 7 lines · 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).

Quoted 4 lines

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.

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

+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+						isl12057_rtc_interrupt,
+						IRQF_SHARED|IRQF_ONESHOT,

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

Quoted 3 lines · Arnaud Ebalard

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

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

Quoted 2 lines · Mark Brown

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 ?

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

Quoted 3 lines · Arnaud Ebalard

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

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

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

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: writing INT failed\n", __func__);

It's generally helpful to log the error code as well.

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

Quoted 11 lines

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

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

drivers/rtc/rtc-isl12057.c static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 216

Quoted 8 lines

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

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

Quoted 7 lines

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

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

drivers/rtc/rtc-isl12057.c static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 311

Quoted 4 lines

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

+
+	/* 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)?

Quoted 10 lines

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

+	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);

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

Quoted 4 lines

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

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

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

Quoted 3 lines

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

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

Quoted 17 lines · Mark Brown

drivers/rtc/rtc-isl12057.c static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 311

Quoted 11 lines · Arnaud Ebalard

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

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

Quoted 10 lines

Quoted 10 lines

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

+	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);

Quoted 8 lines

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

Quoted 4 lines

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

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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

Quoted 4 lines · Arnaud Ebalard

Mark Brown <[email protected]> writes:

FWIW, we had a similar discussion a while ago, during driver inclusion:

Quoted 3 lines

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

Signature · 2 lines

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

Quoted 3 lines · Mark Brown

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.

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

Quoted 4 lines · Arnaud Ebalard

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

+#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)