[PATCH 0/3 v2] The Designware GPIO Supporting

[PATCH 0/3 v2] 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 enables the gpio
'debounce' feature. And the third patch enables the power management.

---
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 (3):
  GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
  GPIO: gpio-dwapb: Support Debounce
  GPIO: gpio-dwapb: Suspend & Resume PM enabling

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

-- 
1.7.9.5

[PATCH 1/3 v2] 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>
Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/Kconfig                     |    1 -
 drivers/gpio/gpio-dwapb.c                |  233 +++++++++++++++++++++++-------
 include/linux/platform_data/gpio-dwapb.h |   32 ++++
 3 files changed, 211 insertions(+), 55 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..f2264a2 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -21,6 +21,7 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
+#include <linux/platform_data/gpio-dwapb.h>
 
 #define GPIO_SWPORTA_DR		0x00
 #define GPIO_SWPORTA_DDR	0x04
@@ -84,11 +85,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_irq_handler(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 +102,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_irq_handler(gpio);
+
 	if (chip->irq_eoi)
 		chip->irq_eoi(irq_desc_get_irq_data(desc));
 }
@@ -207,22 +217,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 = (struct dwapb_gpio *)dev_id;
+
+	worked = _dwapb_irq_handler(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 +283,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 +326,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 +377,130 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 			gpiochip_remove(&gpio->ports[m].bgc.gc);
 }
 
