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 | 252 ++++++++++++++++++++++++++++-
 drivers/iio/pressure/bmp280.h      |  21 +++
 2 files changed, 269 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index e1336aeceec0..f5268a13bfdb 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>
 
@@ -1271,6 +1273,74 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
+				  const struct iio_trigger_ops *trigger_ops,
+				  int (*int_config)(struct bmp280_data *data),
+				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
+{
+	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)
+		return dev_err_probe(data->dev, -ENODEV,
+				     "No interrupt found.\n");
+
+	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:
+		return dev_err_probe(data->dev, -EINVAL,
+				     "Invalid interrupt type specified.\n");
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = 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 = trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_threaded_irq(data->dev, irq, NULL,
+					irq_thread_handler, IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "request irq failed.\n");
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio trigger register failed.\n");
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
 static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
 static const int bme280_humid_coeffs[] = { 1000, 1024 };
 
@@ -1769,6 +1839,85 @@ 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_CONTROL,
+				 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_CONTROL,
+				 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)
+{
+	return __bmp280_trigger_probe(indio_dev, &bmp380_trigger_ops,
+				      bmp380_int_config,
+				      bmp380_irq_thread_handler);
+}
+
 static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1863,6 +2012,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);
@@ -2401,6 +2551,92 @@ 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)
+{
+	return __bmp280_trigger_probe(indio_dev, &bmp580_trigger_ops,
+				      bmp580_int_config,
+				      bmp580_irq_thread_handler);
+}
+
 static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -2477,6 +2713,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);
@@ -3024,10 +3261,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.h b/drivers/iio/pressure/bmp280.h
index c9840b8d58b0..0c32b6430677 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)
@@ -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;
@@ -510,6 +530,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, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.

...

> +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> +				  const struct iio_trigger_ops *trigger_ops,
> +				  int (*int_config)(struct bmp280_data *data),

> +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))

irq_handler_t

...

> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;

Why do you need this? The below will fail anyway.

> +	irq = fwnode_irq_get(fwnode, 0);
> +	if (!irq)

Are you sure this is correct check?

> +		return dev_err_probe(data->dev, -ENODEV,

Shadowed error code.

> +				     "No interrupt found.\n");

> +	desc = irq_get_irq_data(irq);
> +	if (!desc)
> +		return -EINVAL;

When may this fail?

> +	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:
> +		return dev_err_probe(data->dev, -EINVAL,
> +				     "Invalid interrupt type specified.\n");
> +	}

> +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> +							  "int-open-drain");

Better

	data->trig_open_drain =
		fwnode_property_read_bool(fwnode, "int-open-drain");

...

> +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_CONTROL,
> +				 BMP380_INT_CTRL_DRDY_EN,
> +				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> +					    state ? 1 : 0));

				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));

? ( Even <= 80 characters)

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	if (ret)
		dev_err(data->dev, "Could not enable/disable interrupt\n");

	return ret;

?

> +}

...

> +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);

Split these two variables and make the indentation better for int_cfg.

> +	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> +				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> +	if (ret) {
> +		dev_err(data->dev, "Could not set interrupt settings\n");

> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

?

> +}

...

> +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));

!!state ?

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

?

> +}

...

> +static int bmp580_int_config(struct bmp280_data *data)

Same comments as per above.

...

> +	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;
> +		}
>  	}

Can be

	if (irq > 0) {
		if (chip_id == BMP180_CHIP_ID)
			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
		if (data->chip_info->trigger_probe)
			ret = data->chip_info->trigger_probe(indio_dev);
		if (ret)
			return ret;
	}

-- 
With Best Regards,
Andy Shevchenko

On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.
> 
> ...
> 
> > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > +				  const struct iio_trigger_ops *trigger_ops,
> > +				  int (*int_config)(struct bmp280_data *data),
> 
> > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> 
> irq_handler_t
> 

But the function returns an irqreturn_t type, no?

> ...
> 
> > +	fwnode = dev_fwnode(data->dev);
> > +	if (!fwnode)
> > +		return -ENODEV;
> 
> Why do you need this? The below will fail anyway.

Because If I don't make this check then fwnode might be garbage and I will
pass garbage to the fwnode_irq_get() function. Or do I miss something?

> 
> > +	irq = fwnode_irq_get(fwnode, 0);
> > +	if (!irq)
> 
> Are you sure this is correct check?
> 
Well, I think yes, because the function return either the Linux IRQ number
on success or a negative errno on failure.

https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

