Re: [PATCH v3 2/4] iio: adc: ad7380: fix oversampling formula

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 24 Oct 2024 10:16:57 +0200
Julien Stephan <jstephan@baylibre.com> wrote:

> The formula in the datasheet for oversampling time conversion seems to
> be valid when device is at full speed using the maximum number of SDO
> lines. The driver currently support only 1 SDO line. The formula will

The 'new or corrected' formula I assume not the one in the datasheet?

> produce larger delays than what is currently set, but some devices
> actually require it.

This series is I think backed up behind the fixes chasing through Greg's
trees so I'm being a little fussier than I might otherwise be (when I'd
maybe just change stuff whilst applying and ask you to check the result!)

Code here looks fine, just a comment on the comment dropped.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index fb728570debe6432d5f991595cb35e9e7af8b740..d57e17f38925da5fb7c8a0a2320a21474ba04b37 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -77,6 +77,12 @@
>  #define T_CONVERT_X_NS 500		/* xth conversion start time (oversampling) */
>  #define T_POWERUP_US 5000		/* Power up */
>  
> +/*
> + * AD738x support several SDO lines to increase throughput, but driver currently
> + * supports only 1 SDO line (standard SPI transaction)
> + */
> +#define AD7380_NUM_SDO_LINES		1
> +
>  struct ad7380_timing_specs {
>  	const unsigned int t_csh_ns;	/* CS minimum high time */
>  };
> @@ -649,7 +655,8 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
>  
>  	if (st->oversampling_ratio > 1)
>  		xfer.delay.value = T_CONVERT_0_NS +
> -			T_CONVERT_X_NS * (st->oversampling_ratio - 1);
> +			T_CONVERT_X_NS * (st->oversampling_ratio - 1) *
> +			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
>  
>  	return spi_sync_transfer(st->spi, &xfer, 1);
>  }
> @@ -667,12 +674,13 @@ static void ad7380_update_xfers(struct ad7380_state *st,
>  
>  	/*
>  	 * In the case of oversampling, conversion time is higher than in normal
> -	 * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
> -	 * the maximum value for simplicity for now.
> +	 * mode: t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS*(x - 1)*num_channel/number_of_sdo_lines
> +	 * where x is the oversampling ratio

Could drop the comment as the code is pretty clear.  Is the statement about T_CONVERT_X_NS
being the max value of supported chips worth keeping?

>  	 */
>  	if (st->oversampling_ratio > 1)
>  		t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
> -			(st->oversampling_ratio - 1);
> +			(st->oversampling_ratio - 1) *
> +			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
>  
>  	if (st->seq) {
>  		xfer[0].delay.value = xfer[1].delay.value = t_convert;
> @@ -1021,7 +1029,8 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
>  	/* SPI 1-wire mode */
>  	return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
>  				  AD7380_CONFIG2_SDO,
> -				  FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> +				  FIELD_PREP(AD7380_CONFIG2_SDO,
> +					     AD7380_NUM_SDO_LINES));
>  }
>  
>  static int ad7380_probe(struct spi_device *spi)
>