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

[Nuno Sá]

On 2026-10-13 17:56:21-03:00, Marcelo Schmitt wrote:
> 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.
> > 
> > [ ... ]
> > 
> > [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)
> > 
> > [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.

If the datasheet has any specification about this, then it make sense to
have it. But as another patch (could even be a fix).