Re: [PATCH v4 05/13] iio: adc: ad4134: Run shorter transfers when CRC is disabled

[Marcelo Schmitt]

On 10/06, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] iio: adc: ad4134: broken SPI read CRC calculation and ignored failures
> - [Medium] iio: adc: missing delay after reset deassertion in probe
> --
> 
> commit 5284b96efc125640c497213951d3fabe580894ec
> Author: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> 
> iio: adc: ad4134: Run shorter transfers when CRC is disabled
> 
> This commit conditionally reduces the SPI transfer length when the AD4134
> CRC feature is disabled. It introduces a crc_en flag in the driver state
> to skip the CRC validation check and send fewer bytes when CRC is inactive.
> 
> > diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> > index a0113fff4b715..85fa16321d744 100644
> > --- a/drivers/iio/adc/ad4134.c
> > +++ b/drivers/iio/adc/ad4134.c
> [ ... ]
> > @@ -232,7 +233,7 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
> >  	*val = st->rx_buf[1];
> >  
> >  	/* Check CRC */
> > -	if (st->rx_buf[2] != st->tx_buf[2])
> > +	if (st->crc_en && st->rx_buf[2] != st->tx_buf[2])
> >  		dev_dbg(&st->spi->dev, "reg read CRC check failed\n");
> >  
> >  	return 0;
> 
> [Severity: Medium]
> Does this CRC validation logic reliably verify the read data?

It would require mid-transfer data CRC calculation to properly implement AD4134
CRC feature. That would probably require changes to the SPI core or even more
unusual hacks. Fixing the CRC is not part of this series and I don't want to
extend it further.

...
> > @@ -313,6 +314,7 @@ static int ad4134_min_io_mode_setup(struct ad4134_state *st)
> >  	struct device *dev = &st->spi->dev;
> >  	int ret;
> >  
> > +	st->crc_en = true; /* In minimum I/O mode CRC cannot be disabled */
> >  	st->odr_gpio = devm_gpiod_get(dev, "odr", GPIOD_OUT_LOW);
> 
> [Severity: Medium]
> Does the AD4134 device require a stabilization delay before interacting with it
> over SPI?
> 
> In ad4134_probe(), devm_reset_control_get_optional_exclusive_deasserted() is
> called to deassert the reset line. Execution then proceeds directly to regmap
> initialization and SPI transfers without any hardware stabilization delay:
> 
> 	rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> 	if (IS_ERR(rst))
> 		return dev_err_probe(dev, PTR_ERR(rst),
> 				     "failed to get and deassert reset\n");
> 
> 	crc8_populate_msb(ad4134_spi_crc_table, AD4134_SPI_CRC_POLYNOM);
> 
> 	ret = devm_mutex_init(dev, &st->sync_lock);
> 	if (ret)
> 		return ret;
> 
> 	st->regmap = devm_regmap_init(dev, NULL, st, &ad4134_regmap_config);
> 
> Without a delay, could the first SPI transactions during probe drop or return
> invalid data, causing initialization to fail?

Has never been an issue during tests. Though, I guess it's easier to add
fsleep(AD4134_RESET_TIME_US) than to keep replying to bots.