> > +		return dev_err_probe(data->dev, -ENODEV,
> 
> Shadowed error code.
> 

I am not sure I understand what you mean here. You mean that there is no
chance that the first one will pass and this one will fail?

> > +				     "No interrupt found.\n");
> 
> > +	desc = irq_get_irq_data(irq);
> > +	if (!desc)
> > +		return -EINVAL;
> 
> When may this fail?
> 

I think that this will fail when Linux were not able to actually
register that interrupt.

> > +	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:
> > +		return dev_err_probe(data->dev, -EINVAL,
> > +				     "Invalid interrupt type specified.\n");
> > +	}
> 
> > +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> > +							  "int-open-drain");
> 
> Better
> 
> 	data->trig_open_drain =
> 		fwnode_property_read_bool(fwnode, "int-open-drain");
> 

Indeed, thanks!

> ...
> 
> > +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_CONTROL,
> > +				 BMP380_INT_CTRL_DRDY_EN,
> > +				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> > +					    state ? 1 : 0));
> 
> 				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));
> 
> ? ( Even <= 80 characters)

Well, that's true.

> 
> > +	if (ret) {
> > +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	if (ret)
> 		dev_err(data->dev, "Could not enable/disable interrupt\n");
> 
> 	return ret;
> 
> ?

All the other if statements follow the style that I typed. If I
follow yours, will make it different just for this one, does it
make sense?

Cheers,
Vasilis
> 
> > +}
> 
> ...
> 
> > +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);
> 
> Split these two variables and make the indentation better for int_cfg.
> 

True, makes sense.

> > +	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> > +				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> > +	if (ret) {
> > +		dev_err(data->dev, "Could not set interrupt settings\n");
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	return ret;
> 
> ?

Yes, you are right.

> 
> > +}
> 
> ...
> 
> > +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));
> 
> !!state ?
> 

ACK.

> > +	if (ret) {
> > +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	return ret;
> 
> ?
> 
> > +}
> 
> ...
> 
> > +static int bmp580_int_config(struct bmp280_data *data)
> 
> Same comments as per above.
> 
> ...
> 
> > +	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;
> > +		}
> >  	}
> 
> Can be
> 
> 	if (irq > 0) {
> 		if (chip_id == BMP180_CHIP_ID)
> 			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
> 		if (data->chip_info->trigger_probe)
> 			ret = data->chip_info->trigger_probe(indio_dev);
> 		if (ret)
> 			return ret;
> 	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Well, it looks much more beautiful indeed. Thanks again for the feedback
Andy, I really appreciate it a lot!

Cheers,
Vasilis

On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:

...

> > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > +				  const struct iio_trigger_ops *trigger_ops,
> > > +				  int (*int_config)(struct bmp280_data *data),
> > 
> > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> > 
> > irq_handler_t
> 
> But the function returns an irqreturn_t type, no?

The type of the last parameter is irq_handler_t, no need to open code it.

...

> > > +	fwnode = dev_fwnode(data->dev);
> > > +	if (!fwnode)
> > > +		return -ENODEV;
> > 
> > Why do you need this? The below will fail anyway.
> 
> Because If I don't make this check then fwnode might be garbage and I will
> pass garbage to the fwnode_irq_get() function. Or do I miss something?

Yes, the function validates fwnode before use. So, please drop unneeded (or
even duplicate) check.

...

> > > +	irq = fwnode_irq_get(fwnode, 0);
> > > +	if (!irq)
> > 
> > Are you sure this is correct check?
> > 
> Well, I think yes, because the function return either the Linux IRQ number
> on success or a negative errno on failure.

Where is 0 mentioned in this?

> https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> 
> > > +		return dev_err_probe(data->dev, -ENODEV,
> > 
> > Shadowed error code.
> 
> I am not sure I understand what you mean here. You mean that there is no
> chance that the first one will pass and this one will fail?

-ENODEV is not what fwnode_irq_get() returns on error.

> > > +				     "No interrupt found.\n");

...

> > > +	desc = irq_get_irq_data(irq);
> > > +	if (!desc)
> > > +		return -EINVAL;
> > 
> > When may this fail?
> 
> I think that this will fail when Linux were not able to actually
> register that interrupt.

Wouldn't fwnode_irq_get() fail already?

...

> > 	if (ret)
> > 		dev_err(data->dev, "Could not enable/disable interrupt\n");

Btw you may use str_enable_disable() here.

> > 	return ret;
> > 
> > ?
> 
> All the other if statements follow the style that I typed. If I
> follow yours, will make it different just for this one, does it
> make sense?

When a comment is given, it's assumed that the _full_ patch (or patch series)
should be revisited for it. Or should I add to every comment something like
this:

"Please, check the entire code for the same or similar case and amend
accordingly."

?

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > > +				  const struct iio_trigger_ops *trigger_ops,
> > > > +				  int (*int_config)(struct bmp280_data *data),
> > > 
> > > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> > > 
> > > irq_handler_t
> > 
> > But the function returns an irqreturn_t type, no?
> 
> The type of the last parameter is irq_handler_t, no need to open code it.
> 
> ...
> 
> > > > +	fwnode = dev_fwnode(data->dev);
> > > > +	if (!fwnode)
> > > > +		return -ENODEV;
> > > 
> > > Why do you need this? The below will fail anyway.
> > 
> > Because If I don't make this check then fwnode might be garbage and I will
> > pass garbage to the fwnode_irq_get() function. Or do I miss something?
> 
> Yes, the function validates fwnode before use. So, please drop unneeded (or
> even duplicate) check.
> 
> ...
> 
> > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > +	if (!irq)
> > > 
> > > Are you sure this is correct check?
> > > 
> > Well, I think yes, because the function return either the Linux IRQ number
> > on success or a negative errno on failure.
> 
> Where is 0 mentioned in this?
> 
> > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > 
> > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > 
> > > Shadowed error code.
> > 
> > I am not sure I understand what you mean here. You mean that there is no
> > chance that the first one will pass and this one will fail?
> 
> -ENODEV is not what fwnode_irq_get() returns on error.
> 
> > > > +				     "No interrupt found.\n");
> 
> ...
> 
> > > > +	desc = irq_get_irq_data(irq);
> > > > +	if (!desc)
> > > > +		return -EINVAL;
> > > 
> > > When may this fail?
> > 
> > I think that this will fail when Linux were not able to actually
> > register that interrupt.
> 
> Wouldn't fwnode_irq_get() fail already?
> 

Hi Andy,

By looking at it again, I didn't reply correct here. This function
internally calls the irq_to_desc() which basically returns the
irq desctiptor for this irq. This function can return NULL in
case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
or in case the interrupt number is bigger than the NR_IRQs which
the irq controller can handle (!CONFIG_SPARSE_IRQ).

So in my opinion, it makes sense to keep this check.

Cheers,
Vasilis

https://elixir.bootlin.com/linux/v6.10.6/source/kernel/irq/chip.c#L155

> ...
> 
> > > 	if (ret)
> > > 		dev_err(data->dev, "Could not enable/disable interrupt\n");
> 
> Btw you may use str_enable_disable() here.
> 
> > > 	return ret;
> > > 
> > > ?
> > 
> > All the other if statements follow the style that I typed. If I
> > follow yours, will make it different just for this one, does it
> > make sense?
> 
> When a comment is given, it's assumed that the _full_ patch (or patch series)
> should be revisited for it. Or should I add to every comment something like
> this:
> 
> "Please, check the entire code for the same or similar case and amend
> accordingly."
> 
> ?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote:
> On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:

...

> > > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > > +	if (!irq)
> > > > 
> > > > Are you sure this is correct check?
> > > > 
> > > Well, I think yes, because the function return either the Linux IRQ number
> > > on success or a negative errno on failure.
> > 
> > Where is 0 mentioned in this?
> > 
> > > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > > 
> > > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > > 
> > > > Shadowed error code.
> > > 
> > > I am not sure I understand what you mean here. You mean that there is no
> > > chance that the first one will pass and this one will fail?
> > 
> > -ENODEV is not what fwnode_irq_get() returns on error.
> > 
> > > > > +				     "No interrupt found.\n");

...

> > > > > +	desc = irq_get_irq_data(irq);
> > > > > +	if (!desc)
> > > > > +		return -EINVAL;
> > > > 
> > > > When may this fail?
> > > 
> > > I think that this will fail when Linux were not able to actually
> > > register that interrupt.
> > 
> > Wouldn't fwnode_irq_get() fail already?
> 
> By looking at it again, I didn't reply correct here. This function
> internally calls the irq_to_desc() which basically returns the
> irq desctiptor for this irq. This function can return NULL in
> case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
> or in case the interrupt number is bigger than the NR_IRQs which
> the irq controller can handle (!CONFIG_SPARSE_IRQ).
> 
> So in my opinion, it makes sense to keep this check.

So, you mean that if fwnode_irq_get() succeeded there is a chance that returned
Linux IRQ number is invalid?! If it's so, it's something new to me. I would like
to see the details, please!

