[PATCH v1 1/2] gpio: intel-mid: Remove potentially harmful code

[PATCH v1 1/2] gpio: intel-mid: Remove potentially harmful code

[Andy Shevchenko]

The commit d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
doesn't look at all as a proper support for Intel Merrifield and I dare to say
that it distorts the behaviour of the hardware.

The register map is different on Intel Merrifield, i.e. only 6 out of 8
register have the same purpose but none of them has same location in the
address space. The current case potentially harmful to existing hardware since
it's poking registers on wrong offsets and may set some pin to be GPIO output
when connected hardware doesn't expect such.

Besides the above GPIO and pinctrl on Intel Merrifield have been located in
different IP blocks. The functionality has been extended as well, i.e. added
support of level interrupts, special registers for wake capable sources and
thus, in my opinion, requires a completele separate driver.

If someone wondering the existing gpio-intel-mid.c would be converted to actual
pinctrl (which by the fact it is now), though I wouldn't be a volunteer to do
that.

Fixes: d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
Cc: stable@vger.kernel.org # v3.13+
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-intel-mid.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
index a99cf4b..c0f7cce 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -17,7 +17,6 @@
  * Moorestown platform Langwell chip.
  * Medfield platform Penwell chip.
  * Clovertrail platform Cloverview chip.
- * Merrifield platform Tangier chip.
  */
 
 #include <linux/module.h>
@@ -64,10 +63,6 @@ enum GPIO_REG {
 /* intel_mid gpio driver data */
 struct intel_mid_gpio_ddata {
 	u16 ngpio;		/* number of gpio pins */
-	u32 gplr_offset;	/* offset of first GPLR register from base */
-	u32 flis_base;		/* base address of FLIS registers */
-	u32 flis_len;		/* length of FLIS registers */
-	u32 (*get_flis_offset)(int gpio);
 	u32 chip_irq_type;	/* chip interrupt type */
 };
 
@@ -252,15 +247,6 @@ static const struct intel_mid_gpio_ddata gpio_cloverview_core = {
 	.chip_irq_type = INTEL_MID_IRQ_TYPE_EDGE,
 };
 
-static const struct intel_mid_gpio_ddata gpio_tangier = {
-	.ngpio = 192,
-	.gplr_offset = 4,
-	.flis_base = 0xff0c0000,
-	.flis_len = 0x8000,
-	.get_flis_offset = NULL,
-	.chip_irq_type = INTEL_MID_IRQ_TYPE_EDGE,
-};
-
 static const struct pci_device_id intel_gpio_ids[] = {
 	{
 		/* Lincroft */
@@ -287,11 +273,6 @@ static const struct pci_device_id intel_gpio_ids[] = {
 		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x08f7),
 		.driver_data = (kernel_ulong_t)&gpio_cloverview_core,
 	},
-	{
-		/* Tangier */
-		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x1199),
-		.driver_data = (kernel_ulong_t)&gpio_tangier,
-	},
 	{ 0 }
 };
 MODULE_DEVICE_TABLE(pci, intel_gpio_ids);
-- 
2.8.1

[PATCH v1 2/2] gpio: intel-mid: Sort header block alphabetically

[Andy Shevchenko]

Sort the header inclusion lines by alphabetical order.

While here, update Intel Copyright.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-intel-mid.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
index c0f7cce..164de64 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -1,7 +1,7 @@
 /*
  * Intel MID GPIO driver
  *
- * Copyright (c) 2008-2014 Intel Corporation.
+ * Copyright (c) 2008-2014,2016 Intel Corporation.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -19,18 +19,18 @@
  * Clovertrail platform Cloverview chip.
  */
 
-#include <linux/module.h>
-#include <linux/pci.h>
-#include <linux/platform_device.h>
-#include <linux/kernel.h>
 #include <linux/delay.h>
-#include <linux/stddef.h>
-#include <linux/interrupt.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/gpio/driver.h>
-#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/stddef.h>
 
 #define INTEL_MID_IRQ_TYPE_EDGE		(1 << 0)
 #define INTEL_MID_IRQ_TYPE_LEVEL	(1 << 1)
-- 
2.8.1

Re: [PATCH v1 2/2] gpio: intel-mid: Sort header block alphabetically

[Mika Westerberg]

On Wed, Jul 06, 2016 at 12:50:13PM +0300, Andy Shevchenko wrote:
> Sort the header inclusion lines by alphabetical order.
> 
> While here, update Intel Copyright.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I don't see much point in patches like this but it does no harm so,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 2/2] gpio: intel-mid: Sort header block alphabetically

[Andy Shevchenko]

