[PATCH 00/14] iio: pressure: mprls0025pa: driver code cleanup

[PATCH 00/14] iio: pressure: mprls0025pa: driver code cleanup

[Petre Rodan]

This series contains a collection of patches to the MPR sensor based
on feedback I received for other drivers.

major changes:
 - drop the use of devm_kzalloc()
 - trigger flag fix (define edge direction only in the device tree)
 - mitigate SPI timing violation by changing the measurement sequence
    (only affects users that do not use the EOC interrupt)
 - fix scan_type struct
 - pressure calculation fix for custom chips (does not affect users that define a sensor via the pressure-triplet)
 - stricter check for the status byte + better error return levels

minor changes:
 - includes added and removed
 - rename generic 'buffer' variable to 'rx_buf'
 - remove redundant locking

Tested on two sensors - MPRLS0015PA0000SA and MPRLS0001BA00001A

Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
Petre Rodan (14):
      iio: pressure: mprls0025pa: Kconfig allow bus selection
      iio: pressure: mprls0025pa: remove redundant mutex
      iio: pressure: mprls0025pa: rename buffer variable
      iio: pressure: mprls0025pa: introduce tx buffer
      iio: pressure: mprls0025pa: zero out spi_transfer struct
      iio: pressure: mprls0025pa: memset rx_buf before reading new data
      iio: pressure: mprls0025pa: make ops->write function consistent
      iio: pressure: mprls0025pa: stricter checks for the status byte
      iio: pressure: mprls0025pa: mitigate SPI CS delay violation
      iio: pressure: mprls0025pa: cleanup pressure calculation
      iio: pressure: mprls0025pa: fix scan_type struct
      iio: pressure: mprls0025pa: fix interrupt flag
      iio: pressure: mprls0025pa: cleanup includes and forward declarations
      iio: pressure: mprls0025pa: add copyright line

 drivers/iio/pressure/Kconfig           |  34 +++++----
 drivers/iio/pressure/mprls0025pa.c     | 121 +++++++++++++++------------------
 drivers/iio/pressure/mprls0025pa.h     |  22 ++----
 drivers/iio/pressure/mprls0025pa_i2c.c |  20 +++---
 drivers/iio/pressure/mprls0025pa_spi.c |  28 ++------
 5 files changed, 90 insertions(+), 135 deletions(-)
---
base-commit: f9e05791642810a0cf6237d39fafd6fec5e0b4bb
change-id: 20251215-mprls_cleanup-01de8971b439

Best regards,
-- 
Petre Rodan <petre.rodan@subdimension.ro>

[PATCH 01/14] iio: pressure: mprls0025pa: Kconfig allow bus selection

[Petre Rodan]

Allow the user to select either the SPI or the i2c bus specific
module and autoselect core if needed.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/Kconfig | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 2fe9dc90cceb..cbbbcbebf144 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -187,29 +187,33 @@ config MPL3115
 	  will be called mpl3115.
 
 config MPRLS0025PA
-	tristate "Honeywell MPRLS0025PA (MicroPressure sensors series)"
-	depends on (I2C || SPI_MASTER)
-	select MPRLS0025PA_I2C if I2C
-	select MPRLS0025PA_SPI if SPI_MASTER
+	tristate
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
-	help
-	  Say Y here to build support for the Honeywell MicroPressure pressure
-	  sensor series. There are many different types with different pressure
-	  range. These ranges can be set up in the device tree.
-
-	  To compile this driver as a module, choose M here: the module will be
-	  called mprls0025pa.
 
 config MPRLS0025PA_I2C
-	tristate
-	depends on MPRLS0025PA
+	tristate "Honeywell MPR pressure sensor series I2C driver"
 	depends on I2C
+	select MPRLS0025PA
+	help
+	  Say Y here to build I2C bus support for the Honeywell MicroPressure
+	  series sensor.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mprls0025pa_i2c and you will also get mprls0025pa
+	  for the core module.
 
 config MPRLS0025PA_SPI
-	tristate
-	depends on MPRLS0025PA
+	tristate "Honeywell MPR pressure sensor series SPI driver"
 	depends on SPI_MASTER
+	select MPRLS0025PA
+	help
+	  Say Y here to build SPI bus support for the Honeywell MicroPressure
+	  series sensor.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mprls0025pa_spi and you will also get mprls0025pa
+	  for the core module.
 
 config MS5611
 	tristate "Measurement Specialties MS5611 pressure sensor driver"

-- 
2.51.2

[PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex

[Petre Rodan]

Remove the redundant mutex since both i2c and spi transfer functions
provide their own locking mechanism.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 6 ------
 drivers/iio/pressure/mprls0025pa.h | 3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 2336f2760eae..d3e76eed07e6 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -188,7 +188,6 @@ static void mpr_reset(struct mpr_data *data)
  * If there is an end of conversion (EOC) interrupt registered the function
  * waits for a maximum of one second for the interrupt.
  *
- * Context: The function can sleep and data->lock should be held when calling it
  * Return:
  * * 0		- OK, the pressure value could be read
  * * -ETIMEDOUT	- Timeout while waiting for the EOC interrupt or busy flag is
@@ -273,7 +272,6 @@ static irqreturn_t mpr_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mpr_data *data = iio_priv(indio_dev);
 
-	mutex_lock(&data->lock);
 	ret = mpr_read_pressure(data, &data->chan.pres);
 	if (ret < 0)
 		goto err;
@@ -282,7 +280,6 @@ static irqreturn_t mpr_trigger_handler(int irq, void *p)
 					   iio_get_time_ns(indio_dev));
 
 err:
-	mutex_unlock(&data->lock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -300,9 +297,7 @@ static int mpr_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&data->lock);
 		ret = mpr_read_pressure(data, &pressure);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			return ret;
 		*val = pressure;
@@ -342,7 +337,6 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 	data->ops = ops;
 	data->irq = irq;
 
-	mutex_init(&data->lock);
 	init_completion(&data->completion);
 
 	indio_dev->name = "mprls0025pa";
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index d62a018eaff3..d38ac750e392 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -14,7 +14,6 @@
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/device.h>
-#include <linux/mutex.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
 
