[PATCH v3 0/4] iio: mpl3115: add support for DRDY interrupt

[PATCH v3 0/4] iio: mpl3115: add support for DRDY interrupt

[Antoni Pokusinski]

Hello,
This set of patches adds support for the DRDY interrupt in the MPL3115
pressure sensor. The device has 2 interrupt lines, hence the new
binding. I also added support for the sampling frequency which
determines the time interval between subsequent measurements (in the
continuous measurements mode) so it's obiously tied to the DRDY
interrupt feature.

Kind regards,
Antoni Pokusinski

---
Changes since v2:
* P4: included linux/bitfield.h

Changes since v1:
* P1: add `vdd-supply` and `vddio-supply`

* P2: new patch: use guards from cleanup.h   

* P3: change macros of control register bits to convention
      MPL3115_CTRLX_NAME
* P3: MPL3115_PT_DATA_EVENT_ALL: use GENMASK
* P3: trigger_probe: do not fail if dev_fwnode() returns NULL
* P3: trigger_probe: use devm_iio_trigger_register()
* P3: trigger_probe: introduced enum mpl3115_irq_type and 
      changed IRQ setup logic accordingly

* P4: MPL3115_CTRL2_ST: use GENMASK
* P4: read_raw: samp_freq: use FIELD_GET
* P4: write_raw: samp_freq: use FIELD_PREP
---

Antoni Pokusinski (4):
  dt-bindings: iio: pressure: add binding for mpl3115
  iio: mpl3115: use guards from cleanup.h
  iio: mpl3115: add support for DRDY interrupt
  iio: mpl3115: add support for sampling frequency

 .../bindings/iio/pressure/fsl,mpl3115.yaml    |  71 ++++
 .../devicetree/bindings/trivial-devices.yaml  |   2 -
 drivers/iio/pressure/mpl3115.c                | 314 ++++++++++++++++--
 3 files changed, 352 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml

-- 
2.25.1

[PATCH v3 1/4] dt-bindings: iio: pressure: add binding for mpl3115

[Antoni Pokusinski]

MPL3115 is an I2C pressure and temperature sensor. It features 2
interrupt lines which can be configured to indicate events such as data
ready or pressure/temperature threshold reached.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 .../bindings/iio/pressure/fsl,mpl3115.yaml    | 71 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml b/Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml
new file mode 100644
index 000000000000..2933c2e10695
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/fsl,mpl3115.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MPL3115 precision pressure sensor with altimetry
+
+maintainers:
+  - Antoni Pokusinski <apokusinski01@gmail.com>
+
+description: |
+  MPL3115 is a pressure/altitude and temperature sensor with I2C interface.
+  It features two programmable interrupt lines which indicate events such as
+  data ready or pressure/temperature threshold reached.
+  https://www.nxp.com/docs/en/data-sheet/MPL3115A2.pdf
+
+properties:
+  compatible:
+    const: fsl,mpl3115
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  vddio-supply: true
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum:
+        - INT1
+        - INT2
+
+  drive-open-drain:
+    type: boolean
+    description:
+      set if the specified interrupt pins should be configured as
+      open drain. If not set, defaults to push-pull.
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pressure@60 {
+            compatible = "fsl,mpl3115";
+            reg = <0x60>;
+            vdd-supply = <&vdd>;
+            vddio-supply = <&vddio>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "INT2";
+        };
+    };
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index f3dd18681aa6..918d4a12d61c 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -113,8 +113,6 @@ properties:
           - fsl,mma7660
             # MMA8450Q: Xtrinsic Low-power, 3-axis Xtrinsic Accelerometer
           - fsl,mma8450
-            # MPL3115: Absolute Digital Pressure Sensor
-          - fsl,mpl3115
             # MPR121: Proximity Capacitive Touch Sensor Controller
           - fsl,mpr121
             # Honeywell Humidicon HIH-6130 humidity/temperature sensor
-- 
2.25.1

[PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

[Antoni Pokusinski]

Include linux/cleanup.h and use the scoped_guard() to simplify the code.

Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 579da60ef441..80af672f65c6 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -10,14 +10,16 @@
  * interrupts, user offset correction, raw mode
  */
 
-#include <linux/module.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/module.h>
+
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
-#include <linux/delay.h>
 
 #define MPL3115_STATUS 0x00
 #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
@@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	u8 buffer[16] __aligned(8) = { };
 	int ret, pos = 0;
 
-	mutex_lock(&data->lock);
-	ret = mpl3115_request(data);
-	if (ret < 0) {
-		mutex_unlock(&data->lock);
-		goto done;
-	}
-
-	if (test_bit(0, indio_dev->active_scan_mask)) {
-		ret = i2c_smbus_read_i2c_block_data(data->client,
-			MPL3115_OUT_PRESS, 3, &buffer[pos]);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
+	scoped_guard(mutex, &data->lock) {
+		ret = mpl3115_request(data);
+		if (ret < 0)
 			goto done;
+
+		if (test_bit(0, indio_dev->active_scan_mask)) {
+			ret = i2c_smbus_read_i2c_block_data(data->client,
+				MPL3115_OUT_PRESS, 3, &buffer[pos]);
+			if (ret < 0)
+				goto done;
+			pos += 4;
 		}
-		pos += 4;
-	}
 
-	if (test_bit(1, indio_dev->active_scan_mask)) {
-		ret = i2c_smbus_read_i2c_block_data(data->client,
-			MPL3115_OUT_TEMP, 2, &buffer[pos]);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
-			goto done;
+		if (test_bit(1, indio_dev->active_scan_mask)) {
+			ret = i2c_smbus_read_i2c_block_data(data->client,
+				MPL3115_OUT_TEMP, 2, &buffer[pos]);
+			if (ret < 0)
+				goto done;
 		}
 	}
-	mutex_unlock(&data->lock);
 
 	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
 				    iio_get_time_ns(indio_dev));
-- 
2.25.1

[PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

[Antoni Pokusinski]

MPL3115 sensor features a "data ready" interrupt which indicates the
presence of new measurements.

Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 197 ++++++++++++++++++++++++++++++---
 1 file changed, 184 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 80af672f65c6..13c8b338a15e 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -7,49 +7,77 @@
  * (7-bit I2C slave address 0x60)
  *
  * TODO: FIFO buffer, altimeter mode, oversampling, continuous mode,
- * interrupts, user offset correction, raw mode
+ * user offset correction, raw mode
  */
 
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/property.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
 
 #define MPL3115_STATUS 0x00
 #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
 #define MPL3115_OUT_TEMP 0x04 /* MSB first, 12 bit */
 #define MPL3115_WHO_AM_I 0x0c
+#define MPL3115_INT_SOURCE 0x12
+#define MPL3115_PT_DATA_CFG 0x13
 #define MPL3115_CTRL_REG1 0x26
+#define MPL3115_CTRL_REG3 0x28
+#define MPL3115_CTRL_REG4 0x29
+#define MPL3115_CTRL_REG5 0x2a
 
 #define MPL3115_DEVICE_ID 0xc4
 
 #define MPL3115_STATUS_PRESS_RDY BIT(2)
 #define MPL3115_STATUS_TEMP_RDY BIT(1)
 
-#define MPL3115_CTRL_RESET BIT(2) /* software reset */
-#define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
-#define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
-#define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
+#define MPL3115_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
+
+#define MPL3115_CTRL1_RESET BIT(2) /* software reset */
+#define MPL3115_CTRL1_OST BIT(1) /* initiate measurement */
+#define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
+#define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
+
+#define MPL3115_CTRL3_IPOL1 BIT(5)
+#define MPL3115_CTRL3_IPOL2 BIT(1)
+
+#define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
+
+#define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
+
+#define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
 
 struct mpl3115_data {
 	struct i2c_client *client;
+	struct iio_trigger *drdy_trig;
 	struct mutex lock;
 	u8 ctrl_reg1;
 };
 
+enum mpl3115_irq_type {
+	INT2_ACTIVE_LOW  = MPL3115_INT2 | IRQF_TRIGGER_FALLING,
+	INT2_ACTIVE_HIGH = MPL3115_INT2 | IRQF_TRIGGER_RISING,
+	INT1_ACTIVE_LOW  = (!MPL3115_INT2) | IRQF_TRIGGER_FALLING,
+	INT1_ACTIVE_HIGH = (!MPL3115_INT2) | IRQF_TRIGGER_RISING,
+};
+
 static int mpl3115_request(struct mpl3115_data *data)
 {
 	int ret, tries = 15;
 
 	/* trigger measurement */
 	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
-		data->ctrl_reg1 | MPL3115_CTRL_OST);
+		data->ctrl_reg1 | MPL3115_CTRL1_OST);
 	if (ret < 0)
 		return ret;
 
@@ -58,7 +86,7 @@ static int mpl3115_request(struct mpl3115_data *data)
 		if (ret < 0)
 			return ret;
 		/* wait for data ready, i.e. OST cleared */
-		if (!(ret & MPL3115_CTRL_OST))
+		if (!(ret & MPL3115_CTRL1_OST))
 			break;
 		msleep(20);
 	}
@@ -166,9 +194,11 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	int ret, pos = 0;
 
 	scoped_guard(mutex, &data->lock) {
-		ret = mpl3115_request(data);
-		if (ret < 0)
-			goto done;
+		if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
+			ret = mpl3115_request(data);
+			if (ret < 0)
+				goto done;
+		}
 
 		if (test_bit(0, indio_dev->active_scan_mask)) {
 			ret = i2c_smbus_read_i2c_block_data(data->client,
@@ -224,10 +254,147 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
+static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(ret & MPL3115_INT_SRC_DRDY))
+		return IRQ_NONE;
+
+	iio_trigger_poll_nested(data->drdy_trig);
+
+	return IRQ_HANDLED;
+}
+
+static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	int ret;
+	u8 ctrl_reg1 = data->ctrl_reg1;
+
+	if (state)
+		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
+	else
+		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
+
+	guard(mutex)(&data->lock);
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+					ctrl_reg1);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
+					state ? MPL3115_CTRL4_INT_EN_DRDY : 0);
+	if (ret < 0)
+		goto reg1_cleanup;
+
+	data->ctrl_reg1 = ctrl_reg1;
+
+	return 0;
+
+reg1_cleanup:
+	i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+				  data->ctrl_reg1);
+	return ret;
+}
+
+static const struct iio_trigger_ops mpl3115_trigger_ops = {
+	.set_trigger_state = mpl3115_set_trigger_state,
+};
+
 static const struct iio_info mpl3115_info = {
 	.read_raw = &mpl3115_read_raw,
 };
 
+static int mpl3115_trigger_probe(struct mpl3115_data *data,
+				 struct iio_dev *indio_dev)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
+	int ret, irq, irq_type, irq_cfg_flags = 0;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq < 0) {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq < 0)
+			return 0;
+
+		irq_cfg_flags |= MPL3115_INT2;
+	}
+
+	irq_type = irq_get_trigger_type(irq);
+	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
+		return -EINVAL;
+
+	irq_cfg_flags |= irq_type;
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+					MPL3115_PT_DATA_EVENT_ALL);
+	if (ret < 0)
+		return ret;
+
+	switch (irq_cfg_flags) {
+	case INT2_ACTIVE_HIGH:
+		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
+						MPL3115_CTRL3_IPOL2);
+		if (ret)
+			return ret;
+
+		break;
+	case INT2_ACTIVE_LOW:
+		break;
+	case INT1_ACTIVE_HIGH:
+		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
+						MPL3115_CTRL5_INT_CFG_DRDY);
+		if (ret)
+			return ret;
+
+		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
+						MPL3115_CTRL3_IPOL1);
+		if (ret)
+			return ret;
+
+		break;
+	case INT1_ACTIVE_LOW:
+		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
+						MPL3115_CTRL5_INT_CFG_DRDY);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
+						 "%s-dev%d",
+						 indio_dev->name,
+						 iio_device_id(indio_dev));
+	if (!data->drdy_trig)
+		return -ENOMEM;
+
+	data->drdy_trig->ops = &mpl3115_trigger_ops;
+	iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+	ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(data->drdy_trig);
+
+	return devm_request_threaded_irq(&data->client->dev, irq,
+					 NULL,
+					 mpl3115_interrupt_handler,
+					 IRQF_ONESHOT,
+					 "mpl3115_irq",
+					 indio_dev);
+}
+
 static int mpl3115_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -258,15 +425,19 @@ static int mpl3115_probe(struct i2c_client *client)
 
 	/* software reset, I2C transfer is aborted (fails) */
 	i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
