Message-ID: <20250928100232.tafcimfsoljdq6nt@antoni-VivoBook-ASUSLaptop-X512FAY-K512FA>
Date: Sun, 28 Sep 2025 12:02:32 +0200
From: Antoni Pokusinski <apokusinski01@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, dlechner@...libre.com,
	nuno.sa@...log.com, andy@...nel.org, robh@...nel.org,
	krzk+dt@...nel.org
Cc: conor+dt@...nel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
	linux@...ck-us.net, rodrigo.gobbi.7@...il.com,
	naresh.solanki@...ements.com, michal.simek@....com,
	grantpeltier93@...il.com, farouk.bouabid@...rry.de,
	marcelo.schmitt1@...il.com
Subject: Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

On Sat, Sep 27, 2025 at 05:51:25PM +0100, Jonathan Cameron wrote:
> On Sat, 27 Sep 2025 00:01:49 +0200
> Antoni Pokusinski <apokusinski01@...il.com> wrote:
> 
> > MPL3115 sensor features a "data ready" interrupt which indicates the
> > presence of new measurements.
> > 
> > Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
> Various comments inline.  Main ones are more on the guard() usage combined
> with gotos and split out he renames as a precursor patch so only the main
> change occurs in this one.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/pressure/mpl3115.c | 197 ++++++++++++++++++++++++++++++---
> >  1 file changed, 184 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 80af672f65c6..13c8b338a15e 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> 
> >  
> >  #define MPL3115_DEVICE_ID 0xc4
> >  
> >  #define MPL3115_STATUS_PRESS_RDY BIT(2)
> >  #define MPL3115_STATUS_TEMP_RDY BIT(1)
> >  
> > -#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_INT_SRC_DRDY BIT(7)
> > +
> > +#define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> > +
> > +#define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> > +#define MPL3115_CTRL1_OST BIT(1) /* initiate measurement */
> > +#define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
> 
> Precursor patch should make the renames to CTRL1
> That will reduce the noise in this patch.
>
Sure, will add a separate patch in v4 with all the renames.
> > +#define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
> > +
> > +#define MPL3115_CTRL3_IPOL1 BIT(5)
> > +#define MPL3115_CTRL3_IPOL2 BIT(1)
> > +
> > +#define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> > +
> > +#define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> > +
> > +#define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
> >  
> >  struct mpl3115_data {
> >  	struct i2c_client *client;
> > +	struct iio_trigger *drdy_trig;
> >  	struct mutex lock;
> >  	u8 ctrl_reg1;
> >  };
> >  
> > +enum mpl3115_irq_type {
> > +	INT2_ACTIVE_LOW  = MPL3115_INT2 | IRQF_TRIGGER_FALLING,
> > +	INT2_ACTIVE_HIGH = MPL3115_INT2 | IRQF_TRIGGER_RISING,
> > +	INT1_ACTIVE_LOW  = (!MPL3115_INT2) | IRQF_TRIGGER_FALLING,
> > +	INT1_ACTIVE_HIGH = (!MPL3115_INT2) | IRQF_TRIGGER_RISING,
> This mixing and matching of IRQF_ definitions with locally defined
> additional flags is fragile because it is more than possible
> a future kernel wide change will change the IRQF_ values.
> 
> So keep them separate.
> 
> > +};
> > +
> >  static int mpl3115_request(struct mpl3115_data *data)
> >  {
> >  	int ret, tries = 15;
> >  
> >  	/* trigger measurement */
> >  	ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -		data->ctrl_reg1 | MPL3115_CTRL_OST);
> > +		data->ctrl_reg1 | MPL3115_CTRL1_OST);
> More renames that shouldn't be in this patch.
> 
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -58,7 +86,7 @@ static int mpl3115_request(struct mpl3115_data *data)
> >  		if (ret < 0)
> >  			return ret;
> >  		/* wait for data ready, i.e. OST cleared */
> > -		if (!(ret & MPL3115_CTRL_OST))
> > +		if (!(ret & MPL3115_CTRL1_OST))
> More renames for the precursor patch.
> 
> >  			break;
> >  		msleep(20);
> >  	}
> > @@ -166,9 +194,11 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >  	int ret, pos = 0;
> >  
> >  	scoped_guard(mutex, &data->lock) {
> > -		ret = mpl3115_request(data);
> > -		if (ret < 0)
> > -			goto done;
> > +		if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
> > +			ret = mpl3115_request(data);
> > +			if (ret < 0)
> > +				goto done;
> This follows on from comment in earlier patch.
> 
> > +		}
> >
> > +
> > +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_CTRL1_ACTIVE;
> > +	else
> > +		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > +
> > +	guard(mutex)(&data->lock);
> As in earlier patch, don't mix guard() or anything from cleanup.h with
> code doing gotos as scope can be very weirdly defined when you do.
> 
> This is actually bug free (I think), but doesn't match the guidance notes in cleanup.h
> 
> Various options.
> 1. Don't use guard here
> 2. Factor out the stuff under the lock.  The helper function has clearly defined
>    separate scope so that can contain the goto reg1_cleanup.h bit.
> 
Ok, in here I'd try to factor this out and have something like:

  guard(mutex)(&data->lock);

  return mpl3115_set_interrupt_state(data, ctrl_reg1);

