[PATCH v4 8/8] arm,arm64,drivers: add a prefix to drivers arch_topology interfaces

On 29/05/17 12:46, Dietmar Eggemann wrote:

On 05/29/2017 11:58 AM, Greg KH wrote:

quoted

On Mon, May 29, 2017 at 11:20:24AM +0200, Dietmar Eggemann wrote:

quoted

Hi Greg,

On 05/26/2017 08:36 PM, Greg KH wrote:

quoted

On Fri, May 26, 2017 at 11:10:32AM +0100, Juri Lelli wrote:

quoted

Hi,

On 25/05/17 15:18, Greg KH wrote:

quoted

On Thu, Apr 20, 2017 at 03:43:16PM +0100, Juri Lelli wrote:

[...]

quoted

quoted

quoted

But this is all really topology stuff, right?  Why use "capacity" at
all:
	topology_normalize_cpu()
	topology_parse_cpu()
	topology_scale_cpu()
	topology_set_scale()
?

It's always best to put the "subsystem" name first, we have a bad
history of getting this wrong in the past by putting the verb first, not
the noun.

[...]

quoted

quoted

quoted

Oh, and drop "capacity" please :)

[...]

quoted

I think that if you are creating an api that the scheduler will use, you
need to ask the scheduler maintainers/developers what they want to see
here, as that would be up to them, not me...

The scheduler API exists already. It is arch_scale_cpu_capacity() and
arch_scale_freq_capacity() in kernel/sched/sched.h. An arch is able to
overwrite these two functions by defining them (since commit 8cd5601c5060
and dfbca41f3479):

#define arch_scale_cpu_capacity 'arch implementation of capacity scaling by
micro-architectural + max frequency (OPPmax)'

#define arch_scale_freq_capacity 'arch implementation of capacity scaling by
'frequency ((OPPmin..OPPmax)'

There is no naming convention from the scheduler side on these functions
though. They should just express what they're doing, scaling capacity by
something.

So, discussing this naming with Morten off-line we seemed actually to
agree that the following might adhere even better to what the functions
actually do:

 topology_parse_cpu_capacity() - it parses the raw capacity-dmips-mhz
 values from DT; so it seems OK to leave capacity in the name here

 topology_set_cpu_scale() - it sets the per_cpu cpu_scale variable; so
 this name seems saner that the one with "_capacity"

 topology_get_cpu_scale() - dual of the previous one

 topology_normalize_cpu_scale() - it normalizes cpu_scale variables
 across the system CPUs (and calls topology_set_cpu_scale() to set the
 normalized values)

Greg, does this approack look saner to you as well?
If yes, I'll send a new version of the set shortly.

And, as I already said on IRC, apologies for this naming fight. :)

Thanks,

- Juri