Skip to contentSkip to conversation

rtc: abx80x: add support for abx81x

PATCH Superseded Antoni Pokusinski 2 participants 21 messages Sep 7 – Sep 9

Subsystems (2)

v6 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v6 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-v5), 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.

Kind regards,
Antoni Pokusinski

---
Changes since v5:
* PATCH 2 ("rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()") Handle the case of 0<=err<7
* PATCH 4 ("rtc: abx80x: properly handle shared IRQs")
- probe: added missing configuration key unlock before writing REG_OSC
- handle_irq: fixed the value being written to REG_STATUS
* PATCH 6 ("rtc: abx80x: use regmap instead of I2C specific API") Changed the type of "flag" argument of "regmap_read()" from "int" to "unsigned int" in "set_time()"
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                      | 664 +++++++++++-------
 3 files changed, 454 insertions(+), 253 deletions(-)
Signature · 1 line
2.55.0
#1 lore raw

PATCH v6 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 v6 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 v6 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/rtc/rtc-abx80x.c +4 -2
index 5486d9d0b1e5..e4fd7b5d4b11 100644
@@ -294,8 +294,10 @@ 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;
+	if (err < ARRAY_SIZE(buf))
+		return -EIO;
 
 	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
 	if (irq_mask < 0)
@@ -311,7 +313,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	t->enabled = !!(irq_mask & ABX8XX_IRQ_AIE);
 	t->pending = (buf[6] & ABX8XX_STATUS_AF) && t->enabled;
 
-	return err;
+	return 0;
 }
 
 static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t)
Signature · 1 line
2.55.0
#4 lore raw

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

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

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

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=2
#5 lore raw

PATCH v6 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 e4fd7b5d4b11..44de234efecf 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;
@@ -319,6 +326,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;
 
@@ -332,6 +340,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) {
@@ -354,6 +364,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) &&
@@ -362,6 +373,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;
@@ -445,6 +458,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) {
@@ -456,6 +470,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;
@@ -513,8 +529,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 |
@@ -528,6 +547,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) {
@@ -541,16 +561,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;
 
@@ -619,6 +641,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.
@@ -703,6 +727,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)
@@ -910,6 +936,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 v6 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:
- [High] Incomplete mutex protection allows concurrent I2C reads to break configuration key sequences. --

commit c33d071aef97a2d9a731cf3e557a1cf0fbf7c50c
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

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.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index e4fd7b5d4b110..44de234efecf7 100644

[ ... ]

Quoted 9 lines
drivers/rtc/rtc-abx80x.c +1 -0
@@ -445,6 +458,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) {

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

Quoted 5 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -456,6 +470,8 @@ static ssize_t oscillator_store(struct device *dev,
 		return -EINVAL;
 	}
 
+	guard(mutex)(&priv->lock);
+

[Severity: High]
This isn't a bug introduced by this patch, but since this commit aims to protect configuration key sequences from race conditions, does this leave a similar race open for concurrent I2C reads?

If Thread A executes a protected write sequence (like oscillator_store), and Thread B executes an RTC read operation, can Thread B's I2C read interleave and reset the configuration key?

For example, abx80x_rtc_read_time() does not take priv->lock while reading the ABX8XX_REG_HTH registers.

Similarly, abx80x_is_rc_mode() (called from sysfs and read_time) performs an unlocked read of the ABX8XX_REG_OSS register.

The ABX8XX chip specification dictates that any intervening I2C read resets the configuration key. This would leave the register locked, causing Thread A's subsequent protected register write to silently fail without returning an error to userspace.

Should priv->lock be taken during these read paths as well to fully prevent I2C interleaving from aborting configuration key writes?

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

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

Quoted 41 lines · On Mon, Sep 07, 2026 at 08:46:33PM +0000, [email protected] wrote:

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

Pre-existing issues:
- [High] Incomplete mutex protection allows concurrent I2C reads to break configuration key sequences. --

commit c33d071aef97a2d9a731cf3e557a1cf0fbf7c50c
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

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.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index e4fd7b5d4b110..44de234efecf7 100644

[ ... ]

Quoted 9 lines
drivers/rtc/rtc-abx80x.c +1 -0
@@ -445,6 +458,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) {

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

Quoted 5 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -456,6 +470,8 @@ static ssize_t oscillator_store(struct device *dev,
 		return -EINVAL;
 	}
 
+	guard(mutex)(&priv->lock);
+

[Severity: High]
This isn't a bug introduced by this patch, but since this commit aims to protect configuration key sequences from race conditions, does this leave a similar race open for concurrent I2C reads?

Quoted 4 lines

If Thread A executes a protected write sequence (like oscillator_store), and Thread B executes an RTC read operation, can Thread B's I2C read interleave and reset the configuration key?

No, becasue reads do not cause configuration key reset

Quoted 16 lines

For example, abx80x_rtc_read_time() does not take priv->lock while reading the ABX8XX_REG_HTH registers.

Similarly, abx80x_is_rc_mode() (called from sysfs and read_time) performs an unlocked read of the ABX8XX_REG_OSS register.

The ABX8XX chip specification dictates that any intervening I2C read resets the configuration key. This would leave the register locked, causing Thread A's subsequent protected register write to silently fail without returning an error to userspace.

Should priv->lock be taken during these read paths as well to fully prevent I2C interleaving from aborting configuration key writes?

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

PATCH v6 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 | 52 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)
drivers/rtc/rtc-abx80x.c +50 -2
index 44de234efecf..8f9211bfb7e2 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,6 +272,7 @@ 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;
+	unsigned int status_flags_clear;
 	int status;
 
 	guard(mutex)(&priv->lock);
