Re: [RFC PATCH v3 5/7] thermal: add a basic cpu power actor

["Javi Merino" <javi.merino@arm.com>]

On Thu, Jun 12, 2014 at 03:26:50PM +0100, Javi Merino wrote:
> On Wed, Jun 11, 2014 at 01:05:37PM +0100, Eduardo Valentin wrote:
> > On Tue, Jun 03, 2014 at 11:18:33AM +0100, Javi Merino wrote:
> > > Introduce a power actor for cpus.  It has a basic power model to get
> > > the current power utilization and uses cpufreq cooling devices to set
> > > the desired power.  It uses the current frequency (as reported by
> > > cpufreq) as well as load and OPPs for the power calculations.  The
> > > cpus must have registered their OPPs in the OPP library.
> > >
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > Cc: Eduardo Valentin <edubezval@gmail.com>
> > > Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > ---
> > >  Documentation/thermal/power_actor.txt     | 126 +++++++
> > >  drivers/thermal/Kconfig                   |   5 +
> > >  drivers/thermal/power_actor/Kconfig       |   9 +
> > >  drivers/thermal/power_actor/Makefile      |   2 +
> > >  drivers/thermal/power_actor/cpu_actor.c   | 601 ++++++++++++++++++++++++++++++
> > >  drivers/thermal/power_actor/power_actor.h |  41 ++
> > >  6 files changed, 784 insertions(+)
> > >  create mode 100644 drivers/thermal/power_actor/Kconfig
> > >  create mode 100644 drivers/thermal/power_actor/cpu_actor.c
> > >
[...]
> > > diff --git a/drivers/thermal/power_actor/Kconfig b/drivers/thermal/power_actor/Kconfig
> > > new file mode 100644
> > > index 000000000000..fa542ca99cdb
> > > --- /dev/null
> > > +++ b/drivers/thermal/power_actor/Kconfig
> > > @@ -0,0 +1,9 @@
> > > +#
> > > +# Thermal power actor configuration
> > > +#
> > > +
> > > +config THERMAL_POWER_ACTOR_CPU
> > > +     bool
> > > +     prompt "Simple power model for a CPU"
> > > +     help
> > > +       A simple CPU power model
> >
> > A better help is always welcome.
> 
> I guess I can repeat some of the text in the Documentation here.

I've been thinking about it and I'd rather remove the prompt
altogether.  There's no point in asking the user about this, drivers
that use this API can select THERMAL_POWER_ACTOR_CPU themselves.  So
I'll remove the help text.

[...]
> > > diff --git a/drivers/thermal/power_actor/power_actor.h b/drivers/thermal/power_actor/power_actor.h
> > > index 28098f43630b..230317c284b2 100644
> > > --- a/drivers/thermal/power_actor/power_actor.h
> > > +++ b/drivers/thermal/power_actor/power_actor.h
> > > @@ -17,11 +17,16 @@
> > >  #ifndef __POWER_ACTOR_H__
> > >  #define __POWER_ACTOR_H__
> > >
> > > +#include <linux/cpumask.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > >  #include <linux/list.h>
> > > +#include <linux/thermal.h>
> > >
> > >  #define MAX_NUM_ACTORS 8
> > >
> > >  enum power_actor_types {
> > > +     POWER_ACTOR_CPU,
> >
> > Using struct device is more scalable no? What if we want to provide a
> > power actor for a specific bus, or device, or coprocessor? Are we going
> > to maintain this enum for every single new user?
> 
> The counterargument would be, does every single device need to carry
> this?  Anyway, I'll see if we can add the power actor to the "struct
> device".

I've decided to remove the only user of the power_actor_types enum and
remove this field as well.  It's simplifies things which is always
good.

Cheers,
Javi