Re: [PATCH v8 4/5] drm/panthor: Use existing OPP table if present

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Monday, 20 October 2025 10:35:04 Central European Summer Time Karunika Choo wrote:
> On 17/10/2025 16:31, Nicolas Frattaroli wrote:
> > On SoCs where the GPU's power-domain is in charge of setting performance
> > levels, the OPP table of the GPU node will have already been populated
> > during said power-domain's attach_dev operation.
> > 
> > To avoid initialising an OPP table twice, only set the OPP regulator and
> > the OPPs from DT if there's no OPP table present.
> > 
> > Reviewed-by: Chia-I Wu <olvaffe@gmail.com>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_devfreq.c | 32 ++++++++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> > index a6dca599f0a5..ec63e27f4883 100644
> > --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> > @@ -141,6 +141,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >  	struct thermal_cooling_device *cooling;
> >  	struct device *dev = ptdev->base.dev;
> >  	struct panthor_devfreq *pdevfreq;
> > +	struct opp_table *table;
> >  	struct dev_pm_opp *opp;
> >  	unsigned long cur_freq;
> >  	unsigned long freq = ULONG_MAX;
> > @@ -152,17 +153,30 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >  
> >  	ptdev->devfreq = pdevfreq;
> >  
> > -	ret = devm_pm_opp_set_regulators(dev, reg_names);
> > -	if (ret && ret != -ENODEV) {
> > -		if (ret != -EPROBE_DEFER)
> > -			DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
> > -		return ret;
> > +	/*
> > +	 * The power domain associated with the GPU may have already added an
> > +	 * OPP table, complete with OPPs, as part of the platform bus
> > +	 * initialization. If this is the case, the power domain is in charge of
> > +	 * also controlling the performance, with a set_performance callback.
> > +	 * Only add a new OPP table from DT if there isn't such a table present
> > +	 * already.
> > +	 */
> > +	table = dev_pm_opp_get_opp_table(dev);
> > +	if (IS_ERR_OR_NULL(table)) {
> > +		ret = devm_pm_opp_set_regulators(dev, reg_names);
> > +		if (ret && ret != -ENODEV) {
> 
> Is there a reason to not fail on -ENODEV? I would assume it is a valid 
> failure path. 

Hi,

the -ENODEV logic wasn't added by me, it was added in
Commit: a8cb5ca53690 ("drm/panthor: skip regulator setup if no such prop")

with the justification

  The regulator is optional, skip the setup instead of returning an
  error if it is not present

I will not be changing anything about this logic in this patch set,
as it is not in scope for MT8196 enablement, since MT8196 does not
use this code path at all.

Kind regards,
Nicolas Frattaroli

> 
> Kind regards,
> Karunika Choo
> 
> > +			if (ret != -EPROBE_DEFER)
> > +				DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = devm_pm_opp_of_add_table(dev);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		dev_pm_opp_put_opp_table(table);
> >  	}
> >  
> > -	ret = devm_pm_opp_of_add_table(dev);
> > -	if (ret)
> > -		return ret;
> > -
> >  	spin_lock_init(&pdevfreq->lock);
> >  
> >  	panthor_devfreq_reset(pdevfreq);
> > 
> 
>