From: Ladislav Michl <ladis@linux-mips.org>
To: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>,
linux-iio@vger.kernel.org,
Daniel Baluta <daniel.baluta@gmail.com>
Subject: Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion
Date: Tue, 22 Aug 2017 16:36:02 +0200 [thread overview]
Message-ID: <20170822143602.fknflnomr6ng7min@lenoch> (raw)
In-Reply-To: <CAC5umygwstJv0aeWELWz5OL04zdS3T=zRZkVjPYxZ2iTerU=Ww@mail.gmail.com>
On Tue, Aug 22, 2017 at 07:03:12PM +0900, Akinobu Mita wrote:
> 2017-08-22 6:00 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
> > On Sun, Aug 20, 2017 at 11:57:30AM +0100, Jonathan Cameron wrote:
> >> On Sun, 23 Jul 2017 12:36:18 +0100
> >> Jonathan Cameron <jic23@kernel.org> wrote:
> >>
> >> > On Fri, 21 Jul 2017 00:24:22 +0900
> >> > Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >> >
> >> > > This driver assumes that the device is operating in the continuous
> >> > > conversion mode which performs the conversion continuously. So this driver
> >> > > inserts a wait time before reading the conversion register if the
> >> > > configuration is changed from a previous request.
> >> > >
> >> > > Currently, the wait time is only the period required for a single
> >> > > conversion that is calculated as the reciprocal of the sampling frequency.
> >> > > However we also need to wait for the the previous conversion to complete.
> >> > > Otherwise we probably get the conversion result for the previous
> >> > > configuration when the sampling frequency is lower.
> >> > >
> >> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> >> > > Cc: Jonathan Cameron <jic23@kernel.org>
> >> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> > Assuming Daniel is happy with these, I propose to take these
> >> > first 6 through the fixes-togreg branch and mark them all for stable.
> >>
> >> I changed my mind on this given the late staging in the cycle and
> >> am pushing them all through the togreg branch. The fixes can then
> >> be picked up by stable post merge window which may be the quickest
> >> route at the moment!
> >
> > Tested this patch serie and something is still odd, see bellow...
> > Once sorted out, proper patches will be generated.
> >
> >>
> >> Thanks,
> >>
> >> Jonathan
> >> >
> >> > The rest may well have to wait on those patches coming back
> >> > around and into the togreg branch of iio.git.
> >> >
> >> > Hence it may be at least a few weeks.
> >> >
> >> > Jonathan
> >> > > ---
> >> > > drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
> >> > > 1 file changed, 19 insertions(+), 12 deletions(-)
> >> > >
> >> > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> > > index 1c475e2..9c501e5 100644
> >> > > --- a/drivers/iio/adc/ti-ads1015.c
> >> > > +++ b/drivers/iio/adc/ti-ads1015.c
> >> > > @@ -242,27 +242,34 @@ static
> >> > > int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >> > > {
> >> > > int ret, pga, dr, conv_time;
> >> > > - bool change;
> >> > > + unsigned int old, mask, cfg;
> >> > >
> >> > > if (chan < 0 || chan >= ADS1015_CHANNELS)
> >> > > return -EINVAL;
> >> > >
> >> > > + ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
> >> > > + if (ret)
> >> > > + return ret;
> >> > > +
> >> > > pga = data->channel_data[chan].pga;
> >> > > dr = data->channel_data[chan].data_rate;
> >> > > + mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> >> > > + ADS1015_CFG_DR_MASK;
> >> > > + cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >> > > + dr << ADS1015_CFG_DR_SHIFT;
> >> > >
> >> > > - ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
> >> > > - ADS1015_CFG_MUX_MASK |
> >> > > - ADS1015_CFG_PGA_MASK |
> >> > > - ADS1015_CFG_DR_MASK,
> >> > > - chan << ADS1015_CFG_MUX_SHIFT |
> >> > > - pga << ADS1015_CFG_PGA_SHIFT |
> >> > > - dr << ADS1015_CFG_DR_SHIFT,
> >> > > - &change);
> >> > > - if (ret < 0)
> >> > > + cfg = (old & ~mask) | (cfg & mask);
> >> > > +
> >> > > + ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> >> > > + if (ret)
> >> > > return ret;
> >> > >
> >> > > - if (change || data->conv_invalid) {
> >> > > - conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >> > > + if (old != cfg || data->conv_invalid) {
> >> > > + int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> >> > > + ADS1015_CFG_DR_SHIFT;
> >> > > +
> >> > > + conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> >> > > + conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >> > > usleep_range(conv_time, conv_time + 1);
> >> > > data->conv_invalid = false;
> >> > > }
> >
> > Btw, this could be optimized futher this way:
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
> > static
> > int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> > {
> > - int ret, pga, dr, conv_time;
> > + int ret, pga, dr, dr_old, conv_time;
> > unsigned int old, mask, cfg;
> >
> > if (chan < 0 || chan >= ADS1015_CHANNELS)
> > @@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> > ADS1015_CFG_DR_MASK;
> > cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> > dr << ADS1015_CFG_DR_SHIFT;
> > -
> > cfg = (old & ~mask) | (cfg & mask);
> >
> > - ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > - if (ret)
> > - return ret;
> > -
> > if (old != cfg || data->conv_invalid) {
> > - int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> > - ADS1015_CFG_DR_SHIFT;
> > + ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > + if (ret)
> > + return ret;
>
> You can also skip config register write in the case old == cfg &&
> data->conv_invalid.
Yes, something like this:
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
static
int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
{
- int ret, pga, dr, conv_time;
+ int ret, pga, dr, dr_old, conv_time;
unsigned int old, mask, cfg;
if (chan < 0 || chan >= ADS1015_CHANNELS)
@@ -256,17 +256,16 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
ADS1015_CFG_DR_MASK;
cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
dr << ADS1015_CFG_DR_SHIFT;
-
cfg = (old & ~mask) | (cfg & mask);
- ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
- if (ret)
- return ret;
-
- if (old != cfg || data->conv_invalid) {
- int dr_old = (old & ADS1015_CFG_DR_MASK) >>
- ADS1015_CFG_DR_SHIFT;
-
+ if (old != cfg) {
+ ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
+ if (ret)
+ return ret;
+ data->conv_invalid = true;
+ }
+ if (data->conv_invalid) {
+ dr_old = (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_SHIFT;
conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
usleep_range(conv_time, conv_time + 1);
I was temped to use goto, but compiler will probably optimize anyway.
> > + dr_old = (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_SHIFT;
> > conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> > conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > usleep_range(conv_time, conv_time + 1);
> >
> >
> > Now that conv_time is too picky, at first "ADS1015EVM, ADS1115EVM,
> > ADS1015EVM-PDK, ADS1115EVM-PDK User Guide (Rev. B)" [*] states at page 16:
> > "Note that both the ADS1115 and ADS1015 have internal clocks with a ±10%
> > accuracy. If performing FFT tests, frequencies may appear to be incorrect
> > as a result of this tolerance range.", so we should add at least those 10%
> > and at second conv_time + 1 is too precise, increasing number of undesired
> > interrupts, so adding min(dr_period, dr_old_period) instead of 1 should do
> > the trick.
>
> Can we just simply do like below?
>
> conv_time = conv_time * 11 / 10
>
> or
>
> conv_time += conv_time / 10;
That's definitely enough. Question is whenever we should care about usleep_range
being only 1us wide. See Documentation/timers/timers-howto.txt
> > But the real showstopper is increasing probability of reading stale result
> > with lowering sample frequency, to debug this following dirty modification
> > was made to driver:
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -269,6 +269,11 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >
> > conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> > conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > + for (ret = 0; ret < 20; ret++) {
> > + regmap_read(data->regmap, ADS1015_CONV_REG, val);
> > + printk("%d -> %d\n", ret, *val);
> > + usleep_range(conv_time, conv_time + 1);
> > + }
> > usleep_range(conv_time, conv_time + 1);
> > data->conv_invalid = false;
> > }
> >
> > With ADS1015 and sampling frequency set to 128 SPS this leads to:
> > $ cat in_voltage0_raw && cat in_voltage1_raw
> > [285276.998382] mode 0
> > [285277.005096] cfg 4003
> > [285277.015380] 0 -> 13713
> > [285277.037109] 1 -> 13713
> > [285277.058898] 2 -> 13713
> > [285277.080291] 3 -> 13713
> > [285277.101715] 4 -> 13713
> > [285277.123291] 5 -> 13713
> > [285277.144836] 6 -> 13704
> > [285277.166503] 7 -> 13704
> > [285277.188262] 8 -> 13704
> > [285277.210083] 9 -> 13704
> > [285277.231811] 10 -> 13704
> > [285277.253570] 11 -> 13704
> > [285277.275390] 12 -> 3083
> > [285277.296936] 13 -> 3083
> > [285277.318023] 14 -> 3083
> > [285277.339172] 15 -> 3083
> > [285277.360076] 16 -> 3083
> > [285277.381164] 17 -> 3083
> > [285277.402252] 18 -> 3076
> > [285277.423339] 19 -> 3076
> > 192
> > [285277.463623] cfg 5003
> > [285277.468933] 0 -> 3076
> > [285277.489746] 1 -> 3076
> > [285277.510955] 2 -> 3074
> > [285277.531585] 3 -> 3074
> > [285277.552612] 4 -> 3074
> > [285277.573272] 5 -> 3074
> > [285277.594207] 6 -> 3074
> > [285277.615173] 7 -> 3074
> > [285277.636016] 8 -> 13702
> > [285277.657073] 9 -> 13702
> > [285277.678131] 10 -> 13702
> > [285277.699493] 11 -> 13702
> > [285277.721374] 12 -> 13702
> > [285277.742492] 13 -> 13702
> > [285277.763702] 14 -> 13701
> > [285277.784820] 15 -> 13701
> > [285277.806030] 16 -> 13701
> > [285277.827178] 17 -> 13701
> > [285277.848480] 18 -> 13701
> > [285277.869689] 19 -> 13701
> > 856
> >
> > As you can see, it took way longer to switch channel than sampling preriod.
> > Anyone has a clue what is going on here?
>
> Looks like your device is ADS1115 but you use it as ADS1015.
Package mark reads 03 BOGI, which is indeed ADS1115. That's a shame and I
have no excuse for it. Previous samples come with BRPI and I didn't check
this time. Sorry for the noise.
> The slowest sampling rate is 128 SPS for ads1015 and 8 SPS for ads1115
> when DR field in config register is set to zero. According to the kernel
> debug log message that you have added, the device is working with 8 SPS.
>
> > Thank you,
> > ladis
> >
> > [*] http://www.ti.com/lit/ug/sbau157b/sbau157b.pdf
next prev parent reply other threads:[~2017-08-22 14:36 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-20 15:24 [PATCH v2 00/11] iio: adc: ti-ads1015: fixes, cleanups, and threshold event support Akinobu Mita
2017-07-20 15:24 ` [PATCH v2 01/11] iio: adc: ti-ads1015: fix incorrect data rate setting update Akinobu Mita
2017-07-23 11:32 ` Jonathan Cameron
2017-08-20 10:51 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 02/11] iio: adc: ti-ads1015: fix scale information for ADS1115 Akinobu Mita
2017-08-20 10:53 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 03/11] iio: adc: ti-ads1015: enable conversion when CONFIG_PM is not set Akinobu Mita
2017-08-20 10:54 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 04/11] iio: adc: ti-ads1015: avoid getting stale result after runtime resume Akinobu Mita
2017-08-20 10:55 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 05/11] iio: adc: ti-ads1015: don't return invalid value from buffer setup callbacks Akinobu Mita
2017-08-20 10:56 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion Akinobu Mita
2017-07-23 11:36 ` Jonathan Cameron
2017-08-20 10:57 ` Jonathan Cameron
2017-08-21 21:00 ` Ladislav Michl
2017-08-22 10:03 ` Akinobu Mita
2017-08-22 14:36 ` Ladislav Michl [this message]
2017-08-23 13:57 ` Akinobu Mita
2017-07-20 15:24 ` [PATCH v2 07/11] iio: adc: ti-ads1015: remove unnecessary config register update Akinobu Mita
2017-08-20 10:58 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 08/11] iio: adc: ti-ads1015: add helper to set conversion mode Akinobu Mita
2017-08-20 10:59 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 09/11] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup Akinobu Mita
2017-08-20 11:00 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 10/11] iio: adc: ti-ads1015: use iio_device_claim_direct_mode() Akinobu Mita
2017-08-20 11:00 ` Jonathan Cameron
2017-07-20 15:24 ` [PATCH v2 11/11] iio: adc: ti-ads1015: add threshold event support Akinobu Mita
2017-07-23 12:01 ` Jonathan Cameron
2017-08-20 11:05 ` Jonathan Cameron
2017-08-22 10:20 ` Akinobu Mita
2017-08-22 12:35 ` Jonathan Cameron
2017-08-22 12:38 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170822143602.fknflnomr6ng7min@lenoch \
--to=ladis@linux-mips.org \
--cc=akinobu.mita@gmail.com \
--cc=daniel.baluta@gmail.com \
--cc=jic23@jic23.retrosnub.co.uk \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox