Add support for pressure and temperature rising threshold events.

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

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 4cc103e20a39..bb252ff05ff5 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -16,8 +16,10 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/property.h>
+#include <linux/units.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/triggered_buffer.h>
@@ -30,6 +32,8 @@
 #define MPL3115_WHO_AM_I 0x0c
 #define MPL3115_INT_SOURCE 0x12
 #define MPL3115_PT_DATA_CFG 0x13
+#define MPL3115_PRESS_TGT 0x16 /* MSB first, 16 bit */
+#define MPL3115_TEMP_TGT 0x18
 #define MPL3115_CTRL_REG1 0x26
 #define MPL3115_CTRL_REG2 0x27
 #define MPL3115_CTRL_REG3 0x28
@@ -42,6 +46,8 @@
 #define MPL3115_STATUS_TEMP_RDY BIT(1)
 
 #define MPL3115_INT_SRC_DRDY BIT(7)
+#define MPL3115_INT_SRC_PTH BIT(3)
+#define MPL3115_INT_SRC_TTH BIT(2)
 
 #define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
 
@@ -56,6 +62,8 @@
 #define MPL3115_CTRL3_IPOL2 BIT(1)
 
 #define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
+#define MPL3115_CTRL4_INT_EN_PTH BIT(3)
+#define MPL3115_CTRL4_INT_EN_TTH BIT(2)
 
 #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
 
@@ -307,6 +315,15 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static const struct iio_event_spec mpl3115_temp_press_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 static const struct iio_chan_spec mpl3115_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -322,7 +339,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 			.storagebits = 32,
 			.shift = 12,
 			.endianness = IIO_BE,
-		}
+		},
+		.event_spec = mpl3115_temp_press_event,
+		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
 	},
 	{
 		.type = IIO_TEMP,
@@ -338,7 +357,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 			.storagebits = 16,
 			.shift = 4,
 			.endianness = IIO_BE,
-		}
+		},
+		.event_spec = mpl3115_temp_press_event,
+		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
@@ -348,15 +369,45 @@ 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;
+	__be32 val_press;
+	__be16 val_temp;
 
 	ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
 	if (ret < 0)
 		return IRQ_HANDLED;
 
-	if (!(ret & MPL3115_INT_SRC_DRDY))
+	if (!(ret & (MPL3115_INT_SRC_DRDY | MPL3115_INT_SRC_PTH |
+		     MPL3115_INT_SRC_TTH)))
 		return IRQ_NONE;
 
-	iio_trigger_poll_nested(data->drdy_trig);
+	if (ret & MPL3115_INT_SRC_DRDY)
+		iio_trigger_poll_nested(data->drdy_trig);
+
+	if (ret & MPL3115_INT_SRC_PTH) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_PRESSURE, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING),
+						    iio_get_time_ns(indio_dev));
+
+		/* Reset the SRC_PTH bit in INT_SOURCE */
+		i2c_smbus_read_i2c_block_data(data->client,
+					      MPL3115_OUT_PRESS,
+					      3, (u8 *) &val_press);
+	}
+
+	if (ret & MPL3115_INT_SRC_TTH) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING),
+						    iio_get_time_ns(indio_dev));
+
+		/* Reset the SRC_TTH bit in INT_SOURCE */
+		i2c_smbus_read_i2c_block_data(data->client,
+					      MPL3115_OUT_TEMP,
+					      2, (u8 *) &val_temp);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -391,18 +442,22 @@ 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);
-	u8 ctrl_reg1 = data->ctrl_reg1;
-	u8 ctrl_reg4 = data->ctrl_reg4;
+	u8 ctrl_reg1, ctrl_reg4;
+
+	guard(mutex)(&data->lock);
+
+	ctrl_reg1 = data->ctrl_reg1;
+	ctrl_reg4 = data->ctrl_reg4;
 
 	if (state) {
 		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
 		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
 	} else {
-		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
 		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
-	}
 
-	guard(mutex)(&data->lock);
+		if (!ctrl_reg4)
+			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
+	}
 
 	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
 }
@@ -411,10 +466,156 @@ static const struct iio_trigger_ops mpl3115_trigger_ops = {
 	.set_trigger_state = mpl3115_set_trigger_state,
 };
 
