Re: [PATCH v8 05/16] drm/panfrost: Consolidate device clock management and reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete refactoring of clock management in system PM callbacks, contradicting the commit message
--

commit ade848b25069fa976d1163310dd16ab781b653aa
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Consolidate device clock management and reset

This commit introduces helpers to consolidate clock management and avoid
repetition across different power states. It updates the driver init, fini,
and runtime PM paths to prepare/enable and disable/unprepare clocks together.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index b3a53504bd016..48135e412b6c9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c

[ ... ]

> @@ -506,11 +498,11 @@ static int panfrost_device_resume(struct device *dev)
>  	}
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> -		ret = clk_enable(pfdev->clock);
> +		ret = clk_prepare_enable(pfdev->clock);
>  		if (ret)
>  			goto err_clk;
>  
> -		ret = clk_enable(pfdev->bus_clock);
> +		ret = clk_prepare_enable(pfdev->bus_clock);
>  		if (ret)
>  			goto err_bus_clk;
>  	}

[Severity: Medium]
The commit message mentions gathering all clock enables and disables into a
single function to avoid repetitions between driver init/fini and device
resume/suspend. Does this system resume path intentionally bypass the newly
added panfrost_clk_enable() function?

By calling clk_prepare_enable() directly, it appears to miss the refactoring
goal and omits bus_ace_clock for platforms using GPU_PM_CLK_DIS.

[ ... ]

> @@ -543,8 +535,8 @@ static int panfrost_device_suspend(struct device *dev)
>  		return ret;
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> -		clk_disable(pfdev->bus_clock);
> -		clk_disable(pfdev->clock);
> +		clk_disable_unprepare(pfdev->bus_clock);
> +		clk_disable_unprepare(pfdev->clock);
>  	}
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))

[Severity: Medium]
Similarly, should this section use the new panfrost_clk_disable() helper
instead of open-coding the clk_disable_unprepare() calls?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-claude-fixes-v8-0-c2bcb5e82184@collabora.com?part=5