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