On Wed, Dec 30, 2015 at 04:48:42AM +0000, Chakravarty, Souvik K wrote: > > > > -----Original Message----- > > From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver- > > x86-owner@xxxxxxxxxxxxxxx] On Behalf Of Darren Hart > > Sent: Wednesday, December 30, 2015 6:20 AM > > To: Chakravarty, Souvik K <souvik.k.chakravarty@xxxxxxxxx>; Rafael Wysocki > > <rjw@xxxxxxxxxxxxx> > > Cc: platform-driver-x86@xxxxxxxxxxxxxxx; Kasagar, Srinidhi > > <srinidhi.kasagar@xxxxxxxxx>; Zha, Qipeng <qipeng.zha@xxxxxxxxx>; > > Muralidhar, Rajeev D <rajeev.d.muralidhar@xxxxxxxxx>; Ghorai, Sukumar > > <sukumar.ghorai@xxxxxxxxx>; Yu, Ong Hock <ong.hock.yu@xxxxxxxxx>; Li, > > Aubrey <aubrey.li@xxxxxxxxx> > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