+#ifdef CONFIG_OF_GPIO
+
+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 (!node)
+		return ERR_PTR(-ENODEV);
+
+	nports = of_get_child_count(node);
+	if (nports == 0)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->properties = devm_kcalloc(dev, nports, sizeof(*pp), GFP_KERNEL);
+	if (!pdata->properties)
+		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);
+			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 device *dev,
+				       struct dwapb_platform_data *pdata)
+{
+	if (!pdata)
+		return;
+	if (pdata->properties)
+		devm_kfree(dev, pdata->properties);
+	devm_kfree(dev, pdata);
+}
+
+#else
+
+static inline struct dwapb_platform_data *
+dwapb_gpio_get_pdata_of(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline void dwapb_free_pdata_of(struct device *dev,
+				       struct dwapb_platform_data *pdata)
+{
+}
+
+#endif
+
 static int dwapb_gpio_probe(struct platform_device *pdev)
 {
+	int i;
+	bool is_of;
 	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);
+
+	if (!pdata) {
+		pdata = dwapb_gpio_get_pdata_of(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		is_of = true;
+	} else {
+		is_of = false;
+	}
+	if (!pdata->nports)
+		return -ENODEV;
 
 	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
 		return -ENOMEM;
 	gpio->dev = &pdev->dev;
+	gpio->nr_ports = pdata->nports;
 
-	gpio->nr_ports = of_get_child_count(pdev->dev.of_node);
-	if (!gpio->nr_ports) {
-		err = -EINVAL;
-		goto out_err;
-	}
-	gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
+	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
 	if (!gpio->ports) {
 		err = -ENOMEM;
@@ -394,13 +514,18 @@ 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);
 
+	if (is_of) {
+		dwapb_free_pdata_of(dev, pdata);
+		pdata = NULL;
+	}
+
 	return 0;
 
 out_unregister:
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

Re: [PATCH 1/3 v2] 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: 13701 bytes --]

On Fri, 2014-09-05 at 07:53 -0700, 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.

Few comments below.

> 
> 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/Kconfig                     |    1 -
>  drivers/gpio/gpio-dwapb.c                |  233 +++++++++++++++++++++++-------
>  include/linux/platform_data/gpio-dwapb.h |   32 ++++
>  3 files changed, 211 insertions(+), 55 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..f2264a2 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -21,6 +21,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> +#include <linux/platform_data/gpio-dwapb.h>
>  
>  #define GPIO_SWPORTA_DR		0x00
>  #define GPIO_SWPORTA_DDR	0x04
> @@ -84,11 +85,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_irq_handler(struct dwapb_gpio *gpio)

What about dwapb_do_irq() ?


>  {
> -	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 +102,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_irq_handler(gpio);
> +
>  	if (chip->irq_eoi)
>  		chip->irq_eoi(irq_desc_get_irq_data(desc));
>  }
> @@ -207,22 +217,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 = (struct dwapb_gpio *)dev_id;

No need to cast explicitly from void *.

> +
> +	worked = _dwapb_irq_handler(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 +283,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 +326,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

Do we really need this #ifdef ?
of_node will be NULL anyway, or I missed something?

> +	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)

irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely
we have it somewhere, but still it's possible. And yes, IRQ framework
doesn't work with virq == 0 (*virtual* irq), but accepts hwirq == 0. I
recommend to use int type for irq line number, and recognize negative
value (usually -1) as no irq needed / found.

> +		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 +377,130 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>  			gpiochip_remove(&gpio->ports[m].bgc.gc);
>  }
>  
> +#ifdef CONFIG_OF_GPIO
> +
> +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 (!node)
> +		return ERR_PTR(-ENODEV);
> +
> +	nports = of_get_child_count(node);
> +	if (nports == 0)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->properties = devm_kcalloc(dev, nports, sizeof(*pp), GFP_KERNEL);
> +	if (!pdata->properties)
> +		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);
> +			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 device *dev,
> +				       struct dwapb_platform_data *pdata)
> +{
> +	if (!pdata)
> +		return;
> +	if (pdata->properties)
> +		devm_kfree(dev, pdata->properties);
> +	devm_kfree(dev, pdata);
> +}
> +
> +#else
> +
> +static inline struct dwapb_platform_data *
> +dwapb_gpio_get_pdata_of(struct device *dev)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void dwapb_free_pdata_of(struct device *dev,
> +				       struct dwapb_platform_data *pdata)
> +{
> +}
> +
> +#endif
> +
>  static int dwapb_gpio_probe(struct platform_device *pdev)
>  {
> +	int i;
> +	bool is_of;

is_pdata_alloc (it might be not only OF case in future).



>  	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);
> +
> +	if (!pdata) {

(*)

> +		pdata = dwapb_gpio_get_pdata_of(dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);

> +		is_of = true;
> +	} else {
> +		is_of = false;

Instead of above three lines, how about
bool is_pdata_alloc = !pdata;

And (*) if (is_pdata_alloc) ...


> +	}
> +	if (!pdata->nports)
> +		return -ENODEV;
>  
>  	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>  	if (!gpio)
>  		return -ENOMEM;
>  	gpio->dev = &pdev->dev;
> +	gpio->nr_ports = pdata->nports;
>  
> -	gpio->nr_ports = of_get_child_count(pdev->dev.of_node);
> -	if (!gpio->nr_ports) {
> -		err = -EINVAL;
> -		goto out_err;
> -	}
> -	gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
> +	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
>  				   sizeof(*gpio->ports), GFP_KERNEL);
>  	if (!gpio->ports) {
>  		err = -ENOMEM;
> @@ -394,13 +514,18 @@ 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);
>  
> +	if (is_of) {
> +		dwapb_free_pdata_of(dev, pdata);
> +		pdata = NULL;

Besides that pdata assignment is probably redundant, you may use plain
kmalloc/kfree and avoid unnecessary devm_* calls.

> +	}
> +
>  	return 0;
>  
>  out_unregister:
> 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


-- 
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/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Chen, Alvin]

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

> > -static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> > +static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)
> 
> What about dwapb_do_irq() ?
OK, I will improve it.

> > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
> > +	u32 worked;
> > +	struct dwapb_gpio *gpio = (struct dwapb_gpio *)dev_id;
> 
> No need to cast explicitly from void *.
OK.


> > -	port->bgc.gc.ngpio = ngpio;
> > -	port->bgc.gc.of_node = port_np;
> > +#ifdef CONFIG_OF_GPIO
> 
> Do we really need this #ifdef ?
> of_node will be NULL anyway, or I missed something?
Yes, otherwise, can't compile it. Please refer 'struct gpio_chip', 'gc.of_node' is in OF_GPIO micro also.

