Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

Andy Shevchenko
2024-08-23T20:06:35Z

On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.

...

> +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> + const struct iio_trigger_ops *trigger_ops,
> + int (*int_config)(struct bmp280_data *data),

> + irqreturn_t (*irq_thread_handler)(int irq, void *p))

irq_handler_t

...

> + fwnode = dev_fwnode(data->dev);
> + if (!fwnode)
> + return -ENODEV;

Why do you need this? The below will fail anyway.

> + irq = fwnode_irq_get(fwnode, 0);
> + if (!irq)

Are you sure this is correct check?

> + return dev_err_probe(data->dev, -ENODEV,

Shadowed error code.

> + "No interrupt found.\n");

> + desc = irq_get_irq_data(irq);
> + if (!desc)
> + return -EINVAL;

When may this fail?

> + 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:
> + return dev_err_probe(data->dev, -EINVAL,
> + "Invalid interrupt type specified.\n");
> + }

> + data->trig_open_drain = fwnode_property_read_bool(fwnode,
> + "int-open-drain");

Better

data->trig_open_drain =
fwnode_property_read_bool(fwnode, "int-open-drain");

...

> +static int bmp380_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct bmp280_data *data = iio_trigger_get_drvdata(trig);
> + int ret;
> +
> + guard(mutex)(&data->lock);
> +
> + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> + BMP380_INT_CTRL_DRDY_EN,
> + FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> + state ? 1 : 0));

FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));

? ( Even <= 80 characters)

> + if (ret) {
> + dev_err(data->dev, "Could not enable/disable interrupt\n");
> + return ret;
> + }
> +
> + return 0;

if (ret)
dev_err(data->dev, "Could not enable/disable interrupt\n");

return ret;

?

> +}

...

> +static int bmp380_int_config(struct bmp280_data *data)
> +{
> + int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
> + data->trig_open_drain) |
> + FIELD_PREP(BMP380_INT_CTRL_LEVEL,
> + data->trig_active_high);

Split these two variables and make the indentation better for int_cfg.

> + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> + BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> + if (ret) {
> + dev_err(data->dev, "Could not set interrupt settings\n");

> + return ret;
> + }
> +
> + return 0;

return ret;

?

> +}

...

> +static int bmp580_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct bmp280_data *data = iio_trigger_get_drvdata(trig);
> + int ret;
> +
> + guard(mutex)(&data->lock);
> +
> + ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
> + BMP580_INT_CONFIG_INT_EN,

> + FIELD_PREP(BMP580_INT_CONFIG_INT_EN,
> + state ? 1 : 0));

!!state ?

> + if (ret) {
> + dev_err(data->dev, "Could not enable/disable interrupt\n");
> + return ret;
> + }
> +
> + return 0;

return ret;

?

> +}

...

> +static int bmp580_int_config(struct bmp280_data *data)

Same comments as per above.

...

> + if (irq > 0) {
> + if (chip_id == BMP180_CHIP_ID) {
> + ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
> + if (ret)
> + return ret;
> + }
> + if (data->chip_info->trigger_probe) {
> + ret = data->chip_info->trigger_probe(indio_dev);
> + if (ret)
> + return ret;
> + }
> }

Can be

if (irq > 0) {
if (chip_id == BMP180_CHIP_ID)
ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
if (data->chip_info->trigger_probe)
ret = data->chip_info->trigger_probe(indio_dev);
if (ret)
return ret;
}

--
With Best Regards,
Andy Shevchenko