RE: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Adrián Larumbe,

Thanks for the feedback.

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Biju Das
> Sent: 20 March 2026 21:32
> Subject: RE: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Adrián Larumbe
> > Sent: 20 March 2026 21:20
> > Subject: Re: [PATCH v2 2/4] drm/panfrost: Drop redundant optional
> > clock checks in runtime PM
> >
> > Hi Biju,
> >
> > On 20.03.2026 16:41, Biju wrote:
> > > From: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > The clk_enable() and clk_disable() APIs already handle NULL clock
> > > pointers gracefully — clk_enable() returns 0 and clk_disable()
> > > returns immediately when passed a NULL or optional clock. The
> > > explicit if
> > > (pfdev->bus_clock) guards around these calls in the runtime
> > > suspend/resume paths are therefore unnecessary. Remove them to simplify the code.
> > >
> > > Reviewed-by: Steven Price <steven.price@arm.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v1->v2:
> > >  * Collected tag
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_device.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > index dedc13e56631..01e702a0b2f0 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > @@ -429,11 +429,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
> > >  		if (ret)
> > >  			goto err_clk;
> > >
> > > -		if (pfdev->bus_clock) {
> > > -			ret = clk_enable(pfdev->bus_clock);
> > > -			if (ret)
> > > -				goto err_bus_clk;
> > > -		}
> > > +		ret = clk_enable(pfdev->bus_clock);
> > > +		if (ret)
> > > +			goto err_bus_clk;
> > >  	}
> >
> > It seems clk_prepare_enable() can also deal with NULL clock device
> > pointers gracefully, so maybe you could also do away with pointer checks in panfrost_clk_init?
> 
> This is the only check and no need to print rate for optional clk. That is the reason I have not
> touched this.
> 
> 	if (pfdev->bus_clock) {
> 		rate = clk_get_rate(pfdev->bus_clock);
> 		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
> 
> 		err = clk_prepare_enable(pfdev->bus_clock);
> 		if (err)
> 			goto disable_clock;
> 	}

The above block is good for optional clock.

Otherwise, there will be 2 checks for optional clk.

One here:

if (pfdev->bus_clock) {
	rate = clk_get_rate(pfdev->bus_clock);
	dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
}

and one inside the clk_prepare_enable(): 

err = clk_prepare_enable(pfdev->bus_clock);

Please let me know your thoughts.

Cheers,
Biju

> 
> Cheers,
> Biju
> >
> > Other than that,
> >
> > Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> >
> > >  	panfrost_device_reset(pfdev, true); @@ -464,9 +462,7 @@ static int
> > > panfrost_device_runtime_suspend(struct device *dev)
> > >  	panfrost_gpu_power_off(pfdev);
> > >
> > >  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> > > -		if (pfdev->bus_clock)
> > > -			clk_disable(pfdev->bus_clock);
> > > -
> > > +		clk_disable(pfdev->bus_clock);
> > >  		clk_disable(pfdev->clock);
> > >  		reset_control_assert(pfdev->rstc);
> > >  	}
> > > --
> > > 2.43.0
> >
> >
> > Adrian Larumbe