> > +	if (pp->irq)
> 
> irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely we have it
> somewhere, but still it's possible. And yes, IRQ framework doesn't work with
> virq == 0 (*virtual* irq), but accepts hwirq == 0. I recommend to use int type for
> irq line number, and recognize negative value (usually -1) as no irq needed /
> found.
Understand. But if you refer the original code, you can see:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
......
return;
}
>From above code, if irq=0, it indicates irq is not supported for OF devices. If we use '-1' to indicate irq is not supported. To make OF work, then our code should be:

irq = irq_of_parse_and_map(node, 0);
If (!irq) {
pp->irq = -1;
return;
} else {
pp->irq = irq;
}
Then the code looks strange.

How do you think?

> > +	bool is_of;
> 
> is_pdata_alloc (it might be not only OF case in future).
> 
OK


> > +	if (!pdata) {
> 
> (*)

> > +		pdata = dwapb_gpio_get_pdata_of(dev);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> 
> > +		is_of = true;
> > +	} else {
> > +		is_of = false;
> 
> Instead of above three lines, how about
> bool is_pdata_alloc = !pdata;
> 
> And (*) if (is_pdata_alloc) ...
> 
OK. I will improve this part.

> > +	if (is_of) {
> > +		dwapb_free_pdata_of(dev, pdata);
> > +		pdata = NULL;
> 
> Besides that pdata assignment is probably redundant, you may use plain
> kmalloc/kfree and avoid unnecessary devm_* calls.
> 
OK.

ÿôèº{.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/3 v2] 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: 2469 bytes --]

On Fri, 2014-09-05 at 10:20 +0000, Chen, Alvin wrote:
> > > -	port->bgc.gc.ngpio = ngpio;
> > > -	port->bgc.gc.of_node = port_np;
> > > +#ifdef CONFIG_OF_GPIO
> > 
> > Do we really need this #ifdef ?
> > of_node will be NULL anyway, or I missed something?
> Yes, otherwise, can't compile it. Please refer 'struct gpio_chip', 'gc.of_node' is in OF_GPIO micro also.

Ah, okay. Thus, it depends to Linus opinion, since I, for example, would
like to see this field present in the structure independently of
OF_GPIO.

> 
> > > +	if (pp->irq)
> > 
> > irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely we have it
> > somewhere, but still it's possible. And yes, IRQ framework doesn't work with
> > virq == 0 (*virtual* irq), but accepts hwirq == 0. I recommend to use int type for
> > irq line number, and recognize negative value (usually -1) as no irq needed /
> > found.
> Understand. But if you refer the original code, you can see:
> irq = irq_of_parse_and_map(node, 0);
> If (!irq) {
> ......
> return;
> }
> From above code, if irq=0, it indicates irq is not supported for OF devices. If we use '-1' to indicate irq is not supported. To make OF work, then our code should be:

Yes, like I said above. You introduce hw irq in the pdata, which could
be 0.


> irq = irq_of_parse_and_map(node, 0);
> If (!irq) {
> pp->irq = -1;
> return;
> } else {
> pp->irq = irq;
> }
> Then the code looks strange.
> 
> How do you think?

If I understood correctly you messed up with hwirq vs. virq.
Otherwise you have mention that you are using virq everywhere (I guess
you may rename the field in the structure), but in this case the field
in the platform_data looks a bit strange. Linus, what do you think?


P.S. Please, remove my Reviewed-by tag since code is changed enough.

-- 
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/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Arnd Bergmann]

On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
> > irq = irq_of_parse_and_map(node, 0);
> > If (!irq) {
> > pp->irq = -1;
> > return;
> > } else {
> > pp->irq = irq;
> > }
> > Then the code looks strange.
> > 
> > How do you think?
> 
> If I understood correctly you messed up with hwirq vs. virq.
> Otherwise you have mention that you are using virq everywhere (I guess
> you may rename the field in the structure), but in this case the field
> in the platform_data looks a bit strange.

