[PATCHv5 0/2] Enable Quark X1000 support in gpio-sch

[PATCHv5 0/2] Enable Quark X1000 support in gpio-sch

[Chang Rebecca Swee Fun]

Hi all,

This is a revised version for enabling Quark X1000 support in gpio-sch.
This version of patch series had changed according to the feedback provided
by Alexandre and Linus.

Change log for V5:
Patch 1:
- Change variable curr_dirs to reg_val in order to make driver code easier to
  understand.

Patch 3:
- Dropped patch 3 for now. We need to re-design the driver's IRQ implementation.

The patches need to be patched on top of Mika Westerberg's commit at:
gpio: sch: Consolidate core and resume banks
http://marc.info/?l=linux-kernel&m=141392647225885&w=2

The patches has been verifed and tested working on Galileo Board. GPIO sysfs
was able to export gpio pins and changing pin direction. GPIO values were
able to controlled. One of the GPIO pins which is connected to on-board LED
was used to test GPIO functionality. We are able to turn the LED on/off by
changing the pin direction and pin value.

Please help to review the patches once again and thanks for all the review
comments. Your comments are valuable to me and help me to gain more in this
Open Source community as I'm quite a newbie in this area. Thanks for your
patience.

Regards,
Rebecca

Change log for V4:
Patch 1:
- Removed redundant/duplicated functions of sch_gpio_register_set() and
  sch_gpio_register_clear(). The function call had been replaced by
  sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and
  sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear().
- Resolved double spinlock issue caught by Alexandre.

Patch 3:
- Dropped the usage of "if" block that checking irq_data struct
- Restructured the irq detect by using platform_get_irq(pdev, 0) instead of
  platform_get_resource(pdev, IORESOURCE_IRQ, 0) to get IRQ resources from
  MFD LPS-SCH.

Change log for V3:
Patch 3:
- Change variable type of irq_support to bool.
- Update error message and remove redundant code.
- Revert gpiochip_remove() to avoid it to return a value.

Change log for V2:
Patch 1:
- Move sch_gpio_get() and sch_gpio_set() to avoid forward declaration.
- Changed sch_gpio_enable()/sch_gpio_disable() to sch_gpio_register_set()/
  sch_gpio_register_clear().

Patch 3:
- Changed all sch_gpio_enable()/sch_gpio_disable() to sch_gpio_register_set()/
  sch_gpio_register_clear().

Version 1:
Initial version.

Chang Rebecca Swee Fun (2):
  gpio: sch: Consolidate similar algorithms
  gpio: sch: Add support for Intel Quark X1000 SoC

 drivers/gpio/Kconfig    | 11 +++++--
 drivers/gpio/gpio-sch.c | 87 +++++++++++++++++++------------------------------
 2 files changed, 43 insertions(+), 55 deletions(-)

-- 
1.9.1

[PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

[Chang Rebecca Swee Fun]

Consolidating similar algorithms into common functions to make
GPIO SCH simpler and manageable.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpio-sch.c | 81 +++++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 53 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 99720c8..054a8ea 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -63,75 +63,59 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
 	return gpio % 8;
 }
 
-static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
+static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
 {
+	struct sch_gpio *sch = to_sch_gpio(gc);
 	unsigned short offset, bit;
-	u8 enable;
-
-	spin_lock(&sch->lock);
+	u8 reg_val;
 
-	offset = sch_gpio_offset(sch, gpio, GEN);
+	offset = sch_gpio_offset(sch, gpio, reg);
 	bit = sch_gpio_bit(sch, gpio);
 
-	enable = inb(sch->iobase + offset);
-	if (!(enable & (1 << bit)))
-		outb(enable | (1 << bit), sch->iobase + offset);
+	reg_val = !!(inb(sch->iobase + offset) & BIT(bit));
 
-	spin_unlock(&sch->lock);
+	return reg_val;
 }
 
-static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
+static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
+			     int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_dirs;
 	unsigned short offset, bit;
+	u8 reg_val;
 
-	spin_lock(&sch->lock);
+	offset = sch_gpio_offset(sch, gpio, reg);
+	bit = sch_gpio_bit(sch, gpio);
 
-	offset = sch_gpio_offset(sch, gpio_num, GIO);
-	bit = sch_gpio_bit(sch, gpio_num);
+	reg_val = inb(sch->iobase + offset);
 
-	curr_dirs = inb(sch->iobase + offset);
+	if (val)
+		outb(reg_val | BIT(bit), sch->iobase + offset);
+	else
+		outb((reg_val & ~BIT(bit)), sch->iobase + offset);
+}
 
-	if (!(curr_dirs & (1 << bit)))
-		outb(curr_dirs | (1 << bit), sch->iobase + offset);
+static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
+{
+	struct sch_gpio *sch = to_sch_gpio(gc);
 
+	spin_lock(&sch->lock);
+	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
 	spin_unlock(&sch->lock);
 	return 0;
 }
 
 static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
 {
-	struct sch_gpio *sch = to_sch_gpio(gc);
-	int res;
-	unsigned short offset, bit;
-
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	res = !!(inb(sch->iobase + offset) & (1 << bit));
-
-	return res;
+	return sch_gpio_reg_get(gc, gpio_num, GLV);
 }
 
 static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_vals;
