2016-03-01 08:35:08

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

On 01.03.2016 02:42, Michael Welling wrote:
> On Mon, Feb 29, 2016 at 10:09:10PM -0300, Lucas De Marchi wrote:
>> On Mon, Feb 29, 2016 at 9:50 PM, Michael Welling <[email protected]>
>> wrote:
>> > On Fri, Feb 05, 2016 at 03:17:18PM +0200, Daniel Baluta wrote:
>> >> The driver has sysfs readings with runtime PM support for power saving.
>> >> It also offers buffer support that can be used together with IIO software
>> >> triggers.
>> >>
>> >
>> > Daniel,
>> >
>> > So I noticed something yesterday while testing new boards.
>> > The channels are occassionally swapping when accessing data from multiple channels.
>> >
>> > I wrote a simple bash script to demonstrate.
>>
>> This happened to me in a previous version of the patch. I remember it
>> being fixed in the last version (or at least I could not reproduce).
>> I'll test again tomorrow with your script.
>>
>
> Here is what I believe is happening.
>
> The request for a conversion on a new channel comes in while the
> conversion
> for the previous channel is still converting. The driver waits
> approximately
> one conversion cycle. The previous channel completes within this
> timeframe
> and the MUX is changed and the new sample is started. The new sample is
> still
> converting and the driver returns the value from the previous
> conversion.
>
> For a test I multiplied the conv_time value by 2 in the
> ads1015_get_adc_result
> function. This allows time for the current sample flush out and always
> returns
> the appropriate channel's value.
>
> Looking at the buffered mode it appears that only one channel is being
> accessed
> at any time. This being the first one in the active_scan_mask found by
> find_first_bit. So the MUX would never change is buffered mode as far I
> can
> tell.
>
> Don't we typically want to read all of enabled channels in buffered
> mode?
In some devices that is effectively impossible (fifo's that fill only
from the currently
selected channel). That is what the onehot validation callback is about.
In this particular case it looks like doing a multichannel read is fair
bit more time
consuming than a single channel read and so will result in a
considerable reduction in
throughput. This is of course why many parts include a simple
sequencer!

Anyhow, supporting multi channel buffered reads could be done (probably)
but you
would want to fall back to the single channel case as it is now.

Jonathan
>> Lucas De Marchi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2016-03-01 16:02:15

by Michael Welling

[permalink] [raw]
Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

On Tue, Mar 01, 2016 at 08:35:01AM +0000, [email protected] wrote:
> On 01.03.2016 02:42, Michael Welling wrote:
> >On Mon, Feb 29, 2016 at 10:09:10PM -0300, Lucas De Marchi wrote:
> >>On Mon, Feb 29, 2016 at 9:50 PM, Michael Welling <[email protected]>
> >>wrote:
> >>> On Fri, Feb 05, 2016 at 03:17:18PM +0200, Daniel Baluta wrote:
> >>>> The driver has sysfs readings with runtime PM support for power saving.
> >>>> It also offers buffer support that can be used together with IIO software
> >>>> triggers.
> >>>>
> >>>
> >>> Daniel,
> >>>
> >>> So I noticed something yesterday while testing new boards.
> >>> The channels are occassionally swapping when accessing data from multiple channels.
> >>>
> >>> I wrote a simple bash script to demonstrate.
> >>
> >>This happened to me in a previous version of the patch. I remember it
> >>being fixed in the last version (or at least I could not reproduce).
> >>I'll test again tomorrow with your script.
> >>
> >
> >Here is what I believe is happening.
> >
> >The request for a conversion on a new channel comes in while the
> >conversion
> >for the previous channel is still converting. The driver waits
> >approximately
> >one conversion cycle. The previous channel completes within this timeframe
> >and the MUX is changed and the new sample is started. The new sample is
> >still
> >converting and the driver returns the value from the previous conversion.
> >
> >For a test I multiplied the conv_time value by 2 in the
> >ads1015_get_adc_result
> >function. This allows time for the current sample flush out and always
> >returns
> >the appropriate channel's value.
> >
> >Looking at the buffered mode it appears that only one channel is being
> >accessed
> >at any time. This being the first one in the active_scan_mask found by
> >find_first_bit. So the MUX would never change is buffered mode as far I
> >can
> >tell.
> >
> >Don't we typically want to read all of enabled channels in buffered mode?
> In some devices that is effectively impossible (fifo's that fill only from
> the currently
> selected channel). That is what the onehot validation callback is about.
> In this particular case it looks like doing a multichannel read is fair bit
> more time
> consuming than a single channel read and so will result in a considerable
> reduction in
> throughput. This is of course why many parts include a simple sequencer!

