Instead of having a static global structure for a timer, let's
allocate it dynamically so we can create multiple instances in the
future to support multiple timers. At the same time, add the
rollbacking code in case of error.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/timer-vf-pit.c | 48 +++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/timer-vf-pit.c b/drivers/clocksource/timer-vf-pit.c
index a11e6a63c79f..d408dcddb4e9 100644
--- a/drivers/clocksource/timer-vf-pit.c
+++ b/drivers/clocksource/timer-vf-pit.c
@@ -37,8 +37,6 @@ struct pit_timer {
 	struct clocksource cs;
 };
 
-static struct pit_timer pit_timer;
-
 static void __iomem *clksrc_base;
 
 static inline struct pit_timer *ced_to_pit(struct clock_event_device *ced)
@@ -189,38 +187,66 @@ static int __init pit_clockevent_init(struct pit_timer *pit, void __iomem *base,
 
 static int __init pit_timer_init(struct device_node *np)
 {
+	struct pit_timer *pit;
 	struct clk *pit_clk;
 	void __iomem *timer_base;
 	unsigned long clk_rate;
 	int irq, ret;
 
+	pit = kzalloc(sizeof(*pit), GFP_KERNEL);
+	if (!pit)
+		return -ENOMEM;
+
+	ret = -ENXIO;
 	timer_base = of_iomap(np, 0);
 	if (!timer_base) {
 		pr_err("Failed to iomap\n");
-		return -ENXIO;
+		goto out_kfree;
 	}
 
+	ret = -EINVAL;
 	irq = irq_of_parse_and_map(np, 0);
-	if (irq <= 0)
-		return -EINVAL;
+	if (irq <= 0) {
+		pr_err("Failed to irq_of_parse_and_map\n");
+		goto out_iounmap;
+	}
 
 	pit_clk = of_clk_get(np, 0);
-	if (IS_ERR(pit_clk))
-		return PTR_ERR(pit_clk);
+	if (IS_ERR(pit_clk)) {
+		ret = PTR_ERR(pit_clk);
+		goto out_iounmap;
+	}
 
 	ret = clk_prepare_enable(pit_clk);
 	if (ret)
-		return ret;
+		goto out_clk_put;
 
 	clk_rate = clk_get_rate(pit_clk);
 
 	/* enable the pit module */
 	writel(~PITMCR_MDIS, timer_base + PITMCR);
 
-	ret = pit_clocksource_init(&pit_timer, timer_base, clk_rate);
+	ret = pit_clocksource_init(pit, timer_base, clk_rate);
 	if (ret)
-		return ret;
+		goto out_disable_unprepare;
+
+	ret = pit_clockevent_init(pit, timer_base, clk_rate, irq, 0);
+	if (ret)
+		goto out_pit_clocksource_unregister;
+
+	return 0;
 
-	return pit_clockevent_init(&pit_timer, timer_base, clk_rate, irq, 0);
+out_pit_clocksource_unregister:
+	clocksource_unregister(&pit->cs);
+out_disable_unprepare:
+	clk_disable_unprepare(pit_clk);
+out_clk_put:
+	clk_put(pit_clk);
+out_iounmap:
+	iounmap(timer_base);
+out_kfree:
+	kfree(pit);
+
+	return ret;
 }
 TIMER_OF_DECLARE(vf610, "fsl,vf610-pit", pit_timer_init);
-- 
2.43.0

On 7/30/2025 11:27 AM, Daniel Lezcano wrote:

[...]

>  static int __init pit_timer_init(struct device_node *np)
>  {
> +	struct pit_timer *pit;
>  	struct clk *pit_clk;
>  	void __iomem *timer_base;
>  	unsigned long clk_rate;
>  	int irq, ret;
>  
> +	pit = kzalloc(sizeof(*pit), GFP_KERNEL);
> +	if (!pit)
> +		return -ENOMEM;
> +
> +	ret = -ENXIO;
>  	timer_base = of_iomap(np, 0);
>  	if (!timer_base) {
>  		pr_err("Failed to iomap\n");
> -		return -ENXIO;
> +		goto out_kfree;
>  	}
>  
> +	ret = -EINVAL;
>  	irq = irq_of_parse_and_map(np, 0);
> -	if (irq <= 0)
> -		return -EINVAL;
> +	if (irq <= 0) {
> +		pr_err("Failed to irq_of_parse_and_map\n");
> +		goto out_iounmap;
> +	}
>  
>  	pit_clk = of_clk_get(np, 0);
> -	if (IS_ERR(pit_clk))
> -		return PTR_ERR(pit_clk);
> +	if (IS_ERR(pit_clk)) {
> +		ret = PTR_ERR(pit_clk);
> +		goto out_iounmap;

This does not revert the use of irq_of_parse_and_map. In my opinion, it
should be explicitly reverted as part of the cleanup phase by calling
irq_of_parse_and_map.

> +	}
>  
>  	ret = clk_prepare_enable(pit_clk);
>  	if (ret)
> -		return ret;
> +		goto out_clk_put;
>  
>  	clk_rate = clk_get_rate(pit_clk);
>  
>  	/* enable the pit module */
>  	writel(~PITMCR_MDIS, timer_base + PITMCR);
>  
> -	ret = pit_clocksource_init(&pit_timer, timer_base, clk_rate);
> +	ret = pit_clocksource_init(pit, timer_base, clk_rate);
>  	if (ret)
> -		return ret;
> +		goto out_disable_unprepare;
> +
> +	ret = pit_clockevent_init(pit, timer_base, clk_rate, irq, 0);
> +	if (ret)
> +		goto out_pit_clocksource_unregister;
> +
> +	return 0;
>  
> -	return pit_clockevent_init(&pit_timer, timer_base, clk_rate, irq, 0);
> +out_pit_clocksource_unregister:
> +	clocksource_unregister(&pit->cs);
> +out_disable_unprepare:
> +	clk_disable_unprepare(pit_clk);
> +out_clk_put:
> +	clk_put(pit_clk);
> +out_iounmap:
> +	iounmap(timer_base);
> +out_kfree:
> +	kfree(pit);
> +
> +	return ret;
>  }
>  TIMER_OF_DECLARE(vf610, "fsl,vf610-pit", pit_timer_init);

-- 
Regards,
Ghennadi

On 01/08/2025 09:33, Ghennadi Procopciuc wrote:
> On 7/30/2025 11:27 AM, Daniel Lezcano wrote:
> 
> [...]
> 

[ ... ]

>> +	ret = -EINVAL;
>>   	irq = irq_of_parse_and_map(np, 0);
>> -	if (irq <= 0)
>> -		return -EINVAL;
>> +	if (irq <= 0) {
>> +		pr_err("Failed to irq_of_parse_and_map\n");
>> +		goto out_iounmap;
>> +	}
>>   
>>   	pit_clk = of_clk_get(np, 0);
>> -	if (IS_ERR(pit_clk))
>> -		return PTR_ERR(pit_clk);
>> +	if (IS_ERR(pit_clk)) {
>> +		ret = PTR_ERR(pit_clk);
>> +		goto out_iounmap;
> 
> This does not revert the use of irq_of_parse_and_map. In my opinion, it
> should be explicitly reverted as part of the cleanup phase by calling
> irq_of_parse_and_map.

AFAICT, it does not make sense to undo irq_of_parse_and_map() and there 
is no revert function for that.

However, calling free_irq as commented in patch 20/20 makes sense.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

On 8/4/2025 12:12 PM, Daniel Lezcano wrote:
> On 01/08/2025 09:33, Ghennadi Procopciuc wrote:
>> On 7/30/2025 11:27 AM, Daniel Lezcano wrote:
>>
>> [...]
>>
> 
> [ ... ]
> 
>>> +    ret = -EINVAL;
>>>       irq = irq_of_parse_and_map(np, 0);
>>> -    if (irq <= 0)
>>> -        return -EINVAL;
>>> +    if (irq <= 0) {
>>> +        pr_err("Failed to irq_of_parse_and_map\n");
>>> +        goto out_iounmap;
>>> +    }
>>>         pit_clk = of_clk_get(np, 0);
>>> -    if (IS_ERR(pit_clk))
>>> -        return PTR_ERR(pit_clk);
>>> +    if (IS_ERR(pit_clk)) {
>>> +        ret = PTR_ERR(pit_clk);
>>> +        goto out_iounmap;
>>
>> This does not revert the use of irq_of_parse_and_map. In my opinion, it
>> should be explicitly reverted as part of the cleanup phase by calling
>> irq_of_parse_and_map.
> 
> AFAICT, it does not make sense to undo irq_of_parse_and_map() and there
> is no revert function for that.
> 
> However, calling free_irq as commented in patch 20/20 makes sense.
> 
> 

I noticed that irq_of_parse_and_map internally calls both
of_irq_parse_one and irq_create_of_mapping. While the former does not
have a corresponding revert function, the latter can be reversed using
irq_dispose_mapping.

Examples:
    drivers/char/ipmi/ipmi_powernv.c
    drivers/clocksource/timer-microchip-pit64b.c
    drivers/crypto/amcc/crypto4xx_core.c
    drivers/memory/fsl_ifc.c

-- 
Regards,
Ghennadi