Support ROHM BM1390 pressure sensor

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

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

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

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

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

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

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

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

+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+			       raw, sizeof(raw));
+	if (ret < 0)
+		return ret;
+
+	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
+
+	return 0;
+}

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

+static int bm1390_read_data(struct bm1390_data *data,
+			struct iio_chan_spec const *chan, int *val, int *val2)
+{
+	int ret;
+
+	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);

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

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

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

+	}
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	struct bm1390_data_buf buffer;
+	int smp_lvl, ret, i, warn;
+	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)
+		return 0;
+
+	if (smp_lvl > 4) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = 4;
+	}
+
+	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;

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

+		return ret;
+
+	return smp_lvl;
+}

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

+static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
+				int irq)
+{
+	struct iio_trigger *itrig;
+	char *name;
+	int ret;
+
+	/* Nothing to do if we don't have IRQ for data-ready and WMI */
+	if (irq < 0)
+		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)

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

+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;

Le 15/09/2023 à 08:56, Matti Vaittinen a écrit :

Quoted 26 lines

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

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

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

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

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

Quoted 7 lines

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

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

...

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

Quoted 3 lines

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

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

I've not looked in details so I'm not sure if related, but bm1390_read_temp() seems to use int.

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

Quoted 5 lines

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

+	s64 ts __aligned(8);
+};
+
+/* Pressure data is in 3 8-bit registers */
+#define BM1390_PRESSURE_SIZE	3  

Unused? (see other comment below)

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

Quoted 6 lines

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

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

Unused? (see other comment below)

...

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

Quoted 3 lines

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

+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	__be16 temp_raw;  

Something to do with BM1390_TEMP_SIZE?

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

Quoted 9 lines

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

+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+			       sizeof(temp_raw));
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpu(temp_raw);  

See potential link with the comment above related to bm1390_data_buf.temp being a __be16 an temp being a int.

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

Quoted 9 lines

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

+
+	return 0;
+}
+
+static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
+{
+	int ret;
+	u8 raw[3];  

BM1390_PRESSURE_SIZE?

(not sure if it make sense because we still have [0..2] below, so having 3 here looks useful)

Quoted 10 lines

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

+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+			       raw, sizeof(raw));
+	if (ret < 0)
+		return ret;
+
+	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
+
+	return 0;
+}  

...

Quoted 17 lines

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

+static int bm1390_read_data(struct bm1390_data *data,
+			struct iio_chan_spec const *chan, int *val, int *val2)
+{
+	int ret;
+
+	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);

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

Quoted 10 lines

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

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

"ret =" missing, or done on purpose?

Quoted 47 lines

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

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

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

Quoted 4 lines

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

+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;  

Certainly useless, but should we break and return -EINVAL after the switch, so that it is more explicit that bm1390_read_raw() always returns a value?

Le 15/09/2023 à 08:56, Matti Vaittinen a écrit :

Quoted 26 lines

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

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

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

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

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

Quoted 7 lines

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

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

...

Quoted 3 lines

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

I've not looked in details so I'm not sure if related, but bm1390_read_temp() seems to use int.

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

Quoted 6 lines

+    s64 ts __aligned(8);

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

+};
+
+/* Pressure data is in 3 8-bit registers */
+#define BM1390_PRESSURE_SIZE    3

Unused? (see other comment below)

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

Quoted 6 lines

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

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

Unused? (see other comment below)

...

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

Quoted 3 lines

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

+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+    __be16 temp_raw;

Something to do with BM1390_TEMP_SIZE?

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

Quoted 8 lines

+    int ret;

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

+
+    ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+                   sizeof(temp_raw));
+    if (ret)
+        return ret;
+
+    *temp = be16_to_cpu(temp_raw);

See potential link with the comment above related to bm1390_data_buf.temp being a __be16 an temp being a int.

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

Quoted 8 lines

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

+
+    return 0;
+}
+
+static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
+{
+    int ret;
+    u8 raw[3];

BM1390_PRESSURE_SIZE?

(not sure if it make sense because we still have [0..2] below, so having 3 here looks useful)

Quoted 10 lines

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

+
+    ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+                   raw, sizeof(raw));
+    if (ret < 0)
+        return ret;
+
+    *pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
+
+    return 0;
+}

...

Quoted 22 lines

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

+static int bm1390_read_data(struct bm1390_data *data,
+            struct iio_chan_spec const *chan, int *val, int *val2)
+{
+    int ret;
+
+    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);

Quoted 6 lines

+        ret = bm1390_read_temp(data, val); +        break;
+    default:
+        ret = -EINVAL;
+    }
+    bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);

"ret =" missing, or done on purpose?

Quoted 46 lines

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

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

Quoted 6 lines

+        if (ret)
+            return ret;
+
+        return IIO_VAL_INT;
+    default:
+        return -EINVAL;

Certainly useless, but should we break and return -EINVAL after the switch, so that it is more explicit that bm1390_read_raw() always returns a value?

Quoted 70 lines

+    }

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

+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int 

samples,
+                   bool irq)

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

+{
+    struct bm1390_data *data = iio_priv(idev);
+    struct bm1390_data_buf buffer;
+    int smp_lvl, ret, i, warn;
+    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)
+        return 0;
+
+    if (smp_lvl > 4) {
+        /*
+         * The fifo holds maximum of 4 samples so valid values
+         * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+         * in I2C line. Don't overflow if this happens.
+         */
+        dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+        smp_lvl = 4;
+    }
+
+    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 */

