Support ROHM BM1390 pressure sensor

The iio_generic_buffer can return garbage values when the total size of scan data is not a multiple of largest element in the scan. This can be demonstrated by reading a scan consisting for example of one 4 byte and one 2 byte element, where the 4 byte elemnt is first in the buffer.

The IIO generic buffert code does not take into accunt the last two padding bytes that are needed to ensure that the 4byte data for next scan is correctly aligned.

Add padding bytes required to align the next sample into the scan size.

Signed-off-by Matti Vaittinen <[email protected]>

---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment should be used instead? This patch can be dropped from the series if the fix is not correct / agreed.

 tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

tools/iio/iio_generic_buffer.c +4 -1

index 44bbf80f0cfd..fc562799a109 100644
@@ -54,9 +54,12 @@ enum autochan {
 static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
 {
 	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
 
 	while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
 		if (bytes % channels[i].bytes == 0)
 			channels[i].location = bytes;
 		else

tools/iio/iio_generic_buffer.c +2 -0

+		bytes += max - misalignment;
+	}
 
 	return bytes;
 }

Quoted 43 lines · On Fri, 22 Sep 2023 14:16:08 +0300 Matti Vaittinen <[email protected]>…

The iio_generic_buffer can return garbage values when the total size of scan data is not a multiple of largest element in the scan. This can be demonstrated by reading a scan consisting for example of one 4 byte and one 2 byte element, where the 4 byte elemnt is first in the buffer.

The IIO generic buffert code does not take into accunt the last two padding bytes that are needed to ensure that the 4byte data for next scan is correctly aligned.

Add padding bytes required to align the next sample into the scan size.

Signed-off-by Matti Vaittinen <[email protected]>

---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment should be used instead? This patch can be dropped from the series if the fix is not correct / agreed.

  tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

tools/iio/iio_generic_buffer.c +4 -1

index 44bbf80f0cfd..fc562799a109 100644
@@ -54,9 +54,12 @@ enum autochan {
  static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
  {
  	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
 
  	while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
  		if (bytes % channels[i].bytes == 0)
  			channels[i].location = bytes;
  		else

@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
  		bytes = channels[i].location + channels[i].bytes;
  		i++;

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 76

Quoted 6 lines

tools/iio/iio_generic_buffer.c +5 -0

  	}
+	/*
+	 * We wan't the data in next sample to also be properly aligned so
+	 * we'll add padding at the end if needed. TODO: should we use fixed
+	 * 8 byte alignment instead of the size of the biggest samnple?
+	 */

Should be aligned to max size seen in the scan.

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 79

Quoted 4 lines

tools/iio/iio_generic_buffer.c +3 -0

+	misalignment = bytes % max;
+	if (misalignment) {
+		printf("Misalignment %u. Adding Padding %u\n", misalignment,  max - misalignment);

No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it correctly I believe). Fine to add a comment though!

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 84

Quoted 6 lines

tools/iio/iio_generic_buffer.c +2 -0

+		bytes += max - misalignment;
+	}
 
  	return bytes;
  }

Quoted 46 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 22 Sep 2023 14:16:08 +0300 Matti Vaittinen <[email protected]>…

The iio_generic_buffer can return garbage values when the total size of scan data is not a multiple of largest element in the scan. This can be demonstrated by reading a scan consisting for example of one 4 byte and one 2 byte element, where the 4 byte elemnt is first in the buffer.

The IIO generic buffert code does not take into accunt the last two padding bytes that are needed to ensure that the 4byte data for next scan is correctly aligned.

Add padding bytes required to align the next sample into the scan size.

Signed-off-by Matti Vaittinen <[email protected]>

---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment should be used instead? This patch can be dropped from the series if the fix is not correct / agreed.

  tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

tools/iio/iio_generic_buffer.c +4 -1

index 44bbf80f0cfd..fc562799a109 100644
@@ -54,9 +54,12 @@ enum autochan {
  static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
  {
  	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
 
  	while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
  		if (bytes % channels[i].bytes == 0)
  			channels[i].location = bytes;
  		else

@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
  		bytes = channels[i].location + channels[i].bytes;
  		i++;

Quoted 8 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 76

Quoted 6 lines

tools/iio/iio_generic_buffer.c +5 -0

  	}
+	/*
+	 * We wan't the data in next sample to also be properly aligned so
+	 * we'll add padding at the end if needed. TODO: should we use fixed
+	 * 8 byte alignment instead of the size of the biggest samnple?
+	 */  

Should be aligned to max size seen in the scan.

Or, maybe it should be
min(max_size_in_scan, 8);
?

I think my suggestion above may yield undesirable effects should the scan elements be greater than 8 bytes. (Don't know if this is supported though)

Quoted 8 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 79

Quoted 5 lines

tools/iio/iio_generic_buffer.c +3 -0

+	misalignment = bytes % max;
+	if (misalignment) {
+		printf("Misalignment %u. Adding Padding %u\n", misalignment,  max - misalignment);  

No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it correctly I believe). Fine to add a comment though!

Oh, indeed. The print was forgotten from my test runs. Thanks for pointing it out!

Quoted 7 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 84

Quoted 6 lines

tools/iio/iio_generic_buffer.c +2 -0

+		bytes += max - misalignment;
+	}
 
  	return bytes;
  }  

Yours,
-- Matti

Quoted 56 lines · On Mon, 25 Sep 2023 10:01:09 +0300 Matti Vaittinen <[email protected]>…

Quoted 54 lines · Jonathan Cameron

Quoted 41 lines · On Fri, 22 Sep 2023 14:16:08 +0300 Matti Vaittinen <[email protected]>…

The iio_generic_buffer can return garbage values when the total size of scan data is not a multiple of largest element in the scan. This can be demonstrated by reading a scan consisting for example of one 4 byte and one 2 byte element, where the 4 byte elemnt is first in the buffer.

The IIO generic buffert code does not take into accunt the last two padding bytes that are needed to ensure that the 4byte data for next scan is correctly aligned.

