Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Jonathan Cameron]

On Fri, 15 Mar 2024 14:28:30 +0100
Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:

> On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote:
> > On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:  
> > > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:  
> > > > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:  
> > > > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > > > Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
> > > > >   
> > > > > > The read_press/read_humid functions need the updated t_fine value
> > > > > > in order to calculate the current pressure/humidity. Temperature
> > > > > > reads should be removed from the read_press/read_humid functions
> > > > > > and should be placed in the oneshot captures before the pressure
> > > > > > and humidity reads. This makes the code more intuitive.
> > > > > > 
> > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>  
> > > > > 
> > > > > To me this makes the use of these calls less obvious than they were
> > > > > previously.  The calls are made close to where t_fine is used and
> > > > > don't have to go via the indirection of chip_info.
> > > > > 
> > > > > So I disagree. I think this change makes the code a lot less
> > > > > clear.
> > > > >   
> > > > 
> > > > This was mainly driven by the fact that I wanted to avoid reading
> > > > the temperature 3 times in case temp, press and humid are enabled
> > > > and there are consecutive buffer readings. But thank you for the
> > > > proposal I really appreciate it!
> > > >   
> > > 
> > > Hi, just a side note reflecting on this. Depending on your sampling
> > > frequency
> > > and registers data shadowing, to avoid compensating with different samples
> > > between readings, we should be doing burst readings to get a bundle of the
> > > temperature+pressure and/or humidity.
> > > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on
> > > the
> > > memory. On the bmp580 this is not a problem as the values are already
> > > compensated, you`ll get always the latest reading.
> > > 
> > > Kind regard,
> > > Angel  
> > 
> > Hi Angel,
> > 
> > Thank you for pointing this out! Indeed that's true but I noticed that this is
> > not
> > the case for the BMP{085/180} devices. I just feel that some changes might
> > make
> > data acquisition/processing faster for a device (like the one you proposed)
> > but
> > it might make the code much more unreadable and unmaintanable. I will try and
> > see if something could be done in that sense but I feel that keeping it simple
> > will
> > be good for everyone!
> > 
> > Cheers,
> > Vasilis  
> 
> Yeah, data adquisition on bmp085/180 is already different as they don't support
> continuous mode as the newer models and you have to warm-up the sensor and do
> one-shot readings. There's already a different code path in place for that
> models. I guess that is the price to pay to support that wide range of
> sensors...
> Anyway, this patches are already big and you're doing quite a lot of heavy-
> lifting right now, so don't pay much attention to my ramblings! Regardless,
> happy to help with tasks polishing and updating this driver :)

If burst readings do make sense: We can reasonably assume anyone who is using
this sensor and is using buffered mode probably wants to 'mostly' grab all the
channels, then a specific function that always grabs them all, plus use of
available_scan_masks = { ALL BITS, 0 }; will let the IIO core deal with any
cases where only some channels are requested.  This is something we
do in drivers where there is some interaction between the channels (like here)
or where burst reads are much more efficient than single channels (possibly
also true here) and complexity is significant for switching between burst
and single channels reads.

That covers a lot of devices and is part of why we have the core code
do channel de-multiplexing rather than leaving it for the drivers. The other
reason that drove that complexity was unrelated to this driver (SoC ADCs with
some channels used for touchscreens, and others for unrelated purposes).

You may just want to reduce how much code you are reusing from
the oneshot single channel sysfs reads so that you can just do a single set
of readings and use them as needed for compensation etc.

Jonathan

 
> 
> Kind regards,
> Angel
>