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

[Antoni Pokusinski <apokusinski01@gmail.com>]

On Mon, Sep 07, 2026 at 08:46:34PM +0000, sashiko-bot@kernel.org wrote:
> 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?
>
No, because reads do not cause configuration key reset.
Same thing for all the comments below. 
> 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