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