All of lore.kernel.org
 help / color / mirror / Atom feed
From: Akinobu Mita <akinobu.mita@gmail.com>
To: Ladislav Michl <ladis@linux-mips.org>
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: Wed, 23 Aug 2017 22:57:14 +0900	[thread overview]
Message-ID: <CAC5umygnP1OqZL96MqhH4OLBxHaqPGQ8wi2tdhyXQMdCyK6K-A@mail.gmail.com> (raw)
In-Reply-To: <20170822143602.fknflnomr6ng7min@lenoch>

2017-08-22 23:36 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
> 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 continuou=
s
>> >> > > conversion mode which performs the conversion continuously.  So t=
his 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 f=
requency.
>> >> > > However we also need to wait for the the previous conversion to c=
omplete.
>> >> > > 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 stabl=
e.
>> >>
>> >> 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-ad=
s1015.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 >=3D ADS1015_CHANNELS)
>> >> > >           return -EINVAL;
>> >> > >
>> >> > > + ret =3D regmap_read(data->regmap, ADS1015_CFG_REG, &old);
>> >> > > + if (ret)
>> >> > > +         return ret;
>> >> > > +
>> >> > >   pga =3D data->channel_data[chan].pga;
>> >> > >   dr =3D data->channel_data[chan].data_rate;
>> >> > > + mask =3D ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
>> >> > > +         ADS1015_CFG_DR_MASK;
>> >> > > + cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_=
SHIFT |
>> >> > > +         dr << ADS1015_CFG_DR_SHIFT;
>> >> > >
>> >> > > - ret =3D 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 =3D (old & ~mask) | (cfg & mask);
>> >> > > +
>> >> > > + ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> >> > > + if (ret)
>> >> > >           return ret;
>> >> > >
>> >> > > - if (change || data->conv_invalid) {
>> >> > > -         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rat=
e[dr]);
>> >> > > + if (old !=3D cfg || data->conv_invalid) {
>> >> > > +         int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
>> >> > > +                         ADS1015_CFG_DR_SHIFT;
>> >> > > +
>> >> > > +         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rat=
e[dr_old]);
>> >> > > +         conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr]);
>> >> > >           usleep_range(conv_time, conv_time + 1);
>> >> > >           data->conv_invalid =3D 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 >=3D ADS1015_CHANNELS)
>> > @@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *=
data, int chan, int *val)
>> >                 ADS1015_CFG_DR_MASK;
>> >         cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA=
_SHIFT |
>> >                 dr << ADS1015_CFG_DR_SHIFT;
>> > -
>> >         cfg =3D (old & ~mask) | (cfg & mask);
>> >
>> > -       ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> > -       if (ret)
>> > -               return ret;
>> > -
>> >         if (old !=3D cfg || data->conv_invalid) {
>> > -               int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
>> > -                               ADS1015_CFG_DR_SHIFT;
>> > +               ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cf=
g);
>> > +               if (ret)
>> > +                       return ret;
>>
>> You can also skip config register write in the case old =3D=3D 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_dat=
a *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 >=3D ADS1015_CHANNELS)
> @@ -256,17 +256,16 @@ int ads1015_get_adc_result(struct ads1015_data *dat=
a, int chan, int *val)
>                 ADS1015_CFG_DR_MASK;
>         cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SH=
IFT |
>                 dr << ADS1015_CFG_DR_SHIFT;
> -
>         cfg =3D (old & ~mask) | (cfg & mask);
>
> -       ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> -       if (ret)
> -               return ret;
> -
> -       if (old !=3D cfg || data->conv_invalid) {
> -               int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
> -                               ADS1015_CFG_DR_SHIFT;
> -
> +       if (old !=3D cfg) {
> +               ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> +               if (ret)
> +                       return ret;
> +               data->conv_invalid =3D true;
> +       }
> +       if (data->conv_invalid) {
> +               dr_old =3D (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_=
SHIFT;
>                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[=
dr_old]);
>                 conv_time +=3D 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.

Looks good to me.

>> > +               dr_old =3D (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_=
DR_SHIFT;
>> >                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr_old]);
>> >                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_r=
ate[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 =
=C2=B110%
>> > accuracy. If performing FFT tests, frequencies may appear to be incorr=
ect
>> > 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 undes=
ired
>> > interrupts, so adding min(dr_period, dr_old_period) instead of 1 shoul=
d do
>> > the trick.
>>
>> Can we just simply do like below?
>>
>>         conv_time =3D conv_time * 11 / 10
>>
>> or
>>
>>         conv_time +=3D conv_time / 10;
>
> That's definitely enough. Question is whenever we should care about uslee=
p_range
> being only 1us wide. See Documentation/timers/timers-howto.txt

I have no particular opinion on how large range we should choose for
the usleep_range call.  The range has been 1us since this driver has
been committed at first time.

If there is a reasonable range other than 1us, it should be prepared as
a separate patch.  Because it is separate issue from increasing the
minimum conv_time by 10%.

>> > But the real showstopper is increasing probability of reading stale re=
sult
>> > with lowering sample frequency, to debug this following dirty modifica=
tion
>> > 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 *d=
ata, int chan, int *val)
>> >
>> >                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr_old]);
>> >                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_r=
ate[dr]);
>> > +               for (ret =3D 0; ret < 20; ret++) {
>> > +                       regmap_read(data->regmap, ADS1015_CONV_REG, va=
l);
>> > +                       printk("%d -> %d\n", ret, *val);
>> > +                       usleep_range(conv_time, conv_time + 1);
>> > +               }
>> >                 usleep_range(conv_time, conv_time + 1);
>> >                 data->conv_invalid =3D 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 pre=
riod.
>> > 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.

No problem. Thanks for testing.

>> 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 kerne=
l
>> 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

  reply	other threads:[~2017-08-23 13:57 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
2017-08-23 13:57             ` Akinobu Mita [this message]
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=CAC5umygnP1OqZL96MqhH4OLBxHaqPGQ8wi2tdhyXQMdCyK6K-A@mail.gmail.com \
    --to=akinobu.mita@gmail.com \
    --cc=daniel.baluta@gmail.com \
    --cc=jic23@jic23.retrosnub.co.uk \
    --cc=ladis@linux-mips.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.