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

[sashiko-bot@kernel.org]

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 <apokusinski01@gmail.com>
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.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 7b8e6546ccffc..b1d8d5a5ddcf0 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -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);
>  }

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

[ ... ]
> @@ -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)
>  		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.

[ ... ]
> @@ -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");

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

[ ... ]
> @@ -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;
>  	}

[Severity: High]
Will oscillator control initialization at probe time fail due to the injected
read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907203216.53192-1-apokusinski01@gmail.com?part=7