Skip to contentSkip to conversation

iio: mpl3115: add support for DRDY interrupt

PATCH Superseded Antoni Pokusinski 7 participants 13 messages Sep 21, 2025 – Sep 27, 2025

Subsystems (2)
Device tree bindings IIO

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Antoni Pokusinski

author
#0 lore raw

cover iio: mpl3115: add support for DRDY interrupt

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

Antoni Pokusinski (3):
dt-bindings: iio: pressure: add binding for mpl3115
iio: mpl3115: add support for DRDY interrupt
iio: mpl3115: add support for sampling frequency

 .../bindings/iio/pressure/fsl,mpl3115.yaml    |  63 +++++
 .../devicetree/bindings/trivial-devices.yaml  |   2 -
 drivers/iio/pressure/mpl3115.c                | 247 +++++++++++++++++-
 3 files changed, 305 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 1/3 dt-bindings: iio: pressure: add binding for mpl3115

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.

Signed-off-by Antoni Pokusinski <[email protected]>
---
 .../bindings/iio/pressure/fsl,mpl3115.yaml    | 63 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +63 -0
new file mode 100644
index 000000000000..9e21eae7acc7
@@ -0,0 +1,63 @@
+# 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 <[email protected]>
+
+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
+
+  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
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pressure@60 {
+            compatible = "fsl,mpl3115";
+            reg = <0x60>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "INT2";
+        };
+    };
Documentation/devicetree/bindings/trivial-devices.yaml +0 -2
index f3dd18681aa6..918d4a12d61c 100644
@@ -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
Signature · 1 line
2.25.1
maintainer Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] dt-bindings: iio: pressure: add binding for mpl3115

Quoted 11 lines · 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.

Signed-off-by Antoni Pokusinski <[email protected]>
---
 .../bindings/iio/pressure/fsl,mpl3115.yaml    | 63 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml
Reviewed-by Rob Herring (Arm) <[email protected]>
#3 lore raw

Re: [PATCH 1/3] dt-bindings: iio: pressure: add binding for mpl3115

In reply to Antoni Pokusinski

Hi Antoni,

Some inline suggestions for making this binding better comply with the 'attempt to make bindings complete' guideline.

Quoted 6 lines · 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.

Signed-off-by Antoni Pokusinski <[email protected]>

---

...

Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml line 23

Quoted 12 lines
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +12 -0
+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

For completeness, could also add the power supplies.

vdd-supply: true

vddio-supply: true

Quoted 12 lines
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +12 -0
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum:
+        - INT1
+        - INT2

Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml line 45

Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +10 -0
+
+  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

and also add the supplies to the example below.

Quoted 8 lines
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +8 -0
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;

Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml line 63

Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/fsl,mpl3115.yaml new file +10 -0
+        #size-cells = <0>;
+
+        pressure@60 {
+            compatible = "fsl,mpl3115";
+            reg = <0x60>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "INT2";
+        };
+    };

Best regards,
Marcelo

author
#4 lore raw

PATCH 2/3 iio: mpl3115: add support for DRDY interrupt

In reply to Antoni Pokusinski

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

Signed-off-by Antoni Pokusinski <[email protected]>
---
 drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-
 1 file changed, 162 insertions(+), 5 deletions(-)
drivers/iio/pressure/mpl3115.c +162 -5
index 579da60ef441..cf34de8f0d7e 100644
@@ -7,7 +7,7 @@
  * (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/module.h>

@@ -17,26 +17,45 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
 #include <linux/delay.h>
+#include <linux/property.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_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
+
 #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_CTRL_IPOL1 BIT(5)
+#define MPL3115_CTRL_IPOL2 BIT(1)
+
+#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
+
+#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
+
 struct mpl3115_data {
 	struct i2c_client *client;
+	struct iio_trigger *drdy_trig;
 	struct mutex lock;
 	u8 ctrl_reg1;
 };

@@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	int ret, pos = 0;
 
 	mutex_lock(&data->lock);
-	ret = mpl3115_request(data);
-	if (ret < 0) {
-		mutex_unlock(&data->lock);
-		goto done;
+	if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+		ret = mpl3115_request(data);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			goto done;
+		}
 	}
 
 	if (test_bit(0, indio_dev->active_scan_mask)) {

@@ -228,10 +249,142 @@ 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_CTRL_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_CTRL_ACTIVE;
+	else
+		ctrl_reg1 &= ~MPL3115_CTRL_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_CTRL_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;
+	int ret, irq, irq_type;
+	bool act_high, is_int2 = false;
+
+	fwnode = dev_fwnode(&data->client->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq < 0) {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq < 0)
+			return 0;
+
+		is_int2 = true;
+	}
+
+	irq_type = irq_get_trigger_type(irq);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		act_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		act_high = false;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+					MPL3115_PT_DATA_EVENT_ALL);
+	if (ret < 0)
+		return ret;
+
+	if (!is_int2) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG5,
+						MPL3115_CTRL_INT_CFG_DRDY);
+		if (ret)
+			return ret;
+	}
+	if (act_high) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG3,
+						is_int2 ? MPL3115_CTRL_IPOL2 :
+							  MPL3115_CTRL_IPOL1);
+		if (ret)
+			return ret;
+	}
+
+	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 = iio_trigger_register(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);