+static int mpl3115_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 int_en_mask;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
+		break;
+	case IIO_TEMP:
+		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return !!(data->ctrl_reg4 & int_en_mask);
+}
+
+static int mpl3115_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 int_en_mask;
+	u8 ctrl_reg1, ctrl_reg4;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
+		break;
+	case IIO_TEMP:
+		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	guard(mutex)(&data->lock);
+
+	ctrl_reg1 = data->ctrl_reg1;
+	ctrl_reg4 = data->ctrl_reg4;
+
+	if (state) {
+		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
+		ctrl_reg4 |= int_en_mask;
+	} else {
+		ctrl_reg4 &= ~int_en_mask;
+
+		if (!ctrl_reg4)
+			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
+	}
+
+	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
+}
+
+static int mpl3115_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int *val, int *val2)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	int ret, press_pa;
+	__be16 tmp;
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		ret = i2c_smbus_read_i2c_block_data(data->client,
+						    MPL3115_PRESS_TGT, 2,
+						    (u8 *) &tmp);
+		if (ret < 0)
+			return ret;
+
+		/**
+		 * Target value for the pressure is
+		 * 16-bit unsigned value in 2 Pa units
+		 */
+		press_pa = be16_to_cpu(tmp) << 1;
+		*val = press_pa / KILO;
+		*val2 = (press_pa % KILO) * MILLI;
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_TEMP:
+		ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
+		if (ret < 0)
+			return ret;
+
+		/* Target value for the temperature is 8-bit 2's complement */
+		*val = sign_extend32(ret, 7);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mpl3115_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 tmp[2];
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		val = (val * KILO + val2 / MILLI) >> 1;
+
+		if (val < 0 || val > 0xffff)
+			return -EINVAL;
+
+		tmp[0] = FIELD_GET(GENMASK(15, 8), val);
+		tmp[1] = FIELD_GET(GENMASK(7, 0), val);
+
+		return i2c_smbus_write_i2c_block_data(data->client,
+						      MPL3115_PRESS_TGT, 2, tmp);
+	case IIO_TEMP:
+		if (val < -128 || val > 127)
+			return -EINVAL;
+
+		return i2c_smbus_write_byte_data(data->client,
+						 MPL3115_TEMP_TGT, val);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info mpl3115_info = {
 	.read_raw = &mpl3115_read_raw,
 	.read_avail = &mpl3115_read_avail,
 	.write_raw = &mpl3115_write_raw,
+	.read_event_config = mpl3115_read_event_config,
+	.write_event_config = mpl3115_write_event_config,
+	.read_event_value = mpl3115_read_thresh,
+	.write_event_value = mpl3115_write_thresh,
 };
 
 static int mpl3115_trigger_probe(struct mpl3115_data *data,
-- 
2.25.1

...
> +static int mpl3115_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 int_en_mask;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> +		break;
Usual convention in IIO drivers is to return early whenever possible
		return !!(data->ctrl_reg4 & MPL3115_CTRL4_INT_EN_PTH);

> +	case IIO_TEMP:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> +		break;
same here
		return !!(data->ctrl_reg4 & MPL3115_CTRL4_INT_EN_TTH);

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return !!(data->ctrl_reg4 & int_en_mask);
> +}
> +
...
> +static int mpl3115_read_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info,
> +			       int *val, int *val2)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	int ret, press_pa;
> +	__be16 tmp;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		ret = i2c_smbus_read_i2c_block_data(data->client,
> +						    MPL3115_PRESS_TGT, 2,
> +						    (u8 *) &tmp);
> +		if (ret < 0)
> +			return ret;
> +
> +		/**
> +		 * Target value for the pressure is
> +		 * 16-bit unsigned value in 2 Pa units
> +		 */
> +		press_pa = be16_to_cpu(tmp) << 1;
> +		*val = press_pa / KILO;
> +		*val2 = (press_pa % KILO) * MILLI;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;

Looks like this is intended to provide the value in kilopascal. Though, as
specified by pressure_raw ABI [1], we only get to kilopascal after applying
channel _offset and _scale. So, this would have to use _input ABI [2], or
provide a value that can be scaled to kilopascal.
If I'm not missing anything, 

		*val = be16_to_cpu(tmp) << 1;
		return IIO_VAL_INT;

would comply with the _raw ABI.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n397
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n1061

> +	case IIO_TEMP:
> +		ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Target value for the temperature is 8-bit 2's complement */
> +		*val = sign_extend32(ret, 7);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mpl3115_write_thresh(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info,
> +				int val, int val2)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 tmp[2];
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		val = (val * KILO + val2 / MILLI) >> 1;
same here. You seem to want to use the _input ABI.
Or, if you chose to use the _raw ABI, take the raw threshold value
		val = val >> 1;
		if (val < 0 || val > 0xffff)
			return -EINVAL;
...

Might also use a local variable to hold the adjusted val >> 1 pressure threshold.