Quoted 5 lines

+    warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl); +    if (warn)
+        dev_warn(data->dev, "Closing FIFO sequence failed\n"); +
+    if (!ret)

if (ret)?
If done on purpose "return 0;" would be more explicit.

Quoted 4 lines

+        return ret;

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

+
+    return smp_lvl;
+}

...

Quoted 29 lines

+static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
+                int irq)

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

+{
+    struct iio_trigger *itrig;
+    char *name;
+    int ret;
+
+    /* Nothing to do if we don't have IRQ for data-ready and WMI */
+    if (irq < 0)
+        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;

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

Quoted 6 lines

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

+
+    itrig->ops = &bm1390_trigger_ops;
+    iio_trigger_set_drvdata(itrig, data);
+
+    name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+                  dev_name(data->dev));

Missing NULL check?

Quoted 5 lines

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

+
+    ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+                    &bm1390_irq_thread_handler,
+                    IRQF_ONESHOT, name, idev);

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

Quoted 10 lines

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

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

Quoted 42 lines

Quoted 42 lines

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

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

Quoted 8 lines

Quoted 4 lines

+
+        return IIO_VAL_INT;
+    default:
+        return -EINVAL;

Certainly useless, but should we break and return -EINVAL after the switch, so that it is more explicit that bm1390_read_raw() always returns a value?

I think there is also opposite opinions on this. For my eyes the return at the end of the function would also be clearer - but I think I have been asked to drop the useless return when I've been working with other sensors in IIO domain :) My personal preference would definitely be:

int ret;

switch (foo)
{
case BAR:

	ret = func1();
	if (ret)
		break;

	ret = func2();
	if (ret)
		break;

...

	break;

case BAZ:

	ret = -EINVAL;
	break;
}

return ret;

- but I've learned to think this is not the IIO preference.

Quoted 77 lines

Quoted 42 lines

Quoted 42 lines

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

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

Quoted 8 lines

Quoted 4 lines

+
+        return IIO_VAL_INT;
+    default:
+        return -EINVAL;

Certainly useless, but should we break and return -EINVAL after the switch, so that it is more explicit that bm1390_read_raw() always returns a value?

I think there is also opposite opinions on this. For my eyes the return at the end of the function would also be clearer - but I think I have been asked to drop the useless return when I've been working with other sensors in IIO domain :) My personal preference would definitely be:

int ret;

switch (foo)
{
case BAR:

	ret = func1();
	if (ret)
		break;

	ret = func2();
	if (ret)
		break;

...

	break;

case BAZ:

	ret = -EINVAL;
	break;
}

Quoted 3 lines

return ret;

- but I've learned to think this is not the IIO preference.

Some static analyzers get confused (probably when there is a little bit more going on after the function) by that and moan that some cases are not considered in the switch. I got annoyed enough with the noise they were generating to advocate always having explicit defaults.

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

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

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

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

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

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

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

---
Revision history:
v1 => v2:
- prefer s64 over int64_t
- drop not needed handling of 2's complements
- plenty of styling changes
- drop dead code (write_raw)
- fix typos in comments
- explain trigger and FIFO usage in commit message
- do better job at cheking the return values
- ensure there's no race when checking if triggered buffer is used before enabling the FIFO
- print warning if register read fails at IRQ handler
- drop unnecessary warning if IRQ is not given
- explain why we prefer asynchronous probing
---

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

new file mode 100644
index 000000000000..d3cc229d1688
@@ -0,0 +1,899 @@

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

+};
 
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	__be16 temp_raw;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+			       sizeof(temp_raw));
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpu(temp_raw);
+
+	return 0;
+}
 
+static int bm1390_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,

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

+		}
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		ret = bm1390_read_data(data, chan, val, val2);
+		iio_device_release_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)

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

+	 */
+	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
+				       sizeof(temp));
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (ret)
+		return ret;
+
+	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+	if (!smp_lvl)
+		return 0;
+
+	if (smp_lvl > 4) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = 4;
+	}
+
+	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 */
+	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (warn)
+		dev_warn(data->dev, "Closing FIFO sequence failed\n");
+
+	if (!ret)
+		return ret;
+
+	return smp_lvl;

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

+	ret = __bm1390_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	enable_irq(data->irq);
+
+	return ret;
+}
 
+
+static irqreturn_t bm1390_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret, status;
+
+	/* DRDY is acked by reading status reg */
+	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
+	if (ret || !status)
+		return IRQ_NONE;
+
+	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
+

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

+
+	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;

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

+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state == BM1390_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	data->trigger_enabled = state;
+
+	if (state) {
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+		if (ret)
+			goto unlock_out;
+	} else {
+		int dummy;
+
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+		if (ret)
+			goto unlock_out;
+
+		/*
+		 * We need to read the status register in order to ACK the
+		 * data-ready which may have been generated just before we
+		 * disabled the measurement.
+		 */
+		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
+		if (ret)
+			dev_warn(data->dev, "status read failed\n");
+	}
+
+	ret = bm1390_set_drdy_irq(data, state);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops bm1390_trigger_ops = {
+	.set_trigger_state = bm1390_trigger_set_state,
+};
+

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

+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;
+}
+

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

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

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