Add padding bytes required to align the next sample into the scan size.

Signed-off-by Matti Vaittinen <[email protected]>

---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment should be used instead? This patch can be dropped from the series if the fix is not correct / agreed.

   tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
   1 file changed, 14 insertions(+), 1 deletion(-)

tools/iio/iio_generic_buffer.c +4 -1

index 44bbf80f0cfd..fc562799a109 100644
@@ -54,9 +54,12 @@ enum autochan {
   static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
   {
   	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
 
   	while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
   		if (bytes % channels[i].bytes == 0)
   			channels[i].location = bytes;
   		else

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 76

Quoted 8 lines

tools/iio/iio_generic_buffer.c +5 -0

@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
   		bytes = channels[i].location + channels[i].bytes;
   		i++;
   	}
+	/*
+	 * We wan't the data in next sample to also be properly aligned so
+	 * we'll add padding at the end if needed. TODO: should we use fixed
+	 * 8 byte alignment instead of the size of the biggest samnple?
+	 */

Should be aligned to max size seen in the scan.

Quoted 3 lines

Or, maybe it should be
min(max_size_in_scan, 8);
?

Definitely not. If you are grabbing just one channel of 8 bit data,
we want it to be tightly packed.

If we have a bug that already made that true then we might be stuck with it, but I'm fairly sure we don't.

Quoted 4 lines

I think my suggestion above may yield undesirable effects should the scan elements be greater than 8 bytes. (Don't know if this is supported though)

It is supported in theory, in practice not seen one yet.

Quoted 11 lines

Quoted 6 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 79

Quoted 3 lines

tools/iio/iio_generic_buffer.c +3 -0

+	misalignment = bytes % max;
+	if (misalignment) {
+		printf("Misalignment %u. Adding Padding %u\n", misalignment,  max - misalignment);

No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it correctly I believe). Fine to add a comment though!

Oh, indeed. The print was forgotten from my test runs. Thanks for pointing it out!

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 84

Quoted 5 lines

tools/iio/iio_generic_buffer.c +2 -0

+		bytes += max - misalignment;
+	}
 
   	return bytes;
   }

Quoted 63 lines · Jonathan Cameron

Quoted 59 lines · On Mon, 25 Sep 2023 10:01:09 +0300 Matti Vaittinen <[email protected]>…

Quoted 46 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 22 Sep 2023 14:16:08 +0300 Matti Vaittinen <[email protected]>…

The iio_generic_buffer can return garbage values when the total size of scan data is not a multiple of largest element in the scan. This can be demonstrated by reading a scan consisting for example of one 4 byte and one 2 byte element, where the 4 byte elemnt is first in the buffer.

The IIO generic buffert code does not take into accunt the last two padding bytes that are needed to ensure that the 4byte data for next scan is correctly aligned.

Add padding bytes required to align the next sample into the scan size.

Signed-off-by Matti Vaittinen <[email protected]>

---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment should be used instead? This patch can be dropped from the series if the fix is not correct / agreed.

   tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
   1 file changed, 14 insertions(+), 1 deletion(-)

tools/iio/iio_generic_buffer.c +4 -1