-		MPL3115_CTRL_RESET);
+		MPL3115_CTRL1_RESET);
 	msleep(50);
 
-	data->ctrl_reg1 = MPL3115_CTRL_OS_258MS;
+	data->ctrl_reg1 = MPL3115_CTRL1_OS_258MS;
 	ret = i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
 		data->ctrl_reg1);
 	if (ret < 0)
 		return ret;
 
+	ret = mpl3115_trigger_probe(data, indio_dev);
+	if (ret)
+		return ret;
+
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		mpl3115_trigger_handler, NULL);
 	if (ret < 0)
@@ -285,7 +456,7 @@ static int mpl3115_probe(struct i2c_client *client)
 static int mpl3115_standby(struct mpl3115_data *data)
 {
 	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
-		data->ctrl_reg1 & ~MPL3115_CTRL_ACTIVE);
+		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
 }
 
 static void mpl3115_remove(struct i2c_client *client)
-- 
2.25.1

[PATCH v3 4/4] iio: mpl3115: add support for sampling frequency

[Antoni Pokusinski]

When the device is in ACTIVE mode the temperature and pressure measurements
are collected with a frequency determined by the ST[3:0] bits of CTRL_REG2
register.

Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 82 ++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 13c8b338a15e..04c126ff4d46 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -10,6 +10,7 @@
  * user offset correction, raw mode
  */
 
+#include <linux/bitfield.h>
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
@@ -30,6 +31,7 @@
 #define MPL3115_INT_SOURCE 0x12
 #define MPL3115_PT_DATA_CFG 0x13
 #define MPL3115_CTRL_REG1 0x26
+#define MPL3115_CTRL_REG2 0x27
 #define MPL3115_CTRL_REG3 0x28
 #define MPL3115_CTRL_REG4 0x29
 #define MPL3115_CTRL_REG5 0x2a
@@ -48,6 +50,8 @@
 #define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
 #define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
 
+#define MPL3115_CTRL2_ST GENMASK(3, 0)
+
 #define MPL3115_CTRL3_IPOL1 BIT(5)
 #define MPL3115_CTRL3_IPOL2 BIT(1)
 
@@ -57,6 +61,25 @@
 
 #define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
 
+static const unsigned int mpl3115_samp_freq_table[][2] = {
+	{ 1,      0},
+	{ 0, 500000},
+	{ 0, 250000},
+	{ 0, 125000},
+	{ 0,  62500},
+	{ 0,  31250},
+	{ 0,  15625},
+	{ 0,   7812},
+	{ 0,   3906},
+	{ 0,   1953},
+	{ 0,    976},
+	{ 0,    488},
+	{ 0,    244},
+	{ 0,    122},
+	{ 0,     61},
+	{ 0,     30},
+};
+
 struct mpl3115_data {
 	struct i2c_client *client;
 	struct iio_trigger *drdy_trig;
@@ -174,10 +197,61 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = i2c_smbus_read_byte_data(data->client, MPL3115_CTRL_REG2);
+		if (ret < 0)
+			return ret;
+
+		ret = FIELD_GET(MPL3115_CTRL2_ST, ret);
+
+		*val = mpl3115_samp_freq_table[ret][0];
+		*val2 = mpl3115_samp_freq_table[ret][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
 }
 
+static int mpl3115_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	*type = IIO_VAL_INT_PLUS_MICRO;
+	*length = ARRAY_SIZE(mpl3115_samp_freq_table) * 2;
+	*vals = (int *)mpl3115_samp_freq_table;
+	return IIO_AVAIL_LIST;
+}
+
+static int mpl3115_write_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     int val, int val2, long mask)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	int i, ret;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(mpl3115_samp_freq_table); i++)
+		if (val == mpl3115_samp_freq_table[i][0] &&
+		    val2 == mpl3115_samp_freq_table[i][1])
+			break;
+
+	if (i == ARRAY_SIZE(mpl3115_samp_freq_table))
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG2,
+					FIELD_PREP(MPL3115_CTRL2_ST, i));
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
 static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -229,6 +303,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 		.type = IIO_PRESSURE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available =
