[PATCH v3 0/6] Support ROHM BM1390 pressure sensor

[PATCH v3 0/6] Support ROHM BM1390 pressure sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3979 bytes --]

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
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

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 <mazziesaccount@gmail.com>
---
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(-)

diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 44bbf80f0cfd..fc562799a109 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -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++;
 	}
+	/*
+	 * 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?
+	 */
+	misalignment = bytes % max;
+	if (misalignment) {
+		printf("Misalignment %u. Adding Padding %u\n", misalignment,  max - misalignment);
+		bytes += max - misalignment;
+	}
 
 	return bytes;
 }
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

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(-)

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
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/6] iio: try searching for exact scan_mask

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

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 <mazziesaccount@gmail.com>
---
 drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 176d31d9f9d8..e97396623373 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -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,
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 4/6] dt-bindings: Add ROHM BM1390 pressure sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2630 bytes --]

BM1390GLV-Z is a pressure sensor which performs internal temperature
compensation for the MEMS. Pressure range is from 300 hPa to 1300 hPa
and sample averaging and IIR filtering is built in sensor. Temperature
measurement is also supported.

Add dt-bindings for the sensor.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

---
Revision history:
v1 => v2:
- Improve spelling in description
- Drop unnecessary '|'
- Use correct example (fix compatible)
---
 .../bindings/iio/pressure/rohm,bm1390.yaml    | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml b/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
new file mode 100644
index 000000000000..7c4ca6322bf8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/rohm,bm1390.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BM1390 pressure sensor
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description:
+  BM1390GLV-Z is a pressure sensor which performs internal temperature
+  compensation for the MEMS. Pressure range is from 300 hPa to 1300 hPa
+  and sample averaging and IIR filtering is built in. Temperature
+  measurement is also supported.
+
+properties:
+  compatible:
+    const: rohm,bm1390glv-z
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pressure-sensor@5d {
+            compatible = "rohm,bm1390glv-z";
+            reg = <0x5d>;
+
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
+
+            vdd-supply = <&vdd>;
+        };
+    };
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 5/6] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 29878 bytes --]

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 <mazziesaccount@gmail.com>

---
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)

v1 => v2:
- prefer s64 over int64_t
- drop not needed handling of 2's complements
- plenty of styling changes
- drop dead code (write_raw)
- fix typos in comments
- explain trigger and FIFO usage in commit message
- do better job at cheking the return values
- ensure there's no race when checking if triggered buffer is used
  before enabling the FIFO
- print warning if register read fails at IRQ handler
- drop unnecessary warning if IRQ is not given
- explain why we prefer asynchronous probing
---
 drivers/iio/pressure/Kconfig       |   9 +
 drivers/iio/pressure/Makefile      |   1 +
 drivers/iio/pressure/rohm-bm1390.c | 930 +++++++++++++++++++++++++++++
 3 files changed, 940 insertions(+)
 create mode 100644 drivers/iio/pressure/rohm-bm1390.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 7b4c2af32852..95efa32e4289 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -16,6 +16,15 @@ config ABP060MG
 	  To compile this driver as a module, choose M here: the module
 	  will be called abp060mg.
 
