The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows
the sensor to do consecutive measurements depending on the ODR rate value.

The trigger pin can be configured to be open-drain or push-pull and either
rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range
values.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c   | 309 ++++++++++++++++++++++++++-
 drivers/iio/pressure/bmp280-regmap.c |   2 +-
 drivers/iio/pressure/bmp280.h        |  23 +-
 3 files changed, 328 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 4a8d2ed4a9c4..4238f37b7805 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -37,12 +37,14 @@
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/random.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
@@ -1761,6 +1763,148 @@ static int bmp380_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+static void bmp380_trigger_reenable(struct iio_trigger *trig)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &tmp);
+	if (ret)
+		dev_err(data->dev, "Failed to reset interrupt\n");
+}
+
+static int bmp380_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL,
+				 BMP380_INT_CTRL_DRDY_EN,
+				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
+					    state ? 1 : 0));
+	if (ret) {
+		dev_err(data->dev, "Could not enable/disable interrupt\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops bmp380_trigger_ops = {
+	.set_trigger_state = &bmp380_data_rdy_trigger_set_state,
+	.reenable = &bmp380_trigger_reenable,
+};
+
+static int bmp380_int_config(struct bmp280_data *data)
+{
+	int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
+				      data->trig_open_drain) |
+			   FIELD_PREP(BMP380_INT_CTRL_LEVEL,
+				      data->trig_active_high);
+
+	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL,
+				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt settings\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static irqreturn_t bmp380_irq_thread_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct bmp280_data *data = iio_priv(indio_dev);
+	unsigned int int_ctrl;
+	int ret;
+
+	scoped_guard(mutex, &data->lock) {
+		ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl);
+		if (ret)
+			return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
+		iio_trigger_poll_nested(data->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bmp380_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;
+	struct irq_data *desc;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get(fwnode, 0);
+	if (!irq) {
+		dev_err(data->dev, "No interrupt found\n");
+		return -ENODEV;
+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp380_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp380_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_threaded_irq(data->dev, irq, NULL,
+					bmp380_irq_thread_handler, IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
+
 static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1854,6 +1998,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.wait_conv = bmp380_wait_conv,
 	.preinit = bmp380_preinit,
 
+	.trigger_probe = bmp380_trigger_probe,
 	.trigger_handler = bmp380_trigger_handler,
 };
 EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
@@ -2390,6 +2535,154 @@ static int bmp580_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+static void bmp580_trigger_reenable(struct iio_trigger *trig)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp);
+	if (ret)
+		dev_err(data->dev, "Failed to reset interrupt\n");
+}
+
+static int bmp580_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
+				 BMP580_INT_CONFIG_INT_EN,
+				 FIELD_PREP(BMP580_INT_CONFIG_INT_EN,
+					    state ? 1 : 0));
+	if (ret) {
+		dev_err(data->dev, "Could not enable/disable interrupt\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops bmp580_trigger_ops = {
+	.set_trigger_state = &bmp580_data_rdy_trigger_set_state,
+	.reenable = &bmp580_trigger_reenable,
+};
+
+static int bmp580_int_config(struct bmp280_data *data)
+{
+	int ret, int_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN,
+				      data->trig_open_drain) |
+			   FIELD_PREP(BMP580_INT_CONFIG_LEVEL,
+				      data->trig_active_high);
+
+	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
+				 BMP580_INT_CONFIG_MASK, int_cfg);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt settings\n");
+		return ret;
+	}
+
+	ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE,
+			      BMP580_INT_SOURCE_DRDY);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt source\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct bmp280_data *data = iio_priv(indio_dev);
+	unsigned int int_ctrl;
+	int ret;
+
+	scoped_guard(mutex, &data->lock) {
+	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
+	if (ret)
+		return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
+		iio_trigger_poll_nested(data->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bmp580_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;
+	struct irq_data *desc;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get(fwnode, 0);
+	if (!irq) {
+		dev_err(data->dev, "No interrupt found\n");
+		return -ENODEV;
+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp580_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp580_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_threaded_irq(data->dev, irq, NULL,
+					bmp580_irq_thread_handler, IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
 static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -2466,6 +2759,7 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.wait_conv = bmp580_wait_conv,
 	.preinit = bmp580_preinit,
 
+	.trigger_probe = bmp580_trigger_probe,
 	.trigger_handler = bmp580_trigger_handler,
 };
 EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280);
@@ -3013,10 +3307,17 @@ int bmp280_common_probe(struct device *dev,
 	 * however as it happens, the BMP085 shares the chip ID of BMP180
 	 * so we look for an IRQ if we have that.
 	 */
-	if (irq > 0 && (chip_id  == BMP180_CHIP_ID)) {
-		ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
-		if (ret)
-			return ret;
+	if (irq > 0) {
+		if (chip_id == BMP180_CHIP_ID) {
+			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
+			if (ret)
+				return ret;
+		}
+		if (data->chip_info->trigger_probe) {
+			ret = data->chip_info->trigger_probe(indio_dev);
+			if (ret)
+				return ret;
+		}
 	}
 
 	ret = data->chip_info->set_mode(data, BMP280_SLEEP);
diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
index d27d68edd906..cccdf8fc6c09 100644
--- a/drivers/iio/pressure/bmp280-regmap.c
+++ b/drivers/iio/pressure/bmp280-regmap.c
@@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
 	case BMP380_REG_FIFO_WATERMARK_LSB:
 	case BMP380_REG_FIFO_WATERMARK_MSB:
 	case BMP380_REG_POWER_CONTROL:
-	case BMP380_REG_INT_CONTROL:
+	case BMP380_REG_INT_CTRL:
 	case BMP380_REG_IF_CONFIG:
 	case BMP380_REG_ODR:
 	case BMP380_REG_OSR:
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index f5d192509d61..754eda367941 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -55,8 +55,17 @@
 #define BMP580_CMD_NVM_WRITE_SEQ_1	0xA0
 #define BMP580_CMD_SOFT_RESET		0xB6
 
+#define BMP580_INT_STATUS_DRDY_MASK	BIT(0)
 #define BMP580_INT_STATUS_POR_MASK	BIT(4)
 
+#define BMP580_INT_SOURCE_DRDY		BIT(0)
+
+#define BMP580_INT_CONFIG_MASK		GENMASK(3, 0)
+#define BMP580_INT_CONFIG_LATCH		BIT(0)
+#define BMP580_INT_CONFIG_LEVEL		BIT(1)
+#define BMP580_INT_CONFIG_OPEN_DRAIN	BIT(2)
+#define BMP580_INT_CONFIG_INT_EN	BIT(3)
+
 #define BMP580_STATUS_CORE_RDY_MASK	BIT(0)
 #define BMP580_STATUS_NVM_RDY_MASK	BIT(1)
 #define BMP580_STATUS_NVM_ERR_MASK	BIT(2)
@@ -117,7 +126,7 @@
 #define BMP380_REG_OSR			0x1C
 #define BMP380_REG_POWER_CONTROL	0x1B
 #define BMP380_REG_IF_CONFIG		0x1A
-#define BMP380_REG_INT_CONTROL		0x19
+#define BMP380_REG_INT_CTRL		0x19
 #define BMP380_REG_INT_STATUS		0x11
 #define BMP380_REG_EVENT		0x10
 #define BMP380_REG_STATUS		0x03
@@ -175,6 +184,14 @@
 #define BMP380_TEMP_MEAS_OFFSET		163
 #define BMP380_PRESS_MEAS_OFFSET	392
 
+#define BMP380_INT_STATUS_DRDY		BIT(3)
+
+#define BMP380_INT_CTRL_SETTINGS_MASK	GENMASK(2, 0)
+#define BMP380_INT_CTRL_OPEN_DRAIN	BIT(0)
+#define BMP380_INT_CTRL_LEVEL		BIT(1)
+#define BMP380_INT_CTRL_LATCH		BIT(2)
+#define BMP380_INT_CTRL_DRDY_EN		BIT(6)
+
 #define BMP380_MIN_TEMP			-4000
 #define BMP380_MAX_TEMP			8500
 #define BMP380_MIN_PRES			3000000
@@ -406,6 +423,9 @@ struct bmp280_data {
 	struct regmap *regmap;
 	struct completion done;
 	bool use_eoc;
+	bool trig_open_drain;
+	bool trig_active_high;
+	struct iio_trigger *trig;
 	const struct bmp280_chip_info *chip_info;
 	union {
 		struct bmp180_calib bmp180;
@@ -504,6 +524,7 @@ struct bmp280_chip_info {
 	int (*set_mode)(struct bmp280_data *data, enum bmp280_op_mode mode);
 	int (*wait_conv)(struct bmp280_data *data);
 
+	int (*trigger_probe)(struct iio_dev *indio_dev);
 	irqreturn_t (*trigger_handler)(int irq, void *p);
 };
 
-- 
2.25.1

On Fri, 26 Jul 2024 01:10:38 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> a trigger for when there are data ready in the sensor for pick up.
> 
> This use case is used along with NORMAL_MODE in the sensor, which allows
> the sensor to do consecutive measurements depending on the ODR rate value.
> 
> The trigger pin can be configured to be open-drain or push-pull and either
> rising or falling edge.
> 
> No support is added yet for interrupts for FIFO, WATERMARK and out of range
> values.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Hi Vasileios,

A few minor things inline, including a suggestion that perhaps the trigger_probe()
functions can be combined to reduce duplication. That would use a
__bmp280_trigger_probe(struct iio_dev *, struct iio_trigger_ops *,
                       + some function pointers).

Perhaps it's not worth it - I didn't try writing the actual code!

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c   | 309 ++++++++++++++++++++++++++-
>  drivers/iio/pressure/bmp280-regmap.c |   2 +-
>  drivers/iio/pressure/bmp280.h        |  23 +-
>  3 files changed, 328 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 4a8d2ed4a9c4..4238f37b7805 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -37,12 +37,14 @@



> +static irqreturn_t bmp380_irq_thread_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	unsigned int int_ctrl;
> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {
> +		ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl);
> +		if (ret)
> +			return IRQ_NONE;
> +	}
> +
> +	if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
> +		iio_trigger_poll_nested(data->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bmp380_trigger_probe(struct iio_dev *indio_dev)

Two of these functions are very similar.  Perhaps define a common
function that takes a function call for int config, the ops, and
interrupt handler as arguments then add device specific
calls that use that.



> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	struct fwnode_handle *fwnode;
> +	int ret, irq, irq_type;
> +	struct irq_data *desc;
> +
> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;
> +
> +	irq = fwnode_irq_get(fwnode, 0);
> +	if (!irq) {
> +		dev_err(data->dev, "No interrupt found\n");
> +		return -ENODEV;
> +	}
> +
> +	desc = irq_get_irq_data(irq);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	irq_type = irqd_get_trigger_type(desc);
> +	switch (irq_type) {
> +	case IRQF_TRIGGER_RISING:
> +		data->trig_active_high = true;
> +		break;
> +	case IRQF_TRIGGER_FALLING:
> +		data->trig_active_high = false;
> +		break;
> +	default:
> +		dev_err(data->dev, "Invalid interrupt type specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> +							  "int-open-drain");
> +
> +	ret = bmp380_int_config(data);
> +	if (ret)
> +		return ret;
> +
> +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> +					    indio_dev->name,
> +					    iio_device_id(indio_dev));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &bmp380_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, data);
> +
> +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> +					bmp380_irq_thread_handler, IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret) {
> +		dev_err(data->dev, "request irq failed\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_trigger_register(data->dev, data->trig);
> +	if (ret) {
> +		dev_err(data->dev, "iio trigger register failed\n");
> +		return ret;
> +	}
> +
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	return 0;
> +}
> +
> +

one blank line only.

>  static irqreturn_t bmp380_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -1854,6 +1998,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
>  	.wait_conv = bmp380_wait_conv,
>  	.preinit = bmp380_preinit,
>  
> +	.trigger_probe = bmp380_trigger_probe,
>  	.trigger_handler = bmp380_trigger_handler,
>  };
>  EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
> @@ -2390,6 +2535,154 @@ static int bmp580_chip_config(struct bmp280_data *data)
>  	return 0;
>  }
>

...

> +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	unsigned int int_ctrl;
> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {

Indent wrong.

> +	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
> +	if (ret)
> +		return IRQ_NONE;
> +	}
> +
> +	if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
> +		iio_trigger_poll_nested(data->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> +{
...

> +
> +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> +					    indio_dev->name,
> +					    iio_device_id(indio_dev));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &bmp580_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, data);
> +
> +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> +					bmp580_irq_thread_handler, IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret) {
> +		dev_err(data->dev, "request irq failed\n");

Only in probe paths I think, so return dev_err_probe() thoughout these
trigger setup callbacks.



> +}

> diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
> index d27d68edd906..cccdf8fc6c09 100644
> --- a/drivers/iio/pressure/bmp280-regmap.c
> +++ b/drivers/iio/pressure/bmp280-regmap.c
> @@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
>  	case BMP380_REG_FIFO_WATERMARK_LSB:
>  	case BMP380_REG_FIFO_WATERMARK_MSB:
>  	case BMP380_REG_POWER_CONTROL:
> -	case BMP380_REG_INT_CONTROL:
> +	case BMP380_REG_INT_CTRL:

Unrelated change.  I'm also not sure it's worth making.

>  	case BMP380_REG_IF_CONFIG:
>  	case BMP380_REG_ODR:
>  	case BMP380_REG_OSR:
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index f5d192509d61..754eda367941 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -55,8 +55,17 @@
>  #define BMP580_CMD_NVM_WRITE_SEQ_1	0xA0
>  #define BMP580_CMD_SOFT_RESET		0xB6
>  
> +#define BMP580_INT_STATUS_DRDY_MASK	BIT(0)
>  #define BMP580_INT_STATUS_POR_MASK	BIT(4)
>  
> +#define BMP580_INT_SOURCE_DRDY		BIT(0)
> +
> +#define BMP580_INT_CONFIG_MASK		GENMASK(3, 0)
> +#define BMP580_INT_CONFIG_LATCH		BIT(0)
> +#define BMP580_INT_CONFIG_LEVEL		BIT(1)
> +#define BMP580_INT_CONFIG_OPEN_DRAIN	BIT(2)
> +#define BMP580_INT_CONFIG_INT_EN	BIT(3)
> +
>  #define BMP580_STATUS_CORE_RDY_MASK	BIT(0)
>  #define BMP580_STATUS_NVM_RDY_MASK	BIT(1)
>  #define BMP580_STATUS_NVM_ERR_MASK	BIT(2)
> @@ -117,7 +126,7 @@
>  #define BMP380_REG_OSR			0x1C
>  #define BMP380_REG_POWER_CONTROL	0x1B
>  #define BMP380_REG_IF_CONFIG		0x1A
> -#define BMP380_REG_INT_CONTROL		0x19
> +#define BMP380_REG_INT_CTRL		0x19
As above.

Jonathan

On Sun, Jul 28, 2024 at 05:06:50PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2024 01:10:38 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> > a trigger for when there are data ready in the sensor for pick up.
> > 
> > This use case is used along with NORMAL_MODE in the sensor, which allows
> > the sensor to do consecutive measurements depending on the ODR rate value.
> > 
> > The trigger pin can be configured to be open-drain or push-pull and either
> > rising or falling edge.
> > 
> > No support is added yet for interrupts for FIFO, WATERMARK and out of range
> > values.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> Hi Vasileios,
> 
> A few minor things inline, including a suggestion that perhaps the trigger_probe()
> functions can be combined to reduce duplication. That would use a
> __bmp280_trigger_probe(struct iio_dev *, struct iio_trigger_ops *,
>                        + some function pointers).
> 
> Perhaps it's not worth it - I didn't try writing the actual code!
> 
> Jonathan
> 

Hi Jonathan,

Initially, in the v1 of the series, I had designed differently the handling of the
interrupts, and having split functions was making more sense in case you had
sensors with extra interrupts.

Since you explained to me how to do it properly, I think that now, what you are
proposing makes sense.

> > ---
> >  drivers/iio/pressure/bmp280-core.c   | 309 ++++++++++++++++++++++++++-
> >  drivers/iio/pressure/bmp280-regmap.c |   2 +-
> >  drivers/iio/pressure/bmp280.h        |  23 +-
> >  3 files changed, 328 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 4a8d2ed4a9c4..4238f37b7805 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -37,12 +37,14 @@
> 
> 
> 
> > +static irqreturn_t bmp380_irq_thread_handler(int irq, void *p)
> > +{
> > +	struct iio_dev *indio_dev = p;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	unsigned int int_ctrl;
> > +	int ret;
> > +
> > +	scoped_guard(mutex, &data->lock) {
> > +		ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl);
> > +		if (ret)
> > +			return IRQ_NONE;
> > +	}
> > +
> > +	if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
> > +		iio_trigger_poll_nested(data->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int bmp380_trigger_probe(struct iio_dev *indio_dev)
> 
> Two of these functions are very similar.  Perhaps define a common
> function that takes a function call for int config, the ops, and
> interrupt handler as arguments then add device specific
> calls that use that.
> 

True, I will think if it could generate any issues in the future with newer
sensors adding more interrupts or with the current ones (bmp580 supports much
more interrupts than the bmp380) and I will act accordingly.

> 
> 
> > +{
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	struct fwnode_handle *fwnode;
> > +	int ret, irq, irq_type;
> > +	struct irq_data *desc;
> > +
> > +	fwnode = dev_fwnode(data->dev);
> > +	if (!fwnode)
> > +		return -ENODEV;
> > +
> > +	irq = fwnode_irq_get(fwnode, 0);
> > +	if (!irq) {
> > +		dev_err(data->dev, "No interrupt found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	desc = irq_get_irq_data(irq);
> > +	if (!desc)
> > +		return -EINVAL;
> > +
> > +	irq_type = irqd_get_trigger_type(desc);
> > +	switch (irq_type) {
> > +	case IRQF_TRIGGER_RISING:
> > +		data->trig_active_high = true;
> > +		break;
> > +	case IRQF_TRIGGER_FALLING:
> > +		data->trig_active_high = false;
> > +		break;
> > +	default:
> > +		dev_err(data->dev, "Invalid interrupt type specified\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> > +							  "int-open-drain");
> > +
> > +	ret = bmp380_int_config(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> > +					    indio_dev->name,
> > +					    iio_device_id(indio_dev));
> > +	if (!data->trig)
> > +		return -ENOMEM;
> > +
> > +	data->trig->ops = &bmp380_trigger_ops;
> > +	iio_trigger_set_drvdata(data->trig, data);
> > +
> > +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> > +					bmp380_irq_thread_handler, IRQF_ONESHOT,
> > +					indio_dev->name, indio_dev);
> > +	if (ret) {
> > +		dev_err(data->dev, "request irq failed\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_iio_trigger_register(data->dev, data->trig);
> > +	if (ret) {
> > +		dev_err(data->dev, "iio trigger register failed\n");
> > +		return ret;
> > +	}
> > +
> > +	indio_dev->trig = iio_trigger_get(data->trig);
> > +
> > +	return 0;
> > +}
> > +
> > +
> 
> one blank line only.
> 

Ack.

> >  static irqreturn_t bmp380_trigger_handler(int irq, void *p)
> >  {
> >  	struct iio_poll_func *pf = p;
> > @@ -1854,6 +1998,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
> >  	.wait_conv = bmp380_wait_conv,
> >  	.preinit = bmp380_preinit,
> >  
> > +	.trigger_probe = bmp380_trigger_probe,
> >  	.trigger_handler = bmp380_trigger_handler,
> >  };
> >  EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
> > @@ -2390,6 +2535,154 @@ static int bmp580_chip_config(struct bmp280_data *data)
> >  	return 0;
> >  }
> >
> 
> ...
> 
> > +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
> > +{
> > +	struct iio_dev *indio_dev = p;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	unsigned int int_ctrl;
> > +	int ret;
> > +
> > +	scoped_guard(mutex, &data->lock) {
> 
> Indent wrong.
> 

Ack.

> > +	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
> > +	if (ret)
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
> > +		iio_trigger_poll_nested(data->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> > +{
> ...
> 
> > +
> > +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> > +					    indio_dev->name,
> > +					    iio_device_id(indio_dev));
> > +	if (!data->trig)
> > +		return -ENOMEM;
> > +
> > +	data->trig->ops = &bmp580_trigger_ops;
> > +	iio_trigger_set_drvdata(data->trig, data);
> > +
> > +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> > +					bmp580_irq_thread_handler, IRQF_ONESHOT,
> > +					indio_dev->name, indio_dev);
> > +	if (ret) {
> > +		dev_err(data->dev, "request irq failed\n");
> 
> Only in probe paths I think, so return dev_err_probe() thoughout these
> trigger setup callbacks.
> 

Ack.

> 
> 
> > +}
> 
> > diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
> > index d27d68edd906..cccdf8fc6c09 100644
> > --- a/drivers/iio/pressure/bmp280-regmap.c
> > +++ b/drivers/iio/pressure/bmp280-regmap.c
> > @@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
> >  	case BMP380_REG_FIFO_WATERMARK_LSB:
> >  	case BMP380_REG_FIFO_WATERMARK_MSB:
> >  	case BMP380_REG_POWER_CONTROL:
> > -	case BMP380_REG_INT_CONTROL:
> > +	case BMP380_REG_INT_CTRL:
> 
> Unrelated change.  I'm also not sure it's worth making.
> 

I did it because this is tha name in the datasheet and it was also helping
with the 80 char limit. But I can leave it as it is, there is no problem.

> >  	case BMP380_REG_IF_CONFIG:
> >  	case BMP380_REG_ODR:
> >  	case BMP380_REG_OSR:
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index f5d192509d61..754eda367941 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -55,8 +55,17 @@
> >  #define BMP580_CMD_NVM_WRITE_SEQ_1	0xA0
> >  #define BMP580_CMD_SOFT_RESET		0xB6
> >  
> > +#define BMP580_INT_STATUS_DRDY_MASK	BIT(0)
> >  #define BMP580_INT_STATUS_POR_MASK	BIT(4)
> >  
> > +#define BMP580_INT_SOURCE_DRDY		BIT(0)
> > +
> > +#define BMP580_INT_CONFIG_MASK		GENMASK(3, 0)
> > +#define BMP580_INT_CONFIG_LATCH		BIT(0)
> > +#define BMP580_INT_CONFIG_LEVEL		BIT(1)
> > +#define BMP580_INT_CONFIG_OPEN_DRAIN	BIT(2)
> > +#define BMP580_INT_CONFIG_INT_EN	BIT(3)
> > +
> >  #define BMP580_STATUS_CORE_RDY_MASK	BIT(0)
> >  #define BMP580_STATUS_NVM_RDY_MASK	BIT(1)
> >  #define BMP580_STATUS_NVM_ERR_MASK	BIT(2)
> > @@ -117,7 +126,7 @@
> >  #define BMP380_REG_OSR			0x1C
> >  #define BMP380_REG_POWER_CONTROL	0x1B
> >  #define BMP380_REG_IF_CONFIG		0x1A
> > -#define BMP380_REG_INT_CONTROL		0x19
> > +#define BMP380_REG_INT_CTRL		0x19
> As above.
> 
> Jonathan
> 

Cheers,
Vasilis