+
+	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;
+	/*
+	 * 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 0;
+}
+
+static const struct of_device_id bm1390_of_match[] = {
+	{ .compatible = "rohm,bm1390glv-z" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bm1390_of_match);
+
+static const struct i2c_device_id bm1390_id[] = {
+	{ "bm1390glv-z", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, bm1390_id);
+
+static struct i2c_driver bm1390_driver = {
+	.driver = {
+		.name = "bm1390",

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

+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

Quoted 31 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

Quoted 1 line

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

Hi Matti,

I think this is coming together nicely. A few comments inline.

Quoted 25 lines

---
Revision history:
v1 => v2:
- prefer s64 over int64_t
- drop not needed handling of 2's complements
- plenty of styling changes
- drop dead code (write_raw)
- fix typos in comments
- explain trigger and FIFO usage in commit message
- do better job at cheking the return values
- ensure there's no race when checking if triggered buffer is used before enabling the FIFO
- print warning if register read fails at IRQ handler
- drop unnecessary warning if IRQ is not given
- explain why we prefer asynchronous probing
---

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

new file mode 100644
index 000000000000..d3cc229d1688
@@ -0,0 +1,899 @@

...

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

Quoted 3 lines

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

+
+static const unsigned long bm1390_scan_masks[] = {
+	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Why? Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

Quoted 45 lines

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

+};
 
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	__be16 temp_raw;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+			       sizeof(temp_raw));
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpu(temp_raw);
+
+	return 0;
+}
 
+static int bm1390_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;
+

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

Quoted 1 line

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

+			return IIO_VAL_INT_PLUS_NANO;

IIO_VAL_FRACTIONAL? Mind you I'm not sure that will result in enough precision
either here. For in kernel use it will have full precision though as will be
kept as a fraction. I guess question of whether it is worse than what you have here. There hasn't been much demand for IIO_VAL_INTO_PLUS_PICO, but if we have to look at that we can - with proviso that existing userspace software won't know anything about it.

Quoted 27 lines

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

+		}
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		ret = bm1390_read_data(data, chan, val, val2);
+		iio_device_release_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	struct bm1390_data_buf buffer;
+	int smp_lvl, ret, i, warn;
+	u64 sample_period;

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

Quoted 5 lines

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

+	__be16 temp = 0;
+
+	/*
+	 * If the IC is accessed during FIFO read samples can be dropped.
+	 * Prevent access until FIFO_LVL is read

Comment doesn't seem to have much to do with code that follows it? Maybe needs more detail given you clearly access the IC before reading FIFO_LVL.

Quoted 66 lines

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

+	 */
+	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
+				       sizeof(temp));
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (ret)
+		return ret;
+
+	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+	if (!smp_lvl)
+		return 0;
+
+	if (smp_lvl > 4) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = 4;
+	}
+
+	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 */
+	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (warn)
+		dev_warn(data->dev, "Closing FIFO sequence failed\n");
+
+	if (!ret)
+		return ret;
+
+	return smp_lvl;
+}
+
+static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{

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

Quoted 6 lines

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

+	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

Odd line wrapping. Tidy this up to go near but not over 80 chars

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

Quoted 12 lines

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

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

That irq disable is potentially expensive.
Why not just pass the current timestamp into the __bm1390_fifo_flush > The locks should prevent other races I think..

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

Quoted 5 lines

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

+	 */
+	disable_irq(data->irq);
+	data->timestamp = iio_get_time_ns(idev);
+
+	mutex_lock(&data->mutex);

scoped_guard() will work nicely here

Quoted 29 lines

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

+	ret = __bm1390_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	enable_irq(data->irq);
+
+	return ret;
+}
 
+
+static irqreturn_t bm1390_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret, status;
+
+	/* DRDY is acked by reading status reg */
+	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
+	if (ret || !status)
+		return IRQ_NONE;
+
+	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
+
+	ret = bm1390_pressure_read(data, &data->buf.pressure);
+	if (ret) {
+		dev_warn(data->dev, "sample read failed %d\n", ret);
+		return IRQ_NONE;
+	}

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

Quoted 5 lines

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

+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
+			       &data->buf.temp, BM1390_TEMP_SIZE);
+	if (ret)
+		dev_warn(data->dev, "temp read failed %d\n", ret);

I don't want to see garbage (or a zero :) pushed into the buffer. So if you get a read fail, don't push to buffers.

Quoted 6 lines

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

+
+	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;

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

Quoted 10 lines

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

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

Nice case for the new guard() calls which do automatic release of locks on exit from a function - we also have scoped_guard for stuff inside functions.

Here it will let you use direct returns throughout as lock will always be released for you.

Quoted 1 line

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

Quoted 52 lines

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

