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

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

[Andy Shevchenko]

This patch does the following:
- refactors code to use recently introduced DEFINE_DEBUGFS_ATTRIBUTE() macro
- makes absence of DEBUG_FS non-fatal error

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 45 ++++++++---------------------------
 drivers/platform/x86/intel_pmc_core.h |  5 ++--
 2 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index e57f923..c29f59b 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -23,7 +23,6 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/pci.h>
-#include <linux/seq_file.h>
 
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
@@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read);
 
-#if IS_ENABLED(CONFIG_DEBUG_FS)
-static int pmc_core_dev_state_show(struct seq_file *s, void *unused)
+static int pmc_core_dev_state_get(void *data, u64 *val)
 {
-	struct pmc_dev *pmcdev = s->private;
-	u32 counter_val;
+	struct pmc_dev *pmcdev = data;
+	u32 value;
 
-	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);
 
 	return 0;
 }
 
-static int pmc_core_dev_state_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, pmc_core_dev_state_show, inode->i_private);
-}
-
-static const struct file_operations pmc_core_dev_state_ops = {
-	.open           = pmc_core_dev_state_open,
-	.read           = seq_read,
-	.llseek         = seq_lseek,
-	.release        = single_release,
-};
+DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu");
 
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
@@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	struct dentry *dir, *file;
 
 	dir = debugfs_create_dir("pmc_core", NULL);
-	if (!dir)
+	if (IS_ERR_OR_NULL(dir))
 		return -ENOMEM;
 
 	pmcdev->dbgfs_dir = dir;
 	file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO,
-				   dir, pmcdev, &pmc_core_dev_state_ops);
+				   dir, pmcdev, &pmc_core_dev_state);
 
 	if (!file) {
 		pmc_core_dbgfs_unregister(pmcdev);
@@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 
 	return 0;
 }
-#else
-static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
-{
-	return 0;
-}
-
-static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
-{
-}
-#endif /* CONFIG_DEBUG_FS */
 
 static const struct x86_cpu_id intel_pmc_core_ids[] = {
 	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT,
@@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 
 	err = pmc_core_dbgfs_register(pmcdev);
-	if (err < 0) {
-		dev_err(&dev->dev, "PMC Core: debugfs register failed.\n");
-		return err;
-	}
+	if (err < 0)
+		dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n");
 
 	pmc.has_slp_s0_res = true;
 	return 0;
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index a9dadaf..9689b92 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -23,11 +23,14 @@
 
 /* Sunrise Point Power Management Controller PCI Device ID */
 #define SPT_PMC_PCI_DEVICE_ID			0x9d21
+
 #define SPT_PMC_BASE_ADDR_OFFSET		0x48
 #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET	0x13c
 #define SPT_PMC_MMIO_REG_LEN			0x100
 #define SPT_PMC_SLP_S0_RES_COUNTER_STEP		0x64
 
+struct dentry;
+
 /**
  * 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 */
 	bool has_slp_s0_res;
 };
 
-- 
2.8.1

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

[Rajneesh Bhardwaj]

On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote:

Sorry for the late reply. I compiled/tested this on linux-next on a Skylake
system and it looks ok to me except for a couple of minor comments.

