lkml.org 
[lkml]   [2021]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/8] counter/ti-eqep: add support for unit timer
From
Date
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.

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