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

[Boris Brezillon]

On Sat, 12 Sep 2026 00:28:06 +0100
Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx> wrote:

> Gather all clock enables and disables into a single function to avoid
> repetitions between driver init/fini and device resume/suspend, since
> these clocks are always handled in bulk.
> 
> Also do clk (un)prepares and dis/enables at the same time, since the
> clk_prepare_* family of functions can simply increase the refcnt of
> an already prepared clock.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>

Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

Two nits below.

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 118 +++++++++++++----------------
>  1 file changed, 52 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index b3a53504bd01..2fcbd223204c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -34,10 +34,46 @@ static void panfrost_reset_fini(struct panfrost_device *pfdev)
>  	reset_control_assert(pfdev->rstc);
>  }
>  
> -static int panfrost_clk_init(struct panfrost_device *pfdev)
> +static int panfrost_clk_enable(struct panfrost_device *pfdev, bool enable_ace_clock)

I don't want to have a flag added for each new clock we decide to keep
enabled on RPM suspend, so how about s/enable_ace_clock/on_resume/?
Also, since it's enabling more than one clk, I'd go
s/panfrost_clk_enable/panfrost_clks_enable/.

>  {
>  	int err;
> +
> +	err = clk_prepare_enable(pfdev->clock);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(pfdev->bus_clock);
> +	if (err)
> +		goto disable_clock;
> +
> +	if (enable_ace_clock) {
> +		err = clk_prepare_enable(pfdev->bus_ace_clock);
> +		if (err)
> +			goto disable_bus_clock;
> +	}
> +
> +	return 0;
> +
> +disable_bus_clock:
> +	clk_disable_unprepare(pfdev->bus_clock);
> +disable_clock:
> +	clk_disable_unprepare(pfdev->clock);
> +
> +	return err;
> +}
> +
> +static void panfrost_clk_disable(struct panfrost_device *pfdev, bool disable_ace_clock)

Similarly, I'd go s/disable_ace_clock/on_suspend/ and
s/panfrost_clk_disable/panfrost_clks_disable/.