| Message ID | 20240507-iio-add-support-for-multiple-scan-types-v1-3-95ac33ee51e9@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add support for multiple scan types | expand |
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, >
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, >> >
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? > > > > > > >> 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, > >> > > > >
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?
On Fri, 24 May 2024 10:56:55 -0500 David Lechner <dlechner@baylibre.com> wrote: > 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? Hmm. I seem to be failing to explain this! Key is the complete lack of association between what is returned by the get_current_scan_type() callback and this ext_scan_type array. So either: 1) Make it do so - easiest being to return an index into the array rather than a possibly unrelated scan_type - that would guarantee the scan_type returned by the callback was one that has been validated. or 2) Drop validation at initial probe because you are validating something that is irrelevant to what actually gets returned later. Validate when the scan type is read back via get_current_scan_type() I prefer option 1. >
On 5/25/24 11:14 AM, Jonathan Cameron wrote: > On Fri, 24 May 2024 10:56:55 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> 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? > Hmm. I seem to be failing to explain this! Maybe we are talking about two different things but calling them the same thing? > Key is the complete lack of > association between what is returned by the get_current_scan_type() callback > and this ext_scan_type array. Why would the caller of get_current_scan_type() need to know that the returned value is associated with ext_scan_type? > > So either: > 1) Make it do so - easiest being to return an index into the array rather than > a possibly unrelated scan_type - Unrelated to what? > that would guarantee the scan_type returned > by the callback was one that has been validated. Since all scan types are const data and not changed after the iio device is registered, the validation done at registration seems sufficient to me (validation happens in __iio_buffer_alloc_sysfs_and_mask()). All scan types are validated at this time, including all ext_scan_types. So all are guaranteed to be validated already when the get_current_scan_type callback is called. What other validation would need to be done later? > or > 2) Drop validation at initial probe because you are validating something > that is irrelevant to what actually gets returned later. Validate> when the scan type is read back via get_current_scan_type() The validation is just checking for programmer error, so it seems better to catch that at probe where we are guaranteed to catch it for all scan types. If the driver fails to probe, the programmer should notice this and fix their mistake, but if we don't validate until later, the programmer might not check every single configuration every time a change is made. > > I prefer option 1. >> >
On Sat, 25 May 2024 12:04:46 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 5/25/24 11:14 AM, Jonathan Cameron wrote: > > On Fri, 24 May 2024 10:56:55 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> 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? > > Hmm. I seem to be failing to explain this! > > Maybe we are talking about two different things but calling them the same thing? I'm not sure. Sounds like we both think our point is entirely obvious and clearly it isn't :( > > Key is the complete lack of > > association between what is returned by the get_current_scan_type() callback > > and this ext_scan_type array. > > Why would the caller of get_current_scan_type() need to know that the > returned value is associated with ext_scan_type? Because you are validating ext_scan_type, not the return of get_current_scan_type(). They may or may not include the same data - to make this a good interface, that isn't error prone, get_current_scan_type() must return one that has been validated - i.e. is in the ext_scan_type array. I've looked several times and maybe I'm just failing to spot what ensures the validation is sufficient. > > > > > So either: > > 1) Make it do so - easiest being to return an index into the array rather than > > a possibly unrelated scan_type - > > Unrelated to what? Unrelated to anything the IIO core is currently aware of. You could have a list of types of cats that you've validated for feline characteristics and this callback returns a donkey. > > > that would guarantee the scan_type returned > > by the callback was one that has been validated. > > Since all scan types are const data and not changed after the iio device > is registered, the validation done at registration seems sufficient to > me (validation happens in __iio_buffer_alloc_sysfs_and_mask()). All scan > types are validated at this time, including all ext_scan_types. So all > are guaranteed to be validated already when the get_current_scan_type > callback is called. > > What other validation would need to be done later? What makes get_current_scan_type() return a scan type that is in the ext_scan_type array? A possible implementation (which should not be possible!) is static const struct iio_scan_type scan_type_A = { .sign = 's', .realbits = 16, .storagebits = 16, .endianness = IIO_CPU, }; static const struct iio_scan_type scan_type_B = { .sign = 's', .realbits = 18, .storagebits = 16, .endianness = IIO_CPU, }; .ext_scan_type = &ad7380_scan_type_A, static const struct iio_scan_type *get_current_scan_type( const struct iio_dev *indio_dev, struct iio_chan_spec const *chan) { //some stuff to select return scan_type_B; } > > > or > > 2) Drop validation at initial probe because you are validating something > > that is irrelevant to what actually gets returned later. Validate> when the scan type is read back via get_current_scan_type() > > The validation is just checking for programmer error, so it seems better > to catch that at probe where we are guaranteed to catch it for all scan > types. If the driver fails to probe, the programmer should notice this and > fix their mistake, but if we don't validate until later, the programmer > might not check every single configuration every time a change is made. I agreed - but today that isn't happening in the above example. You need to enforce that the scan_type returned is one that has been validated. Maybe I'm missing that validation occurring somewhere? Jonathan > > > > > I prefer option 1. > >> > > > >
On Sun, May 26, 2024 at 7:11 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 25 May 2024 12:04:46 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On 5/25/24 11:14 AM, Jonathan Cameron wrote: > > > On Fri, 24 May 2024 10:56:55 -0500 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > >> 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: > > >>>>> ... > > > > Maybe we are talking about two different things but calling them the same thing? > > I'm not sure. Sounds like we both think our point is entirely obvious and clearly > it isn't :( > > > > Key is the complete lack of > > > association between what is returned by the get_current_scan_type() callback > > > and this ext_scan_type array. > > > > Why would the caller of get_current_scan_type() need to know that the > > returned value is associated with ext_scan_type? > > Because you are validating ext_scan_type, not the return of get_current_scan_type(). > They may or may not include the same data - to make this a good interface, that isn't > error prone, get_current_scan_type() must return one that has been validated - i.e. > is in the ext_scan_type array. > > I've looked several times and maybe I'm just failing to spot what ensures the validation > is sufficient. > Ah, I finally get it now. I was having tunnel vision and it didn't even occur to me that someone might be tempted to return anything that wasn't a pointer to the ext_scan types array. > > > > > > > > So either: > > > 1) Make it do so - easiest being to return an index into the array rather than > > > a possibly unrelated scan_type - > > This option 1) makes sense to me now. Do we also need to validate that the index returned is < num_ext_scan_types in iio_get_current_scan_type()?
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 08103a9e77f7..ef27ce71ec25 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -365,8 +365,10 @@ static ssize_t iio_show_fixed_type(struct device *dev, struct device_attribute *attr, char *buf) { + struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); - const struct iio_scan_type *scan_type = &this_attr->c->scan_type; + const struct iio_scan_type *scan_type = + iio_get_current_scan_type(indio_dev, this_attr->c); u8 type = scan_type->endianness; if (type == IIO_CPU) { @@ -699,7 +701,7 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev, unsigned int bytes; ch = iio_find_channel_from_si(indio_dev, scan_index); - scan_type = &ch->scan_type; + scan_type = iio_get_current_scan_type(indio_dev, ch); bytes = scan_type->storagebits / 8; if (scan_type->repeat > 1) @@ -1597,6 +1599,22 @@ static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp } } +static int iio_channel_validate_scan_type(struct device *dev, int ch, + const struct iio_scan_type *scan_type) +{ + /* Verify that sample bits fit into storage */ + if (scan_type->storagebits < scan_type->realbits + scan_type->shift) { + dev_err(dev, + "Channel %d storagebits (%d) < shifted realbits (%d + %d)\n", + ch, scan_type->storagebits, + scan_type->realbits, + scan_type->shift); + return -EINVAL; + } + + return 0; +} + static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, struct iio_dev *indio_dev, int index) @@ -1622,22 +1640,25 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, /* new magic */ for (i = 0; i < indio_dev->num_channels; i++) { const struct iio_scan_type *scan_type; + int j; if (channels[i].scan_index < 0) continue; scan_type = &channels[i].scan_type; - /* Verify that sample bits fit into storage */ - if (scan_type->storagebits < - scan_type->realbits + scan_type->shift) { - dev_err(&indio_dev->dev, - "Channel %d storagebits (%d) < shifted realbits (%d + %d)\n", - i, scan_type->storagebits, - scan_type->realbits, - scan_type->shift); - ret = -EINVAL; + ret = iio_channel_validate_scan_type(&indio_dev->dev, + i, scan_type); + if (ret) goto error_cleanup_dynamic; + + for (j = 0; j < channels[i].num_ext_scan_type; j++) { + scan_type = &channels[i].ext_scan_type[j]; + + ret = iio_channel_validate_scan_type( + &indio_dev->dev, i, scan_type); + if (ret) + goto error_cleanup_dynamic; } ret = iio_buffer_add_channel_sysfs(indio_dev, buffer, 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. + * @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; 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,
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. 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> --- drivers/iio/industrialio-buffer.c | 43 +++++++++++++++++++++++++++++---------- include/linux/iio/iio.h | 33 ++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-)