Re: [PATCH v1 1/1] platform/x86/intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote:
> On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote:
> > This patch does the following:
> > - refactors code to use recently introduced
> > DEFINE_DEBUGFS_ATTRIBUTE() macro
> > - makes absence of DEBUG_FS non-fatal error


> > -	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));
> > +	value = pmc_core_reg_read(pmcdev,
> > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET);
> > +	*val = pmc_core_adjust_slp_s0_step(value);
> >  
> 
> We were ensuring a new line for printing the output value with
> seq_printf 
> whereas in this case the output is printed along with the shell
> prompt.
> Sometimes this could be harder to read.

This would be a difference in the behaviour to a generic assumption
(since this new macro is going to be used by plenty of simple attributes
under debugfs). If you still consider it's inappropriate I would
recommend to propose a fix globally then.

Regarding to change behaviour of the current implementation I think
Darren can tell his opinion. (For me at least pros and cons of new
format quite obvious).

>  
> > +struct dentry;
> > +
> 
> Why do we need this ?

Since we are using pointer to undefined type.

> 
> >  /**
> >   * struct pmc_dev - pmc device structure
> >   * @base_addr:		comtains pmc base address
> > @@ -42,9 +45,7 @@
> >  struct pmc_dev {
> >  	u32 base_addr;
> >  	void __iomem *regbase;
> > -#if IS_ENABLED(CONFIG_DEBUG_FS)
> >  	struct dentry *dbgfs_dir;
> > -#endif /* CONFIG_DEBUG_FS */



-- 

Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy