On 19/11/15 09:15, Marc Titinger wrote:
> On 18/11/2015 19:55, Jonathan Cameron wrote:
>> On 18/11/15 14:38, Marc Titinger wrote:
>>> The hrtimer sw-trigger allow for polling mode on devices w/o hard irq
>>> trigger source, but setting the frequency from userland for both the
>>> hrtimer trigger device and the adc is error prone.
>>>
>>> Make adc drivers able to setup the sw-trigger at the last second when the
>>> buffer is enabled, and the sampling frequency is known.
>> Hi Marc,
>>
>> This statement rang alarm bells. We are trying to cross synchronize a timer
>> on the processor with that on the device. Doing this is ALWAYS going to end
>> up dropping or duplicating samples depending on whether we happen to run
>> faster
>> or slower than the sample rate.
>>
> Yup, I had a hunch this was an issue, I understand this is not something you
> guys want to generally support in IIO since it should allow for reliable
> signal processing!
>
>> Now I've done a spot of datasheet diving to see if there is a status
>> register or
>> other indication that we are looking at new data (if there isn't one, then
>> any
>> attempt to get a stream of data off the device is going to give us a mess
>> unfortunately)
>>
>> Anyhow, on the INA219 we have a CNVR bit in the bus voltage register which
>> will do as it
>> it's semantics are described in the datasheet and it's basically a 'you
>> haven't read
>> me before bit'
>>
>> The INA226 seems to have a CVRF bit (nothing like consistency in naming ;)
>> that indicates
>> the 'dataready / alert pin' was set high to indicate new data - so you may
>> need to enable
>> the interrupt pin even if you don't have it wired to anything useful.
>>
>> Anyhow, so we have discussed how to handle this in the past (though right
>> now I can't
>> remember the device in question so will be fun to find). The case it came
>> up for was
>> a board where the interrupt pin on a device wasn't wired to anything (or
>> perhaps a stripped
>> down package in which the sensor didn't have a pin for it - I can't recall).
>>
>> First thing to note is that you 'don't' have to use a trigger to drive a
>> buffer.
>> This makes our life rather easier! Here we don't have a timing signal that
>> is terribly
>> useful for any other sensors to use so a trigger won't be much real use.
>>
>> Once we have dropped that requirement you do end up with something close to
>> the
>> strategy you adopted in the first patch with a small twist.
>>
>> You need to check those 'dataready' register bits to decide whether to push
>> anything
>> at all to the buffer.
>
> Ok yes, I can check that dataready bit, and only push new values with
> a usable timestamp.
Hmm. Timestamping accuracy is going to be terrible whatever you do, but I
guess if that is well documented and you need it in your application we'll
need to go with it as whatever is possible.
> So I shall go back to my polling thread version
> with that addition. I'm just concerned that It is one more register
> to read while part of this work was to increase the bandwidth of our
> apllication.
Give it a try and see if you can get away with it.. In some parts cases you
don't need to even read an extra register and if you pick a sensible poll
frequency
might get data you want say 3 out of 4 times.
> Our hwmon sigrok layer would reissue the last value
> anyhow if the hardware is not ready for a new sample, so a bit of
> clock jitter was ok for my purpose.
> Alternatively, I could check if the cnvr signal can be configured as
> an alarm and routed to a gpio irq, and then use a conventional
> trigger.
That would be preferable, though I'm not sure all the parts
actually have a hardware irq line for it so you may need the register
read value as well.
>
>>
>> So basically you need your thread to always query significantly faster than
>> the sampling
>> rate and to push data directly into the buffer iff the device indicates it
>> is new data.
>> (not the significantly is to ensure that the you get one clean full set of
>> readings within the sample period - I'm not sure the docs were all that
>> specific on what happens if you hit
>> the sampling point in your read - maybe I missed it!)
>>
>> The hrtimer trigger etc make a lot of sense for sample of demand devices, but
>> they will result in inconsistent data if used to sample a self clocking
>> device like
>> this one.
>>
>> I recall we discussed once how to do this generically and concluded that it
>> really
>> isn't easy to do so as it involves polling a local register on a given
>> device.
>>
>> Sorry I didn't pick up on this earlier.
>
> No worries, I'm happy to experiment and gain understanding of the features of
> IIO, I'm sorry it cost you guys review time though, _and_ it is still getting
> onto something useful :)
>
> Marc.
>
>
>>
>> Jonathan
>>
>>>
>>> enable_trigger is called from verify_update, before the classical setup_ops
>>> are called in buffers_enable. This gives a chance to complete the setup of
>>> indio_dev->trig.
>>>
>>> Signed-off-by: Marc Titinger <mtitin...@baylibre.com>
>>> ---
>>> drivers/iio/industrialio-buffer.c | 5 +++++
>>> include/linux/iio/iio.h | 3 +++
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c
>>> b/drivers/iio/industrialio-buffer.c
>>> index d7e908a..ba7abd4 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -647,6 +647,11 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>>> if (insert_buffer)
>>> modes &= insert_buffer->access->modes;
>>>
>>> + if (indio_dev->setup_ops &&
>>> + indio_dev->setup_ops->enable_trigger &&
>>> + (indio_dev->setup_ops->enable_trigger(indio_dev) < 0))
>>> + return -ENXIO;
>>> +
>>> /* Definitely possible for devices to support both of these. */
>>> if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>>> config->mode = INDIO_BUFFER_TRIGGERED;
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 7bb7f67..8f82113 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -419,6 +419,8 @@ struct iio_info {
>>>
>>> /**
>>> * struct iio_buffer_setup_ops - buffer setup related callbacks
>>> + * @enable_trigger: [DRIVER] function to call if a trigger is instancied
>>> + * upon enabling the buffer (sw triggers)
>>> * @preenable: [DRIVER] function to run prior to marking buffer
>>> enabled
>>> * @postenable: [DRIVER] function to run after marking buffer
>>> enabled
>>> * @predisable: [DRIVER] function to run prior to marking buffer
>>> @@ -428,6 +430,7 @@ struct iio_info {
>>> * scan mask is valid for the device.
>>> */
>>> struct iio_buffer_setup_ops {
>>> + int (*enable_trigger)(struct iio_dev *);
>>> int (*preenable)(struct iio_dev *);
>>> int (*postenable)(struct iio_dev *);
>>> int (*predisable)(struct iio_dev *);
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