-	unsigned short offset, bit;
 
 	spin_lock(&sch->lock);
-
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	curr_vals = inb(sch->iobase + offset);
-
-	if (val)
-		outb(curr_vals | (1 << bit), sch->iobase + offset);
-	else
-		outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
-
+	sch_gpio_reg_set(gc, gpio_num, GLV, val);
 	spin_unlock(&sch->lock);
 }
 
@@ -139,18 +123,9 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 				  int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_dirs;
-	unsigned short offset, bit;
 
 	spin_lock(&sch->lock);
-
-	offset = sch_gpio_offset(sch, gpio_num, GIO);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	curr_dirs = inb(sch->iobase + offset);
-	if (curr_dirs & (1 << bit))
-		outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
-
+	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
 	spin_unlock(&sch->lock);
 
 	/*
@@ -209,13 +184,13 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		 * GPIO7 is configured by the CMC as SLPIOVR
 		 * Enable GPIO[9:8] core powered gpios explicitly
 		 */
-		sch_gpio_enable(sch, 8);
-		sch_gpio_enable(sch, 9);
+		sch_gpio_reg_set(sch, 8, GEN, 1);
+		sch_gpio_reg_set(sch, 9, GEN, 1);
 		/*
 		 * SUS_GPIO[2:0] enabled by default
 		 * Enable SUS_GPIO3 resume powered gpio explicitly
 		 */
-		sch_gpio_enable(sch, 13);
+		sch_gpio_reg_set(sch, 13, GEN, 1);
 		break;
 
 	case PCI_DEVICE_ID_INTEL_ITC_LPC:
-- 
1.9.1

Re: [PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

[Linus Walleij]

On Mon, Dec 8, 2014 at 10:38 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:

> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

[Linus Walleij]

On Mon, Dec 8, 2014 at 10:38 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:

> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

I have removed this patch from the tree. It breaks completely
in build and looks strange:

> +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
> +                            int val)

Takes struct gpio_chip * as argument...

> +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +       struct sch_gpio *sch = to_sch_gpio(gc);
>
> +       spin_lock(&sch->lock);
> +       sch_gpio_reg_set(sch, gpio_num, GIO, 1);

Passes something else as argument.

>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_vals;
> -       unsigned short offset, bit;
>
>         spin_lock(&sch->lock);
> -
> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> -       bit = sch_gpio_bit(sch, gpio_num);
> -
> -       curr_vals = inb(sch->iobase + offset);
> -
> -       if (val)
> -               outb(curr_vals | (1 << bit), sch->iobase + offset);
> -       else
> -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
> -
> +       sch_gpio_reg_set(gc, gpio_num, GLV, val);

Here it is correct.

> @@ -139,18 +123,9 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>                                   int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_dirs;
> -       unsigned short offset, bit;
>
>         spin_lock(&sch->lock);
> -
> -       offset = sch_gpio_offset(sch, gpio_num, GIO);
> -       bit = sch_gpio_bit(sch, gpio_num);
> -
> -       curr_dirs = inb(sch->iobase + offset);
> -       if (curr_dirs & (1 << bit))
> -               outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
> -
> +       sch_gpio_reg_set(sch, gpio_num, GIO, 0);

Wrong again. Etc.

Makes me suspect that this patch is not tested at all, so dropped.

Yours,
Linus Walleij