The field in platform_data should be the mapped virtual irq number, it
makes no sense to use the hwirq unless you also add a pointer to the domain
in which that hwirq exists.

Also the output of irq_of_parse_and_map() is a mapped irq, as the name
suggests.

	Arnd

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

[Chen, Alvin]

> On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
> > > irq = irq_of_parse_and_map(node, 0); If (!irq) {
> > > pp->irq = -1;
> > > return;
> > > } else {
> > > pp->irq = irq;
> > > }
> > > Then the code looks strange.
> > >
> > > How do you think?
> >
> > If I understood correctly you messed up with hwirq vs. virq.
> > Otherwise you have mention that you are using virq everywhere (I guess
> > you may rename the field in the structure), but in this case the field
> > in the platform_data looks a bit strange.
> 
> The field in platform_data should be the mapped virtual irq number, it makes no
> sense to use the hwirq unless you also add a pointer to the domain in which
> that hwirq exists.
> 
> Also the output of irq_of_parse_and_map() is a mapped irq, as the name
> suggests.
> 
I agree with Arnd. Here, the 'irq' is 'virq'.
Andriy, you may be confused by the code like 'irq_create_mapping'. For Quark case, it has 8 GPIO pins, and each pin can trigger
interrupt, but all these interrupts are triggered by PCI irq which is shared. The 'irq' in pdata is PCI irq. As all GPIO interrupts connect to the PCI
irq, once the GPIO interrupt is triggered, and the PCI irq handler will be called 'dwapb_irq_handler_mfd'. And in 'dwapb_do_irq', it will read the
interrupt register to see the interrupt is triggered by which GPIO pin, and 'irq_create_mapping' is for this case.


.

Re: [PATCH 1/3 v2] 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: 2246 bytes --]

On Tue, 2014-09-09 at 01:50 +0000, Chen, Alvin wrote:
> > On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
> > > > irq = irq_of_parse_and_map(node, 0); If (!irq) {
> > > > pp->irq = -1;
> > > > return;
> > > > } else {
> > > > pp->irq = irq;
> > > > }
> > > > Then the code looks strange.
> > > >
> > > > How do you think?
> > >
> > > If I understood correctly you messed up with hwirq vs. virq.
> > > Otherwise you have mention that you are using virq everywhere (I guess
> > > you may rename the field in the structure), but in this case the field
> > > in the platform_data looks a bit strange.
> > 
> > The field in platform_data should be the mapped virtual irq number, it makes no
> > sense to use the hwirq unless you also add a pointer to the domain in which
> > that hwirq exists.
> > 
> > Also the output of irq_of_parse_and_map() is a mapped irq, as the name
> > suggests.
> > 
> I agree with Arnd. Here, the 'irq' is 'virq'.
> Andriy, you may be confused by the code like 'irq_create_mapping'. For Quark case, it has 8 GPIO pins, and each pin can trigger
> interrupt, but all these interrupts are triggered by PCI irq which is shared. The 'irq' in pdata is PCI irq. As all GPIO interrupts connect to the PCI
> irq, once the GPIO interrupt is triggered, and the PCI irq handler will be called 'dwapb_irq_handler_mfd'. And in 'dwapb_do_irq', it will read the
> interrupt register to see the interrupt is triggered by which GPIO pin, and 'irq_create_mapping' is for this case.

Thanks for clarification. I take back my initial comment then.

-- 
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/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Arnd Bergmann]

On Friday 05 September 2014 07:53:16 Weike Chen wrote:
>  
> -	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;
> +		}
> +	}
>

I think this need some better documentation. Why is it safe to use
devm_request_irq rather than irq_set_chained_handler here?


>  
> +#ifdef CONFIG_OF_GPIO
> +
> +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 (!node)
> +		return ERR_PTR(-ENODEV);

Please replace the #ifdef above with 

	if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)

here so get you proper compile-time coverage of the DT code path.

	Arnd

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

[Sebastian Andrzej Siewior]