@@ -284,9 +291,15 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
 	if (status & ABX8XX_STATUS_WDT)
 		dev_alert(&client->dev, "watchdog timeout interrupt.\n");
 
-	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0);
+	status_flags_clear = status & (ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT);
 
-	return IRQ_HANDLED;
+	if (status_flags_clear) {
+		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
+					  ~status_flags_clear);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
 }
 
 static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
@@ -952,6 +965,41 @@ 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;
+	}
+
+	/* Unlock write access to Oscillator Control Register */
+	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
+		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 v6 4/9] rtc: abx80x: properly handle shared IRQs

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] Multi-step configuration key unlock and register write in abx80x_probe is unprotected by a mutex, leading to a race condition with concurrently registered userspace interfaces.

Pre-existing issues:
- [High] Read-modify-write of the W0C status register in abx80x_ioctl (RTC_VL_CLR) can silently clear and drop concurrently set interrupt flags like the Alarm Flag. --

commit c2d2310ab46eb6d5c2902acd74a3eb39a0bda185
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: properly handle shared IRQs

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.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 44de234efecf7..8f9211bfb7e24 100644

[ ... ]

Quoted 24 lines
drivers/rtc/rtc-abx80x.c +20 -0
@@ -952,6 +965,41 @@ 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");

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

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +10 -0
+		return -EIO;
+	}
+
+	/* Unlock write access to Oscillator Control Register */
+	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
+		return -EIO;
+
+	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC,
+					data & ~(ABX8XX_OSC_ACIE |
+						 ABX8XX_OSC_OFIE));

[Severity: High]
Since this unlocked two-step I2C transaction in abx80x_probe() occurs after the watchdog is exposed to userspace, is it vulnerable to concurrent aborts?

If a concurrent userspace process opens and pings the watchdog, it acquires priv->lock and issues an I2C write. This would interleave and abort the configuration key sequence, causing the subsequent write to ABX8XX_REG_OSC to be silently ignored.

This would leave unwanted hardware interrupts (ACIE, OFIE) enabled. If these fire, the newly updated IRQ handler returns IRQ_NONE without clearing them, causing an interrupt storm that forces the kernel to permanently disable the RTC IRQ line.

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

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,

This isn't a bug introduced by this patch, but looking at the RTC_VL_CLR handling in abx80x_ioctl():

drivers/rtc/rtc-abx80x.c:abx80x_ioctl() {
    ...
	case RTC_VL_CLR:
		scoped_guard(mutex, &priv->lock) {
			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
			if (status < 0)
				return status;

			status &= ~ABX8XX_STATUS_BLF;

			tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
							status);
    ...
}

[Severity: High]
Can this read-modify-write pattern silently clear and drop concurrently set interrupt flags like the Alarm Flag?

