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

From: Jonathan Cameron
Date: Mon Aug 26 2024 - 06:02:08 EST


On Sat, 24 Aug 2024 14:02:22 +0200
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:

> 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?
irq_handler_t is a typdef for the full function signature.
It will still return irqreturn_t
>
> > ...
> >
> > > + 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?
It checks for NULL which is all it can actually be and returns a suitable
error code if it is.

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

Indeed, so if (irq < 0)
return dev_err_probe(data->dev, irq, ...)

carry on as valid irq.
your error check if returning only if irq == 0 which never
happens (due to catch for that in the code you link).
Negative values are true, so !-EINVAL == false
for example.
>