> This patch does the following:
> - refactors code to use recently introduced DEFINE_DEBUGFS_ATTRIBUTE() macro
> - makes absence of DEBUG_FS non-fatal error
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 45 ++++++++---------------------------
>  drivers/platform/x86/intel_pmc_core.h |  5 ++--
>  2 files changed, 13 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index e57f923..c29f59b 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,6 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/pci.h>
> -#include <linux/seq_file.h>
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
> @@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data)
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read);
>  
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> -static int pmc_core_dev_state_show(struct seq_file *s, void *unused)
> +static int pmc_core_dev_state_get(void *data, u64 *val)
>  {
> -	struct pmc_dev *pmcdev = s->private;
> -	u32 counter_val;
> +	struct pmc_dev *pmcdev = data;

I am hoping that data being passed here is similar to inode.i_private
so that we get the address of pmc struct here.

> +	u32 value;
>  
> -	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);
>  
>  	return 0;
>  }
>  
> -static int pmc_core_dev_state_open(struct inode *inode, struct file *file)
> -{
> -	return single_open(file, pmc_core_dev_state_show, inode->i_private);
> -}
> -
> -static const struct file_operations pmc_core_dev_state_ops = {
> -	.open           = pmc_core_dev_state_open,
> -	.read           = seq_read,
> -	.llseek         = seq_lseek,
> -	.release        = single_release,
> -};
> +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu");
>  
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
> @@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  	struct dentry *dir, *file;
>  
>  	dir = debugfs_create_dir("pmc_core", NULL);
> -	if (!dir)
> +	if (IS_ERR_OR_NULL(dir))
>  		return -ENOMEM;
>  
>  	pmcdev->dbgfs_dir = dir;
>  	file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO,

Should we consider using debugfs_create_file_unsafe ?

> -				   dir, pmcdev, &pmc_core_dev_state_ops);
> +				   dir, pmcdev, &pmc_core_dev_state);
>  
>  	if (!file) {
>  		pmc_core_dbgfs_unregister(pmcdev);
> @@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  
>  	return 0;
>  }
> -#else
> -static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> -{
> -	return 0;
> -}
> -
> -static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> -{
> -}
> -#endif /* CONFIG_DEBUG_FS */
>  
>  static const struct x86_cpu_id intel_pmc_core_ids[] = {
>  	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT,
> @@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  
>  	err = pmc_core_dbgfs_register(pmcdev);
> -	if (err < 0) {
> -		dev_err(&dev->dev, "PMC Core: debugfs register failed.\n");
> -		return err;
> -	}
> +	if (err < 0)
> +		dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n");
>  
>  	pmc.has_slp_s0_res = true;
>  	return 0;
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index a9dadaf..9689b92 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -23,11 +23,14 @@
>  
>  /* Sunrise Point Power Management Controller PCI Device ID */
>  #define SPT_PMC_PCI_DEVICE_ID			0x9d21
> +
>  #define SPT_PMC_BASE_ADDR_OFFSET		0x48
>  #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET	0x13c
>  #define SPT_PMC_MMIO_REG_LEN			0x100
>  #define SPT_PMC_SLP_S0_RES_COUNTER_STEP		0x64
>  
> +struct dentry;
> +
>  /**
>   * 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 */
>  	bool has_slp_s0_res;
>  };
>  
> -- 
> 2.8.1
> 

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

On Thu, 2016-06-23 at 20:56 +0530, Rajneesh Bhardwaj wrote:
> On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote:
> 
> Sorry for the late reply. I compiled/tested this on linux-next on a
> Skylake
> system and it looks ok to me except for a couple of minor comments.
> 
> > +static int pmc_core_dev_state_get(void *data, u64 *val)
> >  {
> > -	struct pmc_dev *pmcdev = s->private;
> > -	u32 counter_val;
> > +	struct pmc_dev *pmcdev = data;
> 
> I am hoping that data being passed here is similar to inode.i_private
> so that we get the address of pmc struct here.

Yes, I went through the debugfs support code to be sure.

> > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state,
> > pmc_core_dev_state_get, NULL, "%llu");

