Date:   Mon, 25 Oct 2021 17:48:01 +0900
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     David Lechner <david@...hnology.com>
Cc:     linux-iio@...r.kernel.org, Robert Nelson <robertcnelson@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for the Unit Timer.
> The Unit Timer is a device-level extension that provides a timer to be
> used for speed calculations. The sysfs interface for the Unit Timer is
> new and will be documented in a later commit. It contains a R/W time
> attribute for the current time, a R/W period attribute for the timeout
> period and a R/W enable attribute to start/stop the timer. It also
> implements a timeout event on the chrdev interface that is triggered
> each time the period timeout is reached.
> 
> Signed-off-by: David Lechner <david@...hnology.com>

I'll comment on the sysfs interface in the respective docs patch. Some
comments regarding this patch below.

> ---
>  drivers/counter/ti-eqep.c    | 132 ++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/counter.h |   2 +
>  2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 9881e5115da6..a4a5a4486329 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/counter.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -131,6 +132,7 @@ enum ti_eqep_count_func {
>  
>  struct ti_eqep_cnt {
>  	struct counter_device counter;
> +	unsigned long sysclkout_rate;
>  	struct regmap *regmap32;
>  	struct regmap *regmap16;
>  };
> @@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
>  		case COUNTER_EVENT_DIRECTION_CHANGE:
>  			qeint |= QEINT_QDC;
>  			break;
> +		case COUNTER_EVENT_TIMEOUT:
> +			qeint |= QEINT_UTO;
> +			break;
>  		}
>  	}
>  
> @@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
>  	case COUNTER_EVENT_OVERFLOW:
>  	case COUNTER_EVENT_UNDERFLOW:
>  	case COUNTER_EVENT_DIRECTION_CHANGE:
> +	case COUNTER_EVENT_TIMEOUT:
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
> +				       u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qutmr;
> +
> +	regmap_read(priv->regmap32, QUTMR, &qutmr);
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
> +					u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qutmr;
> +
> +	/* convert nanoseconds to timer ticks */
> +	qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (qutmr != value)
> +		return -ERANGE;
> +
> +	regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_read(struct counter_device *counter,
> +					  u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 quprd;
> +
> +	regmap_read(priv->regmap32, QUPRD, &quprd);
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> +					   u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 quprd;
> +
> +	/* convert nanoseconds to timer ticks */
> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (quprd != value)
> +		return -ERANGE;
> +
> +	/* protect against infinite unit timeout interrupts */
> +	if (quprd == 0)
> +		return -EINVAL;

I doubt there's any practical reason for a user to set the timer period
to 0, but perhaps we should not try to protect users from themselves
here. It's very obvious and expected that setting the timer period to 0
results in timeouts as quickly as possible, so really the user should be
left to reap the fruits of their decision regardless of how asinine that
decision is.

> +
> +	regmap_write(priv->regmap32, QUPRD, quprd);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_read(struct counter_device *counter,
> +					  u8 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +	*value = !!(qepctl & QEPCTL_UTE);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
> +					   u8 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> +	else
> +		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> +
> +	return 0;
> +}
> +
> +static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
> +				ti_eqep_unit_timer_time_write),
> +	COUNTER_COMP_DEVICE_U64("unit_timer_period",
> +				ti_eqep_unit_timer_period_read,
> +				ti_eqep_unit_timer_period_write),
> +	COUNTER_COMP_DEVICE_BOOL("unit_timer_enable",
> +				 ti_eqep_unit_timer_enable_read,
> +				 ti_eqep_unit_timer_enable_write),
> +};
> +
>  static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  {
>  	struct ti_eqep_cnt *priv = dev_id;
> @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_QDC)
>  		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
>  
> +	if (qflg & QFLG_UTO)
> +		counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
>  
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);

Same comment here as the previous patches about clearing all interrupt
flags.

>  
> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct ti_eqep_cnt *priv;
> +	struct clk *clk;
>  	void __iomem *base;
>  	int err;
>  	int irq;
> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	clk = devm_clk_get(dev, "sysclkout");
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get sysclkout");
> +		return PTR_ERR(clk);
> +	}
> +
> +	priv->sysclkout_rate = clk_get_rate(clk);
> +	if (priv->sysclkout_rate == 0) {
> +		dev_err(dev, "failed to get sysclkout rate");
> +		/* prevent divide by zero */
> +		priv->sysclkout_rate = 1;
> +		/*
> +		 * This error is not expected and the driver is mostly usable
> +		 * without clock rate anyway, so don't exit here.
> +		 */

If the values for these new attributes are expected to be denominated in
nanoseconds then we must guarantee that. You should certainly error out
here if you can't guarantee such.

Alternatively, you could provide an additional set of attributes that
are denominated in units of raw timer ticks rather than nanoseconds.
That way if you can't determine the clock rate you can simply have the
nanosecond-denominated timer attributes return an EOPNOTSUPP error code
or similar while still providing users with the raw timer ticks
attributes.

William Breathitt Gray

> +	}
> +
>  	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> @@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	priv->counter.ops = &ti_eqep_counter_ops;
>  	priv->counter.counts = ti_eqep_counts;
>  	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
> +	priv->counter.ext = ti_eqep_device_ext;
> +	priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext);
>  	priv->counter.signals = ti_eqep_signals;
>  	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
>  	priv->counter.priv = priv;
> @@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * We can end up with an interupt infinite loop (interrupts triggered
> -	 * as soon as they are cleared) if we leave this at the default value
> +	 * as soon as they are cleared) if we leave these at the default value
>  	 * of 0 and events are enabled.
>  	 */
>  	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +	regmap_write(priv->regmap32, QUPRD, UINT_MAX);
>  
>  	err = counter_register(&priv->counter);
>  	if (err < 0) {
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 36dd3b474d09..640d9719b88c 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -63,6 +63,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_INDEX,
>  	/* Direction change detected */
>  	COUNTER_EVENT_DIRECTION_CHANGE,
> +	/* Timer exceeded timeout */
> +	COUNTER_EVENT_TIMEOUT,
>  };
>  
>  /**
> -- 
> 2.25.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists