Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Darren Hart]

On 9/9/14, 11:07, "Weike Chen" <alvin.chen@intel.com> wrote:

>The Synopsys DesignWare APB GPIO driver only supports open firmware
>devices.
>But, like Intel Quark X1000 SOC, which has a single PCI function exporting
>a GPIO and an I2C controller, it is a Multifunction device. This patch is
>to enable the current Synopsys DesignWare APB GPIO driver to support the
>Multifunction device which exports the designware GPIO controller.
>
>Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com>
>Signed-off-by: Weike Chen <alvin.chen@intel.com>
>---
> drivers/gpio/Kconfig                     |    1 -
> drivers/gpio/gpio-dwapb.c                |  226
>++++++++++++++++++++++--------
> include/linux/platform_data/gpio-dwapb.h |   32 +++++
> 3 files changed, 201 insertions(+), 58 deletions(-)
> create mode 100644 include/linux/platform_data/gpio-dwapb.h
>
>diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>index 9de1515..8250a44 100644
>--- a/drivers/gpio/Kconfig
>+++ b/drivers/gpio/Kconfig
>@@ -136,7 +136,6 @@ config GPIO_DWAPB
> 	tristate "Synopsys DesignWare APB GPIO driver"
> 	select GPIO_GENERIC
> 	select GENERIC_IRQ_CHIP
>-	depends on OF_GPIO

You cover this specific dependencies with inline ifdefs, but you lose the
CONFIG_OF depends by dropping it, and there are no such checks in the
probe routine. Assumptions of OF are not limited to probe in this driver.

While I would like to see this assumption properly abstracted, the most
expedient/immediate fix is probably to add a depends on OF above.

--
Darren
Intel Open Source Technology Center

[PATCH 0/4 v3] The Designware GPIO Supporting

[Weike Chen]

Hi,
These patches are for Intel Quark X1000 designware GPIO supporting. The first
patch enables the Synopsys DesignWare APB GPIO driver to support the MFD device.
And the Quark designware GPIO controller is registered as MFD device,
because Quark exports a single PCI device with both GPIO and I2C functions.
It is about reviewing the GPIO changes in gpio-dwapb, and in near future,
the Quark I2C driver and the MFD driver that binds these GPIO & I2C
functions will be sent subsequently. The second patch replaces all
'readl&writel' with 'dwapb_read'&'dwapb_write. The third patch enables the gpio
'debounce' feature. And the fourth patch enables the power management.

---
v3:
 Split [PATCH 2/3] into two patches [PATCH 2/4] and [PATCH 3/4],
 and now inlucde 4 patches.

 [PATCH 1/4]
 * Use 'is_pdata_alloc' instead of 'is_of'.
 * Allocate 'pdata' by 'kmalloc/kfree' instead of 'devm_*' for OF.
 * Use 'IS_ENABLED(CONFIG_OF_GPIO)' instead of '#ifdef CONFIG_OF_GPIO'.
 * A couple of other minor changes.
 [PATCH 2/4]
 * New patch splitted from the original [PATCH 2/3]
 * Use 'dwapb_read/write' instead of readl&writel.
 [PATCH 3/4]
 * Move 'dwapb_read/write' defination to new patch [PATCH 2/4].
 * Only port A support 'debounce' now.
 [PATCH 4/4]
 * The original patch [PATCH 3/3]
 * Use 'struct dwapb_context' instead of 'struct gpio_saved_regs'.
 * Allocate and save context dynamically by per port.

v2:
 [PATCH 1/3]
 * Fixed a bug to set the base gpio number to '-1' for the OF flow.
 * Set device node for each gpio chip for the OF flow.
 * Change the name of 'struct dwapb_gpio_platform_data' to
   'struct dwapb_platform_data'.
 * Change the name of 'struct dwapb_gpio_port_property' to
   'struct dwapb_port_property'.
 * Access pdata directly in 'probe' instead of accesing it
   by 'struct dwapb_gpio'.
 * Free 'pdata' at the end of 'probe' if it is OF case to save memory.
 * Improve the interrupt handler.
 * Move 'irq_set_handler_data' together with 'irq_set_chained_hanlder'.
 * Remove unncessary comments.
 [PATCH 2/3]
 * Change all 'readl'&'writel' to 'dwapb_read'&'dwapb_write'.
 [PATCH 3/3]
 * Change the name for 'struct gpio_saved_regs' to 'struct dwapb_context'.
 * Save the registers for all ports according to the port index.
 * Change '#if defined' to '#ifdef'.

Weike Chen (4):
  GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
  GPIO: gpio-dwapb: Change readl&writel to dwapb_read&dwapb_write
  GPIO: gpio-dwapb: Support Debounce
  GPIO: gpio-dwapb: Suspend & Resume PM enabling

 drivers/gpio/Kconfig                     |    1 -
 drivers/gpio/gpio-dwapb.c                |  400 +++++++++++++++++++++++++-----
 include/linux/platform_data/gpio-dwapb.h |   32 +++
 3 files changed, 365 insertions(+), 68 deletions(-)
 create mode 100644 include/linux/platform_data/gpio-dwapb.h

-- 
1.7.9.5

[PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Weike Chen]

The Synopsys DesignWare APB GPIO driver only supports open firmware devices.
But, like Intel Quark X1000 SOC, which has a single PCI function exporting
a GPIO and an I2C controller, it is a Multifunction device. This patch is
to enable the current Synopsys DesignWare APB GPIO driver to support the
Multifunction device which exports the designware GPIO controller.

Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/Kconfig                     |    1 -
 drivers/gpio/gpio-dwapb.c                |  226 ++++++++++++++++++++++--------
 include/linux/platform_data/gpio-dwapb.h |   32 +++++
 3 files changed, 201 insertions(+), 58 deletions(-)
 create mode 100644 include/linux/platform_data/gpio-dwapb.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..8250a44 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -136,7 +136,6 @@ config GPIO_DWAPB
 	tristate "Synopsys DesignWare APB GPIO driver"
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
-	depends on OF_GPIO
 	help
 	  Say Y or M here to build support for the Synopsys DesignWare APB
 	  GPIO block.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index d6618a6..9ddb3cc 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -21,6 +21,8 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
+#include <linux/platform_data/gpio-dwapb.h>
+#include <linux/slab.h>
 
 #define GPIO_SWPORTA_DR		0x00
 #define GPIO_SWPORTA_DDR	0x04
@@ -84,11 +86,10 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 	writel(v, gpio->regs + GPIO_INT_POLARITY);
 }
 
-static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 {
-	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
-	struct irq_chip *chip = irq_desc_get_chip(desc);
 	u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
+	u32 ret = irq_status;
 
 	while (irq_status) {
 		int hwirq = fls(irq_status) - 1;
@@ -102,6 +103,16 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
 			dwapb_toggle_trigger(gpio, hwirq);
 	}
 
+	return ret;
+}
+
+static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+{
+	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	dwapb_do_irq(gpio);
+
 	if (chip->irq_eoi)
 		chip->irq_eoi(irq_desc_get_irq_data(desc));
 }
@@ -207,22 +218,26 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 	return 0;
 }
 
+static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
+{
+	u32 worked;
+	struct dwapb_gpio *gpio = dev_id;
+
+	worked = dwapb_do_irq(gpio);
+
+	return worked ? IRQ_HANDLED : IRQ_NONE;
+}
+
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
-				 struct dwapb_gpio_port *port)
+				 struct dwapb_gpio_port *port,
+				 struct dwapb_port_property *pp)
 {
 	struct gpio_chip *gc = &port->bgc.gc;
-	struct device_node *node =  gc->of_node;
-	struct irq_chip_generic	*irq_gc;
+	struct device_node *node = pp->node;
+	struct irq_chip_generic	*irq_gc = NULL;
 	unsigned int hwirq, ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
-	int err, irq, i;
-
-	irq = irq_of_parse_and_map(node, 0);
-	if (!irq) {
-		dev_warn(gpio->dev, "no irq for bank %s\n",
-			port->bgc.gc.of_node->full_name);
-		return;
-	}
+	int err, i;
 
 	gpio->domain = irq_domain_add_linear(node, ngpio,
 					     &irq_generic_chip_ops, gpio);
@@ -269,8 +284,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
 	irq_gc->chip_types[1].handler = handle_edge_irq;
 
-	irq_set_chained_handler(irq, dwapb_irq_handler);
-	irq_set_handler_data(irq, gpio);
+	if (!pp->irq_shared) {
+		irq_set_chained_handler(pp->irq, dwapb_irq_handler);
+		irq_set_handler_data(pp->irq, gpio);
+	} else {
+		/*
+		 * Request a shared IRQ since where MFD would have devices
+		 * using the same irq pin
+		 */
+		err = devm_request_irq(gpio->dev, pp->irq,
+				       dwapb_irq_handler_mfd,
+				       IRQF_SHARED, "gpio-dwapb-mfd", gpio);
+		if (err) {
+			dev_err(gpio->dev, "error requesting IRQ\n");
+			irq_domain_remove(gpio->domain);
+			gpio->domain = NULL;
+			return;
+		}
+	}
 
 	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
 		irq_create_mapping(gpio->domain, hwirq);
@@ -296,57 +327,42 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
 }
 
 static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
-			       struct device_node *port_np,
+			       struct dwapb_port_property *pp,
 			       unsigned int offs)
 {
 	struct dwapb_gpio_port *port;
-	u32 port_idx, ngpio;
 	void __iomem *dat, *set, *dirout;
 	int err;
 
-	if (of_property_read_u32(port_np, "reg", &port_idx) ||
-		port_idx >= DWAPB_MAX_PORTS) {
-		dev_err(gpio->dev, "missing/invalid port index for %s\n",
-			port_np->full_name);
-		return -EINVAL;
-	}
-
 	port = &gpio->ports[offs];
 	port->gpio = gpio;
 
-	if (of_property_read_u32(port_np, "snps,nr-gpios", &ngpio)) {
-		dev_info(gpio->dev, "failed to get number of gpios for %s\n",
-			 port_np->full_name);
-		ngpio = 32;
-	}
-
-	dat = gpio->regs + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE);
-	set = gpio->regs + GPIO_SWPORTA_DR + (port_idx * GPIO_SWPORT_DR_SIZE);
+	dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE);
+	set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE);
 	dirout = gpio->regs + GPIO_SWPORTA_DDR +