+			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.scan_index = 0,
 		.scan_type = {
 			.sign = 'u',
@@ -242,6 +319,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all_available =
+			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.scan_index = 1,
 		.scan_type = {
 			.sign = 's',
@@ -312,6 +392,8 @@ static const struct iio_trigger_ops mpl3115_trigger_ops = {
 
 static const struct iio_info mpl3115_info = {
 	.read_raw = &mpl3115_read_raw,
+	.read_avail = &mpl3115_read_avail,
+	.write_raw = &mpl3115_write_raw,
 };
 
 static int mpl3115_trigger_probe(struct mpl3115_data *data,
-- 
2.25.1

Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

[Jonathan Cameron]

On Sat, 27 Sep 2025 00:01:48 +0200
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> Include linux/cleanup.h and use the scoped_guard() to simplify the code.
See below. I'm not sure this is in general a good idea in this driver, but
see the comments below.  I think more traditional factoring out of the code
under the lock into a helper function should be the main change here.
That might or might not make sense combined with a scoped_guard().


> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 579da60ef441..80af672f65c6 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -10,14 +10,16 @@
>   * interrupts, user offset correction, raw mode
>   */
>  
> -#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/module.h>
> +
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> -#include <linux/delay.h>
>  
>  #define MPL3115_STATUS 0x00
>  #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
> @@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	u8 buffer[16] __aligned(8) = { };
>  	int ret, pos = 0;
>  
> -	mutex_lock(&data->lock);
> -	ret = mpl3115_request(data);
> -	if (ret < 0) {
> -		mutex_unlock(&data->lock);
> -		goto done;
> -	}
> -
> -	if (test_bit(0, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> +	scoped_guard(mutex, &data->lock) {
> +		ret = mpl3115_request(data);
> +		if (ret < 0)
>  			goto done;
Read the guidance in cleanup.h.  Whilst what you have here is actually not
a bug, it is considered fragile to combine gotos and scoped cleanup in a function.
Sometimes that means that if we are using guards() we need to also duplicate
some error handling.

So, the way to avoid that is to factor out the stuff under the goto to a helper
function.  That function than then return directly on errors like this.

Looks something like

	scoped_guard(mutex, &data->lock) {
		ret = mpl3115_fill_buffer(data, buffer);
		if (ret) {
			iio_trigger_notify_done(indio_dev->trig);
			return IRQ_HANDLED;
		}
	}

	iio_push_to_buffers_with_ts...
	iio_trigger_notify_done(indio_dev->trig);
	return IRQ_HANDLED;


However, it is also worth keeping in mind that sometimes scoped cleanup
of which guards are a special case is not the right solution for a whole
driver. I'm not sure if it is worth while in this case, but try the approach
mentioned above and see how it looks.

Alternative would still be to factor out the helper, but instead just have
	mutex_lock(&data->lock);
	ret = mpl3115_fill_buffer(data, buffer);
	mutex_unlock(&data->lock);
	if (ret)
		goto...


Jonathan

> +
> +		if (test_bit(0, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_PRESS, 3, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
> +			pos += 4;
>  		}
> -		pos += 4;
> -	}
>  
> -	if (test_bit(1, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			goto done;
> +		if (test_bit(1, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_TEMP, 2, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
>  		}
>  	}
> -	mutex_unlock(&data->lock);
>  
>  	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
>  				    iio_get_time_ns(indio_dev));

Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

[Jonathan Cameron]

On Sat, 27 Sep 2025 00:01:49 +0200
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> MPL3115 sensor features a "data ready" interrupt which indicates the
> presence of new measurements.
> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
Various comments inline.  Main ones are more on the guard() usage combined
with gotos and split out he renames as a precursor patch so only the main
change occurs in this one.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/mpl3115.c | 197 ++++++++++++++++++++++++++++++---
>  1 file changed, 184 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 80af672f65c6..13c8b338a15e 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c

>  
>  #define MPL3115_DEVICE_ID 0xc4
>  
>  #define MPL3115_STATUS_PRESS_RDY BIT(2)
>  #define MPL3115_STATUS_TEMP_RDY BIT(1)
>  
> -#define MPL3115_CTRL_RESET BIT(2) /* software reset */
> -#define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> -#define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> -#define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
> +#define MPL3115_INT_SRC_DRDY BIT(7)
> +
> +#define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> +
> +#define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> +#define MPL3115_CTRL1_OST BIT(1) /* initiate measurement */
> +#define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */

Precursor patch should make the renames to CTRL1
That will reduce the noise in this patch.

> +#define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
> +
> +#define MPL3115_CTRL3_IPOL1 BIT(5)
> +#define MPL3115_CTRL3_IPOL2 BIT(1)
> +
> +#define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> +
> +#define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> +
> +#define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
>  
>  struct mpl3115_data {
>  	struct i2c_client *client;
> +	struct iio_trigger *drdy_trig;
>  	struct mutex lock;
>  	u8 ctrl_reg1;
>  };
>  
> +enum mpl3115_irq_type {
> +	INT2_ACTIVE_LOW  = MPL3115_INT2 | IRQF_TRIGGER_FALLING,
> +	INT2_ACTIVE_HIGH = MPL3115_INT2 | IRQF_TRIGGER_RISING,
> +	INT1_ACTIVE_LOW  = (!MPL3115_INT2) | IRQF_TRIGGER_FALLING,
> +	INT1_ACTIVE_HIGH = (!MPL3115_INT2) | IRQF_TRIGGER_RISING,
This mixing and matching of IRQF_ definitions with locally defined
additional flags is fragile because it is more than possible
a future kernel wide change will change the IRQF_ values.

So keep them separate.

> +};
> +
>  static int mpl3115_request(struct mpl3115_data *data)
>  {
>  	int ret, tries = 15;
>  
>  	/* trigger measurement */
>  	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -		data->ctrl_reg1 | MPL3115_CTRL_OST);
> +		data->ctrl_reg1 | MPL3115_CTRL1_OST);
More renames that shouldn't be in this patch.

>  	if (ret < 0)
>  		return ret;
>  
> @@ -58,7 +86,7 @@ static int mpl3115_request(struct mpl3115_data *data)
>  		if (ret < 0)
>  			return ret;
>  		/* wait for data ready, i.e. OST cleared */
> -		if (!(ret & MPL3115_CTRL_OST))
> +		if (!(ret & MPL3115_CTRL1_OST))
More renames for the precursor patch.

>  			break;
>  		msleep(20);
>  	}
> @@ -166,9 +194,11 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	int ret, pos = 0;
>  
>  	scoped_guard(mutex, &data->lock) {
> -		ret = mpl3115_request(data);
> -		if (ret < 0)
> -			goto done;
> +		if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
> +			ret = mpl3115_request(data);
> +			if (ret < 0)
> +				goto done;
This follows on from comment in earlier patch.

> +		}
>
> +
> +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 ctrl_reg1 = data->ctrl_reg1;
> +
> +	if (state)
> +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> +	else
> +		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +
> +	guard(mutex)(&data->lock);
As in earlier patch, don't mix guard() or anything from cleanup.h with
code doing gotos as scope can be very weirdly defined when you do.

This is actually bug free (I think), but doesn't match the guidance notes in cleanup.h

Various options.
1. Don't use guard here
2. Factor out the stuff under the lock.  The helper function has clearly defined
   separate scope so that can contain the goto reg1_cleanup.h bit.

> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +					ctrl_reg1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> +					state ? MPL3115_CTRL4_INT_EN_DRDY : 0);
> +	if (ret < 0)
> +		goto reg1_cleanup;
> +
> +	data->ctrl_reg1 = ctrl_reg1;
> +
> +	return 0;
> +
> +reg1_cleanup:
> +	i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +				  data->ctrl_reg1);
> +	return ret;
> +}

>  
> +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> +				 struct iio_dev *indio_dev)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> +	int ret, irq, irq_type, irq_cfg_flags = 0;
> +
> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> +	if (irq < 0) {
> +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> +		if (irq < 0)
> +			return 0;
> +
> +		irq_cfg_flags |= MPL3115_INT2;
> +	}
> +
> +	irq_type = irq_get_trigger_type(irq);
> +	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> +		return -EINVAL;
> +
> +	irq_cfg_flags |= irq_type;
Commented on this before, but mixing flags that are local to this driver
with those that are global provides not guarantees against future changes
of the global ones to overlap with your local values.

So just track these as two separate values rather than combining them.

> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> +					MPL3115_PT_DATA_EVENT_ALL);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (irq_cfg_flags) {
> +	case INT2_ACTIVE_HIGH:
> +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> +						MPL3115_CTRL3_IPOL2);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case INT2_ACTIVE_LOW:
> +		break;
> +	case INT1_ACTIVE_HIGH:
> +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> +						MPL3115_CTRL5_INT_CFG_DRDY);
> +		if (ret)
> +			return ret;
> +
> +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> +						MPL3115_CTRL3_IPOL1);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case INT1_ACTIVE_LOW:
> +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> +						MPL3115_CTRL5_INT_CFG_DRDY);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> +						 "%s-dev%d",
> +						 indio_dev->name,
> +						 iio_device_id(indio_dev));
> +	if (!data->drdy_trig)
> +		return -ENOMEM;
> +
> +	data->drdy_trig->ops = &mpl3115_trigger_ops;
> +	iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> +	ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);

