Skip to contentSkip to conversation

rtc: abx80x: add support for abx81x

PATCH Superseded Antoni Pokusinski 2 participants 21 messages Aug 11 – Sep 3

Subsystems (2)

v5 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v5 cover rtc: abx80x: add support for abx81x

Hello,
This patch series adds support for the ABX81X RTCs, which are essentially SPI versions of the ABX80X RTCs. The first patches (P2-P4) contain bug fixes (added after sashiko's review in v1-v3), then (P5-P8) we have some cleanup and reorganization changes (converting the driver to use the regmap API was needed for this), followed by the actual ABX81X support.

The v5 is identical as v4, I send it like this because sashiko failed to apply the patch series for some reason:
https://sashiko.dev/#/patchset/20260810193826.210683-1-apokusinski01%40gmail.com
I have no idea why it happened, the patches apply cleanly on v7.2-rc1, sashiko's report is not helpful unfortunately.

Kind regards,
Antoni Pokusinski

---
Changes since v4:
None
Changes since v3:
* PATCH 2 ("rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()") New patch
* PATCH 3 ("rtc: abx80x: add mutex protection for register writes") Replaced "guard()" with "scoped_guard()" in "abx80x_ioctl()"
* PATCH 4 ("rtc: abx80x: properly handle shared IRQs")
Disable all the unused interrupts in "probe()"
* PATCH 6 ("rtc: abx80x: use regmap instead of I2C specific API") Changed the type of "flag" arguments in "regmap_read()" from "int" to "unsigned int"
---
Changes since v2:
* dropped patch "rtc: abx80x: move part autodetection before RV1805 configuration"
* PATCH 2 ("rtc: abx80x: add mutex protection for register writes")
New patch
* PATCH 5 ("rtc: abx80x: use regmap instead of I2C specific API")
- return PTR_ERR(regmap) on I2C regmap init error
* PATCH 8 ("rtc: abx80x: add support for ABX81X")
- return PTR_ERR(regmap) on SPI regmap init error
---
Changes since v1:
* PATCH 1 ("dt-bindings: rtc: abx80x: document ABX81X RTCs")
- added missing "spi-peripherals-props" reference
- added abx815 compatible strings to the trickle charger enum list
* PATCH 2 ("rtc: abx80x: move part autodetection before RV1805 configuration") New patch
* PATCH 3 ("rtc: abx80x: properly handle shared IRQs")
New patch
* PATCH 5 ("rtc: abx80x: use regmap instead of I2C specific API")
- changed regmap_read() argument types from int to unsigned int
---

Antoni Pokusinski (9):
dt-bindings: rtc: abx80x: document ABX81X RTCs
rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()
rtc: abx80x: add mutex protection for register writes
rtc: abx80x: properly handle shared IRQs
rtc: abx80x: add irq to struct abx80x_priv
rtc: abx80x: use regmap instead of I2C specific API
rtc: abx80x: replace read-modify-write pattern with regmap helpers
rtc: abx80x: create abx80x_i2c_probe()
rtc: abx80x: add support for ABX81X

 .../bindings/rtc/abracon,abx80x.yaml          |  18 +-
 drivers/rtc/Kconfig                           |  25 +-
 drivers/rtc/rtc-abx80x.c                      | 663 +++++++++++-------
 3 files changed, 451 insertions(+), 255 deletions(-)
Signature · 1 line
2.55.0
#1 lore raw

PATCH v5 1/9 dt-bindings: rtc: abx80x: document ABX81X RTCs

Abracon AB0815 and AB1815 are RTC modules with SPI interface.

---
 .../bindings/rtc/abracon,abx80x.yaml           | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/rtc/abracon,abx80x.yaml +17 -1
index 355b0598411a..d105570bcd5a 100644
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/rtc/abracon,abx80x.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Abracon ABX80X I2C ultra low power RTC/Alarm chip
+title: Abracon ABX80X (I2C) and ABX81X (SPI) ultra low power RTC/Alarm chips
 
 maintainers:
   - [email protected]
@@ -21,10 +21,12 @@ properties:
       - abracon,ab0803
       - abracon,ab0804
       - abracon,ab0805
+      - abracon,ab0815
       - abracon,ab1801
       - abracon,ab1803
       - abracon,ab1804
       - abracon,ab1805
+      - abracon,ab1815
       - microcrystal,rv1805
 
   reg:
@@ -61,6 +63,7 @@ required:
   - reg
 
 allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
   - $ref: rtc.yaml#
   - if:
       properties:
@@ -73,6 +76,8 @@ allOf:
                 - abracon,ab1804
                 - abracon,ab0805
                 - abracon,ab1805
+                - abracon,ab0815
+                - abracon,ab1815
     then:
       properties:
         abracon,tc-diode: false
@@ -96,3 +101,14 @@ examples:
             interrupts = <44 IRQ_TYPE_EDGE_FALLING>;
         };
     };
+
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@0 {
+            compatible = "abracon,ab1815";
+            reg = <0>;
+        };
+    };
Signature · 1 line
2.55.0
#2 lore raw

Re: [PATCH v5 1/9] dt-bindings: rtc: abx80x: document ABX81X RTCs

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines

Abracon AB0815 and AB1815 are RTC modules with SPI interface.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

PATCH v5 2/9 rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()

In reply to Antoni Pokusinski

i2c_smbus_read_i2c_block_data() returns the number of bytes read on success and a negative error code on failure. The existing code treated any non-zero return value as an error.

Fix the error handling by checking only for negative return values.

Fixes 718a820a303c ("rtc: abx80x: add alarm support")
---
 drivers/rtc/rtc-abx80x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rtc/rtc-abx80x.c +1 -1
index 5486d9d0b1e5..fdf9664ebb6f 100644
@@ -294,7 +294,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
 					    sizeof(buf), buf);
-	if (err)
+	if (err < 0)
 		return err;
 
 	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
Signature · 1 line
2.55.0
#4 lore raw

Re: [PATCH v5 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized Stack Memory Use on short I2C reads

Pre-existing issues:
- [Medium] Incomplete Bug Fix / Unintended Return Value breaking hardware alarm initialization --

--- Patch [2]: [PATCH v5 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm() --- commit 111542521cef97450110e2dc7483bce8d88f644f
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()

The patch relaxes the error check for i2c_smbus_read_i2c_block_data() in abx80x_read_alarm(). It now only returns on negative errors, allowing the function to proceed if the read is successful and returns the number of bytes read.

drivers/rtc/rtc-abx80x.c static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 297

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +1 -1
index 5486d9d0b1e5d..fdf9664ebb6f0 100644
@@ -294,7 +294,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
 					    sizeof(buf), buf);
-	if (err)
+	if (err < 0)

[Severity: Medium]
Since the error check now only stops on negative values, could a short read cause issues here?

If the adapter returns fewer bytes than sizeof(buf) (e.g. 1 to 6 bytes), err will be positive but less than 7. The function will proceed to parse the buffer, but since the stack allocated buf is uninitialized, wouldn't it end up reading uninitialized stack memory later when checking buf[6]?

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
    ...
    t->pending = (buf[6] & ABX8XX_STATUS_AF) && t->enabled;
    ...
}

drivers/rtc/rtc-abx80x.c static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 300

Quoted 3 lines
drivers/rtc/rtc-abx80x.c +0 -0
 		return err;
 
 	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);

[Severity: Medium]
This is a pre-existing issue, but does returning err at the end of this function cause problems with alarm initialization?

Since err holds the positive byte count on success, abx80x_read_alarm() returns 7 instead of 0:

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
    ...
    err = i2c_smbus_read_i2c_block_data(...);
    ...
    return err;
}

Looking at how the rtc core handles this, __rtc_read_alarm() treats any non-zero return as an error. Would this cause __devm_rtc_register_device() to skip calling rtc_initialize_alarm() at boot?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#5 lore raw

PATCH v5 3/9 rtc: abx80x: add mutex protection for register writes

In reply to Antoni Pokusinski

The ABX80X RTC driver performs multi-step register operations such as NVMEM transfers or register writes preceded by a configuration key write. Add a mutex to serialize all the register writes to protect these sequences against race conditions.

---
 drivers/rtc/rtc-abx80x.c | 45 +++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)
drivers/rtc/rtc-abx80x.c +37 -8
index fdf9664ebb6f..0c8d3416fd5a 100644
@@ -15,6 +15,7 @@
 #include <linux/i2c.h>
 #include <linux/kstrtox.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/rtc.h>
 #include <linux/watchdog.h>
