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@xxxxxxxxxxxxxxx> Intel Finland Oy -- 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
