diff mbox

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

Message ID 20160704113518.GC1177@rajaneesh-OptiPlex-9010 (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Rajneesh Bhardwaj July 4, 2016, 11:35 a.m. UTC 
> 
> 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(-)

--
1.9.1


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

Comments

Andy Shevchenko July 4, 2016, 12:01 p.m. UTC | #1 
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.
Rajneesh Bhardwaj July 4, 2016, 12:28 p.m. UTC | #2 
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
diff mbox

Patch

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