+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state == BM1390_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	data->trigger_enabled = state;
+
+	if (state) {
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+		if (ret)
+			goto unlock_out;
+	} else {
+		int dummy;
+
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+		if (ret)
+			goto unlock_out;
+
+		/*
+		 * We need to read the status register in order to ACK the
+		 * data-ready which may have been generated just before we
+		 * disabled the measurement.
+		 */
+		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
+		if (ret)
+			dev_warn(data->dev, "status read failed\n");
+	}
+
+	ret = bm1390_set_drdy_irq(data, state);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops bm1390_trigger_ops = {
+	.set_trigger_state = bm1390_trigger_set_state,
+};
+
+static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
+				int irq)
+{
+	struct iio_trigger *itrig;
+	char *name;
+	int ret;
+

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

Quoted 3 lines

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

+	/* Nothing to do if we don't have IRQ for data-ready and WMI */
+	if (irq < 0)
+		return 0;

Matter of taste, so I won't argue strongly but I'd prefer to see this check at the call site, not down in the function. Makes the flow more obvious and we don't have usual reasons for doing it this way (either too much indenting at the call site, or lots of similar calls).

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

Quoted 6 lines

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

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

Why doesn't this still apply even if we don't have an irq for this device? Can use a sysfs or hrtimer trigger or an irq on another device.

Quoted 7 lines

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

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

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

Quoted 5 lines

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

+	if (!itrig)
+		return -ENOMEM;
+
+	data->trig = itrig;
+	idev->available_scan_masks = bm1390_scan_masks;

Mixing trigger and buffer stuff in here. I'd rather see them separate - so move this up to where you set the buffer up.

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

Quoted 7 lines

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

+
+	itrig->ops = &bm1390_trigger_ops;
+	iio_trigger_set_drvdata(itrig, data);
+
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+			      dev_name(data->dev));

Check the allocation. Definitely don't want name not defined here.

Quoted 22 lines

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

+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;
+}
+
+static int bm1390_probe(struct i2c_client *i2c)
+{
+	struct bm1390_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;

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

Quoted 5 lines

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

+	struct device *dev;
+	unsigned int part_id;
+	int ret;
+
+	dev = &i2c->dev;

Given it's unconditionally set and no line length issue, I'd prefer this done on the local variable definitions block above.

	struct device *dev = &i2c->dev;

Quoted 12 lines

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

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

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

Quoted 3 lines

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

+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	data = iio_priv(idev);

Why this ordering? I'd expect this either:
1) Immediately after it becomes available so above the regulator enable, or
2) immediately before it is used, so a few lines down above data->regmap... Here it just looks a bit lost :)

Quoted 55 lines

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

+
+	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;
+	/*
+	 * 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 0;
+}
+
+static const struct of_device_id bm1390_of_match[] = {
+	{ .compatible = "rohm,bm1390glv-z" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bm1390_of_match);
+
+static const struct i2c_device_id bm1390_id[] = {
+	{ "bm1390glv-z", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, bm1390_id);
+
+static struct i2c_driver bm1390_driver = {
+	.driver = {
+		.name = "bm1390",

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

Quoted 10 lines

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

+		.of_match_table = bm1390_of_match,
+		/*
+		 * The probe issues a few msleep()s - which can result
+		 * measurable delays. Additionally, enabling the VDD may cause
+		 * some (slight) delay depending on the ramp-rate of the
+		 * regulator. Delays are propably magnitude of tens of
+		 * milliseconds - but async probing should not be a problem so
+		 * we should have nothing to lose here. Let's revise this if
+		 * async probing proves to cause problems here.
+		 */

Perhaps we can summarise this with just the sleep info? - don't need to predict the future :)

		/*
		 * Probing explicitly requires a few millisecond of sleep.
	 	 * Enabling the VDD regulator may include ramp up rates.
		 */

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

Quoted 1 line

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

+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

Quoted 35 lines · Jonathan Cameron

Quoted 32 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

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

Quoted 10 lines

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

Quoted 3 lines

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

+
+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Why?  Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

I have a feeling I have misunderstood how this mask works. I have assumed all the channels with corresponding mask bit _can_ be enabled simultaneously, but I have not understood they _must_ all be enabled. I think I must go back studying this, but if all channels really _must_ be enabled, then you are correct. It actually makes a lot of sense to support the pressure values alone, as, according to the data-sheet, the HW is doing a "MEMS temperature compensation" to the pressure values. So, my assuimption is the temperature data may not be required to be captured.

This also means I should revise the scan masks for the BU27008, BU27010 and BU27034 light sensors as I don't think all the users want all the channels enabled. I wonder how I have not noticed any problems when I tested those things - did I really always enable all the channels...? @_@

Anyways, Thanks.

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Quoted 14 lines

Quoted 13 lines

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

Quoted 3 lines

That irq disable is potentially expensive.
Why not just pass the current timestamp into the __bm1390_fifo_flush > The locks should prevent other races I think..

Gah. I hate you Jonathan ;) (Not really!)

Actually, thank you (as always) for pointing this out. I don't instantly see why it wouldn't work, but going throught the IRQ races is never trivial (for me). It's work I've learned not to do at afternoon as my brains work better at the morning :) So, I will still go through this as a first thing tomorrow when I start my work day...

Quoted 40 lines · Matti Vaittinen

Quoted 39 lines · Jonathan Cameron

Quoted 36 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

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

...

Quoted 21 lines

Quoted 10 lines

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

Quoted 3 lines

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

+
+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

Why?  Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

I have a feeling I have misunderstood how this mask works. I have assumed all the channels with corresponding mask bit _can_ be enabled simultaneously, but I have not understood they _must_ all be enabled. I think I must go back studying this, but if all channels really _must_ be enabled, then you are correct. It actually makes a lot of sense to support the pressure values alone, as, according to the data-sheet, the HW is doing a "MEMS temperature compensation" to the pressure values. So, my assuimption is the temperature data may not be required to be captured.

Quoted 6 lines

This also means I should revise the scan masks for the BU27008, BU27010 and BU27034 light sensors as I don't think all the users want all the channels enabled. I wonder how I have not noticed any problems when I tested those things - did I really always enable all the channels...? @_@

Anyways, Thanks.

Hi Jonathan,