Because ABX8XX_REG_STATUS is a Write-0-to-Clear (W0C) register, any bit that was 0 during the read is written back as 0. If the hardware asserts a new flag in the window between the read and the write, the write of 0 will accidentally clear it before the IRQ handler can process it.

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

PATCH v6 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 8f9211bfb7e2..529f63e024f8 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,
@@ -343,7 +345,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;
@@ -949,6 +951,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;
@@ -1000,19 +1003,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 v6 5/9] rtc: abx80x: add irq to struct abx80x_priv

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:

Pre-existing issues:
- [High] Unprotected I2C reads in abx80x_rtc_read_time, abx80x_read_alarm, and sysfs show functions can interleave with and break multi-transaction I2C sequences protected by priv->lock.
- [High] abx80x_ioctl(RTC_VL_CLR) is vulnerable to a TOCTOU race with the hardware that can unintentionally clear unrelated interrupts like ABX8XX_STATUS_AF. --

commit b245a6cf28d590ef87d1389ee95de3f0f3c0348b
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add irq to struct abx80x_priv

This patch 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 also in the case of ABX81X SPI clocks.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 8f9211bfb7e24..529f63e024f8d 100644

[ ... ]

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
@@ -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);

drivers/rtc/rtc-abx80x.c static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) · line 317

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2 -1
+	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,

[Severity: High]
This isn't a bug introduced by this patch, but this pre-existing issue performs I2C reads here in abx80x_read_alarm() without acquiring priv->lock.

Could concurrent execution of these lockless I2C reads via userspace ioctls interleave with multi-transaction I2C sequences protected by priv->lock?

For example, if this read interrupts the sequence between writing a config key and a target register in functions like abx80x_rtc_set_autocalibration() or oscillator_store(), would it reset the hardware config key and cause the subsequent register write to fail silently?

A similar issue also exists in abx80x_rtc_read_time() and the sysfs show functions.

[ ... ]

While reviewing this driver, I noticed another pre-existing issue in the unmodified abx80x_ioctl() code:

drivers/rtc/rtc-abx80x.c:abx80x_ioctl() {
    ...
	case RTC_VL_CLR:
		scoped_guard(mutex, &priv->lock) {
			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
			if (status < 0)
				return status;

			status &= ~ABX8XX_STATUS_BLF;

			tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
							status);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this read-modify-write on a Write-0-to-Clear (W0C) register create a Time-Of-Check to Time-Of-Use (TOCTOU) race?

If the hardware asserts an unrelated interrupt (like ABX8XX_STATUS_AF) between the read and the write, it seems this write will place a 0 in those bits and unintentionally clear the new interrupts, causing permanent loss of alarm or watchdog events.

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

PATCH v6 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 | 393 +++++++++++++++++++++------------------
 2 files changed, 213 insertions(+), 181 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 +212 -181

770 lines not on this page · raw message

Signature · 1 line
2.55.0
#13 lore raw

Re: [PATCH v6 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: - [High] abx80x_read_alarm() unconditionally fails and returns -EIO because of a leftover partial-read…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] abx80x_read_alarm() unconditionally fails and returns -EIO because of a leftover partial-read check. --

commit 927241f9edd452c16c2bc8ee202f13c63e681b56
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: use regmap instead of I2C specific API

This commit refactors the abx80x RTC driver to use the regmap API instead of the i2c_smbus_* functions. This abstraction prepares the driver for future SPI support for ABX81X RTCs.

Quoted 17 lines
drivers/rtc/rtc-abx80x.c +3 -3
@@ -305,25 +308,24 @@ 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);
+	struct abx80x_priv *priv = dev_get_drvdata(dev);
 	unsigned char buf[7];
 
-	int irq_mask, err;
+	unsigned int irq_mask;
+	int err;
 
 	if (priv->irq <= 0)

drivers/rtc/rtc-abx80x.c static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) · line 324

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2 -3
 		return -EINVAL;
 
-	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
-					    sizeof(buf), buf);
-	if (err < 0)
+	err = regmap_bulk_read(priv->regmap, ABX8XX_REG_ASC, buf, sizeof(buf));
+	if (err)
 		return err;
 	if (err < ARRAY_SIZE(buf))
 		return -EIO;

