Re: [PATCH v5] platform:x86: Add PMC Driver for Intel Core SoC

[Rajneesh Bhardwaj]

On Tue, May 24, 2016 at 11:12:10PM +0300, Andy Shevchenko wrote:
> On Tue, May 24, 2016 at 10:43 PM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> > On Tue, May 24, 2016 at 10:07:32PM +0300, Andy Shevchenko wrote:
> >> On Tue, May 24, 2016 at 9:54 PM, Andy Shevchenko
> >> <andy.shevchenko@xxxxxxxxx> wrote:
> >> > On Tue, May 24, 2016 at 5:25 PM, Rajneesh Bhardwaj
> >> > <rajneesh.bhardwaj@xxxxxxxxx> wrote:
> >>
> >> >> +static int pmc_core_dev_state_show(struct seq_file *s, void *unused)
> >> >> +{
> >> >> +       struct pmc_dev *pmcdev = s->private;
> >> >> +       u32 counter_val;
> >> >> +
> >> >> +       counter_val = pmc_core_reg_read(pmcdev,
> >> >> +                                       SPT_PMC_SLP_S0_RES_COUNTER_OFFSET);
> >> >> +       seq_printf(s, "%u\n", pmc_core_adjust_slp_s0_step(counter_val));
> >> >> +
> >> >> +       return 0;
> >> >> +}
> >> >> +
> >> >> +static int pmc_core_dev_state_open(struct inode *inode, struct file *file)
> >> >> +{
> >> >> +       return single_open(file, pmc_core_dev_state_show, inode->i_private);
> >> >> +}
> >> >> +
> >> >> +static const struct file_operations pmc_core_dev_state_ops = {
> >> >> +       .open           = pmc_core_dev_state_open,
> >> >> +       .read           = seq_read,
> >> >> +       .llseek         = seq_lseek,
> >> >> +       .release        = single_release,
> >> >> +};
> >> >
> >> > I suppose DEFINE_SIMPLE_ATTRIBUTE might reduce amount of LOC.
> >>
> >> Correction:
> >> DEFINE_DEBUGFS_ATTRIBUTE()
> >
> > Andy caught the couple things I was going to add and a couple more. Please give
> > his feedback one more pass (except where I noted we had already covered things
> > like the header and the module build), and we should still be able to get this
> > into 4.7 if I have the next rev by tomorrow.
> 
> For me patch is in a good shape, just style issues and couple of
> things indeed that make code looking better.
>

Thanks again for the review.
I have sent patch v6 addressing your consolidated feedback except a couple
of things that you suggested.

I guess DEFINE_DEBUGFS_ATTRIBUTE is linux-next so to avoid possible build
issues can we skip this for now? We can revisit this change when this is
readily available in the mainline kernel in near future.

Also, Makefile and Kconfig files in pdx86 need alphabetical sorting so 
skipped the suggestion related to alphabetical placement in Makefile.
 
> -- 
> With Best Regards,
> Andy Shevchenko

-- 
Best Regards,
Rajneesh