Hi Adrian, > -----Original Message----- > From: Adrián Larumbe <[email protected]> > Sent: 24 March 2026 17:11 > Subject: Re: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock > checks in runtime PM > > 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.
If you are happy with the series, can you please merge it? Cheers, Biju