@@ -44,7 +43,6 @@ enum mpr_func_id {
  * struct mpr_data
  * @dev: current device structure
  * @ops: functions that implement the sensor reads/writes, bus init
- * @lock: access to device during read
  * @pmin: minimal pressure in pascal
  * @pmax: maximal pressure in pascal
  * @function: transfer function
@@ -66,7 +64,6 @@ enum mpr_func_id {
 struct mpr_data {
 	struct device		*dev;
 	const struct mpr_ops	*ops;
-	struct mutex		lock;
 	u32			pmin;
 	u32			pmax;
 	enum mpr_func_id	function;

-- 
2.51.2

[PATCH 03/14] iio: pressure: mprls0025pa: rename buffer variable

[Petre Rodan]

For the reason of better naming consistency rename priv->buffer into
priv->rx_buf since tx_buf will get introduced in the next patch.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c     | 10 +++++-----
 drivers/iio/pressure/mprls0025pa.h     |  4 ++--
 drivers/iio/pressure/mprls0025pa_i2c.c |  4 ++--
 drivers/iio/pressure/mprls0025pa_spi.c |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index d3e76eed07e6..a1fd3807b455 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -230,7 +230,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 					ret);
 				return ret;
 			}
-			if (!(data->buffer[0] & MPR_ST_ERR_FLAG))
+			if (!(data->rx_buf[0] & MPR_ST_ERR_FLAG))
 				break;
 		}
 		if (i == nloops) {
@@ -243,15 +243,15 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 	if (ret < 0)
 		return ret;
 
-	if (data->buffer[0] & MPR_ST_ERR_FLAG) {
+	if (data->rx_buf[0] & MPR_ST_ERR_FLAG) {
 		dev_err(data->dev,
-			"unexpected status byte %02x\n", data->buffer[0]);
+			"unexpected status byte %02x\n", data->rx_buf[0]);
 		return -ETIMEDOUT;
 	}
 
-	*press = get_unaligned_be24(&data->buffer[1]);
+	*press = get_unaligned_be24(&data->rx_buf[1]);
 
-	dev_dbg(dev, "received: %*ph cnt: %d\n", ret, data->buffer, *press);
+	dev_dbg(dev, "received: %*ph cnt: %d\n", ret, data->rx_buf, *press);
 
 	return 0;
 }
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index d38ac750e392..567f9a465a01 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -59,7 +59,7 @@ enum mpr_func_id {
  * @chan: channel values for buffered mode
  * @chan.pres: pressure value
  * @chan.ts: timestamp
- * @buffer: raw conversion data
+ * @rx_buf: raw conversion data
  */
 struct mpr_data {
 	struct device		*dev;
@@ -80,7 +80,7 @@ struct mpr_data {
 		s32 pres;
 		aligned_s64 ts;
 	} chan;
-	u8	    buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+	u8 rx_buf[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
 };
 
 struct mpr_ops {
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
index 79811fd4a02b..36da0059b19f 100644
--- a/drivers/iio/pressure/mprls0025pa_i2c.c
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -30,8 +30,8 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
 	if (cnt > MPR_MEASUREMENT_RD_SIZE)
 		return -EOVERFLOW;
 
-	memset(data->buffer, 0, MPR_MEASUREMENT_RD_SIZE);
-	ret = i2c_master_recv(client, data->buffer, cnt);
+	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
+	ret = i2c_master_recv(client, data->rx_buf, cnt);
 	if (ret < 0)
 		return ret;
 	else if (ret != cnt)
diff --git a/drivers/iio/pressure/mprls0025pa_spi.c b/drivers/iio/pressure/mprls0025pa_spi.c
index d04102f8a4a0..65958b1186fa 100644
--- a/drivers/iio/pressure/mprls0025pa_spi.c
+++ b/drivers/iio/pressure/mprls0025pa_spi.c
@@ -47,7 +47,7 @@ static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
 
 	buf->tx[0] = cmd;
 	xfer.tx_buf = buf->tx;
-	xfer.rx_buf = data->buffer;
+	xfer.rx_buf = data->rx_buf;
 	xfer.len = pkt_len;
 
 	return spi_sync_transfer(spi, &xfer, 1);

-- 
2.51.2

[PATCH 04/14] iio: pressure: mprls0025pa: introduce tx buffer

[Petre Rodan]

Use a tx_buf that is part of the priv struct for transferring data to
the sensor instead of relying on a devm_kzalloc()-ed array.
Remove the .init operation in the process.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c     |  4 ----
 drivers/iio/pressure/mprls0025pa.h     |  3 ++-
 drivers/iio/pressure/mprls0025pa_i2c.c | 10 ++--------
 drivers/iio/pressure/mprls0025pa_spi.c | 24 ++----------------------
 4 files changed, 6 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index a1fd3807b455..00b1ff1e50a8 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -350,10 +350,6 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 		return dev_err_probe(dev, ret,
 				     "can't get and enable vdd supply\n");
 
-	ret = data->ops->init(data->dev);
-	if (ret)
-		return ret;
-
 	ret = device_property_read_u32(dev,
 				       "honeywell,transfer-function", &func);
 	if (ret)
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index 567f9a465a01..83dbc3ef8ba4 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -60,6 +60,7 @@ enum mpr_func_id {
  * @chan.pres: pressure value
  * @chan.ts: timestamp
  * @rx_buf: raw conversion data
+ * @tx_buf: output buffer
  */
 struct mpr_data {
 	struct device		*dev;
@@ -81,10 +82,10 @@ struct mpr_data {
 		aligned_s64 ts;
 	} chan;
 	u8 rx_buf[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+	u8 tx_buf[MPR_MEASUREMENT_RD_SIZE];
 };
 
 struct mpr_ops {
-	int (*init)(struct device *dev);
 	int (*read)(struct mpr_data *data, const u8 cmd, const u8 cnt);
 	int (*write)(struct mpr_data *data, const u8 cmd, const u8 cnt);
 };
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
index 36da0059b19f..a0bbc6af9283 100644
--- a/drivers/iio/pressure/mprls0025pa_i2c.c
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -17,11 +17,6 @@
 
 #include "mprls0025pa.h"
 
-static int mpr_i2c_init(struct device *unused)
-{
-	return 0;
-}
-
 static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
 {
 	int ret;
@@ -44,9 +39,9 @@ static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
 {
 	int ret;
 	struct i2c_client *client = to_i2c_client(data->dev);
-	u8 wdata[MPR_PKT_SYNC_LEN] = { cmd };
 
-	ret = i2c_master_send(client, wdata, MPR_PKT_SYNC_LEN);
+	data->tx_buf[0] = cmd;
+	ret = i2c_master_send(client, data->tx_buf, MPR_PKT_SYNC_LEN);
 	if (ret < 0)
 		return ret;
 	else if (ret != MPR_PKT_SYNC_LEN)
@@ -56,7 +51,6 @@ static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
 }
 
 static const struct mpr_ops mpr_i2c_ops = {
-	.init = mpr_i2c_init,
 	.read = mpr_i2c_read,
 	.write = mpr_i2c_write,
 };
diff --git a/drivers/iio/pressure/mprls0025pa_spi.c b/drivers/iio/pressure/mprls0025pa_spi.c
index 65958b1186fa..021c0ed812c0 100644
--- a/drivers/iio/pressure/mprls0025pa_spi.c
+++ b/drivers/iio/pressure/mprls0025pa_spi.c
@@ -18,35 +18,16 @@
 
 #include "mprls0025pa.h"
 
-struct mpr_spi_buf {
-	u8 tx[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
-};
-
-static int mpr_spi_init(struct device *dev)
-{
-	struct spi_device *spi = to_spi_device(dev);
-	struct mpr_spi_buf *buf;
-
-	buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	spi_set_drvdata(spi, buf);
-
-	return 0;
-}
-
 static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
 {
 	struct spi_device *spi = to_spi_device(data->dev);
-	struct mpr_spi_buf *buf = spi_get_drvdata(spi);
 	struct spi_transfer xfer;
 
 	if (pkt_len > MPR_MEASUREMENT_RD_SIZE)
 		return -EOVERFLOW;
 
-	buf->tx[0] = cmd;
-	xfer.tx_buf = buf->tx;
+	data->tx_buf[0] = cmd;
+	xfer.tx_buf = data->tx_buf;
 	xfer.rx_buf = data->rx_buf;
 	xfer.len = pkt_len;
 
@@ -54,7 +35,6 @@ static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
 }
 
 static const struct mpr_ops mpr_spi_ops = {
-	.init = mpr_spi_init,
 	.read = mpr_spi_xfer,
 	.write = mpr_spi_xfer,
 };

-- 
2.51.2

[PATCH 05/14] iio: pressure: mprls0025pa: zero out spi_transfer struct

[Petre Rodan]

Make sure that the spi_transfer struct is zeroed out before use.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/mprls0025pa_spi.c b/drivers/iio/pressure/mprls0025pa_spi.c
index 021c0ed812c0..73f0714f66ca 100644
--- a/drivers/iio/pressure/mprls0025pa_spi.c
+++ b/drivers/iio/pressure/mprls0025pa_spi.c
@@ -21,7 +21,7 @@
 static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
 {
 	struct spi_device *spi = to_spi_device(data->dev);
-	struct spi_transfer xfer;
+	struct spi_transfer xfer = { };
 
 	if (pkt_len > MPR_MEASUREMENT_RD_SIZE)
 		return -EOVERFLOW;

-- 
2.51.2

[PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Petre Rodan]

Zero out input buffer before reading the new conversion.
Perform this operation in core instead of in the bus specific code.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c     | 2 ++
 drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 00b1ff1e50a8..7cc8dd0d8476 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -16,6 +16,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/property.h>
+#include <linux/string.h>
 #include <linux/units.h>
 
 #include <linux/gpio/consumer.h>
@@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 		}
 	}
 
+	memset(data->rx_buf, 0, sizeof(data->rx_buf));
 	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
index a0bbc6af9283..0fe8cfe0d7e7 100644
--- a/drivers/iio/pressure/mprls0025pa_i2c.c
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
 	if (cnt > MPR_MEASUREMENT_RD_SIZE)
 		return -EOVERFLOW;
 
-	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
 	ret = i2c_master_recv(client, data->rx_buf, cnt);
 	if (ret < 0)
 		return ret;

-- 
2.51.2

[PATCH 07/14] iio: pressure: mprls0025pa: make ops->write function consistent

[Petre Rodan]

Make the i2c bus write operation more consistent with the rest of the
driver.
Also move defines only used by core out of the common header file.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c     | 5 +++++
 drivers/iio/pressure/mprls0025pa.h     | 4 ----
 drivers/iio/pressure/mprls0025pa_i2c.c | 9 ++++++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 7cc8dd0d8476..7f8dc47d7073 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -39,6 +39,11 @@
 
 #define MPR_ST_ERR_FLAG  (MPR_ST_BUSY | MPR_ST_MEMORY | MPR_ST_MATH)
 
+#define MPR_CMD_NOP      0xf0
+#define MPR_CMD_SYNC     0xaa
+#define MPR_PKT_NOP_LEN  MPR_MEASUREMENT_RD_SIZE
+#define MPR_PKT_SYNC_LEN 3
+
 /*
  * support _RAW sysfs interface:
  *
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index 83dbc3ef8ba4..50c99ddc8937 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -20,10 +20,6 @@
 #include <linux/iio/iio.h>
 
 #define MPR_MEASUREMENT_RD_SIZE 4
-#define MPR_CMD_NOP      0xf0
-#define MPR_CMD_SYNC     0xaa
-#define MPR_PKT_NOP_LEN  MPR_MEASUREMENT_RD_SIZE
-#define MPR_PKT_SYNC_LEN 3
 
 struct device;
 
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
index 0fe8cfe0d7e7..5508a1681b7b 100644
--- a/drivers/iio/pressure/mprls0025pa_i2c.c
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -34,16 +34,19 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
 	return 0;
 }
 
-static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
+static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 cnt)
 {
 	int ret;
 	struct i2c_client *client = to_i2c_client(data->dev);
 
+	if (cnt > MPR_MEASUREMENT_RD_SIZE)
+		return -EOVERFLOW;
+
 	data->tx_buf[0] = cmd;
-	ret = i2c_master_send(client, data->tx_buf, MPR_PKT_SYNC_LEN);
+	ret = i2c_master_send(client, data->tx_buf, cnt);
 	if (ret < 0)
 		return ret;
-	else if (ret != MPR_PKT_SYNC_LEN)
+	else if (ret != cnt)
 		return -EIO;
 
 	return 0;

-- 
2.51.2

[PATCH 08/14] iio: pressure: mprls0025pa: stricter checks for the status byte

[Petre Rodan]

Make sure a valid conversion comes with a status byte that only has
the MPR_ST_POWER bit set.
Return -EBUSY if also MPR_ST_BUSY is set or -EIO otherwise.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 7f8dc47d7073..973d205da3e9 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -12,6 +12,7 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/errno.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -195,9 +196,10 @@ static void mpr_reset(struct mpr_data *data)
  * waits for a maximum of one second for the interrupt.
  *
  * Return:
- * * 0		- OK, the pressure value could be read
- * * -ETIMEDOUT	- Timeout while waiting for the EOC interrupt or busy flag is
- *		  still set after nloops attempts of reading
+ * * 0          - OK, the pressure value could be read
+ * * -EBUSY     - Sensor does not have a new conversion ready
+ * * -ETIMEDOUT - Timeout while waiting for the EOC interrupt
+ * * -EIO       - Invalid status byte received from sensor
  */
 static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 {
@@ -250,10 +252,25 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 	if (ret < 0)
 		return ret;
 
-	if (data->rx_buf[0] & MPR_ST_ERR_FLAG) {
+	/*
+	 * Status byte flags
+	 *  bit7 SANITY_CHK   - must always be 0
+	 *  bit6 MPR_ST_POWER - 1 if device is powered
+	 *  bit5 MPR_ST_BUSY  - 1 if device has no new conversion ready
+	 *  bit4 SANITY_CHK   - must always be 0
+	 *  bit3 SANITY_CHK   - must always be 0
+	 *  bit2 MEMORY_ERR   - 1 if integrity test has failed
+	 *  bit1 SANITY_CHK   - must always be 0
+	 *  bit0 MATH_ERR     - 1 during internal math saturation error
+	 */
+
+	if (data->rx_buf[0] == (MPR_ST_POWER | MPR_ST_BUSY))
+		return -EBUSY;
+
+	if (data->rx_buf[0] != MPR_ST_POWER) {
 		dev_err(data->dev,
-			"unexpected status byte %02x\n", data->rx_buf[0]);
-		return -ETIMEDOUT;
+			"unexpected status byte 0x%02x\n", data->rx_buf[0]);
+		return -EIO;
 	}
 
 	*press = get_unaligned_be24(&data->rx_buf[1]);

-- 
2.51.2

[PATCH 09/14] iio: pressure: mprls0025pa: mitigate SPI CS delay violation

[Petre Rodan]

Based on the sensor datasheet in chapter 7.6 SPI timing, Table 20,
during the SPI transfer there is a minimum time interval between the
CS being asserted and the first clock edge (tHDSS).
This minimum interval of 2.5us is being violated if two consecutive SPI
transfers are queued up, at least on my SPI controller (omap2_mcspi) [1]
As you can see in the first package that only contains a NOP the interval
is 0.75us (half a 800kHz clock cycle).

This patch mitigates the problem by implementing a different measurement
technique that does not involve checking for the EOC indicator before
reading the conversion, thus making sure SPI transfers are not queued up.
see Option 2 in Table 19 SPI output measurement command.
Note that Honeywell's example code also follows this technique for both i2c
and SPI.

This change only affects users that do not define the EOC interrupt in the
device tree.

Remove defines that are no longer used.

[1] https://pasteboard.co/66WN38MRI1wc.png

Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 973d205da3e9..a1d7ec358036 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -12,12 +12,14 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/string.h>
+#include <linux/time.h>
 #include <linux/units.h>
 
 #include <linux/gpio/consumer.h>
@@ -35,10 +37,6 @@
 /* bits in status byte */
 #define MPR_ST_POWER  BIT(6) /* device is powered */
 #define MPR_ST_BUSY   BIT(5) /* device is busy */
-#define MPR_ST_MEMORY BIT(2) /* integrity test passed */
-#define MPR_ST_MATH   BIT(0) /* internal math saturation */
-
-#define MPR_ST_ERR_FLAG  (MPR_ST_BUSY | MPR_ST_MEMORY | MPR_ST_MATH)
 
 #define MPR_CMD_NOP      0xf0
 #define MPR_CMD_SYNC     0xaa
@@ -204,8 +202,7 @@ static void mpr_reset(struct mpr_data *data)
 static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 {
 	struct device *dev = data->dev;
-	int ret, i;
-	int nloops = 10;
+	int ret;
 
 	reinit_completion(&data->completion);
 
@@ -222,29 +219,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 			return -ETIMEDOUT;
 		}
 	} else {
-		/* wait until status indicates data is ready */
-		for (i = 0; i < nloops; i++) {
-			/*
-			 * datasheet only says to wait at least 5 ms for the
-			 * data but leave the maximum response time open
-			 * --> let's try it nloops (10) times which seems to be
-			 *     quite long
-			 */
-			usleep_range(5000, 10000);
-			ret = data->ops->read(data, MPR_CMD_NOP, 1);
-			if (ret < 0) {
-				dev_err(dev,
-					"error while reading, status: %d\n",
-					ret);
-				return ret;
-			}
-			if (!(data->rx_buf[0] & MPR_ST_ERR_FLAG))
-				break;
-		}
-		if (i == nloops) {
-			dev_err(dev, "timeout while reading\n");
-			return -ETIMEDOUT;
-		}
+		fsleep(5 * USEC_PER_MSEC);
 	}
 
 	memset(data->rx_buf, 0, sizeof(data->rx_buf));

-- 
2.51.2

[PATCH 10/14] iio: pressure: mprls0025pa: cleanup pressure calculation

[Petre Rodan]

A sign change is needed for proper calculation of the pressure.
The value of the affected element is zero for all official MPR sensors
covered in the datasheet.

The change will however ensure a correct calculation if a custom chip
is used (and if honeywell,pmin-pascal != 0 in the device tree overlay).

Also due to the fact that raw pressure values can not be lower
than output_min (400k-3.3M) there is no need to calculate a decimal for
the offset.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 26 +++++++++++---------------
 drivers/iio/pressure/mprls0025pa.h |  2 --
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index a1d7ec358036..9b18d5fb7e42 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -64,7 +64,7 @@
  *
  * Values given to the userspace in sysfs interface:
  * * raw	- press_cnt
- * * offset	- (-1 * outputmin) - pmin / scale
+ * * offset	- (-1 * outputmin) + pmin / scale
  *                note: With all sensors from the datasheet pmin = 0
  *                which reduces the offset to (-1 * outputmin)
  */
@@ -307,8 +307,7 @@ static int mpr_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_CHAN_INFO_OFFSET:
 		*val = data->offset;
-		*val2 = data->offset2;
-		return IIO_VAL_INT_PLUS_NANO;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -324,8 +323,9 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 	struct mpr_data *data;
 	struct iio_dev *indio_dev;
 	const char *triplet;
-	s64 scale, offset;
 	u32 func;
+	s32 tmp;
+	s64 odelta, pdelta;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -394,17 +394,13 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 	data->outmin = mpr_func_spec[data->function].output_min;
 	data->outmax = mpr_func_spec[data->function].output_max;
 
-	/* use 64 bit calculation for preserving a reasonable precision */
-	scale = div_s64(((s64)(data->pmax - data->pmin)) * NANO,
-			data->outmax - data->outmin);
-	data->scale = div_s64_rem(scale, NANO, &data->scale2);
-	/*
-	 * multiply with NANO before dividing by scale and later divide by NANO
-	 * again.
-	 */
-	offset = ((-1LL) * (s64)data->outmin) * NANO -
-		  div_s64(div_s64((s64)data->pmin * NANO, scale), NANO);
-	data->offset = div_s64_rem(offset, NANO, &data->offset2);
+	odelta = data->outmax - data->outmin;
+	pdelta = data->pmax - data->pmin;
+
+	data->scale = div_s64_rem(div_s64(pdelta * NANO, odelta), NANO, &tmp);
+	data->scale2 = tmp;
+
+	data->offset = div_s64(odelta * data->pmin, pdelta) - data->outmin;
 
 	if (data->irq > 0) {
 		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index 50c99ddc8937..2bdffe1e0eb1 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -47,7 +47,6 @@ enum mpr_func_id {
  * @scale: pressure scale
  * @scale2: pressure scale, decimal number
  * @offset: pressure offset
- * @offset2: pressure offset, decimal number
  * @gpiod_reset: reset
  * @irq: end of conversion irq. used to distinguish between irq mode and
  *       reading in a loop until data is ready
@@ -69,7 +68,6 @@ struct mpr_data {
 	int			scale;
 	int			scale2;
 	int			offset;
-	int			offset2;
 	struct gpio_desc	*gpiod_reset;
 	int			irq;
 	struct completion	completion;

-- 
2.51.2

[PATCH 11/14] iio: pressure: mprls0025pa: fix scan_type struct

[Petre Rodan]

Fix the scan_type sign and realbits assignment.

The pressure is a 24bit unsigned int between output_min and output_max.

 transfer function A: 10%   to 90%   of 2^24
 transfer function B:  2.5% to 22.5% of 2^24
 transfer function C: 20%   to 80%   of 2^24
[MPR_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 }
[MPR_FUNCTION_B] = { .output_min =  419430, .output_max =  3774874 }
[MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 }

Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 9b18d5fb7e42..243a5717b88f 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -165,8 +165,8 @@ static const struct iio_chan_spec mpr_channels[] = {
 					BIT(IIO_CHAN_INFO_OFFSET),
 		.scan_index = 0,
 		.scan_type = {
-			.sign = 's',
-			.realbits = 32,
+			.sign = 'u',
+			.realbits = 24,
 			.storagebits = 32,
 			.endianness = IIO_CPU,
 		},

-- 
2.51.2

[PATCH 12/14] iio: pressure: mprls0025pa: fix interrupt flag

[Petre Rodan]

Interrupt falling/rising flags should only be defined in the device tree.

Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 243a5717b88f..fc04988b9437 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -14,6 +14,7 @@
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -404,9 +405,7 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 
 	if (data->irq > 0) {
 		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
-				       IRQF_TRIGGER_RISING,
-				       dev_name(dev),
-				       data);
+				       IRQF_ONESHOT, dev_name(dev), data);
 		if (ret)
 			return dev_err_probe(dev, ret,
 					  "request irq %d failed\n", data->irq);

-- 
2.51.2

[PATCH 13/14] iio: pressure: mprls0025pa: cleanup includes and forward declarations

[Petre Rodan]

Remove unused headers and add required headers as needed.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 4 ++++
 drivers/iio/pressure/mprls0025pa.h | 6 ------
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index fc04988b9437..71728f9fc498 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -12,9 +12,12 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/export.h>
 #include <linux/interrupt.h>
+#include <linux/jiffies.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -26,6 +29,7 @@
 #include <linux/gpio/consumer.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index 2bdffe1e0eb1..ccd252f64351 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -12,9 +12,6 @@
 #define _MPRLS0025PA_H
 
 #include <linux/completion.h>
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/stddef.h>
 #include <linux/types.h>
 
 #include <linux/iio/iio.h>
@@ -23,9 +20,6 @@
 
 struct device;
 
-struct iio_chan_spec;
-struct iio_dev;
-
 struct mpr_data;
 struct mpr_ops;
 

-- 
2.51.2

[PATCH 14/14] iio: pressure: mprls0025pa: add copyright line

[Petre Rodan]

Add copyright line to the core driver.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 71728f9fc498..d71b36fcc982 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -3,6 +3,7 @@
  * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver
  *
  * Copyright (c) Andreas Klinger <ak@it-klinger.de>
+ * Copyright (c) 2023-2025 Petre Rodan <petre.rodan@subdimension.ro>
  *
  * Data sheet:
  *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf

-- 
2.51.2

Re: [PATCH 01/14] iio: pressure: mprls0025pa: Kconfig allow bus selection

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Allow the user to select either the SPI or the i2c bus specific
> module and autoselect core if needed.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
LGTM
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

Re: [PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Remove the redundant mutex since both i2c and spi transfer functions
> provide their own locking mechanism.
I don't think that is enough to safely dismiss the mutex lock. There could be
concurrent calls to mpr_read_pressure(). E.g., buffer is enabled and user issues
a single-shot read. To avoid the potential concurrent read (without using the
driver mutex), do iio_device_claim_direct() before calling mpr_read_pressure()
in the IIO_CHAN_INFO_RAW case. 

Re: [PATCH 04/14] iio: pressure: mprls0025pa: introduce tx buffer

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Use a tx_buf that is part of the priv struct for transferring data to
> the sensor instead of relying on a devm_kzalloc()-ed array.
> Remove the .init operation in the process.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
LGTM
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Zero out input buffer before reading the new conversion.
> Perform this operation in core instead of in the bus specific code.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/pressure/mprls0025pa.c     | 2 ++
>  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 00b1ff1e50a8..7cc8dd0d8476 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -16,6 +16,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
> +#include <linux/string.h>
>  #include <linux/units.h>
>  
>  #include <linux/gpio/consumer.h>
> @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
>  		}
>  	}
>  
> +	memset(data->rx_buf, 0, sizeof(data->rx_buf));
This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
subsystem overwrite the rx buffer with what it reads from the device?

>  	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> index a0bbc6af9283..0fe8cfe0d7e7 100644
> --- a/drivers/iio/pressure/mprls0025pa_i2c.c
> +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
>  	if (cnt > MPR_MEASUREMENT_RD_SIZE)
>  		return -EOVERFLOW;
>  
> -	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
>  	ret = i2c_master_recv(client, data->rx_buf, cnt);
>  	if (ret < 0)
>  		return ret;

Re: [PATCH 07/14] iio: pressure: mprls0025pa: make ops->write function consistent

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Make the i2c bus write operation more consistent with the rest of the
> driver.
That's not the most appealing reason for updating driver code. Is the update
meaningful for a different purpose? Consider squashing that with another patch
that makes better use of the updated function. 

> Also move defines only used by core out of the common header file.
Do the define thing in a separate patch.
Commits/patches should be semantically concise (in other words, do one thing on each one).
Usually, the word 'also' in a commit description is a strong indicator that the
second thing being done should be on a patch of it's own.

> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/pressure/mprls0025pa.c     | 5 +++++
>  drivers/iio/pressure/mprls0025pa.h     | 4 ----
>  drivers/iio/pressure/mprls0025pa_i2c.c | 9 ++++++---
>  3 files changed, 11 insertions(+), 7 deletions(-)

Re: [PATCH 08/14] iio: pressure: mprls0025pa: stricter checks for the status byte

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Make sure a valid conversion comes with a status byte that only has
> the MPR_ST_POWER bit set.
> Return -EBUSY if also MPR_ST_BUSY is set or -EIO otherwise.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---

Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

Re: [PATCH 09/14] iio: pressure: mprls0025pa: mitigate SPI CS delay violation

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Based on the sensor datasheet in chapter 7.6 SPI timing, Table 20,
> during the SPI transfer there is a minimum time interval between the
> CS being asserted and the first clock edge (tHDSS).
> This minimum interval of 2.5us is being violated if two consecutive SPI
> transfers are queued up, at least on my SPI controller (omap2_mcspi) [1]
> As you can see in the first package that only contains a NOP the interval
> is 0.75us (half a 800kHz clock cycle).
> 
> This patch mitigates the problem by implementing a different measurement
> technique that does not involve checking for the EOC indicator before
> reading the conversion, thus making sure SPI transfers are not queued up.
The correct way of fixing that is with protocol specific delay. Generic delays
like fsleep won't guarantee any delay between CS drop and fist SCLK edge. 
For SPI, we unfortunately we don't have any interface to set a pre-SCLK delay
so I suggest to make an spi_message with a dummy transfer to cause the initial
2.5 µs delay. E.g.

/* dummy transfer with no data, just cause the delay */
xfers[0].delay.value = 2500 * NSEC_PER_SEC;
xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;

/* normal data transfer  */
xfer[1].tx_buf = data->tx_buf;
xfer[1].rx_buf = data->rx_buf;
...

Also, I don't see how the proposed change is 'implementing a different
measurement technique'. Consider updating the commit description or providing
better explanation.

> see Option 2 in Table 19 SPI output measurement command.
> Note that Honeywell's example code also follows this technique for both i2c
> and SPI.
> 
> This change only affects users that do not define the EOC interrupt in the
> device tree.
> 
> Remove defines that are no longer used.
> 
> [1] https://pasteboard.co/66WN38MRI1wc.png
> 
> Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Re: [PATCH 10/14] iio: pressure: mprls0025pa: cleanup pressure calculation

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> A sign change is needed for proper calculation of the pressure.
> The value of the affected element is zero for all official MPR sensors
> covered in the datasheet.
> 
> The change will however ensure a correct calculation if a custom chip
> is used (and if honeywell,pmin-pascal != 0 in the device tree overlay).
I don't think this should be relevant to the commit log. Correct calculation
should be supported regardless of what is in firmware.
Was pressure data read buggy before? If so, this patch needs a Fixes tag.

> 
> Also due to the fact that raw pressure values can not be lower
> than output_min (400k-3.3M) there is no need to calculate a decimal for
> the offset.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/pressure/mprls0025pa.c | 26 +++++++++++---------------
>  drivers/iio/pressure/mprls0025pa.h |  2 --
>  2 files changed, 11 insertions(+), 17 deletions(-)

Re: [PATCH 13/14] iio: pressure: mprls0025pa: cleanup includes and forward declarations

[Marcelo Schmitt]

On 12/18, Petre Rodan wrote:
> Remove unused headers and add required headers as needed.
Bring the patches that fix something to the beginning of the series and
bring this one right after the fixes.

> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
...
>  
> diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
> index 2bdffe1e0eb1..ccd252f64351 100644
> --- a/drivers/iio/pressure/mprls0025pa.h
> +++ b/drivers/iio/pressure/mprls0025pa.h
> @@ -12,9 +12,6 @@
>  #define _MPRLS0025PA_H
>  
>  #include <linux/completion.h>
> -#include <linux/delay.h>
> -#include <linux/device.h>
> -#include <linux/stddef.h>
>  #include <linux/types.h>
>  
>  #include <linux/iio/iio.h>
> @@ -23,9 +20,6 @@
>  
>  struct device;
>  
> -struct iio_chan_spec;
> -struct iio_dev;
> -
Also, I'd suggest to not mess with other stuff if this patch is supposedly only
organizing/cleaning up includes.

>  struct mpr_data;
>  struct mpr_ops;

Re: [PATCH 09/14] iio: pressure: mprls0025pa: mitigate SPI CS delay violation

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 4948 bytes --]


Hello Marcelo,

thank you for the pointers.

On Sat, Dec 20, 2025 at 01:51:42AM -0300, Marcelo Schmitt wrote:
> On 12/18, Petre Rodan wrote:
> > Based on the sensor datasheet in chapter 7.6 SPI timing, Table 20,
> > during the SPI transfer there is a minimum time interval between the
> > CS being asserted and the first clock edge (tHDSS).
> > This minimum interval of 2.5us is being violated if two consecutive SPI
> > transfers are queued up, at least on my SPI controller (omap2_mcspi) [1]
> > As you can see in the first package that only contains a NOP the interval
> > is 0.75us (half a 800kHz clock cycle).
> > 
> > This patch mitigates the problem by implementing a different measurement
> > technique that does not involve checking for the EOC indicator before
> > reading the conversion, thus making sure SPI transfers are not queued up.
> The correct way of fixing that is with protocol specific delay. Generic delays
> like fsleep won't guarantee any delay between CS drop and fist SCLK edge.

sorry for the lack of detail in the commit log, I forgot to add it.

this patch changes the measurement sequence and as a consequence it also mitigates a problem where protocol-specific delays are not implemented in the SPI controller (for the corner case when multiple SPI transfers are queued up).

as per the datasheet pages 36 and 41, the measurement sequence can follow one of these two scenarios, for both bus implementations:

a) send measurement command and wait for the status byte to lose the busy flag.
this means that we would need to keep sending NOP commands and read the first (status) byte in a loop until the busy flag goes away. this is how the driver was designed before this series of patches.

b) send measurement command and wait for 5ms for the data conversion to occur.
same as a, but instead of polling the sensor for the status byte there's just a 5ms+ delay after which only one NOP command is sent.
the read that follows will get the conversion data plus the status byte.
depending on the status byte flags, either error out or pass the conversion along.

my patch replaces the measurement sequence defined by scenario a with scenario b and it only affects users that did not define the EoC interrupt pin in the device tree overlay.
the datasheet contains sample code (pages 38 and 43) and they also happen to use scenario b, so no polling.
in my testing the sensor always performed as expected (the busy flag was never set after the 5ms fsleep()).

> For SPI, we unfortunately we don't have any interface to set a pre-SCLK delay
> so I suggest to make an spi_message with a dummy transfer to cause the initial
> 2.5 µs delay. E.g.

for normal (non-queued) SPI messages the delay that exist between the CS assert and the first clock is ~4us, well above the 2.5us minimum.
the problem only appears when the SPI controller gets multiple xfers without any delays between them - which happens in scenario a when two transfers get queued up (NOP that reads the status byte and the NOP that reads the status + conversions).

this is just a hunch but I think that the CS timing is directly controlled by hardware when the issue happens.
see chapter 24.3.2.8 page 4903 in the AM335x technical reference manual [1].
I'm measuring exactly 0.5 clock cycles CS lead times which is the default for the TCS bit of the register MCSPI_CH(i)CONF.

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf?ts=1766215152875

> /* dummy transfer with no data, just cause the delay */
> xfers[0].delay.value = 2500 * NSEC_PER_SEC;
> xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> 
> /* normal data transfer  */
> xfer[1].tx_buf = data->tx_buf;
> xfer[1].rx_buf = data->rx_buf;
> ...
> 
> Also, I don't see how the proposed change is 'implementing a different
> measurement technique'. Consider updating the commit description or providing
> better explanation.

I feel that using scenario b defined above we cure the problem before it happens.
my patch provides a less complex implementation, it's recommended by the manufacturer in their sample code and provides less code duplication (status byte parsing) with no downsides afaict.

> > see Option 2 in Table 19 SPI output measurement command.
> > Note that Honeywell's example code also follows this technique for both i2c
> > and SPI.
> > 
> > This change only affects users that do not define the EOC interrupt in the
> > device tree.
> > 
> > Remove defines that are no longer used.
> > 
> > [1] https://pasteboard.co/66WN38MRI1wc.png
> > 
> > Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

best regards,
peter

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]


Hello,

On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:
> On 12/18, Petre Rodan wrote:
> > Zero out input buffer before reading the new conversion.
> > Perform this operation in core instead of in the bus specific code.
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > ---
> >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > --- a/drivers/iio/pressure/mprls0025pa.c
> > +++ b/drivers/iio/pressure/mprls0025pa.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/property.h>
> > +#include <linux/string.h>
> >  #include <linux/units.h>
> >  
> >  #include <linux/gpio/consumer.h>
> > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> >  		}
> >  	}
> >  
> > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));
> This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> subsystem overwrite the rx buffer with what it reads from the device?