@@ -127,6 +128,7 @@ struct abx80x_priv {
 	struct rtc_device *rtc;
 	struct i2c_client *client;
 	struct watchdog_device wdog;
+	struct mutex lock;
 };
 
 static int abx80x_write_config_key(struct i2c_client *client, u8 key)
@@ -219,6 +221,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	unsigned char buf[8];
 	int err, flags;
 
@@ -234,6 +237,8 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year - 100);
 	buf[ABX8XX_REG_WD] = tm->tm_wday;
 
+	guard(mutex)(&priv->lock);
+
 	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_HTH,
 					     sizeof(buf), buf);
 	if (err < 0) {
@@ -263,6 +268,8 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
 	struct rtc_device *rtc = priv->rtc;
 	int status;
 
+	guard(mutex)(&priv->lock);
+
 	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
 	if (status < 0)
 		return IRQ_NONE;
@@ -317,6 +324,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	u8 alarm[6];
 	int err;
 
@@ -330,6 +338,8 @@ static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	alarm[4] = bin2bcd(t->time.tm_mday);
 	alarm[5] = bin2bcd(t->time.tm_mon + 1);
 
+	guard(mutex)(&priv->lock);
+
 	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_AHTH,
 					     sizeof(alarm), alarm);
 	if (err < 0) {
@@ -352,6 +362,7 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 					  int autocalibration)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	int retval, flags = 0;
 
 	if ((autocalibration != 0) && (autocalibration != 1024) &&
@@ -360,6 +371,8 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 		return -EINVAL;
 	}
 
+	guard(mutex)(&priv->lock);
+
 	flags = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
 	if (flags < 0)
 		return flags;
