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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240722203138.07b21300@jic23-huawei>
Date: Mon, 22 Jul 2024 20:31:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: 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, 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.

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

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.

Jonathan



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