You may also add docs for those. e.g.
What:		/sys/.../iio:deviceX/events/in_pressure_thresh_rising_en
and
What:		/sys/.../events/in_pressure_raw_thresh_rising_value
to ABI documentation.
The ABI doc would probably be best appreciated in a separate patch.

> +
> +		if (val < 0 || val > 0xffff)
> +			return -EINVAL;
> +
> +		tmp[0] = FIELD_GET(GENMASK(15, 8), val);
> +		tmp[1] = FIELD_GET(GENMASK(7, 0), val);
> +
> +		return i2c_smbus_write_i2c_block_data(data->client,
> +						      MPL3115_PRESS_TGT, 2, tmp);

With best regards,
Marcelo

On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:
> Add support for pressure and temperature rising threshold events.

...

> @@ -322,7 +339,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  			.storagebits = 32,
>  			.shift = 12,
>  			.endianness = IIO_BE,
> -		}
> +		},
> +		.event_spec = mpl3115_temp_press_event,
> +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
>  	},
>  	{
>  		.type = IIO_TEMP,
> @@ -338,7 +357,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  			.storagebits = 16,
>  			.shift = 4,
>  			.endianness = IIO_BE,
> -		}
> +		},

Just a side note below, no action from you required on this!

Yeah, yet another reminder for the comma/not-a-comma choices made initially and
why it's important to follow the advice

> +		.event_spec = mpl3115_temp_press_event,
> +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };

...

> -	if (!(ret & MPL3115_INT_SRC_DRDY))
> +	if (!(ret & (MPL3115_INT_SRC_DRDY | MPL3115_INT_SRC_PTH |
> +		     MPL3115_INT_SRC_TTH)))

Can we rather keep this split logical?

	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
		     MPL3115_INT_SRC_DRDY)))

>  		return IRQ_NONE;

...

> -	u8 ctrl_reg1 = data->ctrl_reg1;
> -	u8 ctrl_reg4 = data->ctrl_reg4;
> +	u8 ctrl_reg1, ctrl_reg4;

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

Why this is moved? Before the access to the data->ctrl* was done without
locking. Is it an existing bug?

> +	ctrl_reg1 = data->ctrl_reg1;
> +	ctrl_reg4 = data->ctrl_reg4;
>  
>  	if (state) {
>  		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
>  		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
>  	} else {
> -		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
>  		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> -	}
>  
> -	guard(mutex)(&data->lock);
> +		if (!ctrl_reg4)
> +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +	}
>  
>  	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);

...

> +static int mpl3115_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      bool state)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 int_en_mask;
> +	u8 ctrl_reg1, ctrl_reg4;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> +		break;
> +	case IIO_TEMP:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

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

Similar Q here, why do you protect data that was (still is?) not protected before?

> +	ctrl_reg1 = data->ctrl_reg1;
> +	ctrl_reg4 = data->ctrl_reg4;
> +
> +	if (state) {
> +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> +		ctrl_reg4 |= int_en_mask;
> +	} else {
> +		ctrl_reg4 &= ~int_en_mask;
> +
> +		if (!ctrl_reg4)
> +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +	}
> +
> +	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> +}

...

> +static int mpl3115_read_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info,
> +			       int *val, int *val2)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	int ret, press_pa;
> +	__be16 tmp;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		ret = i2c_smbus_read_i2c_block_data(data->client,
> +						    MPL3115_PRESS_TGT, 2,

sizeof() ?

> +						    (u8 *) &tmp);

Here and elsewhere, drop the space between casting and operand.

> +		if (ret < 0)
> +			return ret;
> +
> +		/**

It's not a kernel-doc.

> +		 * Target value for the pressure is
> +		 * 16-bit unsigned value in 2 Pa units
> +		 */
> +		press_pa = be16_to_cpu(tmp) << 1;
> +		*val = press_pa / KILO;
> +		*val2 = (press_pa % KILO) * MILLI;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_TEMP:
> +		ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Target value for the temperature is 8-bit 2's complement */
> +		*val = sign_extend32(ret, 7);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int mpl3115_write_thresh(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info,
> +				int val, int val2)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 tmp[2];

Use proper __be16 type.

> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		val = (val * KILO + val2 / MILLI) >> 1;

> +		if (val < 0 || val > 0xffff)
> +			return -EINVAL;

U16_MAX?

> +		tmp[0] = FIELD_GET(GENMASK(15, 8), val);
> +		tmp[1] = FIELD_GET(GENMASK(7, 0), val);
> +
> +		return i2c_smbus_write_i2c_block_data(data->client,
> +						      MPL3115_PRESS_TGT, 2, tmp);