@@ -443,6 +456,7 @@ static ssize_t oscillator_store(struct device *dev,
 				const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	int retval, flags, rc_mode = 0;
 
 	if (strncmp(buf, "rc", 2) == 0) {
@@ -454,6 +468,8 @@ static ssize_t oscillator_store(struct device *dev,
 		return -EINVAL;
 	}
 
+	guard(mutex)(&priv->lock);
+
 	flags =  i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
 	if (flags < 0)
 		return flags;
@@ -511,8 +527,11 @@ static const struct attribute_group rtc_calib_attr_group = {
 static int abx80x_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	int err;
 
+	guard(mutex)(&priv->lock);
+
 	if (enabled)
 		err = i2c_smbus_write_byte_data(client, ABX8XX_REG_IRQ,
 						(ABX8XX_IRQ_IM_1_4 |
@@ -526,6 +545,7 @@ static int abx80x_alarm_irq_enable(struct device *dev, unsigned int enabled)
 static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	int status, tmp;
 
 	switch (cmd) {
@@ -539,16 +559,18 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 		return put_user(tmp, (unsigned int __user *)arg);
 
 	case RTC_VL_CLR:
-		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
-		if (status < 0)
-			return status;
+		scoped_guard(mutex, &priv->lock) {
+			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
+			if (status < 0)
+				return status;
 
-		status &= ~ABX8XX_STATUS_BLF;
+			status &= ~ABX8XX_STATUS_BLF;
 
-		tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
-						status);
-		if (tmp < 0)
-			return tmp;
+			tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
+							status);
+			if (tmp < 0)
+				return tmp;
+		}
 
 		return 0;
 
@@ -617,6 +639,8 @@ static int __abx80x_wdog_set_timeout(struct watchdog_device *wdog,
 	struct abx80x_priv *priv = watchdog_get_drvdata(wdog);
 	u8 val = ABX8XX_WDT_WDS | timeout_bits(timeout);
 
+	guard(mutex)(&priv->lock);
+
 	/*
 	 * Writing any timeout to the WDT register resets the watchdog timer.
 	 * Writing 0 disables it.
@@ -701,6 +725,8 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
 		len = min(lower + bytes, (size_t)ABX8XX_SRAM_WIN_SIZE) - lower;
 		len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
 
+		guard(mutex)(&priv->lock);
+
 		ret = i2c_smbus_write_byte_data(priv->client, ABX8XX_REG_EXTRAM,
 						extram);
 		if (ret)
@@ -908,6 +934,9 @@ static int abx80x_probe(struct i2c_client *client)
 
 	priv->rtc->ops = &abx80x_rtc_ops;
 	priv->client = client;
+	err = devm_mutex_init(&client->dev, &priv->lock);
+	if (err)
+		return err;
 
 	i2c_set_clientdata(client, priv);
 
Signature · 1 line
2.55.0
#6 lore raw

Re: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The patch adds a mutex (priv->lock) to protect multi-step write sequences (like configuration key writes followed by register writes) but fails to acquire it during I2C read operations. Because interleaved I2C access resets the hardware configuration key on this RTC chip, concurrent unprotected reads can silently abort active write sequences in other threads. --

--- Patch [3]: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes --- commit 22bce01eb8bed117145738b82a585f1daa2c3fdc
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

This patch introduces a mutex to serialize multi-step register writes on the ABX80X RTC, such as NVMEM transfers and configuration key writes. It uses scoped guards to protect these sequences from race conditions.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index fdf9664ebb6f0..0c8d3416fd5a9 100644

[ ... ]

drivers/rtc/rtc-abx80x.c static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 243

Quoted 8 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -234,6 +237,8 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year - 100);
 	buf[ABX8XX_REG_WD] = tm->tm_wday;
 
+	guard(mutex)(&priv->lock);
+
 	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_HTH,
 					     sizeof(buf), buf);

[Severity: Medium]
This is a pre-existing issue, but do read operations like abx80x_rtc_read_time() and abx80x_read_alarm() also need to take priv->lock?

If a process reads the time or alarm concurrently with a configuration key write sequence (like a sysfs store to the oscillator or autocalibration attributes), could the read operation interleave and silently reset the hardware unlock state?

This would cause the subsequent configuration write to fail, since interleaved I2C access resets the hardware configuration key on this chip.

[ ... ]

drivers/rtc/rtc-abx80x.c static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) · line 565

Quoted 11 lines
drivers/rtc/rtc-abx80x.c +4 -3
@@ -539,16 +559,18 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 		return put_user(tmp, (unsigned int __user *)arg);
 
 	case RTC_VL_CLR:
-		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
-		if (status < 0)
-			return status;
+		scoped_guard(mutex, &priv->lock) {
+			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
+			if (status < 0)
+				return status;

[Severity: Medium]
This is a pre-existing issue, but does the RTC_VL_READ case just above this block in abx80x_ioctl() also need to acquire the mutex?

Since the read case performs an unprotected I2C read, it could bypass the lock and abort active locked write sequences in other threads.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
#7 lore raw

Re: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes

Quoted 40 lines · On Tue, Aug 11, 2026 at 07:56:55PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The patch adds a mutex (priv->lock) to protect multi-step write sequences (like configuration key writes followed by register writes) but fails to acquire it during I2C read operations. Because interleaved I2C access resets the hardware configuration key on this RTC chip, concurrent unprotected reads can silently abort active write sequences in other threads. --

--- Patch [3]: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes --- commit 22bce01eb8bed117145738b82a585f1daa2c3fdc
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

This patch introduces a mutex to serialize multi-step register writes on the ABX80X RTC, such as NVMEM transfers and configuration key writes. It uses scoped guards to protect these sequences from race conditions.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index fdf9664ebb6f0..0c8d3416fd5a9 100644

[ ... ]

drivers/rtc/rtc-abx80x.c static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 243

Quoted 8 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -234,6 +237,8 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year - 100);
 	buf[ABX8XX_REG_WD] = tm->tm_wday;
 
+	guard(mutex)(&priv->lock);
+
 	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_HTH,
 					     sizeof(buf), buf);

[Severity: Medium]
This is a pre-existing issue, but do read operations like abx80x_rtc_read_time() and abx80x_read_alarm() also need to take priv->lock?

If a process reads the time or alarm concurrently with a configuration key write sequence (like a sysfs store to the oscillator or autocalibration attributes), could the read operation interleave and silently reset the hardware unlock state?

Quoted 4 lines

This would cause the subsequent configuration write to fail, since interleaved I2C access resets the hardware configuration key on this chip.

[ ... ]

Not true, reading the registers does not cause a configuration key reset

Quoted 16 lines

drivers/rtc/rtc-abx80x.c static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) · line 565

Quoted 11 lines
drivers/rtc/rtc-abx80x.c +4 -3
@@ -539,16 +559,18 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 		return put_user(tmp, (unsigned int __user *)arg);
 
 	case RTC_VL_CLR:
-		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
-		if (status < 0)
-			return status;
+		scoped_guard(mutex, &priv->lock) {
+			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
+			if (status < 0)
+				return status;

[Severity: Medium]
This is a pre-existing issue, but does the RTC_VL_READ case just above this block in abx80x_ioctl() also need to acquire the mutex?

Quoted 3 lines

Since the read case performs an unprotected I2C read, it could bypass the lock and abort active locked write sequences in other threads.

As above, the read does cause configuration key reset.

Quoted 2 lines
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
#8 lore raw

PATCH v5 4/9 rtc: abx80x: properly handle shared IRQs

In reply to Antoni Pokusinski

The abx80x RTC driver registers its interrupt handler with IRQF_SHARED, but the handler always returned IRQ_HANDLED and cleared the status register even when no interrupt source from the RTC was active.

This could cause the driver to claim interrupts belonging to other devices sharing the same IRQ line and could also clear unrelated status bits in the RTC status register.

Only handle the interrupt when the alarm or watchdog flags are set, and clear only those interrupt status bits that were handled. Return IRQ_NONE when the interrupt was not generated by the RTC.

Fixes 718a820a303c ("rtc: abx80x: add alarm support")
---
 drivers/rtc/rtc-abx80x.c | 54 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 5 deletions(-)
drivers/rtc/rtc-abx80x.c +49 -5
index 0c8d3416fd5a..485d1ea08030 100644
@@ -51,12 +51,18 @@
 #define ABX8XX_CTRL2_RSVD	BIT(5)
 
 #define ABX8XX_REG_IRQ		0x12
+#define ABX8XX_IRQ_EX1E		BIT(0)
+#define ABX8XX_IRQ_EX2E		BIT(1)
 #define ABX8XX_IRQ_AIE		BIT(2)
+#define ABX8XX_IRQ_TIE		BIT(3)
+#define ABX8XX_IRQ_BLIE		BIT(4)
 #define ABX8XX_IRQ_IM_1_4	(0x3 << 5)
 
 #define ABX8XX_REG_CD_TIMER_CTL	0x18
 
 #define ABX8XX_REG_OSC		0x1c
+#define ABX8XX_OSC_ACIE		BIT(0)
+#define ABX8XX_OSC_OFIE		BIT(1)
 #define ABX8XX_OSC_FOS		BIT(3)
 #define ABX8XX_OSC_BOS		BIT(4)
 #define ABX8XX_OSC_ACAL_512	BIT(5)
@@ -266,27 +272,34 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
 	struct i2c_client *client = dev_id;
 	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	struct rtc_device *rtc = priv->rtc;
+	irqreturn_t handled = IRQ_NONE;
 	int status;
 
 	guard(mutex)(&priv->lock);
 
 	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
 	if (status < 0)
-		return IRQ_NONE;
+		return handled;
 
-	if (status & ABX8XX_STATUS_AF)
+	if (status & ABX8XX_STATUS_AF) {
 		rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF);
+		handled = IRQ_HANDLED;
+	}
 
 	/*
 	 * It is unclear if we'll get an interrupt before the external
 	 * reset kicks in.
 	 */
-	if (status & ABX8XX_STATUS_WDT)
+	if (status & ABX8XX_STATUS_WDT) {
 		dev_alert(&client->dev, "watchdog timeout interrupt.\n");
+		handled = IRQ_HANDLED;
+	}
 
-	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0);
+	if (handled == IRQ_HANDLED)
+		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
+					  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
 
-	return IRQ_HANDLED;
+	return handled;
 }
 
 static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
@@ -950,6 +963,37 @@ static int abx80x_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
+	/* Disable unused interrupts */
+	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
+	if (data < 0) {
+		dev_err(&client->dev, "Unable to read irq register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_IRQ,
+					data & ~(ABX8XX_IRQ_EX1E |
+						 ABX8XX_IRQ_EX2E |
+						 ABX8XX_IRQ_TIE |
+						 ABX8XX_IRQ_BLIE));
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write irq register\n");
+		return -EIO;
+	}
+
+	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
+	if (data < 0) {
+		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC,
+					data & ~(ABX8XX_OSC_ACIE |
+						 ABX8XX_OSC_OFIE));
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
+		return -EIO;
+	}
+
 	if (client->irq > 0) {
 		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
 		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
Signature · 1 line
2.55.0
#9 lore raw

Re: [PATCH v5 4/9] rtc: abx80x: properly handle shared IRQs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The write to the Oscillator Control Register (ABX8XX_REG_OSC) in `abx80x_probe` is silently…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The write to the Oscillator Control Register (ABX8XX_REG_OSC) in abx80x_probe is silently ignored by the hardware because the required configuration key (ABX8XX_CFG_KEY_OSC) is not written to the Configuration Key Register beforehand. If the RTC has previously enabled interrupts like Auto Calibration or Oscillator Fail (e.g., from the bootloader or prior system state), they will remain enabled. When these interrupts trigger, the newly modified abx80x_handle_irq returns IRQ_NONE without clearing the interrupt status bits, leading to an interrupt storm that will eventually cause the kernel to disable the shared IRQ line. --

--- Patch [4]: [PATCH v5 4/9] rtc: abx80x: properly handle shared IRQs --- commit 7460b93d807b8ac0b8ec90945bfa5691c0f9cd26
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: properly handle shared IRQs

This patch modifies the abx80x interrupt handler to support shared IRQs by only returning IRQ_HANDLED when alarm or watchdog events occur. It also updates the probe function to disable unused interrupts.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 0c8d3416fd5a9..485d1ea08030d 100644

[ ... ]

Quoted 5 lines
drivers/rtc/rtc-abx80x.c +1 -0
@@ -950,6 +963,37 @@ static int abx80x_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
+	/* Disable unused interrupts */

[ ... ]

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 991

Quoted 9 lines
drivers/rtc/rtc-abx80x.c +9 -0
+	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
+	if (data < 0) {
+		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
+		return -EIO;
+	}
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC,
+					data & ~(ABX8XX_OSC_ACIE |
+						 ABX8XX_OSC_OFIE));

[Severity: High]
Will this write to ABX8XX_REG_OSC be silently ignored by the hardware?

It appears that writing to the Oscillator Control Register requires an unlock key to be written to the Configuration Key Register first.

If this write fails and the RTC was previously left with ACIE or OFIE interrupts enabled (e.g., from the bootloader), could it lead to an interrupt storm?

When those interrupts fire, the updated abx80x_handle_irq() will now skip them and return IRQ_NONE without clearing the status bits:

	if (status & ABX8XX_STATUS_AF) {
		rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF);
		handled = IRQ_HANDLED;
	}
        ...
	if (handled == IRQ_HANDLED)
		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
					  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));

	return handled;

Since the status bits are not cleared for unhandled events, the IRQ controller would continually retrigger and eventually disable the shared IRQ line.

Quoted 8 lines
drivers/rtc/rtc-abx80x.c +5 -0
+	if (err < 0) {
+		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
+		return -EIO;
+	}
+
 	if (client->irq > 0) {
 		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
 		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
#10 lore raw

PATCH v5 5/9 rtc: abx80x: add irq to struct abx80x_priv

In reply to Antoni Pokusinski

This way the irq number is not tied to the i2c_client and can be easily accessed also in the case of ABX81X SPI clocks.

---
 drivers/rtc/rtc-abx80x.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/rtc/rtc-abx80x.c +10 -7
index 485d1ea08030..12a4dbfec639 100644
@@ -135,6 +135,7 @@ struct abx80x_priv {
 	struct i2c_client *client;
 	struct watchdog_device wdog;
 	struct mutex lock;
+	int irq;
 };
 
 static int abx80x_write_config_key(struct i2c_client *client, u8 key)
@@ -305,11 +306,12 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
 static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	unsigned char buf[7];
 
 	int irq_mask, err;
 
-	if (client->irq <= 0)
+	if (priv->irq <= 0)
 		return -EINVAL;
 
 	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
@@ -341,7 +343,7 @@ static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	u8 alarm[6];
 	int err;
 
-	if (client->irq <= 0)
+	if (priv->irq <= 0)
 		return -EINVAL;
 
 	alarm[0] = 0x0;
@@ -947,6 +949,7 @@ static int abx80x_probe(struct i2c_client *client)
 
 	priv->rtc->ops = &abx80x_rtc_ops;
 	priv->client = client;
+	priv->irq = client->irq;
 	err = devm_mutex_init(&client->dev, &priv->lock);
 	if (err)
 		return err;
@@ -994,19 +997,19 @@ static int abx80x_probe(struct i2c_client *client)
 		return -EIO;
 	}
 