-		(port_idx * GPIO_SWPORT_DDR_SIZE);
+		(pp->idx * GPIO_SWPORT_DDR_SIZE);
 
 	err = bgpio_init(&port->bgc, gpio->dev, 4, dat, set, NULL, dirout,
 			 NULL, false);
 	if (err) {
 		dev_err(gpio->dev, "failed to init gpio chip for %s\n",
-			port_np->full_name);
+			pp->name);
 		return err;
 	}
 
-	port->bgc.gc.ngpio = ngpio;
-	port->bgc.gc.of_node = port_np;
+#ifdef CONFIG_OF_GPIO
+	port->bgc.gc.of_node = pp->node;
+#endif
+	port->bgc.gc.ngpio = pp->ngpio;
+	port->bgc.gc.base = pp->gpio_base;
 
-	/*
-	 * Only port A can provide interrupts in all configurations of the IP.
-	 */
-	if (port_idx == 0 &&
-	    of_property_read_bool(port_np, "interrupt-controller"))
-		dwapb_configure_irqs(gpio, port);
+	if (pp->irq)
+		dwapb_configure_irqs(gpio, port, pp);
 
 	err = gpiochip_add(&port->bgc.gc);
 	if (err)
 		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
-			port_np->full_name);
+			pp->name);
 	else
 		port->is_registered = true;
 
@@ -362,25 +378,118 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 			gpiochip_remove(&gpio->ports[m].bgc.gc);
 }
 