[Severity: High]
Does this inadvertently fail on all successful reads?

The legacy i2c_smbus_read_i2c_block_data() function returned the number of bytes read, but regmap_bulk_read() returns 0 on success. Because ARRAY_SIZE(buf) is 7, a successful regmap read where err == 0 will satisfy the err < ARRAY_SIZE(buf) condition, causing abx80x_read_alarm() to always return -EIO.

Should this partial-read check be removed entirely now that the driver uses regmap_bulk_read()?

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

PATCH v6 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 | 134 ++++++++++-----------------------------
 1 file changed, 34 insertions(+), 100 deletions(-)
drivers/rtc/rtc-abx80x.c +34 -100
index 7b8e6546ccff..b1d8d5a5ddcf 100644
@@ -232,7 +232,6 @@ 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];
-	unsigned int flags;
 	int err;
 
 	if (tm->tm_year < 100)
@@ -257,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)
@@ -380,7 +372,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)) {
@@ -388,12 +379,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) {
@@ -405,13 +390,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)
@@ -475,7 +462,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;
@@ -488,24 +474,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;
 }
@@ -575,18 +551,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;
@@ -843,7 +812,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;
@@ -893,14 +861,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;
@@ -914,30 +877,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;
 		}
 
@@ -948,8 +891,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");
@@ -1000,25 +949,11 @@ 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;
 	}
 
@@ -1026,11 +961,10 @@ static int abx80x_probe(struct i2c_client *client)
 	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
-	err = regmap_write(regmap, ABX8XX_REG_OSC,
-			   data & ~(ABX8XX_OSC_ACIE |
-				    ABX8XX_OSC_OFIE));
+	err = regmap_write_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 v6 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Refactoring to `regmap_write_bits()` and `regmap_update_bits()` breaks the hardware register…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Refactoring to regmap_write_bits() and regmap_update_bits() breaks the hardware register unlock sequence by inserting an implicit I2C read transaction. --

commit 46c86d8ff37edcfaf17e15de29d0c3426614da35

Author: Antoni Pokusinski <[email protected]>
Date:   Mon Sep 7 20:32:14 2026 +0000

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

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.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 7b8e6546ccffc..b1d8d5a5ddcf0 100644

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -405,13 +390,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 402

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 implicit read within regmap_write_bits() reset the hardware unlock key?

Because the driver does not use a regmap cache (REGCACHE_NONE), calling regmap_write_bits() or regmap_update_bits() will perform an implicit I2C read before the write.

In abx80x_rtc_set_autocalibration(), any I2C transaction occurring after the unlock key is written might reset the hardware protection state machine, causing the subsequent write to fail.

[ ... ]

Quoted 6 lines
drivers/rtc/rtc-abx80x.c +0 -0
@@ -488,24 +474,14 @@ static ssize_t oscillator_store(struct device *dev,
 
 	guard(mutex)(&priv->lock);
 
 	/* Unlock write access on Oscillator Control register */
 	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)

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

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -2
 		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 ? retval : count;

[Severity: High]
Does this have the same issue with the implicit read voiding the unlock sequence?

The sysfs update to ABX8XX_REG_OSC might silently fail here.

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +1 -2
@@ -948,8 +891,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 904

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]
Can the RV1805 specific output control configuration fail here?

Similar to the oscillator register, writing to ABX8XX_REG_OUT_CTRL requires an unlock key sequence that could be broken by the implicit I2C read.

[ ... ]

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
@@ -1026,11 +961,10 @@ static int abx80x_probe(struct i2c_client *client)
 	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 

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

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_write_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]
Will oscillator control initialization at probe time fail due to the injected read?

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

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

Quoted 30 lines · On Mon, Sep 07, 2026 at 08:46:34PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Refactoring to regmap_write_bits() and regmap_update_bits() breaks the hardware register unlock sequence by inserting an implicit I2C read transaction. --

commit 46c86d8ff37edcfaf17e15de29d0c3426614da35

Author: Antoni Pokusinski <[email protected]>
Date:   Mon Sep 7 20:32:14 2026 +0000

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

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.

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
index 7b8e6546ccffc..b1d8d5a5ddcf0 100644

[ ... ]

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2 -0
@@ -405,13 +390,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;
 