-	if (client->irq > 0) {
-		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
-		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+	if (priv->irq > 0) {
+		dev_info(&client->dev, "IRQ %d supplied\n", priv->irq);
+		err = devm_request_threaded_irq(&client->dev, priv->irq, NULL,
 						abx80x_handle_irq,
 						IRQF_SHARED | IRQF_ONESHOT,
 						"abx8xx",
 						client);
 		if (err) {
 			dev_err(&client->dev, "unable to request IRQ, alarms disabled\n");
-			client->irq = 0;
+			priv->irq = 0;
 		}
 	}
-	if (client->irq <= 0)
+	if (priv->irq <= 0)
 		clear_bit(RTC_FEATURE_ALARM, priv->rtc->features);
 
 	err = rtc_add_group(priv->rtc, &rtc_calib_attr_group);
Signature · 1 line
2.55.0
#11 lore raw

Re: [PATCH v5 5/9] rtc: abx80x: add irq to struct abx80x_priv

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Failure to clear client->irq on IRQ request error leaves a dangling positive IRQ value that is…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Failure to clear client->irq on IRQ request error leaves a dangling positive IRQ value that is erroneously disabled during system shutdown. --

--- Patch [5]: [PATCH v5 5/9] rtc: abx80x: add irq to struct abx80x_priv --- commit e5e02251b0d732530fe4bd8df1edc078a44d0fe3
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add irq to struct abx80x_priv

This commit adds the irq number to struct abx80x_priv so it is not tied to the i2c_client. This allows the irq to be easily accessed in the case of ABX81X SPI clocks.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 485d1ea08030d..12a4dbfec6396 100644

[ ... ]

Quoted 8 lines
drivers/rtc/rtc-abx80x.c +1 -3
@@ -994,19 +997,19 @@ static int abx80x_probe(struct i2c_client *client)
 		return -EIO;
 	}
 
-	if (client->irq > 0) {
-		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
-		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+	if (priv->irq > 0) {

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 1009

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -1
+		dev_info(&client->dev, "IRQ %d supplied\n", priv->irq);
+		err = devm_request_threaded_irq(&client->dev, priv->irq, NULL,
 						abx80x_handle_irq,
 						IRQF_SHARED | IRQF_ONESHOT,
 						"abx8xx",
 						client);
 		if (err) {
 			dev_err(&client->dev, "unable to request IRQ, alarms disabled\n");
-			client->irq = 0;
+			priv->irq = 0;

[Severity: Medium]
By removing the client->irq = 0 assignment here, doesn't this leave a dangling positive IRQ value in the I2C client if the request fails?

If devm_request_threaded_irq() fails, the driver gracefully falls back to running with alarms disabled. However, during system shutdown, the I2C core in i2c_device_shutdown() relies on client->irq to disable interrupts:

drivers/i2c/i2c-core-base.c:i2c_device_shutdown() {
    ...
    if (client->irq > 0)
        disable_irq(client->irq);
    ...
}

Since this driver uses IRQF_SHARED and has no .shutdown callback to override the default behavior, leaving client->irq > 0 means the I2C core will unconditionally call disable_irq() on an IRQ line the driver failed to acquire.

Could this erroneously mask the shared IRQ line for other active devices during system shutdown?

Quoted 2 lines
drivers/rtc/rtc-abx80x.c +0 -0
 		}
 	}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
#12 lore raw

PATCH v5 6/9 rtc: abx80x: use regmap instead of I2C specific API

In reply to Antoni Pokusinski

Rework the driver so that it uses regmap_* functions for communication with the device instead of the i2c_smbus_* API. This way the support for ABX81X RTCs which use SPI can be added later.

---
 drivers/rtc/Kconfig      |   1 +
 drivers/rtc/rtc-abx80x.c | 389 +++++++++++++++++++++------------------
 2 files changed, 211 insertions(+), 179 deletions(-)
drivers/rtc/Kconfig +1 -0
index 01def8231873..4d14bcba616e 100644
@@ -215,6 +215,7 @@ config RTC_DRV_ABEOZ9
 config RTC_DRV_ABX80X
 	tristate "Abracon ABx80x"
 	select WATCHDOG_CORE if WATCHDOG
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for Abracon AB080X and AB180X
 	  families of ultra-low-power  battery- and capacitor-backed real-time
drivers/rtc/rtc-abx80x.c +210 -179

762 lines not on this page · raw message

Signature · 1 line
2.55.0
#13 lore raw

Re: [PATCH v5 6/9] rtc: abx80x: use regmap instead of I2C specific API

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Passing an `int *` to `regmap_read()` triggers a `-Wpointer-sign` compiler warning. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Passing an int * to regmap_read() triggers a -Wpointer-sign compiler warning. --

--- Patch [6]: [PATCH v5 6/9] rtc: abx80x: use regmap instead of I2C specific API --- commit 451b30314b8903deb9d330c3f8568f14d2e08147
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: use regmap instead of I2C specific API

This commit reworks the ABX80X RTC driver to use the regmap API for device communication instead of I2C-specific functions. This prepares the driver to support SPI-based ABX81X devices in the future.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 12a4dbfec6396..5d41882f3c704 100644

[ ... ]

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +1 -2
@@ -227,8 +230,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct abx80x_priv *priv = i2c_get_clientdata(client);
+	struct abx80x_priv *priv = dev_get_drvdata(dev);
 	unsigned char buf[8];
 	int err, flags;
 

[ ... ]

drivers/rtc/rtc-abx80x.c static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 259

Quoted 5 lines
drivers/rtc/rtc-abx80x.c +1 -3
 	/* Clear the OF bit of Oscillator Status Register */
-	flags = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSS);
-	if (flags < 0)
-		return flags;
+	err = regmap_read(priv->regmap, ABX8XX_REG_OSS, &flags);

[Severity: Low]
Does passing an int pointer to regmap_read() here cause a -Wpointer-sign compiler warning?

regmap_read() expects an unsigned int pointer for its val parameter, but flags is declared as an int at the top of abx80x_rtc_set_time(). This might break the build if CONFIG_WERROR is enabled.

Quoted 2 lines
drivers/rtc/rtc-abx80x.c +2 -0
+	if (err < 0)
+		return err;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
#14 lore raw

PATCH v5 7/9 rtc: abx80x: replace read-modify-write pattern with regmap helpers

In reply to Antoni Pokusinski

Before introducing the regmap usage in the driver, updating specific bits of a register was performed using the read-modify-write pattern. Now, the functions regmap_update_bits() and regmap_write_bits() can be used to simplify the code.

---
 drivers/rtc/rtc-abx80x.c | 135 ++++++++++-----------------------------
 1 file changed, 35 insertions(+), 100 deletions(-)
drivers/rtc/rtc-abx80x.c +35 -100
index 5d41882f3c70..ab88af43b4fa 100644
@@ -232,7 +232,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct abx80x_priv *priv = dev_get_drvdata(dev);
 	unsigned char buf[8];
-	int err, flags;
+	int err;
 
 	if (tm->tm_year < 100)
 		return -EINVAL;
@@ -256,18 +256,11 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	}
 
 	/* Clear the OF bit of Oscillator Status Register */
-	err = regmap_read(priv->regmap, ABX8XX_REG_OSS, &flags);
+	err = regmap_update_bits(priv->regmap, ABX8XX_REG_OSS, ABX8XX_OSS_OF, 0);
 	if (err < 0)
-		return err;
-
-	err = regmap_write(priv->regmap, ABX8XX_REG_OSS,
-			   flags & ~ABX8XX_OSS_OF);
-	if (err < 0) {
 		dev_err(dev, "Unable to write oscillator status register\n");
-		return err;
-	}
 
-	return 0;
+	return err;
 }
 
 static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