On Fri, 2016-07-08 at 11:22 +0300, Mika Westerberg wrote:
> On Wed, Jul 06, 2016 at 12:50:13PM +0300, Andy Shevchenko wrote:
> > Sort the header inclusion lines by alphabetical order.
> > 
> > While here, update Intel Copyright.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I don't see much point in patches like this but it does no harm so,

Consider this one is a part of a bundle. Otherwise I agree with you.

> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

-- 

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

Re: [PATCH v1 2/2] gpio: intel-mid: Sort header block alphabetically

[Linus Walleij]

On Wed, Jul 6, 2016 at 11:50 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Sort the header inclusion lines by alphabetical order.
>
> While here, update Intel Copyright.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's review tag.

Yours,
Linus Walleij

Re: [PATCH v1 1/2] gpio: intel-mid: Remove potentially harmful code

[Mika Westerberg]

On Wed, Jul 06, 2016 at 12:50:12PM +0300, Andy Shevchenko wrote:
> The commit d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
> doesn't look at all as a proper support for Intel Merrifield and I dare to say
> that it distorts the behaviour of the hardware.
> 
> The register map is different on Intel Merrifield, i.e. only 6 out of 8
> register have the same purpose but none of them has same location in the
> address space. The current case potentially harmful to existing hardware since
> it's poking registers on wrong offsets and may set some pin to be GPIO output
> when connected hardware doesn't expect such.
> 
> Besides the above GPIO and pinctrl on Intel Merrifield have been located in
> different IP blocks. The functionality has been extended as well, i.e. added
> support of level interrupts, special registers for wake capable sources and
> thus, in my opinion, requires a completele separate driver.
> 
> If someone wondering the existing gpio-intel-mid.c would be converted to actual
> pinctrl (which by the fact it is now), though I wouldn't be a volunteer to do
> that.
> 
> Fixes: d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
> Cc: stable@vger.kernel.org # v3.13+
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

If it is truely poking wrong registers, this is certainly right thing to
do.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I take it we are getting a driver which is using the correct registers
soon ;-)

Re: [PATCH v1 1/2] gpio: intel-mid: Remove potentially harmful code

[Andy Shevchenko]

On Fri, 2016-07-08 at 11:20 +0300, Mika Westerberg wrote:
> On Wed, Jul 06, 2016 at 12:50:12PM +0300, Andy Shevchenko wrote:
> > The commit d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield
> > support")
> > doesn't look at all as a proper support for Intel Merrifield and I
> > dare to say
> > that it distorts the behaviour of the hardware.
> > 
> > The register map is different on Intel Merrifield, i.e. only 6 out
> > of 8
> > register have the same purpose but none of them has same location in
> > the
> > address space. The current case potentially harmful to existing
> > hardware since
> > it's poking registers on wrong offsets and may set some pin to be
> > GPIO output
> > when connected hardware doesn't expect such.
> > 
> > Besides the above GPIO and pinctrl on Intel Merrifield have been
> > located in
> > different IP blocks. The functionality has been extended as well,
> > i.e. added
> > support of level interrupts, special registers for wake capable
> > sources and
> > thus, in my opinion, requires a completele separate driver.
> > 
> > 

> If it is truely poking wrong registers, this is certainly right thing
> to
> do.

Yes, you might check it yourself, GPIO registers starts with offset 4,
instead of 0, and there are couple of w/o registers in the area to set
pin state. It means potentially you may find the case where suddenly one
pin not that one user asked for) might change direction followed by
voltage.

> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I take it we are getting a driver which is using the correct registers
> soon ;-)

It should be in your mailbox already.


-- 

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

Re: [PATCH v1 1/2] gpio: intel-mid: Remove potentially harmful code

[Linus Walleij]

On Wed, Jul 6, 2016 at 11:50 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The commit d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
> doesn't look at all as a proper support for Intel Merrifield and I dare to say
> that it distorts the behaviour of the hardware.
>
> The register map is different on Intel Merrifield, i.e. only 6 out of 8
> register have the same purpose but none of them has same location in the
> address space. The current case potentially harmful to existing hardware since
> it's poking registers on wrong offsets and may set some pin to be GPIO output
> when connected hardware doesn't expect such.
>
> Besides the above GPIO and pinctrl on Intel Merrifield have been located in
> different IP blocks. The functionality has been extended as well, i.e. added
> support of level interrupts, special registers for wake capable sources and
> thus, in my opinion, requires a completele separate driver.
>
> If someone wondering the existing gpio-intel-mid.c would be converted to actual
> pinctrl (which by the fact it is now), though I wouldn't be a volunteer to do
> that.
>
> Fixes: d56d6b3d7d69 ("gpio: langwell: add Intel Merrifield support")
> Cc: stable@vger.kernel.org # v3.13+
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's review tag.

Yours,
Linus Walleij