Re: [PATCH v2 6/7] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Fri, 26 Jul 2024 01:10:38 +0200
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> 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@xxxxxxxxx>
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