[PATCH 0/4] iio: pressure: mpl3115: add hardware FIFO support

[PATCH 0/4] iio: pressure: mpl3115: add hardware FIFO support

[SeungJu Cheon]

The MPL3115A2 pressure/temperature sensor provides a 32-sample
hardware FIFO with watermark interrupt support. This series adds
FIFO support, enabling buffered data acquisition with reduced
interrupt and CPU overheads.

1/4 - Convert to devm-managed resource allocation
2/4 - Clean up interrupt handling and locking
3/4 - Generalize interrupt pin routing
4/4 - Add hardware FIFO support

Tested on Raspberry Pi 4 Model B with an MPL3115A2 breakout board
(I2C1, INT1 on GPIO17, falling edge)
- FIFO circular mode operation
- Watermark interrupt generation
- Buffered pressure/temperature data acquisition
- Buffer enable/disable cycling
- Oneshot reads after FIFO operation

This work builds on the DRDY interrupt, triggered buffer,
sampling frequency, and threshold event support previously
added by Antoni Pokusinski.

SeungJu Cheon (4):
  iio: pressure: mpl3115: convert probe to fully devm managed
  iio: pressure: mpl3115: clean up interrupt handling and locking
  iio: pressure: mpl3115: generalize interrupt pin routing
  iio: pressure: mpl3115: add hardware FIFO support

 drivers/iio/pressure/mpl3115.c | 421 ++++++++++++++++++++++++++-------
 1 file changed, 332 insertions(+), 89 deletions(-)

-- 
2.52.0

[PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[SeungJu Cheon]

Convert probe to use devm-managed resource allocation,
removing the need for an explicit remove callback.

Replace iio_triggered_buffer_setup() and
iio_device_register() with their devm equivalents.
Register a devm action to return the device to standby,
replacing the cleanup previously performed in
mpl3115_remove().

Move mpl3115_standby() and suspend/resume helpers above
probe to satisfy declaration ordering requirements.

No functional change.

Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 81 ++++++++++++++++------------------
 1 file changed, 39 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index aeac1586f12e..befb6d48efa9 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -691,6 +691,33 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
 	return 0;
 }
 
+static int mpl3115_standby(struct mpl3115_data *data)
+{
+	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
+}
+
+static void mpl3115_standby_action(void *d)
+{
+	mpl3115_standby(d);
+}
+
+static int mpl3115_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return mpl3115_standby(iio_priv(indio_dev));
+}
+
+static int mpl3115_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mpl3115_data *data = iio_priv(indio_dev);
+
+	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+					 data->ctrl_reg1);
+}
+
 static int mpl3115_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -730,53 +757,24 @@ static int mpl3115_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = mpl3115_trigger_probe(data, indio_dev);
+	ret = devm_add_action_or_reset(&client->dev, mpl3115_standby_action,
+				       data);
 	if (ret)
 		return ret;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-		mpl3115_trigger_handler, NULL);
-	if (ret < 0)
+	ret = mpl3115_trigger_probe(data, indio_dev);
+	if (ret)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0)
-		goto buffer_cleanup;
-	return 0;
-
-buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-	return ret;
-}
-
-static int mpl3115_standby(struct mpl3115_data *data)
-{
-	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
-		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
-}
-
-static void mpl3115_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-	mpl3115_standby(iio_priv(indio_dev));
-}
-
-static int mpl3115_suspend(struct device *dev)
-{
-	return mpl3115_standby(iio_priv(i2c_get_clientdata(
-		to_i2c_client(dev))));
-}
-
-static int mpl3115_resume(struct device *dev)
-{
-	struct mpl3115_data *data = iio_priv(i2c_get_clientdata(
-		to_i2c_client(dev)));
+	ret = devm_iio_triggered_buffer_setup(&client->dev,
+				      indio_dev,
+				      NULL,
+				      mpl3115_trigger_handler,
+				      NULL);
+	if (ret)
+		return ret;
 
-	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
-		data->ctrl_reg1);
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(mpl3115_pm_ops, mpl3115_suspend,
@@ -801,7 +799,6 @@ static struct i2c_driver mpl3115_driver = {
 		.pm	= pm_sleep_ptr(&mpl3115_pm_ops),
 	},
 	.probe = mpl3115_probe,
-	.remove = mpl3115_remove,
 	.id_table = mpl3115_id,
 };
 module_i2c_driver(mpl3115_driver);
-- 
2.52.0

Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[Andy Shevchenko]

On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
>
> Convert probe to use devm-managed resource allocation,
> removing the need for an explicit remove callback.
>
> Replace iio_triggered_buffer_setup() and
> iio_device_register() with their devm equivalents.
> Register a devm action to return the device to standby,
> replacing the cleanup previously performed in
> mpl3115_remove().
>
> Move mpl3115_standby() and suspend/resume helpers above
> probe to satisfy declaration ordering requirements.
>
> No functional change.

...

> +static int mpl3115_suspend(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

What's wrong with dev_get_drvdata() ?

> +       return mpl3115_standby(iio_priv(indio_dev));
> +}
> +
> +static int mpl3115_resume(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

Ditto.

> +       struct mpl3115_data *data = iio_priv(indio_dev);
> +
> +       return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +                                        data->ctrl_reg1);
> +}

...

You forgot about mutex_init().

> -       ret = mpl3115_trigger_probe(data, indio_dev);
> +       ret = devm_add_action_or_reset(&client->dev, mpl3115_standby_action,
> +                                      data);

Add

  struct device *dev = &client->dev;

to the top of the function to make this shorter and easier to read.

>         if (ret)
>                 return ret;

...

> -static int mpl3115_standby(struct mpl3115_data *data)
> -{
> -       return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -               data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> -}

Split moving of this function to a separate prerequisite patch.

...

> -static int mpl3115_suspend(struct device *dev)

> -static int mpl3115_resume(struct device *dev)

Why did you touch these two?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[SeungJu Cheon]

Hi Andy, thanks for the review.

On Sat, May 30, 2026 at 9:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > +static int mpl3115_suspend(struct device *dev)
> > +{
> > +       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>
> What's wrong with dev_get_drvdata() ?

Nothing, you're right. Will use dev_get_drvdata() in v2.

> Ditto.

Same, will fix.

> You forgot about mutex_init().

mutex_init(&data->lock) is already present in probe(), but I agree
that converting it to devm_mutex_init() fits better with the devm
conversion. I'll update this in v2.

> Add
>   struct device *dev = &client->dev;
> to the top of the function to make this shorter and easier to read.

Will do.

> Split moving of this function to a separate prerequisite patch.

Will split the function move into its own prerequisite patch in v2.

> > -static int mpl3115_suspend(struct device *dev)
> > -static int mpl3115_resume(struct device *dev)
>
> Why did you touch these two?

No good reason -- I only reworked them to flatten the nested
i2c_get_clientdata(to_i2c_client()) call. Since that's unrelated to
this patch, I'll leave them untouched here and do the minimal move
only, as Jonathan also suggested.

Thanks for the review.

Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[Jonathan Cameron]

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

> Convert probe to use devm-managed resource allocation,
> removing the need for an explicit remove callback.

Hi SeungJu,

> 
> Replace iio_triggered_buffer_setup() and
> iio_device_register() with their devm equivalents.
> Register a devm action to return the device to standby,
> replacing the cleanup previously performed in
> mpl3115_remove().

This should have noted that in error paths the standby wasn't there and now is.
However see below; I think the patch need to be split in at least two parts.

Very short wrap.  Exactly whether to limit commit messages to 72 or 75 chars
is a bit of a matter of opinion, but this is 50 something.
> 
> Move mpl3115_standby() and suspend/resume helpers above
> probe to satisfy declaration ordering requirements.
Not obvious why the suspend/resume moved.  Just move the minimum to make
the patch easier to read.
> 
> No functional change.

Not true. That error path gaining suspend of device is a functional change.

> 
> Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 81 ++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index aeac1586f12e..befb6d48efa9 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -691,6 +691,33 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
>  	return 0;
>  }
>  
> +static int mpl3115_standby(struct mpl3115_data *data)
> +{
> +	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);

Take opportunity to tidy indent up now we are less fuzzy about 80 chars.

	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
					 data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
is the preferred style.

> +}
> +
> +static void mpl3115_standby_action(void *d)
> +{
> +	mpl3115_standby(d);
> +}
> +
> +static int mpl3115_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

Andy covered this. I think we ripped out most instances of this a year
or so back as it is a pointless bit of bouncing back and forth between
container structure and the contained.

> +
> +	return mpl3115_standby(iio_priv(indio_dev));
> +}
> +
> +static int mpl3115_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +
> +	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +					 data->ctrl_reg1);
> +}

Andy called out the question of why these moved. It makes the patch
somewhat harder to review, so don't do that in this patch even if there
is a reason to do it. 

> +
>  static int mpl3115_probe(struct i2c_client *client)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -730,53 +757,24 @@ static int mpl3115_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = mpl3115_trigger_probe(data, indio_dev);
> +	ret = devm_add_action_or_reset(&client->dev, mpl3115_standby_action,
> +				       data);

So in the error path for this function we will now call this standby
action. If there isn't an equivalent in the original code, break this up.

Patch 1. Add that cleanup without any devm stuff (almost a fix but it's
         only going to waste a bit of power in an unlikely error path so
	 we probably won't backport it).
Patch 2. Devm stuff.

>  	if (ret)
>  		return ret;
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> -		mpl3115_trigger_handler, NULL);
> -	if (ret < 0)
> +	ret = mpl3115_trigger_probe(data, indio_dev);
> +	if (ret)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret < 0)
> -		goto buffer_cleanup;
> -	return 0;
> -
> -buffer_cleanup:
> -	iio_triggered_buffer_cleanup(indio_dev);
> -	return ret;
> -}
> -
> -static int mpl3115_standby(struct mpl3115_data *data)
> -{
> -	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> -}
> -
> -static void mpl3115_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> -	iio_device_unregister(indio_dev);
> -	iio_triggered_buffer_cleanup(indio_dev);
> -	mpl3115_standby(iio_priv(indio_dev));
> -}
> -
> -static int mpl3115_suspend(struct device *dev)
> -{
> -	return mpl3115_standby(iio_priv(i2c_get_clientdata(
> -		to_i2c_client(dev))));
> -}
> -
> -static int mpl3115_resume(struct device *dev)
> -{
> -	struct mpl3115_data *data = iio_priv(i2c_get_clientdata(
> -		to_i2c_client(dev)));
> +	ret = devm_iio_triggered_buffer_setup(&client->dev,
> +				      indio_dev,
> +				      NULL,
> +				      mpl3115_trigger_handler,
> +				      NULL);

Indent as something like:

	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
					      mpl3115_trigger_handler, NULL);

> +	if (ret)
> +		return ret;
>  
> -	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> -		data->ctrl_reg1);
> +	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(mpl3115_pm_ops, mpl3115_suspend,
> @@ -801,7 +799,6 @@ static struct i2c_driver mpl3115_driver = {
>  		.pm	= pm_sleep_ptr(&mpl3115_pm_ops),
>  	},
>  	.probe = mpl3115_probe,
> -	.remove = mpl3115_remove,
>  	.id_table = mpl3115_id,
>  };
>  module_i2c_driver(mpl3115_driver);

Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[SeungJu Cheon]

Hi Jonathan, thanks for the detailed review.

On Sat, 30 May 2026 20:39:35 +0900 SeungJu Cheon <suunj1331@gmail.com> wrote:
> > Register a devm action to return the device to standby,
> > replacing the cleanup previously performed in
> > mpl3115_remove().
>
> This should have noted that in error paths the standby wasn't there
> and now is. However see below; I think the patch need to be split in
> at least two parts.

You're right. I'll split it into two patches in v2:
  1. Add the standby cleanup on the probe error path (no devm yet).
  2. Convert to devm-managed allocation.
and note the error-path behaviour change explicitly.

> Very short wrap. Exactly whether to limit commit messages to 72 or
> 75 chars is a bit of a matter of opinion, but this is 50 something.

Understood, I'll rewrap the commit messages closer to 75 chars.

> > Move mpl3115_standby() and suspend/resume helpers above
> > probe to satisfy declaration ordering requirements.
>
> Not obvious why the suspend/resume moved. Just move the minimum to
> make the patch easier to read.

Agreed, that was unnecessary churn. I'll move only what's needed and
leave suspend/resume in place.

> > No functional change.
>
> Not true. That error path gaining suspend of device is a functional
> change.

You're right, I'll drop that line. The error-path change will live in
the split-out fix patch and be described there.

> Take opportunity to tidy indent up now we are less fuzzy about 80
> chars.

Will fix the indentation as you show.

> Andy covered this. I think we ripped out most instances of this a
> year or so back as it is a pointless bit of bouncing back and forth
> between container structure and the contained.

Yes, switching to dev_get_drvdata() in v2.

> Andy called out the question of why these moved. It makes the patch
> somewhat harder to review, so don't do that in this patch even if
> there is a reason to do it.

Understood, I'll leave suspend/resume untouched here.

> So in the error path for this function we will now call this standby
> action. If there isn't an equivalent in the original code, break
> this up.
> Patch 1. Add that cleanup without any devm stuff ...
> Patch 2. Devm stuff.

Confirmed there's no standby on the error path in the current code,
so I'll break it up exactly as you describe.

> Indent as something like:
>         ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>                                               mpl3115_trigger_handler, NULL);

Will do.

Thanks again.

On Sun, May 31, 2026 at 12:10 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 May 2026 20:39:35 +0900
> SeungJu Cheon <suunj1331@gmail.com> wrote:
>
> > Convert probe to use devm-managed resource allocation,
> > removing the need for an explicit remove callback.
>
> Hi SeungJu,
>
> >
> > Replace iio_triggered_buffer_setup() and
> > iio_device_register() with their devm equivalents.
> > Register a devm action to return the device to standby,
> > replacing the cleanup previously performed in
> > mpl3115_remove().
>
> This should have noted that in error paths the standby wasn't there and now is.
> However see below; I think the patch need to be split in at least two parts.
>
> Very short wrap.  Exactly whether to limit commit messages to 72 or 75 chars
> is a bit of a matter of opinion, but this is 50 something.
> >
> > Move mpl3115_standby() and suspend/resume helpers above
> > probe to satisfy declaration ordering requirements.
> Not obvious why the suspend/resume moved.  Just move the minimum to make
> the patch easier to read.
> >
> > No functional change.
>
> Not true. That error path gaining suspend of device is a functional change.
>
> >
> > Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 81 ++++++++++++++++------------------
> >  1 file changed, 39 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index aeac1586f12e..befb6d48efa9 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -691,6 +691,33 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
> >       return 0;
> >  }
> >
> > +static int mpl3115_standby(struct mpl3115_data *data)
> > +{
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +             data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
>
> Take opportunity to tidy indent up now we are less fuzzy about 80 chars.
>
>         return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
>                                          data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> is the preferred style.
>
> > +}
> > +
> > +static void mpl3115_standby_action(void *d)
> > +{
> > +     mpl3115_standby(d);
> > +}
> > +
> > +static int mpl3115_suspend(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>
> Andy covered this. I think we ripped out most instances of this a year
> or so back as it is a pointless bit of bouncing back and forth between
> container structure and the contained.
>
> > +
> > +     return mpl3115_standby(iio_priv(indio_dev));
> > +}
> > +
> > +static int mpl3115_resume(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                      data->ctrl_reg1);
> > +}
>
> Andy called out the question of why these moved. It makes the patch
> somewhat harder to review, so don't do that in this patch even if there
> is a reason to do it.
>
> > +
> >  static int mpl3115_probe(struct i2c_client *client)
> >  {
> >       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > @@ -730,53 +757,24 @@ static int mpl3115_probe(struct i2c_client *client)
> >       if (ret < 0)
> >               return ret;
> >
> > -     ret = mpl3115_trigger_probe(data, indio_dev);
> > +     ret = devm_add_action_or_reset(&client->dev, mpl3115_standby_action,
> > +                                    data);
>
> So in the error path for this function we will now call this standby
> action. If there isn't an equivalent in the original code, break this up.
>
> Patch 1. Add that cleanup without any devm stuff (almost a fix but it's
>          only going to waste a bit of power in an unlikely error path so
>          we probably won't backport it).
> Patch 2. Devm stuff.
>
> >       if (ret)
> >               return ret;
> >
> > -     ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > -             mpl3115_trigger_handler, NULL);
> > -     if (ret < 0)
> > +     ret = mpl3115_trigger_probe(data, indio_dev);
> > +     if (ret)
> >               return ret;
> >
> > -     ret = iio_device_register(indio_dev);
> > -     if (ret < 0)
> > -             goto buffer_cleanup;
> > -     return 0;
> > -
> > -buffer_cleanup:
> > -     iio_triggered_buffer_cleanup(indio_dev);
> > -     return ret;
> > -}
> > -
> > -static int mpl3115_standby(struct mpl3115_data *data)
> > -{
> > -     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -             data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> > -}
> > -
> > -static void mpl3115_remove(struct i2c_client *client)
> > -{
> > -     struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > -     iio_device_unregister(indio_dev);
> > -     iio_triggered_buffer_cleanup(indio_dev);
> > -     mpl3115_standby(iio_priv(indio_dev));
> > -}
> > -
> > -static int mpl3115_suspend(struct device *dev)
> > -{
> > -     return mpl3115_standby(iio_priv(i2c_get_clientdata(
> > -             to_i2c_client(dev))));
> > -}
> > -
> > -static int mpl3115_resume(struct device *dev)
> > -{
> > -     struct mpl3115_data *data = iio_priv(i2c_get_clientdata(
> > -             to_i2c_client(dev)));
> > +     ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                   indio_dev,
> > +                                   NULL,
> > +                                   mpl3115_trigger_handler,
> > +                                   NULL);
>
> Indent as something like:
>
>         ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>                                               mpl3115_trigger_handler, NULL);
>
> > +     if (ret)
> > +             return ret;
> >
> > -     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -             data->ctrl_reg1);
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> >  }
> >
> >  static DEFINE_SIMPLE_DEV_PM_OPS(mpl3115_pm_ops, mpl3115_suspend,
> > @@ -801,7 +799,6 @@ static struct i2c_driver mpl3115_driver = {
> >               .pm     = pm_sleep_ptr(&mpl3115_pm_ops),
> >       },
> >       .probe = mpl3115_probe,
> > -     .remove = mpl3115_remove,
> >       .id_table = mpl3115_id,
> >  };
> >  module_i2c_driver(mpl3115_driver);
>

Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

[Jonathan Cameron]

On Sun, 31 May 2026 19:49:40 +0900
SeungJu Cheon <suunj1331@gmail.com> wrote:

> Hi Jonathan, thanks for the detailed review.

Hi,

Small process thing.  When you agree with a comment and don't have
follow up questions, then crop out that part of the email.  Save any
acknowledgement / thanks for the change log section in the next version.

That lets us quickly see if there are questions.

If you have no questions or follow up on a particular review, don't reply
at all!  We will see the result in the next version anyway.

If there was anything in here that needed a reply, I missed it!
Jonathan

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

[SeungJu Cheon]

Return IRQ_NONE instead of IRQ_HANDLED when reading
INT_SOURCE fails.

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

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

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

No functional change intended.

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;
 
 	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);
-- 
2.52.0

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

[Andy Shevchenko]

On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
>
> Return IRQ_NONE instead of IRQ_HANDLED when reading
> INT_SOURCE fails.
>
> On shared interrupt lines, returning IRQ_HANDLED after a
> failed register read may prevent other handlers from being
> invoked.

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

Split these two. Moving to scoped_guard() is a separate refactoring.

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

> No functional change intended.

But this is a lie. The change is a (big enough) functional change and
actually it feels and sounds like it deserves a Fixes tag.

...

> +reg1_cleanup:
> +       i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +                                 data->ctrl_reg1);

And if this fails?.. (Okay, this is not this patch issue, it's an
original code.)

> +       return ret;
> +}

...

> -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;
> -}

If you need to move it up, split this move to a separate patch.

-- 
With Best Regards,
Andy Shevchenko

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

