Re: [PATCH v1 09/13] iio: adc: ad4134: Support SPI 4-wire mode

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Sep 02, 2026 at 02:24:25PM -0300, Marcelo Schmitt 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
> optimal for high-throughput data acquisition.
> 
> Extend AD4134 support to enable interfacing in SPI 4-wire configuration.

...

> struct ad4134_state {

I hope on each stage you run `pahole` to confirm that this is the optimal
layout.

>  	struct gpio_desc *odr_gpio;
>  	int refin_mv;
>  	bool crc_en;
> +	enum ad4134_spi_mode spi_mode;
> +	struct mux_state *mux_st[2];
>  	/*
>  	 * Synchronize access to members the of driver state, and ensure
>  	 * atomicity of consecutive register access operations.
>  	 */
>  	struct mutex lock;
> +	/*
> +	 * Ensure atomicity of access mode switch operations.
> +	 */
> +	struct mutex access_mode_lock;

>  };

...

> +static int ad4134_set_register_access(struct ad4134_state *st)
> +{
> +	int ret;
> +
> +	guard(mutex)(&st->access_mode_lock);

+ blank line.

> +	st->spi->mode = SPI_MODE_0;
> +	ret = spi_setup(st->spi);
> +	if (ret)
> +		return ret;
> +
> +	ret = mux_state_deselect(st->mux_st[AD4134_DOUT0_INPUT]);
> +	if (ret)
> +		dev_err(&st->spi->dev, "error on DOUT0 deselect: %d\n", ret);

> +	ret = mux_state_try_select(st->mux_st[AD4134_SDO_INPUT]);
> +	if (ret && ret != -EBUSY)
> +		return ret;

Reading this without a comment about EBUSY is difficult. Interpreting BUSY as
occupied mux channel, why do we return success?

> +	return 0;
> +}

...

> +static int ad4134_set_sample_access(struct ad4134_state *st)
> +{

	struct device *dev = &st->spi->dev;

> +	int ret, ret2;

I would go with

	int mux_state_ret;
	int ret;

> +	guard(mutex)(&st->access_mode_lock);
> +	ret = mux_state_deselect(st->mux_st[AD4134_SDO_INPUT]);
> +	if (ret)
> +		dev_err(&st->spi->dev, "error on SDO deselect: %d\n", ret);
> +
> +	ret = mux_state_try_select(st->mux_st[AD4134_DOUT0_INPUT]);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "error on DOUT0 select: %d\n", ret);
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +	}
> +
> +	/*
> +	 * Data output on the DOUT lines is sampled on the falling edge
> +	 * (SPI mode 1).
> +	 */
> +	st->spi->mode = SPI_MODE_1;
> +	ret = spi_setup(st->spi);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "failed to setup SPI mode 1: %d\n", ret);
> +		ret2 = mux_state_deselect(st->mux_st[AD4134_DOUT0_INPUT]);
> +		if (ret2)
> +			dev_err(&st->spi->dev, "error on DOUT0 deselect: %d\n", ret2);
> +
> +		ret2 = mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +		if (ret2)
> +			dev_err(&st->spi->dev, "error on SDO select: %d\n", ret2);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}

...

>  static int ad4134_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct ad4134_state *st = context;
> +	int ret, ret2;
>  
> -	if (reg >= AD4134_CH_VREG(0))
> -		return ad4134_data_read(st, reg, val);
> +	if (reg >= AD4134_CH_VREG(0)) {
> +		if (st->spi_mode == AD4134_SPI_MODE_4_WIRE) {
> +			ret = ad4134_set_sample_access(st);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = ad4134_data_read(st, reg, val);
> +
> +		if (st->spi_mode == AD4134_SPI_MODE_4_WIRE) {
> +			ret2 = ad4134_set_register_access(st);
> +			if (ret2)
> +				dev_err(&st->spi->dev, "access mode error: %d\n", ret2);
> +		}

I would go with duplication of _data_read() call but better flow

		if (...) {
			_set_sample_()
			ret = _data_read();
			_set_register_()
		} else {
			ret = _data_read();
		}

> +		return ret;
> +	}

...

> +	ret = device_property_match_property_string(dev, "adi,spi-mode",
> +						    ad4134_spi_modes,
> +						    ARRAY_SIZE(ad4134_spi_modes));
> +	/* Default to "no-cs" mode if adi,spi-mode is not specified */
> +	if (ret == -EINVAL)

No, use device_property_present() instead.

> +		st->spi_mode = AD4134_SPI_MODE_NO_CS;
> +	else if (ret < 0)
>  		return dev_err_probe(dev, ret,
> -				     "failed to setup minimum I/O mode\n");
> +				     "getting adi,spi-mode property failed\n");
> +	else
> +		st->spi_mode = ret;

-- 
With Best Regards,
Andy Shevchenko