> >  	pmcdev->dbgfs_dir = dir;
> >  	file = debugfs_create_file("slp_s0_residency_usec", S_IFREG
> > | S_IRUGO,
> 
> Should we consider using debugfs_create_file_unsafe ?

But why? I would choose safest option among others if there is no strong
objection and explanation.

-- 

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

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

[Rajneesh Bhardwaj]

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
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 45 ++++++++---------------------------
>  drivers/platform/x86/intel_pmc_core.h |  5 ++--
>  2 files changed, 13 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index e57f923..c29f59b 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,6 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/pci.h>
> -#include <linux/seq_file.h>
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
> @@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data)
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read);
>  
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> -static int pmc_core_dev_state_show(struct seq_file *s, void *unused)
> +static int pmc_core_dev_state_get(void *data, u64 *val)
>  {
> -	struct pmc_dev *pmcdev = s->private;
> -	u32 counter_val;
> +	struct pmc_dev *pmcdev = data;
> +	u32 value;
>  
> -	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.

>  	return 0;
>  }
>  
> -static int pmc_core_dev_state_open(struct inode *inode, struct file *file)
> -{
> -	return single_open(file, pmc_core_dev_state_show, inode->i_private);
> -}
> -
> -static const struct file_operations pmc_core_dev_state_ops = {
> -	.open           = pmc_core_dev_state_open,
> -	.read           = seq_read,
> -	.llseek         = seq_lseek,
> -	.release        = single_release,
> -};
> +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu");
>  
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
> @@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  	struct dentry *dir, *file;
>  
>  	dir = debugfs_create_dir("pmc_core", NULL);
> -	if (!dir)
> +	if (IS_ERR_OR_NULL(dir))
>  		return -ENOMEM;
>  
>  	pmcdev->dbgfs_dir = dir;
>  	file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO,
> -				   dir, pmcdev, &pmc_core_dev_state_ops);
> +				   dir, pmcdev, &pmc_core_dev_state);
>  
>  	if (!file) {
>  		pmc_core_dbgfs_unregister(pmcdev);
> @@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  
>  	return 0;
>  }
> -#else
> -static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> -{
> -	return 0;
> -}
> -
> -static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> -{
> -}
> -#endif /* CONFIG_DEBUG_FS */
>  
>  static const struct x86_cpu_id intel_pmc_core_ids[] = {
>  	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT,
> @@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  
>  	err = pmc_core_dbgfs_register(pmcdev);
> -	if (err < 0) {
> -		dev_err(&dev->dev, "PMC Core: debugfs register failed.\n");
> -		return err;
> -	}
> +	if (err < 0)
> +		dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n");
>  
>  	pmc.has_slp_s0_res = true;
>  	return 0;
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index a9dadaf..9689b92 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -23,11 +23,14 @@
>  
>  /* Sunrise Point Power Management Controller PCI Device ID */
>  #define SPT_PMC_PCI_DEVICE_ID			0x9d21
> +
>  #define SPT_PMC_BASE_ADDR_OFFSET		0x48
>  #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET	0x13c
>  #define SPT_PMC_MMIO_REG_LEN			0x100
>  #define SPT_PMC_SLP_S0_RES_COUNTER_STEP		0x64
>  
> +struct dentry;
> +

Why do we need this ?

>  /**
>   * 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 */
>  	bool has_slp_s0_res;
>  };
>  
> -- 
> 2.8.1
> 

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

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

>  
> > +struct dentry;
> > +
> 
> Why do we need this ?

Since we are using pointer to undefined type.

