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

["Adrián Larumbe" <adrian.larumbe@collabora.com>]

On 14.09.2026 10:45, Boris Brezillon wrote:
> On Sat, 12 Sep 2026 00:28:06 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> 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@collabora.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> 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/.

I agree with you. I was thinking if new clocks also had different needs in the future,
maybe we could pass an array of option clocks but at this point it's pure make-believe.

> >  {
> >  	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/.

Noted.

Adrian Larumbe