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] [day] [month] [year] [list]

Message-ID: <20250131181848.00003c67@huawei.com>
Date: Fri, 31 Jan 2025 18:18:48 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Esteban Blanc <eblanc@...libre.com>
CC: 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

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.

> @@ -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!

> +	}