On Sat, 16 Oct 2021 20:33:38 -0500
David Lechner <david@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
No comments on the interface in here as leaving that for William / later.
A few minor comments on the implementation.
Thanks,
Jonathan
> ---
> drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++-
> include/uapi/linux/counter.h | 2 +
> 2 files changed, 133 insertions(+), 1 deletion(-)
...
> +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);
Hmm. This pattern strikes me as 'too clever' and also likely to trip up static
checkers who will moan about the truncation if they don't understand this trick.
I think I'd prefer you just put the answer in an u64 and then do a simple bounds
check before casting down.
> + if (qutmr != value)
> + return -ERANGE;
> +
> + regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> + return 0;
> +}
> +
...
> 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);
>
> @@ -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");
dev_err_probe() which both removes most of this boilerplate
and stashes the reason for the deferred probe such that it can be checked when
debugging.
> + return PTR_ERR(clk);
> + }
No need to enable the clock?
> +
> + 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.
> + */
> + }
> +
>
> /**
