Re: [PATCH 8/8] iio/counter/ftm-quaddec: add handling of under/overflow of the counter.

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 18 Feb 2019 15:03:21 +0100
Patrick Havelange <patrick.havelange@essensium.com> wrote:

> This is implemented by polling the counter value. A new parameter
> "poll-interval" can be set in the device tree, or can be changed
> at runtime. The reason for the polling is to avoid interrupts flooding.
> If the quadrature input is going up and down around the overflow value
> (or around 0), the interrupt will be triggering all the time. Thus,
> polling is an easy way to handle overflow in a consistent way.
> Polling can still be disabled by setting poll-interval to 0.
> 
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Comments inline.

Jonathan

> ---
>  drivers/iio/counter/ftm-quaddec.c | 199 +++++++++++++++++++++++++++++-
>  1 file changed, 193 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
> index ca7e55a9ab3f..3a0395c3ef33 100644
> --- a/drivers/iio/counter/ftm-quaddec.c
> +++ b/drivers/iio/counter/ftm-quaddec.c
> @@ -25,11 +25,33 @@
>  
>  struct ftm_quaddec {
>  	struct platform_device *pdev;
> +	struct delayed_work delayedcounterwork;
>  	void __iomem *ftm_base;
>  	bool big_endian;
> +
> +	/* Offset added to the counter to adjust for overflows of the
> +	 * 16 bit HW counter. Only the 16 MSB are set.
Comment syntax.
> +	 */
> +	uint32_t counteroffset;
> +
> +	/* Store the counter on each read, this is used to detect
> +	 * if the counter readout if we over or underflow
> +	 */
> +	uint8_t lastregion;
> +
> +	/* Poll-interval, in ms before delayed work must poll counter */
> +	uint16_t poll_interval;
> +
>  	struct mutex ftm_quaddec_mutex;
>  };
>  
> +struct counter_result {
> +	/* 16 MSB are from the counteroffset
> +	 * 16 LSB are from the hardware counter
> +	 */
> +	uint32_t value;
Why the structure?

> +};
> +
>  #define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
>  
>  #define DEFAULT_POLL_INTERVAL    100 /* in msec */
> @@ -74,8 +96,75 @@ static void ftm_set_write_protection(struct ftm_quaddec *ftm)
>  	ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
>  }
>  
> +/* must be called with mutex locked */
> +static void ftm_work_reschedule(struct ftm_quaddec *ftm)
> +{
> +	cancel_delayed_work(&ftm->delayedcounterwork);
> +	if (ftm->poll_interval > 0)
> +		schedule_delayed_work(&ftm->delayedcounterwork,
> +				   msecs_to_jiffies(ftm->poll_interval));
> +}
> +
> +/* Reports the hardware counter added the offset counter.
> + *
> + * The quadrature decodes does not use interrupts, because it cannot be
> + * guaranteed that the counter won't flip between 0xFFFF and 0x0000 at a high
> + * rate, causing Real Time performance degration. Instead the counter must be
> + * read frequently enough - the assumption is 150 KHz input can be handled with
> + * 100 ms read cycles.
> + */
> +static void ftm_work_counter(struct ftm_quaddec *ftm,
> +			     struct counter_result *returndata)
> +{
> +	/* only 16bits filled in*/
> +	uint32_t hwcounter;
> +	uint8_t currentregion;
> +
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> +	ftm_read(ftm, FTM_CNT, &hwcounter);
> +
> +	/* Divide the counter in four regions:
> +	 *   0x0000-0x4000-0x8000-0xC000-0xFFFF
> +	 * When the hwcounter changes between region 0 and 3 there is an
> +	 * over/underflow
> +	 */
> +	currentregion = hwcounter / 0x4000;
> +
> +	if (ftm->lastregion == 3 && currentregion == 0)
> +		ftm->counteroffset += 0x10000;
> +
> +	if (ftm->lastregion == 0 && currentregion == 3)
> +		ftm->counteroffset -= 0x10000;
> +
> +	ftm->lastregion = currentregion;
> +
> +	if (returndata)
> +		returndata->value = ftm->counteroffset + hwcounter;
> +
> +	ftm_work_reschedule(ftm);
> +
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
> +}
> +
> +/* wrapper around the real function */
> +static void ftm_work_counter_delay(struct work_struct *workptr)
> +{
> +	struct delayed_work *work;
> +	struct ftm_quaddec *ftm;
> +
> +	work = container_of(workptr, struct delayed_work, work);
> +	ftm = container_of(work, struct ftm_quaddec, delayedcounterwork);
> +
> +	ftm_work_counter(ftm, NULL);
> +}
> +
> +/* must be called with mutex locked */
>  static void ftm_reset_counter(struct ftm_quaddec *ftm)
>  {
> +	ftm->counteroffset = 0;
> +	ftm->lastregion = 0;
> +
>  	/* Reset hardware counter to CNTIN */
>  	ftm_write(ftm, FTM_CNT, 0x0);
>  }
> @@ -110,18 +199,91 @@ static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
>  				int *val, int *val2, long mask)
>  {
>  	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> -	uint32_t counter;
> +	struct counter_result counter;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ftm_read(ftm, FTM_CNT, &counter);
> -		*val = counter;
> +	case IIO_CHAN_INFO_PROCESSED:

> +		ftm_work_counter(ftm, &counter);
> +		if (mask == IIO_CHAN_INFO_RAW)
> +			counter.value &= 0xffff;
> +
> +		*val = counter.value;
> +
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static uint32_t ftm_get_default_poll_interval(const struct ftm_quaddec *ftm)
> +{
> +	/* Read values from device tree */
> +	uint32_t val;
> +	const struct device_node *node = ftm->pdev->dev.of_node;
> +
> +	if (of_property_read_u32(node, "poll-interval", &val))
> +		val = DEFAULT_POLL_INTERVAL;
> +
> +	return val;
> +}
> +
> +static ssize_t ftm_read_default_poll_interval(struct iio_dev *indio_dev,
> +					uintptr_t private,
> +					struct iio_chan_spec const *chan,
> +					char *buf)
> +{
> +	const struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t val = ftm_get_default_poll_interval(ftm);
> +
> +	return snprintf(buf, PAGE_SIZE, "%u\n", val);
> +}
> +
> +static ssize_t ftm_read_poll_interval(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				struct iio_chan_spec const *chan,
> +				char *buf)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> +	uint32_t poll_interval = READ_ONCE(ftm->poll_interval);
Why bother with the local variable? I'm not awake enough to see
why the READ_ONCE is necessary here.
If worried about it, just take the lock, we are far from high
performance in this path.

> +
> +	return snprintf(buf, PAGE_SIZE, "%u\n", poll_interval);
> +}
> +
> +static ssize_t ftm_write_poll_interval(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				struct iio_chan_spec const *chan,
> +				const char *buf, size_t len)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t newpoll_interval;
> +	uint32_t default_interval;
> +
> +	if (kstrtouint(buf, 10, &newpoll_interval) != 0) {
> +		dev_err(&ftm->pdev->dev, "poll_interval not a number: '%s'\n",
> +			buf);
> +		return -EINVAL;
> +	}
> +
> +	/* Don't accept polling times below the default value to protect the
> +	 * system.
> +	 */
> +	default_interval = ftm_get_default_poll_interval(ftm);
> +
> +	if (newpoll_interval < default_interval && newpoll_interval != 0)
> +		newpoll_interval = default_interval;
> +
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> +	WRITE_ONCE(ftm->poll_interval, newpoll_interval);
> +	ftm_work_reschedule(ftm);
> +
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
> +
> +	return len;
> +}
> +
>  static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
>  				uintptr_t private,
>  				struct iio_chan_spec const *chan,
> @@ -135,8 +297,11 @@ static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
>  	ftm_reset_counter(ftm);
>  
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
>  	return len;
>  }
>  
> @@ -192,6 +357,17 @@ static const struct iio_enum ftm_quaddec_prescaler_en = {
>  };
>  
>  static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> +	{
> +		.name = "default_poll_interval",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = ftm_read_default_poll_interval,
Why is this relevant if the value is set to something else?
> +	},
> +	{
> +		.name = "poll_interval",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = ftm_read_poll_interval,
> +		.write = ftm_write_poll_interval,
Hmm.  New ABI that needs documenting. I'm not sure how we should
handle this.  Perhaps William has a good suggestion for how to do it.
> +	},
>  	{
>  		.name = "reset",
>  		.shared = IIO_SEPARATE,
> @@ -205,7 +381,8 @@ static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
>  static const struct iio_chan_spec ftm_quaddec_channels = {
>  	.type = IIO_COUNT,
>  	.channel = 0,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			      BIT(IIO_CHAN_INFO_PROCESSED),
Why?  As a general rule, we don't allow bother RAW and processed for
a particular channel.  Note the raw value doesn't actually 'have'
to be the value off a sensor - it is just expected to not have
linear scaling and offset applied (which are encode dependent
here so can't be applied).  So just use raw for your non overflowing
version.

>  	.ext_info = ftm_quaddec_ext_info,
>  	.indexed = 1,
>  };
> @@ -232,10 +409,14 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  
>  	ftm->pdev = pdev;
>  	ftm->big_endian = of_property_read_bool(node, "big-endian");
> +	ftm->counteroffset = 0;
> +	ftm->lastregion = 0;
>  	ftm->ftm_base = of_iomap(node, 0);
>  	if (!ftm->ftm_base)
>  		return -EINVAL;
>  
> +	ftm->poll_interval = ftm_get_default_poll_interval(ftm);
> +
>  	indio_dev->name = dev_name(&pdev->dev);
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->info = &ftm_quaddec_iio_info;
> @@ -245,9 +426,13 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  	ftm_quaddec_init(ftm);
>  
>  	mutex_init(&ftm->ftm_quaddec_mutex);
> +	INIT_DELAYED_WORK(&ftm->delayedcounterwork, ftm_work_counter_delay);
> +
> +	ftm_work_reschedule(ftm);
>  
>  	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret) {
> +		cancel_delayed_work_sync(&ftm->delayedcounterwork);
>  		mutex_destroy(&ftm->ftm_quaddec_mutex);
>  		iounmap(ftm->ftm_base);
>  	}
> @@ -261,13 +446,15 @@ static int ftm_quaddec_remove(struct platform_device *pdev)
>  
>  	ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
>  	indio_dev = iio_priv_to_dev(ftm);
> -	/* This is needed to remove sysfs entries */
> +	/* Make sure no concurrent attribute reads happen*/
>  	devm_iio_device_unregister(&pdev->dev, indio_dev);
>  
> +	cancel_delayed_work_sync(&ftm->delayedcounterwork);
> +
>  	ftm_write(ftm, FTM_MODE, 0);
>  
> -	iounmap(ftm->ftm_base);
>  	mutex_destroy(&ftm->ftm_quaddec_mutex);
> +	iounmap(ftm->ftm_base);
Why the reorder?  If it was wrong in the first place, fix the
earlier patch not this one.
>  
>  	return 0;
>  }