@@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
 	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)
Signature · 1 line
2.25.1
#5 lore raw
Quoted 3 lines · Antoni Pokusinski

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

...

drivers/iio/pressure/mpl3115.c

Quoted 9 lines

#include <linux/module.h>

#include <linux/iio/trigger_consumer.h>
#include <linux/iio/buffer.h>
#include <linux/iio/triggered_buffer.h>

drivers/iio/pressure/mpl3115.c +2 -0
+#include <linux/iio/trigger.h>
 #include <linux/delay.h>
 
+#include <linux/property.h>

This is like delay.h is misplaced. What we do here, we group generic ones followed by subsystem (IIO) related ones, and this seems wrong in this driver.

Can you rather move delay.h to be the first, and add property.h after i2c.h followed by a blank line, so in the result it will be like

delay.h
i2c.h
module.h
property.h
...blank.line...
iio/*.h

...

drivers/iio/pressure/mpl3115.c line 42

Quoted 3 lines
drivers/iio/pressure/mpl3115.c +3 -0
+#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))

Not sure I understand this definition in the following aspects:
1) why is this disrupting the _CTRL_ definitions?
2) why is this using BIT(x) and not respective definitions?
3) why can't you use GENMASK() if you just select all bits in a certain bitfield?

Quoted 4 lines

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

...

drivers/iio/pressure/mpl3115.c static irqreturn_t mpl3115_trigger_handler(int irq, void *p)

Quoted 10 lines
        mutex_lock(&data->lock);
drivers/iio/pressure/mpl3115.c +4 -4
-       ret = mpl3115_request(data);
-       if (ret < 0) {
-               mutex_unlock(&data->lock);
-               goto done;
+       if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+               ret = mpl3115_request(data);
+               if (ret < 0) {
 
+                       mutex_unlock(&data->lock);

Instead, I suggest adding a prerequisite that moves the driver to use cleanup.h, in particular scoped_guard(). This will reduce a churn here,

Quoted 3 lines
drivers/iio/pressure/mpl3115.c +2 -0
+                       goto done;
+               }
        }

...

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = {

Quoted 13 lines
drivers/iio/pressure/mpl3115.c +12 -0
+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_CTRL_ACTIVE;
+       else
+               ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
 
+       guard(mutex)(&data->lock);

Oh, and you already use this! Definitely, it misses the prerequisite patch.

Quoted 19 lines
drivers/iio/pressure/mpl3115.c +19 -0
+       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_CTRL_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;
+}

...

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 321

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +9 -0
+static int mpl3115_trigger_probe(struct mpl3115_data *data,
+                                struct iio_dev *indio_dev)
+{
+       struct fwnode_handle *fwnode;
+       int ret, irq, irq_type;
+       bool act_high, is_int2 = false;
 
+       fwnode = dev_fwnode(&data->client->dev);
+       if (!fwnode)
+               return -ENODEV;

Why is this fatal? Also, do we have a board file for users of this right now?

Quoted 31 lines
drivers/iio/pressure/mpl3115.c +30 -0
+       irq = fwnode_irq_get_byname(fwnode, "INT1");
+       if (irq < 0) {
+               irq = fwnode_irq_get_byname(fwnode, "INT2");
+               if (irq < 0)
+                       return 0;
+
+               is_int2 = true;
+       }
+
+       irq_type = irq_get_trigger_type(irq);
+       switch (irq_type) {
+       case IRQF_TRIGGER_RISING:
+               act_high = true;
+               break;
+       case IRQF_TRIGGER_FALLING:
+               act_high = false;
+               break;
+       default:
+               return -EINVAL;
+       }
+
+       ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+                                       MPL3115_PT_DATA_EVENT_ALL);
+       if (ret < 0)
+               return ret;
 
+       if (!is_int2) {
+               ret = i2c_smbus_write_byte_data(data->client,
+                                               MPL3115_CTRL_REG5,
+                                               MPL3115_CTRL_INT_CFG_DRDY);
+               if (ret)

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 363

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+                       return ret;
+       }
+       if (act_high) {
+               ret = i2c_smbus_write_byte_data(data->client,
+                                               MPL3115_CTRL_REG3,
+                                               is_int2 ? MPL3115_CTRL_IPOL2 :
+                                                         MPL3115_CTRL_IPOL1);
+               if (ret)
+                       return ret;
+       }

This if (!is_int2) and ternary with the same argument is kinda hard to read, can we refactor it somehow?

For example, if these two booleans are represented by a common enum, we can do

switch (cfg_flags) {
case INT2_ACTIVE_HIGH:

    _write_byte_data(REG3);
    break;
case INT2_ACTIVE_LOW:
    break;

case INT1_ACTIVE_HIGH:
_write_byte_data(REG5);
_write_byte_data(REG3);
break;
case INT1_ACTIVE_LOW:
_write_byte_data(REG5);
break;
default:

    return -EINVAL;
}

Yes, it's more verbose, but I find this better to read and understand.

Note, you may drop the switch case for IRQ with this approach as you can use a few bits together (separate bits for raising and falling to make the default case working here).

Quoted 22 lines
drivers/iio/pressure/mpl3115.c +22 -0
+       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 = iio_trigger_register(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);
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#6 lore raw
Quoted 25 lines · Andy Shevchenko
Quoted 3 lines · Antoni Pokusinski

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

...

drivers/iio/pressure/mpl3115.c

Quoted 9 lines

#include <linux/module.h>

#include <linux/iio/trigger_consumer.h>
#include <linux/iio/buffer.h>
#include <linux/iio/triggered_buffer.h>

drivers/iio/pressure/mpl3115.c +2 -0
+#include <linux/iio/trigger.h>
 #include <linux/delay.h>
 
+#include <linux/property.h>

This is like delay.h is misplaced. What we do here, we group generic ones followed by subsystem (IIO) related ones, and this seems wrong in this driver.

Can you rather move delay.h to be the first, and add property.h after i2c.h followed by a blank line, so in the result it will be like

Quoted 9 lines

delay.h
i2c.h
module.h
property.h
...blank.line...
iio/*.h

...

Sure, will fix this in v2.

Quoted 11 lines

drivers/iio/pressure/mpl3115.c line 42

Quoted 3 lines
drivers/iio/pressure/mpl3115.c +3 -0
+#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))

Not sure I understand this definition in the following aspects:
1) why is this disrupting the _CTRL_ definitions?
2) why is this using BIT(x) and not respective definitions?
3) why can't you use GENMASK() if you just select all bits in a certain bitfield?

1) I placed the definitions of the bits/masks in the order of the registers that they correspond to, i.e.
CTRL_INT_SRC_DRDY // bit in reg 0x12
PT_DATA_EVENT_ALL // bits in reg 0x13
CTRL_RESET // bit in reg 0x14

Actually, the wrong name here is the INT_SRC_DRDY definition because it is a bit in the INT_SOURCE register, not a control register. Therefore, the name should be MPL3115_INT_SRC_DRDY instead of MPL3115_CTRL_INT_SRC_DRDY

2) I saw that e.g. CTRL_OS_258MS is defined using BIT(x), so I thought that this is the convention in this driver that I did not want to disrupt by using GENMASK()

3) Sure, I'd even prefer GENMASK(), the only reason why I didn't use it is explained in 2)

Quoted 16 lines
Quoted 4 lines

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

...

Quoted 8 lines
        mutex_lock(&data->lock);
drivers/iio/pressure/mpl3115.c +3 -4
-       ret = mpl3115_request(data);
-       if (ret < 0) {
-               mutex_unlock(&data->lock);
-               goto done;
+       if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+               ret = mpl3115_request(data);
+               if (ret < 0) {
Quoted 6 lines
Quoted 1 line

+ mutex_unlock(&data->lock);

Instead, I suggest adding a prerequisite that moves the driver to use cleanup.h, in particular scoped_guard(). This will reduce a churn here,

Will add in v2.

Quoted 51 lines
Quoted 3 lines
drivers/iio/pressure/mpl3115.c +2 -0
+                       goto done;
+               }
        }

...

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = {

Quoted 13 lines
drivers/iio/pressure/mpl3115.c +12 -0
+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_CTRL_ACTIVE;
+       else
+               ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
 
+       guard(mutex)(&data->lock);

Oh, and you already use this! Definitely, it misses the prerequisite patch.

Quoted 19 lines
drivers/iio/pressure/mpl3115.c +19 -0
+       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_CTRL_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;
+}

...

Quoted 6 lines
drivers/iio/pressure/mpl3115.c +6 -0
+static int mpl3115_trigger_probe(struct mpl3115_data *data,
+                                struct iio_dev *indio_dev)
+{
+       struct fwnode_handle *fwnode;
+       int ret, irq, irq_type;
+       bool act_high, is_int2 = false;
Quoted 7 lines

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 321

Quoted 3 lines
drivers/iio/pressure/mpl3115.c +3 -0
+       fwnode = dev_fwnode(&data->client->dev);
+       if (!fwnode)
+               return -ENODEV;

Why is this fatal? Also, do we have a board file for users of this right now?

Actually it seems it does not have to be fatal. If we get rid of this if, then we'd simply return without setting up the trigger and with no interrupt support, which is ok I guess.

As for the board file, do you mean some PCB schematics? I don't know about any, I've only used the following datasheet from NXP:
https://www.nxp.com/docs/en/data-sheet/MPL3115A2S.pdf

Quoted 64 lines
Quoted 31 lines
drivers/iio/pressure/mpl3115.c +30 -0
+       irq = fwnode_irq_get_byname(fwnode, "INT1");
+       if (irq < 0) {
+               irq = fwnode_irq_get_byname(fwnode, "INT2");
+               if (irq < 0)
+                       return 0;
+
+               is_int2 = true;
+       }
+
+       irq_type = irq_get_trigger_type(irq);
+       switch (irq_type) {
+       case IRQF_TRIGGER_RISING:
+               act_high = true;
+               break;
+       case IRQF_TRIGGER_FALLING:
+               act_high = false;
+               break;
+       default:
+               return -EINVAL;
+       }
+
+       ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+                                       MPL3115_PT_DATA_EVENT_ALL);
+       if (ret < 0)
+               return ret;
 
+       if (!is_int2) {
+               ret = i2c_smbus_write_byte_data(data->client,
+                                               MPL3115_CTRL_REG5,
+                                               MPL3115_CTRL_INT_CFG_DRDY);
+               if (ret)

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 363

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+                       return ret;
+       }
+       if (act_high) {
+               ret = i2c_smbus_write_byte_data(data->client,
+                                               MPL3115_CTRL_REG3,
+                                               is_int2 ? MPL3115_CTRL_IPOL2 :
+                                                         MPL3115_CTRL_IPOL1);
+               if (ret)
+                       return ret;
+       }

This if (!is_int2) and ternary with the same argument is kinda hard to read, can we refactor it somehow?

For example, if these two booleans are represented by a common enum, we can do

switch (cfg_flags) {
case INT2_ACTIVE_HIGH:

    _write_byte_data(REG3);
    break;
case INT2_ACTIVE_LOW:
    break;
case INT1_ACTIVE_HIGH:
   _write_byte_data(REG5);
   _write_byte_data(REG3);
  break;
case INT1_ACTIVE_LOW:
   _write_byte_data(REG5);
   break;
default:
    return -EINVAL;
}
Quoted 6 lines

Yes, it's more verbose, but I find this better to read and understand.

Note, you may drop the switch case for IRQ with this approach as you can use a few bits together (separate bits for raising and falling to make the default case working here).

Ok, your suggestion looks nice. I think to define maybe the enums like this:

#define INT2 BIT(2)
enum {
INT2_ACTIVE_LOW = INT2 | IRQF_TRIGGER_FALLING, INT2_ACTIVE_HIGH = INT2 | IRQF_TRIGGER_RISING, INT1_ACTIVE_LOW = !INT2 | IRQF_TRIGGER_FALLING, INT1_ACTIVE_HIGH = !INT2 | IRQF_TRIGGER_RISING,
};

This way the cfg_flags could be first |= INT_2 (after the call to the fwnode_irq_get_byname) and then |= irq_type (after the call to the irq_get_trigger_type)

Quoted 14 lines
Quoted 14 lines
drivers/iio/pressure/mpl3115.c +14 -0
+       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 = iio_trigger_register(data->drdy_trig);
+       if (ret)
+               return ret;
+
+       indio_dev->trig = iio_trigger_get(data->drdy_trig);
Quoted 12 lines
Quoted 8 lines
drivers/iio/pressure/mpl3115.c +8 -0
+
+       return devm_request_threaded_irq(&data->client->dev, irq,
+                                        NULL,
+                                        mpl3115_interrupt_handler,
+                                        IRQF_ONESHOT,
+                                        "mpl3115_irq",
+                                        indio_dev);
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Kind regards,
Antoni Pokusinski

maintainer
#7 lore raw
In reply to Andy Shevchenko
Quoted 12 lines

...

Quoted 8 lines
        mutex_lock(&data->lock);
drivers/iio/pressure/mpl3115.c +3 -4
-       ret = mpl3115_request(data);
-       if (ret < 0) {
-               mutex_unlock(&data->lock);
-               goto done;
+       if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+               ret = mpl3115_request(data);
+               if (ret < 0) {
Quoted 5 lines
Quoted 1 line

+ mutex_unlock(&data->lock);

Instead, I suggest adding a prerequisite that moves the driver to use cleanup.h, in particular scoped_guard(). This will reduce a churn here,

I'll comment on this in version 3, but I'm not sure scoped_guard() is necessarily a good idea here.

Also note that there is no requirement to use one style universally in a driver (guard / vs explicit unlocks) as often they work best in different usecases with the same locks.

Quoted 4 lines
Quoted 4 lines
drivers/iio/pressure/mpl3115.c +2 -0
+                       goto done;
+               }
        }
#8 lore raw
In reply to Antoni Pokusinski
Quoted 112 lines · Antoni Pokusinski

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

Signed-off-by Antoni Pokusinski <[email protected]>

---
 drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-

 1 file changed, 162 insertions(+), 5 deletions(-)
drivers/iio/pressure/mpl3115.c +1 -1
index 579da60ef441..cf34de8f0d7e 100644
@@ -7,7 +7,7 @@
   * (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/module.h>

drivers/iio/pressure/mpl3115.c +2 -0
@@ -17,26 +17,45 @@
  #include <linux/iio/trigger_consumer.h>
  #include <linux/iio/buffer.h>
  #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
  #include <linux/delay.h>
+#include <linux/property.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

drivers/iio/pressure/mpl3115.c +2 -0
+#define MPL3115_INT_SOURCE 0x12
+#define MPL3115_PT_DATA_CFG 0x13

 #define MPL3115_CTRL_REG1 0x26

drivers/iio/pressure/mpl3115.c +3 -0
+#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)

drivers/iio/pressure/mpl3115.c +4 -0
+#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
+

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

drivers/iio/pressure/mpl3115.c +7 -0
+#define MPL3115_CTRL_IPOL1 BIT(5)
+#define MPL3115_CTRL_IPOL2 BIT(1)
+
+#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
+
+#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
+

 struct mpl3115_data {
  struct i2c_client *client;

drivers/iio/pressure/mpl3115.c +1 -0
+	struct iio_trigger *drdy_trig;

  struct mutex lock;
  u8 ctrl_reg1;
 };

drivers/iio/pressure/mpl3115.c +0 -0
@@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void
 *p)
  	int ret, pos = 0;


  mutex_lock(&data->lock);

drivers/iio/pressure/mpl3115.c +6 -4
-	ret = mpl3115_request(data);
-	if (ret < 0) {
-		mutex_unlock(&data->lock);
-		goto done;
+	if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+		ret = mpl3115_request(data);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			goto done;
+		}

  }

  if (test_bit(0, indio_dev->active_scan_mask)) {

drivers/iio/pressure/mpl3115.c +0 -0
@@ -228,10 +249,142 @@ static const struct iio_chan_spec mpl3115_channels[] =
 {
  	IIO_CHAN_SOFT_TIMESTAMP(2),
  };

drivers/iio/pressure/mpl3115.c +22 -0
+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_CTRL_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);

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 283

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+	int ret;
+	u8 ctrl_reg1 = data->ctrl_reg1;
+
+	if (state)
+		ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
+	else
+		ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
+
+	guard(mutex)(&data->lock);
+

As Andy pointed out, you should have a precursor patch converting the complete driver to use the cleanup logic.

Another nice cleanup you could do (if you want of course) would be to get rid of mpl3115_remove().

Quoted 30 lines
drivers/iio/pressure/mpl3115.c +7 -0
+	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_CTRL_INT_EN_DRDY :

0);

drivers/iio/pressure/mpl3115.c +17 -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,
 };

drivers/iio/pressure/mpl3115.c +1 -0
+static int mpl3115_trigger_probe(struct mpl3115_data *data,

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 322

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+				 struct iio_dev *indio_dev)
+{
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;
+	bool act_high, is_int2 = false;
+
+	fwnode = dev_fwnode(&data->client->dev);
+	if (!fwnode)
+		return -ENODEV;
+

And to add to Andy's review, fwnode_irq_get_byname() will give you an error anyways if !fwnode.

Quoted 44 lines
drivers/iio/pressure/mpl3115.c +37 -0
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq < 0) {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq < 0)
+			return 0;
+
+		is_int2 = true;
+	}
+
+	irq_type = irq_get_trigger_type(irq);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		act_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		act_high = false;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+					MPL3115_PT_DATA_EVENT_ALL);
+	if (ret < 0)
+		return ret;
+
+	if (!is_int2) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG5,
+						MPL3115_CTRL_INT_CFG_DRDY);
+		if (ret)
+			return ret;
+	}
+	if (act_high) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG3,
+						is_int2 ? MPL3115_CTRL_IPOL2

:

drivers/iio/pressure/mpl3115.c +1 -0
+							 

MPL3115_CTRL_IPOL1);

drivers/iio/pressure/mpl3115.c +4 -0
+		if (ret)
+			return ret;
+	}
+

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 374

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+	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 = iio_trigger_register(data->drdy_trig);

devm_iio_trigger_register()

- Nuno Sá

Quoted 27 lines
drivers/iio/pressure/mpl3115.c +13 -0
+	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);

drivers/iio/pressure/mpl3115.c +0 -0
@@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
  	if (ret < 0)
  		return ret;

drivers/iio/pressure/mpl3115.c +4 -0
+	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)

author
#9 lore raw
Quoted 127 lines · Nuno Sá
Quoted 112 lines · Antoni Pokusinski

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

Signed-off-by Antoni Pokusinski <[email protected]>

---
 drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-

 1 file changed, 162 insertions(+), 5 deletions(-)
drivers/iio/pressure/mpl3115.c +1 -1
index 579da60ef441..cf34de8f0d7e 100644
@@ -7,7 +7,7 @@
   * (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/module.h>

drivers/iio/pressure/mpl3115.c +2 -0
@@ -17,26 +17,45 @@
  #include <linux/iio/trigger_consumer.h>
  #include <linux/iio/buffer.h>
  #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
  #include <linux/delay.h>
+#include <linux/property.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

drivers/iio/pressure/mpl3115.c +2 -0
+#define MPL3115_INT_SOURCE 0x12
+#define MPL3115_PT_DATA_CFG 0x13

 #define MPL3115_CTRL_REG1 0x26

drivers/iio/pressure/mpl3115.c +3 -0
+#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)

drivers/iio/pressure/mpl3115.c +4 -0
+#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
+

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

drivers/iio/pressure/mpl3115.c +7 -0
+#define MPL3115_CTRL_IPOL1 BIT(5)
+#define MPL3115_CTRL_IPOL2 BIT(1)
+
+#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
+
+#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
+

 struct mpl3115_data {
  struct i2c_client *client;

drivers/iio/pressure/mpl3115.c +1 -0
+	struct iio_trigger *drdy_trig;

  struct mutex lock;
  u8 ctrl_reg1;
 };

drivers/iio/pressure/mpl3115.c +0 -0
@@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void
 *p)
  	int ret, pos = 0;


  mutex_lock(&data->lock);

drivers/iio/pressure/mpl3115.c +6 -4
-	ret = mpl3115_request(data);
-	if (ret < 0) {
-		mutex_unlock(&data->lock);
-		goto done;
+	if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+		ret = mpl3115_request(data);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			goto done;
+		}

  }

  if (test_bit(0, indio_dev->active_scan_mask)) {

drivers/iio/pressure/mpl3115.c +0 -0
@@ -228,10 +249,142 @@ static const struct iio_chan_spec mpl3115_channels[] =
 {
  	IIO_CHAN_SOFT_TIMESTAMP(2),
  };

drivers/iio/pressure/mpl3115.c +22 -0
+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_CTRL_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);

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 283

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+	int ret;
+	u8 ctrl_reg1 = data->ctrl_reg1;
+
+	if (state)
+		ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
+	else
+		ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
+
+	guard(mutex)(&data->lock);
+

As Andy pointed out, you should have a precursor patch converting the complete driver to use the cleanup logic.

Quoted 3 lines

Another nice cleanup you could do (if you want of course) would be to get rid of mpl3115_remove().

Will add the precursor patch in v2

Quoted 101 lines
Quoted 30 lines
drivers/iio/pressure/mpl3115.c +7 -0
+	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_CTRL_INT_EN_DRDY :

0);

drivers/iio/pressure/mpl3115.c +17 -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,
 };

drivers/iio/pressure/mpl3115.c +1 -0
+static int mpl3115_trigger_probe(struct mpl3115_data *data,

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 322

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+				 struct iio_dev *indio_dev)
+{
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;
+	bool act_high, is_int2 = false;
+
+	fwnode = dev_fwnode(&data->client->dev);
+	if (!fwnode)
+		return -ENODEV;
+

And to add to Andy's review, fwnode_irq_get_byname() will give you an error anyways if !fwnode.

Quoted 44 lines
drivers/iio/pressure/mpl3115.c +37 -0
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq < 0) {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq < 0)
+			return 0;
+
+		is_int2 = true;
+	}
+
+	irq_type = irq_get_trigger_type(irq);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		act_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		act_high = false;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+					MPL3115_PT_DATA_EVENT_ALL);
+	if (ret < 0)
+		return ret;
+
+	if (!is_int2) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG5,
+						MPL3115_CTRL_INT_CFG_DRDY);
+		if (ret)
+			return ret;
+	}
+	if (act_high) {
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG3,
+						is_int2 ? MPL3115_CTRL_IPOL2

:

drivers/iio/pressure/mpl3115.c +1 -0
+							 

MPL3115_CTRL_IPOL1);

drivers/iio/pressure/mpl3115.c +4 -0
+		if (ret)
+			return ret;
+	}
+

drivers/iio/pressure/mpl3115.c static const struct iio_chan_spec mpl3115_channels[] = { · line 374

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+	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 = iio_trigger_register(data->drdy_trig);

devm_iio_trigger_register()

Quoted 2 lines

- Nuno Sá

Will fix in v2

Quoted 27 lines
Quoted 27 lines
drivers/iio/pressure/mpl3115.c +13 -0
+	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);

drivers/iio/pressure/mpl3115.c +0 -0
@@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
  	if (ret < 0)
  		return ret;

drivers/iio/pressure/mpl3115.c +4 -0
+	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)

author
#10 lore raw

PATCH 3/3 iio: mpl3115: add support for sampling frequency

In reply to 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.

Signed-off-by Antoni Pokusinski <[email protected]>
---
 drivers/iio/pressure/mpl3115.c | 80 ++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)
drivers/iio/pressure/mpl3115.c +80 -0
index cf34de8f0d7e..2f1860ca1f32 100644
@@ -28,6 +28,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

@@ -46,6 +47,8 @@
 #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
 #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
 
+#define MPL3115_CTRL_ST (BIT(3) | BIT(2) | BIT(1) | BIT(0))
+
 #define MPL3115_CTRL_IPOL1 BIT(5)
 #define MPL3115_CTRL_IPOL2 BIT(1)
 

@@ -53,6 +56,25 @@
 
 #define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
 
+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;

@@ -163,10 +185,60 @@ 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 &= MPL3115_CTRL_ST;
+
+		*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, 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;

@@ -224,6 +296,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',

@@ -237,6 +312,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',

@@ -307,6 +385,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,
Signature · 1 line
2.25.1
Reviewed-by on v1 3/3
#11 lore raw

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

Quoted 6 lines · 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.

Signed-off-by Antoni Pokusinski <[email protected]>

---

LGTM,

Reviewed-by Nuno Sá <[email protected]>
Quoted 142 lines

 drivers/iio/pressure/mpl3115.c | 80 ++++++++++++++++++++++++++++++++++

 1 file changed, 80 insertions(+)
drivers/iio/pressure/mpl3115.c +1 -0
index cf34de8f0d7e..2f1860ca1f32 100644
@@ -28,6 +28,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

@@ -46,6 +47,8 @@
  #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
  #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */

