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_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).
>
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?
> >
> > > +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.
> >
> > > /**
> > > * 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@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
--
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
