[PATCH 0/3] Support ROHM BM1390 pressure sensor

[PATCH 0/3] Support ROHM BM1390 pressure sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3016 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!

Matti Vaittinen (3):
  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/pressure/Kconfig                  |   9 +
 drivers/iio/pressure/Makefile                 |   1 +
 drivers/iio/pressure/rohm-bm1390.c            | 944 ++++++++++++++++++
 5 files changed, 1012 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
 create mode 100644 drivers/iio/pressure/rohm-bm1390.c


base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
-- 
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 1/3] dt-bindings: Add ROHM BM1390 pressure sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2444 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>
---
 .../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..d681fdd0f5ea
--- /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 sensor. 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 = "kionix,kx022a";
+            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 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 28017 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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/pressure/Kconfig       |   9 +
 drivers/iio/pressure/Makefile      |   1 +
 drivers/iio/pressure/rohm-bm1390.c | 944 +++++++++++++++++++++++++++++
 3 files changed, 954 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..2d51a99a58fc
--- /dev/null
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -0,0 +1,944 @@
+// 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);
+};
+
+/* Pressure data is in 3 8-bit registers */
+#define BM1390_PRESSURE_SIZE	3
+
+/* BM1390 has FIFO for 4 pressure samples */
+#define BM1390_FIFO_LENGTH	4
+
+/* Temperature data is in 2 8-bit registers */
+#define BM1390_TEMP_SIZE	2
+
+struct bm1390_data {
+	int64_t 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),
+};
+
+static const unsigned long bm1390_scan_masks[] = {
+	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
+};
+
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	u8 temp_reg[2] __aligned(2);
+	__be16 *temp_raw;
+	int ret;
+	s16 val;
+	bool negative;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
+			       sizeof(temp_reg));
+	if (ret)
+		return ret;
+
+	if (temp_reg[0] & 0x80)
+		negative = true;
+	else
+		negative = false;
+
+	temp_raw = (__be16 *)&temp_reg[0];
+	val = be16_to_cpu(*temp_raw);
+
+	if (negative) {
+		/*
+		 * Two's complement. I am not sure if all supported
+		 * architectures actually use 2's complement represantation of
+		 * signed ints. If yes, then we could just do the endianes
+		 * conversion and say this is the s16 value. However, as I
+		 * don't know, and as the conversion is pretty simple. let's
+		 * just convert the signed 2's complement to absolute value and
+		 * multiply by -1.
+		 */
+		val = ~val + 1;
+		val *= -1;
+	}
+
+	*temp = val;
+
+	return 0;
+}
+
+static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
+{
+	int ret;
+	u8 raw[3];
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+			       &raw[0], 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,
+	BM1390_MEAS_MODE_CONTINUOUS,
+};
+
+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;
+
+	*val2 = 0;
+
+	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;
+	}
+	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+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) {
+			*val = 0;
+			/*
+			 * pressure in hPa is register value divided by 2048.
+			 * This means kPa is 1/20480 times the register value,
+			 * which equals to 48828.125 * 10 ^ -9
+			 * This is 48828.125 nano kPa.
+			 *
+			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
+			 * get 48828 - which means we lose some accuracy. Well,
+			 * let's try to live with that.
+			 */
+			*val2 = 48828;
+
+			return IIO_VAL_INT_PLUS_NANO;
+		}
+
+		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 IIO_VAL_INT;
+
+		return ret;
+
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	/* The fifo holds maximum of 4 samples */
+	struct bm1390_data *data = iio_priv(idev);
+	struct bm1390_data_buf buffer;
+	int smp_lvl, ret, i;
+	u64 sample_period;
+	__be16 temp = 0;
+
+	/*
+	 * If the IC is accessed during FIFO read samples can be dropped.
+	 * Prevent access until FIFO_LVL is read
+	 */
+	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;
+	}
+
+	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 > 4) {
+		/*
+		 * 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 = 4;
+	}
+
+	if (!smp_lvl)
+		return ret;
+
+	sample_period = data->timestamp - data->old_timestamp;
+	do_div(sample_period, smp_lvl);
+
+	if (samples && smp_lvl > samples)
+		smp_lvl = samples;
+
+	for (i = 0; i < smp_lvl; i++) {
+		ret = bm1390_pressure_read(data, &buffer.pressure);
+		if (ret)
+			break;
+
+		buffer.temp = temp;
+		/*
+		 * 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;
+
+		iio_push_to_buffers_with_timestamp(idev, &buffer,
+						   data->old_timestamp);
+	}
+	/* Reading the FIFO_LVL closes the FIFO access sequence */
+	regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+
+	if (!ret)
+		return ret;
+
+	return smp_lvl;
+}
+
+static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	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.
+	 */
+	disable_irq(data->irq);
+	data->timestamp = iio_get_time_ns(idev);
+
+	mutex_lock(&data->mutex);
+	ret = __bm1390_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	enable_irq(data->irq);
+
+	return ret;
+}
+
+static int bm1390_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	int ret;
+
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	default:
+		ret = -EINVAL;
+	}
+	iio_device_release_direct_mode(idev);
+
+	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,
+	.write_raw = &bm1390_write_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;
+
+	ret = regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				 BM1390_MASK_IIR_MODE, BM1390_IIR_MODE_MID);
+
+	return ret;
+}
+
+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;
+
+	mutex_lock(&data->mutex);
+
+	/* 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);
+	/* 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, dummy;
+
+	/* DRDY is acked by reading status reg */
+	ret = regmap_read(data->regmap, BM1390_REG_STATUS,
+			  &dummy);
+	if (ret || !dummy)
+		return IRQ_NONE;
+
+	dev_dbg(data->dev, "DRDY trig status 0x%x\n", dummy);
+
+	ret = bm1390_pressure_read(data, &data->buf.pressure);
+	if (ret) {
+		dev_warn(data->dev, "sample read failed %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
+			       &data->buf.temp, BM1390_TEMP_SIZE);
+
+	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;
+	}
+
+	if (data->state == BM1390_STATE_FIFO) {
+		int ok;
+
+		ok = __bm1390_fifo_flush(idev, BM1390_TEMP_SIZE, true);
+		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);
+	} else {
+		int dummy;
+
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+
+		/*
+		 * 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.
+		 */
+		if (!ret)
+			ret = regmap_read(data->regmap, BM1390_REG_STATUS,
+					  &dummy);
+	}
+
+	ret = bm1390_set_drdy_irq(data, state);
+	if (ret)
+		goto unlock_out;
+
+
+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_trigger(struct bm1390_data *data, struct iio_dev *idev,
+				int irq)
+{
+	struct iio_trigger *itrig;
+	char *name;
+	int ret;
+
+	if (irq < 0) {
+		dev_warn(data->dev, "No IRQ - skipping buffering\n");
+		return 0;
+	}
+
+	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");
+
+	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!itrig)
+		return -ENOMEM;
+
+	data->trig = itrig;
+	idev->available_scan_masks = bm1390_scan_masks;
+
+	itrig->ops = &bm1390_trigger_ops;
+	iio_trigger_set_drvdata(itrig, data);
+
+	/*
+	 * No need to check for NULL. request_threaded_irq() defaults to
+	 * dev_name() should the alloc fail.
+	 */
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+			      dev_name(data->dev));
+
+	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 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");
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	data = iio_priv(idev);
+
+	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);
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = i2c->irq;
+	/*
+	 * Default watermark to WMI_MAX. We could also allow setting WMI to 0,
+	 * and interpret that as "WMI is disabled, use FIFO_FULL" but I've
+	 * no idea what is assumed if watermark is 0. Does it mean each sample
+	 * should trigger IRQ, or no samples should do that?
+	 *
+	 * Well, 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_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 ret;
+}
+
+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,
+		.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 3/3] 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 4cc6bf79fdd8..ad18c76b7b07 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18422,6 +18422,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 2/3] iio: pressure: Support ROHM BU1390

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen 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.

...

> +struct bm1390_data_buf {
> +	u32 pressure;
> +	__be16 temp;

> +	s64 ts __aligned(8);

Would like to see aligned_s64, but it will depend on my series, so not your
problem and not right now :-)

> +};

...

> +struct bm1390_data {
> +	int64_t timestamp, old_timestamp;

Out of a sudden int64_t instead of u64?

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

Or u8 instead of uint8_t?

> +	/* Prevent accessing sensor during FIFO read sequence */
> +	struct mutex mutex;
> +};

...

> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> +{
> +	u8 temp_reg[2] __aligned(2);

Why?! Just use proper bitwise type.

> +	__be16 *temp_raw;
> +	int ret;
> +	s16 val;
> +	bool negative;
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
> +			       sizeof(temp_reg));
> +	if (ret)
> +		return ret;
> +
> +	if (temp_reg[0] & 0x80)
> +		negative = true;
> +	else
> +		negative = false;

> +	temp_raw = (__be16 *)&temp_reg[0];

Heck no. Make temp_reg be properly typed.

> +	val = be16_to_cpu(*temp_raw);
> +
> +	if (negative) {
> +		/*
> +		 * Two's complement. I am not sure if all supported
> +		 * architectures actually use 2's complement represantation of
> +		 * signed ints. If yes, then we could just do the endianes
> +		 * conversion and say this is the s16 value. However, as I
> +		 * don't know, and as the conversion is pretty simple. let's
> +		 * just convert the signed 2's complement to absolute value and
> +		 * multiply by -1.
> +		 */
> +		val = ~val + 1;
> +		val *= -1;
> +	}
> +
> +	*temp = val;
> +
> +	return 0;
> +}

> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
> +{
> +	int ret;
> +	u8 raw[3];
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
> +			       &raw[0], 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 */

In this case assign _all_ values explicitly. Currently it's prone to errors
if somebody squeezed a value in between.

> +enum bm1390_meas_mode {
> +	BM1390_MEAS_MODE_STOP = 0x0,
> +	BM1390_MEAS_MODE_1SHOT,
> +	BM1390_MEAS_MODE_CONTINUOUS,
> +};

...

> +	mutex_lock(&data->mutex);

Wouldn't you like to start using cleanup.h?

...

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

Missing period at the end.

> +	 */

...

> +		goto unlock_out;

cleanup.h makes these goto:s unneeded.

...

> +	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) {
> +			*val = 0;
> +			/*
> +			 * pressure in hPa is register value divided by 2048.
> +			 * This means kPa is 1/20480 times the register value,
> +			 * which equals to 48828.125 * 10 ^ -9
> +			 * This is 48828.125 nano kPa.
> +			 *
> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> +			 * get 48828 - which means we lose some accuracy. Well,
> +			 * let's try to live with that.
> +			 */
> +			*val2 = 48828;
> +
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +
> +		return -EINVAL;

Why not switch-case like other drivers do?

> +	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 IIO_VAL_INT;
> +
> +		return ret;

Why not usual pattern?

		if (ret)
			return ret;

> +	default:
> +		return -EINVAL;
> +	}

...

> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);

> +

Unneeded blank line.

> +	if (smp_lvl > 4) {
> +		/*
> +		 * 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 = 4;
> +	}

> +	if (!smp_lvl)
> +		return ret;

This can be checked first as it's and error condition and previous branch has
no side effects on this. Also, wouldn't be better to use error code explicitly?

...

> +static int bm1390_write_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(idev);
> +	if (ret)
> +		return ret;

> +	switch (mask) {
> +	default:
> +		ret = -EINVAL;
> +	}

This needs a comment: Why we have a dead code.

> +	iio_device_release_direct_mode(idev);
> +
> +	return ret;
> +}

...

> +	/*
> +	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
> +	 * be fixed when IIR is in use

Missing period.

> +	 */

...

> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS,
> +			  &dummy);

This is perfectly one line even for fanatics of 80 characters limitation.

> +	if (ret || !dummy)
> +		return IRQ_NONE;

...

> +	if (state) {
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);

This ret is never used, what's going on here?

> +	} else {
> +		int dummy;
> +
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
> +
> +		/*
> +		 * 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.
> +		 */
> +		if (!ret)
> +			ret = regmap_read(data->regmap, BM1390_REG_STATUS,
> +					  &dummy);
> +	}
> +
> +	ret = bm1390_set_drdy_irq(data, state);
> +	if (ret)
> +		goto unlock_out;

> +
> +

One blank line is enough.

> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;

> +

We do not put blank lines at the end of functions.

> +}

...

> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> +					      &iio_pollfunc_store_time,
> +					      &bm1390_trigger_handler,
> +					      &bm1390_buffer_ops);

> +

Yet another redundant blank line. I dunno how you manage to almost in every
second attempt to randomly place blank lines here and there... I feel like
a conspiracy theory against myself :-)

> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio_triggered_buffer_setup FAIL\n");

...

> +
> +

Ditto.

> +	ret = devm_iio_trigger_register(data->dev, itrig);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Trigger registration failed\n");

> +
> +

Ditto.

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

Do you expect anything than 0 here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

Morning Andy,

Thanks for the review.

On 9/6/23 18:48, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen 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.
> 
> ...
> 
> 
>> +struct bm1390_data {
>> +	int64_t timestamp, old_timestamp;
> 
> Out of a sudden int64_t instead of u64?

Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), 
IIO operates with signed timestamps. One being s64, the other int64_t.

>> +	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;
> 
> Or u8 instead of uint8_t?

So, uint8_t is preferred? I don't really care all that much which of 
these to use - which may even show up as a lack of consistency... I 
think I did use uint8_t when I learned about it - but at some point 
someone somewhere asked me to use u8 instead.. This somewhere might have 
been u-boot though...

So, are you Suggesting I should replace u8 with uint8_t? Can do if it 
matters.
> 
>> +	/* Prevent accessing sensor during FIFO read sequence */
>> +	struct mutex mutex;
>> +};
> 
> ...
> 
>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>> +{
>> +	u8 temp_reg[2] __aligned(2);
> 
> Why?! Just use proper bitwise type.

What is the proper bitwise type in this case?

I'll explain my reasoning:
What we really have in hardware (BM1390) and read from it is 8bit 
registers. This is u8 to me. And as we read two consecutive registers, 
we use u8 arr[2]. In my eyes it describes the data perfectly well, right?

> 
>> +	__be16 *temp_raw;
>> +	int ret;
>> +	s16 val;
>> +	bool negative;
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>> +			       sizeof(temp_reg));
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (temp_reg[0] & 0x80)
>> +		negative = true;
>> +	else
>> +		negative = false;
> 
>> +	temp_raw = (__be16 *)&temp_reg[0];
> 
> Heck no. Make temp_reg be properly typed.

As I explained above, to me it seems ur arr[2} is _the_ proper type to 
represent data we read from the device.

What we need to do is to convert the 16bits of data to an integer we can 
give to the rest of the system. And, we happen to know how to 
'manipulate' the data to get it in format of understandable integer. As 
we have these 16 bits in memory, aligned to 2 byte boundary - why 
shouldn't we just 'manipulate' the data and say - here we have your 
integer, please be my guest and use it?

Well, I am keen to learn the 'correct bitwise type' you talk about - can 
you please explain me what this correct type for two 8bit integers is? 
Maybe I can improve.

> 
>> +	val = be16_to_cpu(*temp_raw);
>> +
>> +	if (negative) {
>> +		/*
>> +		 * Two's complement. I am not sure if all supported
>> +		 * architectures actually use 2's complement represantation of
>> +		 * signed ints. If yes, then we could just do the endianes
>> +		 * conversion and say this is the s16 value. However, as I
>> +		 * don't know, and as the conversion is pretty simple. let's
>> +		 * just convert the signed 2's complement to absolute value and
>> +		 * multiply by -1.
>> +		 */
>> +		val = ~val + 1;
>> +		val *= -1;
>> +	}
>> +
>> +	*temp = val;
>> +
>> +	return 0;
>> +}
> 
>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>> +{
>> +	int ret;
>> +	u8 raw[3];
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>> +			       &raw[0], 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 */
> 
> In this case assign _all_ values explicitly. Currently it's prone to errors
> if somebody squeezed a value in between.

Agree. This is a good practice. Thanks. (although, it shouldn't really 
matter here as nobody really should squeeze a value in between as enum 
is short and we have this just comment above).

>> +enum bm1390_meas_mode {
>> +	BM1390_MEAS_MODE_STOP = 0x0,
>> +	BM1390_MEAS_MODE_1SHOT,
>> +	BM1390_MEAS_MODE_CONTINUOUS,
>> +};
> 
> ...
> 
>> +	mutex_lock(&data->mutex);
> 
> Wouldn't you like to start using cleanup.h?

The new macro "thingee" for C++ destructor like constructs?

TBH, I am not really in a rush with it for two reasons.
1) The syntax looks very alien to me. I would like to get some time to 
get used to it before voluntarily ending up maintaining a code using it. 
(I don't like practicing things upstream as practicing tends to include 
making more errors).

2. Related to 1). I am not (yet) convinced incorporating changes in 
stuff using these cleanups is easy. I'm a bit reserved and would like to 
see how things play out.

3. I expect I will get a few requests to backport the code to some older 
kernels used by some big customers. (I've been doing plenty of extra 
work when backporting my kernel improvements like regmap_irq stuff, 
linear ranges, regulator pickable ranges, gts-helpers to customer 
kernels to get my drivers working - or working around the lack of thiose 
features. I have been willing to pay this prize to get those helpers 
upstream for everyone to use. The cleanup.h however is there so it does 
not _need_ new users. I don't think _all_ existing drivers will be 
converted to use it so one more should probably not hurt. I think that 
in a year at least some customers will be using kernel containing the 
cleanup.h - so by latest then it is time for me to jump on that train. I 
hope I am used to reading those macros by then).
> 
> ...
> 
>> +	/*
>> +	 * 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
> 
> Missing period at the end.
> 
>> +	 */
> 
> ...
> 
>> +		goto unlock_out;
> 
> cleanup.h makes these goto:s unneeded.
> 
> ...
> 
>> +	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) {
>> +			*val = 0;
>> +			/*
>> +			 * pressure in hPa is register value divided by 2048.
>> +			 * This means kPa is 1/20480 times the register value,
>> +			 * which equals to 48828.125 * 10 ^ -9
>> +			 * This is 48828.125 nano kPa.
>> +			 *
>> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
>> +			 * get 48828 - which means we lose some accuracy. Well,
>> +			 * let's try to live with that.
>> +			 */
>> +			*val2 = 48828;
>> +
>> +			return IIO_VAL_INT_PLUS_NANO;
>> +		}
>> +
>> +		return -EINVAL;
> 
> Why not switch-case like other drivers do?

In my eyes avoiding the very simple if - else if does not warrant nested 
switches which look ugly to me.

> 
>> +	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 IIO_VAL_INT;
>> +
>> +		return ret;
> 
> Why not usual pattern?
> 
> 		if (ret)
> 			return ret;

I see your point, ok.

> 
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
> ...
> 
>> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
> 
>> +
> 
> Unneeded blank line.
> 
>> +	if (smp_lvl > 4) {
>> +		/*
>> +		 * 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 = 4;
>> +	}
> 
>> +	if (!smp_lvl)
>> +		return ret;
> 
> This can be checked first as it's and error condition

I wouldn't say it is an error condition. It just means there was no 
samples collected in buffer.

  and previous branch has
> no side effects on this. Also, wouldn't be better to use error code explicitly?

Yes. For the clarity we definitely should explicitly do "return 0" here. 
Thanks.

> 
> ...
> 
>> +static int bm1390_write_raw(struct iio_dev *idev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int val, int val2, long mask)
>> +{
>> +	int ret;
>> +
>> +	ret = iio_device_claim_direct_mode(idev);
>> +	if (ret)
>> +		return ret;
> 
>> +	switch (mask) {
>> +	default:
>> +		ret = -EINVAL;
>> +	}
> 
> This needs a comment: Why we have a dead code.

Oh, thanks! This really, rather requires a clean-up. It looks like a 
left over from the version where I supported setting the number of 
samples to average.

> 
>> +	iio_device_release_direct_mode(idev);
>> +
>> +	return ret;
>> +}
> 
> ...
> 
>> +	/*
>> +	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
>> +	 * be fixed when IIR is in use
> 
> Missing period.
> 
>> +	 */
> 
> ...
> 
>> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS,
>> +			  &dummy);
> 
> This is perfectly one line even for fanatics of 80 characters limitation.
> 
>> +	if (ret || !dummy)
>> +		return IRQ_NONE;
> 
> ...
> 
>> +	if (state) {
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
> 
> This ret is never used, what's going on here?

Thanks. I need to revise this :)

> 
>> +	} else {
>> +		int dummy;
>> +
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
>> +
>> +		/*
>> +		 * 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.
>> +		 */
>> +		if (!ret)
>> +			ret = regmap_read(data->regmap, BM1390_REG_STATUS,
>> +					  &dummy);
>> +	}
>> +
>> +	ret = bm1390_set_drdy_irq(data, state);
>> +	if (ret)
>> +		goto unlock_out;
> 
>> +
>> +
> 
> One blank line is enough.
> 
>> +unlock_out:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
> 
>> +
> 
> We do not put blank lines at the end of functions.
> 
>> +}
> 
> ...
> 
>> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
>> +					      &iio_pollfunc_store_time,
>> +					      &bm1390_trigger_handler,
>> +					      &bm1390_buffer_ops);
> 
>> +
> 
> Yet another redundant blank line. I dunno how you manage to almost in every
> second attempt to randomly place blank lines here and there... I feel like
> a conspiracy theory against myself :-)

Oh, dang. My plot is revealed :)

> 
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "iio_triggered_buffer_setup FAIL\n");
> 
> ...
> 
>> +
>> +
> 
> Ditto.
> 
>> +	ret = devm_iio_trigger_register(data->dev, itrig);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Trigger registration failed\n");
> 
>> +
>> +
> 
> Ditto.
> 
>> +	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 ret;
> 
> Do you expect anything than 0 here?

No, not really. Thanks!


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 1/3] dt-bindings: Add ROHM BM1390 pressure sensor

[Conor Dooley]

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

On Wed, Sep 06, 2023 at 03:37:19PM +0300, Matti Vaittinen wrote:
> 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>
> ---
>  .../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..d681fdd0f5ea
> --- /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: |

You've got no formatting to preserve the | is not needed.

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

nit: "built in to the sensor." or just "built-in."

Otherwise this seems alright to me,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> 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 = "kionix,kx022a";
> +            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: 228 bytes --]

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Andy Shevchenko]

On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:
> On 9/6/23 18:48, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:

...

> > > +struct bm1390_data {
> > > +	int64_t timestamp, old_timestamp;
> > 
> > Out of a sudden int64_t instead of u64?
> 
> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
> operates with signed timestamps. One being s64, the other int64_t.
> 
> > > +	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;
> > 
> > Or u8 instead of uint8_t?
> 
> So, uint8_t is preferred? I don't really care all that much which of these
> to use - which may even show up as a lack of consistency... I think I did
> use uint8_t when I learned about it - but at some point someone somewhere
> asked me to use u8 instead.. This somewhere might have been u-boot though...
> 
> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
> matters.

Consistency matters, since I do not know the intention behind, I suggest use
either, but be consistent in the entire code. However, uXX are specific Linux
kernel internal types and some maintainers prefer them. Also you may grep for
the frequency of intXX_t vs. sXX or their unsigned counterparts.

> > > +	/* Prevent accessing sensor during FIFO read sequence */
> > > +	struct mutex mutex;
> > > +};

...

> > > +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> > > +{
> > > +	u8 temp_reg[2] __aligned(2);
> > 
> > Why?! Just use proper bitwise type.
> 
> What is the proper bitwise type in this case?
> 
> I'll explain my reasoning:
> What we really have in hardware (BM1390) and read from it is 8bit registers.
> This is u8 to me. And as we read two consecutive registers, we use u8
> arr[2]. In my eyes it describes the data perfectly well, right?

Two different registers?! Why bulk is used in that case?
To me looks like you are reading 16-bit (or one that fits in 16-bit) register
in BE notation.

> > > +	__be16 *temp_raw;
> > > +	int ret;
> > > +	s16 val;
> > > +	bool negative;
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
> > > +			       sizeof(temp_reg));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (temp_reg[0] & 0x80)
> > > +		negative = true;
> > > +	else
> > > +		negative = false;
> > 
> > > +	temp_raw = (__be16 *)&temp_reg[0];
> > 
> > Heck no. Make temp_reg be properly typed.
> 
> As I explained above, to me it seems ur arr[2} is _the_ proper type to
> represent data we read from the device.
> 
> What we need to do is to convert the 16bits of data to an integer we can
> give to the rest of the system. And, we happen to know how to 'manipulate'
> the data to get it in format of understandable integer. As we have these 16
> bits in memory, aligned to 2 byte boundary - why shouldn't we just
> 'manipulate' the data and say - here we have your integer, please be my
> guest and use it?

