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

[Jonathan Cameron]

On Fri, 24 May 2024 10:56:55 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:

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