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

Vasileios Amoiridis
2024-08-24T12:02:26Z

On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> 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
>

But the function returns an irqreturn_t type, no?

> ...
>
> > + fwnode = dev_fwnode(data->dev);
> > + if (!fwnode)
> > + return -ENODEV;
>
> Why do you need this? The below will fail anyway.

Because If I don't make this check then fwnode might be garbage and I will
pass garbage to the fwnode_irq_get() function. Or do I miss something?

>
> > + irq = fwnode_irq_get(fwnode, 0);
> > + if (!irq)
>
> Are you sure this is correct check?
>
Well, I think yes, because the function return either the Linux IRQ number
on success or a negative errno on failure.

https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

> > + return dev_err_probe(data->dev, -ENODEV,
>
> Shadowed error code.
>

I am not sure I understand what you mean here. You mean that there is no
chance that the first one will pass and this one will fail?

> > + "No interrupt found.\n");
>
> > + desc = irq_get_irq_data(irq);
> > + if (!desc)
> > + return -EINVAL;
>
> When may this fail?
>

I think that this will fail when Linux were not able to actually
register that interrupt.

> > + 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");
>

Indeed, thanks!

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

Well, that's true.

>
> > + 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;
>
> ?

All the other if statements follow the style that I typed. If I
follow yours, will make it different just for this one, does it
make sense?

Cheers,
Vasilis
>
> > +}
>
> ...
>
> > +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.
>

True, makes sense.

> > + 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;
>
> ?

Yes, you are right.

>
> > +}
>
> ...
>
> > +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 ?
>

ACK.

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

Well, it looks much more beautiful indeed. Thanks again for the feedback
Andy, I really appreciate it a lot!

Cheers,
Vasilis