Whilst unlikely the race matters. It is this call that creates the infrastructure
that might allow the interrupt generation to be triggered via userspace controls.
So the handler should probably be in place firsts.  I.e. do the devm_request_threaded_irq
before this.

> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> +	return devm_request_threaded_irq(&data->client->dev, irq,
> +					 NULL,
> +					 mpl3115_interrupt_handler,
> +					 IRQF_ONESHOT,
> +					 "mpl3115_irq",
> +					 indio_dev);

wrap closer to 80 chars by combining some of those lines.

> +}
> +
>  static int mpl3115_probe(struct i2c_client *client)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -258,15 +425,19 @@ static int mpl3115_probe(struct i2c_client *client)
>  
>  	/* software reset, I2C transfer is aborted (fails) */
>  	i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> -		MPL3115_CTRL_RESET);
> +		MPL3115_CTRL1_RESET);
>  	msleep(50);
>  
> -	data->ctrl_reg1 = MPL3115_CTRL_OS_258MS;
> +	data->ctrl_reg1 = MPL3115_CTRL1_OS_258MS;
As elsewhere.  Do the rename as a precursor patch so that we reduce
the noise around the real changes in here and make that bit easier to review.

>  	ret = i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
>  		data->ctrl_reg1);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = mpl3115_trigger_probe(data, indio_dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		mpl3115_trigger_handler, NULL);
>  	if (ret < 0)
> @@ -285,7 +456,7 @@ static int mpl3115_probe(struct i2c_client *client)
>  static int mpl3115_standby(struct mpl3115_data *data)
>  {
>  	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -		data->ctrl_reg1 & ~MPL3115_CTRL_ACTIVE);
> +		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
As above. This isn't part of the main change here so should have been in a separate
precursor patch

>  }
>  
>  static void mpl3115_remove(struct i2c_client *client)

Re: [PATCH v3 4/4] iio: mpl3115: add support for sampling frequency

[Jonathan Cameron]

On Sat, 27 Sep 2025 00:01:50 +0200
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> When the device is in ACTIVE mode the temperature and pressure measurements
> are collected with a frequency determined by the ST[3:0] bits of CTRL_REG2
> register.
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
One minor formatting thing inline.

And whilst I'm here, slow down a little.  No need to send a v3 for a missing
include.  Just do as you did and reply to say that it needs adding then wait
for reviews on v2.

In general, leaving a series on list for a week or more is a good plan to gather reviews
before doing a new version.

Jonathan


> ---
>  drivers/iio/pressure/mpl3115.c | 82 ++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 13c8b338a15e..04c126ff4d46 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -10,6 +10,7 @@
>   * user offset correction, raw mode
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> @@ -30,6 +31,7 @@
>  #define MPL3115_INT_SOURCE 0x12
>  #define MPL3115_PT_DATA_CFG 0x13
>  #define MPL3115_CTRL_REG1 0x26
> +#define MPL3115_CTRL_REG2 0x27
>  #define MPL3115_CTRL_REG3 0x28
>  #define MPL3115_CTRL_REG4 0x29
>  #define MPL3115_CTRL_REG5 0x2a
> @@ -48,6 +50,8 @@
>  #define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
>  #define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
>  
> +#define MPL3115_CTRL2_ST GENMASK(3, 0)
> +
>  #define MPL3115_CTRL3_IPOL1 BIT(5)
>  #define MPL3115_CTRL3_IPOL2 BIT(1)
>  
> @@ -57,6 +61,25 @@
>  
>  #define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
>  
> +static const unsigned int mpl3115_samp_freq_table[][2] = {
> +	{ 1,      0},

space before } in each of these.