@@ -378,7 +371,6 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 {
 	struct abx80x_priv *priv = dev_get_drvdata(dev);
 	unsigned int flags = 0;
-	int retval;
 
 	if ((autocalibration != 0) && (autocalibration != 1024) &&
 	    (autocalibration != 512)) {
@@ -386,12 +378,6 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 		return -EINVAL;
 	}
 
-	guard(mutex)(&priv->lock);
-
-	retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags);
-	if (retval < 0)
-		return retval;
-
 	if (autocalibration == 0) {
 		flags &= ~(ABX8XX_OSC_ACAL_512 | ABX8XX_OSC_ACAL_1024);
 	} else if (autocalibration == 1024) {
@@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 		flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512);
 	}
 
+	guard(mutex)(&priv->lock);
+
 	/* Unlock write access to Oscillator Control Register */
 	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
-
-	return retval;
+	return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512,
+				 flags);
 }
 
 static int abx80x_rtc_get_autocalibration(struct device *dev)
@@ -473,7 +461,6 @@ static ssize_t oscillator_store(struct device *dev,
 {
 	struct abx80x_priv *priv = dev_get_drvdata(dev->parent);
 	int retval, rc_mode = 0;
-	unsigned int flags;
 
 	if (strncmp(buf, "rc", 2) == 0) {
 		rc_mode = 1;
@@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev,
 
 	guard(mutex)(&priv->lock);
 
-	retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags);
-	if (retval < 0)
-		return retval;
-
-	if (rc_mode == 0)
-		flags &= ~(ABX8XX_OSC_OSEL);
-	else
-		flags |= (ABX8XX_OSC_OSEL);
-
 	/* Unlock write access on Oscillator Control register */
 	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
-	if (retval < 0) {
+	retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL,
+				   rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL));
+	if (retval < 0)
 		dev_err(dev, "Failed to write Oscillator Control register\n");
-		return retval;
-	}
 
 	return retval ? retval : count;
 }
@@ -573,18 +550,11 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 
 	case RTC_VL_CLR:
 		scoped_guard(mutex, &priv->lock) {
-			err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status);
-			if (err < 0)
-				return err;
-
-			status &= ~ABX8XX_STATUS_BLF;
-
-			err = regmap_write(priv->regmap, ABX8XX_REG_STATUS, status);
-			if (err < 0)
-				return err;
+			err = regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS,
+						 ABX8XX_STATUS_BLF, 0);
 		}
 
-		return 0;
+		return err;
 
 	default:
 		return -ENOIOCTLCMD;
@@ -841,7 +811,6 @@ static int abx80x_probe(struct i2c_client *client)
 	struct device_node *np = client->dev.of_node;
 	struct abx80x_priv *priv;
 	int i, err, trickle_cfg = -EINVAL;
-	unsigned int data;
 	char buf[7];
 	unsigned int part = (uintptr_t)i2c_get_match_data(client);
 	unsigned int partnumber;
@@ -891,14 +860,9 @@ static int abx80x_probe(struct i2c_client *client)
 	dev_info(&client->dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n",
 		 partnumber, majrev, minrev, lot, wafer, uid);
 
-	err = regmap_read(regmap, ABX8XX_REG_CTRL1, &data);
-	if (err < 0) {
-		dev_err(&client->dev, "Unable to read control register\n");
-		return -EIO;
-	}
-
-	err = regmap_write(regmap, ABX8XX_REG_CTRL1,
-			   (data & ~(ABX8XX_CTRL_12_24 | ABX8XX_CTRL_ARST)) | ABX8XX_CTRL_WRITE);
+	err = regmap_update_bits(regmap, ABX8XX_REG_CTRL1,
+				 ABX8XX_CTRL_12_24 | ABX8XX_CTRL_ARST | ABX8XX_CTRL_WRITE,
+				 ABX8XX_CTRL_WRITE);
 	if (err < 0) {
 		dev_err(&client->dev, "Unable to write control register\n");
 		return -EIO;
@@ -912,30 +876,10 @@ static int abx80x_probe(struct i2c_client *client)
 		 * register is set. RV-1805-C3 datasheet indicates that
 		 * the bit should be cleared in section 11h - Control2.
 		 */
-		err = regmap_read(regmap, ABX8XX_REG_CTRL2, &data);
-		if (err < 0) {
-			dev_err(&client->dev,
-				"Unable to read control2 register\n");
-			return -EIO;
-		}
-
-		err = regmap_write(regmap, ABX8XX_REG_CTRL2,
-				   data & ~ABX8XX_CTRL2_RSVD);
-		if (err < 0) {
-			dev_err(&client->dev,
-				"Unable to write control2 register\n");
-			return -EIO;
-		}
-
-		/*
-		 * Avoid extra power leakage. The RV1805 uses smaller
-		 * 10pin package and the EXTI input is not present.
-		 * Disable it to avoid leakage.
-		 */
-		err = regmap_read(regmap, ABX8XX_REG_OUT_CTRL, &data);
+		err = regmap_update_bits(regmap, ABX8XX_REG_CTRL2,
+					 ABX8XX_CTRL2_RSVD, 0);
 		if (err < 0) {
-			dev_err(&client->dev,
-				"Unable to read output control register\n");
+			dev_err(&client->dev, "Unable to write control2 register\n");
 			return -EIO;
 		}
 
@@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client)
 		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
 			return -EIO;
 
-		err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL,
-				   data | ABX8XX_OUT_CTRL_EXDS);
+		/*
+		 * Avoid extra power leakage. The RV1805 uses smaller
+		 * 10pin package and the EXTI input is not present.
+		 * Disable it to avoid leakage.
+		 */
+		err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL,
+					ABX8XX_OUT_CTRL_EXDS,
+					ABX8XX_OUT_CTRL_EXDS);
 		if (err < 0) {
 			dev_err(&client->dev,
 				"Unable to write output control register\n");
@@ -998,33 +948,18 @@ static int abx80x_probe(struct i2c_client *client)
 		return err;
 
 	/* Disable unused interrupts */
-	err = regmap_read(regmap, ABX8XX_REG_IRQ, &data);
-	if (err < 0) {
-		dev_err(&client->dev, "Unable to read irq register\n");
-		return -EIO;
-	}
-
-	err = regmap_write(regmap, ABX8XX_REG_IRQ,
-			   data & ~(ABX8XX_IRQ_EX1E |
-				    ABX8XX_IRQ_EX2E |
-				    ABX8XX_IRQ_TIE |
-				    ABX8XX_IRQ_BLIE));
-	if (err < 0) {
-		dev_err(&client->dev, "Unable to write irq register\n");
-		return -EIO;
-	}
-
-	err = regmap_read(regmap, ABX8XX_REG_OSC, &data);
+	err = regmap_update_bits(regmap, ABX8XX_REG_IRQ,
+				 ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E |
+				 ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update irq register\n");
 		return -EIO;
 	}
 
-	err = regmap_write(regmap, ABX8XX_REG_OSC,
-			   data & ~(ABX8XX_OSC_ACIE |
-				    ABX8XX_OSC_OFIE));
+	err = regmap_update_bits(regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update Oscillator Control register\n");
 		return -EIO;
 	}
 
Signature · 1 line
2.55.0
#15 lore raw

Re: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch replaces read-modify-write sequences with regmap_write_bits() after writing the hardware configuration key, breaking the write-protection unlock sequence.

Pre-existing issues:
- [High] The probe function attempts to clear interrupt enable bits (ABX8XX_OSC_ACIE and ABX8XX_OSC_OFIE) in the protected Oscillator Control Register (ABX8XX_REG_OSC) without first writing the required configuration key (ABX8XX_CFG_KEY_OSC). --

--- Patch [7]: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers --- commit 6808e36654fbc0a72e9b540a05e0f2557530aa65
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: replace read-modify-write pattern with regmap helpers

This commit replaces read-modify-write sequences with regmap_update_bits() and regmap_write_bits() to simplify register updates.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 5d41882f3c704..ab88af43b4fa0 100644

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 		flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512);
 	}
 
+	guard(mutex)(&priv->lock);
+
 	/* Unlock write access to Oscillator Control Register */

drivers/rtc/rtc-abx80x.c static int abx80x_rtc_set_autocalibration(struct device *dev, · line 401

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -3
 	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
-
-	return retval;
+	return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512,
+				 flags);
 }

[Severity: High]
Does this change break the configuration key unlock sequence?

The configuration key unlock only allows a single I2C write operation. Because regmap_write_bits() internally performs a read-modify-write sequence, it initiates an I2C read transaction before the actual write. Will this inserted read consume the unlock key and cause the subsequent data write to be silently rejected by the hardware?

