Re: [PATCH] iio: accel: bmc150: Do not configure IRQ registers if no IRQ connected

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Jul 2025 10:55:29 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> On Mon, Jun 16, 2025 at 02:42:54PM +0200, Hans de Goede wrote:
> > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > > index 744a034bb8b5..1c3583ade2b4 100644
> > > --- a/drivers/iio/accel/bmc150-accel-core.c
> > > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > > @@ -550,6 +550,9 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data, int i,
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > +	if (!info)
> > > +		return 0;
> > > +
> > >  	/* map the interrupt to the appropriate pins */
> > >  	ret = regmap_update_bits(data->regmap, info->map_reg, info->map_bitmask,
> > >  				 (state ? info->map_bitmask : 0));  
> > 
> > AFAIK the proper fix would be to not register any IIO-triggers. This fix will
> > avoid the problem, but userspace might still try to use non-working triggers
> > which will now silently fail.  
> 
> I don't have a system that triggers the problem, but there is
> https://bugs.debian.org/1106411 which I guess is the same problem.
> 
> I think the suggestion to not register the triggers in the absence of an
> irq would be implemented by

I was going to say don't do this because it break other triggers being used with this
driver, but... 

We have a validate_trigger callback so this driver can only use a trigger registered
by this driver.  That's a bit of a horrible limitation that we tend to only do
if there is a nasty interaction between event handling and data acquisition.

Here there is such an interaction as a result (I think) of it handling the fifo
off the back of a trigger which is something we don't do in newer drivers.

So this is all a bit nasty but this should work and doesn't require a more
substantial rework.

I think there may be a similar issue though for the events.  For those
we need to switch to a different iio_info instance + different channel array
where the events stuff is all missing if we don't have an irq.

Jonathan




> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index be5fbb0c5d29..8d90802e65fd 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -1687,18 +1687,18 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		fifo_attrs = NULL;
>  	}
>  
> -	ret = iio_triggered_buffer_setup_ext(indio_dev,
> -					     &iio_pollfunc_store_time,
> -					     bmc150_accel_trigger_handler,
> -					     IIO_BUFFER_DIRECTION_IN,
> -					     &bmc150_accel_buffer_ops,
> -					     fifo_attrs);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed: iio triggered buffer setup\n");
> -		goto err_disable_regulators;
> -	}
> -
>  	if (irq > 0) {
> +		ret = iio_triggered_buffer_setup_ext(indio_dev,
> +						     &iio_pollfunc_store_time,
> +						     bmc150_accel_trigger_handler,
> +						     IIO_BUFFER_DIRECTION_IN,
> +						     &bmc150_accel_buffer_ops,
> +						     fifo_attrs);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed: iio triggered buffer setup\n");
> +			goto err_disable_regulators;
> +		}
> +
>  		ret = devm_request_threaded_irq(dev, irq,
>  						bmc150_accel_irq_handler,
>  						bmc150_accel_irq_thread_handler,
> 
> 
> But given that bmc150_accel_write_event_config() also calls
> bmc150_accel_set_interrupt() this might not be a complete fix.
> 
> Best regards
> Uwe