[PATCH v1 0/4] iosf_mbi: clean up

[PATCH v1 0/4] iosf_mbi: clean up

[Andy Shevchenko]

There are few patches to clean up (with small fixes) iosf_mbi driver along with
extension it to cover Intel Tangier.

Andy Shevchenko (4):
  iosf_mbi: check result for all calls of debugfs API
  iosf_mbi: pci_dev_put() is NULL-proof
  iosf_mbi: group global variables
  iosf_mbi: append Intel Tangier ID

 arch/x86/include/asm/iosf_mbi.h |  8 ++++----
 arch/x86/kernel/iosf_mbi.c      | 31 +++++++++++++++----------------
 2 files changed, 19 insertions(+), 20 deletions(-)

-- 
2.1.4

[PATCH v1 1/4] iosf_mbi: check result for all calls of debugfs API

[Andy Shevchenko]

While here, replase IS_ERR_OR_NULL(x) to just !x since first one does the job
for us.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/iosf_mbi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/iosf_mbi.c b/arch/x86/kernel/iosf_mbi.c
index 82f8d02..2362da9 100644
--- a/arch/x86/kernel/iosf_mbi.c
+++ b/arch/x86/kernel/iosf_mbi.c
@@ -240,17 +240,17 @@ static void iosf_sideband_debug_init(void)
 
 	/* mdr */
 	d = debugfs_create_x32("mdr", 0660, iosf_dbg, &dbg_mdr);
-	if (IS_ERR_OR_NULL(d))
+	if (!d)
 		goto cleanup;
 
 	/* mcrx */
-	debugfs_create_x32("mcrx", 0660, iosf_dbg, &dbg_mcrx);
-	if (IS_ERR_OR_NULL(d))
+	d = debugfs_create_x32("mcrx", 0660, iosf_dbg, &dbg_mcrx);
+	if (!d)
 		goto cleanup;
 
 	/* mcr - initiates mailbox tranaction */
-	debugfs_create_file("mcr", 0660, iosf_dbg, &dbg_mcr, &iosf_mcr_fops);
-	if (IS_ERR_OR_NULL(d))
+	d = debugfs_create_file("mcr", 0660, iosf_dbg, &dbg_mcr, &iosf_mcr_fops);
+	if (!d)
 		goto cleanup;
 
 	return;
-- 
2.1.4

[PATCH v1 2/4] iosf_mbi: pci_dev_put() is NULL-proof

[Andy Shevchenko]

No need to check for NULL when call pci_dev_put().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/iosf_mbi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/iosf_mbi.c b/arch/x86/kernel/iosf_mbi.c
index 2362da9..0b33105 100644
--- a/arch/x86/kernel/iosf_mbi.c
+++ b/arch/x86/kernel/iosf_mbi.c
@@ -314,10 +314,8 @@ static void __exit iosf_mbi_exit(void)
 	iosf_debugfs_remove();
 
 	pci_unregister_driver(&iosf_mbi_pci_driver);
-	if (mbi_pdev) {
-		pci_dev_put(mbi_pdev);
-		mbi_pdev = NULL;
-	}
+	pci_dev_put(mbi_pdev);
+	mbi_pdev = NULL;
 }
 
 module_init(iosf_mbi_init);
-- 
2.1.4

[PATCH v1 3/4] iosf_mbi: group global variables

[Andy Shevchenko]

While here, fix indentation in the header and comments.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/include/asm/iosf_mbi.h | 8 ++++----
 arch/x86/kernel/iosf_mbi.c      | 9 ++++-----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index 57995f0..b72ad0f 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -52,20 +52,20 @@
 
 /* Quark available units */
 #define QRK_MBI_UNIT_HBA	0x00
-#define QRK_MBI_UNIT_HB	0x03
+#define QRK_MBI_UNIT_HB		0x03
 #define QRK_MBI_UNIT_RMU	0x04
-#define QRK_MBI_UNIT_MM	0x05
+#define QRK_MBI_UNIT_MM		0x05
 #define QRK_MBI_UNIT_MMESRAM	0x05
 #define QRK_MBI_UNIT_SOC	0x31
 
 /* Quark read/write opcodes */
 #define QRK_MBI_HBA_READ	0x10
 #define QRK_MBI_HBA_WRITE	0x11
-#define QRK_MBI_HB_READ	0x10
+#define QRK_MBI_HB_READ		0x10
 #define QRK_MBI_HB_WRITE	0x11
 #define QRK_MBI_RMU_READ	0x10
 #define QRK_MBI_RMU_WRITE	0x11