The original code explicitly read the register before unlocking it to avoid this sequence.

[ ... ]

Quoted 18 lines
drivers/rtc/rtc-abx80x.c +0 -10
@@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev,
 
 	guard(mutex)(&priv->lock);
 
-	retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags);
-	if (retval < 0)
-		return retval;
-
-	if (rc_mode == 0)
-		flags &= ~(ABX8XX_OSC_OSEL);
-	else
-		flags |= (ABX8XX_OSC_OSEL);
-
 	/* Unlock write access on Oscillator Control register */
 	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);

drivers/rtc/rtc-abx80x.c static ssize_t oscillator_store(struct device *dev, · line 486

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -3
-	if (retval < 0) {
+	retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL,
+				   rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL));
+	if (retval < 0)
 		dev_err(dev, "Failed to write Oscillator Control register\n");
-		return retval;
-	}
 
 	return retval ? retval : count;
 }

[Severity: High]
Will this regmap_write_bits() call also fail for the same reason?

Since it follows abx80x_write_config_key(), the implicit read operation will likely consume the unlock key before the write occurs.

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +1 -2
@@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client)
 		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
 			return -EIO;
 
-		err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL,
-				   data | ABX8XX_OUT_CTRL_EXDS);
+		/*

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 903

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +7 -0
+		 * Avoid extra power leakage. The RV1805 uses smaller
+		 * 10pin package and the EXTI input is not present.
+		 * Disable it to avoid leakage.
+		 */
+		err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL,
+					ABX8XX_OUT_CTRL_EXDS,
+					ABX8XX_OUT_CTRL_EXDS);
 		if (err < 0) {
 			dev_err(&client->dev,
 				"Unable to write output control register\n");

[Severity: High]
Similar to the above, does the implicit read inside regmap_write_bits() consume the configuration key unlock for the MISC register before the actual write happens?

[ ... ]

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +4 -2
-	err = regmap_read(regmap, ABX8XX_REG_OSC, &data);
+	err = regmap_update_bits(regmap, ABX8XX_REG_IRQ,
+				 ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E |
+				 ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update irq register\n");
 		return -EIO;
 	}
 

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 964

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -4
-	err = regmap_write(regmap, ABX8XX_REG_OSC,
-			   data & ~(ABX8XX_OSC_ACIE |
-				    ABX8XX_OSC_OFIE));
+	err = regmap_update_bits(regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update Oscillator Control register\n");
 		return -EIO;
 	}

[Severity: High]
This is a pre-existing issue, but does this register update require unlocking first?

It appears the probe function attempts to clear the interrupt enable bits in the Oscillator Control Register without first writing the required configuration key (ABX8XX_CFG_KEY_OSC).

If the configuration key is not written first, will the hardware silently ignore this write, leaving interrupts enabled if they were previously set by the bootloader?

If an unhandled interrupt (like Autocalibration or Oscillator Failure) fires, could it cause an interrupt storm? The IRQ handler abx80x_handle_irq() only checks for AF and WDT flags. Since the IRQ is requested with IRQF_SHARED, an unhandled interrupt might result in the kernel disabling the shared IRQ line and breaking unrelated devices.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
#16 lore raw

Re: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers

Quoted 52 lines · On Tue, Aug 11, 2026 at 07:56:53PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch replaces read-modify-write sequences with regmap_write_bits() after writing the hardware configuration key, breaking the write-protection unlock sequence.

Pre-existing issues:
- [High] The probe function attempts to clear interrupt enable bits (ABX8XX_OSC_ACIE and ABX8XX_OSC_OFIE) in the protected Oscillator Control Register (ABX8XX_REG_OSC) without first writing the required configuration key (ABX8XX_CFG_KEY_OSC). --

--- Patch [7]: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers --- commit 6808e36654fbc0a72e9b540a05e0f2557530aa65
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: replace read-modify-write pattern with regmap helpers

This commit replaces read-modify-write sequences with regmap_update_bits() and regmap_write_bits() to simplify register updates.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 5d41882f3c704..ab88af43b4fa0 100644

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
 		flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512);
 	}
 
+	guard(mutex)(&priv->lock);
+
 	/* Unlock write access to Oscillator Control Register */

drivers/rtc/rtc-abx80x.c static int abx80x_rtc_set_autocalibration(struct device *dev, · line 401

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -3
 	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
-
-	return retval;
+	return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512,
+				 flags);
 }

[Severity: High]
Does this change break the configuration key unlock sequence?

The configuration key unlock only allows a single I2C write operation. Because regmap_write_bits() internally performs a read-modify-write sequence, it initiates an I2C read transaction before the actual write. Will this inserted read consume the unlock key and cause the subsequent data write to be silently rejected by the hardware?

Quoted 5 lines

The original code explicitly read the register before unlocking it to avoid this sequence.

[ ... ]

No it won't because reads do not cause configuration key reset.

Quoted 32 lines
Quoted 18 lines
drivers/rtc/rtc-abx80x.c +0 -10
@@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev,
 
 	guard(mutex)(&priv->lock);
 
-	retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags);
-	if (retval < 0)
-		return retval;
-
-	if (rc_mode == 0)
-		flags &= ~(ABX8XX_OSC_OSEL);
-	else
-		flags |= (ABX8XX_OSC_OSEL);
-
 	/* Unlock write access on Oscillator Control register */
 	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);

drivers/rtc/rtc-abx80x.c static ssize_t oscillator_store(struct device *dev, · line 486

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -3
-	if (retval < 0) {
+	retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL,
+				   rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL));
+	if (retval < 0)
 		dev_err(dev, "Failed to write Oscillator Control register\n");
-		return retval;
-	}
 
 	return retval ? retval : count;
 }

[Severity: High]
Will this regmap_write_bits() call also fail for the same reason?

Quoted 5 lines

Since it follows abx80x_write_config_key(), the implicit read operation will likely consume the unlock key before the write occurs.

[ ... ]

No, same reason as above

Quoted 23 lines
Quoted 7 lines
drivers/rtc/rtc-abx80x.c +1 -2
@@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client)
 		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
 			return -EIO;
 
