Subject: Re: [RFC 8/9] iio: buffer: allow for last-second trigger spawning
 from device driver
To: Marc Titinger <mtitinger@baylibre.com>, knaack.h@gmx.de, lars@metafoo.de,
        pmeerw@pmeerw.net
References: <1447857515-23935-1-git-send-email-mtitinger@baylibre.com>
 <1447857515-23935-9-git-send-email-mtitinger@baylibre.com>
Cc: daniel.baluta@intel.com, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org
From: Jonathan Cameron <jic23@kernel.org>
Message-ID: <564CC9B4.7010207@kernel.org>
Date: Wed, 18 Nov 2015 18:55:48 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1447857515-23935-9-git-send-email-mtitinger@baylibre.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5205
Lines: 112

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.

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.

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.

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 <mtitinger@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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/