[SeungJu Cheon]

Hi Andy,

On Sat, May 30, 2026 at 9:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> Split these two. Moving to scoped_guard() is a separate refactoring.

Will do. I'll move the scoped_guard() conversion to its own patch in
v2.

> > No functional change intended.
>
> But this is a lie. The change is a (big enough) functional change and
> actually it feels and sounds like it deserves a Fixes tag.

You're right, the IRQ_NONE change is a functional change. I'll split
it into its own patch with a Fixes tag and drop the "no functional
change" wording.

> > +reg1_cleanup:
> > +       i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                 data->ctrl_reg1);
>
> And if this fails?.. (Okay, this is not this patch issue, it's an
> original code.)

Right, that's pre-existing and outside the scope of this series.

> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> If you need to move it up, split this move to a separate patch.

Will split the move into its own patch in v2.

Thanks for the review.

On Sat, May 30, 2026 at 9:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
> >
> > Return IRQ_NONE instead of IRQ_HANDLED when reading
> > INT_SOURCE fails.
> >
> > On shared interrupt lines, returning IRQ_HANDLED after a
> > failed register read may prevent other handlers from being
> > invoked.
>
> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> Split these two. Moving to scoped_guard() is a separate refactoring.
>
> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> > No functional change intended.
>
> But this is a lie. The change is a (big enough) functional change and
> actually it feels and sounds like it deserves a Fixes tag.
>
> ...
>
> > +reg1_cleanup:
> > +       i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                 data->ctrl_reg1);
>
> And if this fails?.. (Okay, this is not this patch issue, it's an
> original code.)
>
> > +       return ret;
> > +}
>
> ...
>
> > -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;
> > -}
>
> If you need to move it up, split this move to a separate patch.
>
> --
> With Best Regards,
> Andy Shevchenko

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

[Jonathan Cameron]

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);

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

[SeungJu Cheon]

Hi Jonathan,

On Sat, 30 May 2026 12:23 ... Jonathan Cameron wrote:
> > Return IRQ_NONE instead of IRQ_HANDLED when reading
> > INT_SOURCE fails.
>
> As in previous, wrap to 75 chars for commit messges.

Will rewrap to 75 chars.

> > 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.

You're right, the shared-handler rationale is wrong, and this driver
doesn't share the interrupt anyway. That's a better rationale. I'll
rework the commit message around the spurious-interrupt accounting
aspect rather than shared interrupt handling.

> > 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(). [...]
> So put it in mpl3115_fill_trig_buffer().

Good idea. I'll move the locking into mpl3115_fill_trig_buffer() with
a guard() so the scope sits next to the accesses it protects, and put
that in its own patch.

> > 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.

Agreed, I'll split the move into its own patch.

> > No functional change intended.
>
> Except for the one you call out above.

Right. I'll drop that line; the IRQ_NONE change moves to its own
patch and will be described there.

Thanks for the detailed review.

On Sun, May 31, 2026 at 12:23 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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);
>

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

[SeungJu Cheon]

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.

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.

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)
 
 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;
+
 	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;
+
+		ret = i2c_smbus_write_byte_data(data->client,
+						MPL3115_CTRL_REG3,
+						ipol);
 		if (ret)
 			return ret;
 	}
-- 
2.52.0

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

[Andy Shevchenko]

On Sat, May 30, 2026 at 1:40 PM 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.
>
> 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.
>
> Consolidate polarity configuration into a single code path
> that handles both interrupt pins uniformly.

> No functional change intended.

All the same issues in the commit message as with the previous change.

-- 
With Best Regards,
Andy Shevchenko

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

[SeungJu Cheon]

Hi Andy,

On Sat, May 30, 2026 at 9:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > No functional change intended.
>
> All the same issues in the commit message as with the previous
> change.

Right. I'll address it the same way in v2: drop the "no functional
change" line, split the unrelated changes, and rework the commit
message accordingly.

Thanks.

On Sat, May 30, 2026 at 9:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, May 30, 2026 at 1:40 PM 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.
> >
> > 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.
> >
> > Consolidate polarity configuration into a single code path
> > that handles both interrupt pins uniformly.
>
> > No functional change intended.
>
> All the same issues in the commit message as with the previous change.
>
> --
> With Best Regards,
> Andy Shevchenko

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

[Jonathan Cameron]

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;
>  	}

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

[SeungJu Cheon]

Hi Jonathan,

On Sat, 30 May 2026 12:32 ... Jonathan Cameron wrote:
> > 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.

You're right, that justification doesn't hold. The reset call above
covers the reload case, so the register won't be in an unexpected
state here. I'll drop the suspend/resume reasoning and describe the
change simply as writing CTRL_REG5 explicitly instead of relying on
the power-on default when INT2 is selected.

> > 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.

Will do. I'll introduce the ctrl_reg5 local here with only the DRDY
bit and add the FIFO bit in the FIFO patch.

> > +#define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
>
> Not in this patch.

Right, I'll move that define to the FIFO patch.

> > +     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.

Will switch to a plain if / else and drop the FIFO bit here.

> 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!

Good catch, I hadn't noticed that one.

> > +             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!)

Will put it on one line.

Thanks again.

On Sun, May 31, 2026 at 12:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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;
> >       }
>

[PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[SeungJu Cheon]

Add support for the MPL3115A2 hardware FIFO.

The device provides a 32-sample FIFO with configurable
watermark interrupts, allowing buffered data acquisition
with reduced interrupt and CPU overhead.

Use a kfifo-backed IIO buffer when a DRDY interrupt and
SMBus block reads are available, falling back to the
existing triggered buffer implementation otherwise.

When enabled, the driver configures the FIFO in circular
mode and drains accumulated samples on watermark
interrupts.

Overflow conditions are handled by flushing available
samples before resetting the FIFO, avoiding unnecessary
data loss.

The hardware always captures pressure and temperature
samples together, so FIFO operation requires both channels
to be enabled.

Register cache updates are performed only after successful
I2C writes to keep cached and hardware state consistent.

No functional change for non-buffered operation.

Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 266 +++++++++++++++++++++++++++++++--
 1 file changed, 256 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 90e83e34ec43..26b6490c8c4c 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -6,7 +6,7 @@
  *
  * (7-bit I2C slave address 0x60)
  *
- * TODO: FIFO buffer, altimeter mode, oversampling, continuous mode,
+ * TODO: altimeter mode, oversampling, continuous mode,
  * user offset correction, raw mode
  */
 
@@ -22,6 +22,7 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
@@ -31,6 +32,8 @@
 #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
 #define MPL3115_OUT_TEMP 0x04 /* MSB first, 12 bit */
 #define MPL3115_WHO_AM_I 0x0c
+#define MPL3115_F_STATUS 0x0d
+#define MPL3115_F_SETUP 0x0f
 #define MPL3115_INT_SOURCE 0x12
 #define MPL3115_PT_DATA_CFG 0x13
 #define MPL3115_PRESS_TGT 0x16 /* MSB first, 16 bit */
@@ -47,9 +50,32 @@
 #define MPL3115_STATUS_TEMP_RDY BIT(1)
 
 #define MPL3115_INT_SRC_DRDY BIT(7)
+#define MPL3115_INT_SRC_FIFO BIT(6)
 #define MPL3115_INT_SRC_PTH BIT(3)
 #define MPL3115_INT_SRC_TTH BIT(2)
 
+#define MPL3115_F_STATUS_F_OVF BIT(7)
+#define MPL3115_F_STATUS_F_CNT GENMASK(5, 0)
+
+#define MPL3115_F_SETUP_F_MODE GENMASK(7, 6)
+#define MPL3115_F_SETUP_F_WMRK GENMASK(5, 0)
+
+enum mpl3115_fifo_mode {
+	MPL3115_F_MODE_DISABLED = 0,
+	MPL3115_F_MODE_CIRCULAR = 1,
+};
+
+#define MPL3115_FIFO_SIZE 32
+#define MPL3115_FIFO_SAMPLE_SIZE 5
+#define MPL3115_DEFAULT_WATERMARK 1
+
+#define MPL3115_BUF_PRESS_OFFSET 0
+#define MPL3115_BUF_TEMP_OFFSET 4
+#define MPL3115_BUF_TS_OFFSET 8
+#define MPL3115_BUF_SIZE (MPL3115_BUF_TS_OFFSET + sizeof(s64))
+
+#define MPL3115_FIFO_SAMPLES_PER_READ 6U
+
 #define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
 
 #define MPL3115_CTRL1_RESET BIT(2) /* software reset */
@@ -63,8 +89,12 @@
 #define MPL3115_CTRL3_IPOL2 BIT(1)
 
 #define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
+#define MPL3115_CTRL4_INT_EN_FIFO BIT(6)
 #define MPL3115_CTRL4_INT_EN_PTH BIT(3)
 #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
+#define MPL3115_CTRL4_ACTIVE_MASK \
+	(MPL3115_CTRL4_INT_EN_DRDY | \
+	 MPL3115_CTRL4_INT_EN_PTH | MPL3115_CTRL4_INT_EN_TTH)
 
 #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
 #define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
@@ -94,6 +124,9 @@ struct mpl3115_data {
 	struct mutex lock;
 	u8 ctrl_reg1;
 	u8 ctrl_reg4;
+	u8 watermark;
+	u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
+		__aligned(IIO_DMA_MINALIGN);
 };
 
 enum mpl3115_irq_pin {
@@ -101,6 +134,25 @@ enum mpl3115_irq_pin {
 	MPL3115_IRQ_INT2,
 };
 
+static int mpl3115_set_active(struct mpl3115_data *data, bool active)
+{
+	u8 reg;
+	int ret;
+
+	if (active)
+		reg = data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE;
+	else
+		reg = data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE;
+
+	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+					reg);
+	if (ret)
+		return ret;
+
+	data->ctrl_reg1 = reg;
+	return 0;
+}
+
 static int mpl3115_request(struct mpl3115_data *data)
 {
 	int ret, tries = 15;
@@ -322,6 +374,109 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int mpl3115_set_fifo(struct mpl3115_data *data,
+			    enum mpl3115_fifo_mode mode)
+{
+	u8 f_setup;
+
+	f_setup = FIELD_PREP(MPL3115_F_SETUP_F_MODE, mode) |
+		  FIELD_PREP(MPL3115_F_SETUP_F_WMRK, data->watermark);
+
+	return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
+			f_setup);
+}
+
+static int mpl3115_fifo_reset(struct mpl3115_data *data)
+{
+	int ret;
+
+	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
+	if (ret)
+		return ret;
+
+	return mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
+}
+
+static int mpl3115_fifo_transfer(struct mpl3115_data *data,
+				 unsigned int sample_count)
+{
+	unsigned int remaining = sample_count;
+	unsigned int offset = 0;
+	unsigned int batch;
+	int ret;
+
+	while (remaining > 0) {
+		batch = min(remaining, MPL3115_FIFO_SAMPLES_PER_READ);
+
+		ret = i2c_smbus_read_i2c_block_data(data->client,
+						    MPL3115_OUT_PRESS,
+						    batch * MPL3115_FIFO_SAMPLE_SIZE,
+						    &data->fifo_buf[offset]);
+		if (ret < 0)
+			return ret;
+		if (ret != batch * MPL3115_FIFO_SAMPLE_SIZE)
+			return -EIO;
+
+		offset += batch * MPL3115_FIFO_SAMPLE_SIZE;
+		remaining -= batch;
+	}
+
+	return 0;
+}
+
+static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
+	unsigned int sample_count, i;
+	int ret;
+	bool overflow;
+	s64 ts;
+
+	ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
+	if (ret < 0)
+		return ret;
+
+	overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
+	sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
+	if (sample_count == 0)
+		return overflow ? mpl3115_fifo_reset(data) : 0;
+
+	ret = mpl3115_fifo_transfer(data, sample_count);
+	if (ret)
+		return ret;
+
+	ts = iio_get_time_ns(indio_dev);
+	for (i = 0; i < sample_count; i++) {
+		u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];
+
+		memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
+		memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
+
+		iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
+	}
+
+	if (overflow) {
+		dev_warn_ratelimited(&data->client->dev,
+				     "FIFO overflow, samples may be lost\n");
+		return mpl3115_fifo_reset(data);
+	}
+
+	return 0;
+}
+
+static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+
+	value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
+
+	guard(mutex)(&data->lock);
+	data->watermark = value;
+
+	return 0;
+}
+
 static int mpl3115_config_interrupt(struct mpl3115_data *data,
 				    u8 ctrl_reg1, u8 ctrl_reg4)
 {
@@ -348,6 +503,67 @@ static int mpl3115_config_interrupt(struct mpl3115_data *data,
 	return ret;
 }
 
+static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 ctrl_reg4;
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = mpl3115_set_active(data, false);
+	if (ret)
+		return ret;
+
+	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
+	if (ret) {
+		mpl3115_set_active(data, true);
+		return ret;
+	}
+
+	ctrl_reg4 = data->ctrl_reg4;
+	ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
+	ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
+
+	return mpl3115_config_interrupt(data,
+		data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
+}
+
+static int mpl3115_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct mpl3115_data *data = iio_priv(indio_dev);
+	u8 ctrl_reg1, ctrl_reg4;
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = mpl3115_set_active(data, false);
+	if (ret)
+		return ret;
+
+	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
+	if (ret)
+		return ret;
+
+	ctrl_reg4 = data->ctrl_reg4 & ~MPL3115_CTRL4_INT_EN_FIFO;
+	ctrl_reg1 = data->ctrl_reg1;
+
+	if (ctrl_reg4 & MPL3115_CTRL4_ACTIVE_MASK)
+		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
+
+	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
+}
+
+static const struct iio_buffer_setup_ops mpl3115_buffer_ops = {
+	.postenable = mpl3115_buffer_postenable,
+	.predisable = mpl3115_buffer_predisable,
+};
+
+static const unsigned long mpl3115_scan_masks[] = {
+	BIT(0) | BIT(1),
+	0,
+};
+
 static const struct iio_event_spec mpl3115_temp_press_event[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -409,10 +625,19 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
 	if (ret < 0)
 		return IRQ_NONE;
 
-	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
-		     MPL3115_INT_SRC_DRDY)))
+	if (!(ret & (MPL3115_INT_SRC_FIFO | MPL3115_INT_SRC_TTH |
+		     MPL3115_INT_SRC_PTH | MPL3115_INT_SRC_DRDY)))
 		return IRQ_NONE;
 
