lkml.org 
[lkml]   [2021]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/8] counter/ti-eqep: add support for unit timer
On Sat, 16 Oct 2021 20:33:38 -0500
David Lechner <david@lechnology.com> 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@lechnology.com>

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.
> + */
> + }
> +
>
> /**

\
 
 \ /
  Last update: 2021-10-17 13:17    [W:0.316 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site