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

["Adrián Larumbe" <adrian.larumbe@collabora.com>]

On 26.03.2026 08:55, Biju Das wrote:
> Hi Adrian,
>
> > -----Original Message-----
> > From: Adrián Larumbe <adrian.larumbe@collabora.com>
> > 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 <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.
> >
> > 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?

Will do, I'll add my Rb's and let you know when it's merged.

> Cheers,
> Biju

Adrian Larumbe