Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner <dlechner@baylibre.com>]

On 5/20/24 11:12 AM, Jonathan Cameron wrote:
> On Mon, 20 May 2024 08:51:52 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> 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.
> 
> That requires the possible scan masks to be listed here but there is
> nothing enforcing the callback returning one from here.  Maybe make it
> return an index instead?
> 

Sorry, still not understanding what we are trying to catch here. Why
would the scan mask have any effect of checking if realbits > storagebits?