Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Mon, 22 Jul 2024 22:38:34 +0200
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:

> On Mon, Jul 22, 2024 at 08:31:38PM +0100, Jonathan Cameron wrote:
> > On Mon, 22 Jul 2024 01:51:13 +0200
> > Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
> >   
> > > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote:  
> > > > On Thu, 11 Jul 2024 23:15:57 +0200
> > > > Vasileios Amoiridis <vassilisamir@xxxxxxxxx> 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.
> > > > > 
> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>    
> > > > 
> > > > A few minor things inline.
> > > > 
> > > > It might be worth thinking a bit about future fifo support as that can
> > > > get a little messy in a driver that already supports a dataready trigger.
> > > > We end up with no trigger being set meaning use the fifo.  Sometimes
> > > > it makes more sense to not support triggers at all.
> > > > 
> > > > What you have here is fine though as we have a bunch of drivers
> > > > that grew dataready trigger support before adding fifos later
> > > > particularly as often it's a 'new chip' that brings the fifo
> > > > support but maintains backwards compatibility if you don't use it.
> > > >     
> > > 
> > > Hi Jonathan,
> > > 
> > > Thank you very much for your thorough review again!
> > > 
> > > What I could do to make the code even better to be able to accept
> > > FIFO irq support are the following:
> > > 
> > > 1) in the bmp{380/580}_trigger_handler() currently, the data registers
> > > are being read. What I could do is to move the reading of registers
> > > to a separe function like bmpxxx_drdy_trigger_handler() and calling
> > > it inside the bmp{380/580}_trigger_handler() when I have DRDY or
> > > sysfs irq. In order to check the enabled irqs I propose also no.2  
> > 
> > You shouldn't get to the trigger_handler by other paths.  But sure 
> > a bit of code reuse might make sense if fifo read out path is same
> > as for other data reads.  Superficially it looks totally different
> > on the bmp380 though as there is a separate fifo register.
> >   
> 
> So, I don't mean getting into the trigger_handler by other paths. I will
> always end up in the trigger_handler and then, depending on the interrupt
> that was triggered (DRDY, FIFO, etc...) I choose different actions.

Often the right thing to do for a fifo is to not use a trigger at all.
So instead it becomes the main interrupt handler that runs that path.
The reason being that triggers are typically one per 'scan' i.e. set of
channels and fifo interrupts are much less common.  That makes a mess
of things like timestamps etc that means different handling is necessary.
So for fifo paths we often just don't use a trigger.


> 
> > > 
> > > 2) in the following bmp{380/580}_trigger_probe() functions instead of
> > > just doing:
> > > 
> > >        irq = fwnode_irq_get_byname(fwnode, "DRDY");
> > >        if (!irq) {
> > >                dev_err(data->dev, "No DRDY interrupt found\n");
> > >                return -ENODEV;
> > >        }
> > > 
> > > I could also use some type of variable like we do for the active
> > > channels in order to track "active/existing irqs".  
> > 
> > I think there is only one IRQ on the 380 at least.  So
> > you should only request it once for this driver.  Then software
> > gets to configure what it is for.
> > 
> > However it shouldn't be called DRDY for these parts at least. It's
> > just INT on the datasheet.
> > The interrupt control register value will tell you what is enabled.
> > No need to track it separately.
> >   
> 
> So I am a bit confused. Indeed, BMP380 has only irq line. So I understand
> why I should call it INT. The actual IRQ inside the chip that will be 
> triggered needs to be configured by software. I do it through the
> bmp{3/5}80_int_config() function. How am I supposed to know
> which IRQ to enable? Options are:
> 
> 	a) DRDY only
> 	b) FIFO only
> 	c) both
> 
> How can I inform the driver about which to enable? Shouldn't this go
> through the device-tree?

No. This is a policy question for the driver, not something to do
with wiring (which is what belongs in device tree).
You choose how it is used based on what userspace configures the
device to do.

DRDY if it uses that trigger.  FIFO typically if the buffer is enabled
without a trigger (though we have a few cases where a dummy trigger
is used for this).  


> 
> > If you mean track the one from the poll function registered for
> > handling triggers - that's an internal detail but you would indeed
> > need to track in your data structures whether that's the trigger
> > currently being used or not (easy to do by comparing iio_dev->trig
> > with a pointer for each trigger in iio_priv() data - so should be no
> > need for separate tracking.
> >   
> 
> My idea is that there is one trigger_handler which inside you check
> which interrupt triggered and you choose which path to choose. If that's
> wrong, do you know any specific driver that implements it in the correct
> way? Because in my mind, the iio_dev->trig is one and just inside the
> handler, different functions are called.

You don't have to use a trigger.  Just look for drivers that handle
a fifo. This is pretty common situation and there are a couple of
different solutions but often the cleanest is to not use the
trigger infrastructure at all if the fifo is in use.

Jonathan

> 
> > Jonathan
> > 
> >   
> 
> Thanks for the feedback :)
> 
> Cheers,
> Vasilis
> 
> >   
> > > 
> > > Like this it would be easier to track the active irqs of the sensor.
> > > 
> > > Let me know what you think, or if I manage to have time I might send
> > > a v2 with those changes even earlier :)
> > > 
> > > Cheers,
> > > Vasilis
> > >   
> >