Message-ID: <20230924131230.6a999c0f@jic23-huawei>
Date:   Sun, 24 Sep 2023 13:12:30 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Benjamin Bara <bbara93@...il.com>
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] iio: pressure: Support ROHM BU1390

On Thu, 21 Sep 2023 11:17:41 +0300
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> On 9/19/23 17:53, Jonathan Cameron wrote:
> > On Tue, 19 Sep 2023 14:28:29 +0300
> > Matti Vaittinen <mazziesaccount@...il.com> wrote:
> >   
> >> On 9/18/23 15:56, Matti Vaittinen wrote:  
> >>> On 9/17/23 13:35, Jonathan Cameron wrote:  
> >>>> On Fri, 15 Sep 2023 09:56:19 +0300
> >>>> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> >>>>     
> >>>>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> >>>>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> >>>>> averaging and internal FIFO. The sensor does also provide temperature
> >>>>> measurements.
> >>>>>
> >>>>> Sensor does also contain IIR filter implemented in HW. The data-sheet
> >>>>> says the IIR filter can be configured to be "weak", "middle" or
> >>>>> "strong". Some RMS noise figures are provided in data sheet but no
> >>>>> accurate maths for the filter configurations is provided. Hence, the IIR
> >>>>> filter configuration is not supported by this driver and the filter is
> >>>>> configured to the "middle" setting (at least not for now).
> >>>>>
> >>>>> The FIFO measurement mode is only measuring the pressure and not the
> >>>>> temperature. The driver measures temperature when FIFO is flushed and
> >>>>> simply uses the same measured temperature value to all reported
> >>>>> temperatures. This should not be a problem when temperature is not
> >>>>> changing very rapidly (several degrees C / second) but allows users to
> >>>>> get the temperature measurements from sensor without any additional
> >>>>> logic.
> >>>>>
> >>>>> This driver allows the sensor to be used in two muitually exclusive
> >>>>> ways,
> >>>>>
> >>>>> 1. With trigger (data-ready IRQ).
> >>>>> In this case the FIFO is not used as we get data ready for each
> >>>>> collected
> >>>>> sample. Instead, for each data-ready IRQ we read the sample from sensor
> >>>>> and push it to the IIO buffer.
> >>>>>
> >>>>> 2. With hardware FIFO and watermark IRQ.
> >>>>> In this case the data-ready is not used but we enable watermark IRQ. At
> >>>>> each watermark IRQ we go and read all samples in FIFO and push them
> >>>>> to the
> >>>>> IIO buffer.
> >>>>>
> >>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>  
> >>
> >> ...
> >>  
> >>>>> +
> >>>>> +static const unsigned long bm1390_scan_masks[] = {
> >>>>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>>> Why?  Doesn't look hard to support just one or the other?
> >>>> Normally we only do this sort of limitation when there is a heavily
> >>>> optimized read routine for a set of channels and it is better
> >>>> to grab them all and throw away the ones we don't care about.
> >>>> That doesn't seem to be true here. So if the fifo grabbed both
> >>>> temp and pressure it would makes sense here, but doesn't seem
> >>>> like it does.  
> >>>
> >>> I have a feeling I have misunderstood how this mask works. I have
> >>> assumed all the channels with corresponding mask bit _can_ be enabled
> >>> simultaneously, but I have not understood they _must_ all be enabled. I
> >>> think I must go back studying this, but if all channels really _must_ be
> >>> enabled, then you are correct. It actually makes a lot of sense to
> >>> support the pressure values alone, as, according to the data-sheet, the
> >>> HW is doing a "MEMS temperature compensation" to the pressure values.
> >>> So, my assuimption is the temperature data may not be required to be
> >>> captured.
> >>>
> >>> This also means I should revise the scan masks for the BU27008, BU27010
> >>> and BU27034 light sensors as I don't think all the users want all the
> >>> channels enabled. I wonder how I have not noticed any problems when I
> >>> tested those things - did I really always enable all the channels...? @_@
> >>>
> >>> Anyways, Thanks.  
> >>
> >> Hi Jonathan,
> >>
> >> There's something in IIO scan_masks / buffer demuxing that I don't quite
> >> understand  
> 
> Thank You for the patience and the explanation Jonathan. I have a - 
> hopefully not totally wrong - feeling I understand this better. I didn't 
> understand that IIO framework has this extra logic and that 
> available_scan_masks was not really meant for telling what IIO allows 
> _users_ to enable - but it was for driver to tell IIO core what the 
> driver can give. (Naturally this impacts to what IIO allows users to 
> enable, but not directly). Eg, if I now get it right, the 
> 'available_scan_masks' is information what data flows between driver and 
> IIO. Similarly, 'active_scan_mask' tells what the IIO core 'expects' to 
> get from the driver, right? So, in bm1390 case, if I set both:
> 
> BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
> and
> BIT(BM1390_CHAN_PRESSURE),
> 
> to 'available_scan_masks', then it means driver is telling IIO it can 
> give both pressure and temperature, or temperature alone. As a result, 
> IIO can set either both temp and pressure to 'active_scan_mask' - in 
> which case driver should push both in the buffers - or just the 
> pressure, in which case the driver should not push the temperature.

I think you have understood this from the comment further down.  But
key is active_scan_masks is telling the driver what to put in the
buffer via iio_push_to_buffer*().  The driver has to have told
the core what it can provide (available scan masks) and the core is
picking one of those.  So active_scan_mask is always an entry
of available_scan_masks with the special case of no available_scan_masks
meaning any combination of channels is fine.
The chosen value is made up of a superset of the channels enable by
the various consumers: userspace and in kernel consumers. A typical
complex example being a SoC integrated ADCs where a couple of channels
are used for the touch screen and the rest for more general purposes.
In those cases, the in kernel touch screen driver enables the channels
it wants, and the userspace IIO interface might enable some more.
Each consumer just sees what it asks for as the IIO core repacks the
requested channels for each consumer dropping the others (maybe
leaving some valid data in the padding. I can't remember if we let
that happen or not and that code is complex so I'm not going to go
figure it out again today :)  There are some subtle cases around
consumers requesting mutually exclusive sets. We also never really
dealt with how to negotiates sample rates etc, so this isn't
quite as intuitive as it would ideally be.  E.g. the core could
subsample to meet a lower sampling rate request, but IIRC we just
let the multiple consumers fight it out for control :(


> 
> >>
> >> The bm1390 driver as sent in v2 does not do demuxing but always pushes
> >> whole chunk of data and trusts IIO to do demuxing.  
> > 
> > Yup. That should work.  But in this case you can trivially decide not to read
> > or fill in the temperature and hence save some bus cycles.
> >   
> >>
> >> 2) I noticed the 'available_scan_masks' was marked as an optional field.
> >> So, I think that if there is no restrictions to which of the channels
> >> can be enabled, then we can omit setting it. This is what I tried.
> >>
> >> It appears that when we do not populate the 'available_scan_masks' with the:  
> >>   >>> +static const unsigned long bm1390_scan_masks[] = {
> >>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>
> >> things change. When I tested enabling only temperature and ran the
> >> iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported
> >> values seemed completely random.  
> > 
> > You need to pack the data correctly yourself in the driver.
> > So it adds small amount of code complexity but potentially saves bus
> > traffic.  
> 
> ...
> 
> >>
> >> Based on this experimenting (and headache obtained from reading the
> >> demuxing code) - the IIO framework does not do channel demuxing if the
> >> 'available_scan_masks' is not given? To me this was somewhat unexpected.  
> > 
> > Yes.  If you don't tell it what channel setups are available (note there can
> > be lots) you are saying that we support any random combination and have
> > to do the data packing by hand.
> >   
> 
> Okay... I think it kind of makes sense to leave an option where the 
> driver can create buffer as it wants. I am not sure I can wrap my head 
> around this to the extent that I knew when the IIO does not know what 
> channels the driver has enabled - and thus, what types of data there 
> will be - but I can accept the answer that such situations can exist :) 
> Besides, allowing the driver to do the buffer formatting may allow some 
> 'use-case specific, custom optimizations' when user-space knows what to 
> expect (for example, regarding the data alignments and ordering to for 
> example save space).
> 
> ...This leads to another thing I noticed - and to another question :)
> 
> I was experimenting with the bm1390 scan masks and iio_generic_buffer 
> tool. It seems to me that the iio_generic_buffer does not expect the 
> padding when the data in buffer is u32 + u16. When I enable the pressure 
> (32bits) and temperature (16 bits) in bm1390 and keep the time-stamp 
> disabled, the user-space will get buffer where 2 bytes of padding is 
> added to each temperature sample so the next pressure sample stays 
> correctly aligned. I believe kernel side does sane job here.
> 
> This, however, did result (at least my version of) the 
> iio_generic_buffer tool to read garbage values after the first sample. 
> My assumption is that the iio_generic_buffer does not add the padding 
> bytes to the end of last data in the scan in order to ensure also the 
> next scan will be correctly aligned.
> 
Ah. That's definitely possible. We've found similar problems with the logic
those example tools before and I can see that this case might not have turned
up much.