+	if (ret & MPL3115_INT_SRC_FIFO) {
+		int flush_ret;
+
+		scoped_guard(mutex, &data->lock)
+			flush_ret = mpl3115_fifo_flush(indio_dev);
+		if (flush_ret < 0)
+			goto err_reset_fifo;
+	}
+
 	if (ret & MPL3115_INT_SRC_DRDY)
 		iio_trigger_poll_nested(data->drdy_trig);
 
@@ -444,6 +669,11 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
 	}
 
 	return IRQ_HANDLED;
+
+err_reset_fifo:
+	scoped_guard(mutex, &data->lock)
+		mpl3115_fifo_reset(data);
+	return IRQ_HANDLED;
 }
 
 static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
@@ -618,6 +848,7 @@ static const struct iio_info mpl3115_info = {
 	.write_event_config = mpl3115_write_event_config,
 	.read_event_value = mpl3115_read_thresh,
 	.write_event_value = mpl3115_write_thresh,
+	.hwfifo_set_watermark = mpl3115_set_watermark,
 };
 
 static int mpl3115_trigger_probe(struct mpl3115_data *data,
@@ -723,6 +954,7 @@ static int mpl3115_probe(struct i2c_client *client)
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	struct mpl3115_data *data;
 	struct iio_dev *indio_dev;
+	bool use_fifo;
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, MPL3115_WHO_AM_I);
@@ -762,17 +994,31 @@ static int mpl3115_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	data->watermark = MPL3115_DEFAULT_WATERMARK;
+
 	ret = mpl3115_trigger_probe(data, indio_dev);
 	if (ret)
 		return ret;
 
-	ret = devm_iio_triggered_buffer_setup(&client->dev,
-				      indio_dev,
-				      NULL,
-				      mpl3115_trigger_handler,
-				      NULL);
-	if (ret)
-		return ret;
+	use_fifo = data->drdy_trig &&
+	   i2c_check_functionality(client->adapter,
+				   I2C_FUNC_SMBUS_READ_I2C_BLOCK);
+
+	if (use_fifo) {
+		indio_dev->available_scan_masks = mpl3115_scan_masks;
+		ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
+						  &mpl3115_buffer_ops);
+		if (ret)
+			return ret;
+	} else {
+		ret = devm_iio_triggered_buffer_setup(&client->dev,
+						      indio_dev,
+						      NULL,
+						      mpl3115_trigger_handler,
+						      NULL);
+		if (ret)
+			return ret;
+	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
-- 
2.52.0

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[Andy Shevchenko]

On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
>
> Add support for the MPL3115A2 hardware FIFO.
>
> The device provides a 32-sample FIFO with configurable
> watermark interrupts, allowing buffered data acquisition
> with reduced interrupt and CPU overhead.
>
> Use a kfifo-backed IIO buffer when a DRDY interrupt and
> SMBus block reads are available, falling back to the
> existing triggered buffer implementation otherwise.
>
> When enabled, the driver configures the FIFO in circular
> mode and drains accumulated samples on watermark
> interrupts.
>
> Overflow conditions are handled by flushing available
> samples before resetting the FIFO, avoiding unnecessary
> data loss.
>
> The hardware always captures pressure and temperature
> samples together, so FIFO operation requires both channels
> to be enabled.
>
> Register cache updates are performed only after successful
> I2C writes to keep cached and hardware state consistent.
>
> No functional change for non-buffered operation.

...

> +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> +               __aligned(IIO_DMA_MINALIGN);

We have a macro for this,

...

> +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> +{
> +       struct mpl3115_data *data = iio_priv(indio_dev);
> +       u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };

Don't we have a macro for this?

> +       unsigned int sample_count, i;
> +       int ret;
> +       bool overflow;
> +       s64 ts;
> +
> +       ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> +       if (ret < 0)
> +               return ret;
> +
> +       overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> +       sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> +       if (sample_count == 0)
> +               return overflow ? mpl3115_fifo_reset(data) : 0;
> +
> +       ret = mpl3115_fifo_transfer(data, sample_count);
> +       if (ret)
> +               return ret;
> +
> +       ts = iio_get_time_ns(indio_dev);
> +       for (i = 0; i < sample_count; i++) {

  for (unsigned int i = 0; ...) {

> +               u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];

> +               memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> +               memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);

These two sound to me like something which might require endianness
handling, but if you put this as CPU native one and leave user space
to take care of, it may be okay.

> +
> +               iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
> +       }
> +
> +       if (overflow) {

> +               dev_warn_ratelimited(&data->client->dev,
> +                                    "FIFO overflow, samples may be lost\n");
> +               return mpl3115_fifo_reset(data);
> +       }
> +
> +       return 0;
> +}
> +
> +static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +       struct mpl3115_data *data = iio_priv(indio_dev);

> +       value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);

Why not clamp()?

> +       guard(mutex)(&data->lock);
> +       data->watermark = value;
> +
> +       return 0;
> +}

...

> +static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +       struct mpl3115_data *data = iio_priv(indio_dev);
> +       u8 ctrl_reg4;
> +       int ret;
> +
> +       guard(mutex)(&data->lock);
> +
> +       ret = mpl3115_set_active(data, false);
> +       if (ret)
> +               return ret;
> +
> +       ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> +       if (ret) {
> +               mpl3115_set_active(data, true);

Instead of having an argument, just make two helpers: set_active() /
set_inactive().

> +               return ret;
> +       }

> +       ctrl_reg4 = data->ctrl_reg4;
> +       ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
> +       ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> +
> +       return mpl3115_config_interrupt(data,
> +               data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);

In some cases the FIELD_PREP()/FIELD_GET() is used, why not use FIELD_MODIFY()?

> +}

...

> +static const unsigned long mpl3115_scan_masks[] = {
> +       BIT(0) | BIT(1),
> +       0,

No comma in terminator entry.

> +};

...

