Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Jonathan Cameron <jic23@kernel.org>]

On 03/07/16 23:24, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:13, Matt Ranostay wrote:
>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>> 16-bit ADC for chemical sensoring applications.
>>>>
>>>>  * add support for the TI LMP91000 potentiostat
>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>
>>> Probably should have put what I am RFC'ing :).
>>>
>>> * does this belong in a new path drivers/iio/potentiostat ?
>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>> as well to take the only similar driver we already have.
>>
>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>> It's 'interesting'.  You've worked around the whole question of how to handle
>> a mux by putting a push interface equipped client on top of the polled interface
>> of the ADC.  It's an elegant solution that I'd never considered.
>>
>> By the very nature of a mux interface, unless we are piping the mux switching
>> out on the same trigger system as the read back, the actual read out must be
>> polled rather than self clocked. Only the mux knows when it is ready.
>> The triggered version has all sorts of additional complexity even if we had
>> output buffers already to go (such as requiring the output buffering to
>> 'lead' the input buffering to give the mux time to switch.
>>
>> Question to my mind is whether this is a generic and flexible enough approach
>> to use for this sort of device in the future... I think we have two classes
>> of 'analog device' that we need to support:
>>
>> 1) Simple all channels always there devices such as analog accelerometers
>> feeding into an ADC with a sequencer (or a software based sequencer).
>> In that case the data flow is clearly going to go over the buffered interface.
>> The accelerometer driver is just massaging the data for types / scale adjustments.
>> It has no influence on the sampling of the data.
>>
>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>> is driven by the front end, not the ADC.  Games could be played to push the
>> sampling of data over to the ADC, but is it worth doing?
>>
> 
> Probably over-engineering unless we actually find a need to do this in
> the future.
I get the feeling we'll end up with a high performance system needing this
at some point.
> 
>> So if we wanted to do this, the AFE could itself export a trigger that is then
>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>> data interface.  The AFE driver would then have to handle the demux of this
>> data stream into a coherent form to push out in it's own buffer.
>> This approach gains the following:
>>  - quick data transfers, particularly if we are dealing with a multiple output
>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>    The mux driver would then have to recombine these 16 channels before kicking
>>    them out.
> 
> Makes sense but there is a slight issue of the settling time for the
> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
> to take the same time constant.
That's down to the mux driver to handle it.  Only trigger once it's known
to be stable..
> 
>>
>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>> the trigger for the ADC to its own.
> 
> Of course in this case the ADC and LMP91000 device are using both the
> hrtimer trigger, albeit of course you can't do it at the same time. So
> it is polling no matter what.
Fair point.
> 
>>
>> If we want to do this quickish I think that's about the lightest weight option
>> we can do.
>>
>> Now, the question is, what are the disadvantages of going with what you
>> have here for this driver but keeping in mind the above for when it matters?
>>
>> I'm guessing we never need to run this particularly driver very fast...
>> I'm inclined to say yes but would like some other opinions on this one!
>> (hence the expanded Cc list - please do pull in anyone else you think
>> might be interested!)
> 
> Yeah the sample response of the sensor isn't that high speed. Maybe a
> few dozen hertz.
> 
>>
>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>> Should be some defined. That was easy ;)
>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>> err. Odd. Go with signed I think.
>>>
>>>>
>>>> Matt Ranostay (2):
>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>   iio: potentiostat: add LMP91000 support
>>>>
>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>  drivers/iio/Kconfig                                |   1 +
>>>>  drivers/iio/Makefile                               |   1 +
>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>  10 files changed, 622 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>