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

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

On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote:
> On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote:
> > 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_COUN
> > > > TER_
> > > > 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).
> > 
> 
> While this is good for simple attributes i see a potential issue with
> this
> approch when we want to display multiple attributes for a single
> debugfs
> entry.
>  We typically use seq_printf in a loop and display multiline output
> for a single debugfs entry. How do we go about the scenarios where we
> want
> to display formatted  multiline debug information?

We are talking about abstract case or is it change coming to this very
driver?

>  
> > >  
> > > > +struct dentry;
> > > > +
> > > 
> > > Why do we need this ?
> > 
> > Since we are using pointer to undefined type.
> > 
> 
> I think we can omit this line if we chose to take this approach.

Yeah, it does not produce a warning on new compilers, though I think
Darren can share his opinion.


-- 

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