Received-SPF: pass (google.com: domain of linux-kernel+bounces-112674-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 24 Mar 2024 13:07:56 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Julien Stephan <jstephan@baylibre.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich
 <Michael.Hennerich@analog.com>, Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>,
 David Lechner <dlechner@baylibre.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
 <conor+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown
 <broonie@kernel.org>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, kernel test robot
 <lkp@intel.com>
Subject: Re: [PATCH v5 5/7] iio: adc: ad7380: prepare for parts with more
 channels
Message-ID: <20240324130756.598a4b36@jic23-huawei>
In-Reply-To: <20240319-adding-new-ad738x-driver-v5-5-ce7df004ceb3@baylibre.com>
References: <20240319-adding-new-ad738x-driver-v5-0-ce7df004ceb3@baylibre.com>
	<20240319-adding-new-ad738x-driver-v5-5-ce7df004ceb3@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 19 Mar 2024 11:11:26 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> The current driver supports only parts with 2 channels.
> In order to prepare the support of new compatible ADCs with more
> channels, this commit:
>   - defines MAX_NUM_CHANNEL to specify the maximum number of
>     channels currently supported by the driver
>   - adds available_scan_mask member in ad7380_chip_info structure
>   - fixes spi xfer struct len depending on number of channels
>   - fixes scan_data.raw buffer size to handle more channels
>   - adds a timing specifications structure in ad7380_chip_info structure
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

>  struct ad7380_state {
> @@ -148,15 +168,15 @@ struct ad7380_state {
>  	struct spi_device *spi;
>  	struct regmap *regmap;
>  	unsigned int vref_mv;
> -	unsigned int vcm_mv[2];
> +	unsigned int vcm_mv[MAX_NUM_CHANNELS];
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> -	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
> +	 * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
>  	 * aligned 64 bit timestamp.
>  	 */
>  	struct {
> -		u16 raw[2];
> +		u16 raw[MAX_NUM_CHANNELS];

This can get problematic for drivers supporting devices with varying numbers of channels.
You are fine up to 4 channels as the structure layout isn't changing.  The reason is that
it implies the timestamp location, which if you were for instance to support 8 channels
would be incorrect if only 4 of them were enabled.
Now the timestamp location is only used implicitly in the driver (as the IIO core
handles the actual insertion of the data) so it's not a bug as such to do this, but it
can give people the wrong impression during testing etc.

As such, I'd like to see the comment mention that "as MAX_NUM_CHANNELS is 4
the layout of the structure is the same for all parts".

Note I've reviewed one driver today that did have different layouts depending on
which device was in use and ended up falsely indicating the timestamp was later
than it actually was in the buffer.  Hence my request for a defensive comment!

>  
>  		s64 ts __aligned(8);
>  	} scan_data __aligned(IIO_DMA_MINALIGN);