Re: [PATCH 04/13] intel_rapl: introduce struct rapl_private

[Zhang Rui <rui.zhang@intel.com>]

On 二, 2019-07-02 at 23:44 +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > 
> > Introduce a new structure, rapl_private, to save the private data
> > for different RAPL Interface.
> The new structure is called rapl_priv in the patch.
> 
> Besides, I would call it rapl_if_priv to indicate that this related
> to
> an interface.
> 
> Also, IMO there should be a kerneldoc comment for it.
> 
okay.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/powercap/intel_rapl.c | 58 +++++++++++++++++++++--------
> > --------------
> >  include/linux/intel_rapl.h    |  7 ++++++
> >  2 files changed, 35 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl.c
> > b/drivers/powercap/intel_rapl.c
> > index adb35ec..a804370 100644
> > --- a/drivers/powercap/intel_rapl.c
> > +++ b/drivers/powercap/intel_rapl.c
> > @@ -75,6 +75,8 @@ enum unit_type {
> >         TIME_UNIT,
> >  };
> > 
> > +static struct rapl_priv rapl_msr_priv;
> Please add a comment describing this variable and its role.
> 
> BTW, I'd rather call it rapl_if_msr.
> 
> > 
> > +
> >  /* per domain data, some are optional */
> >  #define NR_RAW_PRIMITIVES (NR_RAPL_PRIMITIVES - 2)
> > 
> > @@ -155,17 +157,14 @@ static const char * const rapl_domain_names[]
> > = {
> >         "psys",
> >  };
> > 
> > -static struct powercap_control_type *control_type; /* PowerCap
> > Controller */
> > -static struct rapl_domain *platform_rapl_domain; /* Platform
> > (PSys) domain */
> Why do these things need to go into rapl_priv?
> 
different RAPL interface may have its own control_type,
"intel_rapl_msr" vs. "intel_rapl_mmio".

MMIO RAPL interface does not have platform rapl domain so far, but
given that the current platform rapl domain uses
MSR_PLATFORM_POWER_LIMIT and MSR_PLATFORM_ENERGY_STATUS.
I made it part of MSR RAPL interface driver.

thanks,
rui