Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 17 Oct 2021 12:20:43 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     David Lechner <david@lechnology.com>
Cc:     linux-iio@vger.kernel.org,
        William Breathitt Gray <vilhelm.gray@gmail.com>,
        Robert Nelson <robertcnelson@gmail.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer
Message-ID: <20211017122043.731858d3@jic23-huawei>
In-Reply-To: <20211017013343.3385923-4-david@lechnology.com>
References: <20211017013343.3385923-1-david@lechnology.com>
        <20211017013343.3385923-4-david@lechnology.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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