Re: [PATCH 3/6] rtc: abx80x: use regmap instead of I2C specific API

On Wed, Jul 22, 2026 at 08:16:45PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Hardware unlock sequence is broken by read-modify-write inversion
> - [Low] Incompatible pointer types passed to regmap_read()
> --
> 
> --- Patch [3]: [PATCH 3/6] rtc: abx80x: use regmap instead of I2C specific API ---
> commit 97cd1f33d168c96f848a8f59ba284bfe9c5f89fe
> Author: Antoni Pokusinski <[email protected]>
> Date:   Wed Jul 22 19:58:11 2026 +0000
> 
>     rtc: abx80x: use regmap instead of I2C specific API
>     
>     Rework the driver so that it uses regmap_* functions for communication
>     with the device instead of the i2c_smbus_* API. This way the support for
>     ABX81X RTCs which use SPI can be added later.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index 9b0e1d48d9ba3..e5ba821a67689 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> > @@ -125,52 +126,53 @@ static struct abx80x_cap abx80x_caps[] = {
> [ ... ]
> > -static int abx80x_is_rc_mode(struct i2c_client *client)
> > +static int abx80x_is_rc_mode(struct device *dev)
> >  {
> > -	int flags = 0;
> > +	struct abx80x_priv *priv = dev_get_drvdata(dev);
> > +	int err, flags = 0;
> >  
> > -	flags =  i2c_smbus_read_byte_data(client, ABX8XX_REG_OSS);
> > -	if (flags < 0) {
> > -		dev_err(&client->dev,
> > -			"Failed to read autocalibration attribute\n");
> > -		return flags;
> > +	err = regmap_read(priv->regmap, ABX8XX_REG_OSS, &flags);
> 
> [Severity: Low]
> Does this pass an incompatible pointer type to regmap_read()?
> 
> The API signature of regmap_read() expects an unsigned int * for its third
> argument, but the address of a local int variable (&flags) is passed here.
> While this may not cause memory corruption since int and unsigned int are
> the same size, it violates the function signature and generates
> -Wincompatible-pointer-types warnings, which can break builds compiling with
> -Werror.
> 
> > +	if (err < 0) {
> > +		dev_err(dev, "Failed to read autocalibration attribute\n");
> > +		return err;
> >  	}
> [ ... ]
> > @@ -259,13 +260,13 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >  
> >  static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> >  {
> > -	struct i2c_client *client = dev_id;
> > -	struct abx80x_priv *priv = i2c_get_clientdata(client);
> > +	struct device *dev = dev_id;
> > +	struct abx80x_priv *priv = dev_get_drvdata(dev);
> >  	struct rtc_device *rtc = priv->rtc;
> > -	int status;
> > +	int err, status;
> >  
> > -	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> > -	if (status < 0)
> > +	err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status);
> 
> [Severity: Low]
> Does this pass an incompatible pointer type to regmap_read()?
> 
> Similar to abx80x_is_rc_mode() above, this passes an int pointer (&status)
> instead of an unsigned int pointer.
>
I don't think it matters in all these cases, but I could make
these values unsigned int just to maintain the pointer type
compatibility.

> > +	if (err < 0)
> >  		return IRQ_NONE;
> [ ... ]
> > @@ -294,14 +294,13 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> >  	if (priv->irq <= 0)
> >  		return -EINVAL;
> >  
> > -	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> > -					    sizeof(buf), buf);
> > +	err = regmap_bulk_read(priv->regmap, ABX8XX_REG_ASC, buf, sizeof(buf));
> >  	if (err)
> >  		return err;
> >  
> > -	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
> > -	if (irq_mask < 0)
> > -		return irq_mask;
> > +	err = regmap_read(priv->regmap, ABX8XX_REG_IRQ, &irq_mask);
> 
> [Severity: Low]
> Does this pass an incompatible pointer type to regmap_read()?
> 
> This passes an int pointer (&irq_mask) instead of an unsigned int pointer.
> 
> > +	if (err < 0)
> > +		return err;
> [ ... ]
> > @@ -379,22 +376,22 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
> >  	}
> >  
> >  	/* Unlock write access to Oscillator Control Register */
> > -	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
> > +	if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0)
> >  		return -EIO;
> >  
> > -	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC, flags);
> > +	retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags);
> 
> [Severity: High]
> Does this hardware unlock sequence get broken by read-modify-write inversion?
> 
> According to the datasheet, any subsequent I2C access (read or write) resets
> the configuration key to zero. If this sequence uses a read-modify-write
> operation (such as regmap_write_bits()), a hardware read is performed after
> the unlock key is written. This read resets the configuration key, causing the
> hardware to ignore the subsequent write and breaking features like
> autocalibration.
>
I see no issue here. The datasheet clearly states that
"CONFKEY is reset to 0x00 on any register **write**". I experienced no
issues with the current version of the driver when testing it on AB1815 hardware.

(this comment also applies to the other read-modify-write "bug" reports by
sashiko in this patch series)