+static struct dwapb_platform_data *
+dwapb_gpio_get_pdata_of(struct device *dev)
+{
+	struct device_node *node, *port_np;
+	struct dwapb_platform_data *pdata;
+	struct dwapb_port_property *pp;
+	int nports;
+	int i;
+
+	node = dev->of_node;
+	if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
+		return ERR_PTR(-ENODEV);
+
+	nports = of_get_child_count(node);
+	if (nports == 0)
+		return ERR_PTR(-ENODEV);
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->properties = kcalloc(nports, sizeof(*pp), GFP_KERNEL);
+	if (!pdata->properties) {
+		kfree(pdata);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	pdata->nports = nports;
+
+	i = 0;
+	for_each_child_of_node(node, port_np) {
+		pp = &pdata->properties[i++];
+		pp->node = port_np;
+
+		if (of_property_read_u32(port_np, "reg", &pp->idx) ||
+		    pp->idx >= DWAPB_MAX_PORTS) {
+			dev_err(dev, "missing/invalid port index for %s\n",
+				port_np->full_name);
+			kfree(pdata->properties);
+			kfree(pdata);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (of_property_read_u32(port_np, "snps,nr-gpios",
+					 &pp->ngpio)) {
+			dev_info(dev, "failed to get number of gpios for %s\n",
+				 port_np->full_name);
+			pp->ngpio = 32;
+		}
+
+		/*
+		 * Only port A can provide interrupts in all configurations of
+		 * the IP.
+		 */
+		if (pp->idx == 0 &&
+		    of_property_read_bool(port_np, "interrupt-controller")) {
+			pp->irq = irq_of_parse_and_map(port_np, 0);
+			if (!pp->irq) {
+				dev_warn(dev, "no irq for bank %s\n",
+					 port_np->full_name);
+			}
+		} else {
+			pp->irq	= 0;
+		}
+
+		pp->irq_shared	= false;
+		pp->gpio_base	= -1;
+		pp->name	= port_np->full_name;
+	}
+
+	return pdata;
+}
+
+static inline void dwapb_free_pdata_of(struct dwapb_platform_data *pdata)
+{
+	if (!IS_ENABLED(CONFIG_OF_GPIO) || !pdata)
+		return;
+
+	kfree(pdata->properties);
+	kfree(pdata);
+}
+
 static int dwapb_gpio_probe(struct platform_device *pdev)
 {
+	int i;
 	struct resource *res;
 	struct dwapb_gpio *gpio;
-	struct device_node *np;
 	int err;
-	unsigned int offs = 0;
+	struct device *dev = &pdev->dev;
+	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
+	bool is_pdata_alloc = !pdata;
+
+	if (is_pdata_alloc) {
+		pdata = dwapb_gpio_get_pdata_of(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
 
-	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
-	if (!gpio)
-		return -ENOMEM;
-	gpio->dev = &pdev->dev;
+	if (!pdata->nports) {
+		err = -ENODEV;
+		goto out_err;
+	}
 
-	gpio->nr_ports = of_get_child_count(pdev->dev.of_node);
-	if (!gpio->nr_ports) {
-		err = -EINVAL;
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio) {
+		err = -ENOMEM;
 		goto out_err;
 	}
-	gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
+	gpio->dev = &pdev->dev;
+	gpio->nr_ports = pdata->nports;
+
+	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
 	if (!gpio->ports) {
 		err = -ENOMEM;
@@ -394,20 +503,23 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 		goto out_err;
 	}
 
-	for_each_child_of_node(pdev->dev.of_node, np) {
-		err = dwapb_gpio_add_port(gpio, np, offs++);
+	for (i = 0; i < gpio->nr_ports; i++) {
+		err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i);
 		if (err)
 			goto out_unregister;
 	}
 	platform_set_drvdata(pdev, gpio);
 
-	return 0;
+	goto out_err;
 
 out_unregister:
 	dwapb_gpio_unregister(gpio);
 	dwapb_irq_teardown(gpio);
 
 out_err:
+	if (is_pdata_alloc)
+		dwapb_free_pdata_of(pdata);
+
 	return err;
 }
 
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
new file mode 100644
index 0000000..28702c8
--- /dev/null
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright(c) 2014 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef GPIO_DW_APB_H
+#define GPIO_DW_APB_H
+
+struct dwapb_port_property {
+	struct device_node *node;
+	const char	*name;
+	unsigned int	idx;
+	unsigned int	ngpio;
+	unsigned int	gpio_base;
+	unsigned int	irq;
+	bool		irq_shared;
+};
+
+struct dwapb_platform_data {
+	struct dwapb_port_property *properties;
+	unsigned int nports;
+};
+
+#endif
-- 
1.7.9.5

[PATCH 2/4 v3] GPIO: gpio-dwapb: Change readl&writel to dwapb_read&dwapb_write

[Weike Chen]

This patch replaces 'readl&writel' with 'dwapb_read&dwapb_write'.

Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/gpio-dwapb.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 9ddb3cc..c78a878 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -64,6 +64,23 @@ struct dwapb_gpio {
 	struct irq_domain	*domain;
 };
 
+static inline u32 dwapb_read(struct dwapb_gpio *gpio, unsigned int offset)
+{
+	struct bgpio_chip *bgc	= &gpio->ports[0].bgc;
+	void __iomem *reg_base	= gpio->regs;
+
+	return bgc->read_reg(reg_base + offset);
+}
+
+static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset,
+			       u32 val)
+{
+	struct bgpio_chip *bgc	= &gpio->ports[0].bgc;
+	void __iomem *reg_base	= gpio->regs;
+
+	bgc->write_reg(reg_base + offset, val);
+}
+
 static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
@@ -76,14 +93,14 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 
 static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 {
-	u32 v = readl(gpio->regs + GPIO_INT_POLARITY);
+	u32 v = dwapb_read(gpio, GPIO_INT_POLARITY);
 
 	if (gpio_get_value(gpio->ports[0].bgc.gc.base + offs))
 		v &= ~BIT(offs);
 	else
 		v |= BIT(offs);
 
-	writel(v, gpio->regs + GPIO_INT_POLARITY);
+	dwapb_write(gpio, GPIO_INT_POLARITY, v);
 }
 
 static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
@@ -126,9 +143,9 @@ static void dwapb_irq_enable(struct irq_data *d)
 	u32 val;
 
 	spin_lock_irqsave(&bgc->lock, flags);
-	val = readl(gpio->regs + GPIO_INTEN);
+	val = dwapb_read(gpio, GPIO_INTEN);
 	val |= BIT(d->hwirq);
-	writel(val, gpio->regs + GPIO_INTEN);
+	dwapb_write(gpio, GPIO_INTEN, val);
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
@@ -141,9 +158,9 @@ static void dwapb_irq_disable(struct irq_data *d)
 	u32 val;
 
 	spin_lock_irqsave(&bgc->lock, flags);
-	val = readl(gpio->regs + GPIO_INTEN);
+	val = dwapb_read(gpio, GPIO_INTEN);
 	val &= ~BIT(d->hwirq);
-	writel(val, gpio->regs + GPIO_INTEN);
+	dwapb_write(gpio, GPIO_INTEN, val);
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
@@ -183,8 +200,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		return -EINVAL;
 
 	spin_lock_irqsave(&bgc->lock, flags);
-	level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
-	polarity = readl(gpio->regs + GPIO_INT_POLARITY);
+	level = dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
+	polarity = dwapb_read(gpio, GPIO_INT_POLARITY);
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_BOTH:
@@ -211,8 +228,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	irq_setup_alt_chip(d, type);
 
-	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
-	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
+	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, level);
+	dwapb_write(gpio, GPIO_INT_POLARITY, polarity);
 	spin_unlock_irqrestore(&bgc->lock, flags);
 
 	return 0;
-- 
1.7.9.5

[PATCH 3/4 v3] GPIO: gpio-dwapb: Support Debounce

[Weike Chen]

This patch enables 'debounce' for the designware GPIO, and
it is based on Josef Ahmad's previous work.

Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com>
Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/gpio-dwapb.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c78a878..a90afb9 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -37,6 +37,7 @@
 #define GPIO_INTTYPE_LEVEL	0x38
 #define GPIO_INT_POLARITY	0x3c
 #define GPIO_INTSTATUS		0x40
+#define GPIO_PORTA_DEBOUNCE	0x48
 #define GPIO_PORTA_EOI		0x4c
 #define GPIO_EXT_PORTA		0x50
 #define GPIO_EXT_PORTB		0x54
@@ -235,6 +236,29 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 	return 0;
 }
 
+static int dwapb_gpio_set_debounce(struct gpio_chip *gc,
+				   unsigned offset, unsigned debounce)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	struct dwapb_gpio_port *port =
+			container_of(bgc, struct dwapb_gpio_port, bgc);
+	struct dwapb_gpio *gpio = port->gpio;
+	unsigned long flags, val_deb;
+	unsigned long mask = bgc->pin2mask(bgc, offset);
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
+	if (debounce)
+		dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb);
+	else
+		dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask & val_deb);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+
 static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
 {
 	u32 worked;
@@ -373,6 +397,10 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 	port->bgc.gc.ngpio = pp->ngpio;
 	port->bgc.gc.base = pp->gpio_base;
 
+	/* Only port A support debounce */
+	if (pp->idx == 0)
+		port->bgc.gc.set_debounce = dwapb_gpio_set_debounce;
+
 	if (pp->irq)
 		dwapb_configure_irqs(gpio, port, pp);
 
-- 
1.7.9.5

[PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Weike Chen]

This patch enables suspend and resume mode for the power management, and
it is based on Josef Ahmad's previous work.

Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/gpio-dwapb.c |  109 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index a90afb9..9da2ba7 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -50,11 +50,14 @@
 #define GPIO_SWPORT_DDR_SIZE	(GPIO_SWPORTB_DDR - GPIO_SWPORTA_DDR)
 
 struct dwapb_gpio;
+struct dwapb_context;
 
 struct dwapb_gpio_port {
 	struct bgpio_chip	bgc;
 	bool			is_registered;
 	struct dwapb_gpio	*gpio;
+	struct dwapb_context	*ctx;
+	unsigned int		idx;
 };
 
 struct dwapb_gpio {
@@ -377,6 +380,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 
 	port = &gpio->ports[offs];
 	port->gpio = gpio;
+	port->idx = pp->idx;
 
 	dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE);
 	set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE);
