Received-SPF: pass (google.com: domain of linux-kernel+bounces-183808-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ebf18ed1-a82f-4c0a-9a63-2c428b5aee40@baylibre.com>
Date: Mon, 20 May 2024 08:51:52 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC 3/4] iio: add support for multiple scan types per
 channel
To: Jonathan Cameron <jic23@kernel.org>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>,
 =?UTF-8?Q?Nuno_S=C3=A1?= <nuno.sa@analog.com>,
 Julien Stephan <jstephan@baylibre.com>, Esteban Blanc <eblanc@baylibre.com>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
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>
 <20240519201241.7c60abac@jic23-huawei>
Content-Language: en-US
From: David Lechner <dlechner@baylibre.com>
In-Reply-To: <20240519201241.7c60abac@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 5/19/24 2:12 PM, Jonathan Cameron wrote:
> 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.

I like the suggestion of the union to use one or the other. But I'm not
sure I understand the comments about validation.

If you are referring to iio_channel_validate_scan_type(), it only checks
for programmer error of realbits > storagebits, so it seems better to
keep it where it is to fail as early as possible.

> 
> 
>>  	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,
>>
>