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

[Jonathan Cameron <jic23@kernel.org>]

On Mon,  7 Oct 2024 21:49:44 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> 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@linux.intel.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

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.

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

> +
> +	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.
> +		if (ret)
> +			return IRQ_NONE;
> +	}