> +	{ 0, 500000},
> +	{ 0, 250000},
> +	{ 0, 125000},
> +	{ 0,  62500},
> +	{ 0,  31250},
> +	{ 0,  15625},
> +	{ 0,   7812},
> +	{ 0,   3906},
> +	{ 0,   1953},
> +	{ 0,    976},
> +	{ 0,    488},
> +	{ 0,    244},
> +	{ 0,    122},
> +	{ 0,     61},
> +	{ 0,     30},
> +};

Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

[Antoni Pokusinski]

On Sat, Sep 27, 2025 at 05:36:21PM +0100, Jonathan Cameron wrote:
> On Sat, 27 Sep 2025 00:01:48 +0200
> Antoni Pokusinski <apokusinski01@gmail.com> wrote:
> 
> > Include linux/cleanup.h and use the scoped_guard() to simplify the code.
> See below. I'm not sure this is in general a good idea in this driver, but
> see the comments below.  I think more traditional factoring out of the code
> under the lock into a helper function should be the main change here.
> That might or might not make sense combined with a scoped_guard().
> 
> 
> > 
> > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 579da60ef441..80af672f65c6 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -10,14 +10,16 @@
> >   * interrupts, user offset correction, raw mode
> >   */
> >  
> > -#include <linux/module.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/module.h>
> > +
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > -#include <linux/delay.h>
> >  
> >  #define MPL3115_STATUS 0x00
> >  #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
> > @@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >  	u8 buffer[16] __aligned(8) = { };
> >  	int ret, pos = 0;
> >  
> > -	mutex_lock(&data->lock);
> > -	ret = mpl3115_request(data);
> > -	if (ret < 0) {
> > -		mutex_unlock(&data->lock);
> > -		goto done;
> > -	}
> > -
> > -	if (test_bit(0, indio_dev->active_scan_mask)) {
> > -		ret = i2c_smbus_read_i2c_block_data(data->client,
> > -			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->lock);
> > +	scoped_guard(mutex, &data->lock) {
> > +		ret = mpl3115_request(data);
> > +		if (ret < 0)
> >  			goto done;
> Read the guidance in cleanup.h.  Whilst what you have here is actually not
> a bug, it is considered fragile to combine gotos and scoped cleanup in a function.
> Sometimes that means that if we are using guards() we need to also duplicate
> some error handling.
> 
> So, the way to avoid that is to factor out the stuff under the goto to a helper
> function.  That function than then return directly on errors like this.
> 
> Looks something like
> 
> 	scoped_guard(mutex, &data->lock) {
> 		ret = mpl3115_fill_buffer(data, buffer);
> 		if (ret) {
> 			iio_trigger_notify_done(indio_dev->trig);
> 			return IRQ_HANDLED;
> 		}
> 	}
> 
> 	iio_push_to_buffers_with_ts...
> 	iio_trigger_notify_done(indio_dev->trig);
> 	return IRQ_HANDLED;
> 
> 
> However, it is also worth keeping in mind that sometimes scoped cleanup
> of which guards are a special case is not the right solution for a whole
> driver. I'm not sure if it is worth while in this case, but try the approach
> mentioned above and see how it looks.
> 
> Alternative would still be to factor out the helper, but instead just have
> 	mutex_lock(&data->lock);
> 	ret = mpl3115_fill_buffer(data, buffer);
> 	mutex_unlock(&data->lock);
> 	if (ret)
> 		goto...
> 
> 
> Jonathan
>
Thanks for the explanation, both approaches look quite neat to me.
However, if we use scoped_guard() then the iio_trigger_notify_done and
return IRQ_HANDLED are duplicated, so I'd lean slightly towards the
mutex_lock/mutex_unlock solution.

> > +
> > +		if (test_bit(0, indio_dev->active_scan_mask)) {
> > +			ret = i2c_smbus_read_i2c_block_data(data->client,
> > +				MPL3115_OUT_PRESS, 3, &buffer[pos]);
> > +			if (ret < 0)
> > +				goto done;
> > +			pos += 4;
> >  		}
> > -		pos += 4;
> > -	}
> >  
> > -	if (test_bit(1, indio_dev->active_scan_mask)) {
> > -		ret = i2c_smbus_read_i2c_block_data(data->client,
> > -			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->lock);
> > -			goto done;
> > +		if (test_bit(1, indio_dev->active_scan_mask)) {
> > +			ret = i2c_smbus_read_i2c_block_data(data->client,
> > +				MPL3115_OUT_TEMP, 2, &buffer[pos]);
> > +			if (ret < 0)
> > +				goto done;
> >  		}
> >  	}
> > -	mutex_unlock(&data->lock);
> >  
> >  	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
> >  				    iio_get_time_ns(indio_dev));
> 

Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

[Antoni Pokusinski]

