Re: [PATCH v4 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] Using regmap_update_bits() or regmap_write_bits() breaks the hardware Configuration Key unlock sequence by injecting an I2C read between the unlock write and the protected register write.
--

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

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

This commit replaces manual read-modify-write sequences with
regmap_update_bits() and regmap_write_bits() to simplify the code.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5d41882f3c704..ab88af43b4fa0 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c

[ ... ]

> @@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
>  		flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512);
>  	}
>  
> +	guard(mutex)(&priv->lock);
> +
>  	/* Unlock write access to Oscillator Control Register */
>  	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
>  		return -EIO;
>  
> -	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
> -
> -	return retval;
> +	return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC,
> +				 ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512,
> +				 flags);
>  }

[Severity: High]
Does using regmap_write_bits() here break the hardware Configuration Key unlock
sequence?

Since regmap_write_bits() performs a read-modify-write cycle, it injects an
I2C read between the unlock write (abx80x_write_config_key) and the actual
register write.

If the hardware resets the key on any intervening I2C access, this might lock
the register again, causing the subsequent write to fail silently.

[ ... ]

> @@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev,
[ ... ]
>  	/* 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]
Will this read-modify-write cycle also invalidate the unlock key before the
write occurs, similar to the issue in abx80x_rtc_set_autocalibration()?

[ ... ]

> @@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client)
>  		if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0)
>  			return -EIO;
>  
> -		err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL,
> -				   data | ABX8XX_OUT_CTRL_EXDS);
> +		/*
> +		 * Avoid extra power leakage. The RV1805 uses smaller
> +		 * 10pin package and the EXTI input is not present.
> +		 * Disable it to avoid leakage.
> +		 */
> +		err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL,
> +					ABX8XX_OUT_CTRL_EXDS,
> +					ABX8XX_OUT_CTRL_EXDS);
>  		if (err < 0) {
>  			dev_err(&client->dev,
>  				"Unable to write output control register\n");

[Severity: High]
Could the implicit I2C read inside regmap_write_bits() invalidate the MISC
unlock key here as well, causing the EXTI disable write to fail on RV1805
chips and leading to extra power leakage?

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