I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
that's exactly why in this particular case the BUSY flag is implemented on the hardware side.

please tell me how a few byte memset() would be detrimental.

best regards,
peter

> >  	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
> >  	if (ret < 0)
> >  		return ret;
> > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> > index a0bbc6af9283..0fe8cfe0d7e7 100644
> > --- a/drivers/iio/pressure/mprls0025pa_i2c.c
> > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> >  	if (cnt > MPR_MEASUREMENT_RD_SIZE)
> >  		return -EOVERFLOW;
> >  
> > -	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
> >  	ret = i2c_master_recv(client, data->rx_buf, cnt);
> >  	if (ret < 0)
> >  		return ret;

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 07/14] iio: pressure: mprls0025pa: make ops->write function consistent

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]


Hello,

On Sat, Dec 20, 2025 at 01:49:30AM -0300, Marcelo Schmitt wrote:
> On 12/18, Petre Rodan wrote:
> > Make the i2c bus write operation more consistent with the rest of the
> > driver.
> That's not the most appealing reason for updating driver code. Is the update
> meaningful for a different purpose? Consider squashing that with another patch
> that makes better use of the updated function. 

I see your point.

I wanted to have device specific logic in _core instead of one of the bus drivers,
but if I'm the only one upset about it then I will skip this change.

