On Wed, Dec 30, 2015 at 04:48:42AM +0000, Chakravarty, Souvik K wrote:
>
>
> > -----Original Message-----
> > From: [email protected] [mailto:platform-driver-
> > [email protected]] On Behalf Of Darren Hart
> > Sent: Wednesday, December 30, 2015 6:20 AM
> > To: Chakravarty, Souvik K <[email protected]>; Rafael Wysocki
> > <[email protected]>
> > Cc: [email protected]; Kasagar, Srinidhi
> > <[email protected]>; Zha, Qipeng <[email protected]>;
> > Muralidhar, Rajeev D <[email protected]>; Ghorai, Sukumar
> > <[email protected]>; Yu, Ong Hock <[email protected]>; Li,
> > Aubrey <[email protected]>
> > 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
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
