Re: [PATCH v4 12/13] iio: adc: ad4134: Support high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ad4134: ODR PWM is left running on probe failure and device unbind
- [High] iio: adc: ad4134: debugfs_reg_access corrupts active SPI offload capture
- [Low] ad4134: Incorrect macro HZ_PER_GHZ used for nanosecond time math
--

commit 646edf6c83770fad0c88592b752978f3561b3e9e
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
Subject: iio: adc: ad4134: Support high-speed data capture

This commit implements SPI transfer offloading for the AD4134 ADC. It
enables faster data acquisition at sample rates up to 1.496 MSPS.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index debc2ff027051..aa8abe5715c4b 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -517,6 +522,92 @@ ...
> +static int ad4134_update_conversion_rate(struct ad4134_state *st,
> +					 unsigned int freq_Hz)
> +{
[ ... ]
> +	/*
> +	 * Configure SPI offload PWM trigger.
> +	 * For gated DCLK, the minimum required time between ODR rising edge
[ ... ]
> +	 */
> +	offload_period_ns = odr_wf.period_length_ns;
> +	config.periodic.frequency_hz = DIV_ROUND_UP_ULL(HZ_PER_GHZ, offload_period_ns);

[Severity: Low]
This isn't a bug, but should this use NSEC_PER_SEC instead of HZ_PER_GHZ?

While both macros are numerically identical, the calculation here is dividing
by a period in nanoseconds to get a frequency in Hertz, so NSEC_PER_SEC
seems to be the semantically correct unit scaling factor to use here.

[ ... ]
> @@ -606,6 +787,166 @@ ...
> +static const struct iio_info ad4134_offload_info = {
> +	.read_raw = ad4134_read_raw,
> +	.read_avail = ad4134_read_avail,
> +	.write_raw = ad4134_write_raw,
> +	.debugfs_reg_access = ad4134_debugfs_reg_access,

[Severity: High]
Can this corrupt an ongoing SPI offload capture?

It looks like ad4134_debugfs_reg_access() issues regmap_read() and
regmap_write() operations without calling iio_device_claim_direct_mode().
If a privileged user accesses direct_reg_access via debugfs while an
offload engine is actively streaming data, this could dynamically alter
the SPI controller mode to SPI_MODE_0 and shift the pin mux.

Since ad4134_release_reg_access() does not restore the SPI mode to
SPI_MODE_1, would this permanently corrupt the capture stream until it
is restarted?

> +};
[ ... ]
> +static int ad4134_offload_setup(struct iio_dev *indio_dev, struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	ret = ad4134_pwm_get(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4134_offload_buffer_setup(indio_dev, st->spi);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Start with a sampling rate slower than 374 kSPS because that is the
> +	 * maximum rate supported with wideband filter (default on power up).
> +	 */
> +	st->odr_hz = 250 * HZ_PER_KHZ;
> +	ret = ad4134_update_conversion_rate(st, st->odr_hz);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set sampling freq\n");

[Severity: High]
Will the ODR PWM be left running if probe fails later or the device is
unbound?

The call to ad4134_update_conversion_rate() actively starts the PWM
generation by calling pwm_set_waveform_might_sleep(). The PWM is acquired
via devm_pwm_get() earlier in ad4134_offload_setup(), so devres will call
pwm_put() during cleanup. However, pwm_put() only frees the software
resource and does not stop the physical PWM output.

Should there be a devm_add_action_or_reset() handler registered to
explicitly disable the PWM or set the duty cycle to zero on teardown?

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