drivers/iio/pressure/mpl3115.c +2 -0
+#define MPL3115_CTRL_ST (BIT(3) | BIT(2) | BIT(1) | BIT(0))
+

 #define MPL3115_CTRL_IPOL1 BIT(5)
 #define MPL3115_CTRL_IPOL2 BIT(1)

drivers/iio/pressure/mpl3115.c +0 -0
@@ -53,6 +56,25 @@


 #define MPL3115_CTRL_INT_CFG_DRDY BIT(7)

drivers/iio/pressure/mpl3115.c +19 -0
+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;

drivers/iio/pressure/mpl3115.c +0 -0
@@ -163,10 +185,60 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,

  default:
  return -EINVAL;
  }

drivers/iio/pressure/mpl3115.c +2 -0
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = i2c_smbus_read_byte_data(data->client,

MPL3115_CTRL_REG2);

drivers/iio/pressure/mpl3115.c +8 -0
+		if (ret < 0)
+			return ret;
+
+		ret &= MPL3115_CTRL_ST;
+
+		*val = mpl3115_samp_freq_table[ret][0];
+		*val2 = mpl3115_samp_freq_table[ret][1];
+		return IIO_VAL_INT_PLUS_MICRO;

  }
  return -EINVAL;
 }

drivers/iio/pressure/mpl3115.c +40 -0
+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, 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;

