On Fri, 26 Jul 2024 01:10:38 +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.
>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
Hi Vasileios,
A few minor things inline, including a suggestion that perhaps the trigger_probe()
functions can be combined to reduce duplication. That would use a
__bmp280_trigger_probe(struct iio_dev *, struct iio_trigger_ops *,
+ some function pointers).
Perhaps it's not worth it - I didn't try writing the actual code!
Jonathan
> ---
> drivers/iio/pressure/bmp280-core.c | 309 ++++++++++++++++++++++++++-
> drivers/iio/pressure/bmp280-regmap.c | 2 +-
> drivers/iio/pressure/bmp280.h | 23 +-
> 3 files changed, 328 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 4a8d2ed4a9c4..4238f37b7805 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -37,12 +37,14 @@
> +static irqreturn_t bmp380_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, BMP380_REG_INT_STATUS, &int_ctrl);
> + if (ret)
> + return IRQ_NONE;
> + }
> +
> + if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
> + iio_trigger_poll_nested(data->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int bmp380_trigger_probe(struct iio_dev *indio_dev)
Two of these functions are very similar. Perhaps define a common
function that takes a function call for int config, the ops, and
interrupt handler as arguments then add device specific
calls that use that.
> +{
> + struct bmp280_data *data = iio_priv(indio_dev);
> + struct fwnode_handle *fwnode;
> + int ret, irq, irq_type;
> + struct irq_data *desc;
> +
> + fwnode = dev_fwnode(data->dev);
> + if (!fwnode)
> + return -ENODEV;
> +
> + irq = fwnode_irq_get(fwnode, 0);
> + if (!irq) {
> + dev_err(data->dev, "No interrupt found\n");
> + return -ENODEV;
> + }
> +
> + desc = irq_get_irq_data(irq);
> + if (!desc)
> + return -EINVAL;
> +
> + irq_type = irqd_get_trigger_type(desc);
> + switch (irq_type) {
> + case IRQF_TRIGGER_RISING:
> + data->trig_active_high = true;
> + break;
> + case IRQF_TRIGGER_FALLING:
> + data->trig_active_high = false;
> + break;
> + default:
> + dev_err(data->dev, "Invalid interrupt type specified\n");
> + return -EINVAL;
> + }
> +
> + data->trig_open_drain = fwnode_property_read_bool(fwnode,
> + "int-open-drain");
> +
> + ret = bmp380_int_config(data);
> + if (ret)
> + return ret;
> +
> + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->ops = &bmp380_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, data);
> +
> + ret = devm_request_threaded_irq(data->dev, irq, NULL,
> + bmp380_irq_thread_handler, IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret) {
> + dev_err(data->dev, "request irq failed\n");
> + return ret;
> + }
> +
> + ret = devm_iio_trigger_register(data->dev, data->trig);
> + if (ret) {
> + dev_err(data->dev, "iio trigger register failed\n");
> + return ret;
> + }
> +
> + indio_dev->trig = iio_trigger_get(data->trig);
> +
> + return 0;
> +}
> +
> +
one blank line only.
> static irqreturn_t bmp380_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> @@ -1854,6 +1998,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
> .wait_conv = bmp380_wait_conv,
> .preinit = bmp380_preinit,
>
> + .trigger_probe = bmp380_trigger_probe,
> .trigger_handler = bmp380_trigger_handler,
> };
> EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
> @@ -2390,6 +2535,154 @@ static int bmp580_chip_config(struct bmp280_data *data)
> return 0;
> }
>
...
> +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) {
Indent wrong.
> + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
> + if (ret)
> + return IRQ_NONE;
> + }
> +
> + if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
> + iio_trigger_poll_nested(data->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> +{
...
> +
> + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->ops = &bmp580_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, data);
> +
> + ret = devm_request_threaded_irq(data->dev, irq, NULL,
> + bmp580_irq_thread_handler, IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret) {
> + dev_err(data->dev, "request irq failed\n");
Only in probe paths I think, so return dev_err_probe() thoughout these
trigger setup callbacks.
> +}
> diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
> index d27d68edd906..cccdf8fc6c09 100644
> --- a/drivers/iio/pressure/bmp280-regmap.c
> +++ b/drivers/iio/pressure/bmp280-regmap.c
> @@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
> case BMP380_REG_FIFO_WATERMARK_LSB:
> case BMP380_REG_FIFO_WATERMARK_MSB:
> case BMP380_REG_POWER_CONTROL:
> - case BMP380_REG_INT_CONTROL:
> + case BMP380_REG_INT_CTRL:
Unrelated change. I'm also not sure it's worth making.
> case BMP380_REG_IF_CONFIG:
> case BMP380_REG_ODR:
> case BMP380_REG_OSR:
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index f5d192509d61..754eda367941 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -55,8 +55,17 @@
> #define BMP580_CMD_NVM_WRITE_SEQ_1 0xA0
> #define BMP580_CMD_SOFT_RESET 0xB6
>
> +#define BMP580_INT_STATUS_DRDY_MASK BIT(0)
> #define BMP580_INT_STATUS_POR_MASK BIT(4)
>
> +#define BMP580_INT_SOURCE_DRDY BIT(0)
> +
> +#define BMP580_INT_CONFIG_MASK GENMASK(3, 0)
> +#define BMP580_INT_CONFIG_LATCH BIT(0)
> +#define BMP580_INT_CONFIG_LEVEL BIT(1)
> +#define BMP580_INT_CONFIG_OPEN_DRAIN BIT(2)
> +#define BMP580_INT_CONFIG_INT_EN BIT(3)
> +
> #define BMP580_STATUS_CORE_RDY_MASK BIT(0)
> #define BMP580_STATUS_NVM_RDY_MASK BIT(1)
> #define BMP580_STATUS_NVM_ERR_MASK BIT(2)
> @@ -117,7 +126,7 @@
> #define BMP380_REG_OSR 0x1C
> #define BMP380_REG_POWER_CONTROL 0x1B
> #define BMP380_REG_IF_CONFIG 0x1A
> -#define BMP380_REG_INT_CONTROL 0x19
> +#define BMP380_REG_INT_CTRL 0x19
As above.
Jonathan