best regards,
peter

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/14] iio: pressure: mprls0025pa: Kconfig allow bus selection

[Jonathan Cameron]

On Sat, 20 Dec 2025 01:39:01 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 12/18, Petre Rodan wrote:
> > Allow the user to select either the SPI or the i2c bus specific
> > module and autoselect core if needed.
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > ---  
> LGTM
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

Applied this patch to the togreg branch of iio.git.
Initially pushed out as testing for 0-day to take a look.

Thanks,

Jonathan

Re: [PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex

[Jonathan Cameron]

On Sat, 20 Dec 2025 01:45:01 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 12/18, Petre Rodan wrote:
> > Remove the redundant mutex since both i2c and spi transfer functions
> > provide their own locking mechanism.  
> I don't think that is enough to safely dismiss the mutex lock. There could be
> concurrent calls to mpr_read_pressure(). E.g., buffer is enabled and user issues
> a single-shot read. To avoid the potential concurrent read (without using the
> driver mutex), do iio_device_claim_direct() before calling mpr_read_pressure()
> in the IIO_CHAN_INFO_RAW case. 
> 
This is a little messier.

Direct claiming is about preventing transitions between
modes (buffered, polled)
It is an implementation quirk that it only allows one claimer of a mode
at a time. (It would be hard to change that now, as we'd likely expose
many corner cases in drivers, but still the meaning is not the same
as a lock).

If the locking is needed to protect bus access sequences that is a driver
specific thing that should be done with a driver specific lock.  So things
like RWM sequences should never rely on claiming direct mode to serialize
things.

Now, it is also possible that all the sequences only make sense if
we are not in buffered mode, in which case we should be claiming direct
mode as suggested by Marcelo. That is typically an 'as well' thing rather
than instead of a local lock. If it is all about preventing concurrency with
a sequence that only happens in buffered mode, then maybe claiming direct
mode to avoid that is enough.

Jonathan



 

Re: [PATCH 05/14] iio: pressure: mprls0025pa: zero out spi_transfer struct

[Jonathan Cameron]

On Thu, 18 Dec 2025 13:05:47 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Make sure that the spi_transfer struct is zeroed out before use.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Hi Petre

Good catch.

Smells like a bug to me. Probably want to drag this to the start of the
patch set and add an appropriate fixes tag.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/mprls0025pa_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa_spi.c b/drivers/iio/pressure/mprls0025pa_spi.c
> index 021c0ed812c0..73f0714f66ca 100644
> --- a/drivers/iio/pressure/mprls0025pa_spi.c
> +++ b/drivers/iio/pressure/mprls0025pa_spi.c
> @@ -21,7 +21,7 @@
>  static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
>  {
>  	struct spi_device *spi = to_spi_device(data->dev);
> -	struct spi_transfer xfer;
> +	struct spi_transfer xfer = { };
>  
>  	if (pkt_len > MPR_MEASUREMENT_RD_SIZE)
>  		return -EOVERFLOW;
> 

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Jonathan Cameron]

On Sat, 20 Dec 2025 10:25:19 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello,
> 
> On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:
> > On 12/18, Petre Rodan wrote:  
> > > Zero out input buffer before reading the new conversion.
> > > Perform this operation in core instead of in the bus specific code.
> > > 
> > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > ---
> > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > >  #include <linux/property.h>
> > > +#include <linux/string.h>
> > >  #include <linux/units.h>
> > >  
> > >  #include <linux/gpio/consumer.h>
> > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > >  		}
> > >  	}
> > >  
> > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));  
> > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > subsystem overwrite the rx buffer with what it reads from the device?  
> 
> I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> 
> please tell me how a few byte memset() would be detrimental.

