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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 29, 2016 at 05:46:04PM +0530, Rajneesh Bhardwaj wrote:
> On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote:
> > 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?
> >
> 
> We will be sending few patches for this very driver shortly and one such
> feature would require multiline formatted output for a single debugfs entry.
> 
> Can we have seq_printf for such cases and use DEFINE_DEBUGFS_ATTRIBUTE for
> other simple attributes? Doing this might raise some coding style consistency 
> concerns though so lets wait to hear  Darren's opinion on this patch.
>  
> > >  
> > > > >  
> > > > > > +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.
> > 
> 
> Sure.

I'm not sure why we need it. dentry isn't debugfs specific. When is it
undefined?

-- 
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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux