Re: [PATCH 2/4] iio: pressure: mpl3115: clean up interrupt handling and locking

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 May 2026 20:39:36 +0900
SeungJu Cheon <suunj1331@gmail.com> wrote:

> Return IRQ_NONE instead of IRQ_HANDLED when reading
> INT_SOURCE fails.

As in previous, wrap to 75 chars for commit messges.

> 
> On shared interrupt lines, returning IRQ_HANDLED after a
> failed register read may prevent other handlers from being
> invoked.

From what I recall that is simply not true.  Given interrupts might occur
at same moment, the shared interrupt code calls all handlers as it can't
tell if how many interrupt sources have signalled an interrupt.

Look more closely at what isn't invoked (hint it's about when things
go wrong!)

Also, doesn't look to me like this driver supports sharing of the interrupt
anyway.


> 
> Switch the trigger handler from explicit mutex_lock/unlock
> to scoped_guard() for consistency with the locking style
> used elsewhere in the driver.

I'm a little dubious about the value of scoped_guard() for such simple
cases but I guess it is harmless.  As Andy called out though, different
type of change, different patch.

Hmm. Actually do it by pushing the lock down into the function called
instead and there you can use a guard(). Has the added advantage of making
the exact scope of the locking visible next to the calls in it.
So put it in mpl3115_fill_trig_buffer().

> 
> Move mpl3115_config_interrupt() above the interrupt handler
> in preparation for the FIFO support added in a subsequent
> patch.

Separate patch as moving and doing other things just makes for harder
to review code.

> 
> No functional change intended.

Except for the one you call out above.

> 
> Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 59 +++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index befb6d48efa9..52a3d0d59769 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -308,9 +308,8 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	u8 buffer[16] __aligned(8) = { };
>  	int ret;
>  
> -	mutex_lock(&data->lock);
> -	ret = mpl3115_fill_trig_buffer(indio_dev, buffer);
> -	mutex_unlock(&data->lock);
> +	scoped_guard(mutex, &data->lock)
> +		ret = mpl3115_fill_trig_buffer(indio_dev, buffer);
>  	if (ret)
>  		goto done;
>  
> @@ -322,6 +321,32 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int mpl3115_config_interrupt(struct mpl3115_data *data,
> +				    u8 ctrl_reg1, u8 ctrl_reg4)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +					ctrl_reg1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> +					ctrl_reg4);
> +	if (ret < 0)
> +		goto reg1_cleanup;
> +
> +	data->ctrl_reg1 = ctrl_reg1;
> +	data->ctrl_reg4 = ctrl_reg4;
> +
> +	return 0;
> +
> +reg1_cleanup:
> +	i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +				  data->ctrl_reg1);
> +	return ret;
> +}
> +
>  static const struct iio_event_spec mpl3115_temp_press_event[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -381,7 +406,7 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
>  
>  	ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
>  	if (ret < 0)
> -		return IRQ_HANDLED;
> +		return IRQ_NONE;

So the fun of error handling in interrupt threads.  Arguably we have no idea
if it was our interrupt or not if this occurs - which I suspect is the motivation
of the author in returning IRQ_HANDLED here (and IRQ_NONE when we did read but there
were no interrupts).

I think IRQ_NONE still makes more sense because if we do have an intermittent fault
and the state is such that the interrupt will retrigger, then it doesn't matter if we
flag one spurious interrupt.


>  
>  	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
>  		     MPL3115_INT_SRC_DRDY)))
> @@ -420,32 +445,6 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> -static int mpl3115_config_interrupt(struct mpl3115_data *data,
> -				    u8 ctrl_reg1, u8 ctrl_reg4)
> -{
> -	int ret;
> -
> -	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -					ctrl_reg1);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> -					ctrl_reg4);
> -	if (ret < 0)
> -		goto reg1_cleanup;
> -
> -	data->ctrl_reg1 = ctrl_reg1;
> -	data->ctrl_reg4 = ctrl_reg4;
> -
> -	return 0;
> -
> -reg1_cleanup:
> -	i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -				  data->ctrl_reg1);
> -	return ret;
> -}
> -
>  static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);