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