index 44bbf80f0cfd..fc562799a109 100644
@@ -54,9 +54,12 @@ enum autochan {
   static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
   {
   	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
 
   	while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
   		if (bytes % channels[i].bytes == 0)
   			channels[i].location = bytes;
   		else

@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
   		bytes = channels[i].location + channels[i].bytes;
   		i++;

Quoted 8 lines

tools/iio/iio_generic_buffer.c static unsigned int size_from_channelarray(struct iio_channel_info *channels, in · line 76

Quoted 6 lines

tools/iio/iio_generic_buffer.c +5 -0

   	}
+	/*
+	 * We wan't the data in next sample to also be properly aligned so
+	 * we'll add padding at the end if needed. TODO: should we use fixed
+	 * 8 byte alignment instead of the size of the biggest samnple?
+	 */  

Should be aligned to max size seen in the scan.

Or, maybe it should be
min(max_size_in_scan, 8);
?

Quoted 2 lines

Definitely not. If you are grabbing just one channel of 8 bit data,
we want it to be tightly packed.

I think that in this case the max_size_in_scan would be 1, and min(1, 8) would be 1 as well, resulting a tightly packed data. I am just wondering if we should use 8 as maximum alignment - eg, if our scan has 16 bytes data + 1 byte data, we would add 7 bytes of padding, not 15 bytes of padding. I am not sure what is the right thing to do.

Quoted 9 lines

If we have a bug that already made that true then we might be stuck with it, but I'm fairly sure we don't.

Quoted 4 lines

I think my suggestion above may yield undesirable effects should the scan elements be greater than 8 bytes. (Don't know if this is supported though)

It is supported in theory, in practice not seen one yet.

So, whether to unconditionally use largest scan element sized alignment
- or largest scan element up to 8 bytes - is a question we haven't hit yet :)

Actually, more I stare at the alignment code here, less sure I am it is correct - but maybe I don't understand how the data should be aligned.

I think it works if allowed data sizes are 1, 2, 4, and 8. However, I suspect it breaks for other sizes.

For non power of2 sizes, the alignment code will result strange alignments. For example, scan consisting of two 6-byte elements would be packed - meaning the second element would probably break the alignment rules by starting from address '6'. I think that on most architectures the proper access would require 2 padding bytes to be added at the end of the first sample. Current code wouldn't do that.

If we allow only power of 2 sizes - I would expect a scan consisting of a 8 byte element followed by a 16 byte element to be tightly packed. I'd assume that for the 16 byte data, it'd be enough to ensure 8 byte alignment. Current code would however add 8 bytes of padding at the end of the first 8 byte element to make the 16 byte scan element to be aligned at 16 byte address. To my uneducated mind this is not needed - but maybe I just don't know what I am writing about :)

In any case, the patch here should fix things when allowed scan element sizes are 1, 2, 4 and 8 and we have to add padding after last scan element. It won't work for other sizes, but as I wrote, I suspect the whole alignment code here may be broken for other sizes so things shouldn't at least get worse with this patch... I think this should be revised if we see samples of other sizes - and in any case, this might at least warrant a comment here :) (I reserve a right to be wrong. Haven't been sleeping too well lately and my head is humming...)

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 <[email protected]>

---
 include/linux/iio/iio.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

include/linux/iio/iio.h +0 -0

index 202e55b0a28b..7bfa1b9bc8a2 100644

include/linux/iio/iio.h +0 -0

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

Quoted 13 lines · On Fri, 22 Sep 2023 14:16:57 +0300 Matti Vaittinen <[email protected]>…

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

Quoted 1 line

When IIO seatchs for the scan mask it asks the driver to use, it will

Spell check description. searches

Quoted 19 lines

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 <[email protected]>

---
  include/linux/iio/iio.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

include/linux/iio/iio.h struct iio_buffer_setup_ops { · line 561

Quoted 12 lines

include/linux/iio/iio.h +3 -1

index 202e55b0a28b..7bfa1b9bc8a2 100644
@@ -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

Quoted 3 lines

include/linux/iio/iio.h +0 -0

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

Quoted 16 lines · Jonathan Cameron

Quoted 13 lines · On Fri, 22 Sep 2023 14:16:57 +0300 Matti Vaittinen <[email protected]>…

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

Quoted 3 lines

Quoted 1 line

When IIO seatchs for the scan mask it asks the driver to use, it will

Spell check description. searches

Oh, right. Thanks!

Quoted 19 lines

Quoted 19 lines

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 <[email protected]>

---
  include/linux/iio/iio.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 14 lines

include/linux/iio/iio.h struct iio_buffer_setup_ops { · line 561

Quoted 12 lines

include/linux/iio/iio.h +3 -1

index 202e55b0a28b..7bfa1b9bc8a2 100644
@@ -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.

Quoted 5 lines

Quoted 4 lines

include/linux/iio/iio.h +0 -0

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

When IIO goes through the available scan masks in order to select the best suiting one, it will just accept the first listed subset of channels which meets the user's requirements. This works great for most of the drivers as they can sort the list of channels in the order where the 'least costy' channel selections come first.

It may be that in some cases the ordering of the list of available scan masks is not thoroughly considered. We can't really try outsmarting the drivers by selecting the smallest supported subset - as this might not be the 'least costy one' - but we can at least try searching through the list to see if we have an exactly matching mask. It should be sane assumption that if the device can support reading only the exact channels user is interested in, then this should be also the least costy selection - and if it is not and optimization is important, then the driver could consider omitting setting the 'available_scan_mask' and doing demuxing - or just omitting the 'costy exact match' and providing only the more efficient broader selection of channels.

Signed-off-by Matti Vaittinen <[email protected]>

---
 drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

drivers/iio/industrialio-buffer.c +19 -6

index 176d31d9f9d8..e97396623373 100644
@@ -411,19 +411,32 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
 						const unsigned long *mask,
 						bool strict)
 {
+	const unsigned long *first_subset = NULL;
+
 	if (bitmap_empty(mask, masklength))
 		return NULL;
-	while (*av_masks) {
-		if (strict) {
+
+	if (strict) {
+		while (*av_masks) {
 			if (bitmap_equal(mask, av_masks, masklength))
 				return av_masks;
-		} else {
-			if (bitmap_subset(mask, av_masks, masklength))
-				return av_masks;
+
+			av_masks += BITS_TO_LONGS(masklength);
 		}
+
+		return NULL;
+	}
+	while (*av_masks) {
+		if (bitmap_equal(mask, av_masks, masklength))
+			return av_masks;
+
+		if (!first_subset && bitmap_subset(mask, av_masks, masklength))
+			first_subset = av_masks;
+
 		av_masks += BITS_TO_LONGS(masklength);
 	}
-	return NULL;
+
+	return first_subset;
 }
 
 static bool iio_validate_scan_mask(struct iio_dev *indio_dev,

Quoted 18 lines · On Fri, 22 Sep 2023 14:17:49 +0300 Matti Vaittinen <[email protected]>…

When IIO goes through the available scan masks in order to select the best suiting one, it will just accept the first listed subset of channels which meets the user's requirements. This works great for most of the drivers as they can sort the list of channels in the order where the 'least costy' channel selections come first.

It may be that in some cases the ordering of the list of available scan masks is not thoroughly considered. We can't really try outsmarting the drivers by selecting the smallest supported subset - as this might not be the 'least costy one' - but we can at least try searching through the list to see if we have an exactly matching mask. It should be sane assumption that if the device can support reading only the exact channels user is interested in, then this should be also the least costy selection - and if it is not and optimization is important, then the driver could consider omitting setting the 'available_scan_mask' and doing demuxing - or just omitting the 'costy exact match' and providing only the more efficient broader selection of channels.

Quoted 1 line

Signed-off-by Matti Vaittinen <[email protected]>

Whilst I fully agree with the reasoning behind this, I'd rather we did an audit of drivers to find any that have a non logical order (one came up today in review) and fix them up.

A quick and dirty grep didn't find it to be a common problem, at least partly as most users of this feature only provide one valid mask. The few complex corners I found appear to be fine with the expected shortest sequences first.

Defending against driver bugs is losing game if it makes the core code more complex to follow by changing stuff in non debug paths. One option might be to add a trivial check at iio_device_register() that we don't have scan modes that are subsets of modes earlier in the list. These lists are fairly short so should be cheap to run.

That would incorporate ensuring exact matches come earlier by default.

Jonathan

Quoted 47 lines

---
 drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

drivers/iio/industrialio-buffer.c +19 -6

index 176d31d9f9d8..e97396623373 100644
@@ -411,19 +411,32 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
 						const unsigned long *mask,
 						bool strict)
 {
+	const unsigned long *first_subset = NULL;
+
 	if (bitmap_empty(mask, masklength))
 		return NULL;
-	while (*av_masks) {
-		if (strict) {
+
+	if (strict) {
+		while (*av_masks) {
 			if (bitmap_equal(mask, av_masks, masklength))
 				return av_masks;
-		} else {
-			if (bitmap_subset(mask, av_masks, masklength))
-				return av_masks;
+
+			av_masks += BITS_TO_LONGS(masklength);
 		}
+
+		return NULL;
+	}
+	while (*av_masks) {
+		if (bitmap_equal(mask, av_masks, masklength))
+			return av_masks;
+
+		if (!first_subset && bitmap_subset(mask, av_masks, masklength))
+			first_subset = av_masks;
+
 		av_masks += BITS_TO_LONGS(masklength);
 	}
-	return NULL;
+
+	return first_subset;
 }
 
 static bool iio_validate_scan_mask(struct iio_dev *indio_dev,  

Quoted 38 lines · On Sun, 24 Sep 2023 17:07:26 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 18 lines · On Fri, 22 Sep 2023 14:17:49 +0300 Matti Vaittinen <[email protected]>…

When IIO goes through the available scan masks in order to select the best suiting one, it will just accept the first listed subset of channels which meets the user's requirements. This works great for most of the drivers as they can sort the list of channels in the order where the 'least costy' channel selections come first.

It may be that in some cases the ordering of the list of available scan masks is not thoroughly considered. We can't really try outsmarting the drivers by selecting the smallest supported subset - as this might not be the 'least costy one' - but we can at least try searching through the list to see if we have an exactly matching mask. It should be sane assumption that if the device can support reading only the exact channels user is interested in, then this should be also the least costy selection - and if it is not and optimization is important, then the driver could consider omitting setting the 'available_scan_mask' and doing demuxing - or just omitting the 'costy exact match' and providing only the more efficient broader selection of channels.

Quoted 1 line

Signed-off-by Matti Vaittinen <[email protected]>

Whilst I fully agree with the reasoning behind this, I'd rather we did an audit of drivers to find any that have a non logical order (one came up today in review) and fix them up.

A quick and dirty grep didn't find it to be a common problem, at least partly as most users of this feature only provide one valid mask. The few complex corners I found appear to be fine with the expected shortest sequences first.

Defending against driver bugs is losing game if it makes the core code more complex to follow by changing stuff in non debug paths. One option might be to add a trivial check at iio_device_register() that we don't have scan modes that are subsets of modes earlier in the list. These lists are fairly short so should be cheap to run.

Quoted 1 line

That would incorporate ensuring exact matches come earlier by default.

BTW I'd have sent these as a separate series as there is potential that this will distract from or slow down the driver + not all the CC list will care about this core cleanup.

Quoted 18 lines · On Fri, 22 Sep 2023 14:17:49 +0300 Matti Vaittinen <[email protected]>…

When IIO goes through the available scan masks in order to select the best suiting one, it will just accept the first listed subset of channels which meets the user's requirements. This works great for most of the drivers as they can sort the list of channels in the order where the 'least costy' channel selections come first.

It may be that in some cases the ordering of the list of available scan masks is not thoroughly considered. We can't really try outsmarting the drivers by selecting the smallest supported subset - as this might not be the 'least costy one' - but we can at least try searching through the list to see if we have an exactly matching mask. It should be sane assumption that if the device can support reading only the exact channels user is interested in, then this should be also the least costy selection - and if it is not and optimization is important, then the driver could consider omitting setting the 'available_scan_mask' and doing demuxing - or just omitting the 'costy exact match' and providing only the more efficient broader selection of channels.

Quoted 1 line

Signed-off-by Matti Vaittinen <[email protected]>

Whilst I fully agree with the reasoning behind this, I'd rather we did an audit of drivers to find any that have a non logical order (one came up today in review) and fix them up.

A quick and dirty grep didn't find it to be a common problem, at least partly as most users of this feature only provide one valid mask.

The few complex corners I found appear to be fine with the expected shortest sequences first.

Defending against driver bugs is losing game if it makes the core code more complex to follow by changing stuff in non debug paths.

One option might be to add a trivial check at iio_device_register()

that we don't have scan modes that are subsets of modes earlier in the list. These lists are fairly short so should be cheap to run.

That would incorporate ensuring exact matches come earlier by default.

Jonathan

Quoted 47 lines

---
  drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
  1 file changed, 19 insertions(+), 6 deletions(-)

drivers/iio/industrialio-buffer.c +19 -6

index 176d31d9f9d8..e97396623373 100644
@@ -411,19 +411,32 @@ static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
  						const unsigned long *mask,
  						bool strict)
  {
+	const unsigned long *first_subset = NULL;
+
  	if (bitmap_empty(mask, masklength))
  		return NULL;
-	while (*av_masks) {
-		if (strict) {
+
+	if (strict) {
+		while (*av_masks) {
  			if (bitmap_equal(mask, av_masks, masklength))
  				return av_masks;
-		} else {
-			if (bitmap_subset(mask, av_masks, masklength))
-				return av_masks;
+
+			av_masks += BITS_TO_LONGS(masklength);
  		}
+
+		return NULL;
+	}
+	while (*av_masks) {
+		if (bitmap_equal(mask, av_masks, masklength))
+			return av_masks;
+
+		if (!first_subset && bitmap_subset(mask, av_masks, masklength))
+			first_subset = av_masks;
+
  		av_masks += BITS_TO_LONGS(masklength);
  	}
-	return NULL;
+
+	return first_subset;
  }
 
  static bool iio_validate_scan_mask(struct iio_dev *indio_dev,

Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure pressures ranging from 300 hPa to 1300 hPa with configurable measurement averaging and internal FIFO. The sensor does also provide temperature measurements.

Sensor does also contain IIR filter implemented in HW. The data-sheet says the IIR filter can be configured to be "weak", "middle" or "strong". Some RMS noise figures are provided in data sheet but no accurate maths for the filter configurations is provided. Hence, the IIR filter configuration is not supported by this driver and the filter is configured to the "middle" setting (at least not for now).

The FIFO measurement mode is only measuring the pressure and not the temperature. The driver measures temperature when FIFO is flushed and simply uses the same measured temperature value to all reported temperatures. This should not be a problem when temperature is not changing very rapidly (several degrees C / second) but allows users to get the temperature measurements from sensor without any additional logic.

This driver allows the sensor to be used in two muitually exclusive ways,

1. With trigger (data-ready IRQ).
In this case the FIFO is not used as we get data ready for each collected sample. Instead, for each data-ready IRQ we read the sample from sensor and push it to the IIO buffer.

2. With hardware FIFO and watermark IRQ.
In this case the data-ready is not used but we enable watermark IRQ. At each watermark IRQ we go and read all samples in FIFO and push them to the IIO buffer.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:

v2 => v3:
- Read temperature only after FIFO is read to overcome a HW quirck
- Drop unused defines
- Allow scanning the pressure only
- Some clarifying comments added, some made less verbose
- warn if measurement stp fails
- use IIO_VAL_FRACTIONAL for pressure scale
- don't disable IRQ but use timestamp from stack
- fix amount of samples to read
- minor styling
- better separate buffer and trigger parts
- allow buffer even when there is no IRQ
with external trigger to be supported.
- add completely, utterly useless NULL check because we have the cycles to waste (grumbles)

drivers/iio/pressure/Makefile +1 -0

index c90f77210e94..436aec7e65f3 100644
@@ -5,6 +5,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ABP060MG) += abp060mg.o
+obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
 obj-$(CONFIG_BMP280) += bmp280.o
 bmp280-objs := bmp280-core.o bmp280-regmap.o
 obj-$(CONFIG_BMP280_I2C) += bmp280-i2c.o

drivers/iio/pressure/rohm-bm1390.c new file +28 -0

new file mode 100644
index 000000000000..82a0cd61d215
 
+
+/*
+ * If the trigger is not used we just wait until the measurement has
+ * completed. The data-sheet says maximum measurement cycle (regardless
+ * the AVE_NUM) is 200 mS so let's just sleep at least that long. If speed
+ * is needed the trigger should be used.
+ */
+#define BM1390_MAX_MEAS_TIME_MS 205
+
+static int bm1390_read_data(struct bm1390_data *data,
+			struct iio_chan_spec const *chan, int *val, int *val2)
+{
+	int ret, warn;
+
+	mutex_lock(&data->mutex);
+	/*
+	 * We use 'continuous mode' even for raw read because according to the
+	 * data-sheet an one-shot mode can't be used with IIR filter.
+	 */
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+	if (ret)
+		goto unlock_out;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		msleep(BM1390_MAX_MEAS_TIME_MS);
+		ret = bm1390_pressure_read(data, val);
+		break;

drivers/iio/pressure/rohm-bm1390.c new file +34 -0

+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
 
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       s64 timestamp)
+{
+	/* BM1390_FIFO_LENGTH is small so we shouldn't run out of stack */
+	struct bm1390_data_buf buffer[BM1390_FIFO_LENGTH];
+	struct bm1390_data *data = iio_priv(idev);
+	int smp_lvl, ret, i, warn, dummy;
+	u64 sample_period;
+	__be16 temp = 0;
+
+	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (ret)
+		return ret;
+
+	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+	if (!smp_lvl)
+		return 0;
+
+	if (smp_lvl > BM1390_FIFO_LENGTH) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = BM1390_FIFO_LENGTH;
+	}
+

drivers/iio/pressure/rohm-bm1390.c new file +33 -0

+	 * to read the all pressure values to memory and read the temperature
+	 * only after that.
+	 */
+	for (i = 0; i < smp_lvl; i++) {
+		/*
+		 * When we start reading data from the FIFO the sensor goes to
+		 * special FIFO reading mode. If any other register is accessed
+		 * during the FIFO read, samples can be dropped. Prevent access
+		 * until FIFO_LVL is read. We have mutex locked and we do also
+		 * go performing reading of FIFO_LVL even if this read fails.
+		 */
+		if (test_bit(BM1390_CHAN_PRESSURE, idev->active_scan_mask)) {
+			ret = bm1390_pressure_read(data, &buffer[i].pressure);
+			if (ret)
+				break;
+		}
+
+		/*
+		 * Old timestamp is either the previous sample IRQ time,
+		 * previous flush-time or, if this was first sample, the enable
+		 * time. When we add a sample period to that we should get the
+		 * best approximation of the time-stamp we are handling.
+		 *
+		 * Idea is to always keep the "old_timestamp" matching the
+		 * timestamp which we are currently handling.
+		 */
+		data->old_timestamp += sample_period;
+		buffer[i].ts = data->old_timestamp;
+	}
+	/* Reading the FIFO_LVL closes the FIFO access sequence */
+	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &dummy);
+	if (warn)
+		dev_warn(data->dev, "Closing FIFO sequence failed\n");

drivers/iio/pressure/rohm-bm1390.c new file +62 -0

+	regval = FIELD_PREP(BM1390_MASK_FIFO_LEN, regval);
+
+	return regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				  BM1390_MASK_FIFO_LEN, regval);
+}
+
+static int bm1390_fifo_enable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	/* We can't do buffered stuff without IRQ as we never get WMI */
+	if (data->irq <= 0)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	if (data->trigger_enabled) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	/* Update watermark to HW */
+	ret = bm1390_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable WMI_IRQ */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
+			      BM1390_MASK_WMI_EN);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable FIFO */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+			      BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BM1390_STATE_FIFO;
+
+	data->old_timestamp = iio_get_time_ns(idev);
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_fifo_disable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	msleep(1);
+
+	mutex_lock(&data->mutex);
+	/* Disable FIFO */
+	ret = regmap_clear_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;

drivers/iio/pressure/rohm-bm1390.c new file +43 -0

+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
 
 
 
+static int bm1390_probe(struct i2c_client *i2c)
+{
+	struct bm1390_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	struct device *dev;
+	unsigned int part_id;
+	int ret;
+
+	dev = &i2c->dev;
+
+	regmap = devm_regmap_init_i2c(i2c, &bm1390_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	ret = regmap_read(regmap, BM1390_REG_PART_ID, &part_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (part_id != BM1390_ID)
+		dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	data = iio_priv(idev);
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = i2c->irq;
+	/*
+	 * For now we just allow BM1390_WMI_MIN to BM1390_WMI_MAX and

drivers/iio/pressure/rohm-bm1390.c new file +23 -0

+
+	ret = bm1390_chip_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "sensor init failed\n");
+
+	ret = bm1390_setup_buffer(data, idev);
+	if (ret)
+		return ret;
+
+	/* No trigger if we don't have IRQ for data-ready and WMI */
+	if (i2c->irq > 0) {
+		ret = bm1390_setup_trigger(data, idev, i2c->irq);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_iio_device_register(dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return 0;
+}

Quoted 31 lines · On Fri, 22 Sep 2023 14:19:10 +0300 Matti Vaittinen <[email protected]>…

Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure pressures ranging from 300 hPa to 1300 hPa with configurable measurement averaging and internal FIFO. The sensor does also provide temperature measurements.

Sensor does also contain IIR filter implemented in HW. The data-sheet says the IIR filter can be configured to be "weak", "middle" or "strong". Some RMS noise figures are provided in data sheet but no accurate maths for the filter configurations is provided. Hence, the IIR filter configuration is not supported by this driver and the filter is configured to the "middle" setting (at least not for now).

The FIFO measurement mode is only measuring the pressure and not the temperature. The driver measures temperature when FIFO is flushed and simply uses the same measured temperature value to all reported temperatures. This should not be a problem when temperature is not changing very rapidly (several degrees C / second) but allows users to get the temperature measurements from sensor without any additional logic.

This driver allows the sensor to be used in two muitually exclusive ways,

1. With trigger (data-ready IRQ).
In this case the FIFO is not used as we get data ready for each collected sample. Instead, for each data-ready IRQ we read the sample from sensor and push it to the IIO buffer.

2. With hardware FIFO and watermark IRQ.
In this case the data-ready is not used but we enable watermark IRQ. At each watermark IRQ we go and read all samples in FIFO and push them to the IIO buffer.

Quoted 1 line

Signed-off-by Matti Vaittinen <[email protected]>

Main question here is whether the fifo mode is useable if now interrupt is wired up?

I'm guessing not really as it's not worth dealing with making it work if someone can't be bothered to connect the wire. In which case I'd expect few more things to be disable if no IRQ.
Also, didn't think we'd have a validate_own_trigger callback set as that means the buffer is potentially also something that should go away if no interrupts are wired.

Anyhow, other than that a few trivial things inline.

Quoted 4 lines

---
Revision history:

Quoted 15 lines

v2 => v3:
- Read temperature only after FIFO is read to overcome a HW quirck
- Drop unused defines
- Allow scanning the pressure only
- Some clarifying comments added, some made less verbose
- warn if measurement stp fails
- use IIO_VAL_FRACTIONAL for pressure scale
- don't disable IRQ but use timestamp from stack
- fix amount of samples to read
- minor styling
- better separate buffer and trigger parts
- allow buffer even when there is no IRQ
with external trigger to be supported.
- add completely, utterly useless NULL check because we have the cycles to waste (grumbles)

)Smiles)

Quoted 50 lines

drivers/iio/pressure/Makefile +1 -0

index c90f77210e94..436aec7e65f3 100644
@@ -5,6 +5,7 @@
 
  # When adding new entries keep the list in alphabetical order
  obj-$(CONFIG_ABP060MG) += abp060mg.o
+obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
  obj-$(CONFIG_BMP280) += bmp280.o
  bmp280-objs := bmp280-core.o bmp280-regmap.o
  obj-$(CONFIG_BMP280_I2C) += bmp280-i2c.o

drivers/iio/pressure/rohm-bm1390.c new file +32 -0

new file mode 100644
index 000000000000..82a0cd61d215
 
+
+/*
+ * If the trigger is not used we just wait until the measurement has
+ * completed. The data-sheet says maximum measurement cycle (regardless
+ * the AVE_NUM) is 200 mS so let's just sleep at least that long. If speed
+ * is needed the trigger should be used.
+ */
+#define BM1390_MAX_MEAS_TIME_MS 205
+
+static int bm1390_read_data(struct bm1390_data *data,
+			struct iio_chan_spec const *chan, int *val, int *val2)
+{
+	int ret, warn;
+
+	mutex_lock(&data->mutex);
+	/*
+	 * We use 'continuous mode' even for raw read because according to the
+	 * data-sheet an one-shot mode can't be used with IIR filter.
+	 */
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+	if (ret)
+		goto unlock_out;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		msleep(BM1390_MAX_MEAS_TIME_MS);
+		ret = bm1390_pressure_read(data, val);
+		break;
+	case IIO_TEMP:
+		msleep(BM1390_MAX_MEAS_TIME_MS);
+		ret = bm1390_read_temp(data, val);
+		break;

drivers/iio/pressure/rohm-bm1390.c line 289

Quoted 6 lines

drivers/iio/pressure/rohm-bm1390.c new file +6 -0

+	default:
+		ret = -EINVAL;
+	}
+	warn = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+	if (warn)
+		dev_warn(data->dev, "Failed to stop measurementi (%d)\n", warn);

measurement

Quoted 40 lines

drivers/iio/pressure/rohm-bm1390.c new file +38 -0

+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
 
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       s64 timestamp)
+{
+	/* BM1390_FIFO_LENGTH is small so we shouldn't run out of stack */
+	struct bm1390_data_buf buffer[BM1390_FIFO_LENGTH];
+	struct bm1390_data *data = iio_priv(idev);
+	int smp_lvl, ret, i, warn, dummy;
+	u64 sample_period;
+	__be16 temp = 0;
+
+	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (ret)
+		return ret;
+
+	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+	if (!smp_lvl)
+		return 0;
+
+	if (smp_lvl > BM1390_FIFO_LENGTH) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = BM1390_FIFO_LENGTH;
+	}
+
+	sample_period = timestamp - data->old_timestamp;
+	do_div(sample_period, smp_lvl);
+
+	if (samples && smp_lvl > samples)

drivers/iio/pressure/rohm-bm1390.c line 375

Quoted 6 lines

drivers/iio/pressure/rohm-bm1390.c new file +6 -0

+		smp_lvl = samples;
+
+
+	/*
+	 * After some testing it appears that the temperature is not readable
+	 * untill the FIFO access has been done after the WMI. Thus, we need

Spell check. until (Why it doesn't have 2 ls is beyond me but that's English being annoyingly irregular)

Quoted 38 lines

drivers/iio/pressure/rohm-bm1390.c new file +37 -0

+	 * to read the all pressure values to memory and read the temperature
+	 * only after that.
+	 */
+	for (i = 0; i < smp_lvl; i++) {
+		/*
+		 * When we start reading data from the FIFO the sensor goes to
+		 * special FIFO reading mode. If any other register is accessed
+		 * during the FIFO read, samples can be dropped. Prevent access
+		 * until FIFO_LVL is read. We have mutex locked and we do also
+		 * go performing reading of FIFO_LVL even if this read fails.
+		 */
+		if (test_bit(BM1390_CHAN_PRESSURE, idev->active_scan_mask)) {
+			ret = bm1390_pressure_read(data, &buffer[i].pressure);
+			if (ret)
+				break;
+		}
+
+		/*
+		 * Old timestamp is either the previous sample IRQ time,
+		 * previous flush-time or, if this was first sample, the enable
+		 * time. When we add a sample period to that we should get the
+		 * best approximation of the time-stamp we are handling.
+		 *
+		 * Idea is to always keep the "old_timestamp" matching the
+		 * timestamp which we are currently handling.
+		 */
+		data->old_timestamp += sample_period;
+		buffer[i].ts = data->old_timestamp;
+	}
+	/* Reading the FIFO_LVL closes the FIFO access sequence */
+	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &dummy);
+	if (warn)
+		dev_warn(data->dev, "Closing FIFO sequence failed\n");
+
+	if (ret)
+		return ret;
+

drivers/iio/pressure/rohm-bm1390.c line 418

Quoted 6 lines

drivers/iio/pressure/rohm-bm1390.c new file +6 -0

+	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
+				       sizeof(temp));
+		if (ret)
+			return ret;
+		pr_info("Temp before reading the FIFO %u\n", be16_to_cpu(temp));

Why this print?

drivers/iio/pressure/rohm-bm1390.c line 427

Quoted 10 lines

drivers/iio/pressure/rohm-bm1390.c new file +9 -0

+	}
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < smp_lvl; i++) {
+		buffer[i].temp = temp;
+		iio_push_to_buffers_with_timestamp(idev, &buffer[i],
+						   buffer[i].ts);

You can just use iio_push_to_buffers() if you've already filled in the timestamp by hand. The _with_timestamp() version was just to reduce boilerplate (and given the main it has caused over the years, I'm not sure it was a good idea!)

Quoted 4 lines

drivers/iio/pressure/rohm-bm1390.c new file +4 -0

+	}
+
+	return smp_lvl;
+}

drivers/iio/pressure/rohm-bm1390.c line 481

Quoted 6 lines

drivers/iio/pressure/rohm-bm1390.c new file +6 -0

+
+static const struct iio_info bm1390_info = {
+	.read_raw = &bm1390_read_raw,
+	.validate_trigger = iio_validate_own_trigger,
+	.hwfifo_set_watermark = bm1390_set_watermark,
+	.hwfifo_flush_to_buffer = bm1390_fifo_flush,

Given my (possibly incorrect assumption) that the fifo is useless without the interrupt, I'd expect to see another version of this that has read_raw only set.

Also, why do we need validate_own_trigger. I thought this could be used with other triggers. If not, then don't register the buffer for the case with no interrupt either.

drivers/iio/pressure/rohm-bm1390.c line 532

Quoted 10 lines

drivers/iio/pressure/rohm-bm1390.c new file +8 -0

+};
+
 
+
+static int bm1390_fifo_set_wmi(struct bm1390_data *data)
+{
+	u8 regval;
+
+	regval = data->watermark - BM1390_WMI_MIN;

Trivial: I'd rather we didn't put stuff that clearly isn't the register
value in a variable called regval. I'd just go directly to

	regval = FIELD_PREP(BM1390_MASK_FIFO_LEN,
			    data->watermark - BMI1390_WMI_MIN);

and avoid that first 'mis'use.

Quoted 69 lines

drivers/iio/pressure/rohm-bm1390.c new file +68 -0

+	regval = FIELD_PREP(BM1390_MASK_FIFO_LEN, regval);
+
+	return regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				  BM1390_MASK_FIFO_LEN, regval);
+}
+
+static int bm1390_fifo_enable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	/* We can't do buffered stuff without IRQ as we never get WMI */
+	if (data->irq <= 0)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	if (data->trigger_enabled) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	/* Update watermark to HW */
+	ret = bm1390_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable WMI_IRQ */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
+			      BM1390_MASK_WMI_EN);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable FIFO */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+			      BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BM1390_STATE_FIFO;
+
+	data->old_timestamp = iio_get_time_ns(idev);
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_fifo_disable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	msleep(1);
+
+	mutex_lock(&data->mutex);
+	/* Disable FIFO */
+	ret = regmap_clear_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BM1390_STATE_SAMPLE;
+
+	/* Disable WMI_IRQ */
+	ret = regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_WMI_EN);