> I did following change to the iio_generic_buffer:
> 
> Author: Matti Vaittinen <mazziesaccount@...il.com>
> Date:   Thu Sep 21 10:15:19 2023 +0300
> 
>      tools: iio: iio_generic_buffer ensure alignment
> 
>      The iio_generic_buffer can return garbage values when the total size of
>      scan data is not a multiple of largest element in the scan. This can be
>      demonstrated by reading a scan consisting for example of one 4 byte and
>      one 2 byte element, where the 4 byte elemnt is first in the buffer.
> 
>      The IIO generic buffert code does not take into accunt the last two
>      padding bytes that are needed to ensure that the 4byte data for next
>      scan is correctly aligned.
> 
>      Add padding bytes required to align the next sample into the scan size.
> 
>      Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 44bbf80f0cfd..fc562799a109 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -54,9 +54,12 @@ enum autochan {
>   static unsigned int size_from_channelarray(struct iio_channel_info 
> *channels, int num_channels)
>   {
>          unsigned int bytes = 0;
> -       int i = 0;
> +       int i = 0, max = 0;
> +       unsigned int misalignment;
> 
>          while (i < num_channels) {
> +               if (channels[i].bytes > max)
> +                       max = channels[i].bytes;
>                  if (bytes % channels[i].bytes == 0)
>                          channels[i].location = bytes;
>                  else
> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct 
> iio_channel_info *channels, in
>                  bytes = channels[i].location + channels[i].bytes;
>                  i++;
>          }
> +       /*
> +        * We wan't the data in next sample to also be properly aligned so
> +        * we'll add padding at the end if needed. TODO: should we use fixed
> +        * 8 byte alignment instead of the size of the biggest samnple?

Nope, biggest sample is correct.

> +        */
> +       misalignment = bytes % max;
> +       if (misalignment) {
> +               printf("Misalignment %u. Adding Padding %u\n", 
> misalignment,  max - misalignment);
> +               bytes += max - misalignment;
> +       }
> 
>          return bytes;
>   }
> 
> I can send this as a proper patch if you guys think it is correct.
Please do.


> 
> 
> >>
> >> Finally, when the watermark IRQ is used, we can't omit reading the
> >> pressure data because clearing the WMI is done based on the pressure
> >> data in FIFO.  
> > 
> > Hmm. That is a good reason to keep the available scan masks set.
> > Add a comment on that next to the mask.
> >   
> >>
> >> So, I would propose we do:
> >>
> >> static const unsigned long bm1390_scan_masks[] = {
> >> 	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
> >> 	BIT(BM1390_CHAN_PRESSURE), 0  
> > 
> > Makes sense given need to read the pressure channel.  
> >>
> >> which better reflects what the hardware is capable of - and, unless I am
> >> missing something, also allows us to offload the buffer demuxing to the IIO.
> >>
> >> Still, as mentioned in 1), the
> >>  
> >>   >>> +static const unsigned long bm1390_scan_masks[] = {
> >>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>
> >> does not seem to prevent enabling only the temperature channel - so in
> >> the driver buffer handler we still must unconditionally read the
> >> pressure data regardles the active_scan_mask.  
> > 
> > active_scan_mask should match the above - it's separate from what is enabled.
> > active_scan_mask is on the data capture side of the demux - the buffers
> > are then fed repacked data reflecting what is enabled.  
> 
> Cool! So, the driver can rely on IIO asking for the pressure (in 
> active_scan_mask) after it has correctly set the available scan mask. It 
> may not be a big thing but I like it. It is enough to take care of 
> ensuring all required stuff is read from HW in one place (in available 
> scan masks) and not to worry about it in actual data reading path but 
> just read stuff IIO is asking to be read. I like it.
> 
> Another thing to note is that, when we build the available_scan_mask 
> array - we should either pay attention to the order of masks - or change 
> the iio_scan_mask_match() to not accept first matching subset but to go 
> through all of the masks unless it finds and exactly matching one (and 
> in general prefer the smallest subset). Not sure this is worth the extra 
> cycles though.

Yes, they do need to be ordered.  I'm not sure if that's clearly documented
or not :(  If not, a patch to add docs on that would be welcome.
I don't think it's worth likely complexity of finding the 'smallest match'
and that's not always obvious.  Imagine a driver that as options

[A, B, C, D] and [A, B, C, E] - how to chose?  May well be more efficient
to do one option than the other (require fewer address writes for the in
order one.  Hence order in will matter anyway so we make it matter completely
with a policy of first match.


> 
> Thanks again - feel like I've learned something today :)
No problem.  Was quite a while ago that we figured out how to make all this
stuff work and I remember it being very fiddle.

Hohum. I should take another spin at our documentation at some point as
I'm sure some of this is less clear than it should be (or not mentioned) :)

Have a few big docs jobs to do (non IIO) at the moment so this will probably
be a 'while'.

Jonathan

> 
> Yours,
> 	-- Matti Vaittinen
> 

Powered by blists - more mailing lists