rtc: abx80x: add support for abx81x
Subsystems (2)
v7 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 v6:
* PATCH 4 ("rtc: abx80x: properly handle shared IRQs")
- handle_irq: write status & ~status_flags_clear instead of ~status_flags_clear
- probe: setup watchdog right after register writes
* PATCH 6 ("rtc: abx80x: use regmap instead of I2C specific API")
- removed the return -EIO case
---
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 | 656 +++++++++++------- 3 files changed, 449 insertions(+), 250 deletions(-)
Signature · 1 line
2.55.0
PATCH v7 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
@@ -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
[email protected]
AI reviewRe: [PATCH v7 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.
Reviewed-by Krzysztof Kozlowski <[email protected]>Signed-off-by Antoni Pokusinski <[email protected]>
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
PATCH v7 2/9 rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()
In reply to Antoni Pokusinskii2c_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.
--- drivers/rtc/rtc-abx80x.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/rtc/rtc-abx80x.c +4 -2
@@ -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
[email protected]
AI reviewRe: [PATCH v7 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")Signed-off-by Antoni Pokusinski <[email protected]>
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
PATCH v7 3/9 rtc: abx80x: add mutex protection for register writes
In reply to Antoni PokusinskiThe 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
@@ -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
[email protected]
AI reviewRe: [PATCH v7 3/9] rtc: abx80x: add mutex protection for register writes
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 6 lines
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.
Signed-off-by Antoni Pokusinski <[email protected]>
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=3
PATCH v7 4/9 rtc: abx80x: properly handle shared IRQs
In reply to Antoni PokusinskiThe 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.
--- drivers/rtc/rtc-abx80x.c | 52 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)
drivers/rtc/rtc-abx80x.c +50 -2
@@ -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 & ~status_flags_clear); +return IRQ_HANDLED ; + } + +return IRQ_NONE ; }static int abx80x_read_alarm (struct device *dev,struct rtc_wkalrm *t)
@@ -942,6 +955,41 @@ static int abx80x_probe(struct i2c_client *client)i2c_set_clientdata (client, priv); +/* 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 (abx80x_caps[part].has_wdog) { err =abx80x_setup_watchdog (priv);if (err)
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v7 4/9] rtc: abx80x: properly handle shared IRQs
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] rtc: abx80x: race condition loses interrupts due to flawed W0C status register clearing --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: abx80x: race condition loses interrupts due to flawed W0C status register clearing --
commit 27cfcb7135851594344aa538f01f8ef1328d2392
Author: Antoni Pokusinski <[email protected]>
rtc: abx80x: properly handle shared IRQs
This commit updates the abx80x RTC interrupt handler to only return IRQ_HANDLED and clear the status register when the alarm or watchdog flags are set. It aims to properly support IRQF_SHARED by returning IRQ_NONE for unrelated interrupts.
Quoted 3 lines
drivers/rtc/rtc-abx80x.c +0
-0
[ ... ]
drivers/rtc/rtc-abx80x.c static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) · line 298
Quoted 11 lines
drivers/rtc/rtc-abx80x.c +4
-2@@ -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 & ~status_flags_clear);
[Severity: High]
Does this logic introduce a race condition that loses late-arriving interrupts in abx80x_handle_irq()?
Because the ABX8XX_REG_STATUS register is W0C (write-0-to-clear), any bit written as 0 will be cleared in hardware.
The expression status & ~status_flags_clear forces a 0 for all bits that were 0 during the initial read. If a new hardware interrupt asserts between the read and this write, its status bit will be newly set to 1 in hardware. However, writing back the old status value forces a 0 for that new bit, silently clearing it and losing the event.
To preserve unhandled or newly asserted bits, the correct write value for a W0C register is ~status_flags_clear instead.
Quoted 5 lines
drivers/rtc/rtc-abx80x.c +4
-0+return IRQ_HANDLED ; + } + +return IRQ_NONE ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v7 4/9] rtc: abx80x: properly handle shared IRQs
Quoted 39 lines · On Wed, Sep 16, 2026 at 09:05:27PM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: abx80x: race condition loses interrupts due to flawed W0C status register clearing --commit
27cfcb7135851594344aa538f01f8ef1328d2392
Author: Antoni Pokusinski <[email protected]>rtc: abx80x: properly handle shared IRQs
This commit updates the abx80x RTC interrupt handler to only return
IRQ_HANDLEDand clear the status register when the alarm or watchdog flags are set. It aims to properly supportIRQF_SHAREDby returningIRQ_NONEfor unrelated interrupts.Quoted 3 lines
drivers/rtc/rtc-abx80x.c +0
-0[ ... ]
drivers/rtc/rtc-abx80x.c static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) · line 298
Quoted 11 lines
drivers/rtc/rtc-abx80x.c +4
-2@@ -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 & ~status_flags_clear);[Severity: High]
Does this logic introduce a race condition that loses late-arriving interrupts inabx80x_handle_irq()?Because the
ABX8XX_REG_STATUSregister is W0C (write-0-to-clear), any bit written as 0 will be cleared in hardware.
Quoted 9 lines
The expression status & ~status_flags_clear forces a 0 for all bits that were 0 during the initial read. If a new hardware interrupt asserts between the read and this write, its status bit will be newly set to 1 in hardware. However, writing back the old status value forces a 0 for that new bit, silently clearing it and losing the event.
To preserve unhandled or newly asserted bits, the correct write value for a W0C register is ~status_flags_clear instead.
Not really: if the register bits are written with 1, then during the next call to handle_irq() all the interrupts are reported as pending (in other words the written ones are preserved).
In particular, when I was testing the alarm interrupt and the written value was ~status_flags_clear as sashiko suggests, I was always getting the WDT bit enabled and hence the alert "watchdog timeout interrupt" was printed.
Quoted 8 lines
Quoted 5 lines
drivers/rtc/rtc-abx80x.c +4
-0+return IRQ_HANDLED ; + } + +return IRQ_NONE ; }Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v7 5/9 rtc: abx80x: add irq to struct abx80x_priv
In reply to Antoni PokusinskiThis 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
@@ -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)if (err)return err; -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
[email protected]
AI reviewRe: [PATCH v7 5/9] rtc: abx80x: add irq to struct abx80x_priv
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines
This way the irq number is not tied to the
i2c_clientand can be easily accessed also in the case of ABX81X SPI clocks.Signed-off-by Antoni Pokusinski <[email protected]>
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=5
PATCH v7 6/9 rtc: abx80x: use regmap instead of I2C specific API
In reply to Antoni PokusinskiRework 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 | 395 +++++++++++++++++++++------------------ 2 files changed, 213 insertions(+), 183 deletions(-)
drivers/rtc/Kconfig +1 -0
@@ -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 -183
770 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v7 6/9] rtc: abx80x: use regmap instead of I2C specific API
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
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.
Signed-off-by Antoni Pokusinski <[email protected]>
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=6
PATCH v7 7/9 rtc: abx80x: replace read-modify-write pattern with regmap helpers
In reply to Antoni PokusinskiBefore 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
@@ -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)
@@ -378,7 +370,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 +377,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 +388,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 +460,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 +472,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 +549,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 +810,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 +859,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 +875,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 +889,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" );
@@ -988,25 +937,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 ; }
@@ -1014,11 +949,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
[email protected]
AI reviewRe: [PATCH v7 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] rtc: abx80x: hardware unlock broken by regmap RMW helpers --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: abx80x: hardware unlock broken by regmap RMW helpers
--
commit aad0cc39d854acf9e4ccd3b1fa7b17484a428c9a
Author: Antoni Pokusinski <[email protected]>
rtc: abx80x: replace read-modify-write pattern with regmap helpers
This commit refactors the driver to replace open-coded read-modify-write sequences with the regmap_update_bits() and regmap_write_bits() helpers to simplify the code.
Quoted 3 lines
drivers/rtc/rtc-abx80x.c +0
-0
[ ... ]
Quoted 7 lines
drivers/rtc/rtc-abx80x.c +2
-0@@ -403,13 +388,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 400
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3
-3if (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]
Since regmap_write_bits() performs an internal read-modify-write cycle, will the initial read operation consume the configuration key unlock?
It looks like writing the configuration key only unlocks the very next transaction. If the regmap helper reads first, the actual write might fail silently and the hardware would reject the autocalibration settings.
[ ... ]
Quoted 13 lines
drivers/rtc/rtc-abx80x.c +0
-9@@ -486,24 +472,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 ); -
drivers/rtc/rtc-abx80x.c static ssize_t oscillator_store(struct device *dev, · line 482
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3
-2/* 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" );
[Severity: High]
Does the same issue apply here in oscillator_store()? The internal read within regmap_write_bits() may consume the unlock key, preventing the oscillator selection from being properly updated.
[ ... ]
Quoted 5 lines
drivers/rtc/rtc-abx80x.c +0
-1@@ -946,8 +889,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,
drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 900
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +8
-1- 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 ) {
[Severity: High]
Could this cause the power leakage mitigation write in abx80x_probe() to be rejected by the hardware because the unlock transaction was already consumed by the regmap_write_bits() read phase?
[ ... ]
drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 954
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2
-3@@ -1014,11 +949,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 ) {
[Severity: High]
Will the interrupt disable write also fail silently here due to the unlock being consumed by the regmap read?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
Re: [PATCH v7 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers
Quoted 27 lines · On Wed, Sep 16, 2026 at 09:05:20PM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: abx80x: hardware unlock broken by regmap RMW helpers
--commit
aad0cc39d854acf9e4ccd3b1fa7b17484a428c9a
Author: Antoni Pokusinski <[email protected]>rtc: abx80x: replace read-modify-write pattern with regmap helpers
This commit refactors the driver to replace open-coded read-modify-write sequences with the
regmap_update_bits()andregmap_write_bits()helpers to simplify the code.Quoted 3 lines
drivers/rtc/rtc-abx80x.c +0
-0[ ... ]
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2
-0@@ -403,13 +388,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 400
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]
Sinceregmap_write_bits()performs an internal read-modify-write cycle, will the initial read operation consume the configuration key unlock?
Everything alright here, read operations do not cause the configuration key unlock.
Same thing applies to all the comments below.
Quoted 74 lines
It looks like writing the configuration key only unlocks the very next transaction. If the regmap helper reads first, the actual write might fail silently and the hardware would reject the autocalibration settings.
[ ... ]
Quoted 13 lines
drivers/rtc/rtc-abx80x.c +0
-9@@ -486,24 +472,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 ); -drivers/rtc/rtc-abx80x.c static ssize_t oscillator_store(struct device *dev, · line 482
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +3
-2/* 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" );[Severity: High]
Does the same issue apply here inoscillator_store()? The internal read withinregmap_write_bits()may consume the unlock key, preventing the oscillator selection from being properly updated.[ ... ]
Quoted 5 lines
drivers/rtc/rtc-abx80x.c +0
-1@@ -946,8 +889,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,drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 900
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +8
-1- 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 ) {[Severity: High]
Could this cause the power leakage mitigation write inabx80x_probe()to be rejected by the hardware because the unlock transaction was already consumed by theregmap_write_bits()read phase?[ ... ]
drivers/rtc/rtc-abx80x.c static int abx80x_probe(struct i2c_client *client) · line 954
Quoted 10 lines
drivers/rtc/rtc-abx80x.c +2
-3@@ -1014,11 +949,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 ) {[Severity: High]
Will the interrupt disable write also fail silently here due to the unlock being consumed by the regmap read?Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v7 8/9 rtc: abx80x: create abx80x_i2c_probe()
In reply to Antoni PokusinskiMove 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
@@ -780,73 +780,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 ; }
@@ -856,14 +821,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 ; }
@@ -878,7 +843,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 ; }
@@ -886,7 +851,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 ;/*
@@ -898,7 +863,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 ; }
@@ -910,26 +875,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 ));
@@ -941,23 +904,23 @@ 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 (abx80x_caps[part].has_wdog) { - err =abx80x_setup_watchdog (&client->dev); + err =abx80x_setup_watchdog (dev);if (err)return err; }
@@ -967,14 +930,14 @@ static int abx80x_probe(struct i2c_client *client)return err;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 ; } }
@@ -983,14 +946,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[] = { {
@@ -1038,16 +1026,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
[email protected]
AI reviewRe: [PATCH v7 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.Signed-off-by Antoni Pokusinski <[email protected]>
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
PATCH v7 9/9 rtc: abx80x: add support for ABX81X
In reply to Antoni PokusinskiAbracon 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
@@ -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
[email protected]
AI reviewRe: [PATCH v7 9/9] rtc: abx80x: add support for ABX81X
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines
Abracon ABX81X is a family of RTCs with SPI interface featuring the same functionalities as the ABX80X I2C clocks.
Signed-off-by Antoni Pokusinski <[email protected]>
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=9
Quoted 10 lines · Antoni Pokusinski
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[...]
Applied, thanks!
[1/9] dt-bindings: rtc: abx80x: document ABX81X RTCs
https://git.kernel.org/abelloni/c/953c3d2b5836 [2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm() https://git.kernel.org/abelloni/c/07c57022afb3 [3/9] rtc: abx80x: add mutex protection for register writes https://git.kernel.org/abelloni/c/74bff654a4ce [4/9] rtc: abx80x: properly handle shared IRQs https://git.kernel.org/abelloni/c/632916c7bde3 [5/9] rtc: abx80x: add irq to struct abx80x_priv https://git.kernel.org/abelloni/c/00b978f8008e [6/9] rtc: abx80x: use regmap instead of I2C specific API https://git.kernel.org/abelloni/c/724320a61325 [7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers https://git.kernel.org/abelloni/c/0ba70c2b1e3e [8/9] rtc: abx80x: create abx80x_i2c_probe() https://git.kernel.org/abelloni/c/3fcf5a97957b [9/9] rtc: abx80x: add support for ABX81X https://git.kernel.org/abelloni/c/8a017ab98412
Best regards,
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com