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

[sashiko-bot@kernel.org]

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

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?

In ad4134_register_read(), the master's CRC (st->tx_buf[2]) is computed using
the command byte and a dummy payload (0x00). However, the slave returns its CRC
(st->rx_buf[2]) computed over the command byte and the actual register value
being read (st->rx_buf[1]). Comparing st->rx_buf[2] != st->tx_buf[2] will
spuriously fail for any read where the register value is non-zero.

Additionally, both the read and write paths merely log a dev_dbg() message on
CRC mismatch and return success (0) instead of returning an error code (such
as -EIO). Can this allow corrupted data to be used by the caller, rendering
the CRC feature ineffective for data integrity?

[ ... ]
> @@ -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?

The driver defines AD4134_RESET_TIME_US as 10 seconds (10 * USEC_PER_SEC) but
never uses it to invoke any sleep function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791309733.git.marcelo.schmitt@analog.com?part=5