Because it smell like a hack and is a hack here.
Either it's a single BE16 register, or it's two different registers that by
very unknown reason you are reading in a bulk. The code in this form is no
go.

> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
> please explain me what this correct type for two 8bit integers is? Maybe I
> can improve.

If the registers are not of the same nature the bulk access is wrong.
Use one by one reads.

> > > +	val = be16_to_cpu(*temp_raw);
> > > +
> > > +	if (negative) {
> > > +		/*
> > > +		 * Two's complement. I am not sure if all supported
> > > +		 * architectures actually use 2's complement represantation of
> > > +		 * signed ints. If yes, then we could just do the endianes
> > > +		 * conversion and say this is the s16 value. However, as I
> > > +		 * don't know, and as the conversion is pretty simple. let's
> > > +		 * just convert the signed 2's complement to absolute value and
> > > +		 * multiply by -1.
> > > +		 */
> > > +		val = ~val + 1;
> > > +		val *= -1;
> > > +	}
> > > +
> > > +	*temp = val;
> > > +
> > > +	return 0;
> > > +}

...

> > > +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
> > > +{
> > > +	int ret;
> > > +	u8 raw[3];
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
> > > +			       &raw[0], sizeof(raw));

&raw[0] --> raw

> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);

This, btw, looks like le24, but I'm puzzled with right shift.
I need to read datasheet carefully to understand this.

> > > +	return 0;
> > > +}

...

> > > + /* The enum values map directly to register bits */
> > 
> > In this case assign _all_ values explicitly. Currently it's prone to errors
> > if somebody squeezed a value in between.
> 
> Agree. This is a good practice. Thanks. (although, it shouldn't really
> matter here as nobody really should squeeze a value in between as enum is
> short and we have this just comment above).

Right, but code can be done more robust against these. I do not see any
argument against this. It's cheap and correct to add assignments and
then we do not care about the sequence, even if somebody messes it up.

> > > +enum bm1390_meas_mode {
> > > +	BM1390_MEAS_MODE_STOP = 0x0,
> > > +	BM1390_MEAS_MODE_1SHOT,
> > > +	BM1390_MEAS_MODE_CONTINUOUS,
> > > +};

...

> > > +	mutex_lock(&data->mutex);
> > 
> > Wouldn't you like to start using cleanup.h?
> 
> The new macro "thingee" for C++ destructor like constructs?

I was talking about these: guard() / scoped_guard().

> TBH, I am not really in a rush with it for two reasons.
> 1) The syntax looks very alien to me. I would like to get some time to get
> used to it before voluntarily ending up maintaining a code using it. (I
> don't like practicing things upstream as practicing tends to include making
> more errors).
> 
> 2. Related to 1). I am not (yet) convinced incorporating changes in stuff
> using these cleanups is easy. I'm a bit reserved and would like to see how
> things play out.
> 
> 3. I expect I will get a few requests to backport the code to some older
> kernels used by some big customers. (I've been doing plenty of extra work
> when backporting my kernel improvements like regmap_irq stuff, linear
> ranges, regulator pickable ranges, gts-helpers to customer kernels to get my
> drivers working - or working around the lack of thiose features. I have been
> willing to pay this prize to get those helpers upstream for everyone to use.
> The cleanup.h however is there so it does not _need_ new users. I don't
> think _all_ existing drivers will be converted to use it so one more should
> probably not hurt. I think that in a year at least some customers will be
> using kernel containing the cleanup.h - so by latest then it is time for me
> to jump on that train. I hope I am used to reading those macros by then).

OK.

...

> > > +	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) {
> > > +			*val = 0;
> > > +			/*
> > > +			 * pressure in hPa is register value divided by 2048.
> > > +			 * This means kPa is 1/20480 times the register value,
> > > +			 * which equals to 48828.125 * 10 ^ -9
> > > +			 * This is 48828.125 nano kPa.
> > > +			 *
> > > +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> > > +			 * get 48828 - which means we lose some accuracy. Well,
> > > +			 * let's try to live with that.
> > > +			 */
> > > +			*val2 = 48828;
> > > +
> > > +			return IIO_VAL_INT_PLUS_NANO;
> > > +		}
> > > +
> > > +		return -EINVAL;
> > 
> > Why not switch-case like other drivers do?
> 
> In my eyes avoiding the very simple if - else if does not warrant nested
> switches which look ugly to me.

Okay, yet another disagreement.

> > > +	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 IIO_VAL_INT;
> > > +
> > > +		return ret;
> > 
> > Why not usual pattern?
> > 
> > 		if (ret)
> > 			return ret;
> 
> I see your point, ok.
> 
> > > +	default:
> > > +		return -EINVAL;
> > > +	}

...

> > > +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
> > 
> > > +
> > 
> > Unneeded blank line.
> > 
> > > +	if (smp_lvl > 4) {
> > > +		/*
> > > +		 * 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 = 4;
> > > +	}
> > 
> > > +	if (!smp_lvl)
> > > +		return ret;
> > 
> > This can be checked first as it's and error condition
> 
> I wouldn't say it is an error condition.

Returning ret suggests otherwise.

> It just means there was no samples
> collected in buffer.

But as you explained below, the code is actually 0 there.
In any case bailing out conditionals are better to be first.

>  and previous branch has
> > no side effects on this. Also, wouldn't be better to use error code explicitly?
> 
> Yes. For the clarity we definitely should explicitly do "return 0" here.
> Thanks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: Add ROHM BM1390 pressure sensor

[Matti Vaittinen]

On 9/7/23 13:08, Conor Dooley wrote:
> On Wed, Sep 06, 2023 at 03:37:19PM +0300, Matti Vaittinen wrote:
>> 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>
>> ---
>>   .../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..d681fdd0f5ea
>> --- /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: |
> 
> You've got no formatting to preserve the | is not needed.
> 
>> +  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.
> 
> nit: "built in to the sensor." or just "built-in."
> 
> Otherwise this seems alright to me,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>


Thanks Conor! I'll fix them for v2 :)

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 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/7/23 16:57, Andy Shevchenko wrote:
> On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:
>> On 9/6/23 18:48, Andy Shevchenko wrote:
>>> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +struct bm1390_data {
>>>> +	int64_t timestamp, old_timestamp;
>>>
>>> Out of a sudden int64_t instead of u64?
>>
>> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
>> operates with signed timestamps. One being s64, the other int64_t.
>>
>>>> +	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;
>>>
>>> Or u8 instead of uint8_t?
>>
>> So, uint8_t is preferred? I don't really care all that much which of these
>> to use - which may even show up as a lack of consistency... I think I did
>> use uint8_t when I learned about it - but at some point someone somewhere
>> asked me to use u8 instead.. This somewhere might have been u-boot though...
>>
>> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
>> matters.
> 
> Consistency matters, since I do not know the intention behind, I suggest use
> either, but be consistent in the entire code. However, uXX are specific Linux
> kernel internal types and some maintainers prefer them. Also you may grep for
> the frequency of intXX_t vs. sXX or their unsigned counterparts.
> 
>>>> +	/* Prevent accessing sensor during FIFO read sequence */
>>>> +	struct mutex mutex;
>>>> +};
> 
> ...
> 
>>>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>>>> +{
>>>> +	u8 temp_reg[2] __aligned(2);
>>>
>>> Why?! Just use proper bitwise type.
>>
>> What is the proper bitwise type in this case?
>>
>> I'll explain my reasoning:
>> What we really have in hardware (BM1390) and read from it is 8bit registers.
>> This is u8 to me. And as we read two consecutive registers, we use u8
>> arr[2]. In my eyes it describes the data perfectly well, right?
> 
> Two different registers?! Why bulk is used in that case?
> To me looks like you are reading 16-bit (or one that fits in 16-bit) register
> in BE notation.

As I wrote, it is two 8 bit registers at consecutive addresses. And this 
is what u8 arr[2]; also says.

Bulk read is mandatory as HW has a special feature of preventing the 
update of these registers when a read is ongoing. If we do two reads we 
risk of getting one of the registers updated between the accesses - 
resulting incorrect value.

> 
>>>> +	__be16 *temp_raw;
>>>> +	int ret;
>>>> +	s16 val;
>>>> +	bool negative;
>>>> +
>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>>>> +			       sizeof(temp_reg));
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (temp_reg[0] & 0x80)
>>>> +		negative = true;
>>>> +	else
>>>> +		negative = false;
>>>
>>>> +	temp_raw = (__be16 *)&temp_reg[0];
>>>
>>> Heck no. Make temp_reg be properly typed.
>>
>> As I explained above, to me it seems ur arr[2} is _the_ proper type to
>> represent data we read from the device.
>>
>> What we need to do is to convert the 16bits of data to an integer we can
>> give to the rest of the system. And, we happen to know how to 'manipulate'
>> the data to get it in format of understandable integer. As we have these 16
>> bits in memory, aligned to 2 byte boundary - why shouldn't we just
>> 'manipulate' the data and say - here we have your integer, please be my
>> guest and use it?
> 
> Because it smell like a hack and is a hack here.
> Either it's a single BE16 register, or it's two different registers that by
> very unknown reason you are reading in a bulk.

It is two registers. The bulk read might warrant a comment - although I 
believe this is nothing unusual. If it is a hack or not is an opinion. 
To me it looks like a code that explicitly shows what data is and how it 
is being handled. It does what it is supposed to do and shows it in all 
dirty details.

Still, I am open to suggestions but I'd prefer seeing a real improvement 
instead of claiming that the hardware is something it is not (eg, having 
16bit registers or should be read by individual accesses).

The code in this form is no
> go.
> 
>> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
>> please explain me what this correct type for two 8bit integers is? Maybe I
>> can improve.
> 
> If the registers are not of the same nature the bulk access is wrong.
> Use one by one reads.

Of same nature? As I said, there is 2 8bit registers at consecutive 
addresses. They have no other 'nature' as far as I can tell.

Data in these registers in not in standard format - it needs to be 
manipulated to make it an ordinary integer. The code shows this very 
clearly by not reading it in any standard integer.

> ...
> 
>>>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>>>> +{
>>>> +	int ret;
>>>> +	u8 raw[3];
>>>> +
>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>>>> +			       &raw[0], sizeof(raw));
> 
> &raw[0] --> raw
> 
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
> 
> This, btw, looks like le24, but I'm puzzled with right shift.
> I need to read datasheet carefully to understand this.

It's not just le24. We, again, have data placed in registers so that it 
needs some suffling. The data-sheet does decent job explaining it 
though. AFAIR, there was a 'gap' in bits so it needed some more suffling 
to sift the bits so that they're consecutive. I think this indeed is 
something that needs to be looked up from data-sheet to understand why 
this play with bits is done.

> ...
> 
>>>> +	mutex_lock(&data->mutex);
>>>
>>> Wouldn't you like to start using cleanup.h?
>>
>> The new macro "thingee" for C++ destructor like constructs?
> 
> I was talking about these: guard() / scoped_guard().

Right, this was a bit more precise than "The new macro thingee", thanks 
:) This, however is what I was referring to so what I wrote below still 
applies :)

> 
>> TBH, I am not really in a rush with it for two reasons.
>> 1) The syntax looks very alien to me. I would like to get some time to get
>> used to it before voluntarily ending up maintaining a code using it. (I
>> don't like practicing things upstream as practicing tends to include making
>> more errors).
>>
>> 2. Related to 1). I am not (yet) convinced incorporating changes in stuff
>> using these cleanups is easy. I'm a bit reserved and would like to see how
>> things play out.
>>
>> 3. I expect I will get a few requests to backport the code to some older
>> kernels used by some big customers. (I've been doing plenty of extra work
>> when backporting my kernel improvements like regmap_irq stuff, linear
>> ranges, regulator pickable ranges, gts-helpers to customer kernels to get my
>> drivers working - or working around the lack of thiose features. I have been
>> willing to pay this prize to get those helpers upstream for everyone to use.
>> The cleanup.h however is there so it does not _need_ new users. I don't
>> think _all_ existing drivers will be converted to use it so one more should
>> probably not hurt. I think that in a year at least some customers will be
>> using kernel containing the cleanup.h - so by latest then it is time for me
>> to jump on that train. I hope I am used to reading those macros by then).
> 
> OK.
> 
> ...
> 
>>>> +	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) {
>>>> +			*val = 0;
>>>> +			/*
>>>> +			 * pressure in hPa is register value divided by 2048.
>>>> +			 * This means kPa is 1/20480 times the register value,
>>>> +			 * which equals to 48828.125 * 10 ^ -9
>>>> +			 * This is 48828.125 nano kPa.
>>>> +			 *
>>>> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
>>>> +			 * get 48828 - which means we lose some accuracy. Well,
>>>> +			 * let's try to live with that.
>>>> +			 */
>>>> +			*val2 = 48828;
>>>> +
>>>> +			return IIO_VAL_INT_PLUS_NANO;
>>>> +		}
>>>> +
>>>> +		return -EINVAL;
>>>
>>> Why not switch-case like other drivers do?
>>
>> In my eyes avoiding the very simple if - else if does not warrant nested
>> switches which look ugly to me.
> 
> Okay, yet another disagreement.
> 

