Re: [PATCH 4/6] iio: adc: Add AD7768 IIO Driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] The AD7768_REG_CH_STANDBY register is programmed with an incorrect contiguous bitmask for the AD7768-4 variant, preventing unused hardware channels from entering standby.
- [High] Missing or delayed validation for the `adi,data-lines-number` DT property allows a division by zero kernel panic.
- [High] Unvalidated master clock (`mclk`) frequency can result in a division by zero crash.
- [High] Missing sign-extension when reading the 24-bit CALIBBIAS (offset) and 8-bit PHASE calibration registers causes data corruption.
- [High] The SPI read function reuses the same buffer for concurrent `tx_buf` and `rx_buf` across multiple transfers in the same message, violating the DMA API and the device's SPI protocol.
- [High] Missing error rollback in `ad7768_update_scan_mode` leaves the IIO backend hardware in a partially enabled state on failure.
- [High] Hardware configuration is modified without claiming direct mode, allowing state corruption if modified while the IIO buffer is active.
- [Medium] The device will not automatically enter suspend after probe because the autosuspend timer is never started, resulting in a permanent power leak if unused.
- [Medium] Hardware reset sequence asserts and immediately deasserts the reset pin without any delay, potentially violating the minimum reset pulse width.
--

--- Patch [4]: [PATCH 4/6] iio: adc: Add AD7768 IIO Driver support ---
commit 73c0da3f81ae06a5de50f6a53d4da30b75b35a25
Author: Janani Sunil <janani.sunil@analog.com>

iio: adc: Add AD7768 IIO Driver support
    
Add support for AD7768 4/8 channel,simultaneous sampling Sigma-Delta
ADC, supporting configurable power modes, decimation filters and data
output lines

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> new file mode 100644
> index 0000000000000..f76a7081090ec
> --- /dev/null
> +++ b/drivers/iio/adc/ad7768.c
[ ... ]
> +static int ad7768_regmap_read(void *context, const void *reg_buf,
> +			      size_t reg_size, void *val_buf, size_t val_size)
> +{
[ ... ]
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->d16,
> +			.len = 2,
> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = &st->d16,
> +			.rx_buf = &st->d16,
> +			.len = 2,
> +		},
> +	};

[Severity: High]
Does this second transfer violate the DMA API by double-mapping the same
virtual pointer (&st->d16) used in the first transfer?

Also, wouldn't transmitting st->d16 again during the read phase violate
standard SPI protocol by sending the read command on the MOSI line instead
of leaving it 0x00?

Could .tx_buf be left NULL for the second transfer?

[ ... ]
> +static int ad7768_read_calib_value(struct ad7768_state *st,
> +				   unsigned int base_reg, int *val)
> +{
[ ... ]
> +	*val = (msb << 16) | (mid << 8) | lsb;
> +
> +	return 0;
> +}

[Severity: High]
Since the CALIBBIAS register stores a 24-bit two's complement offset, does
this assignment correctly handle negative offsets?

It appears the value is ORed together as an unsigned 24-bit value and assigned
without sign extension, which might cause negative offsets to be returned to
userspace as large positive integers.

Should sign_extend32() be used here?

