Hi Adrián Larumbe, Thanks for the feedback.
> -----Original Message----- > From: dri-devel <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > > > 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 <[email protected]> > > > Signed-off-by: Biju Das <[email protected]> > > > --- > > > 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 <[email protected]> > > > > > 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
