Re: [PATCH v3 3/6] iio: adc: ad4030: add averaging support

[Jonathan Cameron]

On Thu, 30 Jan 2025 12:08:27 +0100
Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:

> This add support for the averaging mode of AD4030 using oversampling IIO
> attribute
> 
> Signed-off-by: Esteban Blanc <eblanc@xxxxxxxxxxxx>
A couple of comments inline. The one about the gpio wiggling
is just me venting at silly hardware, so feel free to ignore that!
Other one is a trivial 'nice to have' for formatting.

Jonathan

>  
> +static const int ad4030_average_modes[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
> +	32768, 65536
> +};
Groups of 8 often best option for lists like this. Make it easy to see how
many there are.

> @@ -498,9 +538,11 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
>  static int ad4030_conversion(struct iio_dev *indio_dev)
>  {
>  	struct ad4030_state *st = iio_priv(indio_dev);
> -	const struct iio_scan_type scan_type = indio_dev->channels->scan_type;
> -	unsigned char diff_realbytes = BITS_TO_BYTES(scan_type.realbits);
> +	unsigned char diff_realbytes =
> +		BITS_TO_BYTES(st->current_scan_type->realbits);
>  	unsigned int bytes_to_read;
> +	unsigned long cnv_nb = BIT(st->avg_log2);
> +	unsigned int i;
>  	int ret;
>  
>  	/* Number of bytes for one differential channel */
> @@ -511,10 +553,12 @@ static int ad4030_conversion(struct iio_dev *indio_dev)
>  	/* Mulitiply by the number of hardware channels */
>  	bytes_to_read *= st->chip->num_voltage_inputs;
>  
> -	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> -	ndelay(AD4030_TCNVH_NS);
> -	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> -	ndelay(st->chip->tcyc_ns);
> +	for (i = 0; i < cnv_nb; i++) {
> +		gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +		ndelay(AD4030_TCNVH_NS);
> +		gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +		ndelay(st->chip->tcyc_ns);
Hmm. This is a bit nasty. To actually use this in anger
and get decent performance I guess a PWM with appropriate
functionality to do the right length pull train is the
way to go.

oh well, nothing wrong with this as a solution beyond it
likely taking much longer than needed!

> +	}