On 2014-09-05 13:50:06 [+0200], Arnd Bergmann wrote:
> On Friday 05 September 2014 07:53:16 Weike Chen wrote:
> >  
> > -	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;
> > +		}
> > +	}
> >
> 
> I think this need some better documentation. Why is it safe to use
> devm_request_irq rather than irq_set_chained_handler here?

Usually it is preferred to use irq_set_chained_handler() for the chained
handler so the handler does not show up in /proc/interrupts.
This requires an exclusive non-shared handler which is not the case on
the intel platform. So they have to use devm_request_irq() instead.

Sebastian

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

[Chen, Alvin]

> > >
> > > -	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;
> > > +		}
> > > +	}
> > >
> >
> > I think this need some better documentation. Why is it safe to use
> > devm_request_irq rather than irq_set_chained_handler here?
> 
> Usually it is preferred to use irq_set_chained_handler() for the chained handler
> so the handler does not show up in /proc/interrupts.
> This requires an exclusive non-shared handler which is not the case on the intel
> platform. So they have to use devm_request_irq() instead.
> 
Yes, for Intel Quark, it has a single PCI function exporting a GPIO and I2C controller, and
the irq is shared by GPIO and I2C, so we need shared irq as the comments said.

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

[Chen, Alvin]

> >
> > +#ifdef CONFIG_OF_GPIO
> > +
> > +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 (!node)
> > +		return ERR_PTR(-ENODEV);
> 
> Please replace the #ifdef above with
> 
> 	if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
> 
> here so get you proper compile-time coverage of the DT code path.
OK, I will improve it.

[PATCH 2/3 v2] 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 |   62 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index f2264a2..6db7501 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -36,6 +36,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
@@ -63,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);
@@ -75,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_irq_handler(struct dwapb_gpio *gpio)
@@ -125,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);
 }
 
@@ -140,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);
 }
 
@@ -182,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:
@@ -210,8 +228,31 @@ 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;
+}
+
+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;
@@ -354,6 +395,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 #endif
 	port->bgc.gc.ngpio = pp->ngpio;
 	port->bgc.gc.base = pp->gpio_base;
+	port->bgc.gc.set_debounce = dwapb_gpio_set_debounce;
 
 	if (pp->irq)
 		dwapb_configure_irqs(gpio, port, pp);
-- 
1.7.9.5

Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce

[Shevchenko, Andriy]

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

On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> This patch enables 'debounce' for the designware GPIO, and
> it is based on Josef Ahmad's previous work.

Can we split dwapb_read/write introducing and changing from this patch
to a separate one?


> 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 |   62 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index f2264a2..6db7501 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -36,6 +36,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
> @@ -63,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);
> @@ -75,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_irq_handler(struct dwapb_gpio *gpio)
> @@ -125,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);
>  }
>  
> @@ -140,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);
>  }
>  
> @@ -182,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:
> @@ -210,8 +228,31 @@ 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;
> +}
> +
> +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;
> @@ -354,6 +395,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  #endif
>  	port->bgc.gc.ngpio = pp->ngpio;
>  	port->bgc.gc.base = pp->gpio_base;
> +	port->bgc.gc.set_debounce = dwapb_gpio_set_debounce;
>  
>  	if (pp->irq)
>  		dwapb_configure_irqs(gpio, port, pp);


-- 
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 2/3 v2] GPIO: gpio-dwapb: Support Debounce

[Chen, Alvin]

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

> Subject: Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
> 
> On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> > This patch enables 'debounce' for the designware GPIO, and it is based
> > on Josef Ahmad's previous work.
> 
> Can we split dwapb_read/write introducing and changing from this patch to a
> separate one?
> 
Since the 'debounce' feature also needs read/write, if we splite this patch, then for 'debounce',
One patch use readl/writel, and another patch change to dwapb_read/write. It seems duplicate since
the previous patch use readl/writel and the fllowing one change it immediately.
Since this patch is small, could we just make them together? How do you think?
ÿôèº{.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 2/3 v2] GPIO: gpio-dwapb: Support Debounce

[Shevchenko, Andriy]

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