@@ -584,10 +588,115 @@ static const struct of_device_id dwapb_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dwapb_of_match);
 
+#ifdef CONFIG_PM_SLEEP
+/* Store GPIO context across system-wide suspend/resume transitions */
+struct dwapb_context {
+	u32 data;
+	u32 dir;
+	u32 ext;
+	u32 int_en;
+	u32 int_mask;
+	u32 int_type;
+	u32 int_pol;
+	u32 int_deb;
+};
+
+static int dwapb_gpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
+	struct bgpio_chip *bgc	= &gpio->ports[0].bgc;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	for (i = 0; i < gpio->nr_ports; i++) {
+		unsigned int offset;
+		unsigned int idx = gpio->ports[i].idx;
+		struct dwapb_context *ctx = gpio->ports[i].ctx;
+
+		if (!ctx) {
+			ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+			gpio->ports[i].ctx = ctx;
+		}
+
+		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
+		ctx->dir = dwapb_read(gpio, offset);
+
+		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
+		ctx->data = dwapb_read(gpio, offset);
+
+		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
+		ctx->ext = dwapb_read(gpio, offset);
+
+		/* Only port A can provide interrupts */
+		if (idx == 0) {
+			ctx->int_mask	= dwapb_read(gpio, GPIO_INTMASK);
+			ctx->int_en	= dwapb_read(gpio, GPIO_INTEN);
+			ctx->int_pol	= dwapb_read(gpio, GPIO_INT_POLARITY);
+			ctx->int_type	= dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
+			ctx->int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
+
+			/* Mask out interrupts */
+			dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
+		}
+	}
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+
+static int dwapb_gpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
+	struct bgpio_chip *bgc	= &gpio->ports[0].bgc;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	for (i = 0; i < gpio->nr_ports; i++) {
+		unsigned int offset;
+		unsigned int idx = gpio->ports[i].idx;
+		struct dwapb_context *ctx = gpio->ports[i].ctx;
+
+		BUG_ON(ctx == 0);
+
+		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
+		dwapb_write(gpio, offset, ctx->data);
+
+		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
+		dwapb_write(gpio, offset, ctx->dir);
+
+		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
+		dwapb_write(gpio, offset, ctx->ext);
+
+		/* Only port A can provide interrupts */
+		if (idx == 0) {
+			dwapb_write(gpio, GPIO_INTTYPE_LEVEL, ctx->int_type);
+			dwapb_write(gpio, GPIO_INT_POLARITY, ctx->int_pol);
+			dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ctx->int_deb);
+			dwapb_write(gpio, GPIO_INTEN, ctx->int_en);
+			dwapb_write(gpio, GPIO_INTMASK, ctx->int_mask);
+
+			/* Clear out spurious interrupts */
+			dwapb_write(gpio, GPIO_PORTA_EOI, 0xffffffff);
+		}
+	}
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend,
+			 dwapb_gpio_resume);
+
 static struct platform_driver dwapb_gpio_driver = {
 	.driver		= {
 		.name	= "gpio-dwapb",
 		.owner	= THIS_MODULE,
+		.pm	= &dwapb_gpio_pm_ops,
 		.of_match_table = of_match_ptr(dwapb_of_match),
 	},
 	.probe		= dwapb_gpio_probe,
-- 
1.7.9.5

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> >@@ -136,7 +136,6 @@ config GPIO_DWAPB
> > 	tristate "Synopsys DesignWare APB GPIO driver"
> > 	select GPIO_GENERIC
> > 	select GENERIC_IRQ_CHIP
> >-	depends on OF_GPIO
> 
> You cover this specific dependencies with inline ifdefs, but you lose the
> CONFIG_OF depends by dropping it, and there are no such checks in the probe
> routine. Assumptions of OF are not limited to probe in this driver.
> 
> While I would like to see this assumption properly abstracted, the most
> expedient/immediate fix is probably to add a depends on OF above.

Andriy, how do you think? 

How about 
depends on OF_GPIO || MFD_GPIO_DWAPB, or
depends on OF_GPIO || MFD_INTEL_QUARK_I2C_GPIO?


> --
> Darren
> Intel Open Source Technology Center
> 

Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Darren Hart]