> -       ret = devm_iio_triggered_buffer_setup(&client->dev,
> -                                     indio_dev,
> -                                     NULL,
> -                                     mpl3115_trigger_handler,
> -                                     NULL);
> -       if (ret)
> -               return ret;

Before modifying this, it might make sense to split necessary bits to
the helper function and then simply reuse it as needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[Jonathan Cameron]

On Sat, 30 May 2026 15:32:23 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
> >
> > Add support for the MPL3115A2 hardware FIFO.
> >
> > The device provides a 32-sample FIFO with configurable
> > watermark interrupts, allowing buffered data acquisition
> > with reduced interrupt and CPU overhead.
> >
> > Use a kfifo-backed IIO buffer when a DRDY interrupt and
> > SMBus block reads are available, falling back to the
> > existing triggered buffer implementation otherwise.
> >
> > When enabled, the driver configures the FIFO in circular
> > mode and drains accumulated samples on watermark
> > interrupts.
> >
> > Overflow conditions are handled by flushing available
> > samples before resetting the FIFO, avoiding unnecessary
> > data loss.
> >
> > The hardware always captures pressure and temperature
> > samples together, so FIFO operation requires both channels
> > to be enabled.
> >
> > Register cache updates are performed only after successful
> > I2C writes to keep cached and hardware state consistent.
> >
> > No functional change for non-buffered operation.  
> 
Some fun stuff that Andy has picked out here, so a few comment on top
(I was wondering some of the same things!)
> ...
> 
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);  
> 
> We have a macro for this,
Which one?  We have the buffer + timestamp ones for IIO but that' not
relevant here.
> 
> ...
> 
> > +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> > +{
> > +       struct mpl3115_data *data = iio_priv(indio_dev);
> > +       u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };  
> 
> Don't we have a macro for this?
This one seems more likely to be one we cover IIO_DECLARE_BUFFER_WITH_TS()

However, looks like the ts is always in the same place and all channels are
captured so I'd prefer this as something like

	struct {
		__be32 pressure;
		__be16 temp;
//there is a gap here hence need to force initialization to avoid stack content leaking.
		aligned_s64 ts;
	} scan = { };

> 
> > +       unsigned int sample_count, i;
> > +       int ret;
> > +       bool overflow;
> > +       s64 ts;
> > +
> > +       ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> > +       sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> > +       if (sample_count == 0)
> > +               return overflow ? mpl3115_fifo_reset(data) : 0;
> > +
> > +       ret = mpl3115_fifo_transfer(data, sample_count);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ts = iio_get_time_ns(indio_dev);
> > +       for (i = 0; i < sample_count; i++) {  
> 
>   for (unsigned int i = 0; ...) {
> 
> > +               u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];  
> 
> > +               memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +               memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);  
> 
> These two sound to me like something which might require endianness
> handling, but if you put this as CPU native one and leave user space
> to take care of, it may be okay.

They are big endian channels so this 'should' be fine but we should make that explicit.
Not the chan_spec for the 20 bit read has a shift of 12 to get around the last byte
not being filled by this memcpy.

(other stuff Andy pointed out cropped)

J

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[Andy Shevchenko]

On Sat, May 30, 2026 at 04:43:06PM +0100, Jonathan Cameron wrote:
> On Sat, 30 May 2026 15:32:23 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:

...

> > > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > > +               __aligned(IIO_DMA_MINALIGN);  
> > 
> > We have a macro for this,
> Which one?  We have the buffer + timestamp ones for IIO but that' not
> relevant here.

Ah, I stand corrected!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[SeungJu Cheon]

Hi Andy,

On Sat, May 30, 2026 at 10:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);
>
> We have a macro for this,
...
> > +       u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> Don't we have a macro for this?

I'll rework the per-sample buffer as a struct with explicit fields
(__be32 pressure, __be16 temp, aligned_s64 ts), as Jonathan suggested,
which removes the manual offsets and alignment here.