> 
> >  /**
> >   * 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@linux.intel.com>
Intel Finland Oy

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

[Rajneesh Bhardwaj]

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@linux.intel.com>
> Intel Finland Oy
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

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

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

[Rajneesh Bhardwaj]

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@linux.intel.com>
> Intel Finland Oy
i> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

On Wed, 2016-06-29 at 17:46 +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:

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

If it touches the same attribute it needs to be seen first before
applying this one.

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

For the rest we have to see the implementation and decide how to proceed
(since it's a debugfs dedicated for _debugging_) we might change it in
the way we like. Though I don't encourage people to do this. I like
interfaces on which people thought before implementing.

-- 

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

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

[Rajneesh Bhardwaj]

On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote:
> On Wed, 2016-06-29 at 17:46 +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:
> 
> > > > 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.
> 
> If it touches the same attribute it needs to be seen first before
> applying this one.
>

No, this attribute wont be touched.
 
> > 
> > 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.
> 
> For the rest we have to see the implementation and decide how to proceed
> (since it's a debugfs dedicated for _debugging_) we might change it in
> the way we like. Though I don't encourage people to do this. I like
> interfaces on which people thought before implementing.
> 

Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave the scope
for having seq_printf for future patches i.e. linux/seq_file.h and thus would
have a dependency on struct file_operations for seq_read.

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

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

On Wed, 2016-06-29 at 18:50 +0530, Rajneesh Bhardwaj wrote:
> On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote:
> > On Wed, 2016-06-29 at 17:46 +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:
> > 
> > > > > 


> > If it touches the same attribute it needs to be seen first before
> > applying this one.
> > 
> 
> No, this attribute wont be touched.

So, if there is no objections (taking into consideration minors you
mentioned) I think the patch can be applied.

> 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.
> > 
> > For the rest we have to see the implementation and decide how to
> > proceed
> > (since it's a debugfs dedicated for _debugging_) we might change it
> > in
> > the way we like. Though I don't encourage people to do this. I like
> > interfaces on which people thought before implementing.
> > 
> 
> Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave
> the scope
> for having seq_printf for future patches i.e. linux/seq_file.h and
> thus would
> have a dependency on struct file_operations for seq_read.

Of course! No-one is insisting to use only the macro and simple
attributes.


-- 

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

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

[Darren Hart]

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.

Agreed.

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

We should use the available subsystem tools for boilerplate reduction whenever
possible. Fortunately, debugfs is debug, and we do not require that it provide a
consistent user interface like with sysfs (via a version attribute).

> 
> >  
> > > +struct dentry;
> > > +
> > 
> > Why do we need this ?
> 
> Since we are using pointer to undefined type.

Erm... why is dentry an undefined type?

> 
> > 
> > >  /**
> > >   * 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@linux.intel.com>
> Intel Finland Oy
> 

-- 
Darren Hart
Intel Open Source Technology Center

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

[Darren Hart]

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

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

[Darren Hart]

On Wed, Jun 29, 2016 at 04:33:50PM +0300, Andy Shevchenko wrote:
> On Wed, 2016-06-29 at 18:50 +0530, Rajneesh Bhardwaj wrote:
> > On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote:
> > > On Wed, 2016-06-29 at 17:46 +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:
> > > 
> > > > > > 
> 
> 
> > > If it touches the same attribute it needs to be seen first before
> > > applying this one.
> > > 
> > 
> > No, this attribute wont be touched.
> 
> So, if there is no objections (taking into consideration minors you
> mentioned) I think the patch can be applied.
> 
> > 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.
> > > 
> > > For the rest we have to see the implementation and decide how to
> > > proceed
> > > (since it's a debugfs dedicated for _debugging_) we might change it
> > > in
> > > the way we like. Though I don't encourage people to do this. I like
> > > interfaces on which people thought before implementing.
> > > 
> > 
> > Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave
> > the scope
> > for having seq_printf for future patches i.e. linux/seq_file.h and
> > thus would
> > have a dependency on struct file_operations for seq_read.
> 
> Of course! No-one is insisting to use only the macro and simple
> attributes.

Agreed. I do wonder if some of the newer debugfs types like array and regset
would be sufficient for your needs. Whenever you can reuse an abstraction of the
subsystem through higher level interfaces, I encourage you to do so as it
reduces code size and potential programming errors.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Andy Shevchenko]

On Fri, 2016-07-01 at 14:24 -0700, Darren Hart wrote:
 
> > > > +struct dentry;
> > > > +
> > > 
> > > Why do we need this ?
> > 
> > Since we are using pointer to undefined type.
> 
> Erm... why is dentry an undefined type?

In this header it's undefined. Though I can't get any warning using
recent compilers, thus the change is not needed.

-- 

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

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

[Rajneesh Bhardwaj]

> 
> So, if there is no objections (taking into consideration minors you
> mentioned) I think the patch can be applied.
>

I think we still need to address a couple of minor issues and might need a
version 2 of this patch. In case we want to apply this one here is one patch
that can applied on top of this one. Please see the proposed changes below.

From 42d899968c8c07f103390fcaebdaf2ba54cb3363 Mon Sep 17 00:00:00 2001
From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Date: Mon, 4 Jul 2016 16:22:31 +0530
Subject: [PATCH] platform/x86/intel_pmc_core: Enhances debugfs attribute
patch

Refines "convert to DEFINE_DEBUGFS_ATTRIBUTE" patch for PMC Core.

This change can be applied over the Convert to DEFINE_DEBUGFS_ATTRIBUTE
patch sent by Andy Shevchenko to the platform drivers mailing list. This
helps preserve usage of seq_file.h in the pmc_core driver. This also caters
to some minor cosmetic changes suggested during the code review process.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 4 +++-
 drivers/platform/x86/intel_pmc_core.h | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c
b/drivers/platform/x86/intel_pmc_core.c
index c29f59b..18af49f 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/pci.h>
+#include <linux/seq_file.h>

 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
@@ -88,7 +89,8 @@ static int pmc_core_dev_state_get(void *data, u64 *val)
        return 0;
 }

-DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL,
"%llu");
+DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL,
+                       "%llu\n");

 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
diff --git a/drivers/platform/x86/intel_pmc_core.h
b/drivers/platform/x86/intel_pmc_core.h
index 9689b92..6265480 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -29,7 +29,6 @@
 #define SPT_PMC_MMIO_REG_LEN                   0x100
 #define SPT_PMC_SLP_S0_RES_COUNTER_STEP                0x64

-struct dentry;

 /**
  * struct pmc_dev - pmc device structure
--
1.9.1


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

-- 
Best Regards,
Rajneesh

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

[Andy Shevchenko]

On Mon, 2016-07-04 at 17:05 +0530, Rajneesh Bhardwaj wrote:
> > 
> > 
> > So, if there is no objections (taking into consideration minors you
> > mentioned) I think the patch can be applied.
> > 
> 
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,6 +23,7 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/pci.h>
> +#include <linux/seq_file.h>

What is this for?

> 
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
> @@ -88,7 +89,8 @@ static int pmc_core_dev_state_get(void *data, u64
> *val)
>         return 0;
>  }
> 
> -DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get,
> NULL,
> "%llu");
> +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get,
> NULL,
> +                       "%llu\n");

Okay, since debugfs is actually using them in standard code, we may add
it here as well.

> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -29,7 +29,6 @@
>  #define SPT_PMC_MMIO_REG_LEN                   0x100
>  #define SPT_PMC_SLP_S0_RES_COUNTER_STEP                0x64
> 
> -struct dentry;

Agreed.

-- 

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

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

[Rajneesh Bhardwaj]

On Mon, Jul 04, 2016 at 03:01:33PM +0300, Andy Shevchenko wrote:
> On Mon, 2016-07-04 at 17:05 +0530, Rajneesh Bhardwaj wrote:
> > > 
> > > 
> > > So, if there is no objections (taking into consideration minors you
> > > mentioned) I think the patch can be applied.
> > > 
> > 
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> >  #include <linux/pci.h>
> > +#include <linux/seq_file.h>
> 
> What is this for?
>

At this point it may be redundant as there will be no consumer for this.
We may add it later when it's needed for seq_printf / seq_* etc. So i think
we might just do away with this for v2.
 
> > 
> >  #include <asm/cpu_device_id.h>
> >  #include <asm/intel-family.h>
> > @@ -88,7 +89,8 @@ static int pmc_core_dev_state_get(void *data, u64
> > *val)
> >         return 0;
> >  }
> > 
> > -DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get,
> > NULL,
> > "%llu");
> > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get,
> > NULL,
> > +                       "%llu\n");
> 
> Okay, since debugfs is actually using them in standard code, we may add
> it here as well.
> 
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -29,7 +29,6 @@
> >  #define SPT_PMC_MMIO_REG_LEN                   0x100
> >  #define SPT_PMC_SLP_S0_RES_COUNTER_STEP                0x64
> > 
> > -struct dentry;
> 
> Agreed.
> 
> -- 
> 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

-- 
Best Regards,
Rajneesh