[ ... ]
> +static int ad7768_set_clk_divs(struct ad7768_state *st,
> +			       unsigned int freq, unsigned int ch)
> +{
[ ... ]
> +	chan_per_doutx = st->chip_info->num_channels / st->datalines;
> +	if (!chan_per_doutx)
> +		return -EINVAL;
[ ... ]
> +	dclk = f_cfg.freq * AD7768_SAMPLE_SIZE * chan_per_doutx;
> +	if (dclk > mclk)
> +		return -EINVAL;
> +
> +	/* Set dclk_div to the nearest power of 2 less than the original value */
> +	dclk_div = DIV_ROUND_CLOSEST_ULL(mclk, dclk);

[Severity: High]
Is it possible for the configured mclk to be 0?

If the clock rate returns 0, dclk will also become 0. Would this trigger a
division by zero kernel panic inside the DIV_ROUND_CLOSEST_ULL() macro?

[ ... ]
> +static int ad7768_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
[ ... ]
> +	case IIO_CHAN_INFO_PHASE:
[ ... ]
> +		ret = regmap_read(st->regmap, base_reg, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;

[Severity: High]
Similar to the CALIBBIAS register, the PHASE register stores an 8-bit two's
complement value. Since regmap_read() zero-extends the 8-bit read, will this
corrupt negative phase values?

[ ... ]
> +static int ad7768_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
[ ... ]
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad7768_set_sampling_freq(indio_dev, val, chan->channel);

[Severity: High]
Is it safe to update the sampling frequency without claiming direct mode?

Since ad7768_set_sampling_freq() invokes ad7768_sync() to reset the ADC's
digital filters and SPI synchronization, could userspace writes to sysfs
attributes corrupt or desynchronize ongoing buffer captures?

Should iio_device_claim_direct_mode() be used here?

[ ... ]
> +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
[ ... ]
> +	for (c = 0; c < st->chip_info->num_channels; c++) {
> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
If an error occurs midway through this loop, returning ret immediately leaves
previously enabled backend channels active.

Does this leak backend resources and leave the hardware streaming
indefinitely, potentially breaking future captures? Would it be better to
rollback the enabled channels on failure?

[ ... ]
> +static int ad7768_parse_config(struct iio_dev *indio_dev,
> +			       struct device *dev)
> +{
[ ... ]
> +	ret = regmap_write(st->regmap, AD7768_REG_CH_STANDBY,
> +			   GENMASK(st->chip_info->num_channels - 1, 0));
> +	if (ret < 0)
> +		return ret;
> +
> +	device_for_each_child_node_scoped(dev, child) {
[ ... ]
> +		ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +					 BIT(channel), 0);

[Severity: High]
For the AD7768-4 variant, which uses physical channels 0, 1, 4, and 5, will
this contiguous mask GENMASK(3, 0) correctly power down the unused channels?
It seems this would write 1s to bits 0-3 and 0s (active) to bits 4 and 5,
potentially leaving the upper hardware channels permanently active.

Additionally, will BIT(channel) correctly clear the bits for the upper
hardware channels? If channel is 2 or 3 (representing the 3rd and 4th
channels), this would attempt to wake up non-existent physical channels while
leaving bits 4 and 5 active.

[ ... ]
> +	st->datalines = 1;
> +	ret = device_property_read_u32(&st->spi->dev, "adi,data-lines-number",
> +				       &st->datalines);
> +	if (ret)
> +		return dev_err_probe(&st->spi->dev, ret,
> +				     "Missing \"adi,data-lines-number\" property\n");
> +
> +	ad7768_set_available_sampl_freq(st);
[ ... ]
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		ret = ad7768_set_sampling_freq(indio_dev, st->sampling_freq,
> +					       indio_dev->channels[i].channel);

[Severity: High]
Could st->datalines be 0 if the user provides adi,data-lines-number = <0>
in the device tree?

Because validation of this property happens at the end of the function,
calling ad7768_set_sampling_freq() here delegates to ad7768_set_clk_divs(),
which performs st->chip_info->num_channels / st->datalines.

Would this trigger a division by zero kernel panic?

[ ... ]
> +static int ad7768_set_filter_mode(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int mode)
> +{
[ ... ]
> +	ret = regmap_update_bits(st->regmap,
> +				 AD7768_REG_CH_MODE(st->chn_mode[chan->address]),
> +				 AD7768_CH_MODE_FILTER_TYPE_MSK,
> +				 AD7768_CH_MODE_FILTER_TYPE_MODE(mode));
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad7768_sync(st);
> +}

[Severity: High]
Similar to ad7768_write_raw(), is it safe to update the filter mode and sync
the hardware without claiming direct mode using
iio_device_claim_direct_mode()?

[ ... ]
> +static int ad7768_reset(struct ad7768_state *st)
> +{
[ ... ]
> +	if (reset_ctrl) {
> +		ret = reset_control_assert(reset_ctrl);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_control_deassert(reset_ctrl);

[Severity: Medium]
Is a delay required between asserting and deasserting the reset line?

Executing reset_control_deassert() immediately after reset_control_assert()
might result in an extremely brief reset pulse. Could this fail to reset the
hardware properly on fast platforms?

[ ... ]
> +static int ad7768_probe(struct spi_device *spi)
> +{
[ ... ]
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +
> +	ret = devm_pm_runtime_enable(dev);

[Severity: Medium]
Does this properly start the autosuspend timer after probe?

Because the usage counter is 0 and no idle check is manually queued, the core
never starts the autosuspend timer, leaving the ADC fully powered on
indefinitely until userspace triggers a pm_runtime_get()/put() cycle.

Should pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() be called
before exiting probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-ad7768-driver-v1-0-44e1194fd96a@analog.com?part=4