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

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)

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

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