diff mbox series

[v3,2/6] iio: improve doc for available_scan_mask

Message ID 74b66a5b9eee2fb7046f254928391e3da61aa3b2.1695380366.git.mazziesaccount@gmail.com
State Accepted
Commit 8a76356e7db02ec7b1913db06605e70294d94672
Headers show
Series Support ROHM BM1390 pressure sensor | expand

Commit Message

Matti Vaittinen Sept. 22, 2023, 11:16 a.m. UTC
The available_scan_mask is an array of bitmaps representing the channels
which can be simultaneously enabled by the driver. In many cases the
hardware can offer more channels than what the user is interested in
obtaining. In such cases it may be preferred that only subset of
channels are enabled, and driver reads only a subset of the channels from
the hardware.

Some devices can't support all channel combinations. For example the
BM1390 pressure sensor must always read the pressure data in order to
acknowledge the watermark IRQ, while reading temperature can be omitted.
So, the available scan mask would be 'pressure and temperature' and
'pressure only'.

When IIO seatchs for the scan mask it asks the driver to use, it will
pick the first suitable one from the 'available_scan_mask' array. Hence,
ordering the masks in the array makes difference. We should 'prefer'
reading just the pressure from the hardware (as it is cheaper operation
than reading both pressure and temperature) over reading both pressure
and temperature. Hence, we should set the 'only pressure' as first scan
mask in available_scan_mask array. If we set the 'pressure and
temperature' as first in array, then the 'only temperature' will never
get used as 'pressure and temperature' can always serve the user's
needs.

Add (minimal) kerneldoc to the 'available_scan_mask' to hint the user
that ordering of masks matters.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 include/linux/iio/iio.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Matti Vaittinen Sept. 25, 2023, 9:50 a.m. UTC | #1
On 9/24/23 18:59, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:16:57 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The available_scan_mask is an array of bitmaps representing the channels
>> which can be simultaneously enabled by the driver. In many cases the
>> hardware can offer more channels than what the user is interested in
>> obtaining. In such cases it may be preferred that only subset of
>> channels are enabled, and driver reads only a subset of the channels from
>> the hardware.
>>
>> Some devices can't support all channel combinations. For example the
>> BM1390 pressure sensor must always read the pressure data in order to
>> acknowledge the watermark IRQ, while reading temperature can be omitted.
>> So, the available scan mask would be 'pressure and temperature' and
>> 'pressure only'.
>>
>> When IIO seatchs for the scan mask it asks the driver to use, it will
> 
> Spell check description.  searches

Oh, right. Thanks!

> 
>> pick the first suitable one from the 'available_scan_mask' array. Hence,
>> ordering the masks in the array makes difference. We should 'prefer'
>> reading just the pressure from the hardware (as it is cheaper operation
>> than reading both pressure and temperature) over reading both pressure
>> and temperature. Hence, we should set the 'only pressure' as first scan
>> mask in available_scan_mask array. If we set the 'pressure and
>> temperature' as first in array, then the 'only temperature' will never
>> get used as 'pressure and temperature' can always serve the user's
>> needs.
>>
>> Add (minimal) kerneldoc to the 'available_scan_mask' to hint the user
>> that ordering of masks matters.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   include/linux/iio/iio.h | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 202e55b0a28b..7bfa1b9bc8a2 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -556,7 +556,9 @@ struct iio_buffer_setup_ops {
>>    *			and owner
>>    * @buffer:		[DRIVER] any buffer present
>>    * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
>> - * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
>> + * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
>> + *			   array in order of preference, the most preferred
>> + *			   masks first.
> 
> LGTM

I'll try to spell check the commit message and then I treat this as an 
ack. Please, let me know if it's not Ok.

>>    * @masklength:		[INTERN] the length of the mask established from
>>    *			channels
>>    * @active_scan_mask:	[INTERN] union of all scan masks requested by buffers
>
Jonathan Cameron Sept. 25, 2023, 1:17 p.m. UTC | #2
On Mon, 25 Sep 2023 12:50:46 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/24/23 18:59, Jonathan Cameron wrote:
> > On Fri, 22 Sep 2023 14:16:57 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> The available_scan_mask is an array of bitmaps representing the channels
> >> which can be simultaneously enabled by the driver. In many cases the
> >> hardware can offer more channels than what the user is interested in
> >> obtaining. In such cases it may be preferred that only subset of
> >> channels are enabled, and driver reads only a subset of the channels from
> >> the hardware.
> >>
> >> Some devices can't support all channel combinations. For example the
> >> BM1390 pressure sensor must always read the pressure data in order to
> >> acknowledge the watermark IRQ, while reading temperature can be omitted.
> >> So, the available scan mask would be 'pressure and temperature' and
> >> 'pressure only'.
> >>
> >> When IIO seatchs for the scan mask it asks the driver to use, it will  
> > 
> > Spell check description.  searches  
> 
> Oh, right. Thanks!
> 
> >   
> >> pick the first suitable one from the 'available_scan_mask' array. Hence,
> >> ordering the masks in the array makes difference. We should 'prefer'
> >> reading just the pressure from the hardware (as it is cheaper operation
> >> than reading both pressure and temperature) over reading both pressure
> >> and temperature. Hence, we should set the 'only pressure' as first scan
> >> mask in available_scan_mask array. If we set the 'pressure and
> >> temperature' as first in array, then the 'only temperature' will never
> >> get used as 'pressure and temperature' can always serve the user's
> >> needs.
> >>
> >> Add (minimal) kerneldoc to the 'available_scan_mask' to hint the user
> >> that ordering of masks matters.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >> ---
> >>   include/linux/iio/iio.h | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> >> index 202e55b0a28b..7bfa1b9bc8a2 100644
> >> --- a/include/linux/iio/iio.h
> >> +++ b/include/linux/iio/iio.h
> >> @@ -556,7 +556,9 @@ struct iio_buffer_setup_ops {
> >>    *			and owner
> >>    * @buffer:		[DRIVER] any buffer present
> >>    * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
> >> - * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
> >> + * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
> >> + *			   array in order of preference, the most preferred
> >> + *			   masks first.  
> > 
> > LGTM  
> 
> I'll try to spell check the commit message and then I treat this as an 
> ack. Please, let me know if it's not Ok.
Given I'll be picking these up anyway, not a formal Ack that you should carry
with the next version, but looks fine to me.

J

> 
> >>    * @masklength:		[INTERN] the length of the mask established from
> >>    *			channels
> >>    * @active_scan_mask:	[INTERN] union of all scan masks requested by buffers  
> >   
>
diff mbox series

Patch

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 202e55b0a28b..7bfa1b9bc8a2 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -556,7 +556,9 @@  struct iio_buffer_setup_ops {
  *			and owner
  * @buffer:		[DRIVER] any buffer present
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
- * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
+ * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
+ *			   array in order of preference, the most preferred
+ *			   masks first.
  * @masklength:		[INTERN] the length of the mask established from
  *			channels
  * @active_scan_mask:	[INTERN] union of all scan masks requested by buffers