Quoted 12 lines

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

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +3 -3
-	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 implicit read within regmap_write_bits() reset the hardware unlock key?

No, because reads do not cause configuration key reset. Same thing for all the comments below.

Quoted 79 lines

Because the driver does not use a regmap cache (REGCACHE_NONE), calling regmap_write_bits() or regmap_update_bits() will perform an implicit I2C read before the write.

In abx80x_rtc_set_autocalibration(), any I2C transaction occurring after the unlock key is written might reset the hardware protection state machine, causing the subsequent write to fail.

[ ... ]

Quoted 6 lines
drivers/rtc/rtc-abx80x.c +0 -0
@@ -488,24 +474,14 @@ static ssize_t oscillator_store(struct device *dev,
 
 	guard(mutex)(&priv->lock);
 
 	/* Unlock write access on Oscillator Control register */
 	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)

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

Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3 -2
 		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 ? retval : count;

[Severity: High]
Does this have the same issue with the implicit read voiding the unlock sequence?

The sysfs update to ABX8XX_REG_OSC might silently fail here.

[ ... ]

Quoted 7 lines
drivers/rtc/rtc-abx80x.c +1 -2
@@ -948,8 +891,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 904

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]
Can the RV1805 specific output control configuration fail here?

Similar to the oscillator register, writing to ABX8XX_REG_OUT_CTRL requires an unlock key sequence that could be broken by the implicit I2C read.

[ ... ]

Quoted 4 lines
drivers/rtc/rtc-abx80x.c +0 -0
@@ -1026,11 +961,10 @@ static int abx80x_probe(struct i2c_client *client)
 	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 

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

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_write_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]
Will oscillator control initialization at probe time fail due to the injected read?

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

PATCH v6 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 | 173 ++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 67 deletions(-)
drivers/rtc/rtc-abx80x.c +106 -67
index b1d8d5a5ddcf..c90407a023fe 100644
@@ -782,73 +782,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;
 	}
 
@@ -858,14 +823,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;
 	}
 
@@ -880,7 +845,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;
 		}
 
@@ -888,7 +853,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;
 
 		/*
@@ -900,7 +865,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;
 		}
@@ -912,26 +877,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));
@@ -939,7 +902,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;
 	}
@@ -953,30 +916,30 @@ 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;
 	}
 
 	/* Unlock write access to Oscillator Control Register */
-	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
+	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
 		return -EIO;
 
 	err = regmap_write_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;
 		}
 	}
@@ -985,14 +948,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[] = {
 	{
@@ -1040,16 +1028,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 v6 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 v6 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 v6 9/9] rtc: abx80x: add support for ABX81X

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The `regmap_bulk_read` return value check in `abx80x_read_alarm` causes the function to…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The regmap_bulk_read return value check in abx80x_read_alarm causes the function to unconditionally fail with -EIO on success. --

commit 7f9a5b9080b676338469e285a0ce05b36660c82c
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add support for ABX81X

This patch adds support for the Abracon ABX81X family of RTCs, which feature a SPI interface. It updates the Kconfig to support both I2C and SPI, and adds the necessary SPI device IDs, device tree bindings, and probe functions.

Quoted 5 lines
drivers/rtc/rtc-abx80x.c +0 -0
index c90407a023fe8..c3bf6f0ae48b9 100644

drivers/rtc/rtc-abx80x.c line 7

Quoted 9 lines
drivers/rtc/rtc-abx80x.c +2 -2
@@ -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
  *
  * Copyright 2014-2015 Macq S.A.
  *

[Severity: High]
Will the regmap_bulk_read return value check in abx80x_read_alarm() cause the function to unconditionally fail?

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
...

	err = regmap_bulk_read(priv->regmap, ABX8XX_REG_ASC, buf, sizeof(buf));
	if (err)
		return err;
	if (err < ARRAY_SIZE(buf))
		return -EIO;

...
}

When the RTC core calls abx80x_read_alarm(), regmap_bulk_read() will return 0 on a successful read. The subsequent check evaluates
if (err < ARRAY_SIZE(buf)), which is 0 < 7. This evaluates to true, resulting in an unconditional return of -EIO and breaking the alarm read functionality for any unprivileged userspace application reading the RTC alarm.

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