+config ROHM_BM1390
+	tristate "ROHM BM1390GLV-Z pressure sensor driver"
+	depends on I2C
+	help
+	  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.
+
 config BMP280
 	tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor driver"
 	depends on (I2C || SPI_MASTER)
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index c90f77210e94..436aec7e65f3 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -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
diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
new file mode 100644
index 000000000000..82a0cd61d215
--- /dev/null
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -0,0 +1,930 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * BM1390 ROHM pressure sensor
+ *
+ * Copyright (c) 2023, ROHM Semiconductor.
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/pressure/bm1390glv-z-e.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define BM1390_REG_MANUFACT_ID		0x0f
+#define BM1390_REG_PART_ID		0x10
+#define BM1390_REG_POWER		0x12
+#define BM1390_MASK_POWER		BIT(0)
+#define BM1390_POWER_ON			BM1390_MASK_POWER
+#define BM1390_POWER_OFF		0x00
+#define BM1390_REG_RESET		0x13
+#define BM1390_MASK_RESET		BIT(0)
+#define BM1390_RESET_RELEASE		BM1390_MASK_RESET
+#define BM1390_RESET			0x00
+#define BM1390_REG_MODE_CTRL		0x14
+#define BM1390_MASK_MEAS_MODE		GENMASK(1, 0)
+#define BM1390_MASK_DRDY_EN		BIT(4)
+#define BM1390_MASK_WMI_EN		BIT(2)
+#define BM1390_MASK_AVE_NUM		GENMASK(7, 5)
+
+/*
+ * Data-sheet states that when the IIR is used, the AVE_NUM must be set to
+ * value 110b
+ */
+#define BM1390_IIR_AVE_NUM		0x06
+#define BM1390_REG_FIFO_CTRL		0x15
+#define BM1390_MASK_IIR_MODE		GENMASK(1, 0)
+#define BM1390_IIR_MODE_OFF		0x0
+#define BM1390_IIR_MODE_WEAK		0x1
+#define BM1390_IIR_MODE_MID		0x2
+#define BM1390_IIR_MODE_STRONG		0x3
+
+#define BM1390_MASK_FIFO_LEN		BIT(6)
+#define BM1390_MASK_FIFO_EN		BIT(7)
+#define BM1390_WMI_MIN			2
+#define BM1390_WMI_MAX			3
+
+#define BM1390_REG_FIFO_LVL		0x18
+#define BM1390_MASK_FIFO_LVL		GENMASK(2, 0)
+#define BM1390_REG_STATUS		0x19
+#define BM1390_REG_PRESSURE_BASE	0x1a
+#define BM1390_REG_TEMP_HI		0x1d
+#define BM1390_REG_TEMP_LO		0x1e
+#define BM1390_MAX_REGISTER		BM1390_REG_TEMP_LO
+
+#define BM1390_ID			0x34
+
+/* Regmap configs */
+static const struct regmap_range bm1390_volatile_ranges[] = {
+	{
+		.range_min = BM1390_REG_STATUS,
+		.range_max = BM1390_REG_STATUS,
+	},
+	{
+		.range_min = BM1390_REG_FIFO_LVL,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_volatile_regs = {
+	.yes_ranges = &bm1390_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_volatile_ranges),
+};
+
+static const struct regmap_range bm1390_precious_ranges[] = {
+	{
+		.range_min = BM1390_REG_STATUS,
+		.range_max = BM1390_REG_STATUS,
+	},
+};
+
+static const struct regmap_access_table bm1390_precious_regs = {
+	.yes_ranges = &bm1390_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_precious_ranges),
+};
+
+static const struct regmap_range bm1390_read_only_ranges[] = {
+	{
+		.range_min = BM1390_REG_MANUFACT_ID,
+		.range_max = BM1390_REG_PART_ID,
+	}, {
+		.range_min = BM1390_REG_FIFO_LVL,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_ro_regs = {
+	.no_ranges = &bm1390_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(bm1390_read_only_ranges),
+};
+
+static const struct regmap_range bm1390_noinc_read_ranges[] = {
+	{
+		.range_min = BM1390_REG_PRESSURE_BASE,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_nir_regs = {
+	.yes_ranges = &bm1390_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_noinc_read_ranges),
+};
+
+static const struct regmap_config bm1390_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &bm1390_volatile_regs,
+	.wr_table = &bm1390_ro_regs,
+	.rd_noinc_table = &bm1390_nir_regs,
+	.precious_table = &bm1390_precious_regs,
+	.max_register = BM1390_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+	.disable_locking = true,
+};
+
+enum {
+	BM1390_STATE_SAMPLE,
+	BM1390_STATE_FIFO,
+};
+
+struct bm1390_data_buf {
+	u32 pressure;
+	__be16 temp;
+	s64 ts __aligned(8);
+};
+
+/* BM1390 has FIFO for 4 pressure samples */
+#define BM1390_FIFO_LENGTH	4
+
+struct bm1390_data {
+	s64 timestamp, old_timestamp;
+	struct iio_trigger *trig;
+	struct regmap *regmap;
+	struct device *dev;
+	struct bm1390_data_buf buf;
+	int irq;
+	unsigned int state;
+	bool trigger_enabled;
+	u8 watermark;
+
+	/* Prevent accessing sensor during FIFO read sequence */
+	struct mutex mutex;
+};
+
+enum {
+	BM1390_CHAN_PRESSURE,
+	BM1390_CHAN_TEMP,
+};
+
+static const struct iio_chan_spec bm1390_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		/*
+		 * When IIR is used, we must fix amount of averaged samples.
+		 * Thus we don't allow setting oversampling ratio.
+		 */
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = BM1390_CHAN_PRESSURE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 22,
+			.storagebits = 32,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = BM1390_CHAN_TEMP,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+/*
+ * We can't skip reading the pressure because the watermark IRQ is acked
+ * only when the pressure data is read from the FIFO.
+ */
+static const unsigned long bm1390_scan_masks[] = {
+	BIT(BM1390_CHAN_PRESSURE),
+	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
+	0
+};
+
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	__be16 temp_raw;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+			       sizeof(temp_raw));
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpu(temp_raw);
+
+	return 0;
+}
+
+static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
+{
+	/* Pressure data is in 3 8-bit registers */
+	u8 raw[3];
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+			       raw, sizeof(raw));
+	if (ret < 0)
+		return ret;
+
+	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
+
+	return 0;
+}
+
+ /* The enum values map directly to register bits */
+enum bm1390_meas_mode {
+	BM1390_MEAS_MODE_STOP = 0x0,
+	BM1390_MEAS_MODE_1SHOT = 0x1,
+	BM1390_MEAS_MODE_CONTINUOUS = 0x2,
+};
+
+static int bm1390_meas_set(struct bm1390_data *data, enum bm1390_meas_mode mode)
+{
+	return regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				  BM1390_MASK_MEAS_MODE, mode);
+}
+
+/*
+ * 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;
+	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);
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			*val = 31;
+			*val2 = 250000;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		} else if (chan->type == IIO_PRESSURE) {
+			/*
+			 * pressure in hPa is register value divided by 2048.
+			 * This means kPa is 1/20480 times the register value,
+			 */
+			*val = 1;
+			*val2 = 2048;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		ret = bm1390_read_data(data, chan, val, val2);
+		iio_device_release_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+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)
+		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
+	 * 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;
+
+	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));
+	}
+
+	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);
+	}
+
+	return smp_lvl;
+}
+
+static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	s64 timestamp;
+	int ret;
+
+	/*
+	 * If fifo_flush is being called from IRQ handler we know the stored
+	 * timestamp is fairly accurate for the last stored sample. If we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 * We need to have the IRQ disabled or we risk of messing-up
+	 * the timestamps. If we are ran from IRQ, then the
+	 * IRQF_ONESHOT has us covered - but if we are ran by the
+	 * user-space read we need to disable the IRQ to be on a safe
+	 * side. We do this usng synchronous disable so that if the
+	 * IRQ thread is being ran on other CPU we wait for it to be
+	 * finished.
+	 */
+
+	timestamp = iio_get_time_ns(idev);
+	mutex_lock(&data->mutex);
+	ret = __bm1390_fifo_flush(idev, samples, timestamp);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_set_watermark(struct iio_dev *idev, unsigned int val)
+{
+	struct bm1390_data *data = iio_priv(idev);
+
+	if (val < BM1390_WMI_MIN || val > BM1390_WMI_MAX)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 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,
+};
+
+static int bm1390_chip_init(struct bm1390_data *data)
+{
+	int ret;
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_POWER,
+				BM1390_MASK_POWER, BM1390_POWER_ON);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
+				BM1390_MASK_RESET, BM1390_RESET);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
+				BM1390_MASK_RESET, BM1390_RESET_RELEASE);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_reinit_cache(data->regmap, &bm1390_regmap);
+	if (ret) {
+		dev_err(data->dev, "Failed to reinit reg cache\n");
+		return ret;
+	}
+
+	/*
+	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
+	 * be fixed when IIR is in use.
+	 */
+	ret = regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				  BM1390_MASK_IIR_MODE, BM1390_IIR_MODE_MID);
+}
+
+static int bm1390_fifo_set_wmi(struct bm1390_data *data)
+{
+	u8 regval;
+
+	regval = data->watermark - BM1390_WMI_MIN;
+	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);
+	if (ret)
+		goto unlock_out;
+
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_buffer_postenable(struct iio_dev *idev)
+{
+	/*
+	 * If we use data-ready trigger, then the IRQ masks should be handled by
+	 * trigger enable and the hardware buffer is not used but we just update
+	 * results to the IIO FIFO when data-ready triggers.
+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bm1390_fifo_enable(idev);
+}
+
+static int bm1390_buffer_predisable(struct iio_dev *idev)
+{
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bm1390_fifo_disable(idev);
+}
+
+static const struct iio_buffer_setup_ops bm1390_buffer_ops = {
+	.postenable = bm1390_buffer_postenable,
+	.predisable = bm1390_buffer_predisable,
+};
+
+static irqreturn_t bm1390_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret, status;
+
+	/* DRDY is acked by reading status reg */
+	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
+	if (ret || !status)
+		return IRQ_NONE;
+
+	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
+
+	if (test_bit(BM1390_CHAN_PRESSURE, idev->active_scan_mask)) {
+		ret = bm1390_pressure_read(data, &data->buf.pressure);
+		if (ret) {
+			dev_warn(data->dev, "sample read failed %d\n", ret);
+			return IRQ_NONE;
+		}
+	}
+
+	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
+				       &data->buf.temp, sizeof(data->buf.temp));
+		if (ret) {
+			dev_warn(data->dev, "temp read failed %d\n", ret);
+			return IRQ_HANDLED;
+		}
+	}
+
+	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t bm1390_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bm1390_data *data = iio_priv(idev);
+
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->state == BM1390_STATE_FIFO || data->trigger_enabled)
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t bm1390_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled) {
+		iio_trigger_poll_nested(data->trig);
+		ret = IRQ_HANDLED;
+	} else if (data->state == BM1390_STATE_FIFO) {
+		int ok;
+
+		ok = __bm1390_fifo_flush(idev, BM1390_FIFO_LENGTH,
+					 data->timestamp);
+		if (ok > 0)
+			ret = IRQ_HANDLED;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_set_drdy_irq(struct bm1390_data *data, bool en)
+{
+	if (en)
+		return regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				       BM1390_MASK_DRDY_EN);
+	return regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_DRDY_EN);
+}
+
+static int bm1390_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct bm1390_data *data = iio_trigger_get_drvdata(trig);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state == BM1390_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	data->trigger_enabled = state;
+
+	if (state) {
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+		if (ret)
+			goto unlock_out;
+	} else {
+		int dummy;
+
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+		if (ret)
+			goto unlock_out;
+
+		/*
+		 * We need to read the status register in order to ACK the
+		 * data-ready which may have been generated just before we
+		 * disabled the measurement.
+		 */
+		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
+		if (ret)
+			dev_warn(data->dev, "status read failed\n");
+	}
+
+	ret = bm1390_set_drdy_irq(data, state);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops bm1390_trigger_ops = {
+	.set_trigger_state = bm1390_trigger_set_state,
+};
+
+static int bm1390_setup_buffer(struct bm1390_data *data, struct iio_dev *idev)
+{
+	int ret;
+
+	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
+					      &iio_pollfunc_store_time,
+					      &bm1390_trigger_handler,
+					      &bm1390_buffer_ops);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup FAIL\n");
+
+	idev->available_scan_masks = bm1390_scan_masks;
+
+	return 0;
+}
+
+static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
+				int irq)
+{
+	struct iio_trigger *itrig;
+	char *name;
+	int ret;
+
+	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!itrig)
+		return -ENOMEM;
+
+	data->trig = itrig;
+
+	itrig->ops = &bm1390_trigger_ops;
+	iio_trigger_set_drvdata(itrig, data);
+
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+			      dev_name(data->dev));
+	if (name == NULL)
+		return -ENOMEM;
+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;
+}
+
+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;
+	idev->num_channels = ARRAY_SIZE(bm1390_channels);
+	idev->name = "bm1390";
+	idev->info = &bm1390_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+
+	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;
+}
+
+static const struct of_device_id bm1390_of_match[] = {
+	{ .compatible = "rohm,bm1390glv-z" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bm1390_of_match);
+
+static const struct i2c_device_id bm1390_id[] = {
+	{ "bm1390glv-z", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, bm1390_id);
+
+static struct i2c_driver bm1390_driver = {
+	.driver = {
+		.name = "bm1390",
+		.of_match_table = bm1390_of_match,
+		/*
+		 * Probing explicitly requires a few millisecond of sleep.
+		 * Enabling the VDD regulator may include ramp up rates.
+		 */
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = bm1390_probe,
+	.id_table = bm1390_id,
+};
+module_i2c_driver(bm1390_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Driver for ROHM BM1390 pressure sensor");
+MODULE_LICENSE("GPL");
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 6/6] MAINTAINERS: Add ROHM BM1390

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

Add myself as a maintainer for ROHM BM1390 pressure sensor driver.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bf0f54c24f81..2f296320d642 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18562,6 +18562,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/bh1750.yaml
 F:	drivers/iio/light/bh1750.c
 
+ROHM BM1390 PRESSURE SENSOR DRIVER
+M:	Matti Vaittinen <mazziesaccount@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/pressure/rohm-bm1390.c
+
 ROHM BU270xx LIGHT SENSOR DRIVERs
 M:	Matti Vaittinen <mazziesaccount@gmail.com>
 L:	linux-iio@vger.kernel.org
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/6] Support ROHM BM1390 pressure sensor

[Jonathan Cameron]

On Fri, 22 Sep 2023 14:14:52 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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!

At somepoint we'll just have to decide it's enough. To be honest most
drivers in IIO get testing along those lines and we find bugs years
later when someone tries something a little different!

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

Jonathan

> 	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

Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Jonathan Cameron]

On Fri, 22 Sep 2023 14:16:08 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>
> ---
> 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(-)
> 
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 44bbf80f0cfd..fc562799a109 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -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++;
>  	}
> +	/*
> +	 * 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. 

> +	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!

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

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

[Jonathan Cameron]

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

> 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
>   * @masklength:		[INTERN] the length of the mask established from
>   *			channels
>   * @active_scan_mask:	[INTERN] union of all scan masks requested by buffers

Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

[Jonathan Cameron]

On Fri, 22 Sep 2023 14:17:49 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>

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


> ---
>  drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 176d31d9f9d8..e97396623373 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -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,

Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

[Jonathan Cameron]

On Sun, 24 Sep 2023 17:07:26 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 22 Sep 2023 14:17:49 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > 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 <mazziesaccount@gmail.com>  
> 
> 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.

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.

Jonathan

> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 176d31d9f9d8..e97396623373 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -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,  
> 

Re: [PATCH v3 5/6] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Fri, 22 Sep 2023 14:19:10 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>

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.

Jonathan


> 
> ---
> 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)

)Smiles)



> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> index c90f77210e94..436aec7e65f3 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -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
> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
> new file mode 100644
> index 000000000000..82a0cd61d215
> --- /dev/null
> +++ b/drivers/iio/pressure/rohm-bm1390.c

> +
> +/*
> + * 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;
> +	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

> +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)
> +		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)

> +	 * 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;
> +
> +	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? 

> +	}
> +
> +	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!)
> +	}
> +
> +	return smp_lvl;
> +}

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

> +};
> +

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

> +	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);
> +	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.
> +
> +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;
> +	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?

> +
> +	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;
> +}

Re: [PATCH v3 0/6] Support ROHM BM1390 pressure sensor

[Matti Vaittinen]

On 9/24/23 18:53, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:14:52 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> 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.

It shouldn't probably change much - but it 's still good to know -rc1 is 
what you'll stick at :) Thanks for pointing it out.

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

Yours,
	--Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Matti Vaittinen]

On 9/24/23 18:57, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:16:08 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
>> index 44bbf80f0cfd..fc562799a109 100644
>> --- a/tools/iio/iio_generic_buffer.c
>> +++ b/tools/iio/iio_generic_buffer.c
>> @@ -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++;
>>   	}
>> +	/*
>> +	 * 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)

> 
>> +	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!

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

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

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

[Matti Vaittinen]

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
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

[Matti Vaittinen]

On 9/24/23 19:07, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:17:49 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
> 
> 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.

It's always good to hear there is not many problems found :) This patch 
was not inspired by auditing the existing code - it was inspired by the 
fact that I would have wrongly ordered the available_scan_masks for 
bm1390 myself. I just happened to notice the oddity in active_scan_masks 
while I was trying to figure out if it was the driver, IIO or user-space 
code which messed my buffer when I disabled timestamps.

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

I think I agree, although I could argue that it depends on the amount of 
added complexity. Still ...

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

... this suggestion is superior to the check added in this patch.

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

Yes. And running the check at the registration phase should not be a big 
problem. And, if it appears to be a problem, then we can add a 
registration variant which omits the checks for those rare drivers which 
would _really_ be hurt by the few extra cycles spent on registration.

> That would incorporate ensuring exact matches come earlier by default.

Yes. I like the idea, wish I had invented it myself ;)

> 
> Jonathan
> 
> 
>> ---
>>   drivers/iio/industrialio-buffer.c | 25 +++++++++++++++++++------
>>   1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 176d31d9f9d8..e97396623373 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -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,
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 3/6] iio: try searching for exact scan_mask

[Matti Vaittinen]

On 9/24/23 19:10, Jonathan Cameron wrote:
> On Sun, 24 Sep 2023 17:07:26 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Fri, 22 Sep 2023 14:17:49 +0300
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>>> 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 <mazziesaccount@gmail.com>
>>
>> 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.
> 
> 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.

I was not so worried about the driver being postponed. I was prepared to 
suggest to merging a subset of the patches if need be - while I can 
continue work with the rest of the series ;)

What comes to people being interested in the core-changes Vs. people 
being interested in the driver changes - I'd expect the core changes to 
concern much wider audience than the driver changes. But yes, knowing 
the amount of mails people go through, limiting the recipient to most 
relevant ones never hurts. Besides, I think there is no 
conflicts/dependencies as driver changes don't change core/tools, and 
core/tool changes don't touch the driver so splitting this to two series 
should be trivial. Will do that for next version.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 5/6] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/24/23 19:29, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:19:10 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
> 
> Main question here is whether the fifo mode is useable if now interrupt
> is wired up? 

I don't think it is.

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

This was what I originally had been thinking. You made me think that we 
should perhaps allow using an external trigger as well - and I tried 
doing that (but obviously didn't test it as I forgot the 
validate_own_trigger in place). I don't really know the potential 
applications for this sensor so I can't imagine if it'd be useful to 
support external triggers. Still, if it is just a matter of dropping the 
validate_own_trigger - then, why limiting the users?

> Anyhow, other than that a few trivial things inline.
> 
> 
>>
>> ---
>> 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)
> 
> )Smiles)
> 
> 
> 
>> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
>> index c90f77210e94..436aec7e65f3 100644
>> --- a/drivers/iio/pressure/Makefile
>> +++ b/drivers/iio/pressure/Makefile
>> @@ -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
>> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
>> new file mode 100644
>> index 000000000000..82a0cd61d215
>> --- /dev/null
>> +++ b/drivers/iio/pressure/rohm-bm1390.c
> 
>> +
>> +/*
>> + * 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;
>> +	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

Thanks! Seems like an useless vim 'insert' mode change ;)

> 
>> +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)
>> +		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)

Thanks. I keep mistyping it. I think I've seen checkpatch warnings on 
this kind of mistakes before. Makes me wonder if I forgot to run the 
checkpatch after latest modifications...

> 
>> +	 * 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;
>> +
>> +	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?

Thanks! Forgot a debug phase print here...

> 
>> +	}
>> +
>> +	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!)

Right. I had a brainfart on this one. Thought that the 
iio_push_to_buffers_with_timestamp() would help in case where timestamp 
was disabled by the user. Now I see this thought was very much not 
correct. That's what I get when writing code at the afternoon, and 
especially at Friday afternoon :) My brains are shutting down early :)

>> +	}
>> +
>> +	return smp_lvl;
>> +}
> 
>> +
>> +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.

Yes...

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

... and yes. Thank you :)

> 
>> +};
>> +
> 
>> +
>> +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.

Ok.

> 
>> +	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);
>> +	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.

I think stopping the masurement should indeed be done first. I'm not 
sure why the order is this. I know I tried suffling the order of 
starting the measurement while trying to figure out the temperature 
measurements for first FIFO sample - but I don't think I touched the 
disabling. So, it has probably been like this from the v1. I'll revise 
this, thanks!

>> +
>> +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;
>> +	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?

No. I can't say the exact meaning of all these flags is 100% clear to me 
- but I think we only have the INDIO_BUFFER_TRIGGERED if we don't have 
the IRQ. Thanks!

> 
>> +
>> +	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;
>> +}
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Jonathan Cameron]

On Mon, 25 Sep 2023 10:01:09 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/24/23 18:57, Jonathan Cameron wrote:
> > On Fri, 22 Sep 2023 14:16:08 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> 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 <mazziesaccount@gmail.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> >> index 44bbf80f0cfd..fc562799a109 100644
> >> --- a/tools/iio/iio_generic_buffer.c
> >> +++ b/tools/iio/iio_generic_buffer.c
> >> @@ -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++;
> >>   	}
> >> +	/*
> >> +	 * 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);
> ?

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

> 
> >   
> >> +	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!
> 
> >   
> >> +		bytes += max - misalignment;
> >> +	}
> >>   
> >>   	return bytes;
> >>   }  
> >   
> 
> Yours,
> 	-- Matti
> 

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

[Jonathan Cameron]

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

Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Matti Vaittinen]

On 9/25/23 16:16, Jonathan Cameron wrote:
> On Mon, 25 Sep 2023 10:01:09 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 9/24/23 18:57, Jonathan Cameron wrote:
>>> On Fri, 22 Sep 2023 14:16:08 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> 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 <mazziesaccount@gmail.com>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
>>>> index 44bbf80f0cfd..fc562799a109 100644
>>>> --- a/tools/iio/iio_generic_buffer.c
>>>> +++ b/tools/iio/iio_generic_buffer.c
>>>> @@ -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++;
>>>>    	}
>>>> +	/*
>>>> +	 * 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);
>> ?
> 
> 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.

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

>>>> +	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!
>>
>>>    
>>>> +		bytes += max - misalignment;
>>>> +	}
>>>>    
>>>>    	return bytes;
>>>>    }
>>>    

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Jonathan Cameron]

On Tue, 26 Sep 2023 13:29:02 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/25/23 16:16, Jonathan Cameron wrote:
> > On Mon, 25 Sep 2023 10:01:09 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 9/24/23 18:57, Jonathan Cameron wrote:  
> >>> On Fri, 22 Sep 2023 14:16:08 +0300
> >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>>      
> >>>> 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 <mazziesaccount@gmail.com>
> >>>> ---
> >>>> 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(-)
> >>>>
> >>>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> >>>> index 44bbf80f0cfd..fc562799a109 100644
> >>>> --- a/tools/iio/iio_generic_buffer.c
> >>>> +++ b/tools/iio/iio_generic_buffer.c
> >>>> @@ -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++;
> >>>>    	}
> >>>> +	/*
> >>>> +	 * 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);
> >> ?  
> > 
> > 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.
Ah I read that backwards as you've noticed.

We don't have any such big channels, so indeed have some flexibility here.
I think we stick to naturally aligned so 16 bytes case would be 16 byte
aligned - mostly because I don't expect to see one any time soon and
because it makes the docs simpler.

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

Indeed - it's meant to be power of 2 only. More than possible we don't check
that rigorously enough or have it clearly documented though.

The aim is for the data to be padded for efficient accesses
+ because it is a pain to deal with arbitrary padding so we restrict
it to power of 2 naturally aligned only.  One relaxation we've talked
about in the past is packing multiple channels per byte (for logic
analyser cases). Not done it yet though.

> 
> 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 :)

16 byte alignement is probably not needed - but who knows for future
architecture. Note this has been really non obvious in the past and
is why we force alignment for timestamp elements in lots of drivers.

Most architectures align an s64 in a c structure to an 8 byte boundary
but x86 32 bit doesn't.  Hence we have to force it all over the place.

Fixing that (because userspace ABI data placement should not vary across
architectures) was a pain. I don't want to do it again!

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

Might be worth a power of 2 only comment.

J