drivers/iio/pressure/mpl3115.c +6 -0
@@ -224,6 +296,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',

@@ -237,6 +312,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',

@@ -307,6 +385,8 @@ static const struct iio_trigger_ops mpl3115_trigger_ops =
 {


 static const struct iio_info mpl3115_info = {
  .read_raw = &mpl3115_read_raw,

drivers/iio/pressure/mpl3115.c +2 -0
+	.read_avail = &mpl3115_read_avail,
+	.write_raw = &mpl3115_write_raw,

 };

 static int mpl3115_trigger_probe(struct mpl3115_data *data,

reviewer
#12 lore raw

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

In reply to Antoni Pokusinski
Quoted 22 lines · 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.

Signed-off-by Antoni Pokusinski <[email protected]>
---
 drivers/iio/pressure/mpl3115.c | 80 ++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)
drivers/iio/pressure/mpl3115.c +1 -0
index cf34de8f0d7e..2f1860ca1f32 100644
@@ -28,6 +28,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

drivers/iio/pressure/mpl3115.c line 50

Quoted 4 lines
drivers/iio/pressure/mpl3115.c +1 -0
@@ -46,6 +47,8 @@
 #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
 #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
 
+#define MPL3115_CTRL_ST (BIT(3) | BIT(2) | BIT(1) | BIT(0))

Can be simplified with GENMASK(3, 0).

Would also be more clear to call it MPL3115_CTRL_REG2_ST. Otherwise it looks like it overlaps with the bits above and below.

Quoted 15 lines
drivers/iio/pressure/mpl3115.c +8 -0
+
 #define MPL3115_CTRL_IPOL1 BIT(5)
 #define MPL3115_CTRL_IPOL2 BIT(1)
 

@@ -53,6 +56,25 @@
 
 #define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
 
+static const unsigned int mpl3115_samp_freq_table[][2] = {
+	{ 1,      0},
+	{ 0, 500000},
+	{ 0, 250000},
+	{ 0, 125000},
+	{ 0,  62500},
+	{ 0,  31250},

drivers/iio/pressure/mpl3115.c line 75

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -0
+	{ 0,  15625},
+	{ 0,   7812},
+	{ 0,   3906},
+	{ 0,   1953},
+	{ 0,    976},
+	{ 0,    488},
+	{ 0,    244},
+	{ 0,    122},
+	{ 0,     61},
+	{ 0,     30},

Didn't check these all, but got 30.5... for the last one, which would round to 31. Not that it matters much in this case. ;-)

Quoted 6 lines
drivers/iio/pressure/mpl3115.c +2 -0
+};
+
 struct mpl3115_data {
 	struct i2c_client *client;
 	struct iio_trigger *drdy_trig;

drivers/iio/pressure/mpl3115.c static int mpl3115_read_raw(struct iio_dev *indio_dev, · line 193

Quoted 9 lines
drivers/iio/pressure/mpl3115.c +6 -0
@@ -163,10 +185,60 @@ 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 &= MPL3115_CTRL_ST;

FIELD_GET() would be appropriate here.

Quoted 34 lines
drivers/iio/pressure/mpl3115.c +30 -0
+
+		*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] &&

drivers/iio/pressure/mpl3115.c static int mpl3115_read_raw(struct iio_dev *indio_dev, · line 237

Quoted 10 lines
drivers/iio/pressure/mpl3115.c +10 -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, i);

This potentially clears some unrelated bits. I guess it is not a problem for now since those bit aren't used, but could be easy to overlook in the future.

Also, FIELD_PREP() would be appropriate.

Quoted 36 lines
drivers/iio/pressure/mpl3115.c +12 -0
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
 static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;

@@ -224,6 +296,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',

@@ -237,6 +312,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',

@@ -307,6 +385,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,