-- 
With Best Regards,
Andy Shevchenko

On Wed, Aug 28, 2024 at 05:17:40PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote:
> > On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> > > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > > > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > > > +	if (!irq)
> > > > > 
> > > > > Are you sure this is correct check?
> > > > > 
> > > > Well, I think yes, because the function return either the Linux IRQ number
> > > > on success or a negative errno on failure.
> > > 
> > > Where is 0 mentioned in this?
> > > 
> > > > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > > > 
> > > > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > > > 
> > > > > Shadowed error code.
> > > > 
> > > > I am not sure I understand what you mean here. You mean that there is no
> > > > chance that the first one will pass and this one will fail?
> > > 
> > > -ENODEV is not what fwnode_irq_get() returns on error.
> > > 
> > > > > > +				     "No interrupt found.\n");
> 
> ...
> 
> > > > > > +	desc = irq_get_irq_data(irq);
> > > > > > +	if (!desc)
> > > > > > +		return -EINVAL;
> > > > > 
> > > > > When may this fail?
> > > > 
> > > > I think that this will fail when Linux were not able to actually
> > > > register that interrupt.
> > > 
> > > Wouldn't fwnode_irq_get() fail already?
> > 
> > By looking at it again, I didn't reply correct here. This function
> > internally calls the irq_to_desc() which basically returns the
> > irq desctiptor for this irq. This function can return NULL in
> > case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
> > or in case the interrupt number is bigger than the NR_IRQs which
> > the irq controller can handle (!CONFIG_SPARSE_IRQ).
> > 
> > So in my opinion, it makes sense to keep this check.
> 
> So, you mean that if fwnode_irq_get() succeeded there is a chance that returned
> Linux IRQ number is invalid?! If it's so, it's something new to me. I would like
> to see the details, please!
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Hi Andy,

I did some more digging, and from my understanding, fwnode_irq_get()
directly returns the Linux IRQ which means that there should already
exist the irq_desc structure that is returned by the irq_to_desc().

So as you already said, I cannot see how this function can fail, if
the fwnode_irq_get() has succeeded. Thanks for asking the right
questions, I am getting to know things better.

Cheers,
Vasilis

On Sat, 24 Aug 2024 14:02:22 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.  
> > 
> > ...
> >   
> > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > +				  const struct iio_trigger_ops *trigger_ops,
> > > +				  int (*int_config)(struct bmp280_data *data),  
> >   
> > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))  
> > 
> > irq_handler_t
> >   
> 
> But the function returns an irqreturn_t type, no?
irq_handler_t is a typdef for the full function signature.
It will still return irqreturn_t 
> 
> > ...
> >   
> > > +	fwnode = dev_fwnode(data->dev);
> > > +	if (!fwnode)
> > > +		return -ENODEV;  
> > 
> > Why do you need this? The below will fail anyway.  
> 
> Because If I don't make this check then fwnode might be garbage and I will
> pass garbage to the fwnode_irq_get() function. Or do I miss something?
It checks for NULL which is all it can actually be and returns a suitable
error code if it is.

> 
> >   
> > > +	irq = fwnode_irq_get(fwnode, 0);
> > > +	if (!irq)  
> > 
> > Are you sure this is correct check?
> >   
> Well, I think yes, because the function return either the Linux IRQ number
> on success or a negative errno on failure.
> 
> https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

Indeed, so if (irq < 0)
		return dev_err_probe(data->dev, irq, ...)

	carry on as valid irq.
your error check if returning only if irq == 0 which never
happens (due to catch for that in the code you link).
Negative values are true, so !-EINVAL == false
for example.
>

On Mon, Aug 26, 2024 at 11:01:50AM +0100, Jonathan Cameron wrote:
> On Sat, 24 Aug 2024 14:02:22 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:  

...

> > > > +	fwnode = dev_fwnode(data->dev);
> > > > +	if (!fwnode)
> > > > +		return -ENODEV;  
> > > 
> > > Why do you need this? The below will fail anyway.  
> > 
> > Because If I don't make this check then fwnode might be garbage and I will
> > pass garbage to the fwnode_irq_get() function. Or do I miss something?
> It checks for NULL which is all it can actually be and returns a suitable
> error code if it is.

Actually not. It may be NULL, error pointer, or valid. So, for a bare minimum
this check is not full (and again, fwnode APIs should validate fwnode before
accessing them where it makes sense; if fwnode_irq_get() does not do that or
misses the case(s), it has to be improved).

-- 
With Best Regards,
Andy Shevchenko