Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer
From: David Lechner
Date: Wed Oct 27 2021 - 11:29:10 EST
On 10/25/21 3:48 AM, William Breathitt Gray wrote:
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@xxxxxxxxxxxxxx>
I'll comment on the sysfs interface in the respective docs patch. Some
comments regarding this patch below.
...
+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.
Even if the operating system ceases operation because the interrupt
handler keeps running because clearing the interrupt has no effect
in this condition?
...
@@ -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.
I think we should just fail here.