Yes, we have a few :) Luckily most are more on a styling side of things. 
I do still appreciate you reviewing the patches as I think I do agree 
with most of your suggestions - and having an extra pair of eyes looking 
at the code never hurts :)

> ...
> 
>>>> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
>>>
>>>> +
>>>
>>> Unneeded blank line.
>>>
>>>> +	if (smp_lvl > 4) {
>>>> +		/*
>>>> +		 * 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 = 4;
>>>> +	}
>>>
>>>> +	if (!smp_lvl)
>>>> +		return ret;
>>>
>>> This can be checked first as it's and error condition
>>
>> I wouldn't say it is an error condition.
> 
> Returning ret suggests otherwise.

Yes. I agree.

> 
>> It just means there was no samples
>> collected in buffer.
> 
> But as you explained below, the code is actually 0 there.
> In any case bailing out conditionals are better to be first.

Kind of agree. Or by very least it should be else if. But as you said, 
early return is probably better here, thanks.

> 
>>   and previous branch has
>>> no side effects on this. Also, wouldn't be better to use error code explicitly?
>>
>> Yes. For the clarity we definitely should explicitly do "return 0" here.
>> Thanks.
> 

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

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

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/7/23 16:57, Andy Shevchenko wrote:
> On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:
>> On 9/6/23 18:48, Andy Shevchenko wrote:
>>> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +struct bm1390_data {
>>>> +	int64_t timestamp, old_timestamp;
>>>
>>> Out of a sudden int64_t instead of u64?
>>
>> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
>> operates with signed timestamps. One being s64, the other int64_t.
>>
>>>> +	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;
>>>
>>> Or u8 instead of uint8_t?
>>
>> So, uint8_t is preferred? I don't really care all that much which of these
>> to use - which may even show up as a lack of consistency... I think I did
>> use uint8_t when I learned about it - but at some point someone somewhere
>> asked me to use u8 instead.. This somewhere might have been u-boot though...
>>
>> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
>> matters.
> 
> Consistency matters, since I do not know the intention behind, I suggest use
> either, but be consistent in the entire code. However, uXX are specific Linux
> kernel internal types and some maintainers prefer them. Also you may grep for
> the frequency of intXX_t vs. sXX or their unsigned counterparts.
> 
>>>> +	/* Prevent accessing sensor during FIFO read sequence */
>>>> +	struct mutex mutex;
>>>> +};
> 
> ...
> 
>>>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>>>> +{
>>>> +	u8 temp_reg[2] __aligned(2);
>>>
>>> Why?! Just use proper bitwise type.
>>
>> What is the proper bitwise type in this case?
>>
>> I'll explain my reasoning:
>> What we really have in hardware (BM1390) and read from it is 8bit registers.
>> This is u8 to me. And as we read two consecutive registers, we use u8
>> arr[2]. In my eyes it describes the data perfectly well, right?
> 
> Two different registers?! Why bulk is used in that case?
> To me looks like you are reading 16-bit (or one that fits in 16-bit) register
> in BE notation.

As I wrote, it is two 8 bit registers at consecutive addresses. And this 
is what u8 arr[2]; also says.

Bulk read is mandatory as HW has a special feature of preventing the 
update of these registers when a read is ongoing. If we do two reads we 
risk of getting one of the registers updated between the accesses - 
resulting incorrect value.

> 
>>>> +	__be16 *temp_raw;
>>>> +	int ret;
>>>> +	s16 val;
>>>> +	bool negative;
>>>> +
>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>>>> +			       sizeof(temp_reg));
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (temp_reg[0] & 0x80)
>>>> +		negative = true;
>>>> +	else
>>>> +		negative = false;
>>>
>>>> +	temp_raw = (__be16 *)&temp_reg[0];
>>>
>>> Heck no. Make temp_reg be properly typed.
>>
>> As I explained above, to me it seems ur arr[2} is _the_ proper type to
>> represent data we read from the device.
>>
>> What we need to do is to convert the 16bits of data to an integer we can
>> give to the rest of the system. And, we happen to know how to 'manipulate'
>> the data to get it in format of understandable integer. As we have these 16
>> bits in memory, aligned to 2 byte boundary - why shouldn't we just
>> 'manipulate' the data and say - here we have your integer, please be my
>> guest and use it?
> 
> Because it smell like a hack and is a hack here.
> Either it's a single BE16 register, or it's two different registers that by
> very unknown reason you are reading in a bulk.

It is two registers. The bulk read might warrant a comment - although I 
believe this is nothing unusual. If it is a hack or not is an opinion. 
To me it looks like a code that explicitly shows what data is and how it 
is being handled. It does what it is supposed to do and shows it in all 
dirty details.

Still, I am open to suggestions but I'd prefer seeing a real improvement 
instead of claiming that the hardware is something it is not (eg, having 
16bit registers or should be read by individual accesses).

The code in this form is no
> go.
> 
>> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
>> please explain me what this correct type for two 8bit integers is? Maybe I
>> can improve.
> 
> If the registers are not of the same nature the bulk access is wrong.
> Use one by one reads.

Of same nature? As I said, there is 2 8bit registers at consecutive 
addresses. They have no other 'nature' as far as I can tell.

Data in these registers in not in standard format - it needs to be 
manipulated to make it an ordinary integer. The code shows this very 
clearly by not reading it in any standard integer.

> ...
> 
>>>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>>>> +{
>>>> +	int ret;
>>>> +	u8 raw[3];
>>>> +
>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>>>> +			       &raw[0], sizeof(raw));
> 
> &raw[0] --> raw
> 
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
> 
> This, btw, looks like le24, but I'm puzzled with right shift.
> I need to read datasheet carefully to understand this.

It's not just le24. We, again, have data placed in registers so that it 
needs some suffling. The data-sheet does decent job explaining it 
though. AFAIR, there was a 'gap' in bits so it needed some more suffling 
to sift the bits so that they're consecutive. I think this indeed is 
something that needs to be looked up from data-sheet to understand why 
this play with bits is done.

> ...
> 
>>>> +	mutex_lock(&data->mutex);
>>>
>>> Wouldn't you like to start using cleanup.h?
>>
>> The new macro "thingee" for C++ destructor like constructs?
> 
> I was talking about these: guard() / scoped_guard().

Right, this was a bit more precise than "The new macro thingee", thanks 
:) This, however is what I was referring to so what I wrote below still 
applies :)

> 
>> TBH, I am not really in a rush with it for two reasons.
>> 1) The syntax looks very alien to me. I would like to get some time to get
>> used to it before voluntarily ending up maintaining a code using it. (I
>> don't like practicing things upstream as practicing tends to include making
>> more errors).
>>
>> 2. Related to 1). I am not (yet) convinced incorporating changes in stuff
>> using these cleanups is easy. I'm a bit reserved and would like to see how
>> things play out.
>>
>> 3. I expect I will get a few requests to backport the code to some older
>> kernels used by some big customers. (I've been doing plenty of extra work
>> when backporting my kernel improvements like regmap_irq stuff, linear
>> ranges, regulator pickable ranges, gts-helpers to customer kernels to get my
>> drivers working - or working around the lack of thiose features. I have been
>> willing to pay this prize to get those helpers upstream for everyone to use.
>> The cleanup.h however is there so it does not _need_ new users. I don't
>> think _all_ existing drivers will be converted to use it so one more should
>> probably not hurt. I think that in a year at least some customers will be
>> using kernel containing the cleanup.h - so by latest then it is time for me
>> to jump on that train. I hope I am used to reading those macros by then).
> 
> OK.
> 
> ...
> 
>>>> +	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) {
>>>> +			*val = 0;
>>>> +			/*
>>>> +			 * pressure in hPa is register value divided by 2048.
>>>> +			 * This means kPa is 1/20480 times the register value,
>>>> +			 * which equals to 48828.125 * 10 ^ -9
>>>> +			 * This is 48828.125 nano kPa.
>>>> +			 *
>>>> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
>>>> +			 * get 48828 - which means we lose some accuracy. Well,
>>>> +			 * let's try to live with that.
>>>> +			 */
>>>> +			*val2 = 48828;
>>>> +
>>>> +			return IIO_VAL_INT_PLUS_NANO;
>>>> +		}
>>>> +
>>>> +		return -EINVAL;
>>>
>>> Why not switch-case like other drivers do?
>>
>> In my eyes avoiding the very simple if - else if does not warrant nested
>> switches which look ugly to me.
> 
> Okay, yet another disagreement.
> 

Yes, we have a few :) Luckily most are more on a styling side of things. 
I do still appreciate you reviewing the patches as I think I do agree 
with most of your suggestions - and having an extra pair of eyes looking 
at the code never hurts :)

