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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 24 Aug 2024 14:02:22 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> 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.
>