Re: [PATCH v3 1/2] counter: microchip-tcb-capture: Add IRQ handling

[<Dharma.B@microchip.com>]

Hi Bence,

On 05/02/25 4:19 pm, Bence Csókás wrote:
> Add interrupt servicing to allow userspace to wait for the following events:
> * Change-of-state caused by external trigger
> * Capture of timer value into RA/RB
> * Compare to RC register
> * Overflow
> 
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> ---
> 
> Notes:
>      New in v2
>      Changes in v3:
>      * Add IRQs for Capture events (from next patch)
>      * Add IRQ for RC Compare
>      * Add events as bullet points to commit msg
> 
>   drivers/counter/microchip-tcb-capture.c | 67 +++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index 2f096a5b973d..fef4bb69b486 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -6,10 +6,12 @@
>    */
>   #include <linux/clk.h>
>   #include <linux/counter.h>
> +#include <linux/interrupt.h>
>   #include <linux/mfd/syscon.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of.h>
> +#include <linux/of_irq.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <soc/at91/atmel_tcb.h>
> @@ -18,6 +20,9 @@
>   				 ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \
>   				 ATMEL_TC_LDBSTOP)
>   
> +#define ATMEL_TC_DEF_IRQS	(ATMEL_TC_ETRGS | ATMEL_TC_COVFS | \
> +				 ATMEL_TC_LDRAS | ATMEL_TC_LDRBS | ATMEL_TC_CPCS)
> +
>   #define ATMEL_TC_QDEN			BIT(8)
>   #define ATMEL_TC_POSEN			BIT(9)
>   
> @@ -27,6 +32,7 @@ struct mchp_tc_data {
>   	int qdec_mode;
>   	int num_channels;
>   	int channel[2];
> +	int irq;
>   };
>   
>   static const enum counter_function mchp_tc_count_functions[] = {
> @@ -294,6 +300,60 @@ static const struct of_device_id atmel_tc_of_match[] = {
>   	{ /* sentinel */ }
>   };
>   
> +static irqreturn_t mchp_tc_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *const counter = dev_id;
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	u32 sr, mask;
> +
> +	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
> +	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], IMR), &mask);
> +
> +	sr &= mask;
> +	if (!(sr & ATMEL_TC_ALL_IRQ))
> +		return IRQ_NONE;
> +
> +	if (sr & ATMEL_TC_ETRGS)
> +		counter_push_event(counter, COUNTER_EVENT_CHANGE_OF_STATE, 0);
> +	if (sr & ATMEL_TC_LDRAS)
> +		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 0);
> +	if (sr & ATMEL_TC_LDRBS)
> +		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 1);
> +	if (sr & ATMEL_TC_CPCS)
> +		counter_push_event(counter, COUNTER_EVENT_THRESHOLD, 2);
> +	if (sr & ATMEL_TC_COVFS)
> +		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);

can we have macros for the channel (3rd argument) for better clarity?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void mchp_tc_irq_remove(void *ptr)
> +{
> +	struct mchp_tc_data *priv = ptr;

> +
> +	regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IDR), ATMEL_TC_DEF_IRQS);
> +}
> +
> +static int mchp_tc_irq_enable(struct counter_device *const counter)

Can we have it as mchp_tc_irq_init ?

> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	int ret = devm_request_irq(counter->parent, priv->irq, mchp_tc_isr, 0,
> +				   dev_name(counter->parent), counter);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IER), ATMEL_TC_DEF_IRQS);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(counter->parent, mchp_tc_irq_remove, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   static void mchp_tc_clk_remove(void *ptr)
>   {
>   	clk_disable_unprepare((struct clk *)ptr);
> @@ -378,6 +438,13 @@ static int mchp_tc_probe(struct platform_device *pdev)
>   	counter->num_signals = ARRAY_SIZE(mchp_tc_count_signals);
>   	counter->signals = mchp_tc_count_signals;
>   
> +	priv->irq = of_irq_get(np->parent, 0);
> +	if (priv->irq > 0) {
> +		ret = mchp_tc_irq_enable(counter);

missing error handling in irq retrieval (check for -EPROBE_DEFER).

> +		if (ret < 0)
> +			return dev_err_probe(&pdev->dev, ret, "Failed to set up IRQ");
> +	}
> +
>   	ret = devm_counter_add(&pdev->dev, counter);
>   	if (ret < 0)
>   		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");


-- 
With Best Regards,
Dharma B.