Re: [PATCH v2 8/8] rtc: abx80x: add support for ABX81X

On Sat, Jul 25, 2026 at 03:30:32PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Incorrect SPI read/write flag masks invert SPI commands.
> - [Medium] Masking of valid error codes from `devm_regmap_init_*()`.
> --
> 
> --- Patch [8]: [PATCH v2 8/8] rtc: abx80x: add support for ABX81X ---
> commit f05e60a6ab0d5bdeb5665a7f2de3be7428abd444
> Author: Antoni Pokusinski <[email protected]>
> Subject: rtc: abx80x: add support for ABX81X
> 
> This commit adds SPI support for the Abracon ABX81X family of RTCs. It
> introduces the required regmap configuration and SPI driver probe logic.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index 7cba72656d094..46434f05a1f7a 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> [ ... ]
> > @@ -1026,14 +1029,107 @@ static void abx80x_unregister_driver(void)
> >  
> >  #endif /* IS_ENABLED(CONFIG_I2C) */
> >  
> > +#if IS_ENABLED(CONFIG_SPI_MASTER)
> > +
> > +static const struct regmap_config abx80x_regmap_config_spi = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = ABX8XX_SRAM_BASE + ABX8XX_SRAM_WIN_SIZE - 1,
> > +
> > +	.rd_table = &abx80x_read_table,
> > +	.wr_table = &abx80x_write_table,
> > +
> > +	.write_flag_mask = BIT(7),
> > +};
> 
> [Severity: Medium]
> Does the hardware expect MSB=1 for writes and MSB=0 for reads? Since
> .read_flag_mask is omitted, regmap defaults it to 0. If the device expects
> the opposite (MSB=1 for reads), the SPI commands could be inverted.
Everything ok here. According to the datasheet, the MSB of the register
indicates read (if 0) or write (if 1).

> 
> [ ... ]
> > +static int abx81x_spi_probe(struct spi_device *spi)
> > +{
> > +	unsigned int part = (uintptr_t)spi_get_device_match_data(spi);
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init_spi(spi, &abx80x_regmap_config_spi);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&spi->dev, "Unable to allocate regmap\n");
> > +		return -EIO;
> > +	}
> 
> [Severity: Medium]
> Should this return PTR_ERR(regmap) instead of a hardcoded -EIO? Returning
> -EIO here masks the true error code from devm_regmap_init_spi() (like -ENOMEM
> or -EPROBE_DEFER), which could disrupt probe deferral logic. (It looks like
> the existing abx80x_i2c_probe() might have the same behavior.)
> 
> [ ... ]
> 
Yes, this should return PTR_ERR(), not sure why I added -EIO here...
Will change this in v3.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8