Received-SPF: pass (google.com: domain of linux-kernel+bounces-183218-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 19 May 2024 20:12:41 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>, Nuno =?UTF-8?B?U8Oh?=
 <nuno.sa@analog.com>, Julien Stephan <jstephan@baylibre.com>, Esteban Blanc
 <eblanc@baylibre.com>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 3/4] iio: add support for multiple scan types per
 channel
Message-ID: <20240519201241.7c60abac@jic23-huawei>
In-Reply-To: <20240507-iio-add-support-for-multiple-scan-types-v1-3-95ac33ee51e9@baylibre.com>
References: <20240507-iio-add-support-for-multiple-scan-types-v1-0-95ac33ee51e9@baylibre.com>
	<20240507-iio-add-support-for-multiple-scan-types-v1-3-95ac33ee51e9@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue,  7 May 2024 14:02:07 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds new fields to the iio_channel structure to support multiple
> scan types per channel. This is useful for devices that support multiple
> resolution modes or other modes that require different data formats of
> the raw data.
> 
> To make use of this, drivers can still use the old scan_type field for
> the "default" scan type and use the new scan_type_ext field for any
> additional scan types.

Comment inline says that you should commit scan_type if scan_type_ext
is provided.  That makes sense to me rather that a default no one reads.

The example that follows in patch 4 uses both the scan_type and
the scan_type_ext which is even more confusing.

> And they must implement the new callback
> get_current_scan_type() to return the current scan type based on the
> current state of the device.
> 
> The buffer code is the only code in the IIO core code that is using the
> scan_type field. This patch updates the buffer code to use the new
> iio_channel_validate_scan_type() function to ensure it is returning the
> correct scan type for the current state of the device when reading the
> sysfs attributes. The buffer validation code is also update to validate
> any additional scan types that are set in the scan_type_ext field. Part
> of that code is refactored to a new function to avoid duplication.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 19de573a944a..66f0b4c68f53 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -205,6 +205,9 @@ struct iio_scan_type {
>   * @scan_index:		Monotonic index to give ordering in scans when read
>   *			from a buffer.
>   * @scan_type:		struct describing the scan type
> + * @ext_scan_type:	Used in rare cases where there is more than one scan
> + *			format for a channel. When this is used, omit scan_type.

Here is the disagreement with the patch description.

> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
>   * @info_mask_separate: What information is to be exported that is specific to
>   *			this channel.
>   * @info_mask_separate_available: What availability information is to be
> @@ -256,6 +259,8 @@ struct iio_chan_spec {
>  	unsigned long		address;
>  	int			scan_index;
>  	struct iio_scan_type scan_type;
> +	const struct iio_scan_type *ext_scan_type;
> +	unsigned int		num_ext_scan_type;

Let's make it explicit that you can't do both.

	union {
		struct iio_scan_type scan_type;
		struct {
			const struct iio_scan_type *ext_scan_type;
			unsigned int num_ext_scan_type;
		};
	};
should work for that I think.

However this is I think only used for validation. If that's the case
do we care about values not in use?  Can we move the validation to
be runtime if the get_current_scan_type() callback is used.


>  	long			info_mask_separate;
>  	long			info_mask_separate_available;
>  	long			info_mask_shared_by_type;
> @@ -435,6 +440,9 @@ struct iio_trigger; /* forward declaration */
>   *			for better event identification.
>   * @validate_trigger:	function to validate the trigger when the
>   *			current trigger gets changed.
> + * @get_current_scan_type: must be implemented by drivers that use ext_scan_type
> + *			in the channel spec to return the currently active scan
> + *			type based on the current state of the device.
>   * @update_scan_mode:	function to configure device and scan buffer when
>   *			channels have changed
>   * @debugfs_reg_access:	function to read or write register value of device
> @@ -519,6 +527,9 @@ struct iio_info {
>  
>  	int (*validate_trigger)(struct iio_dev *indio_dev,
>  				struct iio_trigger *trig);
> +	const struct iio_scan_type *(*get_current_scan_type)(
> +					const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan);
>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
>  				const unsigned long *scan_mask);
>  	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
> @@ -804,6 +815,28 @@ static inline bool iio_read_acpi_mount_matrix(struct device *dev,
>  }
>  #endif
>  
> +/**
> + * iio_get_current_scan_type - Get the current scan type for a channel
> + * @indio_dev:	the IIO device to get the scan type for
> + * @chan:	the channel to get the scan type for
> + *
> + * Most devices only have one scan type per channel and can just access it
> + * directly without calling this function. Core IIO code and drivers that
> + * implement ext_scan_type in the channel spec should use this function to
> + * get the current scan type for a channel.
> + *
> + * Returns: the current scan type for the channel
> + */
> +static inline const struct iio_scan_type *iio_get_current_scan_type(
> +					const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	if (indio_dev->info->get_current_scan_type)
> +		return indio_dev->info->get_current_scan_type(indio_dev, chan);
> +
> +	return &chan->scan_type;
> +}
> +
>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>  
>  int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
>