On 21.03.2026 14:16, Biju Das wrote: > 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.
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 <[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
