Message-ID: <20240722203834.GA468105@vamoiridPC>
Date: Mon, 22 Jul 2024 22:38:34 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, lars@...afoo.de,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	andriy.shevchenko@...ux.intel.com, ang.iglesiasg@...il.com,
	linus.walleij@...aro.org, biju.das.jz@...renesas.com,
	javier.carrasco.cruz@...il.com, semen.protsenko@...aro.org,
	579lpy@...il.com, ak@...klinger.de, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger
 support

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@...il.com> 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@...il.com> 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@...il.com>  
> > > 
> > > 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.

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

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

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

Powered by blists - more mailing lists