We don't normally do this as old data isn't a potential leak of anything
sensitive.  However in most drivers this only spills out at all as
a result of say a change in configured channels and is normally harmless
as userspace knows to ignore stuff in the gaps anyway.  If there is
another cases here (you mention the busy flag) then add a comment on why
it makes sense. I don't in general want drivers to start doing this as
it is in the fast path and sometimes the memset is non trivial (here it
is probably irrelevant as the buffer is small).

Thanks,

Jonathan

> 
> best regards,
> peter
> 
> > >  	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
> > >  	if (ret < 0)
> > >  		return ret;
> > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > index a0bbc6af9283..0fe8cfe0d7e7 100644
> > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c
> > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> > >  	if (cnt > MPR_MEASUREMENT_RD_SIZE)
> > >  		return -EOVERFLOW;
> > >  
> > > -	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
> > >  	ret = i2c_master_recv(client, data->rx_buf, cnt);
> > >  	if (ret < 0)
> > >  		return ret;  
> 

Re: [PATCH 11/14] iio: pressure: mprls0025pa: fix scan_type struct

[Jonathan Cameron]

On Thu, 18 Dec 2025 13:05:53 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Fix the scan_type sign and realbits assignment.
> 
> The pressure is a 24bit unsigned int between output_min and output_max.
> 
>  transfer function A: 10%   to 90%   of 2^24
>  transfer function B:  2.5% to 22.5% of 2^24

