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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware unlock sequence is broken by read-modify-write inversion
- [Low] Incompatible pointer types passed to regmap_read()
--

--- Patch [3]: [PATCH 3/6] rtc: abx80x: use regmap instead of I2C specific API ---
commit 97cd1f33d168c96f848a8f59ba284bfe9c5f89fe
Author: Antoni Pokusinski <[email protected]>
Date:   Wed Jul 22 19:58:11 2026 +0000

    rtc: abx80x: use regmap instead of I2C specific API
    
    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.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 9b0e1d48d9ba3..e5ba821a67689 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -125,52 +126,53 @@ static struct abx80x_cap abx80x_caps[] = {
[ ... ]
> -static int abx80x_is_rc_mode(struct i2c_client *client)
> +static int abx80x_is_rc_mode(struct device *dev)
>  {
> -	int flags = 0;
> +	struct abx80x_priv *priv = dev_get_drvdata(dev);
> +	int err, flags = 0;
>  
> -	flags =  i2c_smbus_read_byte_data(client, ABX8XX_REG_OSS);
> -	if (flags < 0) {
> -		dev_err(&client->dev,
> -			"Failed to read autocalibration attribute\n");
> -		return flags;
> +	err = regmap_read(priv->regmap, ABX8XX_REG_OSS, &flags);

[Severity: Low]
Does this pass an incompatible pointer type to regmap_read()?

The API signature of regmap_read() expects an unsigned int * for its third
argument, but the address of a local int variable (&flags) is passed here.
While this may not cause memory corruption since int and unsigned int are
the same size, it violates the function signature and generates
-Wincompatible-pointer-types warnings, which can break builds compiling with
-Werror.

> +	if (err < 0) {
> +		dev_err(dev, "Failed to read autocalibration attribute\n");
> +		return err;
>  	}
[ ... ]
> @@ -259,13 +260,13 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  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 device *dev = dev_id;
> +	struct abx80x_priv *priv = dev_get_drvdata(dev);
>  	struct rtc_device *rtc = priv->rtc;
> -	int status;
> +	int err, status;
>  
> -	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> -	if (status < 0)
> +	err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status);

[Severity: Low]
Does this pass an incompatible pointer type to regmap_read()?

Similar to abx80x_is_rc_mode() above, this passes an int pointer (&status)
instead of an unsigned int pointer.

> +	if (err < 0)
>  		return IRQ_NONE;
[ ... ]
> @@ -294,14 +294,13 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
>  	if (priv->irq <= 0)
>  		return -EINVAL;
>  
> -	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> -					    sizeof(buf), buf);
> +	err = regmap_bulk_read(priv->regmap, ABX8XX_REG_ASC, buf, sizeof(buf));
>  	if (err)
>  		return err;
>  
> -	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
> -	if (irq_mask < 0)
> -		return irq_mask;
> +	err = regmap_read(priv->regmap, ABX8XX_REG_IRQ, &irq_mask);

[Severity: Low]
Does this pass an incompatible pointer type to regmap_read()?

This passes an int pointer (&irq_mask) instead of an unsigned int pointer.

> +	if (err < 0)
> +		return err;
[ ... ]
> @@ -379,22 +376,22 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
>  	}
>  
>  	/* 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;
>  
> -	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC, flags);
> +	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);

[Severity: High]
Does this hardware unlock sequence get broken by read-modify-write inversion?

According to the datasheet, any subsequent I2C access (read or write) resets
the configuration key to zero. If this sequence uses a read-modify-write
operation (such as regmap_write_bits()), a hardware read is performed after
the unlock key is written. This read resets the configuration key, causing the
hardware to ignore the subsequent write and breaking features like
autocalibration.

>  
>  	return retval;
>  }
>  
>  static int abx80x_rtc_get_autocalibration(struct device *dev)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	int flags = 0, autocalibration;
> +	struct abx80x_priv *priv = dev_get_drvdata(dev);
> +	int flags = 0, autocalibration, err;
>  
> -	flags =  i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
> -	if (flags < 0)
> -		return flags;
> +	err = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags);

[Severity: Low]
Does this pass an incompatible pointer type to regmap_read()?

This passes an int pointer (&flags) instead of an unsigned int pointer.

> +	if (err < 0)
> +		return err;
[ ... ]
> @@ -467,10 +464,10 @@ static ssize_t oscillator_store(struct device *dev,
>  		flags |= (ABX8XX_OSC_OSEL);
>  
>  	/* Unlock write access on Oscillator Control register */
> -	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
> +	if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0)
>  		return -EIO;
>  
> -	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC, flags);
> +	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);

[Severity: High]
Does this hardware unlock sequence get broken by read-modify-write inversion?

As noted in abx80x_rtc_set_autocalibration() above, a hardware read performed
after the unlock key is written clears the key. This resets the key before the
protected write if a read-modify-write operation like regmap_write_bits() is
used.

>  	if (retval < 0) {
>  		dev_err(dev, "Failed to write Oscillator Control register\n");
[ ... ]
> @@ -513,45 +509,43 @@ static const struct attribute_group rtc_calib_attr_group = {
>  
>  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);
> -	int status, tmp;
> +	struct abx80x_priv *priv = dev_get_drvdata(dev);
> +	int err, status, tmp;
>  
>  	switch (cmd) {
>  	case RTC_VL_READ:
> -		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> -		if (status < 0)
> -			return status;
> +		err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status);

[Severity: Low]
Does this pass an incompatible pointer type to regmap_read()?

This passes an int pointer (&status) instead of an unsigned int pointer.

> +		if (err < 0)
> +			return err;
[ ... ]
> @@ -856,11 +898,11 @@ 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, ABX8XX_CFG_KEY_MISC) < 0)
> +		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
>  			return -EIO;
>  
> -		err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OUT_CTRL,
> -						data | ABX8XX_OUT_CTRL_EXDS);
> +		err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL,
> +				   data | ABX8XX_OUT_CTRL_EXDS);

[Severity: High]
Does this hardware unlock sequence get broken by read-modify-write inversion?

For RV1805, the key is written, but if a read-modify-write operation like
regmap_write_bits() is used here, it performs a read before writing the EXTI
disable bit. This read clears the key, leaving the write ineffective and
defeating the extra power leakage workaround.

>  		if (err < 0) {
>  			dev_err(&client->dev,
>  				"Unable to write output control register\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3