> ...
> 
>>>> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
>>>
>>>> +
>>>
>>> Unneeded blank line.
>>>
>>>> +	if (smp_lvl > 4) {
>>>> +		/*
>>>> +		 * 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 = 4;
>>>> +	}
>>>
>>>> +	if (!smp_lvl)
>>>> +		return ret;
>>>
>>> This can be checked first as it's and error condition
>>
>> I wouldn't say it is an error condition.
> 
> Returning ret suggests otherwise.

Yes. I agree.

> 
>> It just means there was no samples
>> collected in buffer.
> 
> But as you explained below, the code is actually 0 there.
> In any case bailing out conditionals are better to be first.

Kind of agree. Or by very least it should be else if. But as you said, 
early return is probably better here, thanks.

> 
>>   and previous branch has
>>> no side effects on this. Also, wouldn't be better to use error code explicitly?
>>
>> Yes. For the clarity we definitely should explicitly do "return 0" here.
>> Thanks.
> 

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

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

Re: [PATCH 1/3] dt-bindings: Add ROHM BM1390 pressure sensor

[Benjamin Bara]

Hi Matti,

just accidentally saw your series.

On Wed, Sep 06, 2023 at 03:37:19PM +0300, Matti Vaittinen wrote:
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        pressure-sensor@5d {
> +            compatible = "kionix,kx022a";

I think the compatible in the example is not right.

Best Regards,
Benjamin

Re: [PATCH 1/3] dt-bindings: Add ROHM BM1390 pressure sensor

[Matti Vaittinen]

On 9/8/23 10:14, Benjamin Bara wrote:
> Hi Matti,
> 
> just accidentally saw your series.
> 
> On Wed, Sep 06, 2023 at 03:37:19PM +0300, Matti Vaittinen wrote:
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        pressure-sensor@5d {
>> +            compatible = "kionix,kx022a";
> 
> I think the compatible in the example is not right.

Indeed it's not! Great catch! Thanks! Must be fixed in v2.

Oh - and long time no see - I hope you had a great summer :) I'm waiting 
to see you continuing the regulator stuff ;)

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 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Thu, 7 Sep 2023 08:57:17 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Morning Andy,
> 
> Thanks for the review.
> 
> On 9/6/23 18:48, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen 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.  
> > 
> > ...
> > 
> >   
> >> +struct bm1390_data {
> >> +	int64_t timestamp, old_timestamp;  
> > 
> > Out of a sudden int64_t instead of u64?  
> 
> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), 
> IIO operates with signed timestamps. One being s64, the other int64_t.

That's odd. Ah well.  Should both be s64 as internal to the kernel only.


> 
> >> +	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;  
> > 
> > Or u8 instead of uint8_t?  
> 
> So, uint8_t is preferred? I don't really care all that much which of 
> these to use - which may even show up as a lack of consistency... I 
> think I did use uint8_t when I learned about it - but at some point 
> someone somewhere asked me to use u8 instead.. This somewhere might have 
> been u-boot though...
> 
> So, are you Suggesting I should replace u8 with uint8_t? Can do if it 
> matters.
u8 preferred for internal to kernel stuff, uint8_t if a userspace header.

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Fri, 8 Sep 2023 09:12:51 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/7/23 16:57, Andy Shevchenko wrote:
> > On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:  
> >> On 9/6/23 18:48, Andy Shevchenko wrote:  
> >>> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:  
> > 
> > ...
> >   
> >>>> +struct bm1390_data {
> >>>> +	int64_t timestamp, old_timestamp;  
> >>>
> >>> Out of a sudden int64_t instead of u64?  
> >>
> >> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
> >> operates with signed timestamps. One being s64, the other int64_t.
> >>  
> >>>> +	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;  
> >>>
> >>> Or u8 instead of uint8_t?  
> >>
> >> So, uint8_t is preferred? I don't really care all that much which of these
> >> to use - which may even show up as a lack of consistency... I think I did
> >> use uint8_t when I learned about it - but at some point someone somewhere
> >> asked me to use u8 instead.. This somewhere might have been u-boot though...
> >>
> >> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
> >> matters.  
> > 
> > Consistency matters, since I do not know the intention behind, I suggest use
> > either, but be consistent in the entire code. However, uXX are specific Linux
> > kernel internal types and some maintainers prefer them. Also you may grep for
> > the frequency of intXX_t vs. sXX or their unsigned counterparts.
> >   
> >>>> +	/* Prevent accessing sensor during FIFO read sequence */
> >>>> +	struct mutex mutex;
> >>>> +};  
> > 
> > ...
> >   
> >>>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> >>>> +{
> >>>> +	u8 temp_reg[2] __aligned(2);  
> >>>
> >>> Why?! Just use proper bitwise type.  
> >>
> >> What is the proper bitwise type in this case?
> >>
> >> I'll explain my reasoning:
> >> What we really have in hardware (BM1390) and read from it is 8bit registers.
> >> This is u8 to me. And as we read two consecutive registers, we use u8
> >> arr[2]. In my eyes it describes the data perfectly well, right?  
> > 
> > Two different registers?! Why bulk is used in that case?
> > To me looks like you are reading 16-bit (or one that fits in 16-bit) register
> > in BE notation.  
> 
> As I wrote, it is two 8 bit registers at consecutive addresses. And this 
> is what u8 arr[2]; also says.
> 
> Bulk read is mandatory as HW has a special feature of preventing the 
> update of these registers when a read is ongoing. If we do two reads we 
> risk of getting one of the registers updated between the accesses - 
> resulting incorrect value.

Far as the kernel is concerned this is a __be16 and we use a bulk read to
fill it from two registers.  If the use showed them having unrelated uses
then it would be fair to handle it as an array of u8.

> 
> >   
> >>>> +	__be16 *temp_raw;
> >>>> +	int ret;
> >>>> +	s16 val;
> >>>> +	bool negative;
> >>>> +
> >>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
> >>>> +			       sizeof(temp_reg));
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	if (temp_reg[0] & 0x80)
> >>>> +		negative = true;
> >>>> +	else
> >>>> +		negative = false;  
> >>>  
> >>>> +	temp_raw = (__be16 *)&temp_reg[0];  
> >>>
> >>> Heck no. Make temp_reg be properly typed.  
> >>
> >> As I explained above, to me it seems ur arr[2} is _the_ proper type to
> >> represent data we read from the device.
> >>
> >> What we need to do is to convert the 16bits of data to an integer we can
> >> give to the rest of the system. And, we happen to know how to 'manipulate'
> >> the data to get it in format of understandable integer. As we have these 16
> >> bits in memory, aligned to 2 byte boundary - why shouldn't we just
> >> 'manipulate' the data and say - here we have your integer, please be my
> >> guest and use it?  
> > 
> > Because it smell like a hack and is a hack here.
> > Either it's a single BE16 register, or it's two different registers that by
> > very unknown reason you are reading in a bulk.  
> 
> It is two registers. The bulk read might warrant a comment - although I 
> believe this is nothing unusual. If it is a hack or not is an opinion. 
> To me it looks like a code that explicitly shows what data is and how it 
> is being handled. It does what it is supposed to do and shows it in all 
> dirty details.

Read it directly into a __be16

> 
> Still, I am open to suggestions but I'd prefer seeing a real improvement 
> instead of claiming that the hardware is something it is not (eg, having 
> 16bit registers or should be read by individual accesses).
> 
> The code in this form is no
> > go.
> >   
> >> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
> >> please explain me what this correct type for two 8bit integers is? Maybe I
> >> can improve.  
> > 
> > If the registers are not of the same nature the bulk access is wrong.
> > Use one by one reads.  
> 
> Of same nature? As I said, there is 2 8bit registers at consecutive 
> addresses. They have no other 'nature' as far as I can tell.
> 
> Data in these registers in not in standard format - it needs to be 
> manipulated to make it an ordinary integer. The code shows this very 
> clearly by not reading it in any standard integer.

I'm not convinced it does.  All support arch are 2s complement.
be16_to_cpu() is fine for what you have unless I'm missing something.
If it weren't we would have signed versions of those macros.


> 
> > ...
> >   
> >>>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
> >>>> +{
> >>>> +	int ret;
> >>>> +	u8 raw[3];
> >>>> +
> >>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
> >>>> +			       &raw[0], sizeof(raw));  
> > 
> > &raw[0] --> raw
> >   
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);  
> > 
> > This, btw, looks like le24, but I'm puzzled with right shift.
> > I need to read datasheet carefully to understand this.  
> 
> It's not just le24. We, again, have data placed in registers so that it 
> needs some suffling. The data-sheet does decent job explaining it 
> though. AFAIR, there was a 'gap' in bits so it needed some more suffling 
> to sift the bits so that they're consecutive. I think this indeed is 
> something that needs to be looked up from data-sheet to understand why 
> this play with bits is done.


These cases are harder to argue.  I'm fine with either approach for this one
as a get_unaligned_le24() >> 2 would give same answer unless I'm also missing
something but it isn't that obvious.

Jonathan

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> +{
> +	u8 temp_reg[2] __aligned(2);
> +	__be16 *temp_raw;
> +	int ret;
> +	s16 val;
> +	bool negative;
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
> +			       sizeof(temp_reg));
> +	if (ret)
> +		return ret;
> +
> +	if (temp_reg[0] & 0x80)
> +		negative = true;
> +	else
> +		negative = false;
> +
> +	temp_raw = (__be16 *)&temp_reg[0];
> +	val = be16_to_cpu(*temp_raw);
> +
> +	if (negative) {
> +		/*
> +		 * Two's complement. I am not sure if all supported
> +		 * architectures actually use 2's complement represantation of

AFAIK they do. Many IIO drivers would be broken if not..

> +		 * signed ints. If yes, then we could just do the endianes
> +		 * conversion and say this is the s16 value. However, as I
> +		 * don't know, and as the conversion is pretty simple. let's
> +		 * just convert the signed 2's complement to absolute value and
> +		 * multiply by -1.
> +		 */
> +		val = ~val + 1;
> +		val *= -1;
> +	}
> +
> +	*temp = val;
> +
> +	return 0;
> +}

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Wed, 6 Sep 2023 15:37:48 +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).

*sigh*  We all love fuzzily described filters...

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

IIRC this is a bit different from what we've typically done in the past
where we just take the view that a slow moving thing like temperature
can be read from sysfs or that enabling it disables the watermark.

However, what you have seems like valid use of the ABI to me so I'm
fine with this approach if it makes sense for your expected users
(which you get to guess at ;)

Given there are multiple ways of handling combination of FIFO hardware
and triggers, it's good to add a little bit of info on that in this
patch description (I misunderstood what you were doing initially)




> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
> new file mode 100644
> index 000000000000..2d51a99a58fc
> --- /dev/null
> +++ b/drivers/iio/pressure/rohm-bm1390.c
> @@ -0,0 +1,944 @@

...

> +
> +enum {
> +	BM1390_CHAN_PRESSURE,
> +	BM1390_CHAN_TEMP,
> +};

> +static int bm1390_read_data(struct bm1390_data *data,
> +			struct iio_chan_spec const *chan, int *val, int *val2)
> +{
> +	int ret;
> +
> +	*val2 = 0;

Why?  No need to pass val2 in or zero it if going to return IIO_VAL_INT to the
IIO core.  It's never used.

> +
> +	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;
> +	}
> +	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}

> +
> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
> +			       bool irq)
> +{
> +	/* The fifo holds maximum of 4 samples */

Whilst useful info - why have comment here?

> +	struct bm1390_data *data = iio_priv(idev);
> +	struct bm1390_data_buf buffer;
> +	int smp_lvl, ret, i;
> +	u64 sample_period;
> +	__be16 temp = 0;
> +
> +	/*
> +	 * If the IC is accessed during FIFO read samples can be dropped.
> +	 * Prevent access until FIFO_LVL is read
> +	 */
> +	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;
> +	}
> +
> +	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 > 4) {
> +		/*
> +		 * 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 = 4;
> +	}
> +
> +	if (!smp_lvl)
> +		return ret;
> +
> +	sample_period = data->timestamp - data->old_timestamp;
> +	do_div(sample_period, smp_lvl);
> +
> +	if (samples && smp_lvl > samples)
> +		smp_lvl = samples;
> +
> +	for (i = 0; i < smp_lvl; i++) {
> +		ret = bm1390_pressure_read(data, &buffer.pressure);
> +		if (ret)
> +			break;
> +
> +		buffer.temp = temp;
> +		/*
> +		 * 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;
> +
> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
> +						   data->old_timestamp);
> +	}
> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
> +	regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);

ret = 

> +
> +	if (!ret)
> +		return ret;
> +
> +	return smp_lvl;
> +}

...

> +static int bm1390_write_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(idev);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	default:
> +		ret = -EINVAL;

?

> +	}
> +	iio_device_release_direct_mode(idev);
> +
> +	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;

It's definitely possible to argue a watermark of 1 is also supported - just
run in bypass mode with data ready interrupt.  Not sure we care enough to 
make that supported in this interface though.  Driver doesn't need to
support everything possible.


> +
> +	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,
> +	.write_raw = &bm1390_write_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;
> +
> +	ret = regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
> +				 BM1390_MASK_IIR_MODE, BM1390_IIR_MODE_MID);
> +
> +	return ret;

return regmap...

> +}
> +
> +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;
> +
> +	mutex_lock(&data->mutex);
> +
> +	/* 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);

check ret.

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

FIFO

> +	 */
> +	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, dummy;
> +
> +	/* DRDY is acked by reading status reg */
> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS,
> +			  &dummy);

I'd expect 'dummy' to be irrelevant data.  Perhaps give it
a better name if you are going to use the value like this.

> +	if (ret || !dummy)
> +		return IRQ_NONE;
> +
> +	dev_dbg(data->dev, "DRDY trig status 0x%x\n", dummy);
> +
> +	ret = bm1390_pressure_read(data, &data->buf.pressure);
> +	if (ret) {
> +		dev_warn(data->dev, "sample read failed %d\n", ret);
> +		return IRQ_NONE;
As below.  Still our interrupt even if we couldn't read the data
As such, IRQ_HANDLED; is current reutrn value.
> +	}
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
> +			       &data->buf.temp, BM1390_TEMP_SIZE);
> +
ret not checked.

> +	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;
> +	}
> +
> +	if (data->state == BM1390_STATE_FIFO) {

Can this and trigger_enabled be true?  

> +		int ok;
> +
> +		ok = __bm1390_fifo_flush(idev, BM1390_TEMP_SIZE, true);
> +		if (ok > 0)
> +			ret = IRQ_HANDLED;

An error should not result in IRQ_NONE.  At most we tend to print
an error message and return IRQ_HANDLED anyway to avoid the interrupt
being treated as spurious.

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

Why not?  Spec reference ideally.  I haven't gotten my head around this
but I'd expect an attempt to set the trigger state to indicate that we don't
want to use the fifo and as such it should be disabled in this path.

> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	data->trigger_enabled = state;
> +
> +	if (state) {
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);

Is this a trigger characteristic?  Feels more like a feature of enabling buffering.
Ah well. That's always a blurry boundary so it is probably fine here.

ret set but not checked...  

> +	} else {
> +		int dummy;
> +
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
> +
> +		/*
> +		 * 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.
> +		 */
> +		if (!ret)
> +			ret = regmap_read(data->regmap, BM1390_REG_STATUS,
> +					  &dummy);

Should check for errors in ret.

> +	}
> +
> +	ret = bm1390_set_drdy_irq(data, state);
> +	if (ret)
> +		goto unlock_out;