> > +
> > +	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_CTRL4_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 = dev_fwnode(&data->client->dev);
> > +	int ret, irq, irq_type, irq_cfg_flags = 0;
> > +
> > +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> > +	if (irq < 0) {
> > +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> > +		if (irq < 0)
> > +			return 0;
> > +
> > +		irq_cfg_flags |= MPL3115_INT2;
> > +	}
> > +
> > +	irq_type = irq_get_trigger_type(irq);
> > +	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> > +		return -EINVAL;
> > +
> > +	irq_cfg_flags |= irq_type;
> Commented on this before, but mixing flags that are local to this driver
> with those that are global provides not guarantees against future changes
> of the global ones to overlap with your local values.
> 
> So just track these as two separate values rather than combining them.
>

So you mean 2 separate variables, one for INT1/INT2 and one for the
trigger RISING/FALLING, am I right?
This was the approach in v1, but the code for writing the regs CTRL3 and
CTRL5 should be improved, I was thinking something like:

if (irq_pin == MPL3115_IRQ_INT1) {
    write_byte_data(REG5, INT_CFG_DRDY);
    if (irq_type == IRQF_TRIGGER_RISING)
        write_byte_data(REG3, IPOL1);
} else if (irq_type == IRQF_TRIGGER_RISING) {
    write_byte_data(REG3, IPOL2);
}

This is perhaps a bit less readable than the switch(int_cfg_flags) with 4
cases... but IMO it's still quite ok and it's less verbose since we do not
duplicate the write_byte_data(REG5, INT_CFG_DRDY).

> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> > +					MPL3115_PT_DATA_EVENT_ALL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (irq_cfg_flags) {
> > +	case INT2_ACTIVE_HIGH:
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> > +						MPL3115_CTRL3_IPOL2);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case INT2_ACTIVE_LOW:
> > +		break;
> > +	case INT1_ACTIVE_HIGH:
> > +		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_REG3,
> > +						MPL3115_CTRL3_IPOL1);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case INT1_ACTIVE_LOW:
> > +		ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> > +						MPL3115_CTRL5_INT_CFG_DRDY);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	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 = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);
> 
> Whilst unlikely the race matters. It is this call that creates the infrastructure
> that might allow the interrupt generation to be triggered via userspace controls.
> So the handler should probably be in place firsts.  I.e. do the devm_request_threaded_irq
> before this.
>
Will fix in v4
> > +	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);
> 
> wrap closer to 80 chars by combining some of those lines.
>
Will fix in v4
> > +}
> > +
> >  static int mpl3115_probe(struct i2c_client *client)
> >  {
> >  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > @@ -258,15 +425,19 @@ static int mpl3115_probe(struct i2c_client *client)
> >  
> >  	/* software reset, I2C transfer is aborted (fails) */
> >  	i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> > -		MPL3115_CTRL_RESET);
> > +		MPL3115_CTRL1_RESET);
> >  	msleep(50);
> >  
> > -	data->ctrl_reg1 = MPL3115_CTRL_OS_258MS;
> > +	data->ctrl_reg1 = MPL3115_CTRL1_OS_258MS;
> As elsewhere.  Do the rename as a precursor patch so that we reduce
> the noise around the real changes in here and make that bit easier to review.
> 
> >  	ret = i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> >  		data->ctrl_reg1);
> >  	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)
> > @@ -285,7 +456,7 @@ static int mpl3115_probe(struct i2c_client *client)
> >  static int mpl3115_standby(struct mpl3115_data *data)
> >  {
> >  	return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -		data->ctrl_reg1 & ~MPL3115_CTRL_ACTIVE);
> > +		data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> As above. This isn't part of the main change here so should have been in a separate
> precursor patch
> 
> >  }
> >  
> >  static void mpl3115_remove(struct i2c_client *client)
> 
Kind regards,
Antoni

Powered by blists - more mailing lists