There's something in IIO scan_masks / buffer demuxing that I don't quite understand. I noticed following things:

1) Strict available scan mask check seems to be in use only for INDIO_BUFFER_HARDWARE stuff.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/iio/industrialio-buffer.c#L881

So, the:

Quoted 2 lines

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

is not exclusive for BM1390 as long as it is not setting the INDIO_BUFFER_HARDWARE.

My test seems to agree with the code:

// Only enable the temperature:

root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
scan_elements/in_temp_en

// Run the geneeric buffer without -a:

root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
0 -t bm1390data-rdy-dev0
iio device number being used is 0
iio trigger number being used is 0
/sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
22968.750000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23031.250000
23000.000000
23000.000000
root@arm:/sys/bus/iio/devices/iio:device0#

In above case the temperature values and only the temperature values were shown. I must admit I did not spend enough time with the iio_generic_buffer.c or IIO demuxing code to really understand all the details (I got headache very quickly ;) ). I still believe the iio_generic_buffer expects to see only the enabled channel data in the buffer - so, it seems to me the kernel is also only adding the enabled channel data to the buffer. Also, judging the values, the demuxing is correctly extracting the temperature data from data the driver pushes here:

iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);

The bm1390 driver as sent in v2 does not do demuxing but always pushes whole chunk of data and trusts IIO to do demuxing.

2) I noticed the 'available_scan_masks' was marked as an optional field. So, I think that if there is no restrictions to which of the channels can be enabled, then we can omit setting it. This is what I tried.

It appears that when we do not populate the 'available_scan_masks' with the:

Quoted 2 lines

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

things change. When I tested enabling only temperature and ran the iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported values seemed completely random.

When I initialized the pressure data in driver:
data->buf.pressure = 1;
before doing the:
iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);

I saw following:

root@arm:/# cd /sys/bus/iio/devices/iio\:device0
root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
scan_elements/in_temp_en
root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
0 -t bm1390data-rdy-dev0
iio device number being used is 0
iio trigger number being used is 0
/sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
root@arm:/sys/bus/iio/devices/iio:device0# cat in_temp_scale
31.250000

If we calculate 8000/31.250000 we will get value 256. This looks like value '1' in BE16 format.

Based on this experimenting (and headache obtained from reading the demuxing code) - the IIO framework does not do channel demuxing if the 'available_scan_masks' is not given? To me this was somewhat unexpected.

Finally, when the watermark IRQ is used, we can't omit reading the pressure data because clearing the WMI is done based on the pressure data in FIFO.

So, I would propose we do:

static const unsigned long bm1390_scan_masks[] = {
	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
	BIT(BM1390_CHAN_PRESSURE), 0

which better reflects what the hardware is capable of - and, unless I am missing something, also allows us to offload the buffer demuxing to the IIO.

Still, as mentioned in 1), the

Quoted 2 lines

Quoted 2 lines

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

Quoted 2 lines

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

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

does not seem to prevent enabling only the temperature channel - so in the driver buffer handler we still must unconditionally read the pressure data regardles the active_scan_mask.

Quoted 21 lines

Quoted 15 lines

Quoted 14 lines

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

Quoted 3 lines

That irq disable is potentially expensive.
Why not just pass the current timestamp into the __bm1390_fifo_flush > The locks should prevent other races I think..

Gah. I hate you Jonathan ;) (Not really!)

Quoted 5 lines

Actually, thank you (as always) for pointing this out. I don't instantly see why it wouldn't work, but going throught the IRQ races is never trivial (for me). It's work I've learned not to do at afternoon as my brains work better at the morning :) So, I will still go through this as a first thing tomorrow when I start my work day...

After staring this for a while, I see no reason why we couldn't do as you suggested. Thanks! It really improves this :)

Yours,
-- Matti

Quoted 70 lines · On Tue, 19 Sep 2023 14:28:29 +0300 Matti Vaittinen <[email protected]>…

Quoted 40 lines · Matti Vaittinen

Quoted 39 lines · Jonathan Cameron

Quoted 36 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

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

...

Quoted 26 lines

Quoted 10 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Why?  Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

I have a feeling I have misunderstood how this mask works. I have assumed all the channels with corresponding mask bit _can_ be enabled simultaneously, but I have not understood they _must_ all be enabled. I think I must go back studying this, but if all channels really _must_ be enabled, then you are correct. It actually makes a lot of sense to support the pressure values alone, as, according to the data-sheet, the HW is doing a "MEMS temperature compensation" to the pressure values. So, my assuimption is the temperature data may not be required to be captured.

This also means I should revise the scan masks for the BU27008, BU27010 and BU27034 light sensors as I don't think all the users want all the channels enabled. I wonder how I have not noticed any problems when I tested those things - did I really always enable all the channels...? @_@

Quoted 6 lines

Quoted 1 line

Anyways, Thanks.

Hi Jonathan,

There's something in IIO scan_masks / buffer demuxing that I don't quite understand

Quoted 3 lines

The bm1390 driver as sent in v2 does not do demuxing but always pushes whole chunk of data and trusts IIO to do demuxing.

Yup. That should work. But in this case you can trivially decide not to read or fill in the temperature and hence save some bus cycles.

Quoted 5 lines

2) I noticed the 'available_scan_masks' was marked as an optional field. So, I think that if there is no restrictions to which of the channels can be enabled, then we can omit setting it. This is what I tried.

Quoted 7 lines