Re: [PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

[Alexandre Courbot]

On Thu, Jan 15, 2015 at 6:49 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Dec 8, 2014 at 10:38 AM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
>
>> Consolidating similar algorithms into common functions to make
>> GPIO SCH simpler and manageable.
>>
>> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>
> I have removed this patch from the tree. It breaks completely
> in build and looks strange:
>
>> +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
>> +                            int val)
>
> Takes struct gpio_chip * as argument...
>
>> +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>> +{
>> +       struct sch_gpio *sch = to_sch_gpio(gc);
>>
>> +       spin_lock(&sch->lock);
>> +       sch_gpio_reg_set(sch, gpio_num, GIO, 1);
>
> Passes something else as argument.
>
>>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>>  {
>>         struct sch_gpio *sch = to_sch_gpio(gc);
>> -       u8 curr_vals;
>> -       unsigned short offset, bit;
>>
>>         spin_lock(&sch->lock);
>> -
>> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
>> -       bit = sch_gpio_bit(sch, gpio_num);
>> -
>> -       curr_vals = inb(sch->iobase + offset);
>> -
>> -       if (val)
>> -               outb(curr_vals | (1 << bit), sch->iobase + offset);
>> -       else
>> -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
>> -
>> +       sch_gpio_reg_set(gc, gpio_num, GLV, val);
>
> Here it is correct.
>
>> @@ -139,18 +123,9 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>>                                   int val)
>>  {
>>         struct sch_gpio *sch = to_sch_gpio(gc);
>> -       u8 curr_dirs;
>> -       unsigned short offset, bit;
>>
>>         spin_lock(&sch->lock);
>> -
>> -       offset = sch_gpio_offset(sch, gpio_num, GIO);
>> -       bit = sch_gpio_bit(sch, gpio_num);
>> -
>> -       curr_dirs = inb(sch->iobase + offset);
>> -       if (curr_dirs & (1 << bit))
>> -               outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
>> -
>> +       sch_gpio_reg_set(sch, gpio_num, GIO, 0);
>
> Wrong again. Etc.
>
> Makes me suspect that this patch is not tested at all, so dropped.

IIRC at least one of the previous versions had the same issue and at
that time I already pointed out that *compiling and testing* patches
before sending them in the wild is common sense.

Ironically the cover letter says "The patches has been verifed and
tested working on Galileo Board". One may wonder...

[PATCHv5 2/2] gpio: sch: Add support for Intel Quark X1000 SoC

[Chang Rebecca Swee Fun]

Intel Quark X1000 provides a total of 16 GPIOs. The GPIOs are split between
the legacy I/O bridge and the GPIO controller.

GPIO-SCH is the GPIO pins on legacy bridge for Intel Quark SoC.
Intel Quark X1000 has 2 GPIOs powered by the core power well and 6 from
the suspend power well.

This piece of work is derived from Dan O'Donovan's initial work for Quark
X1000 enabling.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/Kconfig    | 11 +++++++++--
 drivers/gpio/gpio-sch.c |  6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 414d055..24c4f83 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -394,25 +394,32 @@ config GPIO_VR41XX
 	  Say yes here to support the NEC VR4100 series General-purpose I/O Uint
 
 config GPIO_SCH
-	tristate "Intel SCH/TunnelCreek/Centerton GPIO"
+	tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
 	depends on PCI && X86
 	select MFD_CORE
 	select LPC_SCH
 	help
 	  Say yes here to support GPIO interface on Intel Poulsbo SCH,
-	  Intel Tunnel Creek processor or Intel Centerton processor.
+	  Intel Tunnel Creek processor, Intel Centerton processor or
+	  Intel Quark X1000 SoC.
+
 	  The Intel SCH contains a total of 14 GPIO pins. Ten GPIOs are
 	  powered by the core power rail and are turned off during sleep
 	  modes (S3 and higher). The remaining four GPIOs are powered by
 	  the Intel SCH suspend power supply. These GPIOs remain
 	  active during S3. The suspend powered GPIOs can be used to wake the
 	  system from the Suspend-to-RAM state.
+
 	  The Intel Tunnel Creek processor has 5 GPIOs powered by the
 	  core power rail and 9 from suspend power supply.
+
 	  The Intel Centerton processor has a total of 30 GPIO pins.
 	  Twenty-one are powered by the core power rail and 9 from the
 	  suspend power supply.
 
+	  The Intel Quark X1000 SoC has 2 GPIOs powered by the core
+	  power well and 6 from the suspend power well.
+
 config GPIO_ICH
 	tristate "Intel ICH GPIO"
 	depends on PCI && X86
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 054a8ea..1495105 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -205,6 +205,12 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		sch->chip.ngpio = 30;
 		break;
 
+	case PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB:
+		sch->core_base = 0;
+		sch->resume_base = 2;
+		sch->chip.ngpio = 8;
+		break;
+
 	default:
 		return -ENODEV;
 	}
-- 
1.9.1

Re: [PATCHv5 2/2] gpio: sch: Add support for Intel Quark X1000 SoC

[Linus Walleij]

On Mon, Dec 8, 2014 at 10:38 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:

> Intel Quark X1000 provides a total of 16 GPIOs. The GPIOs are split between
> the legacy I/O bridge and the GPIO controller.
>
> GPIO-SCH is the GPIO pins on legacy bridge for Intel Quark SoC.
> Intel Quark X1000 has 2 GPIOs powered by the core power well and 6 from
> the suspend power well.
>
> This piece of work is derived from Dan O'Donovan's initial work for Quark
> X1000 enabling.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCHv5 0/2] Enable Quark X1000 support in gpio-sch

[Alexandre Courbot]

On Mon, Dec 8, 2014 at 6:38 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> Hi all,
>
> This is a revised version for enabling Quark X1000 support in gpio-sch.
> This version of patch series had changed according to the feedback provided
> by Alexandre and Linus.
>
> Change log for V5:
> Patch 1:
> - Change variable curr_dirs to reg_val in order to make driver code easier to
>   understand.
>
> Patch 3:
> - Dropped patch 3 for now. We need to re-design the driver's IRQ implementation.
>
> The patches need to be patched on top of Mika Westerberg's commit at:
> gpio: sch: Consolidate core and resume banks
> http://marc.info/?l=linux-kernel&m=141392647225885&w=2
>
> The patches has been verifed and tested working on Galileo Board. GPIO sysfs
> was able to export gpio pins and changing pin direction. GPIO values were
> able to controlled. One of the GPIO pins which is connected to on-board LED
> was used to test GPIO functionality. We are able to turn the LED on/off by
> changing the pin direction and pin value.

I think these patches are good to go, if Linus is ok with it. Nice job
making this driver simpler and more legible. Looking forward to seeing
the IRQ implementation!