Hmm. So, that's not ever going to use all the bits. It fits in 22 bits?

>  transfer function C: 20%   to 80%   of 2^24
> [MPR_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 }
> [MPR_FUNCTION_B] = { .output_min =  419430, .output_max =  3774874 }
> [MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 }
> 
> Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Where possible drag any fixes as early as possible in the series as they
are more likely to be backported.

Here, I think the impact is constrained to the expected limits userspace
might work out rather than how the actual value of pressure is interpreted?
If so, good to fix but fairly minor bug.

Jonathan

> ---
>  drivers/iio/pressure/mprls0025pa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 9b18d5fb7e42..243a5717b88f 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -165,8 +165,8 @@ static const struct iio_chan_spec mpr_channels[] = {
>  					BIT(IIO_CHAN_INFO_OFFSET),
>  		.scan_index = 0,
>  		.scan_type = {
> -			.sign = 's',
> -			.realbits = 32,
> +			.sign = 'u',
> +			.realbits = 24,
>  			.storagebits = 32,
>  			.endianness = IIO_CPU,
>  		},
> 

Re: [PATCH 12/14] iio: pressure: mprls0025pa: fix interrupt flag

[Jonathan Cameron]

On Thu, 18 Dec 2025 13:05:54 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Interrupt falling/rising flags should only be defined in the device tree.
> 
> Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Gah. We shoudn't have let this slip through in 2023.
Some old drivers have done this for many years and for those we definitely
can't be sure there aren't boards with it set wrong in DT that will see
a regression with this fix.

For a 2023 driver, maybe we can gamble that no one has broken DT (where this
would annoyingly be a regression).  One other question below.


> ---
>  drivers/iio/pressure/mprls0025pa.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 243a5717b88f..fc04988b9437 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -14,6 +14,7 @@
>  #include <linux/bits.h>
>  #include <linux/delay.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/math64.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -404,9 +405,7 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
>  
>  	if (data->irq > 0) {
>  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> -				       IRQF_TRIGGER_RISING,
> -				       dev_name(dev),
> -				       data);
> +				       IRQF_ONESHOT, dev_name(dev), data);
IRQF_ONESHOT addition here needs a separate explanation. Might well be correct
but it's not related to your patch description.

>  		if (ret)
>  			return dev_err_probe(dev, ret,
>  					  "request irq %d failed\n", data->irq);
> 

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 4767 bytes --]


hello Jonathan,

thank you for the review.

On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:
> On Sat, 20 Dec 2025 10:25:19 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Hello,
> > 
> > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:
> > > On 12/18, Petre Rodan wrote:  
> > > > Zero out input buffer before reading the new conversion.
> > > > Perform this operation in core instead of in the bus specific code.
> > > > 
> > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > ---
> > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include <linux/mod_devicetable.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/property.h>
> > > > +#include <linux/string.h>
> > > >  #include <linux/units.h>
> > > >  
> > > >  #include <linux/gpio/consumer.h>
> > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > >  		}
> > > >  	}
> > > >  
> > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));  
> > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > subsystem overwrite the rx buffer with what it reads from the device?  
> > 
> > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > 
> > please tell me how a few byte memset() would be detrimental.
> 
> We don't normally do this as old data isn't a potential leak of anything
> sensitive.

from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.

as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.

the same logic applies to any instrument in a lab setting. 
a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.

getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.

> However in most drivers this only spills out at all as
> a result of say a change in configured channels and is normally harmless
> as userspace knows to ignore stuff in the gaps anyway.  If there is
> another cases here (you mention the busy flag) then add a comment on why
> it makes sense. I don't in general want drivers to start doing this as
> it is in the fast path and sometimes the memset is non trivial (here it
> is probably irrelevant as the buffer is small).
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > best regards,
> > peter
> > 
> > > >  	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > > index a0bbc6af9283..0fe8cfe0d7e7 100644
> > > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c
> > > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> > > >  	if (cnt > MPR_MEASUREMENT_RD_SIZE)
> > > >  		return -EOVERFLOW;
> > > >  
> > > > -	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
> > > >  	ret = i2c_master_recv(client, data->rx_buf, cnt);
> > > >  	if (ret < 0)
> > > >  		return ret;  
> > 
> 

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/14] iio: pressure: mprls0025pa: fix interrupt flag

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]


hello Jonathan,

On Sun, Dec 21, 2025 at 06:38:26PM +0000, Jonathan Cameron wrote:
> On Thu, 18 Dec 2025 13:05:54 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Interrupt falling/rising flags should only be defined in the device tree.
> > 
> > Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Gah. We shoudn't have let this slip through in 2023.
> Some old drivers have done this for many years and for those we definitely
> can't be sure there aren't boards with it set wrong in DT that will see
> a regression with this fix.
> 
> For a 2023 driver, maybe we can gamble that no one has broken DT (where this
> would annoyingly be a regression).  One other question below.
> 
> 
> > ---
> >  drivers/iio/pressure/mprls0025pa.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > index 243a5717b88f..fc04988b9437 100644
> > --- a/drivers/iio/pressure/mprls0025pa.c
> > +++ b/drivers/iio/pressure/mprls0025pa.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/delay.h>
> >  #include <linux/errno.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/math64.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -404,9 +405,7 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
> >  
> >  	if (data->irq > 0) {
> >  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > -				       IRQF_TRIGGER_RISING,
> > -				       dev_name(dev),
> > -				       data);
> > +				       IRQF_ONESHOT, dev_name(dev), data);
> IRQF_ONESHOT addition here needs a separate explanation. Might well be correct
> but it's not related to your patch description.

do you feel like IRQF_TRIGGER_NONE would be a better fit?

I used ONESHOT just because it seemed like a good idea to isolate the sensor while the handler is running (if I understand the documentation correctly).

best regards,
peter

> 
> >  		if (ret)
> >  			return dev_err_probe(dev, ret,
> >  					  "request irq %d failed\n", data->irq);
> > 
> 

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Marcelo Schmitt]

Hi Petre,

On 12/22, Petre Rodan wrote:
> 
> hello Jonathan,
> 
> thank you for the review.
> 
> On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:
> > On Sat, 20 Dec 2025 10:25:19 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> > 
> > > Hello,
> > > 
> > > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:
> > > > On 12/18, Petre Rodan wrote:  
> > > > > Zero out input buffer before reading the new conversion.
> > > > > Perform this operation in core instead of in the bus specific code.
> > > > > 
> > > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > > ---
> > > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > > @@ -16,6 +16,7 @@
> > > > >  #include <linux/mod_devicetable.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/property.h>
> > > > > +#include <linux/string.h>
> > > > >  #include <linux/units.h>
> > > > >  
> > > > >  #include <linux/gpio/consumer.h>
> > > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));  
> > > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > > subsystem overwrite the rx buffer with what it reads from the device?  
> > > 
> > > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > > 
> > > please tell me how a few byte memset() would be detrimental.
> > 
> > We don't normally do this as old data isn't a potential leak of anything
> > sensitive.
> 
> from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
> it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.
> 
> as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.
> 
> the same logic applies to any instrument in a lab setting. 
> a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.
> 
> getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.

I agree with you that old conversions should not be accidentally re-used nor
errors silently be ignored. But, to me, memset the read buffer to zero looks
like we don't trust the underlying I2C and SPI layers. In that case, we should
fix data read in those subsystems (if there is anyhting be fixed there).
Though probably unlikely scenario to happen, how would one trust the sensor
reading in a scenario where the extected measurement would be close to zero.

My suggestion is to look for a way of ensuring the transfer timing requirements
specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
patch 09.

[1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6

> 
> > However in most drivers this only spills out at all as
> > a result of say a change in configured channels and is normally harmless
> > as userspace knows to ignore stuff in the gaps anyway.  If there is
> > another cases here (you mention the busy flag) then add a comment on why
> > it makes sense. I don't in general want drivers to start doing this as
> > it is in the fast path and sometimes the memset is non trivial (here it
> > is probably irrelevant as the buffer is small).
> > 
> > Thanks,
> > 
> > Jonathan

Re: [PATCH 09/14] iio: pressure: mprls0025pa: mitigate SPI CS delay violation

[Marcelo Schmitt]

On 12/20, Petre Rodan wrote:
> 
> Hello Marcelo,
> 
> thank you for the pointers.
> 
> On Sat, Dec 20, 2025 at 01:51:42AM -0300, Marcelo Schmitt wrote:
> > On 12/18, Petre Rodan wrote:
> > > Based on the sensor datasheet in chapter 7.6 SPI timing, Table 20,
> > > during the SPI transfer there is a minimum time interval between the
> > > CS being asserted and the first clock edge (tHDSS).
> > > This minimum interval of 2.5us is being violated if two consecutive SPI
> > > transfers are queued up, at least on my SPI controller (omap2_mcspi) [1]
> > > As you can see in the first package that only contains a NOP the interval
> > > is 0.75us (half a 800kHz clock cycle).
> > > 
> > > This patch mitigates the problem by implementing a different measurement
> > > technique that does not involve checking for the EOC indicator before
> > > reading the conversion, thus making sure SPI transfers are not queued up.
> > The correct way of fixing that is with protocol specific delay. Generic delays
> > like fsleep won't guarantee any delay between CS drop and fist SCLK edge.
> 
> sorry for the lack of detail in the commit log, I forgot to add it.
> 
> this patch changes the measurement sequence and as a consequence it also mitigates a problem where protocol-specific delays are not implemented in the SPI controller (for the corner case when multiple SPI transfers are queued up).
> 
> as per the datasheet pages 36 and 41, the measurement sequence can follow one of these two scenarios, for both bus implementations:
> 
> a) send measurement command and wait for the status byte to lose the busy flag.
> this means that we would need to keep sending NOP commands and read the first (status) byte in a loop until the busy flag goes away. this is how the driver was designed before this series of patches.
> 
> b) send measurement command and wait for 5ms for the data conversion to occur.
> same as a, but instead of polling the sensor for the status byte there's just a 5ms+ delay after which only one NOP command is sent.
> the read that follows will get the conversion data plus the status byte.
> depending on the status byte flags, either error out or pass the conversion along.
> 
> my patch replaces the measurement sequence defined by scenario a with scenario b and it only affects users that did not define the EoC interrupt pin in the device tree overlay.
> the datasheet contains sample code (pages 38 and 43) and they also happen to use scenario b, so no polling.
> in my testing the sensor always performed as expected (the busy flag was never set after the 5ms fsleep()).
> 
> > For SPI, we unfortunately we don't have any interface to set a pre-SCLK delay
> > so I suggest to make an spi_message with a dummy transfer to cause the initial
> > 2.5 µs delay. E.g.
> 
> for normal (non-queued) SPI messages the delay that exist between the CS assert and the first clock is ~4us, well above the 2.5us minimum.
> the problem only appears when the SPI controller gets multiple xfers without any delays between them - which happens in scenario a when two transfers get queued up (NOP that reads the status byte and the NOP that reads the status + conversions).

