Re: [PATCH v3 15/18] iio: accel: bma220: add interrupt trigger

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 13 Sep 2025 18:39:36 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Add interrupt trigger.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Hi Petre

A few lock questions from a fresh look.
Sorry for delay - busy few weeks and a few days lain up in bed with
a cold.

> ---
> v1->v2 no change, just patch split
> v2->v3 replace regmap_bulk_read with regmap_read (Jonathan)
>  (I just realized BMA220_REG_IF0 is never used, even by future event
> patches)
> ---
>  drivers/iio/accel/bma220_core.c | 61 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index 425a8b981e141aa496351f29df0597c989aa4a0a..6297882bcf1b955291a2d8747984648bc6ee8512 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c
> @@ -23,6 +23,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> @@ -125,6 +126,7 @@ struct bma220_data {
>  	struct regmap *regmap;
>  	struct mutex lock;
>  	u8 range_idx;
> +	struct iio_trigger *trig;
>  	struct {
>  		s8 chans[3];
>  		/* Ensure timestamp is naturally aligned. */
> @@ -193,6 +195,23 @@ const struct regmap_config bma220_i2c_regmap_config = {
>  };
>  EXPORT_SYMBOL_NS_GPL(bma220_i2c_regmap_config, "IIO_BOSCH_BMA220");
>  
> +static int bma220_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct bma220_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->lock);

What is this lock protecting? 

> +	return regmap_update_bits(data->regmap, BMA220_REG_IE0,
> +				  BMA220_INT_EN_DRDY_MSK,
> +				  FIELD_PREP(BMA220_INT_EN_DRDY_MSK, state));
> +}

> @@ -417,6 +436,24 @@ static void bma220_deinit(void *data_ptr)
>  			 ERR_PTR(ret));
>  }
>  
> +static irqreturn_t bma220_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct bma220_data *data = iio_priv(indio_dev);
> +	int rv;
> +	unsigned int bma220_reg_if1;
> +
> +	guard(mutex)(&data->lock);

What is the lock protecting here?  The internal locks in regmap
superficially look like they'd be enough given we only have a single
read.  I'd expect any necessary locking to be on the otherside
of that use of the iio trigger framework (so the poll function thread).

> +	rv = regmap_read(data->regmap, BMA220_REG_IF1, &bma220_reg_if1);
> +	if (rv)
> +		return IRQ_NONE;
> +
> +	if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if1)) {
> +		iio_trigger_poll_nested(data->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  int bma220_common_probe(struct device *dev, struct regmap *regmap, int irq)
>  {
>  	int ret;
> @@ -446,6 +483,30 @@ int bma220_common_probe(struct device *dev, struct regmap *regmap, int irq)
>  	indio_dev->num_channels = ARRAY_SIZE(bma220_channels);
>  	indio_dev->available_scan_masks = bma220_accel_scan_masks;
>  
> +	if (irq > 0) {
> +		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						    indio_dev->name,
> +						    iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return -ENOMEM;
> +
> +		data->trig->ops = &bma220_trigger_ops;
> +		iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "iio trigger register fail\n");
> +		indio_dev->trig = iio_trigger_get(data->trig);
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						&bma220_irq_handler,
> +						IRQF_TRIGGER_RISING | IRQF_ONESHOT,
Interrupt polarity should be coming from firmware.

It's a historically common mistake to encode it in the driver as it breaks
fairly standard things like using an inverter as a cheap level converter
in front of the interrupt pin.

So IRQF_ONEHSOT only should be all that is needed here.

> +						indio_dev->name, indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "request irq %d failed\n", irq);
> +	}
> +
>  	ret = devm_add_action_or_reset(dev, bma220_deinit, data);
>  	if (ret)
>  		return ret;
>