Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c2aeab83-7bd8-a1b2-5dfa-4e99f2250aeb@gmail.com>
Date:   Mon, 26 Sep 2022 13:28:41 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Content-Language: en-US
To:     Matti Vaittinen <mazziesaccount@gmail.com>,
        "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Alexandru Ardelean <alexandru.ardelean@analog.com>,
        Mihail Chindris <mihail.chindris@analog.com>,
        Miaoqian Lin <linmq006@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: bmc150-accel-core: potential timestamp calculation issue.
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi dee Ho peeps!

I selected you as a recipient purely based on get_maintainer.pl. Please 
drop me a note if the bmc150-accel-core.c does not feel like your 
playground and I'll drop you from the CC if I send further mails for 
this topic. Sorry in advance.

I was ruthlessly copying timestamp logic from the bmc150-accel-core.c 
for my accelerometer driver. I noticed there may be a problem if FIFO is 
not read empty:

The code computes time between two samples by

...
          *
          * To avoid this issue we compute the actual sample period 
ourselves
          * based on the timestamp delta between the last two flush 
operations.
          */
         sample_period = (data->timestamp - data->old_timestamp);
         do_div(sample_period, count);
         tstamp = data->timestamp - (count - 1) * sample_period;

Here the "count" is amount of samples bmc-150 reports there is in fifo.

As far as I understand, this works only if the old_timestamp match the 
time when first sample in FIFO has been captured. This is true if the 
previous 'flush' (where the old_timestamp is stored) has emptied the 
FIFO - but as far as I understand the IIO does not guarantee the flush 
reading all samples there are in the FIFO.

If the flush leaves - say 10 samples in FIFO, then at the next flush 
there old_timestamp will be time when FIFO was previously flushed, but 
this time does not match the first sample in FIFO, but the 6.th. Let's 
say the sensor has collected 10 new samples between the flushes. This 
would mean there is now 10 new and 5 old samples in FIFO, totaling 15 
samples. Yet, the time difference computed by

sample_period = (data->timestamp - data->old_timestamp);

will reflect time between flushes - which means time of 10 samples. The 
division which should compute time between two samples:

do_div(sample_period, count);

will now use incorrect amount of samples (10 + 6) and the sample period 
will be too small. (Or is there something I don't quite understand).

I added following piece of code in the driver I am developing:

        if (samples && count > samples) {
                 /*
                  * Here we leave some old samples to the buffer. We need to
                  * adjust the timestamp to match the first sample in 
the buffer
                  * or we will miscalculate the sample_period at next round.
                  */
                 data->timestamp -= (count - samples) * sample_period;
                 count = samples;
         }

I can send this as a patch also to the bmc150-accel-core.c - but I'd 
prefer someone who is familiar with this IC to verify my finding and 
suggested fix :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~