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

[Antoni Pokusinski <apokusinski01@gmail.com>]

On Wed, Sep 16, 2026 at 09:05:20PM +0000, sashiko-bot@kernel.org 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 <apokusinski01@gmail.com>
> 
> 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.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> [ ... ]
> > @@ -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);
> >  }
> 
> [Severity: High]
> Since regmap_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.
> 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.
> 
> [ ... ]
> > @@ -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");
> 
> [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.
> 
> [ ... ]
> > @@ -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) {
> 
> [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?
> 
> [ ... ]
> > @@ -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?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260916205138.16771-1-apokusinski01@gmail.com?part=7