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

[Adrián Larumbe]

On 21.03.2026 14:16, Biju Das wrote:
> Hi Adrián Larumbe,
>
> Thanks for the feedback.
>
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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.

You're right, it's probably best to leave it the way it is.

Cheers,
Adrian

> Cheers,
> Biju
>
> >
> > Cheers,
> > Biju
> > >
> > > Other than that,
> > >
> > > Reviewed-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> > >
> > > >  	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