sizeof()

> +	case IIO_TEMP:
> +		if (val < -128 || val > 127)
> +			return -EINVAL;

S8_MIN, S8_MAX ?

> +		return i2c_smbus_write_byte_data(data->client,
> +						 MPL3115_TEMP_TGT, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

On Wed, Oct 29, 2025 at 10:24:49AM +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:
> > Add support for pressure and temperature rising threshold events.
> 
> ...
> 
> > @@ -322,7 +339,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >  			.storagebits = 32,
> >  			.shift = 12,
> >  			.endianness = IIO_BE,
> > -		}
> > +		},
> > +		.event_spec = mpl3115_temp_press_event,
> > +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
> >  	},
> >  	{
> >  		.type = IIO_TEMP,
> > @@ -338,7 +357,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >  			.storagebits = 16,
> >  			.shift = 4,
> >  			.endianness = IIO_BE,
> > -		}
> > +		},
> 
> Just a side note below, no action from you required on this!
> 
> Yeah, yet another reminder for the comma/not-a-comma choices made initially and
> why it's important to follow the advice
> 
> > +		.event_spec = mpl3115_temp_press_event,
> > +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
> >  	},
> >  	IIO_CHAN_SOFT_TIMESTAMP(2),
> >  };
> 
> ...
> 
> > -	if (!(ret & MPL3115_INT_SRC_DRDY))
> > +	if (!(ret & (MPL3115_INT_SRC_DRDY | MPL3115_INT_SRC_PTH |
> > +		     MPL3115_INT_SRC_TTH)))
> 
> Can we rather keep this split logical?
> 
> 	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> 		     MPL3115_INT_SRC_DRDY)))
> 
> >  		return IRQ_NONE;
> 
> ...
> 
> > -	u8 ctrl_reg1 = data->ctrl_reg1;
> > -	u8 ctrl_reg4 = data->ctrl_reg4;
> > +	u8 ctrl_reg1, ctrl_reg4;
> 
> > +	guard(mutex)(&data->lock);
> 
> Why this is moved? Before the access to the data->ctrl* was done without
> locking. Is it an existing bug?
> 
Since this patchset adds `write_event_config()` in which CTRL_REG1.ACTIVE
and CTRL_REG4 are modified, the lock now needs to guard the read of
data->ctrl_regX as well. Otherwise, we could have e.g. 2 concurrent
threads executing `set_trigger_state()` and `write_event_config()` that
would read data->ctrl_regX at the same time and then one would overwrite
the other's values in `config_interrupt()`.

In the current driver I don't think there is any bug in here. The only
place (except probe) where the data->ctrl_regX is modified is
`config_interrupt()`, called from `set_trigger_state()`. If we had
concurrent calls to this function, then the final values of CTRL_REG1
and CTRL_REG4 would simply depend on which thread is scheduled as the last one.
With the `guard(mutex)` before accessing data->ctrl_reg1, the situation
would be exactly the same.

