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

[Antoni Pokusinski <apokusinski01@gmail.com>]

On Tue, Aug 11, 2026 at 07:56:53PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] The patch replaces read-modify-write sequences with `regmap_write_bits()` after writing the hardware configuration key, breaking the write-protection unlock sequence.
> 
> Pre-existing issues:
> - [High] The probe function attempts to clear interrupt enable bits (`ABX8XX_OSC_ACIE` and `ABX8XX_OSC_OFIE`) in the protected Oscillator Control Register (`ABX8XX_REG_OSC`) without first writing the required configuration key (`ABX8XX_CFG_KEY_OSC`).
> --
> 
> --- Patch [7]: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers ---
> commit 6808e36654fbc0a72e9b540a05e0f2557530aa65
> Author: Antoni Pokusinski <apokusinski01@gmail.com>
> 
> rtc: abx80x: replace read-modify-write pattern with regmap helpers
> 
> This commit replaces read-modify-write sequences with regmap_update_bits()
> and regmap_write_bits() to simplify register updates.
> 
> > 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 this change break the configuration key unlock sequence?
> 
> The configuration key unlock only allows a single I2C write operation. Because
> regmap_write_bits() internally performs a read-modify-write sequence, it
> initiates an I2C read transaction before the actual write. Will this inserted
> read consume the unlock key and cause the subsequent data write to be silently
> rejected by the hardware?
> 
> The original code explicitly read the register before unlocking it to avoid
> this sequence.
> 
> [ ... ]
> 
No it won't because reads do not cause configuration key reset.
> > @@ -486,24 +473,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");
> > -		return retval;
> > -	}
> >  
> >  	return retval ? retval : count;
> >  }
> 
> [Severity: High]
> Will this regmap_write_bits() call also fail for the same reason? 
> 
> Since it follows abx80x_write_config_key(), the implicit read operation will
> likely consume the unlock key before the write occurs.
> 
> [ ... ]
> 
No, same reason as above
> > @@ -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]
> Similar to the above, does the implicit read inside regmap_write_bits()
> consume the configuration key unlock for the MISC register before the actual
> write happens?
> 
> [ ... ]
> 
No, same reason as above
> > -	err = regmap_read(regmap, ABX8XX_REG_OSC, &data);
> > +	err = regmap_update_bits(regmap, ABX8XX_REG_IRQ,
> > +				 ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E |
> > +				 ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0);
> >  	if (err < 0) {
> > -		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
> > +		dev_err(&client->dev, "Unable to update irq register\n");
> >  		return -EIO;
> >  	}
> >  
> > -	err = regmap_write(regmap, ABX8XX_REG_OSC,
> > -			   data & ~(ABX8XX_OSC_ACIE |
> > -				    ABX8XX_OSC_OFIE));
> > +	err = regmap_update_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]
> This is a pre-existing issue, but does this register update require unlocking
> first?
> 
> It appears the probe function attempts to clear the interrupt enable bits in the
> Oscillator Control Register without first writing the required configuration key
> (ABX8XX_CFG_KEY_OSC).
> 
> If the configuration key is not written first, will the hardware silently ignore
> this write, leaving interrupts enabled if they were previously set by the
> bootloader?
>
Yes, will be fixed in v6
> If an unhandled interrupt (like Autocalibration or Oscillator Failure) fires,
> could it cause an interrupt storm? The IRQ handler abx80x_handle_irq() only
> checks for AF and WDT flags. Since the IRQ is requested with IRQF_SHARED, an
> unhandled interrupt might result in the kernel disabling the shared IRQ line
> and breaking unrelated devices.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260811194034.28937-1-apokusinski01@gmail.com?part=7