Re: [PATCH v2 4/5] platform:x86: Add Intel Telemetry Debugfs interfaces

[Darren Hart <dvhart@infradead.org>]

On Wed, Dec 30, 2015 at 04:48:42AM +0000, Chakravarty, Souvik K wrote:
> 
> 
> > -----Original Message-----
> > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-
> > x86-owner@vger.kernel.org] On Behalf Of Darren Hart
> > Sent: Wednesday, December 30, 2015 6:20 AM
> > To: Chakravarty, Souvik K <souvik.k.chakravarty@intel.com>; Rafael Wysocki
> > <rjw@rjwysocki.net>
> > Cc: platform-driver-x86@vger.kernel.org; Kasagar, Srinidhi
> > <srinidhi.kasagar@intel.com>; Zha, Qipeng <qipeng.zha@intel.com>;
> > Muralidhar, Rajeev D <rajeev.d.muralidhar@intel.com>; Ghorai, Sukumar
> > <sukumar.ghorai@intel.com>; Yu, Ong Hock <ong.hock.yu@intel.com>; Li,
> > Aubrey <aubrey.li@intel.com>
> > Subject: Re: [PATCH v2 4/5] platform:x86: Add Intel Telemetry Debugfs
> > interfaces
> > 
> > On Wed, Dec 23, 2015 at 04:14:16PM +0530, Souvik Kumar Chakravarty wrote:

...

> > What is the plan for supporting future SoCs here? You have APL incorporated
> > into this file. Do you intend to add each generation to this file?
> 
> Yes that is the intention.

OK, I guess we'll see how it scales and how many SoC generations follow with
this interface.

...

> > 
> > > +				d3_sts[idx] =
> > > +				(evtlog[index].telem_evtlog >>
> > > +				debugfs_conf->ioss_d0ix_data[idx].bit_pos)
> > &
> > > +				TELEM_MASK_BIT;
> > 
> > By the time you have indented 4, and really should be 5, the preference is to
> > determine if this can be refactored into a shallower nesting structure.
> > 
> > Perhaps a macro of some sort as these all seem fairly repetitive.
> 
> We can get in a macro or inline function, something like this:  
>         for (index = 0; index < ret; index++) {
>                 seq_printf(s, "%-32s %llu\n",
>                            name[index], evtlog[index].telem_evtlog);
> 
>                 if (evtlog[index].telem_evtid == debugfs_conf->pss_idle_id) 
> 		TELEM_PARSE_LOG(debugfs_conf->ioss_d3_id, debugfs_conf->ioss_d0ix_evts , d3_sts , debugfs_conf->ioss_d0ix_data)
> 	If.....
> 

With a shorter alians for debugfs_conf, something like this is much more legible
and far less repetitive (easier to maintain).

> 	}
> 
> #define TELEM_PARSE_LOG (EVTS, BUF, EVT_DATA)     \    
>                         for (int idx = 0; idx < EVTS; idx++) \
>                                	BUF[idx] = (evtlog[index].telem_evtlog >> EVT_DATA[idx].bit_pos) & TELEM_MASK_BIT; \
>                         continue; \
> 
> And something on the same lines for parsing counters.
> 
> Or we could have TELEM_CHECK_AND_PARSE where even the 'if' is moved inside the macro.
> Does that look OK?

I think the above will work, but if you think you can remove some of the
redundancy with a CHECK_AND_PARSE without obfuscating the logic too much, give
it a shot.

-- 
Darren Hart
Intel Open Source Technology Center