-#define QRK_MBI_MM_READ	0x10
+#define QRK_MBI_MM_READ		0x10
 #define QRK_MBI_MM_WRITE	0x11
 #define QRK_MBI_MMESRAM_READ	0x12
 #define QRK_MBI_MMESRAM_WRITE	0x13
diff --git a/arch/x86/kernel/iosf_mbi.c b/arch/x86/kernel/iosf_mbi.c
index 0b33105..79bd5fc 100644
--- a/arch/x86/kernel/iosf_mbi.c
+++ b/arch/x86/kernel/iosf_mbi.c
@@ -31,6 +31,7 @@
 #define PCI_DEVICE_ID_BRASWELL		0x2280
 #define PCI_DEVICE_ID_QUARK_X1000	0x0958
 
+static struct pci_dev *mbi_pdev;	/* one mbi device */
 static DEFINE_SPINLOCK(iosf_mbi_lock);
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
@@ -38,8 +39,6 @@ static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 	return (op << 24) | (port << 16) | (offset << 8) | MBI_ENABLE;
 }
 
-static struct pci_dev *mbi_pdev;	/* one mbi device */
-
 static int iosf_mbi_pci_read_mdr(u32 mcrx, u32 mcr, u32 *mdr)
 {
 	int result;
@@ -104,7 +103,7 @@ int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
 	unsigned long flags;
 	int ret;
 
-	/*Access to the GFX unit is handled by GPU code */
+	/* Access to the GFX unit is handled by GPU code */
 	if (port == BT_MBI_UNIT_GFX) {
 		WARN_ON(1);
 		return -EPERM;
@@ -127,7 +126,7 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr)
 	unsigned long flags;
 	int ret;
 
-	/*Access to the GFX unit is handled by GPU code */
+	/* Access to the GFX unit is handled by GPU code */
 	if (port == BT_MBI_UNIT_GFX) {
 		WARN_ON(1);
 		return -EPERM;
@@ -151,7 +150,7 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
 	unsigned long flags;
 	int ret;
 
-	/*Access to the GFX unit is handled by GPU code */
+	/* Access to the GFX unit is handled by GPU code */
 	if (port == BT_MBI_UNIT_GFX) {
 		WARN_ON(1);
 		return -EPERM;
-- 
2.1.4

[PATCH v1 4/4] iosf_mbi: append Intel Tangier ID

[Andy Shevchenko]

Intel Tangier has an IOSF Mailbox with PCI ID 8086:1170.

While here, keep list ordered.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/iosf_mbi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/iosf_mbi.c b/arch/x86/kernel/iosf_mbi.c
index 79bd5fc..b086501 100644
--- a/arch/x86/kernel/iosf_mbi.c
+++ b/arch/x86/kernel/iosf_mbi.c
@@ -27,9 +27,10 @@
 
 #include <asm/iosf_mbi.h>
 
+#define PCI_DEVICE_ID_QUARK_X1000	0x0958
 #define PCI_DEVICE_ID_BAYTRAIL		0x0F00
+#define PCI_DEVICE_ID_TANGIER		0x1170
 #define PCI_DEVICE_ID_BRASWELL		0x2280
-#define PCI_DEVICE_ID_QUARK_X1000	0x0958
 
 static struct pci_dev *mbi_pdev;	/* one mbi device */
 static DEFINE_SPINLOCK(iosf_mbi_lock);
@@ -288,9 +289,10 @@ static int iosf_mbi_probe(struct pci_dev *pdev,
 }
 
 static const struct pci_device_id iosf_mbi_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, iosf_mbi_pci_ids);
-- 
2.1.4

Re: [PATCH v1 0/4] iosf_mbi: clean up

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> There are few patches to clean up (with small fixes) iosf_mbi driver along with
> extension it to cover Intel Tangier.
> 
> Andy Shevchenko (4):
>   iosf_mbi: check result for all calls of debugfs API
>   iosf_mbi: pci_dev_put() is NULL-proof
>   iosf_mbi: group global variables
>   iosf_mbi: append Intel Tangier ID
> 
>  arch/x86/include/asm/iosf_mbi.h |  8 ++++----
>  arch/x86/kernel/iosf_mbi.c      | 31 +++++++++++++++----------------
>  2 files changed, 19 insertions(+), 20 deletions(-)

Could you please also move it from arch/x86/kernel/ to 
arch/x86/platform/[intel-quark?] while at it?

arch/x86/kernel/ is the wrong place for this driver.

Thanks,

	Ingo

Re: [PATCH v1 1/4] iosf_mbi: check result for all calls of debugfs API

[Thomas Gleixner]

On Wed, 8 Jul 2015, Andy Shevchenko wrote:

> While here, replase IS_ERR_OR_NULL(x) to just !x since first one does the job
> for us.

While here? The first one? What? Can you please provide a sensible changelog?

Thanks,

	tglx

Re: [PATCH v1 0/4] iosf_mbi: clean up

[Thomas Gleixner]

On Wed, 8 Jul 2015, Ingo Molnar wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > There are few patches to clean up (with small fixes) iosf_mbi driver along with
> > extension it to cover Intel Tangier.
> > 
> > Andy Shevchenko (4):
> >   iosf_mbi: check result for all calls of debugfs API
> >   iosf_mbi: pci_dev_put() is NULL-proof
> >   iosf_mbi: group global variables
> >   iosf_mbi: append Intel Tangier ID
> > 
> >  arch/x86/include/asm/iosf_mbi.h |  8 ++++----
> >  arch/x86/kernel/iosf_mbi.c      | 31 +++++++++++++++----------------
> >  2 files changed, 19 insertions(+), 20 deletions(-)
> 
> Could you please also move it from arch/x86/kernel/ to 
> arch/x86/platform/[intel-quark?] while at it?
> 
> arch/x86/kernel/ is the wrong place for this driver.

Aside of that the patch prefixes should be:

      x86/platform/iosf_mbi:

or something like that.

Thanks,

	tglx

Re: [PATCH v1 0/4] iosf_mbi: clean up

[Andy Shevchenko]

On Wed, 2015-07-08 at 11:36 +0200, Ingo Molnar wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > There are few patches to clean up (with small fixes) iosf_mbi 
> > driver along with
> > extension it to cover Intel Tangier.
> > 
> > Andy Shevchenko (4):
> >   iosf_mbi: check result for all calls of debugfs API
> >   iosf_mbi: pci_dev_put() is NULL-proof
> >   iosf_mbi: group global variables
> >   iosf_mbi: append Intel Tangier ID
> > 
> >  arch/x86/include/asm/iosf_mbi.h |  8 ++++----
> >  arch/x86/kernel/iosf_mbi.c      | 31 +++++++++++++++--------------
> > --
> >  2 files changed, 19 insertions(+), 20 deletions(-)
> 
> Could you please also move it from arch/x86/kernel/ to 
> arch/x86/platform/[intel-quark?] while at it?

I will think about proper folder (apparently not quark, since it is
specific to man different Intel SoCs).

> 
> arch/x86/kernel/ is the wrong place for this driver.

Would be okay to send an additional patch or better to regenerate the
whole series?


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

Re: [PATCH v1 1/4] iosf_mbi: check result for all calls of debugfs API

[Andy Shevchenko]

On Wed, 2015-07-08 at 11:49 +0200, Thomas Gleixner wrote:
> On Wed, 8 Jul 2015, Andy Shevchenko wrote:
> 
> > While here, replase IS_ERR_OR_NULL(x) to just !x since first one 
> does the job
> > for us.
> 
> While here? The first one? What? Can you please provide a sensible 
> changelog?

It's a continuation of the subject line. I will amend the commit
message.

> 
> Thanks,
> 
>         tglx

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

Re: [PATCH v1 0/4] iosf_mbi: clean up

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, 2015-07-08 at 11:36 +0200, Ingo Molnar wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > There are few patches to clean up (with small fixes) iosf_mbi 
> > > driver along with
> > > extension it to cover Intel Tangier.
> > > 
> > > Andy Shevchenko (4):
> > >   iosf_mbi: check result for all calls of debugfs API
> > >   iosf_mbi: pci_dev_put() is NULL-proof
> > >   iosf_mbi: group global variables
> > >   iosf_mbi: append Intel Tangier ID
> > > 
> > >  arch/x86/include/asm/iosf_mbi.h |  8 ++++----
> > >  arch/x86/kernel/iosf_mbi.c      | 31 +++++++++++++++--------------
> > > --
> > >  2 files changed, 19 insertions(+), 20 deletions(-)
> > 
> > Could you please also move it from arch/x86/kernel/ to 
> > arch/x86/platform/[intel-quark?] while at it?
> 
> I will think about proper folder (apparently not quark, since it is
> specific to man different Intel SoCs).

Yeah, you could make it arch/x86/platform/iosf/, and the file would be 
arch/x86/platform/iosf/mbi.c? Or something like that?

> > 
> > arch/x86/kernel/ is the wrong place for this driver.
> 
> Would be okay to send an additional patch or better to regenerate the whole 
> series?

So it might be better to first do the file movement, then the fixes. It should be 
relatively easy to regenerate the patches like that, and if any failure is 
bisected back to a commit it's easy to revert - or if some new review observation 
comes in then it will be easy to adjust the series.

Thanks,

	Ingo