Re: [PATCH v8 3/4] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Sat, Oct 12, 2024 at 05:08:23PM +0100, Jonathan Cameron wrote:
> On Mon,  7 Oct 2024 21:49:44 +0200
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
> 
> > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> > a trigger for when there are data ready in the sensor for pick up.
> > 
> > This use case is used along with NORMAL_MODE in the sensor, which allows
> > the sensor to do consecutive measurements depending on the ODR rate value.
> > 
> > The trigger pin can be configured to be open-drain or push-pull and either
> > rising or falling edge.
> > 
> > No support is added yet for interrupts for FIFO, WATERMARK and out of range
> > values.
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> 
> Hi Vasileios,
> 
> One questing about locking below.  What you have is probably correct
> but might be tighter than it needs to be, or need a comment to say why
> for future readers.
> 
> I hate register reads with side effects btw.  It's an 'optimization'
> hardware designers thing is nice, but makes for really ugly software
> interfaces.
> 

Hi Jonathan,

> > @@ -2429,6 +2564,88 @@ static int bmp580_chip_config(struct bmp280_data *data)
> >  	return 0;
> >  }
> >  
> > +static void bmp580_trigger_reenable(struct iio_trigger *trig)
> > +{
> > +	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
> > +	unsigned int tmp;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp);
> As below. Seems this read has side effects (horrible!)
> I'm not sure if this is related to the locking though.

Yes indeed, this read is needed in order to clear the interrupt. In that
way the interrupt is kind of resetted so it can get triggered again. It
is actually the 1st line in page 28 of [1].

[1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp585-ds003.pdf
> > +	if (ret)
> > +		dev_err(data->dev, "Failed to reset interrupt.\n");
> > +}
> 
> > +static int bmp580_int_pin_config(struct bmp280_data *data)
> > +{
> > +	int pin_drive_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN,
> > +				       data->trig_open_drain);
> > +	int pin_level_cfg = FIELD_PREP(BMP580_INT_CONFIG_LEVEL,
> > +				       data->trig_active_high);
> > +	int ret, int_pin_cfg = pin_drive_cfg | pin_level_cfg;
> 	int int_pin_cfg = pin...
> 	int ret;
> 
> Is easier to follow.
> 

ACK.

> > +
> > +	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
> > +				 BMP580_INT_CONFIG_MASK, int_pin_cfg);
> > +	if (ret) {
> > +		dev_err(data->dev, "Could not set interrupt settings.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE,
> > +			      BMP580_INT_SOURCE_DRDY);
> > +	if (ret)
> > +		dev_err(data->dev, "Could not set interrupt source.\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
> > +{
> > +	struct iio_dev *indio_dev = p;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	unsigned int int_ctrl;
> > +	int ret;
> > +
> > +	scoped_guard(mutex, &data->lock) {
> > +		ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
> What are you locking against here?  Seems this read may have side effects?
> If not the regmap internal locking should be enough for a register read.

I had a 2nd look and the lock is redundant indeed. This came from the
fact that I use the lock in the oneshot captures becasue when we do
reads of pressure or humidity (BME280) measurements, the temp regs are
read first, they are processed and later the pressure/humidity regs are
read so this operation should be kept serialized. But here there is no
problem.

Cheers,
Vasilis

> > +		if (ret)
> > +			return IRQ_NONE;
> > +	}
> 
>