-		err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL,
-				   data | ABX8XX_OUT_CTRL_EXDS);
+		/*

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 903

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +7 -0
+		 * Avoid extra power leakage. The RV1805 uses smaller
+		 * 10pin package and the EXTI input is not present.
+		 * Disable it to avoid leakage.
+		 */
+		err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL,
+					ABX8XX_OUT_CTRL_EXDS,
+					ABX8XX_OUT_CTRL_EXDS);
 		if (err < 0) {
 			dev_err(&client->dev,
 				"Unable to write output control register\n");

[Severity: High]
Similar to the above, does the implicit read inside regmap_write_bits() consume the configuration key unlock for the MISC register before the actual write happens?

Quoted 2 lines

[ ... ]

No, same reason as above

Quoted 25 lines
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +4 -2
-	err = regmap_read(regmap, ABX8XX_REG_OSC, &data);
+	err = regmap_update_bits(regmap, ABX8XX_REG_IRQ,
+				 ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E |
+				 ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update irq register\n");
 		return -EIO;
 	}
 

drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 964

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -4
-	err = regmap_write(regmap, ABX8XX_REG_OSC,
-			   data & ~(ABX8XX_OSC_ACIE |
-				    ABX8XX_OSC_OFIE));
+	err = regmap_update_bits(regmap, ABX8XX_REG_OSC,
+				 ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
+		dev_err(&client->dev, "Unable to update Oscillator Control register\n");
 		return -EIO;
 	}

[Severity: High]
This is a pre-existing issue, but does this register update require unlocking first?

Quoted 8 lines

It appears the probe function attempts to clear the interrupt enable bits in the Oscillator Control Register without first writing the required configuration key (ABX8XX_CFG_KEY_OSC).

If the configuration key is not written first, will the hardware silently ignore this write, leaving interrupts enabled if they were previously set by the bootloader?

Yes, will be fixed in v6

Quoted 8 lines

If an unhandled interrupt (like Autocalibration or Oscillator Failure) fires, could it cause an interrupt storm? The IRQ handler abx80x_handle_irq() only checks for AF and WDT flags. Since the IRQ is requested with IRQF_SHARED, an unhandled interrupt might result in the kernel disabling the shared IRQ line and breaking unrelated devices.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
#17 lore raw

PATCH v5 8/9 rtc: abx80x: create abx80x_i2c_probe()

In reply to Antoni Pokusinski

Move the I2C-specific code from abx80x_probe() to the newly created function. This is a preparation for introducing the support for ABX81X RTCs which use SPI interface.

---
 drivers/rtc/rtc-abx80x.c | 171 ++++++++++++++++++++++++---------------
 1 file changed, 105 insertions(+), 66 deletions(-)
drivers/rtc/rtc-abx80x.c +105 -66
index ab88af43b4fa..08b22c067699 100644
@@ -781,73 +781,38 @@ static const struct regmap_access_table abx80x_write_table = {
 	.n_no_ranges = ARRAY_SIZE(abx80x_no_write_ranges),
 };
 
-static const struct regmap_config abx80x_regmap_config_i2c = {
-	.reg_bits = 8,
-	.val_bits = 8,
-	.max_register = ABX8XX_SRAM_BASE + ABX8XX_SRAM_WIN_SIZE - 1,
-
-	.rd_table = &abx80x_read_table,
-	.wr_table = &abx80x_write_table,
-};
-
-static const struct i2c_device_id abx80x_id[] = {
-	{ .name = "abx80x", .driver_data = ABX80X },
-	{ .name = "ab0801", .driver_data = AB0801 },
-	{ .name = "ab0803", .driver_data = AB0803 },
-	{ .name = "ab0804", .driver_data = AB0804 },
-	{ .name = "ab0805", .driver_data = AB0805 },
-	{ .name = "ab1801", .driver_data = AB1801 },
-	{ .name = "ab1803", .driver_data = AB1803 },
-	{ .name = "ab1804", .driver_data = AB1804 },
-	{ .name = "ab1805", .driver_data = AB1805 },
-	{ .name = "rv1805", .driver_data = RV1805 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, abx80x_id);
-
-static int abx80x_probe(struct i2c_client *client)
+static int abx80x_probe(struct device *dev, struct regmap *regmap, int irq,
+			struct device_node *np, unsigned int part)
 {
-	struct regmap *regmap;
-	struct device_node *np = client->dev.of_node;
 	struct abx80x_priv *priv;
 	int i, err, trickle_cfg = -EINVAL;
 	char buf[7];
-	unsigned int part = (uintptr_t)i2c_get_match_data(client);
 	unsigned int partnumber;
 	unsigned int majrev, minrev;
 	unsigned int lot;
 	unsigned int wafer;
 	unsigned int uid;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
-		return -ENODEV;
-
-	regmap = devm_regmap_init_i2c(client, &abx80x_regmap_config_i2c);
-	if (IS_ERR(regmap)) {
-		dev_err(&client->dev, "Unable to allocate regmap\n");
-		return PTR_ERR(regmap);
-	}
-
-	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
 		return -ENOMEM;
 
-	priv->rtc = devm_rtc_allocate_device(&client->dev);
+	priv->rtc = devm_rtc_allocate_device(dev);
 	if (IS_ERR(priv->rtc))
 		return PTR_ERR(priv->rtc);
 
 	priv->rtc->ops = &abx80x_rtc_ops;
-	priv->irq = client->irq;
+	priv->irq = irq;
 	priv->regmap = regmap;
-	err = devm_mutex_init(&client->dev, &priv->lock);
+	err = devm_mutex_init(dev, &priv->lock);
 	if (err)
 		return err;
 
-	dev_set_drvdata(&client->dev, priv);
+	dev_set_drvdata(dev, priv);
 
 	err = regmap_bulk_read(regmap, ABX8XX_REG_ID0, buf, sizeof(buf));
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to read partnumber\n");
+		dev_err(dev, "Unable to read partnumber\n");
 		return -EIO;
 	}
 
@@ -857,14 +822,14 @@ static int abx80x_probe(struct i2c_client *client)
 	lot = ((buf[4] & 0x80) << 2) | ((buf[6] & 0x80) << 1) | buf[3];
 	uid = ((buf[4] & 0x7f) << 8) | buf[5];
 	wafer = (buf[6] & 0x7c) >> 2;
-	dev_info(&client->dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n",
+	dev_info(dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n",
 		 partnumber, majrev, minrev, lot, wafer, uid);
 
 	err = regmap_update_bits(regmap, ABX8XX_REG_CTRL1,
 				 ABX8XX_CTRL_12_24 | ABX8XX_CTRL_ARST | ABX8XX_CTRL_WRITE,
 				 ABX8XX_CTRL_WRITE);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to write control register\n");
+		dev_err(dev, "Unable to write control register\n");
 		return -EIO;
 	}
 
@@ -879,7 +844,7 @@ static int abx80x_probe(struct i2c_client *client)
 		err = regmap_update_bits(regmap, ABX8XX_REG_CTRL2,
 					 ABX8XX_CTRL2_RSVD, 0);
 		if (err < 0) {
-			dev_err(&client->dev, "Unable to write control2 register\n");
+			dev_err(dev, "Unable to write control2 register\n");
 			return -EIO;
 		}
 
@@ -887,7 +852,7 @@ static int abx80x_probe(struct i2c_client *client)
 		 * Write the configuration key register to enable access to
 		 * the config2 register
 		 */
-		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
+		if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_MISC) < 0)
 			return -EIO;
 
 		/*
@@ -899,7 +864,7 @@ static int abx80x_probe(struct i2c_client *client)
 					ABX8XX_OUT_CTRL_EXDS,
 					ABX8XX_OUT_CTRL_EXDS);
 		if (err < 0) {
-			dev_err(&client->dev,
+			dev_err(dev,
 				"Unable to write output control register\n");
 			return -EIO;
 		}
@@ -911,26 +876,24 @@ static int abx80x_probe(struct i2c_client *client)
 			if (partnumber == abx80x_caps[i].pn)
 				break;
 		if (abx80x_caps[i].pn == 0) {
-			dev_err(&client->dev, "Unknown part: %04x\n",
-				partnumber);
+			dev_err(dev, "Unknown part: %04x\n", partnumber);
 			return -EINVAL;
 		}
 		part = i;
 	}
 
 	if (partnumber != abx80x_caps[part].pn) {
-		dev_err(&client->dev, "partnumber mismatch %04x != %04x\n",
+		dev_err(dev, "partnumber mismatch %04x != %04x\n",
 			partnumber, abx80x_caps[part].pn);
 		return -EINVAL;
 	}
 
 	if (np && abx80x_caps[part].has_tc)
-		trickle_cfg = abx80x_dt_trickle_cfg(&client->dev);
+		trickle_cfg = abx80x_dt_trickle_cfg(dev);
 
 	if (trickle_cfg > 0) {
-		dev_info(&client->dev, "Enabling trickle charger: %02x\n",
-			 trickle_cfg);
-		abx80x_enable_trickle_charger(&client->dev, trickle_cfg);
+		dev_info(dev, "Enabling trickle charger: %02x\n", trickle_cfg);
+		abx80x_enable_trickle_charger(dev, trickle_cfg);
 	}
 
 	err = regmap_write(regmap, ABX8XX_REG_CD_TIMER_CTL, BIT(2));
@@ -938,7 +901,7 @@ static int abx80x_probe(struct i2c_client *client)
 		return err;
 
 	if (abx80x_caps[part].has_wdog) {
-		err = abx80x_setup_watchdog(&client->dev);
+		err = abx80x_setup_watchdog(dev);
 		if (err)
 			return err;
 	}
@@ -952,26 +915,26 @@ static int abx80x_probe(struct i2c_client *client)
 				 ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E |
 				 ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to update irq register\n");
+		dev_err(dev, "Unable to update irq register\n");
 		return -EIO;
 	}
 
 	err = regmap_update_bits(regmap, ABX8XX_REG_OSC,
 				 ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0);
 	if (err < 0) {
-		dev_err(&client->dev, "Unable to update Oscillator Control register\n");
+		dev_err(dev, "Unable to update Oscillator Control register\n");
 		return -EIO;
 	}
 
 	if (priv->irq > 0) {
-		dev_info(&client->dev, "IRQ %d supplied\n", priv->irq);
-		err = devm_request_threaded_irq(&client->dev, priv->irq, NULL,
+		dev_info(dev, "IRQ %d supplied\n", priv->irq);
+		err = devm_request_threaded_irq(dev, priv->irq, NULL,
 						abx80x_handle_irq,
 						IRQF_SHARED | IRQF_ONESHOT,
 						"abx8xx",
-						&client->dev);
+						dev);
 		if (err) {
-			dev_err(&client->dev, "unable to request IRQ, alarms disabled\n");
+			dev_err(dev, "unable to request IRQ, alarms disabled\n");
 			priv->irq = 0;
 		}
 	}
@@ -980,14 +943,39 @@ static int abx80x_probe(struct i2c_client *client)
 
 	err = rtc_add_group(priv->rtc, &rtc_calib_attr_group);
 	if (err) {
-		dev_err(&client->dev, "Failed to create sysfs group: %d\n",
-			err);
+		dev_err(dev, "Failed to create sysfs group: %d\n", err);
 		return err;
 	}
 
 	return devm_rtc_register_device(priv->rtc);
 }
 
+#if IS_ENABLED(CONFIG_I2C)
+
+static const struct regmap_config abx80x_regmap_config_i2c = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = ABX8XX_SRAM_BASE + ABX8XX_SRAM_WIN_SIZE - 1,
+
+	.rd_table = &abx80x_read_table,
+	.wr_table = &abx80x_write_table,
+};
+
+static const struct i2c_device_id abx80x_id[] = {
+	{ .name = "abx80x", .driver_data = ABX80X },
+	{ .name = "ab0801", .driver_data = AB0801 },
+	{ .name = "ab0803", .driver_data = AB0803 },
+	{ .name = "ab0804", .driver_data = AB0804 },
+	{ .name = "ab0805", .driver_data = AB0805 },
+	{ .name = "ab1801", .driver_data = AB1801 },
+	{ .name = "ab1803", .driver_data = AB1803 },
+	{ .name = "ab1804", .driver_data = AB1804 },
+	{ .name = "ab1805", .driver_data = AB1805 },
+	{ .name = "rv1805", .driver_data = RV1805 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, abx80x_id);
+
 #ifdef CONFIG_OF
 static const struct of_device_id abx80x_of_match[] = {
 	{
@@ -1035,16 +1023,67 @@ static const struct of_device_id abx80x_of_match[] = {
 MODULE_DEVICE_TABLE(of, abx80x_of_match);
 #endif
 
+static int abx80x_i2c_probe(struct i2c_client *client)
+{
+	unsigned int part = (uintptr_t)i2c_get_match_data(client);
+	struct regmap *regmap;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	regmap = devm_regmap_init_i2c(client, &abx80x_regmap_config_i2c);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "Unable to allocate regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	return abx80x_probe(&client->dev, regmap, client->irq,
+			    client->dev.of_node, part);
+}
+
 static struct i2c_driver abx80x_driver = {
 	.driver		= {
 		.name	= "rtc-abx80x",
 		.of_match_table = of_match_ptr(abx80x_of_match),
 	},
-	.probe		= abx80x_probe,
+	.probe		= abx80x_i2c_probe,
 	.id_table	= abx80x_id,
 };
 
-module_i2c_driver(abx80x_driver);
+static int abx80x_register_driver(void)
+{
+	return i2c_add_driver(&abx80x_driver);
+}
+
+static void abx80x_unregister_driver(void)
+{
+	i2c_del_driver(&abx80x_driver);
+}
+
+#else
+
+static int abx80x_register_driver(void)
+{
+	return 0;
+}
+
+static void abx80x_unregister_driver(void)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_I2C) */
+
+static int __init abx80x_init(void)
+{
+	return abx80x_register_driver();
+}
+module_init(abx80x_init);
+
+static void __exit abx80x_exit(void)
+{
+	abx80x_unregister_driver();
+}
+module_exit(abx80x_exit);
 
 MODULE_AUTHOR("Philippe De Muyter <[email protected]>");
 MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