drivers/iio/pressure/rohm-bm1390.c line 604

Quoted 4 lines

drivers/iio/pressure/rohm-bm1390.c new file +4 -0

+	if (ret)
+		goto unlock_out;
+
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);

I'm sure it works in this order but to my mind it would make more sense (and might still work) for fifo_disable() to be reverse of steps in fifo_enable(). So I'd expect the mode change first.

Quoted 52 lines

drivers/iio/pressure/rohm-bm1390.c new file +49 -0

+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
 
 
 
+static int bm1390_probe(struct i2c_client *i2c)
+{
+	struct bm1390_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	struct device *dev;
+	unsigned int part_id;
+	int ret;
+
+	dev = &i2c->dev;
+
+	regmap = devm_regmap_init_i2c(i2c, &bm1390_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	ret = regmap_read(regmap, BM1390_REG_PART_ID, &part_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (part_id != BM1390_ID)
+		dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	data = iio_priv(idev);
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = i2c->irq;
+	/*
+	 * For now we just allow BM1390_WMI_MIN to BM1390_WMI_MAX and
+	 * discard every other configuration when triggered mode is not used.
+	 */
+	data->watermark = BM1390_WMI_MAX;
+	mutex_init(&data->mutex);
+
+	idev->channels = bm1390_channels;

drivers/iio/pressure/rohm-bm1390.c line 876

Quoted 4 lines

drivers/iio/pressure/rohm-bm1390.c new file +4 -0

+	idev->num_channels = ARRAY_SIZE(bm1390_channels);
+	idev->name = "bm1390";
+	idev->info = &bm1390_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;

Silly question that I might resolve as I read on.
If we don't have the WMI interrupt, do we have buffer_software support? It could be made to work with a timer, but do you do so?

Quoted 24 lines

drivers/iio/pressure/rohm-bm1390.c new file +23 -0

+
+	ret = bm1390_chip_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "sensor init failed\n");
+
+	ret = bm1390_setup_buffer(data, idev);
+	if (ret)
+		return ret;
+
+	/* No trigger if we don't have IRQ for data-ready and WMI */
+	if (i2c->irq > 0) {
+		ret = bm1390_setup_trigger(data, idev, i2c->irq);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_iio_device_register(dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return 0;
+}

ROHM BM1390 Pressure sensor (BM1390GLV-Z) can measure pressures ranging from 300 hPa to 1300 hPa with configurable measurement averaging and an internal FIFO. The sensor does also provide temperature measurements although, according to the data sheet, sensor performs internal temperature compensation for the MEMS.

Sensor does also contain IIR filter implemented in HW. The data-sheet says the IIR filter can be configured to be "weak", "middle" or "strong". Some RMS noise figures are provided in data sheet but no accurate maths for the filter configurations is provided.

I actually asked if we can define 3db frequencies corresponding to these IIR filter settings - and I received values 0.452Hz, 0.167Hz, and 0.047Hz but I am not at all sure we understood each others with the HW colleagues... Hence, the IIR filter configuration is not supported by this driver and the filter is just configured to the "middle" setting. (at least for now)

It would also be possible to not use IIR filter but just do some simple averaging. I wonder if it would make sense to implement the OVERSAMPLING value setting so that if this value is written, IIR filter is disabled and number of samples to be averaged is set to value requested by OVERSAMPLING. The data-sheet has a mention that if IIR is used, the number of averaged samples must be set to a fixed value.

The FIFO measurement mode (in sensor hardware) is only measuring the pressure and not the temperature. The driver measures temperature when FIFO is flushed and simply uses the same measured temperature value to all reported temperatures. This should not be a problem when temperature is not changing very rapidly (several degrees C / second) but allows users to get the temperature measurements from sensor without any additional logic.

This driver has received limited amount of testing this far. It's in a state 'works on my machine, for my use cases' - and all feedback is appreciated!

Revision history:
Major changes here, please see the head room of individual patches for more detailed list.
v2 => v3:

	rebased on v6.6-rc2

added three IIO fixup patches so numbering of patches changed dt-bindings/MAINTAINERS: No changes
bm1390 driver:
- various cleanups and fixes
- do not disable IRQ
- fix temperature reading when FIFO is used
- separate buffer and trigger initialization

v1 => v2:
rebased on v6.6-rc1
dt-bindings:
- fix compatible in the example
sensor driver:
- drop unnecessary write_raw callback
- plenty of small improvements and fixes
MAINTAINERS:
- No changes

Matti Vaittinen (6):
tools: iio: iio_generic_buffer ensure alignment
iio: improve doc for available_scan_mask
iio: try searching for exact scan_mask
dt-bindings: Add ROHM BM1390 pressure sensor
iio: pressure: Support ROHM BU1390
MAINTAINERS: Add ROHM BM1390

 .../bindings/iio/pressure/rohm,bm1390.yaml    |  52 +
 MAINTAINERS                                   |   6 +
 drivers/iio/industrialio-buffer.c             |  25 +-
 drivers/iio/pressure/Kconfig                  |   9 +
 drivers/iio/pressure/Makefile                 |   1 +
 drivers/iio/pressure/rohm-bm1390.c            | 930 ++++++++++++++++++
 include/linux/iio/iio.h                       |   4 +-
 tools/iio/iio_generic_buffer.c                |  15 +-
 8 files changed, 1034 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
 create mode 100644 drivers/iio/pressure/rohm-bm1390.c

base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70

Quoted 6 lines · On Fri, 22 Sep 2023 14:14:52 +0300 Matti Vaittinen <[email protected]>…

Revision history:
Major changes here, please see the head room of individual patches for more detailed list.
v2 => v3:

	rebased on v6.6-rc2

Stick to rc1 though I doubt it makes much difference.

The IIO tree
will remain based on v6.6-rc1 until it is rebased after Greg takes a pull request.