Messages in this thread | | | Subject | Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer | From | David Lechner <> | Date | Wed, 27 Oct 2021 10:28:59 -0500 |
| |
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@lechnology.com> > > 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.
| |