> > +	ctrl_reg1 = data->ctrl_reg1;
> > +	ctrl_reg4 = data->ctrl_reg4;
> >  
> >  	if (state) {
> >  		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> >  		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
> >  	} else {
> > -		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> >  		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > -	}
> >  
> > -	guard(mutex)(&data->lock);
> > +		if (!ctrl_reg4)
> > +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > +	}
> >  
> >  	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> 
> ...
> 
> > +static int mpl3115_write_event_config(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      enum iio_event_type type,
> > +				      enum iio_event_direction dir,
> > +				      bool state)
> > +{
> > +	struct mpl3115_data *data = iio_priv(indio_dev);
> > +	u8 int_en_mask;
> > +	u8 ctrl_reg1, ctrl_reg4;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PRESSURE:
> > +		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> > +		break;
> > +	case IIO_TEMP:
> > +		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> > +	guard(mutex)(&data->lock);
> 
> Similar Q here, why do you protect data that was (still is?) not protected before?
> 
Same situation here as in `set_trigger_state()`
> > +	ctrl_reg1 = data->ctrl_reg1;
> > +	ctrl_reg4 = data->ctrl_reg4;
> > +
> > +	if (state) {
> > +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > +		ctrl_reg4 |= int_en_mask;
> > +	} else {
> > +		ctrl_reg4 &= ~int_en_mask;
> > +
> > +		if (!ctrl_reg4)
> > +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > +	}
> > +
> > +	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> > +}
> 
> ...
> 
> > +static int mpl3115_read_thresh(struct iio_dev *indio_dev,
> > +			       const struct iio_chan_spec *chan,
> > +			       enum iio_event_type type,
> > +			       enum iio_event_direction dir,
> > +			       enum iio_event_info info,
> > +			       int *val, int *val2)
> > +{
> > +	struct mpl3115_data *data = iio_priv(indio_dev);
> > +	int ret, press_pa;
> > +	__be16 tmp;
> > +
> > +	if (info != IIO_EV_INFO_VALUE)
> > +		return -EINVAL;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PRESSURE:
> > +		ret = i2c_smbus_read_i2c_block_data(data->client,
> > +						    MPL3115_PRESS_TGT, 2,
> 
> sizeof() ?
> 
> > +						    (u8 *) &tmp);
> 
> Here and elsewhere, drop the space between casting and operand.
> 
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/**
> 
> It's not a kernel-doc.
> 
> > +		 * Target value for the pressure is
> > +		 * 16-bit unsigned value in 2 Pa units
> > +		 */
> > +		press_pa = be16_to_cpu(tmp) << 1;
> > +		*val = press_pa / KILO;
> > +		*val2 = (press_pa % KILO) * MILLI;
> > +
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_TEMP:
> > +		ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* Target value for the temperature is 8-bit 2's complement */
> > +		*val = sign_extend32(ret, 7);
> > +
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> ...
> 
> > +static int mpl3115_write_thresh(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				enum iio_event_type type,
> > +				enum iio_event_direction dir,
> > +				enum iio_event_info info,
> > +				int val, int val2)
> > +{
> > +	struct mpl3115_data *data = iio_priv(indio_dev);
> > +	u8 tmp[2];
> 
> Use proper __be16 type.
> 
> > +	if (info != IIO_EV_INFO_VALUE)
> > +		return -EINVAL;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PRESSURE:
> > +		val = (val * KILO + val2 / MILLI) >> 1;
> 
> > +		if (val < 0 || val > 0xffff)
> > +			return -EINVAL;
> 
> U16_MAX?
> 
> > +		tmp[0] = FIELD_GET(GENMASK(15, 8), val);
> > +		tmp[1] = FIELD_GET(GENMASK(7, 0), val);
> > +
> > +		return i2c_smbus_write_i2c_block_data(data->client,
> > +						      MPL3115_PRESS_TGT, 2, tmp);
> 
> sizeof()
> 
> > +	case IIO_TEMP:
> > +		if (val < -128 || val > 127)
> > +			return -EINVAL;
> 
> S8_MIN, S8_MAX ?
> 
> > +		return i2c_smbus_write_byte_data(data->client,
> > +						 MPL3115_TEMP_TGT, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Kind regards,
Antoni

On Wed, Oct 29, 2025 at 11:46:05PM +0100, Antoni Pokusinski wrote:
> On Wed, Oct 29, 2025 at 10:24:49AM +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:

Please, remove context you are agree with!
Otherwise raise your point(s).

...


> > > -	u8 ctrl_reg1 = data->ctrl_reg1;
> > > -	u8 ctrl_reg4 = data->ctrl_reg4;
> > > +	u8 ctrl_reg1, ctrl_reg4;
> > 
> > > +	guard(mutex)(&data->lock);
> > 
> > Why this is moved? Before the access to the data->ctrl* was done without
> > locking. Is it an existing bug?
> > 
> Since this patchset adds `write_event_config()` in which CTRL_REG1.ACTIVE
> and CTRL_REG4 are modified, the lock now needs to guard the read of
> data->ctrl_regX as well. Otherwise, we could have e.g. 2 concurrent
> threads executing `set_trigger_state()` and `write_event_config()` that
> would read data->ctrl_regX at the same time and then one would overwrite
> the other's values in `config_interrupt()`.
> 
> In the current driver I don't think there is any bug in here. The only
> place (except probe) where the data->ctrl_regX is modified is
> `config_interrupt()`, called from `set_trigger_state()`. If we had
> concurrent calls to this function, then the final values of CTRL_REG1
> and CTRL_REG4 would simply depend on which thread is scheduled as the last one.
> With the `guard(mutex)` before accessing data->ctrl_reg1, the situation
> would be exactly the same.

I see, can you summarize this in the commit message as well?
And/or in the code near to the lock description.

> > > +	ctrl_reg1 = data->ctrl_reg1;
> > > +	ctrl_reg4 = data->ctrl_reg4;
> > >  
> > >  	if (state) {
> > >  		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > >  		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
> > >  	} else {
> > > -		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > >  		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > > -	}
> > >  
> > > -	guard(mutex)(&data->lock);
> > > +		if (!ctrl_reg4)
> > > +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko