Re: [PATCH v3 1/5] PM / OPP: Introduce a power estimation helper

[Matthias Kaehlcke]

On Fri, Feb 01, 2019 at 12:09:53PM +0000, Quentin Perret wrote:
> Hi Sudeep,
> 
> On Friday 01 Feb 2019 at 12:04:53 (+0000), Sudeep Holla wrote:
> > On Fri, Feb 01, 2019 at 09:30:57AM +0000, Quentin Perret wrote:
> > > +void dev_pm_opp_of_register_em(struct cpumask *cpus, int nr_opp)
> > > +{
> > > +	struct em_data_callback em_cb = EM_DATA_CB(_get_cpu_power);
> > > +	int ret, cpu = cpumask_first(cpus);
> > > +	struct device *cpu_dev;
> > > +	struct device_node *np;
> > > +	u32 cap;
> > > +
> > > +	cpu_dev = get_cpu_device(cpu);
> > > +	if (!cpu_dev)
> > > +		return;
> > > +
> > > +	np = of_node_get(cpu_dev->of_node);
> > > +	if (!np)
> > > +		return;
> > > +
> > 
> > Does it make sense to add the check for OPP count here. You need not pass
> > that as parameter. Just makes one less thing to check in new drivers adding
> > this support. Thoughts ?
> 
> Yeah Matthias had the same suggestion. I don't mind moving it here TBH.
> It's just that some users already do the opp count before calling this
> function, so I figured I could as well use that data instead of counting
> again.
> 
> But yeah, that's one less thing to worry about on the driver side so
> I'll move the OPP count in there for v4 and we'll see if people ask me
> to move it out to optimize things ;-)

>From an API perspective it would be nice to get rid of the nr_opp
parameter, it seems somewhat arbitrary. Moving dev_pm_opp_get_opp_count()
from the drivers into dev_pm_opp_of_register_em() (instead of calling
it twice) also sounds good in general, as long as the error handling
doesn't become too messy. In the current version
dev_pm_opp_of_register_em() doesn't return a value, with the change it
would have to return one to catch an empty OPP table, and it needs
to be distinguished from other cases where the EM registration fails
but the cpufreq driver is still functional (e.g. no
'dynamic-power-coefficient'). Maybe return -ENOTSUPP in those cases?

Well, let's see how it looks like :)