Sure, as you say, the probable reason to have enough first SCLK delay was
because the SPI controller was not initially dealing with many transfers.
Though, can't the MPR read transfer lack enough first SCLK delay if a different
device on the same bus bursts SPI transfers during MPR device read?
fsleep() may fix it for single-device use case, but I don't think it is the
proper way of getting transfer timings correctly. To ensure that first SCLK
delay, you got to use protocol specific delays so that the controller will
be able to properly set that delay.

> 
> this is just a hunch but I think that the CS timing is directly controlled by hardware when the issue happens.
> see chapter 24.3.2.8 page 4903 in the AM335x technical reference manual [1].
> I'm measuring exactly 0.5 clock cycles CS lead times which is the default for the TCS bit of the register MCSPI_CH(i)CONF.
> 
> [1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf?ts=1766215152875
> 
> > /* dummy transfer with no data, just cause the delay */
> > xfers[0].delay.value = 2500 * NSEC_PER_SEC;
> > xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > 
> > /* normal data transfer  */
> > xfer[1].tx_buf = data->tx_buf;
> > xfer[1].rx_buf = data->rx_buf;
> > ...

See spi.h for documentation of the delays available for SPI transfers. As I
mentioned, we unfortunately don't have a first SCLK delay for SPI but I think
you may get something equivalent with a dummy transfer like the example above.

> > 
> > Also, I don't see how the proposed change is 'implementing a different
> > measurement technique'. Consider updating the commit description or providing
> > better explanation.
> 
> I feel that using scenario b defined above we cure the problem before it happens.
> my patch provides a less complex implementation, it's recommended by the manufacturer in their sample code and provides less code duplication (status byte parsing) with no downsides afaict.
> 
> > > see Option 2 in Table 19 SPI output measurement command.
> > > Note that Honeywell's example code also follows this technique for both i2c
> > > and SPI.
> > > 
> > > This change only affects users that do not define the EOC interrupt in the
> > > device tree.
> > > 
> > > Remove defines that are no longer used.
> > > 
> > > [1] https://pasteboard.co/66WN38MRI1wc.png
> > > 
> > > Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
> > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> 
> best regards,
> peter
> 
> -- 
> petre rodan

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Jonathan Cameron]

On Mon, 22 Dec 2025 11:06:07 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hi Petre,
> 
> On 12/22, Petre Rodan wrote:
> > 
> > hello Jonathan,
> > 
> > thank you for the review.
> > 
> > On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:  
> > > On Sat, 20 Dec 2025 10:25:19 +0200
> > > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> > >   
> > > > Hello,
> > > > 
> > > > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:  
> > > > > On 12/18, Petre Rodan wrote:    
> > > > > > Zero out input buffer before reading the new conversion.
> > > > > > Perform this operation in core instead of in the bus specific code.
> > > > > > 
> > > > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > > > ---
> > > > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > >  #include <linux/mod_devicetable.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/property.h>
> > > > > > +#include <linux/string.h>
> > > > > >  #include <linux/units.h>
> > > > > >  
> > > > > >  #include <linux/gpio/consumer.h>
> > > > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > > > >  		}
> > > > > >  	}
> > > > > >  
> > > > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));    
> > > > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > > > subsystem overwrite the rx buffer with what it reads from the device?    
> > > > 
> > > > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > > > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > > > 
> > > > please tell me how a few byte memset() would be detrimental.  
> > > 
> > > We don't normally do this as old data isn't a potential leak of anything
> > > sensitive.  
> > 
> > from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
> > it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.
> > 
> > as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.
> > 
> > the same logic applies to any instrument in a lab setting. 
> > a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.
> > 
> > getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.  
> 
> I agree with you that old conversions should not be accidentally re-used nor
> errors silently be ignored. But, to me, memset the read buffer to zero looks
> like we don't trust the underlying I2C and SPI layers. In that case, we should
> fix data read in those subsystems (if there is anyhting be fixed there).
> Though probably unlikely scenario to happen, how would one trust the sensor
> reading in a scenario where the extected measurement would be close to zero.

There are two different ways to get stale data in these buffers.
1) read fail. Those we absolutely should trust to be handled correctly by lower layer
   with errors reported and the data therefore not used.  Whether it is stale or random
   garbage doesn't really matter.  0 is often a perfectly valid reading so no extra
   info from seeing that in the buffer.
2) Changes in configuration that move the holes around or create some.  This is the
   more interesting corner.  I don't think the stale data argument applies because
   any software reading the data in the holes and doing anything with it is inherently
   buggy (plus as above, 0 is almost always a valid reading!)  Some drivers will
   even read other data into those holes (checksums etc are common).

So I don't see either as being a particularly problem here.  I don't mind the buffer
being cleared on each read but it doesn't to me seem necessary for any correctness
or security related reasons which are the two cases we need to make sure we clear
data for.

Jonathan

> 
> My suggestion is to look for a way of ensuring the transfer timing requirements
> specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
> patch 09.
> 
> [1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6
> 
> >   
> > > However in most drivers this only spills out at all as
> > > a result of say a change in configured channels and is normally harmless
> > > as userspace knows to ignore stuff in the gaps anyway.  If there is
> > > another cases here (you mention the busy flag) then add a comment on why
> > > it makes sense. I don't in general want drivers to start doing this as
> > > it is in the fast path and sometimes the memset is non trivial (here it
> > > is probably irrelevant as the buffer is small).
> > > 
> > > Thanks,
> > > 
> > > Jonathan  

Re: [PATCH 12/14] iio: pressure: mprls0025pa: fix interrupt flag

[Jonathan Cameron]

On Mon, 22 Dec 2025 09:22:06 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> hello Jonathan,
> 
> On Sun, Dec 21, 2025 at 06:38:26PM +0000, Jonathan Cameron wrote:
> > On Thu, 18 Dec 2025 13:05:54 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> >   
> > > Interrupt falling/rising flags should only be defined in the device tree.
> > > 
> > > Fixes: 713337d9143e ("iio: pressure: Honeywell mprls0025pa pressure sensor")
> > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>  
> > Gah. We shoudn't have let this slip through in 2023.
> > Some old drivers have done this for many years and for those we definitely
> > can't be sure there aren't boards with it set wrong in DT that will see
> > a regression with this fix.
> > 
> > For a 2023 driver, maybe we can gamble that no one has broken DT (where this
> > would annoyingly be a regression).  One other question below.
> > 
> >   
> > > ---
> > >  drivers/iio/pressure/mprls0025pa.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > index 243a5717b88f..fc04988b9437 100644
> > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/bits.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/errno.h>
> > > +#include <linux/interrupt.h>
> > >  #include <linux/math64.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > > @@ -404,9 +405,7 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
> > >  
> > >  	if (data->irq > 0) {
> > >  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > > -				       IRQF_TRIGGER_RISING,
> > > -				       dev_name(dev),
> > > -				       data);
> > > +				       IRQF_ONESHOT, dev_name(dev), data);  
> > IRQF_ONESHOT addition here needs a separate explanation. Might well be correct
> > but it's not related to your patch description.  
> 
> do you feel like IRQF_TRIGGER_NONE would be a better fit?
That would be the minimal change.
> 
> I used ONESHOT just because it seemed like a good idea to isolate the sensor while the handler is running (if I understand the documentation correctly).
It's normally only necessary for devices where we might get
more interrupts signalled before we have performed all the operations
we need to for the one we are already handling.  E.g. level interrupts
with a threaded handler, or IIRC any freerunning pulse type interrupt.

Jonathan

> 
> best regards,
> peter
> 
> >   
> > >  		if (ret)
> > >  			return dev_err_probe(dev, ret,
> > >  					  "request irq %d failed\n", data->irq);
> > >   
> >   
> 

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 8038 bytes --]


Hello and a Happy New Year!