Yes the sampling rate would effectively be divided by the number of channels
enabled.

>
> Anyhow, supporting multi channel buffered reads could be done (probably) but
> you
> would want to fall back to the single channel case as it is now.

Understood.

>
> Jonathan
> >>Lucas De Marchi
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> >the body of a message to [email protected]
> >More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-03-01 17:25:32

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support



On 1 March 2016 16:02:00 GMT+00:00, Michael Welling <[email protected]> wrote:
>On Tue, Mar 01, 2016 at 08:35:01AM +0000, [email protected]
>wrote:
>> On 01.03.2016 02:42, Michael Welling wrote:
>> >On Mon, Feb 29, 2016 at 10:09:10PM -0300, Lucas De Marchi wrote:
>> >>On Mon, Feb 29, 2016 at 9:50 PM, Michael Welling
><[email protected]>
>> >>wrote:
>> >>> On Fri, Feb 05, 2016 at 03:17:18PM +0200, Daniel Baluta wrote:
>> >>>> The driver has sysfs readings with runtime PM support for power
>saving.
>> >>>> It also offers buffer support that can be used together with IIO
>software
>> >>>> triggers.
>> >>>>
>> >>>
>> >>> Daniel,
>> >>>
>> >>> So I noticed something yesterday while testing new boards.
>> >>> The channels are occassionally swapping when accessing data from
>multiple channels.
>> >>>
>> >>> I wrote a simple bash script to demonstrate.
>> >>
>> >>This happened to me in a previous version of the patch. I remember
>it
>> >>being fixed in the last version (or at least I could not
>reproduce).
>> >>I'll test again tomorrow with your script.
>> >>
>> >
>> >Here is what I believe is happening.
>> >
>> >The request for a conversion on a new channel comes in while the
>> >conversion
>> >for the previous channel is still converting. The driver waits
>> >approximately
>> >one conversion cycle. The previous channel completes within this
>timeframe
>> >and the MUX is changed and the new sample is started. The new sample
>is
>> >still
>> >converting and the driver returns the value from the previous
>conversion.
>> >
>> >For a test I multiplied the conv_time value by 2 in the
>> >ads1015_get_adc_result
>> >function. This allows time for the current sample flush out and
>always
>> >returns
>> >the appropriate channel's value.
>> >
>> >Looking at the buffered mode it appears that only one channel is
>being
>> >accessed
>> >at any time. This being the first one in the active_scan_mask found
>by
>> >find_first_bit. So the MUX would never change is buffered mode as
>far I
>> >can
>> >tell.
>> >
>> >Don't we typically want to read all of enabled channels in buffered
>mode?
>> In some devices that is effectively impossible (fifo's that fill only
>from
>> the currently
>> selected channel). That is what the onehot validation callback is
>about.
>> In this particular case it looks like doing a multichannel read is
>fair bit
>> more time
>> consuming than a single channel read and so will result in a
>considerable
>> reduction in
>> throughput. This is of course why many parts include a simple
>sequencer!
>
>Yes the sampling rate would effectively be divided by the number of
>channels
>enabled.
Worse than that as you have to explicitly change the mux for each channel requiring an i2c write.
>
>>
>> Anyhow, supporting multi channel buffered reads could be done
>(probably) but
>> you
>> would want to fall back to the single channel case as it is now.
>
>Understood.
>
>>
>> Jonathan
>> >>Lucas De Marchi
>> >--
>> >To unsubscribe from this list: send the line "unsubscribe linux-iio"
>in
>> >the body of a message to [email protected]
>> >More majordomo info at http://vger.kernel.org/majordomo-info.html
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.