Not a lot of point in that :)

> +
> +
> +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_trigger(struct bm1390_data *data, struct iio_dev *idev,
> +				int irq)
> +{
> +	struct iio_trigger *itrig;
> +	char *name;
> +	int ret;
> +
> +	if (irq < 0) {
> +		dev_warn(data->dev, "No IRQ - skipping buffering\n");

I don't see that as deserving a warning. Device should be fine with out it.
Maybe dev_info could be justified (just).

> +		return 0;
> +	}
> +
> +	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");
> +
> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
> +					    iio_device_id(idev));
> +	if (!itrig)
> +		return -ENOMEM;
> +
> +	data->trig = itrig;
> +	idev->available_scan_masks = bm1390_scan_masks;
> +
> +	itrig->ops = &bm1390_trigger_ops;
> +	iio_trigger_set_drvdata(itrig, data);



> +
> +	/*
> +	 * No need to check for NULL. request_threaded_irq() defaults to
> +	 * dev_name() should the alloc fail.
> +	 */
> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
> +			      dev_name(data->dev));

Whist true, checking for it is less code than not doing so and
adding the comment. So I'd just check for it!

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

return 0 as that's the only value and it's good to make it clear that
was the intention in this path.


> +}
> +
> +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");
> +
> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
> +
> +	data = iio_priv(idev);
> +
> +	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);
> +
> +	data->regmap = regmap;
> +	data->dev = dev;
> +	data->irq = i2c->irq;
> +	/*
> +	 * Default watermark to WMI_MAX. We could also allow setting WMI to 0,
> +	 * and interpret that as "WMI is disabled, use FIFO_FULL" but I've
> +	 * no idea what is assumed if watermark is 0. Does it mean each sample
> +	 * should trigger IRQ, or no samples should do that?

In my head at least, if we are using fifo_full it is still a watermark but one
that matches size of buffer.
I think for previous drivers we've had 0 and 1 both meaning it's a dataready
interrupt (i.e. one entry in the fifo).  That would be fine here as well
and would correspond to putting the device in bypass mode.
	
> +	 *
> +	 * Well, 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;

The triggered buffer setup should set the second of these inside
the IIO core code (as it always needs to be set if that code is called).


> +
> +	ret = bm1390_chip_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "sensor init failed\n");
> +
> +	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 ret;
> +}
> +
> +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,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

I don't mind seeing this but I do want it to have a comment saying 'why'.
I assume there is something really slow involved in probing this?

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

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/8/23 21:45, Jonathan Cameron wrote:
> 
>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>> +{
>> +	u8 temp_reg[2] __aligned(2);
>> +	__be16 *temp_raw;
>> +	int ret;
>> +	s16 val;
>> +	bool negative;
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>> +			       sizeof(temp_reg));
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (temp_reg[0] & 0x80)
>> +		negative = true;
>> +	else
>> +		negative = false;
>> +
>> +	temp_raw = (__be16 *)&temp_reg[0];
>> +	val = be16_to_cpu(*temp_raw);
>> +
>> +	if (negative) {
>> +		/*
>> +		 * Two's complement. I am not sure if all supported
>> +		 * architectures actually use 2's complement represantation of
> 
> AFAIK they do. Many IIO drivers would be broken if not..

Great. Then I think it means we really can treat the combined value of 
the registers as s16 BE. Should make this much simpler :)

> 
>> +		 * signed ints. If yes, then we could just do the endianes
>> +		 * conversion and say this is the s16 value. However, as I
>> +		 * don't know, and as the conversion is pretty simple. let's
>> +		 * just convert the signed 2's complement to absolute value and
>> +		 * multiply by -1.
>> +		 */
>> +		val = ~val + 1;
>> +		val *= -1;
>> +	}
>> +
>> +	*temp = val;
>> +
>> +	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 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/8/23 21:44, Jonathan Cameron wrote:
> On Fri, 8 Sep 2023 09:12:51 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 9/7/23 16:57, Andy Shevchenko wrote:
>>> On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:
>>>> On 9/6/23 18:48, Andy Shevchenko wrote:
>>>>> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:
>>>
>>> ...
>>>    
>>>>>> +struct bm1390_data {
>>>>>> +	int64_t timestamp, old_timestamp;
>>>>>
>>>>> Out of a sudden int64_t instead of u64?
>>>>
>>>> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
>>>> operates with signed timestamps. One being s64, the other int64_t.
>>>>   
>>>>>> +	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;
>>>>>
>>>>> Or u8 instead of uint8_t?
>>>>
>>>> So, uint8_t is preferred? I don't really care all that much which of these
>>>> to use - which may even show up as a lack of consistency... I think I did
>>>> use uint8_t when I learned about it - but at some point someone somewhere
>>>> asked me to use u8 instead.. This somewhere might have been u-boot though...
>>>>
>>>> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
>>>> matters.
>>>
>>> Consistency matters, since I do not know the intention behind, I suggest use
>>> either, but be consistent in the entire code. However, uXX are specific Linux
>>> kernel internal types and some maintainers prefer them. Also you may grep for
>>> the frequency of intXX_t vs. sXX or their unsigned counterparts.
>>>    
>>>>>> +	/* Prevent accessing sensor during FIFO read sequence */
>>>>>> +	struct mutex mutex;
>>>>>> +};
>>>
>>> ...
>>>    
>>>>>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>>>>>> +{
>>>>>> +	u8 temp_reg[2] __aligned(2);
>>>>>
>>>>> Why?! Just use proper bitwise type.
>>>>
>>>> What is the proper bitwise type in this case?
>>>>
>>>> I'll explain my reasoning:
>>>> What we really have in hardware (BM1390) and read from it is 8bit registers.
>>>> This is u8 to me. And as we read two consecutive registers, we use u8
>>>> arr[2]. In my eyes it describes the data perfectly well, right?
>>>
>>> Two different registers?! Why bulk is used in that case?
>>> To me looks like you are reading 16-bit (or one that fits in 16-bit) register
>>> in BE notation.
>>
>> As I wrote, it is two 8 bit registers at consecutive addresses. And this
>> is what u8 arr[2]; also says.
>>
>> Bulk read is mandatory as HW has a special feature of preventing the
>> update of these registers when a read is ongoing. If we do two reads we
>> risk of getting one of the registers updated between the accesses -
>> resulting incorrect value.
> 
> Far as the kernel is concerned this is a __be16 and we use a bulk read to
> fill it from two registers.  If the use showed them having unrelated uses
> then it would be fair to handle it as an array of u8.

After you explained me that we don't need the extra hassle with the 
negative values (specific two's complement handling) - I do agree. If we 
needed to check the high bit in reg X to detect if special handling is 
required (as I thought) - then I would have still thought that checking 
the high bit from u8 which matches the spec would be much clearer. But 
yes - in this case, after your explanation - I do agree. Thanks :)

> 
>>
>>>    
>>>>>> +	__be16 *temp_raw;
>>>>>> +	int ret;
>>>>>> +	s16 val;
>>>>>> +	bool negative;
>>>>>> +
>>>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>>>>>> +			       sizeof(temp_reg));
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (temp_reg[0] & 0x80)
>>>>>> +		negative = true;
>>>>>> +	else
>>>>>> +		negative = false;
>>>>>   
>>>>>> +	temp_raw = (__be16 *)&temp_reg[0];
>>>>>
>>>>> Heck no. Make temp_reg be properly typed.
>>>>
>>>> As I explained above, to me it seems ur arr[2} is _the_ proper type to
>>>> represent data we read from the device.
>>>>
>>>> What we need to do is to convert the 16bits of data to an integer we can
>>>> give to the rest of the system. And, we happen to know how to 'manipulate'
>>>> the data to get it in format of understandable integer. As we have these 16
>>>> bits in memory, aligned to 2 byte boundary - why shouldn't we just
>>>> 'manipulate' the data and say - here we have your integer, please be my
>>>> guest and use it?
>>>
>>> Because it smell like a hack and is a hack here.
>>> Either it's a single BE16 register, or it's two different registers that by
>>> very unknown reason you are reading in a bulk.
>>
>> It is two registers. The bulk read might warrant a comment - although I
>> believe this is nothing unusual. If it is a hack or not is an opinion.
>> To me it looks like a code that explicitly shows what data is and how it
>> is being handled. It does what it is supposed to do and shows it in all
>> dirty details.
> 
> Read it directly into a __be16

Yes. This makes sense also to me now that we don't really need the:
 >>>>>> +	if (temp_reg[0] & 0x80)
 >>>>>> +		negative = true;
 >>>>>> +	else
 >>>>>> +		negative = false;

shown above.

> 
>>
>> Still, I am open to suggestions but I'd prefer seeing a real improvement
>> instead of claiming that the hardware is something it is not (eg, having
>> 16bit registers or should be read by individual accesses).
>>
>> The code in this form is no
>>> go.
>>>    
>>>> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
>>>> please explain me what this correct type for two 8bit integers is? Maybe I
>>>> can improve.
>>>
>>> If the registers are not of the same nature the bulk access is wrong.
>>> Use one by one reads.
>>
>> Of same nature? As I said, there is 2 8bit registers at consecutive
>> addresses. They have no other 'nature' as far as I can tell.
>>
>> Data in these registers in not in standard format - it needs to be
>> manipulated to make it an ordinary integer. The code shows this very
>> clearly by not reading it in any standard integer.
> 
> I'm not convinced it does.  All support arch are 2s complement.
> be16_to_cpu() is fine for what you have unless I'm missing something.
> If it weren't we would have signed versions of those macros.

No. You don't miss a thing. I didn't know if it's ok to treat i6bit 
two's complement just as a native 16bit signed integer.

>>> ...
>>>    
>>>>>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +	u8 raw[3];
>>>>>> +
>>>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>>>>>> +			       &raw[0], sizeof(raw));
>>>
>>> &raw[0] --> raw
>>>    
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
>>>
>>> This, btw, looks like le24, but I'm puzzled with right shift.
>>> I need to read datasheet carefully to understand this.
>>
>> It's not just le24. We, again, have data placed in registers so that it
>> needs some suffling. The data-sheet does decent job explaining it
>> though. AFAIR, there was a 'gap' in bits so it needed some more suffling
>> to sift the bits so that they're consecutive. I think this indeed is
>> something that needs to be looked up from data-sheet to understand why
>> this play with bits is done.
> 
> 
> These cases are harder to argue.  I'm fine with either approach for this one
> as a get_unaligned_le24() >> 2 would give same answer unless I'm also missing
> something but it isn't that obvious.

I need to look at the spec and the get_unaligned_le24() to be able to 
say something :) I wrote this code before my summer holidays but didn't 
have the time to test and send the patches until now. Will get back to 
this IC later this week.

Thanks for the feedback guys!

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 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/10/23 16:22, Jonathan Cameron wrote:
> On Wed, 6 Sep 2023 15:37:48 +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).
> 
> *sigh*  We all love fuzzily described filters...
> 
>>
>> 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.
> 
> IIRC this is a bit different from what we've typically done in the past
> where we just take the view that a slow moving thing like temperature
> can be read from sysfs or that enabling it disables the watermark.
> 
> However, what you have seems like valid use of the ABI to me so I'm
> fine with this approach if it makes sense for your expected users
> (which you get to guess at ;)
> 
> Given there are multiple ways of handling combination of FIFO hardware
> and triggers, it's good to add a little bit of info on that in this
> patch description (I misunderstood what you were doing initially)
> 
My idea is that the sensor can either be used in two 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.

Do you mean the commit message should explain this?

>> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
>> new file mode 100644
>> index 000000000000..2d51a99a58fc
>> --- /dev/null
>> +++ b/drivers/iio/pressure/rohm-bm1390.c
>> @@ -0,0 +1,944 @@
> 
> ...
> 
>> +
>> +enum {
>> +	BM1390_CHAN_PRESSURE,
>> +	BM1390_CHAN_TEMP,
>> +};
> 
>> +static int bm1390_read_data(struct bm1390_data *data,
>> +			struct iio_chan_spec const *chan, int *val, int *val2)
>> +{
>> +	int ret;
>> +
>> +	*val2 = 0;
> 
> Why?  No need to pass val2 in or zero it if going to return IIO_VAL_INT to the
> IIO core.  It's never used.

Ok

> 
>> +
>> +	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;
>> +	}
>> +	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
>> +unlock_out:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
> 
>> +
>> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
>> +			       bool irq)
>> +{
>> +	/* The fifo holds maximum of 4 samples */
> 
> Whilst useful info - why have comment here?

It really looks like it's missplaced. I may have suffled some code to 
other place without moving the comment. I'll drop or move it - thanks!

> 
>> +	struct bm1390_data *data = iio_priv(idev);
>> +	struct bm1390_data_buf buffer;
>> +	int smp_lvl, ret, i;
>> +	u64 sample_period;
>> +	__be16 temp = 0;
>> +
>> +	/*
>> +	 * If the IC is accessed during FIFO read samples can be dropped.
>> +	 * Prevent access until FIFO_LVL is read
>> +	 */
>> +	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;
>> +	}
>> +
>> +	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 > 4) {
>> +		/*
>> +		 * 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 = 4;
>> +	}
>> +
>> +	if (!smp_lvl)
>> +		return ret;
>> +
>> +	sample_period = data->timestamp - data->old_timestamp;
>> +	do_div(sample_period, smp_lvl);
>> +
>> +	if (samples && smp_lvl > samples)
>> +		smp_lvl = samples;
>> +
>> +	for (i = 0; i < smp_lvl; i++) {
>> +		ret = bm1390_pressure_read(data, &buffer.pressure);
>> +		if (ret)
>> +			break;
>> +
>> +		buffer.temp = temp;
>> +		/*
>> +		 * 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;
>> +
>> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
>> +						   data->old_timestamp);
>> +	}
>> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
>> +	regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
> 
> ret =

I didn't want to determine success of getting the data by success of 
this read. Instead, I wanted to ret to contain the success of reading 
the pressure in the for-loop. We might spill out a warning if this reads 
fails though.

> 
>> +
>> +	if (!ret)
>> +		return ret;
>> +
>> +	return smp_lvl;
>> +}
> 
> ...
> 
>> +static int bm1390_write_raw(struct iio_dev *idev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int val, int val2, long mask)
>> +{
>> +	int ret;
>> +
>> +	ret = iio_device_claim_direct_mode(idev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (mask) {
>> +	default:
>> +		ret = -EINVAL;
> 
> ?

Yeah :) This was forgotten when I dropped the OVERSAMPLING setting. Need 
to drop thw whole raw_write.

> 
>> +	}
>> +	iio_device_release_direct_mode(idev);
>> +
>> +	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;
> 
> It's definitely possible to argue a watermark of 1 is also supported - just
> run in bypass mode with data ready interrupt.  Not sure we care enough to
> make that supported in this interface though.  Driver doesn't need to
> support everything possible.

True. I'm not sure this is worth the complexity though. It'd mean we had 
to support DRDY when in FIFO mode - iff watermark is 1. Currently we 
don't need to bother the DRDY or FIFO disabling when in FIFO mode.

>> +
>> +	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,
>> +	.write_raw = &bm1390_write_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;
>> +
>> +	ret = regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
>> +				 BM1390_MASK_IIR_MODE, BM1390_IIR_MODE_MID);
>> +
>> +	return ret;
> 
> return regmap...

Ok.

> 
>> +}
>> +
>> +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;
>> +
>> +	mutex_lock(&data->mutex);
>> +
>> +	/* 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);
> 
> check ret.
> 

Yes.

>> +	/* 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.
> 
> FIFO
> 

Ok

>> +	 */
>> +	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, dummy;
>> +
>> +	/* DRDY is acked by reading status reg */
>> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS,
>> +			  &dummy);
> 
> I'd expect 'dummy' to be irrelevant data.  Perhaps give it
> a better name if you are going to use the value like this.
> 

Ok.

>> +	if (ret || !dummy)
>> +		return IRQ_NONE;
>> +
>> +	dev_dbg(data->dev, "DRDY trig status 0x%x\n", dummy);
>> +
>> +	ret = bm1390_pressure_read(data, &data->buf.pressure);
>> +	if (ret) {
>> +		dev_warn(data->dev, "sample read failed %d\n", ret);
>> +		return IRQ_NONE;
> As below.  Still our interrupt even if we couldn't read the data
> As such, IRQ_HANDLED; is current reutrn value.

I've learned a hard way that it can be a very good idea to return 
IRQ_NONE if HW access fails in IRQ handling. If we don't (can't) ack the 
IRQ line, we'll be sitting in IRQ handler. The feature which disables 
local IRQ after <N> unhandled IRQs has made a huge difference when I've 
received bug reports from people. Being able to access the device when 
the 'wild' IRQ gets disabled and being able to retrieve the logs is 
priceless. Also, seeing the 'nobody cares, disabling IRQ' in the log is 
a really valuable hint :)

>> +	}
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
>> +			       &data->buf.temp, BM1390_TEMP_SIZE);
>> +
> ret not checked.

Ok.

>> +	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;
>> +	}
>> +
>> +	if (data->state == BM1390_STATE_FIFO) {
> 
> Can this and trigger_enabled be true?

No, it shouldn't be possible.

> 
>> +		int ok;
>> +
>> +		ok = __bm1390_fifo_flush(idev, BM1390_TEMP_SIZE, true);
>> +		if (ok > 0)
>> +			ret = IRQ_HANDLED;
> 
> An error should not result in IRQ_NONE.  At most we tend to print
> an error message and return IRQ_HANDLED anyway to avoid the interrupt
> being treated as spurious.

The same story here. If we can't read the samples from the FIFO, we'll 
end up having new watermark IRQ immediately when the oneshot thread 
exits. Returning the IRQ none allows genirq core to disable the host 
side of the IRQ, allowing us to see the 'nobody cared' and go get the 
logs even with single core system.

> 
>> +	}
>> +
>> +	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");
> 
> Why not?  Spec reference ideally.  I haven't gotten my head around this
> but I'd expect an attempt to set the trigger state to indicate that we don't
> want to use the fifo and as such it should be disabled in this path.

This may be my limited understanding of how IIO is typically used. To me 
it sounded a bit strange that someone setting trigger could abort an 
ongoing buffered measurement. I would have expected the buffering to be 
explicitly disabled by user before allowing the trigger to be used. 
Especially because the trigger and FIFO are pretty much exclusive 
features (as with data-ready trigger you'll get each individual sample 
read). My assumption was that an attempt to enable data-ready when FIFO 
was in use is a misconfiguration.

> 
>> +		ret = -EBUSY;
>> +		goto unlock_out;
>> +	}
>> +
>> +	data->trigger_enabled = state;
>> +
>> +	if (state) {
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
> 
> Is this a trigger characteristic?  Feels more like a feature of enabling buffering.
> Ah well. That's always a blurry boundary so it is probably fine here.

Well, the mode needs to be set to continuous even for a single shot 
measurement when IIR filter is used. Furthermore, if we use the 
single-shot mode, then the mode must be re-written to register after 
every colleted sample. Hence we set the continuous mode also when 
trigger is used.

It might be we could set this mode already at probe and forget it then. 
Well, maybe it has some impact to power consumption (?) so perhaps we 
just leave the continuous mode to be enabled for all, triggered, FIFO 
and raw sample collections.

> ret set but not checked...

Thanks for pointing this out - I need to check (what / if) I have been 
thinking :)

> 
>> +	} else {
>> +		int dummy;
>> +
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
>> +
>> +		/*
>> +		 * 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.
>> +		 */
>> +		if (!ret)
>> +			ret = regmap_read(data->regmap, BM1390_REG_STATUS,
>> +					  &dummy);
> 
> Should check for errors in ret.
> 
>> +	}
>> +
>> +	ret = bm1390_set_drdy_irq(data, state);
>> +	if (ret)
>> +		goto unlock_out;
> 
> Not a lot of point in that :)

I like the way you put it :) Better to clean this too.

> 
>> +
>> +
>> +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_trigger(struct bm1390_data *data, struct iio_dev *idev,
>> +				int irq)
>> +{
>> +	struct iio_trigger *itrig;
>> +	char *name;
>> +	int ret;
>> +
>> +	if (irq < 0) {
>> +		dev_warn(data->dev, "No IRQ - skipping buffering\n");
> 
> I don't see that as deserving a warning. Device should be fine with out it.
> Maybe dev_info could be justified (just).

Ok.

> 
>> +		return 0;
>> +	}
>> +
>> +	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");
>> +
>> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
>> +					    iio_device_id(idev));
>> +	if (!itrig)
>> +		return -ENOMEM;
>> +
>> +	data->trig = itrig;
>> +	idev->available_scan_masks = bm1390_scan_masks;
>> +
>> +	itrig->ops = &bm1390_trigger_ops;
>> +	iio_trigger_set_drvdata(itrig, data);
> 
> 
> 
>> +
>> +	/*
>> +	 * No need to check for NULL. request_threaded_irq() defaults to
>> +	 * dev_name() should the alloc fail.
>> +	 */
>> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
>> +			      dev_name(data->dev));
> 
> Whist true, checking for it is less code than not doing so and
> adding the comment. So I'd just check for it!

comment does not eat cycles :) Well, I know everyone is saying this does 
not matter as we're far from a hot path... Sigh, I guess you're right 
while I also know why we need such powerful computers while we do same 
basic things we did 10 years ago...

> 
>> +
>> +	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 ret;
> 
> return 0 as that's the only value and it's good to make it clear that
> was the intention in this path.

Ok

