Re: [PATCH v5 12/15] iio: adc: ad4134: Support SPI 4-wire mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] iio: adc: ad4134: race with trigger handler in ad4134_buffer_setup_ops
- [Medium] iio: adc: ad4134: missing DOUT0 format config and cs_off in 4-wire mode
- [Medium] iio: adc: ad4134: spi_setup() ordering and mode desync in claim helpers
--

commit 8eb548670148a37e8ad1ee608a1ea7bc67bedb0c
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Support SPI 4-wire mode

This commit extends the AD4134 ADC driver to support SPI 4-wire mode in
addition to minimum I/O mode by switching SPI clock phase and controlling
an external SDO/DOUT0 multiplexer between register and sample accesses.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index eaf5bb55f25ba..6b05e40f7aeb7 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -219,6 +237,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = {
>  	.n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range),
>  };
>  
> +/*
> + * When operating in modes other than minimum I/O mode, AD4134 register data on
> + * the SDO line is sampled on SCLK rising edge (SPI mode 0). In addition to
> + * that, when AD4134 SDO and DOUT0 pins are multiplexed by hardware OUTSIDE
> + * AD4134 SILICON, the multiplexer state must be set to route SDO to the SPI
> + * controller. With both SPI mode and multiplexer state properly set, register
> + * access runs as usual. Successful calls to this function must be balanced with
> + * calls to ad4134_release_reg_access() so to release the mux state lock. See
> + * AD4134 IIO documentation for details.
> + */
> +static int ad4134_claim_reg_access(struct ad4134_state *st)
> +{
> +	unsigned int cur_mode = st->spi->mode & SPI_MODE_X_MASK;
> +	int ret;
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && cur_mode != SPI_MODE_0) {
> +		st->spi->mode &= ~SPI_MODE_X_MASK;
> +		st->spi->mode |= SPI_MODE_0;
> +		ret = spi_setup(st->spi);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}

[Severity: Medium]
In ad4134_claim_reg_access() and ad4134_claim_sample_access(), can
mutating st->spi->mode and calling spi_setup() before mux_state_select()
leave the SPI mode out of sync or reconfigure the controller during an
active capture?

If spi_setup(st->spi) fails, st->spi->mode is not restored to cur_mode. On
the next call to the same claim helper, cur_mode already matches the
target mode, so spi_setup() is skipped while the SPI controller remains
configured in the previous mode.

Similarly, because spi_setup() runs before mux_state_select() acquires the
mux lock, if mux_state_select() fails (for example with -EINTR while
waiting in down_killable()), st->spi->mode and the SPI controller remain in
the new mode on an error return. In addition, if a register read occurs via
regmap debugfs while buffered capture holds the mux lock in SPI_MODE_1,
ad4134_claim_reg_access() will call spi_setup() to switch the controller to
SPI_MODE_0 before blocking on mux_state_select().

Should mux_state_select() be called before changing st->spi->mode and
calling spi_setup(), with st->spi->mode restored and the mux deselected if
spi_setup() fails?

[ ... ]

> @@ -261,7 +367,11 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  {
>  	unsigned int i;
>  	u32 sample = 0;
> -	int ret;
> +	int access_ret, ret;
> +
> +	ret = ad4134_claim_sample_access(st);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * To be able to read data from all 4 channels through a single line, we
> @@ -277,7 +387,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  		ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf,
>  					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
>  		if (ret)
> -			return ret;
> +			goto out_data_read_release;

[ ... ]

> @@ -372,6 +496,21 @@ static irqreturn_t ad4134_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ad4134_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	return ad4134_claim_sample_access(iio_priv(indio_dev));
> +}
> +
> +static int ad4134_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	return ad4134_release_sample_access(iio_priv(indio_dev));
> +}
> +
> +static const struct iio_buffer_setup_ops ad4134_buffer_setup_ops = {
> +	.postenable = &ad4134_buffer_postenable,
> +	.predisable = &ad4134_buffer_predisable,
> +};

[Severity: Medium]
Can using .postenable and .predisable in ad4134_buffer_setup_ops race with
ad4134_trigger_handler() in 4-wire mode?

In drivers/iio/industrialio-buffer.c, iio_enable_buffers() attaches the
trigger pollfunc before running .postenable:

	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
		ret = iio_trigger_attach_poll_func(indio_dev->trig,
						   indio_dev->pollfunc);
		if (ret)
			goto err_disable_buffers;
	}

	if (indio_dev->setup_ops->postenable) {
		ret = indio_dev->setup_ops->postenable(indio_dev);

Because iio_trigger_attach_poll_func() requests the threaded IRQ and enables
the external trigger, ad4134_trigger_handler() can start running
spi_sync_transfer() before or concurrently with ad4134_buffer_postenable()
calling ad4134_claim_sample_access() to update st->spi->mode, call
spi_setup(), and select st->mux_st[AD4134_DOUT0_INPUT].

Symmetrically, iio_disable_buffers() runs .predisable before detaching the
trigger pollfunc:

	if (indio_dev->setup_ops->predisable) {
		ret2 = indio_dev->setup_ops->predisable(indio_dev);
		if (ret2 && !ret)
			ret = ret2;
	}

	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
		iio_trigger_detach_poll_func(indio_dev->trig,
					     indio_dev->pollfunc);
	}

