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

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 <[email protected]>

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.

[ ... ]

> @@ -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.

[ ... ]

> @@ -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?

[ ... ]

> -	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?

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/[email protected]?part=7