It appears that when we do not populate the 'available_scan_masks' with the:

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

things change. When I tested enabling only temperature and ran the iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported values seemed completely random.

You need to pack the data correctly yourself in the driver. So it adds small amount of code complexity but potentially saves bus traffic.

Quoted 4 lines

Based on this experimenting (and headache obtained from reading the demuxing code) - the IIO framework does not do channel demuxing if the 'available_scan_masks' is not given? To me this was somewhat unexpected.

Yes. If you don't tell it what channel setups are available (note there can be lots) you are saying that we support any random combination and have to do the data packing by hand.

Quoted 4 lines

Finally, when the watermark IRQ is used, we can't omit reading the pressure data because clearing the WMI is done based on the pressure data in FIFO.

Hmm. That is a good reason to keep the available scan masks set. Add a comment on that next to the mask.

Quoted 6 lines

So, I would propose we do:

static const unsigned long bm1390_scan_masks[] = {
	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
	BIT(BM1390_CHAN_PRESSURE), 0

Makes sense given need to read the pressure channel.

Quoted 9 lines

which better reflects what the hardware is capable of - and, unless I am missing something, also allows us to offload the buffer demuxing to the IIO.

Still, as mentioned in 1), the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

Quoted 3 lines

does not seem to prevent enabling only the temperature channel - so in the driver buffer handler we still must unconditionally read the pressure data regardles the active_scan_mask.

active_scan_mask should match the above - it's separate from what is enabled. active_scan_mask is on the data capture side of the demux - the buffers are then fed repacked data reflecting what is enabled.

Another thing to note is that, when we build the available_scan_mask array - we should either pay attention to the order of masks - or change the iio_scan_mask_match() to not accept first matching subset but to go through all of the masks unless it finds and exactly matching one (and in general prefer the smallest subset). Not sure this is worth the extra cycles though.

Quoted 6 lines · Matti Vaittinen

Another thing to note is that, when we build the available_scan_mask array - we should either pay attention to the order of masks - or change the iio_scan_mask_match() to not accept first matching subset but to go through all of the masks unless it finds and exactly matching one (and in general prefer the smallest subset). Not sure this is worth the extra cycles though.

Replying to myself and to those who I perhaps managed to confuse :)

As a result of above pondering I wrote this:

(quoted diff) +2 -0

@@ -411,6 +418,8 @@ static const unsigned long 
 *iio_scan_mask_match(const unsigned long *av_masks,
                                                 const unsigned long *mask,
                                                 bool strict)
  {
+       const unsigned long *smallest = NULL;
+
         if (bitmap_empty(mask, masklength))
                 return NULL;

         while (*av_masks) {

(quoted diff) +7 -3

@@ -418,12 +427,16 @@ static const unsigned long 
 *iio_scan_mask_match(const unsigned long *av_masks,
                         if (bitmap_equal(mask, av_masks, masklength))
                                 return av_masks;
                 } else {
-                       if (bitmap_subset(mask, av_masks, masklength))
-                               return av_masks;
+                       if (bitmap_subset(mask, av_masks, masklength)) {
+                               if (!smallest ||
+                                   bitmap_weight(av_masks, BITS_PER_LONG) <
+                                   bitmap_weight(smallest, BITS_PER_LONG))
+                                       smallest = av_masks;
+                       }
                 }
                 av_masks += BITS_TO_LONGS(masklength);
         }
-       return NULL;
+       return smallest;
  }
 

but ...
... I see a problem that some of the channels may be more costly to

access than the other. It could be that reading some of the channels is just a matter of getting a cached value, while other could require a long measurement time and access to significant amount of registers. So, the knowledge of preferred scan masks should indeed be on the driver side. Hence, the ordering of the masks in the order of preference makes perfect sense. What we could do in the IIO core side is still go through all of the available masks to see if we find an exact match. I guess we could also document the fact that the order of masks matters.

Thanks for listening - and sorry for the noise :)

Yours,
-- Matti

Quoted 73 lines · Jonathan Cameron

Quoted 70 lines · On Tue, 19 Sep 2023 14:28:29 +0300 Matti Vaittinen <[email protected]>…

Quoted 40 lines · Matti Vaittinen

Quoted 39 lines · Jonathan Cameron

Quoted 36 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

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

...

Quoted 26 lines

Quoted 10 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

Why?  Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

I have a feeling I have misunderstood how this mask works. I have assumed all the channels with corresponding mask bit _can_ be enabled simultaneously, but I have not understood they _must_ all be enabled. I think I must go back studying this, but if all channels really _must_ be enabled, then you are correct. It actually makes a lot of sense to support the pressure values alone, as, according to the data-sheet, the HW is doing a "MEMS temperature compensation" to the pressure values. So, my assuimption is the temperature data may not be required to be captured.

This also means I should revise the scan masks for the BU27008, BU27010 and BU27034 light sensors as I don't think all the users want all the channels enabled. I wonder how I have not noticed any problems when I tested those things - did I really always enable all the channels...? @_@

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Anyways, Thanks.

Hi Jonathan,

There's something in IIO scan_masks / buffer demuxing that I don't quite understand

Thank You for the patience and the explanation Jonathan. I have a - hopefully not totally wrong - feeling I understand this better. I didn't understand that IIO framework has this extra logic and that available_scan_masks was not really meant for telling what IIO allows _users_ to enable - but it was for driver to tell IIO core what the driver can give. (Naturally this impacts to what IIO allows users to enable, but not directly). Eg, if I now get it right, the 'available_scan_masks' is information what data flows between driver and IIO. Similarly, 'active_scan_mask' tells what the IIO core 'expects' to get from the driver, right? So, in bm1390 case, if I set both:

BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), and
BIT(BM1390_CHAN_PRESSURE),

to 'available_scan_masks', then it means driver is telling IIO it can give both pressure and temperature, or temperature alone. As a result, IIO can set either both temp and pressure to 'active_scan_mask' - in which case driver should push both in the buffers - or just the pressure, in which case the driver should not push the temperature.

Quoted 24 lines

Quoted 4 lines

The bm1390 driver as sent in v2 does not do demuxing but always pushes whole chunk of data and trusts IIO to do demuxing.

Yup. That should work. But in this case you can trivially decide not to read or fill in the temperature and hence save some bus cycles.

Quoted 5 lines

2) I noticed the 'available_scan_masks' was marked as an optional field. So, I think that if there is no restrictions to which of the channels can be enabled, then we can omit setting it. This is what I tried.

Quoted 7 lines

It appears that when we do not populate the 'available_scan_masks' with the:

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

things change. When I tested enabling only temperature and ran the iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported values seemed completely random.

You need to pack the data correctly yourself in the driver. So it adds small amount of code complexity but potentially saves bus traffic.

...

Quoted 9 lines

Quoted 4 lines

Based on this experimenting (and headache obtained from reading the demuxing code) - the IIO framework does not do channel demuxing if the 'available_scan_masks' is not given? To me this was somewhat unexpected.

Yes. If you don't tell it what channel setups are available (note there can be lots) you are saying that we support any random combination and have to do the data packing by hand.

Okay... I think it kind of makes sense to leave an option where the driver can create buffer as it wants. I am not sure I can wrap my head around this to the extent that I knew when the IIO does not know what channels the driver has enabled - and thus, what types of data there will be - but I can accept the answer that such situations can exist :) Besides, allowing the driver to do the buffer formatting may allow some 'use-case specific, custom optimizations' when user-space knows what to expect (for example, regarding the data alignments and ordering to for example save space).

...This leads to another thing I noticed - and to another question :)

I was experimenting with the bm1390 scan masks and iio_generic_buffer tool. It seems to me that the iio_generic_buffer does not expect the padding when the data in buffer is u32 + u16. When I enable the pressure (32bits) and temperature (16 bits) in bm1390 and keep the time-stamp disabled, the user-space will get buffer where 2 bytes of padding is added to each temperature sample so the next pressure sample stays correctly aligned. I believe kernel side does sane job here.

This, however, did result (at least my version of) the iio_generic_buffer tool to read garbage values after the first sample. My assumption is that the iio_generic_buffer does not add the padding bytes to the end of last data in the scan in order to ensure also the next scan will be correctly aligned.

I did following change to the iio_generic_buffer:

Author: Matti Vaittinen <[email protected]>
Date:   Thu Sep 21 10:15:19 2023 +0300

tools: iio: iio_generic_buffer ensure alignment

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

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

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

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

tools/iio/iio_generic_buffer.c +4 -1

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

else

tools/iio/iio_generic_buffer.c +0 -0

@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct 

iio_channel_info *channels, in

                 bytes = channels[i].location + channels[i].bytes;
                 i++;
         }

tools/iio/iio_generic_buffer.c +4 -0

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

misalignment, max - misalignment);

tools/iio/iio_generic_buffer.c +2 -0

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

I can send this as a proper patch if you guys think it is correct.

Quoted 27 lines

Quoted 6 lines

Finally, when the watermark IRQ is used, we can't omit reading the pressure data because clearing the WMI is done based on the pressure data in FIFO.

Hmm. That is a good reason to keep the available scan masks set. Add a comment on that next to the mask.

Quoted 6 lines

So, I would propose we do:

static const unsigned long bm1390_scan_masks[] = {
	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
	BIT(BM1390_CHAN_PRESSURE), 0  

Makes sense given need to read the pressure channel.

Quoted 9 lines

which better reflects what the hardware is capable of - and, unless I am missing something, also allows us to offload the buffer demuxing to the IIO.

Still, as mentioned in 1), the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+static const unsigned long bm1390_scan_masks[] = {
+    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

Quoted 7 lines

Quoted 3 lines

does not seem to prevent enabling only the temperature channel - so in the driver buffer handler we still must unconditionally read the pressure data regardles the active_scan_mask.

active_scan_mask should match the above - it's separate from what is enabled. active_scan_mask is on the data capture side of the demux - the buffers are then fed repacked data reflecting what is enabled.

Cool! So, the driver can rely on IIO asking for the pressure (in active_scan_mask) after it has correctly set the available scan mask. It may not be a big thing but I like it. It is enough to take care of ensuring all required stuff is read from HW in one place (in available scan masks) and not to worry about it in actual data reading path but just read stuff IIO is asking to be read. I like it.

Another thing to note is that, when we build the available_scan_mask array - we should either pay attention to the order of masks - or change the iio_scan_mask_match() to not accept first matching subset but to go through all of the masks unless it finds and exactly matching one (and in general prefer the smallest subset). Not sure this is worth the extra cycles though.

Thanks again - feel like I've learned something today :)

Yours,
-- Matti Vaittinen

Quoted 34 lines · Jonathan Cameron

Quoted 31 lines · On Fri, 15 Sep 2023 09:56:19 +0300 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

Quoted 5 lines

Quoted 1 line

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

Hi Matti,

I think this is coming together nicely. A few comments inline.

Thanks for the review (once again) :)