> 
> 
>> +}
>> +
>> +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");
>> +
>> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!idev)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_regulator_get_enable(dev, "vdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
>> +
>> +	data = iio_priv(idev);
>> +
>> +	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);
>> +
>> +	data->regmap = regmap;
>> +	data->dev = dev;
>> +	data->irq = i2c->irq;
>> +	/*
>> +	 * Default watermark to WMI_MAX. We could also allow setting WMI to 0,
>> +	 * and interpret that as "WMI is disabled, use FIFO_FULL" but I've
>> +	 * no idea what is assumed if watermark is 0. Does it mean each sample
>> +	 * should trigger IRQ, or no samples should do that?
> 
> In my head at least, if we are using fifo_full it is still a watermark but one
> that matches size of buffer.
> I think for previous drivers we've had 0 and 1 both meaning it's a dataready
> interrupt (i.e. one entry in the fifo).  That would be fine here as well
> and would correspond to putting the device in bypass mode.

Thanks for the explanation. It makes sense.
Meanwhile I was reading the spec and I think the FIFO_FULL usage with 
this IC is like asking for a nose-bleed. FIFO_FULL IRQ is acked by 
reading the status - and if data is not read from the FIFO, then the 
status will remain cleared (and I assume also the IRQ line will stay 
low). I assume it's easy enough to write a code that acks the IRQ while 
leaving the buffer full - and effectively killing the FIFO_FULL IRQ. No 
more samples for you then.

> 	
>> +	 *
>> +	 * Well, 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;
> 
> The triggered buffer setup should set the second of these inside
> the IIO core code (as it always needs to be set if that code is called).

Right. Thanks.
> 
> 
>> +
>> +	ret = bm1390_chip_init(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "sensor init failed\n");
>> +
>> +	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 ret;
>> +}
>> +
>> +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,
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> 
> I don't mind seeing this but I do want it to have a comment saying 'why'.
> I assume there is something really slow involved in probing this?

I wouldn't say there is anything really slow. What I have gathered is:

1) The grand idea is that at the end of the day (week, month, year) 
async probing should be the default when it causes no problems.

2) Problems are rare (although they may exist).

3) All regulator drivers were changes (or suggested to be changed) to do 
async probing because for example the ramp-rates may be slow.

4) This driver enables the VDD supply - with potentially slow ramp-rate 
(well, the I need to guess user's setups statement holds again) so it 
may potentially take a while (probably not long though).

5) We have a few msleeps() in bm1390_chip_init() - which are likely to 
result a sleep tens of milliseconds. So there is some delay.

 From those 5 things I derived assumption that async probe would be the 
right thing to start with. We can drop it if it causes a problem in an 
actual setup :)

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

Thanks again for all the help!

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 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

> >> 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.  
> > 
> > IIRC this is a bit different from what we've typically done in the past
> > where we just take the view that a slow moving thing like temperature
> > can be read from sysfs or that enabling it disables the watermark.
> > 
> > However, what you have seems like valid use of the ABI to me so I'm
> > fine with this approach if it makes sense for your expected users
> > (which you get to guess at ;)
> > 
> > Given there are multiple ways of handling combination of FIFO hardware
> > and triggers, it's good to add a little bit of info on that in this
> > patch description (I misunderstood what you were doing initially)
> >   
> My idea is that the sensor can either be used in two 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.
> 
> Do you mean the commit message should explain this?
yes


> >> +
> >> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
> >> +			       bool irq)
> >> +{
> >> +	/* The fifo holds maximum of 4 samples */  
> > 
> > Whilst useful info - why have comment here?  
> 
> It really looks like it's missplaced. I may have suffled some code to 
> other place without moving the comment. I'll drop or move it - thanks!
> 
> >   
> >> +	struct bm1390_data *data = iio_priv(idev);
> >> +	struct bm1390_data_buf buffer;
> >> +	int smp_lvl, ret, i;
> >> +	u64 sample_period;
> >> +	__be16 temp = 0;
> >> +
> >> +	/*
> >> +	 * If the IC is accessed during FIFO read samples can be dropped.
> >> +	 * Prevent access until FIFO_LVL is read
> >> +	 */
> >> +	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;
> >> +	}
> >> +
> >> +	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 > 4) {
> >> +		/*
> >> +		 * 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 = 4;
> >> +	}
> >> +
> >> +	if (!smp_lvl)
> >> +		return ret;
> >> +
> >> +	sample_period = data->timestamp - data->old_timestamp;
> >> +	do_div(sample_period, smp_lvl);
> >> +
> >> +	if (samples && smp_lvl > samples)
> >> +		smp_lvl = samples;
> >> +
> >> +	for (i = 0; i < smp_lvl; i++) {
> >> +		ret = bm1390_pressure_read(data, &buffer.pressure);
> >> +		if (ret)
> >> +			break;
> >> +
> >> +		buffer.temp = temp;
> >> +		/*
> >> +		 * 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;
> >> +
> >> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
> >> +						   data->old_timestamp);
> >> +	}
> >> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
> >> +	regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);  
> > 
> > ret =  
> 
> I didn't want to determine success of getting the data by success of 
> this read. Instead, I wanted to ret to contain the success of reading 
> the pressure in the for-loop. We might spill out a warning if this reads 
> fails though.

Does it mean the next read is going to fail?  I'm too lazy to look at
datasheet to see what you mean by FIFO access sequence.
If it does I'd rather we reported error as early as possible.

Mind you I can't remember if errors in here result in anything useful anyway.




> >> +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;  
> > 
> > It's definitely possible to argue a watermark of 1 is also supported - just
> > run in bypass mode with data ready interrupt.  Not sure we care enough to
> > make that supported in this interface though.  Driver doesn't need to
> > support everything possible.  
> 
> True. I'm not sure this is worth the complexity though. It'd mean we had 
> to support DRDY when in FIFO mode - iff watermark is 1. Currently we 
> don't need to bother the DRDY or FIFO disabling when in FIFO mode.

Ok.


> 
> >> +	if (ret || !dummy)
> >> +		return IRQ_NONE;
> >> +
> >> +	dev_dbg(data->dev, "DRDY trig status 0x%x\n", dummy);
> >> +
> >> +	ret = bm1390_pressure_read(data, &data->buf.pressure);
> >> +	if (ret) {
> >> +		dev_warn(data->dev, "sample read failed %d\n", ret);
> >> +		return IRQ_NONE;  
> > As below.  Still our interrupt even if we couldn't read the data
> > As such, IRQ_HANDLED; is current reutrn value.  
> 
> I've learned a hard way that it can be a very good idea to return 
> IRQ_NONE if HW access fails in IRQ handling. If we don't (can't) ack the 
> IRQ line, we'll be sitting in IRQ handler. The feature which disables 
> local IRQ after <N> unhandled IRQs has made a huge difference when I've 
> received bug reports from people. Being able to access the device when 
> the 'wild' IRQ gets disabled and being able to retrieve the logs is 
> priceless. Also, seeing the 'nobody cares, disabling IRQ' in the log is 
> a really valuable hint :)

Hmm.  Usually the warn splat works out as enough to provide
a hint. Kind of depends on whether the particular failure blocks correct
handling or not.  Some devices don't care if you successfully
read the buffer or not.  Its always annoyed me that there is no
IRQ_ERROR return.


> 
> >   
> >> +	}
> >> +
> >> +	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");  
> > 
> > Why not?  Spec reference ideally.  I haven't gotten my head around this
> > but I'd expect an attempt to set the trigger state to indicate that we don't
> > want to use the fifo and as such it should be disabled in this path.  
> 
> This may be my limited understanding of how IIO is typically used. To me 
> it sounded a bit strange that someone setting trigger could abort an 
> ongoing buffered measurement. I would have expected the buffering to be 
> explicitly disabled by user before allowing the trigger to be used.

We mostly try to enable as many userspace paths as possible.  If they
ask for something and we can easily do it (even if it stops something
else) then we are fine with it.  
 
> Especially because the trigger and FIFO are pretty much exclusive 
> features (as with data-ready trigger you'll get each individual sample 
> read). My assumption was that an attempt to enable data-ready when FIFO 
> was in use is a misconfiguration.

I agree, but doesn't mean we aren't nice to them anyway :)  Expectation
is they should go back to a sensible state before setting up something new
and complex.


Now if it's tricky to handle in code then we can indeed use an error.

I have better understood how you are are handing fifo vs data ready
now hence I'm not that bothered by this spitting out an error.  It's the same
as changing a trigger when a buffer is running which makes little sense,
though it only applies in INDIO_BUFFER_TRIGGERED mode.
Perhaps we should broaden that and then we'll block much earlier.
 
...



> > 
> >   
> >> +}
> >> +
> >> +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");
> >> +
> >> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
> >> +	if (!idev)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = devm_regulator_get_enable(dev, "vdd");
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
> >> +
> >> +	data = iio_priv(idev);
> >> +
> >> +	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);
> >> +
> >> +	data->regmap = regmap;
> >> +	data->dev = dev;
> >> +	data->irq = i2c->irq;
> >> +	/*
> >> +	 * Default watermark to WMI_MAX. We could also allow setting WMI to 0,
> >> +	 * and interpret that as "WMI is disabled, use FIFO_FULL" but I've
> >> +	 * no idea what is assumed if watermark is 0. Does it mean each sample
> >> +	 * should trigger IRQ, or no samples should do that?  
> > 
> > In my head at least, if we are using fifo_full it is still a watermark but one
> > that matches size of buffer.
> > I think for previous drivers we've had 0 and 1 both meaning it's a dataready
> > interrupt (i.e. one entry in the fifo).  That would be fine here as well
> > and would correspond to putting the device in bypass mode.  
> 
> Thanks for the explanation. It makes sense.
> Meanwhile I was reading the spec and I think the FIFO_FULL usage with 
> this IC is like asking for a nose-bleed. FIFO_FULL IRQ is acked by 
> reading the status - and if data is not read from the FIFO, then the 
> status will remain cleared (and I assume also the IRQ line will stay 
> low). I assume it's easy enough to write a code that acks the IRQ while 
> leaving the buffer full - and effectively killing the FIFO_FULL IRQ. No 
> more samples for you then.

Its not a level IRQ?  What fun. Indeed silly corner case.

> 
> > 	  
> >> +	 *
> >> +	 * Well, 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;  
> > 
> > The triggered buffer setup should set the second of these inside
> > the IIO core code (as it always needs to be set if that code is called).  
> 
> Right. Thanks.

Oops. I'm wrong in this.  I'd forgotten the distinction between
INDIO_BUFFER_TRIGGERED and INDIO_BUFFER_SOFTWARE.

You should set this as you have done.

...

> >> +static struct i2c_driver bm1390_driver = {
> >> +	.driver = {
> >> +		.name = "bm1390",
> >> +		.of_match_table = bm1390_of_match,
> >> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,  
> > 
> > I don't mind seeing this but I do want it to have a comment saying 'why'.
> > I assume there is something really slow involved in probing this?  
> 
> I wouldn't say there is anything really slow. What I have gathered is:
> 
> 1) The grand idea is that at the end of the day (week, month, year) 
> async probing should be the default when it causes no problems.

Agreed.

> 
> 2) Problems are rare (although they may exist).
> 
> 3) All regulator drivers were changes (or suggested to be changed) to do 
> async probing because for example the ramp-rates may be slow.
> 
> 4) This driver enables the VDD supply - with potentially slow ramp-rate 
> (well, the I need to guess user's setups statement holds again) so it 
> may potentially take a while (probably not long though).
> 
> 5) We have a few msleeps() in bm1390_chip_init() - which are likely to 
> result a sleep tens of milliseconds. So there is some delay.
> 
>  From those 5 things I derived assumption that async probe would be the 
> right thing to start with. We can drop it if it causes a problem in an 
> actual setup :)
> 
I don't mind having it - I'd just like a little comment including what
minimum sleep time is and that a VDD supply enable may add time to it.

So set the bounds.

Jonathan

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/10/23 16:22, Jonathan Cameron wrote:
> On Wed, 6 Sep 2023 15:37:48 +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).

>> +
>> +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;
>> +	}
>> +
>> +	if (data->state == BM1390_STATE_FIFO) {
> 
> Can this and trigger_enabled be true?

Thanks for asking this question. Intention was that these are mutually 
exclusive. However, I think that the check
if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
in bm1390_buffer_postenable(), before calling the bm1390_fifo_enable() 
is not 100% race free.

I, however, like the idea of having this check in the buffer-enable 
function - I think it makes the design much more obvious. What I will do 
is adding another check for:
	if (data->trigger_enable) {
		ret = -EBUSY;
		goto unlock_out;
	}

inside the bm1390_fifo_enable() to the section which holds the mutex.


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 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Thu, 14 Sep 2023 14:47:44 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/10/23 16:22, Jonathan Cameron wrote:
> > On Wed, 6 Sep 2023 15:37:48 +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).  
> 
> >> +
> >> +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;
> >> +	}
> >> +
> >> +	if (data->state == BM1390_STATE_FIFO) {  
> > 
> > Can this and trigger_enabled be true?  
> 
> Thanks for asking this question. Intention was that these are mutually 
> exclusive. However, I think that the check
> if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
> in bm1390_buffer_postenable(), before calling the bm1390_fifo_enable() 
> is not 100% race free.
> 
> I, however, like the idea of having this check in the buffer-enable 
> function - I think it makes the design much more obvious. What I will do 
> is adding another check for:
> 	if (data->trigger_enable) {
> 		ret = -EBUSY;
> 		goto unlock_out;
> 	}
> 
> inside the bm1390_fifo_enable() to the section which holds the mutex.

You could make the exclusive nature obvious in the thread_handler by using an
else if () above. 

> 
> 
> Yours,
> 	-- Matti
> 
> 
> 

Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Matti Vaittinen]

On 9/17/23 12:26, Jonathan Cameron wrote:
> On Thu, 14 Sep 2023 14:47:44 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 9/10/23 16:22, Jonathan Cameron wrote:
>>> On Wed, 6 Sep 2023 15:37:48 +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).
>>
>>>> +
>>>> +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;
>>>> +	}
>>>> +
>>>> +	if (data->state == BM1390_STATE_FIFO) {
>>>
>>> Can this and trigger_enabled be true?
>>
>> Thanks for asking this question. Intention was that these are mutually
>> exclusive. However, I think that the check
>> if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
>> in bm1390_buffer_postenable(), before calling the bm1390_fifo_enable()
>> is not 100% race free.
>>
>> I, however, like the idea of having this check in the buffer-enable
>> function - I think it makes the design much more obvious. What I will do
>> is adding another check for:
>> 	if (data->trigger_enable) {
>> 		ret = -EBUSY;
>> 		goto unlock_out;
>> 	}
>>
>> inside the bm1390_fifo_enable() to the section which holds the mutex.
> 
> You could make the exclusive nature obvious in the thread_handler by using an
> else if () above.

Right. Thanks.

> 
>>
>>
>> Yours,
>> 	-- Matti
>>
>>
>>
> 

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

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