Signature · 1 line
2.55.0
#18 lore raw

Re: [PATCH v5 8/9] rtc: abx80x: create abx80x_i2c_probe()

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines

Move the I2C-specific code from abx80x_probe() to the newly created function. This is a preparation for introducing the support for ABX81X RTCs which use SPI interface.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
#19 lore raw

PATCH v5 9/9 rtc: abx80x: add support for ABX81X

In reply to Antoni Pokusinski

Abracon ABX81X is a family of RTCs with SPI interface featuring the same functionalities as the ABX80X I2C clocks.

---
 drivers/rtc/Kconfig      |  26 +++++-----
 drivers/rtc/rtc-abx80x.c | 106 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 115 insertions(+), 17 deletions(-)
drivers/rtc/Kconfig +14 -12
index 4d14bcba616e..d35221f81024 100644
@@ -212,18 +212,6 @@ config RTC_DRV_ABEOZ9
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ab-e0z9.
 
-config RTC_DRV_ABX80X
-	tristate "Abracon ABx80x"
-	select WATCHDOG_CORE if WATCHDOG
-	select REGMAP_I2C
-	help
-	  If you say yes here you get support for Abracon AB080X and AB180X
-	  families of ultra-low-power  battery- and capacitor-backed real-time
-	  clock chips.
-
-	  This driver can also be built as a module. If so, the module
-	  will be called rtc-abx80x.
-
 config RTC_DRV_AC100
 	tristate "X-Powers AC100"
 	depends on MFD_AC100
@@ -955,6 +943,20 @@ config RTC_I2C_AND_SPI
 
 comment "SPI and I2C RTC drivers"
 
+config RTC_DRV_ABX80X
+	tristate "Abracon ABx80x"
+	depends on RTC_I2C_AND_SPI
+	select WATCHDOG_CORE if WATCHDOG
+	select REGMAP_I2C if I2C
+	select REGMAP_SPI if SPI_MASTER
+	help
+	  If you say yes here you get support for Abracon AB080X, AB180X,
+	  AB081X and AB181X families of ultra-low-power  battery- and
+	  capacitor-backed real-time clock chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-abx80x.
+
 config RTC_DRV_DS3232
 	tristate "Dallas/Maxim DS3232/DS3234"
 	depends on RTC_I2C_AND_SPI
drivers/rtc/rtc-abx80x.c +101 -5

146 lines not on this page · raw message

Signature · 1 line
2.55.0
#20 lore raw

Re: [PATCH v5 9/9] rtc: abx80x: add support for ABX81X

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Read-modify-write (RMW) operations on the W0C (write-zero-to-clear) status register cause lost interrupts. --

--- Patch [9]: [PATCH v5 9/9] rtc: abx80x: add support for ABX81X --- commit de21e840f75a86e42ad746c99077b6acf5f3e1fb
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add support for ABX81X

Abracon ABX81X is a family of RTCs with SPI interface featuring the same functionalities as the ABX80X I2C clocks.

drivers/rtc/rtc-abx80x.c line 4

Quoted 11 lines
drivers/rtc/rtc-abx80x.c +2 -2
index 08b22c0676995..7145de876b50b 100644
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * A driver for the I2C members of the Abracon AB x8xx RTC family,
- * and compatible: AB 1805 and AB 0805
+ * A driver for the I2C and SPI members of the Abracon AB x8xx RTC family,
+ * and compatible: AB 1805, AB 0805, AB 1815 and AB 0815

[ ... ]

While reviewing this patch, I noticed a pre-existing issue in the driver that might cause lost interrupts.

drivers/rtc/rtc-abx80x.c:abx80x_handle_irq() {
[ ... ]

	if (handled == IRQ_HANDLED)
		regmap_write(priv->regmap, ABX8XX_REG_STATUS,
			     status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));

[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but does this write inadvertently clear newly-asserted interrupts?

The ABX8XX_REG_STATUS register uses a 'Write 0 to Clear' (W0C) mechanism, where writing 0 clears a bit and writing 1 preserves it. Because the code bitwise-ANDs the previously read status, it writes 0 to any bit that was 0 during the read.

If a hardware event asserts a flag between the read and this write, the write of 0 will clear the new event before it is handled.

[ ... ]

drivers/rtc/rtc-abx80x.c:abx80x_ioctl() {
[ ... ]

	case RTC_VL_CLR:
		scoped_guard(mutex, &priv->lock) {
			err = regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS,
						 ABX8XX_STATUS_BLF, 0);
		}

[ ... ]
}

[Severity: High]
This is also a pre-existing issue, but does this read-modify-write operation expose all other W0C flags to the exact same race condition?

Using regmap_update_bits to clear the BLF bit performs a read-modify-write and writes 0 to all other unasserted bits, which could cause them to be lost if they were asserted right after the read phase.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9