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

[Uwe Kleine-König]

Hello,

On Fri, Jan 31, 2025 at 06:18:48PM +0000, Jonathan Cameron wrote:
> 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.

I personally also like adding the indexes explicitly. So I would write
this as something like:

	static const int ad4030_average_modes[] = {
		/* [ 0] */ 1, 2, 4, 8, 16, 32, 64, 128,
		/* [ 8] */ 256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
		/* [16] */ 65536,
	};

Or (being unsure this is long enough to make this useful) just:

	/* [i] = 2 ** i for i in [0 ... 16] */
	static const int ad4030_average_modes[] = {
		1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
		32768, 65536
	};

> > -	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.

I think the imx27 PWM can be made to do that, but that's not actually
easy. (It has a FIFO for duty_cycle values of length 4 and always
consumes the next entry at the end of a period if the FIFO is non-empty,
so either you have to hit the right timing when filling the FIFO (but
then you can just do it generically for all PWMs) or you have to write
cnv_nb times the a non-zero value and then a zero. But with cnv_nb > 4
that's also non-trivial. Though it might support DMA for that, not
sure.) I'm not aware of other supported PWM hardware that has such a
feature (but I might miss one or the other). So I don't think this is
something valuable and useful to add to the hardware abstraction.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature