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. > > -- > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Intel Finland Oy i> -- > 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 -- Best Regards, Rajneesh -- 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
