Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[David Lechner <david@lechnology.com>]

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.