This allows ad4134_buffer_predisable() to deselect the DOUT0 mux state
before iio_trigger_detach_poll_func() disables the trigger and frees the
pollfunc IRQ.

(Note also that in this commit, indio_dev->setup_ops is overwritten with
NULL by the subsequent devm_iio_triggered_buffer_setup(..., NULL) call in
ad4134_probe() until a later commit in the series passes
&ad4134_buffer_setup_ops to devm_iio_triggered_buffer_setup().)

Should ad4134_buffer_setup_ops use .preenable and .postdisable instead of
.postenable and .predisable?

[ ... ]

> @@ -591,10 +725,61 @@ static int ad4134_probe(struct spi_device *spi)
>  		return dev_err_probe(dev, PTR_ERR(st->regmap),
>  				     "failed to initialize regmap");
>  
> -	ret = ad4134_min_io_mode_setup(st);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "failed to setup minimum I/O mode\n");
> +	if (device_property_present(dev, "adi,spi-mode")) {
> +		ret = device_property_match_property_string(dev, "adi,spi-mode",
> +							    ad4134_spi_modes,
> +							    ARRAY_SIZE(ad4134_spi_modes));
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "getting adi,spi-mode property failed\n");
> +
> +		st->spi_mode = ret;
> +	} else {
> +		/* Default to no-cs mode if adi,spi-mode is not specified */
> +		st->spi_mode = AD4134_SPI_MODE_NO_CS;
> +	}
> +
> +	if (st->spi_mode == AD4134_SPI_MODE_NO_CS) {
> +		/*
> +		 * The support hardware for AD4134 may have a multiplexer for
> +		 * selecting between AD4134 SDO and AD4134 DOUT0. If that mux
> +		 * is set but the user still wants to run AD4134 in minimum I/O
> +		 * mode, then DOUT0 is not used and the multiplexer OUTSIDE OF
> +		 * AD4134 SILICON must be set to select AD4134 SDO. See AD4134
> +		 * IIO documentation for details.
> +		 */
> +		st->mux_st[AD4134_SDO_INPUT] =
> +			devm_mux_state_get_optional_selected(dev, "reg_access");
> +		if (IS_ERR(st->mux_st[AD4134_SDO_INPUT]))
> +			return dev_err_probe(dev, PTR_ERR(st->mux_st[AD4134_SDO_INPUT]),
> +					     "failed to get reg_access mux-state\n");
> +
> +		ret = ad4134_min_io_mode_setup(st);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup minimum I/O mode\n");
> +	} else {
> +		/*
> +		 * This controls a multiplexer OUTSIDE OF AD4134 SILICON.
> +		 * See AD4134 IIO documentation for details.
> +		 */
> +		st->mux_st[AD4134_SDO_INPUT] = devm_mux_state_get(dev, "reg_access");
> +		if (IS_ERR(st->mux_st[AD4134_SDO_INPUT]))
> +			return dev_err_probe(dev, PTR_ERR(st->mux_st[AD4134_SDO_INPUT]),
> +					     "failed to get reg_access mux-state\n");
> +
> +		st->mux_st[AD4134_DOUT0_INPUT] = devm_mux_state_get(dev, "data_read");
> +		if (IS_ERR(st->mux_st[AD4134_DOUT0_INPUT]))
> +			return dev_err_probe(dev, PTR_ERR(st->mux_st[AD4134_DOUT0_INPUT]),
> +					     "failed to get data_read mux-state\n");
> +
> +		indio_dev->setup_ops = &ad4134_buffer_setup_ops;
> +	}

[Severity: Medium]
When st->spi_mode is AD4134_SPI_MODE_4_WIRE (and without SPI offload in the
later high-speed data capture commit), how does sample readout work in
ad4134_data_read() and ad4134_trigger_handler()?

Both ad4134_data_read() and ad4134_trigger_handler() read all 4 channels
sequentially over DOUT0, which requires AD4134_DIG_IF_CFG_REG to be set to
AD4134_DATA_FORMAT_SINGLE_CH_MODE. However, that register update is only
performed inside ad4134_min_io_mode_setup() when st->spi_mode is
AD4134_SPI_MODE_NO_CS (and in ad4134_offload_setup() for 1-lane offload in
the later commit), leaving AD4134_DIG_IF_CFG_REG unconfigured in non-offload
4-wire mode.

In addition, while ad4134_trigger_handler() sets .cs_off = true on its
spi_transfer, ad4134_data_read() uses spi_write_then_read(), which asserts
CS during each 3-byte read while transmitting 0x00 on SDI. In 4-wire mode,
CS is wired to the register SPI interface and st->crc_en is false, so won't
asserting CS while clocking SCLK perform an unintended write to
AD4134_IFACE_CONFIG_A_REG (0x00)?

Should AD4134_DIG_IF_CFG_REG also be configured for single-channel DOUT0
output in 4-wire mode, and should ad4134_data_read() use a spi_transfer
with .cs_off = true?

> +
> +	st->odr_gpio = devm_gpiod_get(dev, "odr", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->odr_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->odr_gpio),
> +				     "failed to get ODR GPIO\n");
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,

[ ... ]

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