MPL3115 sensor features a "data ready" interrupt which indicates the
presence of new measurements.
Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-
1 file changed, 162 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 579da60ef441..cf34de8f0d7e 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -7,7 +7,7 @@
* (7-bit I2C slave address 0x60)
*
* TODO: FIFO buffer, altimeter mode, oversampling, continuous mode,
- * interrupts, user offset correction, raw mode
+ * user offset correction, raw mode
*/
#include <linux/module.h>
@@ -17,26 +17,45 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/buffer.h>
#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
#include <linux/delay.h>
+#include <linux/property.h>
#define MPL3115_STATUS 0x00
#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_INT_SOURCE 0x12
+#define MPL3115_PT_DATA_CFG 0x13
#define MPL3115_CTRL_REG1 0x26
+#define MPL3115_CTRL_REG3 0x28
+#define MPL3115_CTRL_REG4 0x29
+#define MPL3115_CTRL_REG5 0x2a
#define MPL3115_DEVICE_ID 0xc4
#define MPL3115_STATUS_PRESS_RDY BIT(2)
#define MPL3115_STATUS_TEMP_RDY BIT(1)
+#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
+
+#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
+
#define MPL3115_CTRL_RESET BIT(2) /* software reset */
#define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
#define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
#define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
+#define MPL3115_CTRL_IPOL1 BIT(5)
+#define MPL3115_CTRL_IPOL2 BIT(1)
+
+#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
+
+#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
+
struct mpl3115_data {
struct i2c_client *client;
+ struct iio_trigger *drdy_trig;
struct mutex lock;
u8 ctrl_reg1;
};
@@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
int ret, pos = 0;
mutex_lock(&data->lock);
- ret = mpl3115_request(data);
- if (ret < 0) {
- mutex_unlock(&data->lock);
- goto done;
+ if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
+ ret = mpl3115_request(data);
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
+ goto done;
+ }
}
if (test_bit(0, indio_dev->active_scan_mask)) {
@@ -228,10 +249,142 @@ static const struct iio_chan_spec mpl3115_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(2),
};
+static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct mpl3115_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
+ if (ret < 0)
+ return IRQ_HANDLED;
+
+ if (!(ret & MPL3115_CTRL_INT_SRC_DRDY))
+ return IRQ_NONE;
+
+ iio_trigger_poll_nested(data->drdy_trig);
+
+ return IRQ_HANDLED;
+}
+
+static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct mpl3115_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 ctrl_reg1 = data->ctrl_reg1;
+
+ if (state)
+ ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
+ else
+ ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
+
+ guard(mutex)(&data->lock);
+
+ 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,
+ state ? MPL3115_CTRL_INT_EN_DRDY : 0);
+ if (ret < 0)
+ goto reg1_cleanup;
+
+ data->ctrl_reg1 = ctrl_reg1;
+
+ return 0;
+
+reg1_cleanup:
+ i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
+ data->ctrl_reg1);
+ return ret;
+}
+
+static const struct iio_trigger_ops mpl3115_trigger_ops = {
+ .set_trigger_state = mpl3115_set_trigger_state,
+};
+
static const struct iio_info mpl3115_info = {
.read_raw = &mpl3115_read_raw,
};
+static int mpl3115_trigger_probe(struct mpl3115_data *data,
+ struct iio_dev *indio_dev)
+{
+ struct fwnode_handle *fwnode;
+ int ret, irq, irq_type;
+ bool act_high, is_int2 = false;
+
+ fwnode = dev_fwnode(&data->client->dev);
+ if (!fwnode)
+ return -ENODEV;
+
+ irq = fwnode_irq_get_byname(fwnode, "INT1");
+ if (irq < 0) {
+ irq = fwnode_irq_get_byname(fwnode, "INT2");
+ if (irq < 0)
+ return 0;
+
+ is_int2 = true;
+ }
+
+ irq_type = irq_get_trigger_type(irq);
+ switch (irq_type) {
+ case IRQF_TRIGGER_RISING:
+ act_high = true;
+ break;
+ case IRQF_TRIGGER_FALLING:
+ act_high = false;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
+ MPL3115_PT_DATA_EVENT_ALL);
+ if (ret < 0)
+ return ret;
+
+ if (!is_int2) {
+ ret = i2c_smbus_write_byte_data(data->client,
+ MPL3115_CTRL_REG5,
+ MPL3115_CTRL_INT_CFG_DRDY);
+ if (ret)
+ return ret;
+ }
+ if (act_high) {
+ ret = i2c_smbus_write_byte_data(data->client,
+ MPL3115_CTRL_REG3,
+ is_int2 ? MPL3115_CTRL_IPOL2 :
+ MPL3115_CTRL_IPOL1);
+ if (ret)
+ return ret;
+ }
+
+ data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
+ "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->drdy_trig)
+ return -ENOMEM;
+
+ data->drdy_trig->ops = &mpl3115_trigger_ops;
+ iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+ ret = iio_trigger_register(data->drdy_trig);
+ if (ret)
+ return ret;
+
+ indio_dev->trig = iio_trigger_get(data->drdy_trig);
+
+ return devm_request_threaded_irq(&data->client->dev, irq,
+ NULL,
+ mpl3115_interrupt_handler,
+ IRQF_ONESHOT,
+ "mpl3115_irq",
+ indio_dev);
+}
+
static int mpl3115_probe(struct i2c_client *client)
{
const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
if (ret < 0)
return ret;
+ ret = mpl3115_trigger_probe(data, indio_dev);
+ if (ret)
+ return ret;
+
ret = iio_triggered_buffer_setup(indio_dev, NULL,
mpl3115_trigger_handler, NULL);
if (ret < 0)
--
2.25.1On Sun, 2025-09-21 at 15:33 +0200, Antoni Pokusinski wrote:
> MPL3115 sensor features a "data ready" interrupt which indicates the
> presence of new measurements.
>
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> ---
> drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-
> 1 file changed, 162 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 579da60ef441..cf34de8f0d7e 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -7,7 +7,7 @@
> * (7-bit I2C slave address 0x60)
> *
> * TODO: FIFO buffer, altimeter mode, oversampling, continuous mode,
> - * interrupts, user offset correction, raw mode
> + * user offset correction, raw mode
> */
>
> #include <linux/module.h>
> @@ -17,26 +17,45 @@
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger.h>
> #include <linux/delay.h>
> +#include <linux/property.h>
>
> #define MPL3115_STATUS 0x00
> #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_INT_SOURCE 0x12
> +#define MPL3115_PT_DATA_CFG 0x13
> #define MPL3115_CTRL_REG1 0x26
> +#define MPL3115_CTRL_REG3 0x28
> +#define MPL3115_CTRL_REG4 0x29
> +#define MPL3115_CTRL_REG5 0x2a
>
> #define MPL3115_DEVICE_ID 0xc4
>
> #define MPL3115_STATUS_PRESS_RDY BIT(2)
> #define MPL3115_STATUS_TEMP_RDY BIT(1)
>
> +#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
> +
> +#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
> +
> #define MPL3115_CTRL_RESET BIT(2) /* software reset */
> #define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
>
> +#define MPL3115_CTRL_IPOL1 BIT(5)
> +#define MPL3115_CTRL_IPOL2 BIT(1)
> +
> +#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
> +
> +#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
> +
> struct mpl3115_data {
> struct i2c_client *client;
> + struct iio_trigger *drdy_trig;
> struct mutex lock;
> u8 ctrl_reg1;
> };
> @@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void
> *p)
> int ret, pos = 0;
>
> mutex_lock(&data->lock);
> - ret = mpl3115_request(data);
> - if (ret < 0) {
> - mutex_unlock(&data->lock);
> - goto done;
> + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> + ret = mpl3115_request(data);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
> + goto done;
> + }
> }
>
> if (test_bit(0, indio_dev->active_scan_mask)) {
> @@ -228,10 +249,142 @@ static const struct iio_chan_spec mpl3115_channels[] =
> {
> IIO_CHAN_SOFT_TIMESTAMP(2),
> };
>
> +static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
> + if (ret < 0)
> + return IRQ_HANDLED;
> +
> + if (!(ret & MPL3115_CTRL_INT_SRC_DRDY))
> + return IRQ_NONE;
> +
> + iio_trigger_poll_nested(data->drdy_trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 ctrl_reg1 = data->ctrl_reg1;
> +
> + if (state)
> + ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
> + else
> + ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
> +
> + guard(mutex)(&data->lock);
> +
As Andy pointed out, you should have a precursor patch converting the complete
driver to use the cleanup logic.
Another nice cleanup you could do (if you want of course) would be to get rid of
mpl3115_remove().
> + 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,
> + state ? MPL3115_CTRL_INT_EN_DRDY :
> 0);
> + if (ret < 0)
> + goto reg1_cleanup;
> +
> + data->ctrl_reg1 = ctrl_reg1;
> +
> + return 0;
> +
> +reg1_cleanup:
> + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> + data->ctrl_reg1);
> + return ret;
> +}
> +
> +static const struct iio_trigger_ops mpl3115_trigger_ops = {
> + .set_trigger_state = mpl3115_set_trigger_state,
> +};
> +
> static const struct iio_info mpl3115_info = {
> .read_raw = &mpl3115_read_raw,
> };
>
> +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> + struct iio_dev *indio_dev)
> +{
> + struct fwnode_handle *fwnode;
> + int ret, irq, irq_type;
> + bool act_high, is_int2 = false;
> +
> + fwnode = dev_fwnode(&data->client->dev);
> + if (!fwnode)
> + return -ENODEV;
> +
And to add to Andy's review, fwnode_irq_get_byname() will give you an error
anyways if !fwnode.
> + irq = fwnode_irq_get_byname(fwnode, "INT1");
> + if (irq < 0) {
> + irq = fwnode_irq_get_byname(fwnode, "INT2");
> + if (irq < 0)
> + return 0;
> +
> + is_int2 = true;
> + }
> +
> + irq_type = irq_get_trigger_type(irq);
> + switch (irq_type) {
> + case IRQF_TRIGGER_RISING:
> + act_high = true;
> + break;
> + case IRQF_TRIGGER_FALLING:
> + act_high = false;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> + MPL3115_PT_DATA_EVENT_ALL);
> + if (ret < 0)
> + return ret;
> +
> + if (!is_int2) {
> + ret = i2c_smbus_write_byte_data(data->client,
> + MPL3115_CTRL_REG5,
> + MPL3115_CTRL_INT_CFG_DRDY);
> + if (ret)
> + return ret;
> + }
> + if (act_high) {
> + ret = i2c_smbus_write_byte_data(data->client,
> + MPL3115_CTRL_REG3,
> + is_int2 ? MPL3115_CTRL_IPOL2
> :
> +
> MPL3115_CTRL_IPOL1);
> + if (ret)
> + return ret;
> + }
> +
> + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->drdy_trig)
> + return -ENOMEM;
> +
> + data->drdy_trig->ops = &mpl3115_trigger_ops;
> + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> + ret = iio_trigger_register(data->drdy_trig);
devm_iio_trigger_register()
- Nuno Sá
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> + return devm_request_threaded_irq(&data->client->dev, irq,
> + NULL,
> + mpl3115_interrupt_handler,
> + IRQF_ONESHOT,
> + "mpl3115_irq",
> + indio_dev);
> +}
> +
> static int mpl3115_probe(struct i2c_client *client)
> {
> const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
> if (ret < 0)
> return ret;
>
> + ret = mpl3115_trigger_probe(data, indio_dev);
> + if (ret)
> + return ret;
> +
> ret = iio_triggered_buffer_setup(indio_dev, NULL,
> mpl3115_trigger_handler, NULL);
> if (ret < 0)
On Mon, Sep 22, 2025 at 10:15:19AM +0100, Nuno Sá wrote:
> On Sun, 2025-09-21 at 15:33 +0200, Antoni Pokusinski wrote:
> > MPL3115 sensor features a "data ready" interrupt which indicates the
> > presence of new measurements.
> >
> > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> > ---
> > drivers/iio/pressure/mpl3115.c | 167 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 162 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 579da60ef441..cf34de8f0d7e 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -7,7 +7,7 @@
> > * (7-bit I2C slave address 0x60)
> > *
> > * TODO: FIFO buffer, altimeter mode, oversampling, continuous mode,
> > - * interrupts, user offset correction, raw mode
> > + * user offset correction, raw mode
> > */
> >
> > #include <linux/module.h>
> > @@ -17,26 +17,45 @@
> > #include <linux/iio/trigger_consumer.h>
> > #include <linux/iio/buffer.h>
> > #include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger.h>
> > #include <linux/delay.h>
> > +#include <linux/property.h>
> >
> > #define MPL3115_STATUS 0x00
> > #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_INT_SOURCE 0x12
> > +#define MPL3115_PT_DATA_CFG 0x13
> > #define MPL3115_CTRL_REG1 0x26
> > +#define MPL3115_CTRL_REG3 0x28
> > +#define MPL3115_CTRL_REG4 0x29
> > +#define MPL3115_CTRL_REG5 0x2a
> >
> > #define MPL3115_DEVICE_ID 0xc4
> >
> > #define MPL3115_STATUS_PRESS_RDY BIT(2)
> > #define MPL3115_STATUS_TEMP_RDY BIT(1)
> >
> > +#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
> > +
> > +#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
> > +
> > #define MPL3115_CTRL_RESET BIT(2) /* software reset */
> > #define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> > #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> > #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
> >
> > +#define MPL3115_CTRL_IPOL1 BIT(5)
> > +#define MPL3115_CTRL_IPOL2 BIT(1)
> > +
> > +#define MPL3115_CTRL_INT_EN_DRDY BIT(7)
> > +
> > +#define MPL3115_CTRL_INT_CFG_DRDY BIT(7)
> > +
> > struct mpl3115_data {
> > struct i2c_client *client;
> > + struct iio_trigger *drdy_trig;
> > struct mutex lock;
> > u8 ctrl_reg1;
> > };
> > @@ -164,10 +183,12 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void
> > *p)
> > int ret, pos = 0;
> >
> > mutex_lock(&data->lock);
> > - ret = mpl3115_request(data);
> > - if (ret < 0) {
> > - mutex_unlock(&data->lock);
> > - goto done;
> > + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> > + ret = mpl3115_request(data);
> > + if (ret < 0) {
> > + mutex_unlock(&data->lock);
> > + goto done;
> > + }
> > }
> >
> > if (test_bit(0, indio_dev->active_scan_mask)) {
> > @@ -228,10 +249,142 @@ static const struct iio_chan_spec mpl3115_channels[] =
> > {
> > IIO_CHAN_SOFT_TIMESTAMP(2),
> > };
> >
> > +static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct mpl3115_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
> > + if (ret < 0)
> > + return IRQ_HANDLED;
> > +
> > + if (!(ret & MPL3115_CTRL_INT_SRC_DRDY))
> > + return IRQ_NONE;
> > +
> > + iio_trigger_poll_nested(data->drdy_trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct mpl3115_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 ctrl_reg1 = data->ctrl_reg1;
> > +
> > + if (state)
> > + ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
> > + else
> > + ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
> > +
> > + guard(mutex)(&data->lock);
> > +
>
> As Andy pointed out, you should have a precursor patch converting the complete
> driver to use the cleanup logic.
>
> Another nice cleanup you could do (if you want of course) would be to get rid of
> mpl3115_remove().
>
Will add the precursor patch in v2
> > + 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,
> > + state ? MPL3115_CTRL_INT_EN_DRDY :
> > 0);
> > + if (ret < 0)
> > + goto reg1_cleanup;
> > +
> > + data->ctrl_reg1 = ctrl_reg1;
> > +
> > + return 0;
> > +
> > +reg1_cleanup:
> > + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > + data->ctrl_reg1);
> > + return ret;
> > +}
> > +
> > +static const struct iio_trigger_ops mpl3115_trigger_ops = {
> > + .set_trigger_state = mpl3115_set_trigger_state,
> > +};
> > +
> > static const struct iio_info mpl3115_info = {
> > .read_raw = &mpl3115_read_raw,
> > };
> >
> > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct fwnode_handle *fwnode;
> > + int ret, irq, irq_type;
> > + bool act_high, is_int2 = false;
> > +
> > + fwnode = dev_fwnode(&data->client->dev);
> > + if (!fwnode)
> > + return -ENODEV;
> > +
>
> And to add to Andy's review, fwnode_irq_get_byname() will give you an error
> anyways if !fwnode.
>
> > + irq = fwnode_irq_get_byname(fwnode, "INT1");
> > + if (irq < 0) {
> > + irq = fwnode_irq_get_byname(fwnode, "INT2");
> > + if (irq < 0)
> > + return 0;
> > +
> > + is_int2 = true;
> > + }
> > +
> > + irq_type = irq_get_trigger_type(irq);
> > + switch (irq_type) {
> > + case IRQF_TRIGGER_RISING:
> > + act_high = true;
> > + break;
> > + case IRQF_TRIGGER_FALLING:
> > + act_high = false;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> > + MPL3115_PT_DATA_EVENT_ALL);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!is_int2) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG5,
> > + MPL3115_CTRL_INT_CFG_DRDY);
> > + if (ret)
> > + return ret;
> > + }
> > + if (act_high) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG3,
> > + is_int2 ? MPL3115_CTRL_IPOL2
> > :
> > +
> > MPL3115_CTRL_IPOL1);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> > + "%s-dev%d",
> > + indio_dev->name,
> > + iio_device_id(indio_dev));
> > + if (!data->drdy_trig)
> > + return -ENOMEM;
> > +
> > + data->drdy_trig->ops = &mpl3115_trigger_ops;
> > + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > + ret = iio_trigger_register(data->drdy_trig);
>
> devm_iio_trigger_register()
>
> - Nuno Sá
>
Will fix in v2
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> > +
> > + return devm_request_threaded_irq(&data->client->dev, irq,
> > + NULL,
> > + mpl3115_interrupt_handler,
> > + IRQF_ONESHOT,
> > + "mpl3115_irq",
> > + indio_dev);
> > +}
> > +
> > static int mpl3115_probe(struct i2c_client *client)
> > {
> > const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > @@ -271,6 +424,10 @@ static int mpl3115_probe(struct i2c_client *client)
> > if (ret < 0)
> > return ret;
> >
> > + ret = mpl3115_trigger_probe(data, indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > mpl3115_trigger_handler, NULL);
> > if (ret < 0)
On Sun, Sep 21, 2025 at 4:34 PM Antoni Pokusinski
<apokusinski01@gmail.com> wrote:
>
> MPL3115 sensor features a "data ready" interrupt which indicates the
> presence of new measurements.
...
> #include <linux/module.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger.h>
> #include <linux/delay.h>
> +#include <linux/property.h>
This is like delay.h is misplaced. What we do here, we group generic
ones followed by subsystem (IIO) related ones, and this seems wrong in
this driver.
Can you rather move delay.h to be the first, and add property.h after
i2c.h followed by a blank line, so in the result it will be like
delay.h
i2c.h
module.h
property.h
...blank.line...
iio/*.h
...
> +#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
> +
> +#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
Not sure I understand this definition in the following aspects:
1) why is this disrupting the _CTRL_ definitions?
2) why is this using BIT(x) and not respective definitions?
3) why can't you use GENMASK() if you just select all bits in a
certain bitfield?
> #define MPL3115_CTRL_RESET BIT(2) /* software reset */
> #define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
...
> mutex_lock(&data->lock);
> - ret = mpl3115_request(data);
> - if (ret < 0) {
> - mutex_unlock(&data->lock);
> - goto done;
> + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> + ret = mpl3115_request(data);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
Instead, I suggest adding a prerequisite that moves the driver to use
cleanup.h, in particular scoped_guard(). This will reduce a churn
here,
> + goto done;
> + }
> }
...
> +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 ctrl_reg1 = data->ctrl_reg1;
> +
> + if (state)
> + ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
> + else
> + ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
> + guard(mutex)(&data->lock);
Oh, and you already use this! Definitely, it misses the prerequisite patch.
> + 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,
> + state ? MPL3115_CTRL_INT_EN_DRDY : 0);
> + if (ret < 0)
> + goto reg1_cleanup;
> +
> + data->ctrl_reg1 = ctrl_reg1;
> +
> + return 0;
> +
> +reg1_cleanup:
> + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> + data->ctrl_reg1);
> + return ret;
> +}
...
> +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> + struct iio_dev *indio_dev)
> +{
> + struct fwnode_handle *fwnode;
> + int ret, irq, irq_type;
> + bool act_high, is_int2 = false;
> + fwnode = dev_fwnode(&data->client->dev);
> + if (!fwnode)
> + return -ENODEV;
Why is this fatal? Also, do we have a board file for users of this right now?
> + irq = fwnode_irq_get_byname(fwnode, "INT1");
> + if (irq < 0) {
> + irq = fwnode_irq_get_byname(fwnode, "INT2");
> + if (irq < 0)
> + return 0;
> +
> + is_int2 = true;
> + }
> +
> + irq_type = irq_get_trigger_type(irq);
> + switch (irq_type) {
> + case IRQF_TRIGGER_RISING:
> + act_high = true;
> + break;
> + case IRQF_TRIGGER_FALLING:
> + act_high = false;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> + MPL3115_PT_DATA_EVENT_ALL);
> + if (ret < 0)
> + return ret;
> + if (!is_int2) {
> + ret = i2c_smbus_write_byte_data(data->client,
> + MPL3115_CTRL_REG5,
> + MPL3115_CTRL_INT_CFG_DRDY);
> + if (ret)
> + return ret;
> + }
> + if (act_high) {
> + ret = i2c_smbus_write_byte_data(data->client,
> + MPL3115_CTRL_REG3,
> + is_int2 ? MPL3115_CTRL_IPOL2 :
> + MPL3115_CTRL_IPOL1);
> + if (ret)
> + return ret;
> + }
This if (!is_int2) and ternary with the same argument is kinda hard to
read, can we refactor it somehow?
For example, if these two booleans are represented by a common enum, we can do
switch (cfg_flags) {
case INT2_ACTIVE_HIGH:
_write_byte_data(REG3);
break;
case INT2_ACTIVE_LOW:
break;
case INT1_ACTIVE_HIGH:
_write_byte_data(REG5);
_write_byte_data(REG3);
break;
case INT1_ACTIVE_LOW:
_write_byte_data(REG5);
break;
default:
return -EINVAL;
}
Yes, it's more verbose, but I find this better to read and understand.
Note, you may drop the switch case for IRQ with this approach as you
can use a few bits together (separate bits for raising and falling to
make the default case working here).
> + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->drdy_trig)
> + return -ENOMEM;
> +
> + data->drdy_trig->ops = &mpl3115_trigger_ops;
> + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> + ret = iio_trigger_register(data->drdy_trig);
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> + return devm_request_threaded_irq(&data->client->dev, irq,
> + NULL,
> + mpl3115_interrupt_handler,
> + IRQF_ONESHOT,
> + "mpl3115_irq",
> + indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
> ...
>
> > mutex_lock(&data->lock);
> > - ret = mpl3115_request(data);
> > - if (ret < 0) {
> > - mutex_unlock(&data->lock);
> > - goto done;
> > + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> > + ret = mpl3115_request(data);
> > + if (ret < 0) {
>
> > + mutex_unlock(&data->lock);
>
> Instead, I suggest adding a prerequisite that moves the driver to use
> cleanup.h, in particular scoped_guard(). This will reduce a churn
> here,
I'll comment on this in version 3, but I'm not sure scoped_guard() is
necessarily a good idea here.
Also note that there is no requirement to use one style universally in
a driver (guard / vs explicit unlocks) as often they work best in different
usecases with the same locks.
>
> > + goto done;
> > + }
> > }
On Sun, Sep 21, 2025 at 10:29:28PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 21, 2025 at 4:34 PM Antoni Pokusinski
> <apokusinski01@gmail.com> wrote:
> >
> > MPL3115 sensor features a "data ready" interrupt which indicates the
> > presence of new measurements.
>
> ...
>
> > #include <linux/module.h>
>
> > #include <linux/iio/trigger_consumer.h>
> > #include <linux/iio/buffer.h>
> > #include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger.h>
> > #include <linux/delay.h>
>
> > +#include <linux/property.h>
>
> This is like delay.h is misplaced. What we do here, we group generic
> ones followed by subsystem (IIO) related ones, and this seems wrong in
> this driver.
>
> Can you rather move delay.h to be the first, and add property.h after
> i2c.h followed by a blank line, so in the result it will be like
>
> delay.h
> i2c.h
> module.h
> property.h
> ...blank.line...
> iio/*.h
>
> ...
>
Sure, will fix this in v2.
> > +#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
> > +
> > +#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
>
> Not sure I understand this definition in the following aspects:
> 1) why is this disrupting the _CTRL_ definitions?
> 2) why is this using BIT(x) and not respective definitions?
> 3) why can't you use GENMASK() if you just select all bits in a
> certain bitfield?
>
>
1) I placed the definitions of the bits/masks in the order of the registers
that they correspond to, i.e.
CTRL_INT_SRC_DRDY // bit in reg 0x12
PT_DATA_EVENT_ALL // bits in reg 0x13
CTRL_RESET // bit in reg 0x14
Actually, the wrong name here is the INT_SRC_DRDY definition because it is a
bit in the INT_SOURCE register, not a control register. Therefore, the
name should be MPL3115_INT_SRC_DRDY instead of MPL3115_CTRL_INT_SRC_DRDY
2) I saw that e.g. CTRL_OS_258MS is defined using BIT(x), so I thought
that this is the convention in this driver that I did not want to
disrupt by using GENMASK()
3) Sure, I'd even prefer GENMASK(), the only reason why I didn't use it
is explained in 2)
> > #define MPL3115_CTRL_RESET BIT(2) /* software reset */
> > #define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> > #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> > #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
>
> ...
>
> > mutex_lock(&data->lock);
> > - ret = mpl3115_request(data);
> > - if (ret < 0) {
> > - mutex_unlock(&data->lock);
> > - goto done;
> > + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> > + ret = mpl3115_request(data);
> > + if (ret < 0) {
>
> > + mutex_unlock(&data->lock);
>
> Instead, I suggest adding a prerequisite that moves the driver to use
> cleanup.h, in particular scoped_guard(). This will reduce a churn
> here,
>
Will add in v2.
> > + goto done;
> > + }
> > }
>
> ...
>
> > +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct mpl3115_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 ctrl_reg1 = data->ctrl_reg1;
> > +
> > + if (state)
> > + ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
> > + else
> > + ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
>
> > + guard(mutex)(&data->lock);
>
> Oh, and you already use this! Definitely, it misses the prerequisite patch.
>
> > + 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,
> > + state ? MPL3115_CTRL_INT_EN_DRDY : 0);
> > + if (ret < 0)
> > + goto reg1_cleanup;
> > +
> > + data->ctrl_reg1 = ctrl_reg1;
> > +
> > + return 0;
> > +
> > +reg1_cleanup:
> > + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > + data->ctrl_reg1);
> > + return ret;
> > +}
>
> ...
>
> > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct fwnode_handle *fwnode;
> > + int ret, irq, irq_type;
> > + bool act_high, is_int2 = false;
>
> > + fwnode = dev_fwnode(&data->client->dev);
> > + if (!fwnode)
> > + return -ENODEV;
>
>
> Why is this fatal? Also, do we have a board file for users of this right now?
>
Actually it seems it does not have to be fatal. If we get rid of this
if, then we'd simply return without setting up the trigger and with no
interrupt support, which is ok I guess.
As for the board file, do you mean some PCB schematics? I don't know
about any, I've only used the following datasheet from NXP:
https://www.nxp.com/docs/en/data-sheet/MPL3115A2S.pdf
> > + irq = fwnode_irq_get_byname(fwnode, "INT1");
> > + if (irq < 0) {
> > + irq = fwnode_irq_get_byname(fwnode, "INT2");
> > + if (irq < 0)
> > + return 0;
> > +
> > + is_int2 = true;
> > + }
> > +
> > + irq_type = irq_get_trigger_type(irq);
> > + switch (irq_type) {
> > + case IRQF_TRIGGER_RISING:
> > + act_high = true;
> > + break;
> > + case IRQF_TRIGGER_FALLING:
> > + act_high = false;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> > + MPL3115_PT_DATA_EVENT_ALL);
> > + if (ret < 0)
> > + return ret;
>
> > + if (!is_int2) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG5,
> > + MPL3115_CTRL_INT_CFG_DRDY);
> > + if (ret)
> > + return ret;
> > + }
> > + if (act_high) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG3,
> > + is_int2 ? MPL3115_CTRL_IPOL2 :
> > + MPL3115_CTRL_IPOL1);
> > + if (ret)
> > + return ret;
> > + }
>
> This if (!is_int2) and ternary with the same argument is kinda hard to
> read, can we refactor it somehow?
>
> For example, if these two booleans are represented by a common enum, we can do
>
> switch (cfg_flags) {
> case INT2_ACTIVE_HIGH:
> _write_byte_data(REG3);
> break;
> case INT2_ACTIVE_LOW:
> break;
> case INT1_ACTIVE_HIGH:
> _write_byte_data(REG5);
> _write_byte_data(REG3);
> break;
> case INT1_ACTIVE_LOW:
> _write_byte_data(REG5);
> break;
> default:
> return -EINVAL;
> }
>
> Yes, it's more verbose, but I find this better to read and understand.
>
> Note, you may drop the switch case for IRQ with this approach as you
> can use a few bits together (separate bits for raising and falling to
> make the default case working here).
>
Ok, your suggestion looks nice. I think to define maybe the enums like this:
#define INT2 BIT(2)
enum {
INT2_ACTIVE_LOW = INT2 | IRQF_TRIGGER_FALLING,
INT2_ACTIVE_HIGH = INT2 | IRQF_TRIGGER_RISING,
INT1_ACTIVE_LOW = !INT2 | IRQF_TRIGGER_FALLING,
INT1_ACTIVE_HIGH = !INT2 | IRQF_TRIGGER_RISING,
};
This way the cfg_flags could be first |= INT_2 (after the call to the
fwnode_irq_get_byname) and then |= irq_type (after the call to the irq_get_trigger_type)
> > + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> > + "%s-dev%d",
> > + indio_dev->name,
> > + iio_device_id(indio_dev));
> > + if (!data->drdy_trig)
> > + return -ENOMEM;
> > +
> > + data->drdy_trig->ops = &mpl3115_trigger_ops;
> > + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > + ret = iio_trigger_register(data->drdy_trig);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> > +
> > + return devm_request_threaded_irq(&data->client->dev, irq,
> > + NULL,
> > + mpl3115_interrupt_handler,
> > + IRQF_ONESHOT,
> > + "mpl3115_irq",
> > + indio_dev);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Kind regards,
Antoni Pokusinski
© 2016 - 2025 Red Hat, Inc.