linux-kernel - Re: [PATCH 3/5] ad7380: add support for single-ended parts

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: <20240728172309.13155e1b@jic23-huawei>
Date: Sun, 28 Jul 2024 17:23:09 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Julien Stephan <jstephan@...libre.com>
Cc: Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, Lars-Peter
 Clausen <lars@...afoo.de>, 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 3/5] ad7380: add support for single-ended parts

On Fri, 26 Jul 2024 17:20:08 +0200
Julien Stephan <jstephan@...libre.com> wrote:

> Adding ad7386/7/8 (16/14/12 bits) unsigned, dual simultaneous sampling,
> single-ended compatible parts, and the corresponding ad7386-4/7-4/8-4
> 4 channels.
> 
> These parts have a 2:1 multiplexer in front of each ADC. They also include
> additional configuration registers that allow for either manual selection
> or automatic switching (sequencer mode), of the multiplexer inputs.
> This commit focus on integrating manual selection. Sequencer mode will
> be implemented later.
> 
> From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> export 4 channels and ad7386-4/7-4/8-4 export 8 channels.
> 
> Inputs AinX0 of multiplexers correspond to the first half of IIO channels
> (i.e 0-1 or 0-3) and inputs AinX1 correspond to second half (i.e 2-3 or
> 4-7). Example for AD7386/7/8 (2 channels parts):
> 
>           IIO   | AD7386/7/8
>                 |         +----------------------------
>                 |         |     _____        ______
>                 |         |    |     |      |      |
>        voltage0 | AinA0 --|--->|     |      |      |
>                 |         |    | mux |----->| ADCA |---
>        voltage2 | AinA1 --|--->|     |      |      |
>                 |         |    |_____|      |_____ |
>                 |         |     _____        ______
>                 |         |    |     |      |      |
>        voltage1 | AinB0 --|--->|     |      |      |
>                 |         |    | mux |----->| ADCB |---
>        voltage3 | AinB1 --|--->|     |      |      |
>                 |         |    |_____|      |______|
>                 |         |
>                 |         +----------------------------
> 
> When switching channel, the ADC require an additional settling time.
> According to the datasheet, data is valid on the third CS low. We already
> have an extra toggle before each read (either direct reads or buffered
> reads) to sample correct data, so we just add a single CS toggle at the
> end of the register write.
> 
> Signed-off-by: Julien Stephan <jstephan@...libre.com>
Hi Julien

LGTM - only one trivial comment inline.
If nothing else comes up I can change that whilst applying.
I won't be applying today however given this is a new series and has only been
on the list since Friday.

...

> @@ -92,8 +96,24 @@ enum {
>  	AD7380_SCAN_TYPE_RESOLUTION_BOOST,
>  };
>  
> -/* Extended scan types for 14-bit chips. */
> -static const struct iio_scan_type ad7380_scan_type_14[] = {
> +/* Extended scan types for 12-bit unsigned chips. */
> +static const struct iio_scan_type ad7380_scan_type_12_u[] = {
> +	[AD7380_SCAN_TYPE_NORMAL] = {
> +		.sign = 'u',
> +		.realbits = 12,
> +		.storagebits = 16,
> +		.endianness = IIO_CPU

Add trailing commas.  In theory we might expand this structure
in the future.   The only time we don't add trailing commas is
for 'null' terminator type entries where we know anything added
must come before them.


> +	},
> +	[AD7380_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 'u',
> +		.realbits = 14,
> +		.storagebits = 16,
> +		.endianness = IIO_CPU
> +	},
> +};

>  
> +/*
> + * Single ended parts have a 2:1 multiplexer in front of each ADC.
> + *
> + * From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> + * export 4 channels and ad7386-4/7-4/8-4 export 8 channels.
> + *
> + * Inputs AinX0 of multiplexers correspond to the first half of IIO channels
> + * (i.e 0-1 or 0-3) and inputs AinX1 correspond to second half (i.e 2-3 or
> + * 4-7). Example for AD7386/7/8 (2 channels parts):
> + *
> + *           IIO   | AD7386/7/8
> + *                 |         +----------------------------
> + *                 |         |     _____        ______
> + *                 |         |    |     |      |      |
> + *        voltage0 | AinA0 --|--->|     |      |      |
> + *                 |         |    | mux |----->| ADCA |---
> + *        voltage2 | AinA1 --|--->|     |      |      |
> + *                 |         |    |_____|      |_____ |
> + *                 |         |     _____        ______
> + *                 |         |    |     |      |      |
> + *        voltage1 | AinB0 --|--->|     |      |      |
> + *                 |         |    | mux |----->| ADCB |---
> + *        voltage3 | AinB1 --|--->|     |      |      |
> + *                 |         |    |_____|      |______|
> + *                 |         |
> + *                 |         +----------------------------
> + *
> + * Since this is simultaneous sampling for AinX0 OR AinX1 we have two separate
> + * scan masks.
> + */

Good. I always like some nice art :)
+ your implementation takes the same approach I would have done.