Re: [PATCH 3/4] iio: pressure: mpl3115: generalize interrupt pin routing

[Jonathan Cameron <jic23@kernel.org>]

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

> Write CTRL_REG5 explicitly for both INT1 and INT2 rather than
> relying on power-on defaults when INT2 is selected.
> 
> This avoids incorrect interrupt routing after a suspend/resume
> cycle where the register may not return to its default state.

Need more information on this.  What do you actually mean by not return
to it's default state?  There are a lot of registers in this device
that affect config - are all the others restored after resume?

I suspect you are actually talking about a soft reboot where the
driver is reloading. Given there is a reset call just above this
that should never be an issue.

> 
> Route both DRDY and FIFO interrupts to the selected pin. The
> FIFO routing is not yet used but is required by the hardware
> FIFO support added in the following patch.
Do it in that patch then, not now.  By all means introduce a
local variable that you change in that patch, but don't set
the fifo bit.
> 
> Consolidate polarity configuration into a single code path
> that handles both interrupt pins uniformly.
> 
> No functional change intended.
> 
> Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 52a3d0d59769..90e83e34ec43 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -67,6 +67,7 @@
>  #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
>  
>  #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> +#define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)

Not in this patch.  

>  
>  static const unsigned int mpl3115_samp_freq_table[][2] = {
>  	{ 1,      0 },
> @@ -624,6 +625,7 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
>  {
>  	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
>  	int ret, irq, irq_type, irq_pin = MPL3115_IRQ_INT1;
> +	u8 ctrl_reg5;
>  
>  	irq = fwnode_irq_get_byname(fwnode, "INT1");
>  	if (irq < 0) {
> @@ -634,6 +636,9 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
>  		irq_pin = MPL3115_IRQ_INT2;
>  	}
>  
> +	ctrl_reg5 = irq_pin == MPL3115_IRQ_INT1 ?
> +	    MPL3115_CTRL5_INT_CFG_DRDY | MPL3115_CTRL5_INT_CFG_FIFO : 0;

That's complex enough that I'd prefer it as a simple if / else
as will be easier to read.  Also you are enabling the fifo interrupt.
That should be in the next patch, not here.

> +
>  	irq_type = irq_get_trigger_type(irq);
>  	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
>  		return -EINVAL;
> @@ -643,23 +648,19 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (irq_pin == MPL3115_IRQ_INT1) {
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						MPL3115_CTRL_REG5,
> -						MPL3115_CTRL5_INT_CFG_DRDY);
> -		if (ret)
> -			return ret;
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> +					ctrl_reg5);
> +	if (ret)
> +		return ret;
>  
> -		if (irq_type == IRQF_TRIGGER_RISING) {
> -			ret = i2c_smbus_write_byte_data(data->client,
> -							MPL3115_CTRL_REG3,
> -							MPL3115_CTRL3_IPOL1);
> -			if (ret)
> -				return ret;
> -		}
> -	} else if (irq_type == IRQF_TRIGGER_RISING) {
> -		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> -						MPL3115_CTRL3_IPOL2);
> +	if (irq_type == IRQF_TRIGGER_RISING) {
> +		u8 ipol = irq_pin == MPL3115_IRQ_INT1 ?
> +			  MPL3115_CTRL3_IPOL1 :
> +			  MPL3115_CTRL3_IPOL2;

Oh that's fun. The data sheet has bit 1 as IPOL2 in the compact
table 49, but PP_OD2 as bits 0 and bit 1 in table 50.  If you are bored,
might be wroth pointing that typo out to them!

> +
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						MPL3115_CTRL_REG3,
> +						ipol);
		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3, ipol);

Fine to go a bit long where it helps readability (stay under 100 chars though!)



>  		if (ret)
>  			return ret;
>  	}