On Wed, Sep 10, 2014 at 12:34:50AM +0000, Chen, Alvin wrote:
> > >@@ -136,7 +136,6 @@ config GPIO_DWAPB
> > > 	tristate "Synopsys DesignWare APB GPIO driver"
> > > 	select GPIO_GENERIC
> > > 	select GENERIC_IRQ_CHIP
> > >-	depends on OF_GPIO
> > 
> > You cover this specific dependencies with inline ifdefs, but you lose the
> > CONFIG_OF depends by dropping it, and there are no such checks in the probe
> > routine. Assumptions of OF are not limited to probe in this driver.
> > 
> > While I would like to see this assumption properly abstracted, the most
> > expedient/immediate fix is probably to add a depends on OF above.
> 
> Andriy, how do you think? 
> 
> How about 
> depends on OF_GPIO || MFD_GPIO_DWAPB, or
> depends on OF_GPIO || MFD_INTEL_QUARK_I2C_GPIO?
> 

Upon closer inspection, I think my concern is invalid. the OF header already
tests for CONFIG_OF and provides no-op / -ENOSYS (tsk tsk) alternatives. So long
as you can compile with "#CONFIG_OF is not set" as it is, then I withdraw my
comment.

Sorry for the noise.

-- 
Darren Hart
Intel Open Source Technology Center

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> > > You cover this specific dependencies with inline ifdefs, but you
> > > lose the CONFIG_OF depends by dropping it, and there are no such
> > > checks in the probe routine. Assumptions of OF are not limited to probe in
> this driver.
> > >
> > > While I would like to see this assumption properly abstracted, the
> > > most expedient/immediate fix is probably to add a depends on OF above.
> >
> > Andriy, how do you think?
> >
> > How about
> > depends on OF_GPIO || MFD_GPIO_DWAPB, or depends on OF_GPIO ||
> > MFD_INTEL_QUARK_I2C_GPIO?
> >
> 
> Upon closer inspection, I think my concern is invalid. the OF header already
> tests for CONFIG_OF and provides no-op / -ENOSYS (tsk tsk) alternatives. So
> long as you can compile with "#CONFIG_OF is not set" as it is, then I withdraw
> my comment.
Yes, it can be compiled with "#CONFIG_OF is not set" and "#CONFIG_OF_GPIO is not set"

> 
> Sorry for the noise.
> 
> --
> Darren Hart
> Intel Open Source Technology Center

Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[atull]

On Tue, 9 Sep 2014, Weike Chen wrote:

> The Synopsys DesignWare APB GPIO driver only supports open firmware devices.
> But, like Intel Quark X1000 SOC, which has a single PCI function exporting
> a GPIO and an I2C controller, it is a Multifunction device. This patch is
> to enable the current Synopsys DesignWare APB GPIO driver to support the
> Multifunction device which exports the designware GPIO controller.
> 
> Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com>
> Signed-off-by: Weike Chen <alvin.chen@intel.com>

Hi Alvin,

I did a quick test and this looks like it works for me (with device tree).
I had a couple of small fixes below.

Alan

>  
> -	port->bgc.gc.ngpio = ngpio;
> -	port->bgc.gc.of_node = port_np;
> +#ifdef CONFIG_OF_GPIO
> +	port->bgc.gc.of_node = pp->node;
> +#endif

Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
elsewhere.

>  static int dwapb_gpio_probe(struct platform_device *pdev)
>  {
> +	int i;
>  	struct resource *res;
>  	struct dwapb_gpio *gpio;
> -	struct device_node *np;
>  	int err;
> -	unsigned int offs = 0;
> +	struct device *dev = &pdev->dev;
> +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> +	bool is_pdata_alloc = !pdata;

Please combine the int's in one line (int err, i;) and put them as
the last one on this list.  It looks the same to the compiler of
course, but more uniform for human eyes :)

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> 
> Hi Alvin,
> 
> I did a quick test and this looks like it works for me (with device tree).
> I had a couple of small fixes below.
It is very appreciated to help testing.