> > +       ts = iio_get_time_ns(indio_dev);
> > +       for (i = 0; i < sample_count; i++) {
>
>   for (unsigned int i = 0; ...) {

Will declare i in the loop.

> > +               memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +               memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
>
> These two sound to me like something which might require endianness
> handling, but if you put this as CPU native one and leave user space
> to take care of, it may be okay.

These are big-endian channels (scan_type.endianness = IIO_BE), pushed
as-is for user space to decode. The struct rework will make that
explicit with __be32/__be16 fields.

> > +       value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
>
> Why not clamp()?

Will use clamp().

> > +               mpl3115_set_active(data, true);
>
> Instead of having an argument, just make two helpers: set_active() /
> set_inactive().

Will split into mpl3115_set_active() / mpl3115_set_inactive().

> > +       return mpl3115_config_interrupt(data,
> > +               data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
>
> In some cases the FIELD_PREP()/FIELD_GET() is used, why not use FIELD_MODIFY()?

I'll review the FIELD_MODIFY() usage and apply it where appropriate.

> > +static const unsigned long mpl3115_scan_masks[] = {
> > +       BIT(0) | BIT(1),
> > +       0,
>
> No comma in terminator entry.

Will fix.

> > -       ret = devm_iio_triggered_buffer_setup(&client->dev,
> > [...]
>
> Before modifying this, it might make sense to split necessary bits to
> the helper function and then simply reuse it as needed.

Makes sense. I'll look into factoring the common setup into a helper
so the FIFO and triggered-buffer paths can reuse it.

Thanks for the review.

On Sat, May 30, 2026 at 10:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> wrote:
> >
> > Add support for the MPL3115A2 hardware FIFO.
> >
> > The device provides a 32-sample FIFO with configurable
> > watermark interrupts, allowing buffered data acquisition
> > with reduced interrupt and CPU overhead.
> >
> > Use a kfifo-backed IIO buffer when a DRDY interrupt and
> > SMBus block reads are available, falling back to the
> > existing triggered buffer implementation otherwise.
> >
> > When enabled, the driver configures the FIFO in circular
> > mode and drains accumulated samples on watermark
> > interrupts.
> >
> > Overflow conditions are handled by flushing available
> > samples before resetting the FIFO, avoiding unnecessary
> > data loss.
> >
> > The hardware always captures pressure and temperature
> > samples together, so FIFO operation requires both channels
> > to be enabled.
> >
> > Register cache updates are performed only after successful
> > I2C writes to keep cached and hardware state consistent.
> >
> > No functional change for non-buffered operation.
>
> ...
>
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);
>
> We have a macro for this,
>
> ...
>
> > +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> > +{
> > +       struct mpl3115_data *data = iio_priv(indio_dev);
> > +       u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> Don't we have a macro for this?
>
> > +       unsigned int sample_count, i;
> > +       int ret;
> > +       bool overflow;
> > +       s64 ts;
> > +
> > +       ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> > +       sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> > +       if (sample_count == 0)
> > +               return overflow ? mpl3115_fifo_reset(data) : 0;
> > +
> > +       ret = mpl3115_fifo_transfer(data, sample_count);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ts = iio_get_time_ns(indio_dev);
> > +       for (i = 0; i < sample_count; i++) {
>
>   for (unsigned int i = 0; ...) {
>
> > +               u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];
>
> > +               memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +               memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
>
> These two sound to me like something which might require endianness
> handling, but if you put this as CPU native one and leave user space
> to take care of, it may be okay.
>
> > +
> > +               iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
> > +       }
> > +
> > +       if (overflow) {
>
> > +               dev_warn_ratelimited(&data->client->dev,
> > +                                    "FIFO overflow, samples may be lost\n");
> > +               return mpl3115_fifo_reset(data);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +       struct mpl3115_data *data = iio_priv(indio_dev);
>
> > +       value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
>
> Why not clamp()?
>
> > +       guard(mutex)(&data->lock);
> > +       data->watermark = value;
> > +
> > +       return 0;
> > +}
>
> ...
>
> > +static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +       struct mpl3115_data *data = iio_priv(indio_dev);
> > +       u8 ctrl_reg4;
> > +       int ret;
> > +
> > +       guard(mutex)(&data->lock);
> > +
> > +       ret = mpl3115_set_active(data, false);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> > +       if (ret) {
> > +               mpl3115_set_active(data, true);
>
> Instead of having an argument, just make two helpers: set_active() /
> set_inactive().
>
> > +               return ret;
> > +       }
>
> > +       ctrl_reg4 = data->ctrl_reg4;
> > +       ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
> > +       ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > +
> > +       return mpl3115_config_interrupt(data,
> > +               data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
>
> In some cases the FIELD_PREP()/FIELD_GET() is used, why not use FIELD_MODIFY()?
>
> > +}
>
> ...
>
> > +static const unsigned long mpl3115_scan_masks[] = {
> > +       BIT(0) | BIT(1),
> > +       0,
>
> No comma in terminator entry.
>
> > +};
>
> ...
>
> > -       ret = devm_iio_triggered_buffer_setup(&client->dev,
> > -                                     indio_dev,
> > -                                     NULL,
> > -                                     mpl3115_trigger_handler,
> > -                                     NULL);
> > -       if (ret)
> > -               return ret;
>
> Before modifying this, it might make sense to split necessary bits to
> the helper function and then simply reuse it as needed.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[Jonathan Cameron]

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

> Add support for the MPL3115A2 hardware FIFO.
> 
> The device provides a 32-sample FIFO with configurable
> watermark interrupts, allowing buffered data acquisition
> with reduced interrupt and CPU overhead.
> 
> Use a kfifo-backed IIO buffer when a DRDY interrupt and
> SMBus block reads are available, falling back to the
> existing triggered buffer implementation otherwise.
> 
> When enabled, the driver configures the FIFO in circular
> mode and drains accumulated samples on watermark
> interrupts.
> 
> Overflow conditions are handled by flushing available
> samples before resetting the FIFO, avoiding unnecessary
> data loss.
> 
> The hardware always captures pressure and temperature
> samples together, so FIFO operation requires both channels
> to be enabled.
> 
> Register cache updates are performed only after successful
> I2C writes to keep cached and hardware state consistent.
> 
> No functional change for non-buffered operation.
> 
> Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>

Various comments inline to add to what Andy pointed out.

Note that even though you've had a couple of quick reviews wait at least a few
days / 1 week to see if others have review comments before doing a new version.
Any discussion will probably have finished by then.

(I can't speak for Andy, but in the UK it's too hot to do much outside
today - so I'm bored!)

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/mpl3115.c | 266 +++++++++++++++++++++++++++++++--
>  1 file changed, 256 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 90e83e34ec43..26b6490c8c4c 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c

> +#define MPL3115_BUF_PRESS_OFFSET 0
> +#define MPL3115_BUF_TEMP_OFFSET 4
> +#define MPL3115_BUF_TS_OFFSET 8
> +#define MPL3115_BUF_SIZE (MPL3115_BUF_TS_OFFSET + sizeof(s64))
> +
> +#define MPL3115_FIFO_SAMPLES_PER_READ 6U
comment on why 6.
> +
>  #define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
>  
>  #define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> @@ -63,8 +89,12 @@
>  #define MPL3115_CTRL3_IPOL2 BIT(1)
>  
>  #define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> +#define MPL3115_CTRL4_INT_EN_FIFO BIT(6)
>  #define MPL3115_CTRL4_INT_EN_PTH BIT(3)
>  #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
> +#define MPL3115_CTRL4_ACTIVE_MASK \
> +	(MPL3115_CTRL4_INT_EN_DRDY | \
> +	 MPL3115_CTRL4_INT_EN_PTH | MPL3115_CTRL4_INT_EN_TTH)
>  
>  #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
>  #define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
> @@ -94,6 +124,9 @@ struct mpl3115_data {
>  	struct mutex lock;
>  	u8 ctrl_reg1;
>  	u8 ctrl_reg4;
> +	u8 watermark;
> +	u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> +		__aligned(IIO_DMA_MINALIGN);
>  };
>  
>  enum mpl3115_irq_pin {
> @@ -101,6 +134,25 @@ enum mpl3115_irq_pin {
>  	MPL3115_IRQ_INT2,
>  };
>  
> +static int mpl3115_set_active(struct mpl3115_data *data, bool active)
> +{
> +	u8 reg;
> +	int ret;
> +
> +	if (active)
> +		reg = data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE;
> +	else
> +		reg = data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> +					reg);
One line.  Or as Andy suggested split this in two, then the reg local
variable isn't needed.

> +	if (ret)
> +		return ret;
> +
> +	data->ctrl_reg1 = reg;
> +	return 0;
> +}
> +
>  static int mpl3115_request(struct mpl3115_data *data)
>  {
>  	int ret, tries = 15;
> @@ -322,6 +374,109 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int mpl3115_set_fifo(struct mpl3115_data *data,
> +			    enum mpl3115_fifo_mode mode)
> +{
> +	u8 f_setup;
> +
> +	f_setup = FIELD_PREP(MPL3115_F_SETUP_F_MODE, mode) |
> +		  FIELD_PREP(MPL3115_F_SETUP_F_WMRK, data->watermark);
> +
> +	return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
> +			f_setup);

Align after (  Or just go a bit long and put it one line.


> +}
> +
> +static int mpl3115_fifo_reset(struct mpl3115_data *data)
> +{
> +	int ret;
> +
> +	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> +	if (ret)
> +		return ret;
> +
> +	return mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> +}
> +
> +static int mpl3115_fifo_transfer(struct mpl3115_data *data,
> +				 unsigned int sample_count)
> +{
> +	unsigned int remaining = sample_count;
> +	unsigned int offset = 0;
> +	unsigned int batch;
> +	int ret;
> +
> +	while (remaining > 0) {

Don't think this can got negative. I'd make that explicit as != 0

> +		batch = min(remaining, MPL3115_FIFO_SAMPLES_PER_READ);
> +
> +		ret = i2c_smbus_read_i2c_block_data(data->client,
> +						    MPL3115_OUT_PRESS,
> +						    batch * MPL3115_FIFO_SAMPLE_SIZE,
> +						    &data->fifo_buf[offset]);
Given this is reading up to 6 samples, it feels odd to just keep reading without
pushing any of them to the buffer. I'd rework the logic so for each iteration
of the fifo we push the data we have. That will reduce the storage requirement
quite a bit.
> +		if (ret < 0)
> +			return ret;
> +		if (ret != batch * MPL3115_FIFO_SAMPLE_SIZE)
> +			return -EIO;
> +
> +		offset += batch * MPL3115_FIFO_SAMPLE_SIZE;
> +		remaining -= batch;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };

This is only used int he loop below. Declare it down there with
an appropriate struct (see reply I sent to Andy's review)

> +	unsigned int sample_count, i;
> +	int ret;
> +	bool overflow;
> +	s64 ts;
When no other reason for ordering, reverse xmas tree preferred in IIO.


See below. I'd pull the guard() in here.

> +
> +	ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> +	sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> +	if (sample_count == 0)

Add a comment on what overflow with no samples means. That seems
unusual.

> +		return overflow ? mpl3115_fifo_reset(data) : 0;
> +
> +	ret = mpl3115_fifo_transfer(data, sample_count);
> +	if (ret)
> +		return ret;
> +
> +	ts = iio_get_time_ns(indio_dev);

This data is coming from a fifo. How is the time grabbed here relevant?

Timestamp + fifo handling is hard. See how the invensense IMU drivers
do it.  For 1st version of this you may have to just fail the buffer
enable if the timestamp is turned on.  I'd revisit timestamps after the
rest is upstream (or at least as additional patches).

> +	for (i = 0; i < sample_count; i++) {
> +		u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];
> +
> +		memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> +		memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
> +
> +		iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
> +	}
> +
> +	if (overflow) {
> +		dev_warn_ratelimited(&data->client->dev,
> +				     "FIFO overflow, samples may be lost\n");
> +		return mpl3115_fifo_reset(data);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +
> +	value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
> +
> +	guard(mutex)(&data->lock);
> +	data->watermark = value;
> +
> +	return 0;
> +}
> +
>  static int mpl3115_config_interrupt(struct mpl3115_data *data,
>  				    u8 ctrl_reg1, u8 ctrl_reg4)
>  {
> @@ -348,6 +503,67 @@ static int mpl3115_config_interrupt(struct mpl3115_data *data,
>  	return ret;
>  }
>  
> +static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 ctrl_reg4;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = mpl3115_set_active(data, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> +	if (ret) {
> +		mpl3115_set_active(data, true);
> +		return ret;
> +	}
> +
> +	ctrl_reg4 = data->ctrl_reg4;
> +	ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
> +	ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> +
> +	return mpl3115_config_interrupt(data,
> +		data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
> +}
> +
> +static int mpl3115_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 ctrl_reg1, ctrl_reg4;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = mpl3115_set_active(data, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> +	if (ret)
> +		return ret;
> +
> +	ctrl_reg4 = data->ctrl_reg4 & ~MPL3115_CTRL4_INT_EN_FIFO;
> +	ctrl_reg1 = data->ctrl_reg1;
> +
> +	if (ctrl_reg4 & MPL3115_CTRL4_ACTIVE_MASK)
> +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> +
> +	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> +}
> +
> +static const struct iio_buffer_setup_ops mpl3115_buffer_ops = {
> +	.postenable = mpl3115_buffer_postenable,
> +	.predisable = mpl3115_buffer_predisable,
> +};
> +
> +static const unsigned long mpl3115_scan_masks[] = {
> +	BIT(0) | BIT(1),
Maybe worth introducing an enum for those bits (use in the iio_chan_spec array elements
 as well)
> +	0,
No comma (Andy got this already)
> +};
> +
>  static const struct iio_event_spec mpl3115_temp_press_event[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -409,10 +625,19 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
>  	if (ret < 0)
>  		return IRQ_NONE;
>  
> -	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> -		     MPL3115_INT_SRC_DRDY)))
> +	if (!(ret & (MPL3115_INT_SRC_FIFO | MPL3115_INT_SRC_TTH |
> +		     MPL3115_INT_SRC_PTH | MPL3115_INT_SRC_DRDY)))
>  		return IRQ_NONE;
>  
> +	if (ret & MPL3115_INT_SRC_FIFO) {
> +		int flush_ret;
> +
> +		scoped_guard(mutex, &data->lock)
> +			flush_ret = mpl3115_fifo_flush(indio_dev);
> +		if (flush_ret < 0)
So this is technically not a bug but it runs into the basic argument that if you are
using automatic cleanup you shouldn't be using gotos in the same function
(says that in comments in cleanup.h - Linus was very explicit about this when people
 1st starting using these functions!)

With it pushed into the function though that doesn't matter.

> +			goto err_reset_fifo;
> +	}
> +
>  	if (ret & MPL3115_INT_SRC_DRDY)
>  		iio_trigger_poll_nested(data->drdy_trig);
>  
> @@ -444,6 +669,11 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
>  	}
>  
>  	return IRQ_HANDLED;
> +
> +err_reset_fifo:
> +	scoped_guard(mutex, &data->lock)

Hmm. This is trickier given fifo_reset is called in fifo_flush.
Maybe a wrapper and guard()  The none guard version would typically use
__mpl3115_fifo_reset() naming.


> +		mpl3115_fifo_reset(data);
> +	return IRQ_HANDLED;
>  }

>  static int mpl3115_trigger_probe(struct mpl3115_data *data,
> @@ -723,6 +954,7 @@ static int mpl3115_probe(struct i2c_client *client)
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	struct mpl3115_data *data;
>  	struct iio_dev *indio_dev;
> +	bool use_fifo;
>  	int ret;
>  
>  	ret = i2c_smbus_read_byte_data(client, MPL3115_WHO_AM_I);
> @@ -762,17 +994,31 @@ static int mpl3115_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	data->watermark = MPL3115_DEFAULT_WATERMARK;
> +
>  	ret = mpl3115_trigger_probe(data, indio_dev);
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_iio_triggered_buffer_setup(&client->dev,
> -				      indio_dev,
> -				      NULL,
> -				      mpl3115_trigger_handler,
> -				      NULL);
> -	if (ret)
> -		return ret;
> +	use_fifo = data->drdy_trig &&
> +	   i2c_check_functionality(client->adapter,
> +				   I2C_FUNC_SMBUS_READ_I2C_BLOCK);

Hmm. So this is always a fun thing to work out how to handle.
Gating on a particular i2c controller function isn't appropriate.
More to the point existing code is relying on that being available.

static int mpl3115_fill_trig_buffer(struct iio_dev *indio_dev, u8 *buffer)
{
	struct mpl3115_data *data = iio_priv(indio_dev);
	int ret, pos = 0;

	if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
		ret = mpl3115_request(data);
		if (ret < 0)
			return ret;
	}

	if (test_bit(0, indio_dev->active_scan_mask)) {
		ret = i2c_smbus_read_i2c_block_data(data->client,
			MPL3115_OUT_PRESS, 3, &buffer[pos]);
		if (ret < 0)
			return ret;
		pos += 4;
	}

	if (test_bit(1, indio_dev->active_scan_mask)) {
		ret = i2c_smbus_read_i2c_block_data(data->client,
			MPL3115_OUT_TEMP, 2, &buffer[pos]);
		if (ret < 0)
			return ret;
	}

	return 0;
}

It's hard to retrofit a fifo and not end up with a weird control
interface. We can't remove the triggered buffer because that will be
a regression for existing users.

So the trick we have done in the past is to enable the fifo whenever
a trigger is not set, but the buffer enabled.

> +
> +	if (use_fifo) {
> +		indio_dev->available_scan_masks = mpl3115_scan_masks;
Given other changes I'm suggesting we have nothing to gate that on.
2 options:
A) take the view that's not that costly and optimize the exiting triggered
   capture for that case (as a precursor)
B) Do manual data mangling so that you don't need this.  That is just
   fill the buffer with enabled channels (that makes a mess of using a struct
   though as the first channel type changes depending on what is enabled).

I'd do (A)

> +		ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> +						  &mpl3115_buffer_ops);
Don't do this - instead...
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = devm_iio_triggered_buffer_setup(&client->dev,
> +						      indio_dev,
> +						      NULL,
> +						      mpl3115_trigger_handler,
> +						      NULL);

Do this unconditionally. It will register a kfifo so we can use that
anyway, but will also have the interface we can't remove (without it
being an ABI regression) to set the trigger.

> +		if (ret)
> +			return ret;
> +	}
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }

Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

[SeungJu Cheon]

Hi Jonathan,

On Sat, 30 May 2026 01:05 ... Jonathan Cameron wrote:
> Note that even though you've had a couple of quick reviews wait at
> least a few days / 1 week to see if others have review comments
> before doing a new version.

Will do. I'll hold v2 for a week or so to let any further comments
come in.

> > +#define MPL3115_FIFO_SAMPLES_PER_READ 6U
>
> comment on why 6.

Will add a comment explaining the 6.

> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
> > +                     f_setup);
>
> Align after ( Or just go a bit long and put it one line.

Will put it on one line.

> > +     while (remaining > 0) {
>
> Don't think this can got negative. I'd make that explicit as != 0

Will change to != 0.

> > +             ret = i2c_smbus_read_i2c_block_data(data->client,
> > [...]
>
> Given this is reading up to 6 samples, it feels odd to just keep
> reading without pushing any of them to the buffer. I'd rework the
> logic so for each iteration of the fifo we push the data we have.
> That will reduce the storage requirement quite a bit.

Good point. I'll push samples as I read each batch instead of
buffering them all first, which also shrinks fifo_buf considerably.

> > +     u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> This is only used in the loop below. Declare it down there with
> an appropriate struct (see reply I sent to Andy's review)

Will move it into the loop as a struct with __be32/__be16 fields and
an aligned s64 timestamp.

> > +     unsigned int sample_count, i;
> > +     int ret;
> > +     bool overflow;
> > +     s64 ts;
>
> When no other reason for ordering, reverse xmas tree preferred in
> IIO.

Will reorder.

> > +     if (sample_count == 0)
>
> Add a comment on what overflow with no samples means. That seems
> unusual.

Will add a comment for the overflow-with-no-samples case.

> > +     ts = iio_get_time_ns(indio_dev);
>
> This data is coming from a fifo. How is the time grabbed here
> relevant?
> Timestamp + fifo handling is hard. See how the invensense IMU
> drivers do it. For 1st version of this you may have to just fail the
> buffer enable if the timestamp is turned on. I'd revisit timestamps
> after the rest is upstream (or at least as additional patches).

Agreed, stamping FIFO samples with the current time isn't ideal. For
v2 I'll reject buffer enable if the timestamp channel is on, and leave
proper FIFO timestamping (along the lines of the invensense drivers)
to a follow-up once the rest is in.

> Maybe worth introducing an enum for those bits (use in the
> iio_chan_spec array elements as well)
> > +     0,
> No comma (Andy got this already)

Will drop the comma and introduce an enum for the scan indices, reused
in the channel specs.

> > +             if (flush_ret < 0)
> > +                     goto err_reset_fifo;
> > [...]
> > +err_reset_fifo:
> > +     scoped_guard(mutex, &data->lock)
>
> So this is technically not a bug but it runs into the basic argument
> that if you are using automatic cleanup you shouldn't be using gotos
> in the same function [...]
> Hmm. This is trickier given fifo_reset is called in fifo_flush.
> Maybe a wrapper and guard() The none guard version would typically
> use __mpl3115_fifo_reset() naming.

Makes sense. I'll push the locking into the functions and use guard()
there, with a __mpl3115_fifo_reset() helper for the already-locked
caller, so the handler no longer mixes scoped_guard() with a goto.

> It's hard to retrofit a fifo and not end up with a weird control
> interface. We can't remove the triggered buffer because that will be
> a regression for existing users.
> So the trick we have done in the past is to enable the fifo whenever
> a trigger is not set, but the buffer enabled.
> [...]
> Do this unconditionally. It will register a kfifo so we can use that
> anyway, but will also have the interface we can't remove [...]

Understood. I'll drop the use_fifo gating and keep the triggered
buffer registration unconditional, then select the FIFO path from
postenable when no trigger is configured. I'll try option (A) by first
making the triggered capture always read both channels, so the FIFO
path can avoid introducing available_scan_masks.

Thanks very much for the detailed review.

On Sun, May 31, 2026 at 1:05 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 May 2026 20:39:38 +0900
> SeungJu Cheon <suunj1331@gmail.com> wrote:
>
> > Add support for the MPL3115A2 hardware FIFO.
> >
> > The device provides a 32-sample FIFO with configurable
> > watermark interrupts, allowing buffered data acquisition
> > with reduced interrupt and CPU overhead.
> >
> > Use a kfifo-backed IIO buffer when a DRDY interrupt and
> > SMBus block reads are available, falling back to the
> > existing triggered buffer implementation otherwise.
> >
> > When enabled, the driver configures the FIFO in circular
> > mode and drains accumulated samples on watermark
> > interrupts.
> >
> > Overflow conditions are handled by flushing available
> > samples before resetting the FIFO, avoiding unnecessary
> > data loss.
> >
> > The hardware always captures pressure and temperature
> > samples together, so FIFO operation requires both channels
> > to be enabled.
> >
> > Register cache updates are performed only after successful
> > I2C writes to keep cached and hardware state consistent.
> >
> > No functional change for non-buffered operation.
> >
> > Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
>
> Various comments inline to add to what Andy pointed out.
>
> Note that even though you've had a couple of quick reviews wait at least a few
> days / 1 week to see if others have review comments before doing a new version.
> Any discussion will probably have finished by then.
>
> (I can't speak for Andy, but in the UK it's too hot to do much outside
> today - so I'm bored!)
>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 266 +++++++++++++++++++++++++++++++--
> >  1 file changed, 256 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 90e83e34ec43..26b6490c8c4c 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
>
> > +#define MPL3115_BUF_PRESS_OFFSET 0
> > +#define MPL3115_BUF_TEMP_OFFSET 4
> > +#define MPL3115_BUF_TS_OFFSET 8
> > +#define MPL3115_BUF_SIZE (MPL3115_BUF_TS_OFFSET + sizeof(s64))
> > +
> > +#define MPL3115_FIFO_SAMPLES_PER_READ 6U
> comment on why 6.
> > +
> >  #define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> >
> >  #define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> > @@ -63,8 +89,12 @@
> >  #define MPL3115_CTRL3_IPOL2 BIT(1)
> >
> >  #define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> > +#define MPL3115_CTRL4_INT_EN_FIFO BIT(6)
> >  #define MPL3115_CTRL4_INT_EN_PTH BIT(3)
> >  #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
> > +#define MPL3115_CTRL4_ACTIVE_MASK \
> > +     (MPL3115_CTRL4_INT_EN_DRDY | \
> > +      MPL3115_CTRL4_INT_EN_PTH | MPL3115_CTRL4_INT_EN_TTH)
> >
> >  #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> >  #define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
> > @@ -94,6 +124,9 @@ struct mpl3115_data {
> >       struct mutex lock;
> >       u8 ctrl_reg1;
> >       u8 ctrl_reg4;
> > +     u8 watermark;
> > +     u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +             __aligned(IIO_DMA_MINALIGN);
> >  };
> >
> >  enum mpl3115_irq_pin {
> > @@ -101,6 +134,25 @@ enum mpl3115_irq_pin {
> >       MPL3115_IRQ_INT2,
> >  };
> >
> > +static int mpl3115_set_active(struct mpl3115_data *data, bool active)
> > +{
> > +     u8 reg;
> > +     int ret;
> > +
> > +     if (active)
> > +             reg = data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE;
> > +     else
> > +             reg = data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE;
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                     reg);
> One line.  Or as Andy suggested split this in two, then the reg local
> variable isn't needed.
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->ctrl_reg1 = reg;
> > +     return 0;
> > +}
> > +
> >  static int mpl3115_request(struct mpl3115_data *data)
> >  {
> >       int ret, tries = 15;
> > @@ -322,6 +374,109 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static int mpl3115_set_fifo(struct mpl3115_data *data,
> > +                         enum mpl3115_fifo_mode mode)
> > +{
> > +     u8 f_setup;
> > +
> > +     f_setup = FIELD_PREP(MPL3115_F_SETUP_F_MODE, mode) |
> > +               FIELD_PREP(MPL3115_F_SETUP_F_WMRK, data->watermark);
> > +
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
> > +                     f_setup);
>
> Align after (  Or just go a bit long and put it one line.
>
>
> > +}
> > +
> > +static int mpl3115_fifo_reset(struct mpl3115_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> > +}
> > +
> > +static int mpl3115_fifo_transfer(struct mpl3115_data *data,
> > +                              unsigned int sample_count)
> > +{
> > +     unsigned int remaining = sample_count;
> > +     unsigned int offset = 0;
> > +     unsigned int batch;
> > +     int ret;
> > +
> > +     while (remaining > 0) {
>
> Don't think this can got negative. I'd make that explicit as != 0
>
> > +             batch = min(remaining, MPL3115_FIFO_SAMPLES_PER_READ);
> > +
> > +             ret = i2c_smbus_read_i2c_block_data(data->client,
> > +                                                 MPL3115_OUT_PRESS,
> > +                                                 batch * MPL3115_FIFO_SAMPLE_SIZE,
> > +                                                 &data->fifo_buf[offset]);
> Given this is reading up to 6 samples, it feels odd to just keep reading without
> pushing any of them to the buffer. I'd rework the logic so for each iteration
> of the fifo we push the data we have. That will reduce the storage requirement
> quite a bit.
> > +             if (ret < 0)
> > +                     return ret;
> > +             if (ret != batch * MPL3115_FIFO_SAMPLE_SIZE)
> > +                     return -EIO;
> > +
> > +             offset += batch * MPL3115_FIFO_SAMPLE_SIZE;
> > +             remaining -= batch;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> This is only used int he loop below. Declare it down there with
> an appropriate struct (see reply I sent to Andy's review)
>
> > +     unsigned int sample_count, i;
> > +     int ret;
> > +     bool overflow;
> > +     s64 ts;
> When no other reason for ordering, reverse xmas tree preferred in IIO.
>
>
> See below. I'd pull the guard() in here.
>
> > +
> > +     ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> > +     sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> > +     if (sample_count == 0)
>
> Add a comment on what overflow with no samples means. That seems
> unusual.
>
> > +             return overflow ? mpl3115_fifo_reset(data) : 0;
> > +
> > +     ret = mpl3115_fifo_transfer(data, sample_count);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ts = iio_get_time_ns(indio_dev);
>
> This data is coming from a fifo. How is the time grabbed here relevant?
>
> Timestamp + fifo handling is hard. See how the invensense IMU drivers
> do it.  For 1st version of this you may have to just fail the buffer
> enable if the timestamp is turned on.  I'd revisit timestamps after the
> rest is upstream (or at least as additional patches).
>
> > +     for (i = 0; i < sample_count; i++) {
> > +             u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];
> > +
> > +             memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +             memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
> > +
> > +             iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
> > +     }
> > +
> > +     if (overflow) {
> > +             dev_warn_ratelimited(&data->client->dev,
> > +                                  "FIFO overflow, samples may be lost\n");
> > +             return mpl3115_fifo_reset(data);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +
> > +     value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
> > +
> > +     guard(mutex)(&data->lock);
> > +     data->watermark = value;
> > +
> > +     return 0;
> > +}
> > +
> >  static int mpl3115_config_interrupt(struct mpl3115_data *data,
> >                                   u8 ctrl_reg1, u8 ctrl_reg4)
> >  {
> > @@ -348,6 +503,67 @@ static int mpl3115_config_interrupt(struct mpl3115_data *data,
> >       return ret;
> >  }
> >
> > +static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 ctrl_reg4;
> > +     int ret;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = mpl3115_set_active(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> > +     if (ret) {
> > +             mpl3115_set_active(data, true);
> > +             return ret;
> > +     }
> > +
> > +     ctrl_reg4 = data->ctrl_reg4;
> > +     ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
> > +     ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > +
> > +     return mpl3115_config_interrupt(data,
> > +             data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
> > +}
> > +
> > +static int mpl3115_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 ctrl_reg1, ctrl_reg4;
> > +     int ret;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = mpl3115_set_active(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ctrl_reg4 = data->ctrl_reg4 & ~MPL3115_CTRL4_INT_EN_FIFO;
> > +     ctrl_reg1 = data->ctrl_reg1;
> > +
> > +     if (ctrl_reg4 & MPL3115_CTRL4_ACTIVE_MASK)
> > +             ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > +
> > +     return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> > +}
> > +
> > +static const struct iio_buffer_setup_ops mpl3115_buffer_ops = {
> > +     .postenable = mpl3115_buffer_postenable,
> > +     .predisable = mpl3115_buffer_predisable,
> > +};
> > +
> > +static const unsigned long mpl3115_scan_masks[] = {
> > +     BIT(0) | BIT(1),
> Maybe worth introducing an enum for those bits (use in the iio_chan_spec array elements
>  as well)
> > +     0,
> No comma (Andy got this already)
> > +};
> > +
> >  static const struct iio_event_spec mpl3115_temp_press_event[] = {
> >       {
> >               .type = IIO_EV_TYPE_THRESH,
> > @@ -409,10 +625,19 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >       if (ret < 0)
> >               return IRQ_NONE;
> >
> > -     if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> > -                  MPL3115_INT_SRC_DRDY)))
> > +     if (!(ret & (MPL3115_INT_SRC_FIFO | MPL3115_INT_SRC_TTH |
> > +                  MPL3115_INT_SRC_PTH | MPL3115_INT_SRC_DRDY)))
> >               return IRQ_NONE;
> >
> > +     if (ret & MPL3115_INT_SRC_FIFO) {
> > +             int flush_ret;
> > +
> > +             scoped_guard(mutex, &data->lock)
> > +                     flush_ret = mpl3115_fifo_flush(indio_dev);
> > +             if (flush_ret < 0)
> So this is technically not a bug but it runs into the basic argument that if you are
> using automatic cleanup you shouldn't be using gotos in the same function
> (says that in comments in cleanup.h - Linus was very explicit about this when people
>  1st starting using these functions!)
>
> With it pushed into the function though that doesn't matter.
>
> > +                     goto err_reset_fifo;
> > +     }
> > +
> >       if (ret & MPL3115_INT_SRC_DRDY)
> >               iio_trigger_poll_nested(data->drdy_trig);
> >
> > @@ -444,6 +669,11 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >       }
> >
> >       return IRQ_HANDLED;
> > +
> > +err_reset_fifo:
> > +     scoped_guard(mutex, &data->lock)
>
> Hmm. This is trickier given fifo_reset is called in fifo_flush.
> Maybe a wrapper and guard()  The none guard version would typically use
> __mpl3115_fifo_reset() naming.
>
>
> > +             mpl3115_fifo_reset(data);
> > +     return IRQ_HANDLED;
> >  }
>
> >  static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > @@ -723,6 +954,7 @@ static int mpl3115_probe(struct i2c_client *client)
> >       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> >       struct mpl3115_data *data;
> >       struct iio_dev *indio_dev;
> > +     bool use_fifo;
> >       int ret;
> >
> >       ret = i2c_smbus_read_byte_data(client, MPL3115_WHO_AM_I);
> > @@ -762,17 +994,31 @@ static int mpl3115_probe(struct i2c_client *client)
> >       if (ret)
> >               return ret;
> >
> > +     data->watermark = MPL3115_DEFAULT_WATERMARK;
> > +
> >       ret = mpl3115_trigger_probe(data, indio_dev);
> >       if (ret)
> >               return ret;
> >
> > -     ret = devm_iio_triggered_buffer_setup(&client->dev,
> > -                                   indio_dev,
> > -                                   NULL,
> > -                                   mpl3115_trigger_handler,
> > -                                   NULL);
> > -     if (ret)
> > -             return ret;
> > +     use_fifo = data->drdy_trig &&
> > +        i2c_check_functionality(client->adapter,
> > +                                I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>
> Hmm. So this is always a fun thing to work out how to handle.
> Gating on a particular i2c controller function isn't appropriate.
> More to the point existing code is relying on that being available.
>
> static int mpl3115_fill_trig_buffer(struct iio_dev *indio_dev, u8 *buffer)
> {
>         struct mpl3115_data *data = iio_priv(indio_dev);
>         int ret, pos = 0;
>
>         if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
>                 ret = mpl3115_request(data);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         if (test_bit(0, indio_dev->active_scan_mask)) {
>                 ret = i2c_smbus_read_i2c_block_data(data->client,
>                         MPL3115_OUT_PRESS, 3, &buffer[pos]);
>                 if (ret < 0)
>                         return ret;
>                 pos += 4;
>         }
>
>         if (test_bit(1, indio_dev->active_scan_mask)) {
>                 ret = i2c_smbus_read_i2c_block_data(data->client,
>                         MPL3115_OUT_TEMP, 2, &buffer[pos]);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         return 0;
> }
>
> It's hard to retrofit a fifo and not end up with a weird control
> interface. We can't remove the triggered buffer because that will be
> a regression for existing users.
>
> So the trick we have done in the past is to enable the fifo whenever
> a trigger is not set, but the buffer enabled.
>
> > +
> > +     if (use_fifo) {
> > +             indio_dev->available_scan_masks = mpl3115_scan_masks;
> Given other changes I'm suggesting we have nothing to gate that on.
> 2 options:
> A) take the view that's not that costly and optimize the exiting triggered
>    capture for that case (as a precursor)
> B) Do manual data mangling so that you don't need this.  That is just
>    fill the buffer with enabled channels (that makes a mess of using a struct
>    though as the first channel type changes depending on what is enabled).
>
> I'd do (A)
>
> > +             ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > +                                               &mpl3115_buffer_ops);
> Don't do this - instead...
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                                   indio_dev,
> > +                                                   NULL,
> > +                                                   mpl3115_trigger_handler,
> > +                                                   NULL);
>
> Do this unconditionally. It will register a kfifo so we can use that
> anyway, but will also have the interface we can't remove (without it
> being an ABI regression) to set the trigger.
>
> > +             if (ret)
> > +                     return ret;
> > +     }
> >
> >       return devm_iio_device_register(&client->dev, indio_dev);
> >  }
>