On Sat, Dec 27, 2025 at 02:31:49PM +0000, Jonathan Cameron wrote:
> On Mon, 22 Dec 2025 11:06:07 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> 
> > Hi Petre,
> > 
> > On 12/22, Petre Rodan wrote:
> > > 
> > > hello Jonathan,
> > > 
> > > thank you for the review.
> > > 
> > > On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:  
> > > > On Sat, 20 Dec 2025 10:25:19 +0200
> > > > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> > > >   
> > > > > Hello,
> > > > > 
> > > > > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:  
> > > > > > On 12/18, Petre Rodan wrote:    
> > > > > > > Zero out input buffer before reading the new conversion.
> > > > > > > Perform this operation in core instead of in the bus specific code.
> > > > > > > 
> > > > > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > > > > ---
> > > > > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > > > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > >  #include <linux/mod_devicetable.h>
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/property.h>
> > > > > > > +#include <linux/string.h>
> > > > > > >  #include <linux/units.h>
> > > > > > >  
> > > > > > >  #include <linux/gpio/consumer.h>
> > > > > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > > > > >  		}
> > > > > > >  	}
> > > > > > >  
> > > > > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));    
> > > > > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > > > > subsystem overwrite the rx buffer with what it reads from the device?    
> > > > > 
> > > > > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > > > > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > > > > 
> > > > > please tell me how a few byte memset() would be detrimental.  
> > > > 
> > > > We don't normally do this as old data isn't a potential leak of anything
> > > > sensitive.  
> > > 
> > > from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
> > > it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.
> > > 
> > > as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.
> > > 
> > > the same logic applies to any instrument in a lab setting. 
> > > a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.
> > > 
> > > getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.  
> > 
> > I agree with you that old conversions should not be accidentally re-used nor
> > errors silently be ignored. But, to me, memset the read buffer to zero looks
> > like we don't trust the underlying I2C and SPI layers. In that case, we should
> > fix data read in those subsystems (if there is anyhting be fixed there).
> > Though probably unlikely scenario to happen, how would one trust the sensor
> > reading in a scenario where the extected measurement would be close to zero.
> 
> There are two different ways to get stale data in these buffers.
> 1) read fail. Those we absolutely should trust to be handled correctly by lower layer
>    with errors reported and the data therefore not used.  Whether it is stale or random
>    garbage doesn't really matter.  0 is often a perfectly valid reading so no extra
>    info from seeing that in the buffer.

for this sensor a raw conversion of 0 is out of bounds. valid values are between
 output_min and output_max:

	[MPR_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
	[MPR_FUNCTION_B] = { .output_min =  419430, .output_max =  3774874 },
	[MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 },

I expect that the user will recognize such an abnormal reading.

for the SPI transfer specifically there is a latch-up scenario in which the MOSI
signal is being clamped down by the sensor in sync with SCLK [1].
I do not know how all SPI controllers act when MOSI is forced to GND, thus
 provided the memset() for a hypothetical scenario in which the SPI controller
 just resets during the xfer (due to a brownout) and if this condition is not
  recognized by the low level spi layer (I see no way to test this).

[1] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1588325/am3358-spi-tx-data-corruption
(my guess is that ABP2 and MPR series of sensors share the same silicon implementation)

> 2) Changes in configuration that move the holes around or create some.  This is the
>    more interesting corner.  I don't think the stale data argument applies because
>    any software reading the data in the holes and doing anything with it is inherently
>    buggy (plus as above, 0 is almost always a valid reading!)  Some drivers will
>    even read other data into those holes (checksums etc are common).
> 
> So I don't see either as being a particularly problem here.  I don't mind the buffer
> being cleared on each read but it doesn't to me seem necessary for any correctness
> or security related reasons which are the two cases we need to make sure we clear
> data for.

clamping an output signal to GND smells to me like 'undefined behaviour' when
one looks at a large number of SPI controller implementations and this memset
would provide an early warning, at no cost.

> Jonathan
> 
> > 
> > My suggestion is to look for a way of ensuring the transfer timing requirements
> > specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
> > patch 09.
> > 
> > [1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6

yes, thank you Marcelo. your delay xfer idea works and it will be part of V2.
but that issue is completely unrelated to the memset modification.

best regards,
peter

> > > > However in most drivers this only spills out at all as
> > > > a result of say a change in configured channels and is normally harmless
> > > > as userspace knows to ignore stuff in the gaps anyway.  If there is
> > > > another cases here (you mention the busy flag) then add a comment on why
> > > > it makes sense. I don't in general want drivers to start doing this as
> > > > it is in the fast path and sometimes the memset is non trivial (here it
> > > > is probably irrelevant as the buffer is small).
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan  
> 

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before reading new data

[Jonathan Cameron]

On Sat, 3 Jan 2026 10:00:19 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello and a Happy New Year!
> 
> On Sat, Dec 27, 2025 at 02:31:49PM +0000, Jonathan Cameron wrote:
> > On Mon, 22 Dec 2025 11:06:07 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >   
> > > Hi Petre,
> > > 
> > > On 12/22, Petre Rodan wrote:  
> > > > 
> > > > hello Jonathan,
> > > > 
> > > > thank you for the review.
> > > > 
> > > > On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:    
> > > > > On Sat, 20 Dec 2025 10:25:19 +0200
> > > > > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> > > > >     
> > > > > > Hello,
> > > > > > 
> > > > > > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:    
> > > > > > > On 12/18, Petre Rodan wrote:      
> > > > > > > > Zero out input buffer before reading the new conversion.
> > > > > > > > Perform this operation in core instead of in the bus specific code.
> > > > > > > > 
> > > > > > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > > > > > ---
> > > > > > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > > > > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > > > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > > > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > > > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > > @@ -16,6 +16,7 @@
> > > > > > > >  #include <linux/mod_devicetable.h>
> > > > > > > >  #include <linux/module.h>
> > > > > > > >  #include <linux/property.h>
> > > > > > > > +#include <linux/string.h>
> > > > > > > >  #include <linux/units.h>
> > > > > > > >  
> > > > > > > >  #include <linux/gpio/consumer.h>
> > > > > > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > > > > > >  		}
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));      
> > > > > > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > > > > > subsystem overwrite the rx buffer with what it reads from the device?      
> > > > > > 
> > > > > > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > > > > > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > > > > > 
> > > > > > please tell me how a few byte memset() would be detrimental.    
> > > > > 
> > > > > We don't normally do this as old data isn't a potential leak of anything
> > > > > sensitive.    
> > > > 
> > > > from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
> > > > it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.
> > > > 
> > > > as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.
> > > > 
> > > > the same logic applies to any instrument in a lab setting. 
> > > > a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.
> > > > 
> > > > getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.    
> > > 
> > > I agree with you that old conversions should not be accidentally re-used nor
> > > errors silently be ignored. But, to me, memset the read buffer to zero looks
> > > like we don't trust the underlying I2C and SPI layers. In that case, we should
> > > fix data read in those subsystems (if there is anyhting be fixed there).
> > > Though probably unlikely scenario to happen, how would one trust the sensor
> > > reading in a scenario where the extected measurement would be close to zero.  
> > 
> > There are two different ways to get stale data in these buffers.
> > 1) read fail. Those we absolutely should trust to be handled correctly by lower layer
> >    with errors reported and the data therefore not used.  Whether it is stale or random
> >    garbage doesn't really matter.  0 is often a perfectly valid reading so no extra
> >    info from seeing that in the buffer.  
> 
> for this sensor a raw conversion of 0 is out of bounds. valid values are between
>  output_min and output_max:
> 
> 	[MPR_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
> 	[MPR_FUNCTION_B] = { .output_min =  419430, .output_max =  3774874 },
> 	[MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 },
> 
> I expect that the user will recognize such an abnormal reading.
> 
> for the SPI transfer specifically there is a latch-up scenario in which the MOSI
> signal is being clamped down by the sensor in sync with SCLK [1].
> I do not know how all SPI controllers act when MOSI is forced to GND, thus
>  provided the memset() for a hypothetical scenario in which the SPI controller
>  just resets during the xfer (due to a brownout) and if this condition is not
>   recognized by the low level spi layer (I see no way to test this).
> 
> [1] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1588325/am3358-spi-tx-data-corruption
> (my guess is that ABP2 and MPR series of sensors share the same silicon implementation)
> 
> > 2) Changes in configuration that move the holes around or create some.  This is the
> >    more interesting corner.  I don't think the stale data argument applies because
> >    any software reading the data in the holes and doing anything with it is inherently
> >    buggy (plus as above, 0 is almost always a valid reading!)  Some drivers will
> >    even read other data into those holes (checksums etc are common).
> > 
> > So I don't see either as being a particularly problem here.  I don't mind the buffer
> > being cleared on each read but it doesn't to me seem necessary for any correctness
> > or security related reasons which are the two cases we need to make sure we clear
> > data for.  
> 
> clamping an output signal to GND smells to me like 'undefined behaviour' when
> one looks at a large number of SPI controller implementations and this memset
> would provide an early warning, at no cost.

Ok. Add a comment on 0 not being a valid value for these channels and the
fact that it might be useful for diagnosing the condition you describe.

All I really want to do is avoid people seeing a 'memset is needed here' patch
and posting them for other drivers where it doesn't provide anything useful.

Jonathan

> 
> > Jonathan
> >   
> > > 
> > > My suggestion is to look for a way of ensuring the transfer timing requirements
> > > specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
> > > patch 09.
> > > 
> > > [1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6  
> 
> yes, thank you Marcelo. your delay xfer idea works and it will be part of V2.
> but that issue is completely unrelated to the memset modification.
> 
> best regards,
> peter
> 
> > > > > However in most drivers this only spills out at all as
> > > > > a result of say a change in configured channels and is normally harmless
> > > > > as userspace knows to ignore stuff in the gaps anyway.  If there is
> > > > > another cases here (you mention the busy flag) then add a comment on why
> > > > > it makes sense. I don't in general want drivers to start doing this as
> > > > > it is in the fast path and sometimes the memset is non trivial (here it
> > > > > is probably irrelevant as the buffer is small).
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Jonathan    
> >   
>