Re: [PATCH 4/6] 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] Replacing manual read-modify-write patterns with `regmap_write_bits()` or `regmap_update_bits()` breaks the hardware's unlock sequence because they inject an implicit I2C read between unlocking the configuration key and the actual write, causing the hardware to re-lock the register.
--

--- Patch [4]: [PATCH 4/6] rtc: abx80x: replace read-modify-write pattern with regmap helpers ---
commit ac40d4eea0cf607d67d28eb1332803662646f9c2
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: replace read-modify-write pattern with regmap helpers
    
This patch refactors the driver to use regmap_update_bits() and
regmap_write_bits() instead of manually reading, modifying, and writing
registers. The goal is to simplify the codebase.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index e5ba821a67689..7761cb353994e 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -379,9 +368,9 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
>  	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 replacing regmap_write() with regmap_write_bits() here break the hardware's
unlock sequence?

The regmap_write_bits() function performs an implicit read before writing.
Because it is called immediately after abx80x_write_config_key(), this injects
an I2C read between unlocking the configuration key and the actual write.

Since the AB08XX/RV1805 RTC requires the protected register write to be the
very next transaction after the configuration key write, won't this read
clear the unlock state and cause the hardware to silently discard the write?

>  }
>  
>  static int abx80x_rtc_get_autocalibration(struct device *dev)
[ ... ]
> @@ -454,24 +443,14 @@ static ssize_t oscillator_store(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	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));

[Severity: High]
Could the implicit read introduced by regmap_write_bits() cause the
hardware to re-lock the register?

As with abx80x_rtc_set_autocalibration(), the injected read operation occurs
after unlocking the configuration key with abx80x_write_config_key(), which
disrupts the sequence required to successfully write to the Oscillator Control
register.

> +	if (retval < 0)
>  		dev_err(dev, "Failed to write Oscillator Control register\n");
> -		return retval;
> -	}
>  
>  	return retval ? retval : count;
>  }
[ ... ]
> @@ -901,8 +846,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);

[Severity: High]
Is it possible that the OUT_CTRL EXDS bit configuration silently fails here?

Since regmap_write_bits() performs a read under the hood, placing it directly
after the abx80x_write_config_key() unlock command interrupts the required
write sequence, causing the hardware to discard the write operation.

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

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