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

[Jonathan Cameron]

> 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.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
There is a lot of sashiko feedback on this one. Make sure
to take a close look.
https://sashiko.dev/#/patchset/cover.1789494473.git.marcelo.schmitt%40analog.com

>

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 0b6843bf8a9e..cc6bc325f6ee 100644
> --- a/drivers/iio/adc/ad4134.c

>  enum ad4134_filter_type {
>  	AD4134_WIDEBAND,
>  	AD4134_SINC6,
> @@ -155,6 +171,12 @@ struct ad4134_state {
>  	 * atomicity of consecutive register access operations.
>  	 */
>  	struct mutex lock;
> +	/*
> +	 * Ensure atomicity of access mode switch operations.
> +	 */

One line comment seems like enough.

> +	struct mutex access_mode_lock;
> +	struct mux_state *mux_st[2]; /* For external multiplexer control */
> +	enum ad4134_spi_mode spi_mode;
>  	int refin_mv;
>  	bool crc_en;
>  	/*
> @@ -230,6 +252,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = {
>  	.n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range),
>  };
>  
> +/*
> + * This function controls a multiplexer OUTSIDE OF AD4134 SILICON.
> + * When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the
> + * multiplexer state to route SDO to the SPI controller. See AD4134 IIO
> + * documentation for details.
> + */
> +static int ad4134_set_register_access(struct ad4134_state *st)
> +{
> +	int ret;
> +
> +	guard(mutex)(&st->access_mode_lock);
> +
> +	st->spi->mode = SPI_MODE_0;

Oh goody we are flipping the spi bus type and that is not
describable in firmware.

> +	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);
> +
> +	/*
> +	 * Try to update the multiplexer state to route SDO to the SPI
> +	 * controller. If the SDO mux state is BUSY (i.e. locked), that means
> +	 * the desired state is already selected and we should not block here.
> +	 */

I'm confused whether this is an expected path or not. Is there
some race after the deselect above? If so call out how that
occurs.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>