> Alan
> 
> >
> > -	port->bgc.gc.ngpio = ngpio;
> > -	port->bgc.gc.of_node = port_np;
> > +#ifdef CONFIG_OF_GPIO
> > +	port->bgc.gc.of_node = pp->node;
> > +#endif
> 
> Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
> elsewhere.
OK.

> 
> >  static int dwapb_gpio_probe(struct platform_device *pdev)  {
> > +	int i;
> >  	struct resource *res;
> >  	struct dwapb_gpio *gpio;
> > -	struct device_node *np;
> >  	int err;
> > -	unsigned int offs = 0;
> > +	struct device *dev = &pdev->dev;
> > +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > +	bool is_pdata_alloc = !pdata;
> 
> Please combine the int's in one line (int err, i;) and put them as the last one on
> this list.  It looks the same to the compiler of course, but more uniform for
> human eyes :)
OK.

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> >
> > Hi Alvin,
> >
> > I did a quick test and this looks like it works for me (with device tree).
> > I had a couple of small fixes below.
> It is very appreciated to help testing.
> 
> 
> > Alan
> >
> > >
> > > -	port->bgc.gc.ngpio = ngpio;
> > > -	port->bgc.gc.of_node = port_np;
> > > +#ifdef CONFIG_OF_GPIO
> > > +	port->bgc.gc.of_node = pp->node;
> > > +#endif
> >
> > Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
> > elsewhere.
> OK.
> 
Alan, I just do a quick test, here we can't use 'IS_ENABLED', it can't be compiled without OF_GPIO set.
Because 'gc.of_node' is not defined without 'OF_GPIO'. You can refer the structure of 'gc'.

Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1529 bytes --]

On Wed, 2014-09-10 at 14:11 -0500, atull wrote:

[]

> >  static int dwapb_gpio_probe(struct platform_device *pdev)
> >  {
> > +	int i;
> >  	struct resource *res;
> >  	struct dwapb_gpio *gpio;
> > -	struct device_node *np;
> >  	int err;
> > -	unsigned int offs = 0;
> > +	struct device *dev = &pdev->dev;
> > +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > +	bool is_pdata_alloc = !pdata;
> 
> Please combine the int's in one line (int err, i;) and put them as
> the last one on this list.  It looks the same to the compiler of
> course, but more uniform for human eyes :)

Do you think it's a good idea? In this case I, for example, would like
to see int err as a separate line at the end of definition block. It
would be better to distinguish counters and return code storage.
Moreover, often counters would be unsigned int.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[atull]

On Thu, 11 Sep 2014, Shevchenko, Andriy wrote:

> On Wed, 2014-09-10 at 14:11 -0500, atull wrote:
> 
> []
> 
> > >  static int dwapb_gpio_probe(struct platform_device *pdev)
> > >  {
> > > +	int i;
> > >  	struct resource *res;
> > >  	struct dwapb_gpio *gpio;
> > > -	struct device_node *np;
> > >  	int err;
> > > -	unsigned int offs = 0;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > > +	bool is_pdata_alloc = !pdata;
> > 
> > Please combine the int's in one line (int err, i;) and put them as
> > the last one on this list.  It looks the same to the compiler of
> > course, but more uniform for human eyes :)
> 
> Do you think it's a good idea? In this case I, for example, would like
> to see int err as a separate line at the end of definition block. It
> would be better to distinguish counters and return code storage.
> Moreover, often counters would be unsigned int.

If they are both 'int' they should be combined.  If 'i' is changed to be
an unsigned int they would be separate.

> 
> -- 
> Andy Shevchenko <andriy.shevchenko@intel.com>
> Intel Finland Oy
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki 
> Business Identity Code: 0357606 - 4 
> Domiciled in Helsinki 
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

Andy,

This confidentiality footer is problematic on this mailing list.  Please
see https://lkml.org/lkml/2014/3/5/790 for example.

Alan

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[atull]

On Thu, 11 Sep 2014, Chen, Alvin wrote:

> > >
> > > Hi Alvin,
> > >
> > > I did a quick test and this looks like it works for me (with device tree).
> > > I had a couple of small fixes below.
> > It is very appreciated to help testing.

Sure.  Thanks for adding features to the driver!

> > 
> > 
> > > Alan
> > >
> > > >
> > > > -	port->bgc.gc.ngpio = ngpio;
> > > > -	port->bgc.gc.of_node = port_np;
> > > > +#ifdef CONFIG_OF_GPIO
> > > > +	port->bgc.gc.of_node = pp->node;
> > > > +#endif
> > >
> > > Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
> > > elsewhere.
> > OK.
> > 
> Alan, I just do a quick test, here we can't use 'IS_ENABLED', it can't be compiled without OF_GPIO set.
> Because 'gc.of_node' is not defined without 'OF_GPIO'. You can refer the structure of 'gc'.
> 

That makes sense.

Thanks,
Alan

