On 5/20/24 11:12 AM, Jonathan Cameron wrote:
> On Mon, 20 May 2024 08:51:52 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
>> On 5/19/24 2:12 PM, Jonathan Cameron wrote:
>>> On Tue, 7 May 2024 14:02:07 -0500
>>> David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> ---
>>>
>>>> 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?
