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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 16 Oct 2026 22:59:56 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> AD4134 devices can be wired in a few different ways. So far, only minimum
> I/O mode was supported. While minimum I/O mode allows interfacing with
> AD4134 with a reduced number of wires, that wiring configuration is not
> suitable for high-throughput data acquisition.
> 
> Extend AD4134 support to enable interfacing in SPI 4-wire configuration.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

I think a bit of ACQUIRE() magic sprinkled in here to handle
the claim / release pairs would make this more readable.

Otherwise, just a few little style suggestions.

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

> +
> +/*
> + * Must be balanced with successful calls to ad4134_claim_reg_access().

If you follow the ACQUIRE() suggestion below, probably don't need
this comment.

> + */
> +static int ad4134_release_reg_access(struct ad4134_state *st)
> +{
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_deselect(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}


>  static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
> @@ -301,14 +416,18 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
>  		.len = st->crc_en ? AD4134_SPI_MAX_XFER_LEN : 2,
>  	};
>  	unsigned int inst;
> -	int ret;
> +	int access_ret, ret;
> +
> +	ret = ad4134_claim_reg_access(st);
> +	if (ret)
> +		return ret;
>  
>  	inst = AD4134_REG_READ_MASK | reg;
>  	ad4134_prepare_spi_tx_buf(inst, 0, st->tx_buf);
>  
>  	ret = spi_sync_transfer(st->spi, &xfer, 1);
>  	if (ret)
> -		return ret;
> +		goto out_read_release;
>  
>  	*val = st->rx_buf[1];
>  
> @@ -316,7 +435,12 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
>  	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;
> +out_read_release:
> +	access_ret = ad4134_release_reg_access(st);
> +	if (access_ret)
> +		dev_err(&st->spi->dev, "error on access release: %d\n", access_ret);

If you are going to not return access_ret anyway (which is common simplifying
approach for cases like this where we are stuck anyway), then how about wrapping
all this in an ACQUIRE() and letting the cleanup.h magic call the release for you?

> +
> +	return ret;
>  }

>  static int ad4134_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long info)
> @@ -440,11 +579,6 @@ static int ad4134_min_io_mode_setup(struct ad4134_state *st)
>  	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);
> -	if (IS_ERR(st->odr_gpio))
> -		return dev_err_probe(dev, PTR_ERR(st->odr_gpio),
> -				     "failed to get ODR GPIO\n");
> -
>  	ret = regmap_update_bits(st->regmap, AD4134_DIG_IF_CFG_REG,
>  				 AD4134_DIF_IF_CFG_FORMAT_MASK,
>  				 FIELD_PREP(AD4134_DIF_IF_CFG_FORMAT_MASK,
> @@ -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");

Maybe use a local variable for the return of devm_mux_state_get_optional_selected
and only sets st->mux_st[] after we know it's not an error. Should make this
a little more readable.

Similar applies in other places.

> +
> +		ret = ad4134_min_io_mode_setup(st);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup minimum I/O mode\n");

When it's 80-90 chars, feel free to put dev_err_probe() on one line.

> +	} 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;
> +	}
> +
> +	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,