Re: [PATCH 2/2] hwmon: pwm-fan: use managed timer shutdown

[Guenter Roeck]

On 11/4/26 15:03, Kirtikumar Anandrao Ramchandani wrote:
> Replace the private devres timer cleanup action with
> devm_timer_autoshutdown(). Keep initialization and action registration at
> their existing positions, preserving the cleanup order. The system
> shutdown path continues to call timer_shutdown_sync() directly.
>
> Signed-off-by: Kirtikumar Anandrao Ramchandani <kirtiar15502@xxxxxxxxx>

I fail to see how this would improve the code. This looks like just
a random change to me with no real benefit. It just replaces
devm_add_action_or_reset() with devm_timer_autoshutdown().

It also seems logically wrong: devm functions should be associated with
some initialization or action (such as devm_kzalloc -> kfree), and not
require a separate cleanup. I'd expect to see a devm_timer_setup()
or similar instead, if at all.

Guenter

> ---
>   drivers/hwmon/pwm-fan.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 9cabbe9f8..6b02c2687 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -8,6 +8,7 @@
>    */
>
>   #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/hwmon.h>
>   #include <linux/interrupt.h>
>   #include <linux/module.h>
> @@ -493,13 +494,6 @@ static void pwm_fan_cleanup(void *__ctx)
>   	}
>   }
>
> -static void pwm_fan_timer_cleanup(void *__ctx)
> -{
> -	struct pwm_fan_ctx *ctx = __ctx;
> -
> -	timer_shutdown_sync(&ctx->rpm_timer);
> -}
> -
>   static int pwm_fan_probe(struct platform_device *pdev)
>   {
>   	struct thermal_cooling_device *cdev;
> @@ -650,7 +644,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   	}
>
>   	if (ctx->tach_count > 0) {
> -		ret = devm_add_action_or_reset(dev, pwm_fan_timer_cleanup, ctx);
> +		ret = devm_timer_autoshutdown(dev, &ctx->rpm_timer);
>   		if (ret)
>   			return ret;
>
> @@ -710,7 +704,7 @@ static void pwm_fan_shutdown(struct platform_device *pdev)
>   {
>   	struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
>
> -	pwm_fan_timer_cleanup(ctx);
> +	timer_shutdown_sync(&ctx->rpm_timer);
>   	pwm_fan_cleanup(ctx);
>   }