Quoted 28 lines

Quoted 25 lines

---
Revision history:
v1 => v2:
- prefer s64 over int64_t
- drop not needed handling of 2's complements
- plenty of styling changes
- drop dead code (write_raw)
- fix typos in comments
- explain trigger and FIFO usage in commit message
- do better job at cheking the return values
- ensure there's no race when checking if triggered buffer is used before enabling the FIFO
- print warning if register read fails at IRQ handler
- drop unnecessary warning if IRQ is not given
- explain why we prefer asynchronous probing
---

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

new file mode 100644
index 000000000000..d3cc229d1688
@@ -0,0 +1,899 @@  

...

Quoted 10 lines

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

Quoted 3 lines

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

+
+static const unsigned long bm1390_scan_masks[] = {
+	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  

Why? Doesn't look hard to support just one or the other? Normally we only do this sort of limitation when there is a heavily optimized read routine for a set of channels and it is better to grab them all and throw away the ones we don't care about. That doesn't seem to be true here. So if the fifo grabbed both temp and pressure it would makes sense here, but doesn't seem like it does.

I have a feeling I have misunderstood how this mask works. I have assumed all the channels with corresponding mask bit _can_ be enabled simultaneously, but I have not understood they _must_ all be enabled. I think I must go back studying this, but if all channels really _must_ be enabled, then you are correct. It actually makes a lot of sense to support the pressure values alone, as, according to the data-sheet, the HW is doing a "MEMS temperature compensation" to the pressure values. So, my assuimption is the temperature data may not be required to be captured.

This also means I should revise the scan masks for the BU27008, BU27010 and BU27034 light sensors as I don't think all the users want all the channels enabled. I wonder how I have not noticed any problems when I tested those things - did I really always enable all the channels...? @_@

Anyways, Thanks.

Quoted 28 lines

Quoted 28 lines

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

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

Quoted 8 lines

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

Quoted 1 line

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

+			return IIO_VAL_INT_PLUS_NANO;  

IIO_VAL_FRACTIONAL? Mind you I'm not sure that will result in enough precision
either here. For in kernel use it will have full precision though as will be
kept as a fraction. I guess question of whether it is worse than what you have here. There hasn't been much demand for IIO_VAL_INTO_PLUS_PICO, but if we have to look at that we can - with proviso that existing userspace software won't know anything about it.

I must take a look at the IIO_VAL_FRACTIONAL. I think either it or the NANO at this point. I have no understanding what is the precision that is enough. Hmm.. Would adding the PICO really require a change in user-space or could we hide it in-kernel when formatting the value we emit via sysfs?

Quoted 17 lines

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

Quoted 13 lines

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

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

That irq disable is potentially expensive.
Why not just pass the current timestamp into the __bm1390_fifo_flush > The locks should prevent other races I think..

Gah. I hate you Jonathan ;) (Not really!)

Actually, thank you (as always) for pointing this out. I don't instantly see why it wouldn't work, but going throught the IRQ races is never trivial (for me). It's work I've learned not to do at afternoon as my brains work better at the morning :) So, I will still go through this as a first thing tomorrow when I start my work day...

Quoted 10 lines

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

Quoted 6 lines

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

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

Why doesn't this still apply even if we don't have an irq for this device? Can use a sysfs or hrtimer trigger or an irq on another device.

Another good question. I know I wrote the code only thinking that when the trigger is used, it will be the data-ready from the device. I think I even used the trigger validation function which requires us to use the device's own trigger.

Now, whether it would make sense to allow other triggers, and whether the trigger handler has any data-ready specific handling here is something I need to revise. What does slightly bug me with allowing other triggers is that as far as I remmeber the BM1390 had no way to indicate that a single sample was actually ready. So, when we use any other trigger it may be we don't have a valid sample to read. (The read_raw uses a delay which should be long enough to guarantee a sample).

Quoted 16 lines

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

Quoted 13 lines

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

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

Mixing trigger and buffer stuff in here. I'd rather see them separate - so move this up to where you set the buffer up.

Thanks for pointing this out. In my mind the trigger and buffer went hand to hand with this device. Now, after your previous comment, I need to rethink if they really can be sepated. Anyways, moving this makes sense.

Quoted 9 lines

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

Quoted 7 lines

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

+
+	itrig->ops = &bm1390_trigger_ops;
+	iio_trigger_set_drvdata(itrig, data);
+
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+			      dev_name(data->dev));  

Check the allocation. Definitely don't want name not defined here.

The request_threaded_irq will use the dev_name() if NULL is passed to it. I think it should be fine - but I think this is not worth fighting if you strongly prefer the check :)

Quoted 22 lines

Quoted 22 lines

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

+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;
+}
+
+static int bm1390_probe(struct i2c_client *i2c)
+{
+	struct bm1390_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;

Quoted 8 lines

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

Quoted 5 lines

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

+	struct device *dev;
+	unsigned int part_id;
+	int ret;
+
+	dev = &i2c->dev;  

Given it's unconditionally set and no line length issue, I'd prefer this done on the local variable definitions block above.

I can change this if you feel strong about it. Still, I personally find assignments in declarations less clear and so I tend to avoid them. (There's a few exceptions like repeated private data pointer getting).