On Fri, 2014-09-05 at 09:35 +0000, Chen, Alvin wrote:
> > Subject: Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
> > 
> > On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> > > This patch enables 'debounce' for the designware GPIO, and it is based
> > > on Josef Ahmad's previous work.
> > 
> > Can we split dwapb_read/write introducing and changing from this patch to a
> > separate one?
> > 
> Since the 'debounce' feature also needs read/write, if we splite this patch, then for 'debounce',
> One patch use readl/writel, and another patch change to dwapb_read/write. It seems duplicate since
> the previous patch use readl/writel and the fllowing one change it immediately.
> Since this patch is small, could we just make them together? How do you think?

Make the dwapb_writel/readl patch goes first in the series.

-- 
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 2/3 v2] GPIO: gpio-dwapb: Support Debounce

[Chen, Alvin]

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

> > Since the 'debounce' feature also needs read/write, if we splite this
> > patch, then for 'debounce', One patch use readl/writel, and another
> > patch change to dwapb_read/write. It seems duplicate since the previous
> patch use readl/writel and the fllowing one change it immediately.
> > Since this patch is small, could we just make them together? How do you
> think?
> 
> Make the dwapb_writel/readl patch goes first in the series.
> 
OK.
ÿôèº{.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¥

[PATCH 3/3 v2] 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>
Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
Signed-off-by: Weike Chen <alvin.chen@intel.com>
---
 drivers/gpio/gpio-dwapb.c |  102 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 6db7501..a103def 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -54,6 +54,7 @@ struct dwapb_gpio_port {
 	struct bgpio_chip	bgc;
 	bool			is_registered;
 	struct dwapb_gpio	*gpio;
+	unsigned int		idx;
 };
 
 struct dwapb_gpio {
@@ -376,6 +377,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);
@@ -594,10 +596,110 @@ 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 */
+static struct dwapb_context {
+	u32 data[DWAPB_MAX_PORTS];
+	u32 dir[DWAPB_MAX_PORTS];
+	u32 ext[DWAPB_MAX_PORTS];
+	u32 int_en;
+	u32 int_mask;
+	u32 int_type;
+	u32 int_pol;
+	u32 int_deb;
+} dwapb_context;
+
+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;
+
+		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
+		dwapb_context.dir[i]	= dwapb_read(gpio, offset);
+
+		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
+		dwapb_context.data[i]	= dwapb_read(gpio, offset);
+
+		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
+		dwapb_context.ext[i]	= dwapb_read(gpio, offset);
+
+		if (idx == 0) {
+			dwapb_context.int_mask = dwapb_read(gpio, GPIO_INTMASK);
+			dwapb_context.int_en = dwapb_read(gpio, GPIO_INTEN);
+			dwapb_context.int_pol =
+					dwapb_read(gpio, GPIO_INT_POLARITY);
+			dwapb_context.int_type =
+					dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
+			dwapb_context.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;
+
+		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
+		dwapb_write(gpio, offset, dwapb_context.data[i]);
+
+		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
+		dwapb_write(gpio, offset, dwapb_context.dir[i]);
+
+		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
+		dwapb_write(gpio, offset, dwapb_context.ext[i]);
+
+		if (idx == 0) {
+			dwapb_write(gpio, GPIO_INTTYPE_LEVEL,
+				    dwapb_context.int_type);
+			dwapb_write(gpio, GPIO_INT_POLARITY,
+				    dwapb_context.int_pol);
+			dwapb_write(gpio, GPIO_PORTA_DEBOUNCE,
+				    dwapb_context.int_deb);
+			dwapb_write(gpio, GPIO_INTEN, dwapb_context.int_en);
+			dwapb_write(gpio, GPIO_INTMASK, dwapb_context.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 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Shevchenko, Andriy]

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

On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> 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>
> Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>

I have to recall my reviwed-by tag since patch is quite changed and as I
understood Linus is continuing to be changed.

> Signed-off-by: Weike Chen <alvin.chen@intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c |  102 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 6db7501..a103def 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -54,6 +54,7 @@ struct dwapb_gpio_port {
>  	struct bgpio_chip	bgc;
>  	bool			is_registered;
>  	struct dwapb_gpio	*gpio;
> +	unsigned int		idx;
>  };
>  
>  struct dwapb_gpio {
> @@ -376,6 +377,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);
> @@ -594,10 +596,110 @@ 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 */
> +static struct dwapb_context {
> +	u32 data[DWAPB_MAX_PORTS];
> +	u32 dir[DWAPB_MAX_PORTS];
> +	u32 ext[DWAPB_MAX_PORTS];
> +	u32 int_en;
> +	u32 int_mask;
> +	u32 int_type;
> +	u32 int_pol;
> +	u32 int_deb;
> +} dwapb_context;
> +
> +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;
> +
> +		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
> +		dwapb_context.dir[i]	= dwapb_read(gpio, offset);
> +
> +		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
> +		dwapb_context.data[i]	= dwapb_read(gpio, offset);
> +
> +		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
> +		dwapb_context.ext[i]	= dwapb_read(gpio, offset);
> +
> +		if (idx == 0) {
> +			dwapb_context.int_mask = dwapb_read(gpio, GPIO_INTMASK);
> +			dwapb_context.int_en = dwapb_read(gpio, GPIO_INTEN);
> +			dwapb_context.int_pol =
> +					dwapb_read(gpio, GPIO_INT_POLARITY);
> +			dwapb_context.int_type =
> +					dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
> +			dwapb_context.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;
> +
> +		offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
> +		dwapb_write(gpio, offset, dwapb_context.data[i]);
> +
> +		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
> +		dwapb_write(gpio, offset, dwapb_context.dir[i]);
> +
> +		offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
> +		dwapb_write(gpio, offset, dwapb_context.ext[i]);
> +
> +		if (idx == 0) {
> +			dwapb_write(gpio, GPIO_INTTYPE_LEVEL,
> +				    dwapb_context.int_type);
> +			dwapb_write(gpio, GPIO_INT_POLARITY,
> +				    dwapb_context.int_pol);
> +			dwapb_write(gpio, GPIO_PORTA_DEBOUNCE,
> +				    dwapb_context.int_deb);
> +			dwapb_write(gpio, GPIO_INTEN, dwapb_context.int_en);
> +			dwapb_write(gpio, GPIO_INTMASK, dwapb_context.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,


-- 
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 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Chen, Alvin]

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

> 
> On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> > 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>
> > Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> 
> I have to recall my reviwed-by tag since patch is quite changed and as I
> understood Linus is continuing to be changed.
OK.
ÿôèº{.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 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Linus Walleij]

On Fri, Sep 5, 2014 at 7:53 AM, Weike Chen <alvin.chen@intel.com> wrote:

> 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>
> Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> Signed-off-by: Weike Chen <alvin.chen@intel.com>
(...)

> +/* Store GPIO context across system-wide suspend/resume transitions */
> +static struct dwapb_context {
> +       u32 data[DWAPB_MAX_PORTS];
> +       u32 dir[DWAPB_MAX_PORTS];
> +       u32 ext[DWAPB_MAX_PORTS];
> +       u32 int_en;
> +       u32 int_mask;
> +       u32 int_type;
> +       u32 int_pol;
> +       u32 int_deb;
> +} dwapb_context;

NAK, this should STILL be part of the device state container. Not
a local variable.

I've said this before. Please fix this, thanks.

Yours,
Linus Walleij

RE: [PATCH 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Chen, Alvin]

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

> 
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct dwapb_context {
> > +       u32 data[DWAPB_MAX_PORTS];
> > +       u32 dir[DWAPB_MAX_PORTS];
> > +       u32 ext[DWAPB_MAX_PORTS];
> > +       u32 int_en;
> > +       u32 int_mask;
> > +       u32 int_type;
> > +       u32 int_pol;
> > +       u32 int_deb;
> > +} dwapb_context;
> 
> NAK, this should STILL be part of the device state container. Not a local
> variable.
> 
> I've said this before. Please fix this, thanks.
> 
Linus, please review Version 5 that I sent on Sep.17th. I have fixed it in the v5.
ÿôèº{.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¥