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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cqklmqc23dw2t2wyyl73wjamj5judqzq7yqtjhduokjljgm6iz@isurygto66as>
Date: Mon, 3 Feb 2025 10:54:05 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Esteban Blanc <eblanc@...libre.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Nuno Sá <nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 3/6] iio: adc: ad4030: add averaging support

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@...libre.com> wrote:
> 
> > This add support for the averaging mode of AD4030 using oversampling IIO
> > attribute
> > 
> > Signed-off-by: Esteban Blanc <eblanc@...libre.com>
> 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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)