Re: [PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[atull]

On Tue, 9 Sep 2014, Weike Chen wrote:

>  
>  struct dwapb_gpio;
> +struct dwapb_context;
>  
>  struct dwapb_gpio_port {
>  	struct bgpio_chip	bgc;
>  	bool			is_registered;
>  	struct dwapb_gpio	*gpio;
> +	struct dwapb_context	*ctx;

Alvin,

Will this build if CONFIG_PM_SLEEP is not defined?

Alan

> +	unsigned int		idx;
>  };
>  

>  struct dwapb_gpio {
> @@ -377,6 +380,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  
>  	port = &gpio->ports[offs];
>  	port->gpio = gpio;
> +	port->idx = pp->idx;
>  
>  	dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE);
>  	set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE);
> @@ -584,10 +588,115 @@ static const struct of_device_id dwapb_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dwapb_of_match);
>  
> +#ifdef CONFIG_PM_SLEEP
> +/* Store GPIO context across system-wide suspend/resume transitions */
> +struct dwapb_context {
> +	u32 data;
> +	u32 dir;
> +	u32 ext;
> +	u32 int_en;
> +	u32 int_mask;
> +	u32 int_type;
> +	u32 int_pol;
> +	u32 int_deb;
> +};
> +

RE: [PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Chen, Alvin]

> On Tue, 9 Sep 2014, Weike Chen wrote:
> 
> >
> >  struct dwapb_gpio;
> > +struct dwapb_context;
> >
> >  struct dwapb_gpio_port {
> >  	struct bgpio_chip	bgc;
> >  	bool			is_registered;
> >  	struct dwapb_gpio	*gpio;
> > +	struct dwapb_context	*ctx;
> 
> Alvin,
> 
> Will this build if CONFIG_PM_SLEEP is not defined?
Actually, PM_SLEEP is always set as 'y' in 'kerne/power/Kconfig'. But I manually change it to 'n', this module can be compiled correctly.
You may be concern with 'ctx', and you can see 'ctx' accessing is always in CONFIG_PM_SLEEP.


> Alan

RE: [PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[atull]

On Fri, 12 Sep 2014, Chen, Alvin wrote:

> > On Tue, 9 Sep 2014, Weike Chen wrote:
> > 
> > >
> > >  struct dwapb_gpio;
> > > +struct dwapb_context;
> > >
> > >  struct dwapb_gpio_port {
> > >  	struct bgpio_chip	bgc;
> > >  	bool			is_registered;
> > >  	struct dwapb_gpio	*gpio;
> > > +	struct dwapb_context	*ctx;
> > 
> > Alvin,
> > 
> > Will this build if CONFIG_PM_SLEEP is not defined?
> Actually, PM_SLEEP is always set as 'y' in 'kerne/power/Kconfig'. But I manually change it to 'n', this module can be compiled correctly.
> You may be concern with 'ctx', and you can see 'ctx' accessing is always in CONFIG_PM_SLEEP.

Yes and in the case of 'struct dwapb_context *ctx;' it is ok for struct 
dwapb_context to be an incomplete type since that's just a pointer.

Alan 

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> >
> > > >  static int dwapb_gpio_probe(struct platform_device *pdev)  {
> > > > +	int i;
> > > >  	struct resource *res;
> > > >  	struct dwapb_gpio *gpio;
> > > > -	struct device_node *np;
> > > >  	int err;
> > > > -	unsigned int offs = 0;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > > > +	bool is_pdata_alloc = !pdata;
> > >
> > > Please combine the int's in one line (int err, i;) and put them as
> > > the last one on this list.  It looks the same to the compiler of
> > > course, but more uniform for human eyes :)
> >
> > Do you think it's a good idea? In this case I, for example, would like
> > to see int err as a separate line at the end of definition block. It
> > would be better to distinguish counters and return code storage.
> > Moreover, often counters would be unsigned int.
> 
> If they are both 'int' they should be combined.  If 'i' is changed to be an
> unsigned int they would be separate.

Linus, do you have any idea about it? I think it is not a big issue.
> 

RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

> 
> > >
> > > > >  static int dwapb_gpio_probe(struct platform_device *pdev)  {
> > > > > +	int i;
> > > > >  	struct resource *res;
> > > > >  	struct dwapb_gpio *gpio;
> > > > > -	struct device_node *np;
> > > > >  	int err;
> > > > > -	unsigned int offs = 0;
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > > > > +	bool is_pdata_alloc = !pdata;
> > > >
> > > > Please combine the int's in one line (int err, i;) and put them as
> > > > the last one on this list.  It looks the same to the compiler of
> > > > course, but more uniform for human eyes :)
> > >
> > > Do you think it's a good idea? In this case I, for example, would
> > > like to see int err as a separate line at the end of definition
> > > block. It would be better to distinguish counters and return code storage.
> > > Moreover, often counters would be unsigned int.
> >
> > If they are both 'int' they should be combined.  If 'i' is changed to
> > be an unsigned int they would be separate.
> 
> Linus, do you have any idea about it? I think it is not a big issue.
> >

Since no further feedbacks, I decide to use 'unsigned int i' to align the two feedbacks, since 'i' is just a counter.
And will send a new version with just this changes later.