On Sat, Sep 27, 2025 at 05:51:25PM +0100, Jonathan Cameron wrote:
> On Sat, 27 Sep 2025 00:01:49 +0200
> Antoni Pokusinski <apokusinski01@gmail.com> wrote:
> 
> > MPL3115 sensor features a "data ready" interrupt which indicates the
> > presence of new measurements.
> > 
> > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> Various comments inline.  Main ones are more on the guard() usage combined
> with gotos and split out he renames as a precursor patch so only the main
> change occurs in this one.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/pressure/mpl3115.c | 197 ++++++++++++++++++++++++++++++---
> >  1 file changed, 184 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 80af672f65c6..13c8b338a15e 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> 
> >  
> >  #define MPL3115_DEVICE_ID 0xc4
> >  
> >  #define MPL3115_STATUS_PRESS_RDY BIT(2)
> >  #define MPL3115_STATUS_TEMP_RDY BIT(1)
> >  
> > -#define MPL3115_CTRL_RESET BIT(2) /* software reset */
> > -#define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> > -#define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> > -#define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
> > +#define MPL3115_INT_SRC_DRDY BIT(7)
> > +
> > +#define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> > +
> > +#define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> > +#define MPL3115_CTRL1_OST BIT(1) /* initiate measurement */
> > +#define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
> 
> Precursor patch should make the renames to CTRL1
> That will reduce the noise in this patch.
>
Sure, will add a separate patch in v4 with all the renames.
> > +#define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
> > +
> > +#define MPL3115_CTRL3_IPOL1 BIT(5)
> > +#define MPL3115_CTRL3_IPOL2 BIT(1)
> > +
> > +#define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> > +
> > +#define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> > +
> > +#define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
> >  
> >  struct mpl3115_data {
> >  	struct i2c_client *client;
> > +	struct iio_trigger *drdy_trig;
> >  	struct mutex lock;
> >  	u8 ctrl_reg1;
> >  };
> >  
> > +enum mpl3115_irq_type {
> > +	INT2_ACTIVE_LOW  = MPL3115_INT2 | IRQF_TRIGGER_FALLING,
> > +	INT2_ACTIVE_HIGH = MPL3115_INT2 | IRQF_TRIGGER_RISING,
> > +	INT1_ACTIVE_LOW  = (!MPL3115_INT2) | IRQF_TRIGGER_FALLING,
> > +	INT1_ACTIVE_HIGH = (!MPL3115_INT2) | IRQF_TRIGGER_RISING,
> This mixing and matching of IRQF_ definitions with locally defined
> additional flags is fragile because it is more than possible
> a future kernel wide change will change the IRQF_ values.
> 
> So keep them separate.
> 
> > +};
> > +
> >  static int mpl3115_request(struct mpl3115_data *data)
> >  {
> >  	int ret, tries = 15;
> >  
> >  	/* trigger measurement */
> >  	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -		data->ctrl_reg1 | MPL3115_CTRL_OST);
> > +		data->ctrl_reg1 | MPL3115_CTRL1_OST);
> More renames that shouldn't be in this patch.
> 
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -58,7 +86,7 @@ static int mpl3115_request(struct mpl3115_data *data)
> >  		if (ret < 0)
> >  			return ret;
> >  		/* wait for data ready, i.e. OST cleared */
> > -		if (!(ret & MPL3115_CTRL_OST))
> > +		if (!(ret & MPL3115_CTRL1_OST))
> More renames for the precursor patch.
> 
> >  			break;
> >  		msleep(20);
> >  	}
> > @@ -166,9 +194,11 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >  	int ret, pos = 0;
> >  
> >  	scoped_guard(mutex, &data->lock) {
> > -		ret = mpl3115_request(data);
> > -		if (ret < 0)
> > -			goto done;
> > +		if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
> > +			ret = mpl3115_request(data);
> > +			if (ret < 0)
> > +				goto done;
> This follows on from comment in earlier patch.
> 
> > +		}
> >
> > +
> > +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> > +{
> > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +	struct mpl3115_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	u8 ctrl_reg1 = data->ctrl_reg1;
> > +
> > +	if (state)
> > +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > +	else
> > +		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > +
> > +	guard(mutex)(&data->lock);
> As in earlier patch, don't mix guard() or anything from cleanup.h with
> code doing gotos as scope can be very weirdly defined when you do.
> 
> This is actually bug free (I think), but doesn't match the guidance notes in cleanup.h
> 
> Various options.
> 1. Don't use guard here
> 2. Factor out the stuff under the lock.  The helper function has clearly defined
>    separate scope so that can contain the goto reg1_cleanup.h bit.
> 
Ok, in here I'd try to factor this out and have something like:

  guard(mutex)(&data->lock);

  return mpl3115_set_interrupt_state(data, ctrl_reg1);

> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +					ctrl_reg1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> > +					state ? MPL3115_CTRL4_INT_EN_DRDY : 0);
> > +	if (ret < 0)
> > +		goto reg1_cleanup;
> > +
> > +	data->ctrl_reg1 = ctrl_reg1;
> > +
> > +	return 0;
> > +
> > +reg1_cleanup:
> > +	i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +				  data->ctrl_reg1);
> > +	return ret;
> > +}
> 
> >  
> > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > +				 struct iio_dev *indio_dev)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> > +	int ret, irq, irq_type, irq_cfg_flags = 0;
> > +
> > +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> > +	if (irq < 0) {
> > +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> > +		if (irq < 0)
> > +			return 0;
> > +
> > +		irq_cfg_flags |= MPL3115_INT2;
> > +	}
> > +
> > +	irq_type = irq_get_trigger_type(irq);
> > +	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> > +		return -EINVAL;
> > +
> > +	irq_cfg_flags |= irq_type;
> Commented on this before, but mixing flags that are local to this driver
> with those that are global provides not guarantees against future changes
> of the global ones to overlap with your local values.
> 
> So just track these as two separate values rather than combining them.
>

So you mean 2 separate variables, one for INT1/INT2 and one for the
trigger RISING/FALLING, am I right?
This was the approach in v1, but the code for writing the regs CTRL3 and
CTRL5 should be improved, I was thinking something like:

if (irq_pin == MPL3115_IRQ_INT1) {
    write_byte_data(REG5, INT_CFG_DRDY);
    if (irq_type == IRQF_TRIGGER_RISING)
        write_byte_data(REG3, IPOL1);
} else if (irq_type == IRQF_TRIGGER_RISING) {
    write_byte_data(REG3, IPOL2);
}

This is perhaps a bit less readable than the switch(int_cfg_flags) with 4
cases... but IMO it's still quite ok and it's less verbose since we do not
duplicate the write_byte_data(REG5, INT_CFG_DRDY).

> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> > +					MPL3115_PT_DATA_EVENT_ALL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (irq_cfg_flags) {
> > +	case INT2_ACTIVE_HIGH:
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> > +						MPL3115_CTRL3_IPOL2);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case INT2_ACTIVE_LOW:
> > +		break;
> > +	case INT1_ACTIVE_HIGH:
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> > +						MPL3115_CTRL5_INT_CFG_DRDY);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> > +						MPL3115_CTRL3_IPOL1);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case INT1_ACTIVE_LOW:
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> > +						MPL3115_CTRL5_INT_CFG_DRDY);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> > +						 "%s-dev%d",
> > +						 indio_dev->name,
> > +						 iio_device_id(indio_dev));
> > +	if (!data->drdy_trig)
> > +		return -ENOMEM;
> > +
> > +	data->drdy_trig->ops = &mpl3115_trigger_ops;
> > +	iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > +	ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);
> 
> Whilst unlikely the race matters. It is this call that creates the infrastructure
> that might allow the interrupt generation to be triggered via userspace controls.
> So the handler should probably be in place firsts.  I.e. do the devm_request_threaded_irq
> before this.
>
Will fix in v4
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->trig = iio_trigger_get(data->drdy_trig);
> > +
> > +	return devm_request_threaded_irq(&data->client->dev, irq,
> > +					 NULL,
> > +					 mpl3115_interrupt_handler,
> > +					 IRQF_ONESHOT,
> > +					 "mpl3115_irq",
> > +					 indio_dev);
> 
> wrap closer to 80 chars by combining some of those lines.
>
Will fix in v4
> > +}
> > +
> >  static int mpl3115_probe(struct i2c_client *client)
> >  {
> >  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > @@ -258,15 +425,19 @@ static int mpl3115_probe(struct i2c_client *client)
> >  
> >  	/* software reset, I2C transfer is aborted (fails) */
> >  	i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> > -		MPL3115_CTRL_RESET);
> > +		MPL3115_CTRL1_RESET);
> >  	msleep(50);
> >  
> > -	data->ctrl_reg1 = MPL3115_CTRL_OS_258MS;
> > +	data->ctrl_reg1 = MPL3115_CTRL1_OS_258MS;
> As elsewhere.  Do the rename as a precursor patch so that we reduce
> the noise around the real changes in here and make that bit easier to review.
> 
> >  	ret = i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> >  		data->ctrl_reg1);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	ret = mpl3115_trigger_probe(data, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> >  		mpl3115_trigger_handler, NULL);
> >  	if (ret < 0)
> > @@ -285,7 +456,7 @@ static int mpl3115_probe(struct i2c_client *client)
> >  static int mpl3115_standby(struct mpl3115_data *data)
> >  {
> >  	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -		data->ctrl_reg1 & ~MPL3115_CTRL_ACTIVE);
> > +		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> As above. This isn't part of the main change here so should have been in a separate
> precursor patch
> 
> >  }
> >  
> >  static void mpl3115_remove(struct i2c_client *client)
> 
Kind regards,
Antoni

Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

[Jonathan Cameron]

> > > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > > +				 struct iio_dev *indio_dev)
> > > +{
> > > +	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> > > +	int ret, irq, irq_type, irq_cfg_flags = 0;
> > > +
> > > +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> > > +	if (irq < 0) {
> > > +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> > > +		if (irq < 0)
> > > +			return 0;
> > > +
> > > +		irq_cfg_flags |= MPL3115_INT2;
> > > +	}
> > > +
> > > +	irq_type = irq_get_trigger_type(irq);
> > > +	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> > > +		return -EINVAL;
> > > +
> > > +	irq_cfg_flags |= irq_type;  
> > Commented on this before, but mixing flags that are local to this driver
> > with those that are global provides not guarantees against future changes
> > of the global ones to overlap with your local values.
> > 
> > So just track these as two separate values rather than combining them.
> >  
> 
> So you mean 2 separate variables, one for INT1/INT2 and one for the
> trigger RISING/FALLING, am I right?

Yes.

> This was the approach in v1, but the code for writing the regs CTRL3 and
> CTRL5 should be improved, I was thinking something like:
> 
> if (irq_pin == MPL3115_IRQ_INT1) {
>     write_byte_data(REG5, INT_CFG_DRDY);
>     if (irq_type == IRQF_TRIGGER_RISING)
>         write_byte_data(REG3, IPOL1);
> } else if (irq_type == IRQF_TRIGGER_RISING) {
>     write_byte_data(REG3, IPOL2);
> }
> 
> This is perhaps a bit less readable than the switch(int_cfg_flags) with 4
> cases... but IMO it's still quite ok and it's less verbose since we do not
> duplicate the write_byte_data(REG5, INT_CFG_DRDY).

That looks ok to me.

...

				 indio_dev->name,
> > > +						 iio_device_id(indio_dev));
> > > +	if (!data->drdy_trig)
> > > +		return -ENOMEM;
> > > +
> > > +	data->drdy_trig->ops = &mpl3115_trigger_ops;
> > > +	iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > > +	ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);  
> > 
> > Whilst unlikely the race matters. It is this call that creates the infrastructure
> > that might allow the interrupt generation to be triggered via userspace controls.
> > So the handler should probably be in place firsts.  I.e. do the devm_request_threaded_irq
> > before this.
> >  
> Will fix in v4

Side process related note: If you agree with something, just crop it out!  That means
we get to focus in quickly on the bits where there is more discussion to be done.

Your change log in v4 is where I'll see you made these changes.

When there is nothing to continue the discussion around in a thread, don't reply at all.
Thanks etc can come alongside the change log.

Thanks,

Jonathan

p.s. I have periodic sessions of mailing people about the process stuff once the
overall list traffic is larger than it should be for stuff like this. You just happened
to be an 'unlucky' recipient today!