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

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

[atull]

On Wed, 27 Aug 2014, 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>
> Signed-off-by: Weike Chen <alvin.chen@intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c |   67 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 

Hi Weike,

I tried out these patches on the current master branch (v3.17-rc2-9-g68e3702) with a socfpga cyclone5 board.

If I apply all 3 patches, the kernel doesn't boot.

If I rebuild with only the first patch, I get only one gpio block showing up (should
have 3 for this board) and these messages:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at /home/atull/repos/linux-socfpga/fs/sysfs/dir.c:31 sysfs_warn_dup+0x58/0x74()
sysfs: cannot create duplicate filename '/class/gpio/gpiochip0'
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc2-00012-g7ad6a41 #1
[<c0014ab4>] (unwind_backtrace) from [<c0011684>] (show_stack+0x10/0x14)
[<c0011684>] (show_stack) from [<c03d3d80>] (dump_stack+0x74/0x90)
[<c03d3d80>] (dump_stack) from [<c0020c88>] (warn_slowpath_common+0x68/0x8c)
[<c0020c88>] (warn_slowpath_common) from [<c0020d40>] (warn_slowpath_fmt+0x30/0x40)
[<c0020d40>] (warn_slowpath_fmt) from [<c0144e5c>] (sysfs_warn_dup+0x58/0x74)
[<c0144e5c>] (sysfs_warn_dup) from [<c0145180>] (sysfs_do_create_link_sd+0xbc/0xc4)
[<c0145180>] (sysfs_do_create_link_sd) from [<c02357f4>] (device_add+0x304/0x4ec)
[<c02357f4>] (device_add) from [<c0235a78>] (device_create_groups_vargs+0x9c/0xcc)
[<c0235a78>] (device_create_groups_vargs) from [<c0235af4>] (device_create_vargs+0x20/0x28)
[<c0235af4>] (device_create_vargs) from [<c0235b1c>] (device_create+0x20/0x28)
[<c0235b1c>] (device_create) from [<c020be4c>] (gpiochip_export+0x4c/0x98)
[<c020be4c>] (gpiochip_export) from [<c020a408>] (gpiochip_add+0x114/0x29c)
[<c020a408>] (gpiochip_add) from [<c020c818>] (dwapb_gpio_probe+0x16c/0x634)
[<c020c818>] (dwapb_gpio_probe) from [<c0239198>] (platform_drv_probe+0x2c/0x5c)
[<c0239198>] (platform_drv_probe) from [<c0237bd4>] (driver_probe_device+0x78/0x208)
[<c0237bd4>] (driver_probe_device) from [<c0237df0>] (__driver_attach+0x8c/0x90)
[<c0237df0>] (__driver_attach) from [<c02364b8>] (bus_for_each_dev+0x54/0x88)
[<c02364b8>] (bus_for_each_dev) from [<c023748c>] (bus_add_driver+0xd8/0x1d4)
[<c023748c>] (bus_add_driver) from [<c0238564>] (driver_register+0x78/0xf4)
[<c0238564>] (driver_register) from [<c0008884>] (do_one_initcall+0x80/0x1b8)
[<c0008884>] (do_one_initcall) from [<c052fcd4>] (kernel_init_freeable+0x170/0x240)
[<c052fcd4>] (kernel_init_freeable) from [<c03d0238>] (kernel_init+0x8/0xec)
[<c03d0238>] (kernel_init) from [<c000e578>] (ret_from_fork+0x14/0x3c)
---[ end trace 209e273f37ff62dd ]---
gpiochip_add: GPIOs 0..28 (ff709000.gpio) failed to register
gpio-dwapb ff709000.gpio: failed to register gpiochip for /soc/gpio@ff709000/gpio-controller@0
gpio-dwapb: probe of ff709000.gpio failed with error -17
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at /home/atull/repos/linux-socfpga/fs/sysfs/dir.c:31 sysfs_warn_dup+0x58/0x74()
sysfs: cannot create duplicate filename '/class/gpio/gpiochip0'
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W      3.17.0-rc2-00012-g7ad6a41 #1
[<c0014ab4>] (unwind_backtrace) from [<c0011684>] (show_stack+0x10/0x14)
[<c0011684>] (show_stack) from [<c03d3d80>] (dump_stack+0x74/0x90)
[<c03d3d80>] (dump_stack) from [<c0020c88>] (warn_slowpath_common+0x68/0x8c)
[<c0020c88>] (warn_slowpath_common) from [<c0020d40>] (warn_slowpath_fmt+0x30/0x40)
[<c0020d40>] (warn_slowpath_fmt) from [<c0144e5c>] (sysfs_warn_dup+0x58/0x74)
[<c0144e5c>] (sysfs_warn_dup) from [<c0145180>] (sysfs_do_create_link_sd+0xbc/0xc4)
[<c0145180>] (sysfs_do_create_link_sd) from [<c02357f4>] (device_add+0x304/0x4ec)
[<c02357f4>] (device_add) from [<c0235a78>] (device_create_groups_vargs+0x9c/0xcc)
[<c0235a78>] (device_create_groups_vargs) from [<c0235af4>] (device_create_vargs+0x20/0x28)
[<c0235af4>] (device_create_vargs) from [<c0235b1c>] (device_create+0x20/0x28)
[<c0235b1c>] (device_create) from [<c020be4c>] (gpiochip_export+0x4c/0x98)
[<c020be4c>] (gpiochip_export) from [<c020a408>] (gpiochip_add+0x114/0x29c)
[<c020a408>] (gpiochip_add) from [<c020c818>] (dwapb_gpio_probe+0x16c/0x634)
[<c020c818>] (dwapb_gpio_probe) from [<c0239198>] (platform_drv_probe+0x2c/0x5c)
[<c0239198>] (platform_drv_probe) from [<c0237bd4>] (driver_probe_device+0x78/0x208)
[<c0237bd4>] (driver_probe_device) from [<c0237df0>] (__driver_attach+0x8c/0x90)
[<c0237df0>] (__driver_attach) from [<c02364b8>] (bus_for_each_dev+0x54/0x88)
[<c02364b8>] (bus_for_each_dev) from [<c023748c>] (bus_add_driver+0xd8/0x1d4)
[<c023748c>] (bus_add_driver) from [<c0238564>] (driver_register+0x78/0xf4)
[<c0238564>] (driver_register) from [<c0008884>] (do_one_initcall+0x80/0x1b8)
[<c0008884>] (do_one_initcall) from [<c052fcd4>] (kernel_init_freeable+0x170/0x240)
[<c052fcd4>] (kernel_init_freeable) from [<c03d0238>] (kernel_init+0x8/0xec)
[<c03d0238>] (kernel_init) from [<c000e578>] (ret_from_fork+0x14/0x3c)
---[ end trace 209e273f37ff62de ]---
gpiochip_add: GPIOs 0..26 (ff70a000.gpio) failed to register
gpio-dwapb ff70a000.gpio: failed to register gpiochip for /soc/gpio@ff70a000/gpio-controller@0
gpio-dwapb: probe of ff70a000.gpio failed with error -17

How are you testing this?

Alan

[PATCH 0/3] 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.

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                |  296 +++++++++++++++++++++++++-----
 include/linux/platform_data/gpio-dwapb.h |   32 ++++
 3 files changed, 280 insertions(+), 49 deletions(-)
 create mode 100644 include/linux/platform_data/gpio-dwapb.h

-- 
1.7.9.5

[PATCH 1/3] 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                |  187 ++++++++++++++++++++++--------
 include/linux/platform_data/gpio-dwapb.h |   32 +++++
 3 files changed, 171 insertions(+), 49 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..155a64b 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
@@ -52,14 +53,15 @@ struct dwapb_gpio_port {
 	struct bgpio_chip	bgc;
 	bool			is_registered;
 	struct dwapb_gpio	*gpio;
+	struct dwapb_gpio_port_property *pp;
 };
 
 struct dwapb_gpio {
 	struct	device		*dev;
 	void __iomem		*regs;
 	struct dwapb_gpio_port	*ports;
-	unsigned int		nr_ports;
 	struct irq_domain	*domain;
+	const struct dwapb_gpio_platform_data	*pdata;
 };
 
 static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
@@ -207,22 +209,24 @@ 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)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+
+	dwapb_irq_handler(irq, desc);
+
+	return IRQ_HANDLED;
+}
+
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				 struct dwapb_gpio_port *port)
 {
 	struct gpio_chip *gc = &port->bgc.gc;
-	struct device_node *node =  gc->of_node;
-	struct irq_chip_generic	*irq_gc;
+	struct device_node *node = port->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 +273,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 (!port->pp->irq_shared) {
+		irq_set_chained_handler(port->pp->irq, dwapb_irq_handler);
+	} else {
+		/*
+		 * Request a shared IRQ since where MFD would have devices
+		 * using the same irq pin
+		 */
+		err = devm_request_irq(gpio->dev, port->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;
+		}
+	}
+	irq_set_handler_data(port->pp->irq, gpio);
 
 	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
 		irq_create_mapping(gpio->domain, hwirq);
@@ -296,57 +316,43 @@ 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_gpio_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;
+	port->pp   = pp;
 
-	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;
+	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"))
+	if (pp->irq)
 		dwapb_configure_irqs(gpio, port);
 
 	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;
 
@@ -357,30 +363,115 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 {
 	unsigned int m;
 
-	for (m = 0; m < gpio->nr_ports; ++m)
+	for (m = 0; m < gpio->pdata->nports; ++m)
 		if (gpio->ports[m].is_registered)
 			gpiochip_remove(&gpio->ports[m].bgc.gc);
 }
 
+/*
+ * Handlers for alternative sources of platform_data
+ */
+
+#ifdef CONFIG_OF_GPIO
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static struct dwapb_gpio_platform_data *
+dwapb_gpio_get_pdata_of(struct device *dev)
+{
+	struct device_node *node, *port_np;
+	struct dwapb_gpio_platform_data *pdata;
+	struct dwapb_gpio_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;
+		}
+
+		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	= 0;
+		pp->name	= port_np->full_name;
+	}
+
+	return pdata;
+}
+#else
+static inline struct dwapb_gpio_platform_data *
+dwapb_gpio_get_pdata_of(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
 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;
+	const struct dwapb_gpio_platform_data *pdata = dev_get_platdata(dev);
+
+	if (!pdata) {
+		pdata = dwapb_gpio_get_pdata_of(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
+	if (!pdata->nports)
+		return -ENODEV;
 
 	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
 		return -ENOMEM;
 	gpio->dev = &pdev->dev;
+	gpio->pdata = pdata;
 
-	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, pdata->nports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
 	if (!gpio->ports) {
 		err = -ENOMEM;
@@ -394,8 +485,8 @@ 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 < pdata->nports; i++) {
+		err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i);
 		if (err)
 			goto 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..1f1ce3a
--- /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_gpio_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_gpio_platform_data {
+	struct dwapb_gpio_port_property *properties;
+	unsigned int nports;
+};
+
+#endif
-- 
1.7.9.5

[PATCH 2/3] 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 |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 155a64b..e0ab988 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
@@ -64,6 +65,23 @@ struct dwapb_gpio {
 	const struct dwapb_gpio_platform_data	*pdata;
 };
 
+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);
@@ -209,6 +227,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)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
@@ -342,6 +383,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 
 	port->bgc.gc.ngpio = pp->ngpio;
 	port->bgc.gc.base = pp->gpio_base;
+	port->bgc.gc.set_debounce = dwapb_gpio_set_debounce;
 
 	/*
 	 * Only port A can provide interrupts in all configurations of the IP.
-- 
1.7.9.5

[PATCH 3/3] 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 |   67 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index e0ab988..1055cb3 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -560,10 +560,77 @@ static const struct of_device_id dwapb_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dwapb_of_match);
 
+#if defined CONFIG_PM_SLEEP
+/* Store GPIO context across system-wide suspend/resume transitions */
+static struct gpio_saved_regs {
+	unsigned long data;
+	unsigned long dir;
+	unsigned long int_en;
+	unsigned long int_mask;
+	unsigned long int_type;
+	unsigned long int_pol;
+	unsigned long int_deb;
+} saved_regs;
+
+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;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	saved_regs.int_mask	= dwapb_read(gpio, GPIO_INTMASK);
+	saved_regs.int_en	= dwapb_read(gpio, GPIO_INTEN);
+	saved_regs.int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
+	saved_regs.int_pol	= dwapb_read(gpio, GPIO_INT_POLARITY);
+	saved_regs.int_type	= dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
+	saved_regs.dir		= dwapb_read(gpio, GPIO_SWPORTA_DDR);
+	saved_regs.data		= dwapb_read(gpio, GPIO_SWPORTA_DR);
+
+	/* 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;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
+	dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);
+	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, saved_regs.int_type);
+	dwapb_write(gpio, GPIO_INT_POLARITY, saved_regs.int_pol);
+	dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, saved_regs.int_deb);
+	dwapb_write(gpio, GPIO_INTEN, saved_regs.int_en);
+	dwapb_write(gpio, GPIO_INTMASK, saved_regs.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] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Chen, Alvin]

> 
> Hi Weike,
> 
> I tried out these patches on the current master branch (v3.17-rc2-9-g68e3702)
> with a socfpga cyclone5 board.
> 
> If I apply all 3 patches, the kernel doesn't boot.
> 
> If I rebuild with only the first patch, I get only one gpio block showing up (should
> have 3 for this board) and these messages:
> 
> 
> How are you testing this?
The patches try to not change the current OF flow, and from the log message, the other two GPIO registered failed due to duplicate name.
Let me check the code to see what happen.


> 
> Alan

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

[atull]

On Wed, 27 Aug 2014, 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>
> Signed-off-by: Weike Chen <alvin.chen@intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c |   67 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index e0ab988..1055cb3 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -560,10 +560,77 @@ static const struct of_device_id dwapb_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dwapb_of_match);
>  
> +#if defined CONFIG_PM_SLEEP
> +/* Store GPIO context across system-wide suspend/resume transitions */
> +static struct gpio_saved_regs {
> +	unsigned long data;
> +	unsigned long dir;
> +	unsigned long int_en;
> +	unsigned long int_mask;
> +	unsigned long int_type;
> +	unsigned long int_pol;
> +	unsigned long int_deb;
> +} saved_regs;
> +
> +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;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	saved_regs.int_mask	= dwapb_read(gpio, GPIO_INTMASK);
> +	saved_regs.int_en	= dwapb_read(gpio, GPIO_INTEN);
> +	saved_regs.int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
> +	saved_regs.int_pol	= dwapb_read(gpio, GPIO_INT_POLARITY);
> +	saved_regs.int_type	= dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
> +	saved_regs.dir		= dwapb_read(gpio, GPIO_SWPORTA_DDR);
> +	saved_regs.data		= dwapb_read(gpio, GPIO_SWPORTA_DR);

Hello,

The DesignWare GPIO IP can be configured to have ports a, b, c, and d. So 
you will need to save and restore any ports that are present.  I think 
that *some* configurations of the IP include a register that can tell us 
how it was configured, but that register is also optional :)  I don't have 
confidence that we can read/write the registers blindly whether they are 
known to be there or not.  So you may be stuck with looking at the
device tree or platform data to know whether banks b, c, or d exist.

Alan

> +
> +	/* 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;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
> +	dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);
> +	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, saved_regs.int_type);
> +	dwapb_write(gpio, GPIO_INT_POLARITY, saved_regs.int_pol);
> +	dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, saved_regs.int_deb);
> +	dwapb_write(gpio, GPIO_INTEN, saved_regs.int_en);
> +	dwapb_write(gpio, GPIO_INTMASK, saved_regs.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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[atull]

On Wed, 27 Aug 2014, Weike Chen wrote:

> 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 |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 155a64b..e0ab988 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
> @@ -64,6 +65,23 @@ struct dwapb_gpio {
>  	const struct dwapb_gpio_platform_data	*pdata;
>  };
>  
> +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);
> +}
> +

Hello,

I don't understand the reason for adding dwapb_read and dwapb_write here.  
The rest of the driver is using readl and writel.  I'd rather not see two 
different methods being used in the same driver for register access.  
Maybe I'm missing something, but if we need to add dwapb_read/write, then
it should be used for all register access.

Alan

>  static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> @@ -209,6 +227,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)
>  {
>  	struct irq_desc *desc = irq_to_desc(irq);
> @@ -342,6 +383,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  
>  	port->bgc.gc.ngpio = pp->ngpio;
>  	port->bgc.gc.base = pp->gpio_base;
> +	port->bgc.gc.set_debounce = dwapb_gpio_set_debounce;
>  
>  	/*
>  	 * Only port A can provide interrupts in all configurations of the IP.
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] 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: 1751 bytes --]

On Thu, 2014-08-28 at 10:11 -0500, atull wrote:
> On Wed, 27 Aug 2014, Weike Chen wrote:

[]

> > +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);
> > +}
> > +
> 
> Hello,
> 
> I don't understand the reason for adding dwapb_read and dwapb_write here.  
> The rest of the driver is using readl and writel.  I'd rather not see two 
> different methods being used in the same driver for register access.  
> Maybe I'm missing something, but if we need to add dwapb_read/write, then
> it should be used for all register access.

Alan, it was my proposal. I rather agree that is better to convert all
accesses to that.

-- 
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] 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: 677 bytes --]

> >
> > I don't understand the reason for adding dwapb_read and dwapb_write here.
> > The rest of the driver is using readl and writel.  I'd rather not see
> > two different methods being used in the same driver for register access.
> > Maybe I'm missing something, but if we need to add dwapb_read/write,
> > then it should be used for all register access.
> 
> Alan, it was my proposal. I rather agree that is better to convert all accesses to
> that.
OK, I will convert all readl&writel to dwapb_read&dwapb_write.
ÿôèº{.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] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Chen, Alvin]

> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
> > +	unsigned long data;
> > +	unsigned long dir;
> > +	unsigned long int_en;
> > +	unsigned long int_mask;
> > +	unsigned long int_type;
> > +	unsigned long int_pol;
> > +	unsigned long int_deb;
> > +} saved_regs;
> > +
> > +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;
> > +
> > +	spin_lock_irqsave(&bgc->lock, flags);
> > +
> > +	saved_regs.int_mask	= dwapb_read(gpio, GPIO_INTMASK);
> > +	saved_regs.int_en	= dwapb_read(gpio, GPIO_INTEN);
> > +	saved_regs.int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
> > +	saved_regs.int_pol	= dwapb_read(gpio, GPIO_INT_POLARITY);
> > +	saved_regs.int_type	= dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
> > +	saved_regs.dir		= dwapb_read(gpio, GPIO_SWPORTA_DDR);
> > +	saved_regs.data		= dwapb_read(gpio, GPIO_SWPORTA_DR);
> 
> Hello,
> 
> The DesignWare GPIO IP can be configured to have ports a, b, c, and d. So you
> will need to save and restore any ports that are present.  I think that *some*
> configurations of the IP include a register that can tell us how it was configured,
> but that register is also optional :)  I don't have confidence that we can
> read/write the registers blindly whether they are known to be there or not.
> So you may be stuck with looking at the device tree or platform data to know
> whether banks b, c, or d exist.
>
>From the code, the device node has one property for port index. If the index is '0', then it is port A.
So I think we can store the status according to the port index.

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

[Linus Walleij]

On Wed, Aug 27, 2014 at 7:46 PM, 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>
> Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> Signed-off-by: Weike Chen <alvin.chen@intel.com>

Sebastian, Jamie: can either of you take a look at this patch?

Yours,
Linus Walleij

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

[Sebastian Andrzej Siewior]

On 2014-08-27 10:46:26 [-0700], Weike Chen wrote:
> 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 need either OF_GPIO or PCI

>  	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..155a64b 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
> @@ -52,14 +53,15 @@ struct dwapb_gpio_port {
>  	struct bgpio_chip	bgc;
>  	bool			is_registered;
>  	struct dwapb_gpio	*gpio;
> +	struct dwapb_gpio_port_property *pp;
>  };
>  
>  struct dwapb_gpio {
>  	struct	device		*dev;
>  	void __iomem		*regs;
>  	struct dwapb_gpio_port	*ports;
> -	unsigned int		nr_ports;

you could keep this the way it is

>  	struct irq_domain	*domain;
> +	const struct dwapb_gpio_platform_data	*pdata;

and not making this a member of this struct. I've been going up and down
the source and I don't see the need to marry dwapb_gpio to
dwapb_gpio_platform_data. 
That dwapb_gpio_port_property thing has a long name. Could you just set
it up, pass it for registration and the free it? You can't free the
pdata for the non-OF tree but for the OF case you keep that struct
around for no reason.
You could safe some memory and use pdata directly for setup.

>  };
>  
>  static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> @@ -207,22 +209,24 @@ 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)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	dwapb_irq_handler(irq, desc);
> +
> +	return IRQ_HANDLED;

I suggest to teach dwapb_irq_handler() to return something that makes
you decide whether or not IRQ_HANDLED is the thing to do here. If
something goes crazy the core has no way of knowing and shutting you down.
Also invoking ->irq_eoi() _before_ your shared was invoked might not smart.
What you want is something like

 static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio,  struct irq_chip *chip)
 {
         u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
	 u32 ret = irq_status;
 
         while (irq_status) {
                 int hwirq = fls(irq_status) - 1;
                 int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
 
                 generic_handle_irq(gpio_irq);
                 irq_status &= ~BIT(hwirq);
 
                 if ((irq_get_trigger_type(gpio_irq) & IRQ_TYPE_SENSE_MASK)
                         == IRQ_TYPE_EDGE_BOTH)
                         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, chip);
 
         if (chip->irq_eoi)
                 chip->irq_eoi(irq_desc_get_irq_data(desc));
 }

 static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
	struct irq_chip *chip = irq_desc_get_chip(desc);
	int worked;
 
 	worked = dwapb_irq_handler(dev_id, chip);
	if (worked)
		return IRQ_HANDLED;
	else
		return IRQ_NONE;
 }

How about it?

> +
>  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  				 struct dwapb_gpio_port *port)
>  {
>  	struct gpio_chip *gc = &port->bgc.gc;
> -	struct device_node *node =  gc->of_node;
> -	struct irq_chip_generic	*irq_gc;
> +	struct device_node *node = port->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 +273,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 (!port->pp->irq_shared) {
> +		irq_set_chained_handler(port->pp->irq, dwapb_irq_handler);
> +	} else {
> +		/*
> +		 * Request a shared IRQ since where MFD would have devices
> +		 * using the same irq pin
> +		 */
> +		err = devm_request_irq(gpio->dev, port->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;
> +		}
> +	}
> +	irq_set_handler_data(port->pp->irq, gpio);

This does not look like it belongs here. It should only be used together
with irq_set_chained_handler() or am I missing here something?

>  
>  	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
>  		irq_create_mapping(gpio->domain, hwirq);
> @@ -296,57 +316,43 @@ 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_gpio_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;
> +	port->pp   = pp;
>  
> -	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;
> +	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.
>  	 */
So we had a port_idx check which was refered by the comment. Now we have
a comment and no check for it. You could do that loop over that array
here and keep that check.

> -	if (port_idx == 0 &&
> -	    of_property_read_bool(port_np, "interrupt-controller"))
> +	if (pp->irq)
>  		dwapb_configure_irqs(gpio, port);
>  
>  	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;
>  
> @@ -357,30 +363,115 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>  {
>  	unsigned int m;
>  
> -	for (m = 0; m < gpio->nr_ports; ++m)
> +	for (m = 0; m < gpio->pdata->nports; ++m)
>  		if (gpio->ports[m].is_registered)
>  			gpiochip_remove(&gpio->ports[m].bgc.gc);
>  }
>  
> +/*
> + * Handlers for alternative sources of platform_data
> + */

Those abvious comments…

> +
> +#ifdef CONFIG_OF_GPIO
> +/*
> + * Translate OpenFirmware node properties into platform_data
> + */
… are not really helping

> +static struct dwapb_gpio_platform_data *

Sebastian

RE: [PATCH 1/3] 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: 5915 bytes --]

> 
> > --- 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 need either OF_GPIO or PCI
Since we enable this module not only support OF devices, but also support MFD devices, so we remove OF_GPIO dependency.
For 'PCI', the original code is also not depended on PCI, and this patch also not, do you think it is necessary?

> >
> >  struct dwapb_gpio {
> >  	struct	device		*dev;
> >  	void __iomem		*regs;
> >  	struct dwapb_gpio_port	*ports;
> > -	unsigned int		nr_ports;
> you could keep this the way it is
It has been moved to 'pdata'.

> >  	struct irq_domain	*domain;
> > +	const struct dwapb_gpio_platform_data	*pdata;
> 
> and not making this a member of this struct. I've been going up and down the
> source and I don't see the need to marry dwapb_gpio to
> dwapb_gpio_platform_data.
> That dwapb_gpio_port_property thing has a long name. Could you just set it up,
> pass it for registration and the free it? You can't free the pdata for the non-OF
> tree but for the OF case you keep that struct around for no reason.
> You could safe some memory and use pdata directly for setup.
Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD.
For OF, 'pdata' is getting from device nodes properties. Why do we have such design? Because it can make the handling
more easy for flowing routine. All necessary properties get from 'pdata', never care it is from OF or MFD. And someone
are working on replacing OF interface with a firmware agnostic device_property* interface which will work with both OF and ACPI.
More information for this design, please contact Darren Hart <dvhart@linux.intel.com>. Darren Hart wrote to me:
"Generally speaking, rather than if/else blocks throughout the code checking if it is enumerated via open firmware or as a platform device,
a cleaner approach is to check if the pdata is null, and if so, populate the pdata from the open firmware description if present.
Then use the pdata throughout the driver.

Something to keep in mind is that we are working to replace the of_* interface with a firmware agnostic device_property* interface
which will work with both OF and ACPI. Patches to hit LKML this week for the acpi and driver core. 
Organizing the code as described above will better encapsulate the firmware-config logic and make it easier to update."

And you can also refer the code 'driver/input/keyboard/gpio_keys.c', and this patch takes it as an example.


> >  };
> >
> >  static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> > @@ -207,22 +209,24 @@ 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) {
> > +	struct irq_desc *desc = irq_to_desc(irq);
> > +
> > +	dwapb_irq_handler(irq, desc);
> > +
> > +	return IRQ_HANDLED;
> 
> I suggest to teach dwapb_irq_handler() to return something that makes you
> decide whether or not IRQ_HANDLED is the thing to do here. If something goes
> crazy the core has no way of knowing and shutting you down.
> Also invoking ->irq_eoi() _before_ your shared was invoked might not smart.
> What you want is something like
> 
>  static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio,  struct irq_chip
> *chip)  {
>          u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
> 	 u32 ret = irq_status;
> 
>          while (irq_status) {
>                  int hwirq = fls(irq_status) - 1;
>                  int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
> 
>                  generic_handle_irq(gpio_irq);
>                  irq_status &= ~BIT(hwirq);
> 
>                  if ((irq_get_trigger_type(gpio_irq) &
> IRQ_TYPE_SENSE_MASK)
>                          == IRQ_TYPE_EDGE_BOTH)
>                          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, chip);
> 
>          if (chip->irq_eoi)
>                  chip->irq_eoi(irq_desc_get_irq_data(desc));
>  }
> 
>  static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)  {
>  	struct irq_desc *desc = irq_to_desc(irq);
> 	struct irq_chip *chip = irq_desc_get_chip(desc);
> 	int worked;
> 
>  	worked = dwapb_irq_handler(dev_id, chip);
> 	if (worked)
> 		return IRQ_HANDLED;
> 	else
> 		return IRQ_NONE;
>  }
> 
> How about it?
OK, I will improve it as your suggestion.


> > +	irq_set_handler_data(port->pp->irq, gpio);
> 
> This does not look like it belongs here. It should only be used together with
> irq_set_chained_handler() or am I missing here something?
This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' irq_set_handler_data', it just sets the irq data.
Why do you think it must be used together with ' irq_set_chained_handler'?

> >  	/*
> >  	 * Only port A can provide interrupts in all configurations of the IP.
> >  	 */
> So we had a port_idx check which was refered by the comment. Now we have a
> comment and no check for it. You could do that loop over that array here and
> keep that check.
Let me move it to 'dwapb_gpio_get_pdata_of'.

> >
> > +/*
> > + * Handlers for alternative sources of platform_data  */
> 
> Those abvious comments…
> 
> > +
> > +#ifdef CONFIG_OF_GPIO
> > +/*
> > + * Translate OpenFirmware node properties into platform_data  */
> … are not really helping
Let me remove them.

> > +static struct dwapb_gpio_platform_data *
> 
> Sebastian
ÿôèº{.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] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

[Sebastian Andrzej Siewior]

On 2014-09-04 02:00:03 [+0000], Chen, Alvin wrote:
> > 
> > > --- 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 need either OF_GPIO or PCI
> Since we enable this module not only support OF devices, but also support MFD devices, so we remove OF_GPIO dependency.
> For 'PCI', the original code is also not depended on PCI, and this patch also not, do you think it is necessary?

if not PCI then you should add something likwe 
	"depends on OF_GPIO || MFD"

looking further, you need also HAS_IOMEM for things like
devm_ioremap_resource(). Linus, wouldn't it make sense to group this
driver and make the block depend on it?

> > >  struct dwapb_gpio {
> > >  	struct	device		*dev;
> > >  	void __iomem		*regs;
> > >  	struct dwapb_gpio_port	*ports;
> > > -	unsigned int		nr_ports;
> > you could keep this the way it is
> It has been moved to 'pdata'.

I saw that. But there is no need keep a pointer to pdata across the
whole driver since only need nr_ports in the driver removal part of the
whole driver.

> > >  	struct irq_domain	*domain;
> > > +	const struct dwapb_gpio_platform_data	*pdata;
> > 
> > and not making this a member of this struct. I've been going up and down the
> > source and I don't see the need to marry dwapb_gpio to
> > dwapb_gpio_platform_data.
> > That dwapb_gpio_port_property thing has a long name. Could you just set it up,
> > pass it for registration and the free it? You can't free the pdata for the non-OF
> > tree but for the OF case you keep that struct around for no reason.
> > You could safe some memory and use pdata directly for setup.
> Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD.
> For OF, 'pdata' is getting from device nodes properties. Why do we have such design? Because it can make the handling
> more easy for flowing routine. All necessary properties get from 'pdata', never care it is from OF or MFD. And someone
> are working on replacing OF interface with a firmware agnostic device_property* interface which will work with both OF and ACPI.
> More information for this design, please contact Darren Hart <dvhart@linux.intel.com>. Darren Hart wrote to me:
> "Generally speaking, rather than if/else blocks throughout the code checking if it is enumerated via open firmware or as a platform device,
> a cleaner approach is to check if the pdata is null, and if so, populate the pdata from the open firmware description if present.
> Then use the pdata throughout the driver.

But isn't it enough to hold on to this pdata thing through the probe
function only? After probe is done you could drop that memory in the
OF-case, right?
 
> > > +	irq_set_handler_data(port->pp->irq, gpio);
> > 
> > This does not look like it belongs here. It should only be used together with
> > irq_set_chained_handler() or am I missing here something?
> This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' irq_set_handler_data', it just sets the irq data.
> Why do you think it must be used together with ' irq_set_chained_handler'?

because you do request_irq(…, driver_data). If you you look close to
irq_set_chained_handler() it does not have such a member. Thus it uses
irq_set_handler_data() for that same purpose.

Sebastian

RE: [PATCH 1/3] 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: 3813 bytes --]


> > Since we enable this module not only support OF devices, but also support
> MFD devices, so we remove OF_GPIO dependenc
> > For 'PCI', the original code is also not depended on PCI, and this patch also
> not, do you think it is necessary?
> 
> if not PCI then you should add something likwe
> 	"depends on OF_GPIO || MFD"
> 
> looking further, you need also HAS_IOMEM for things like
> devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver
> and make the block depend on it?
> 
I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD path only support Intel Quark.

Andriy, how do you think?

> > > >  struct dwapb_gpio {
> > > >  	struct	device		*dev;
> > > >  	void __iomem		*regs;
> > > >  	struct dwapb_gpio_port	*ports;
> > > > -	unsigned int		nr_ports;
> > > you could keep this the way it is
> > It has been moved to 'pdata'.
> 
> I saw that. But there is no need keep a pointer to pdata across the whole driver
> since only need nr_ports in the driver removal part of the whole driver.
Got your idea. So can I just use a global static pdata pointer instead of adding it as a member of ' struct dwapb_gpio'?
Since pdata is used in all 'probe' functions, and make pdata as global static make programming more easy.

> 
> > > >  	struct irq_domain	*domain;
> > > > +	const struct dwapb_gpio_platform_data	*pdata;
> > >
> > > and not making this a member of this struct. I've been going up and
> > > down the source and I don't see the need to marry dwapb_gpio to
> > > dwapb_gpio_platform_data.
> > > That dwapb_gpio_port_property thing has a long name. Could you just
> > > set it up, pass it for registration and the free it? You can't free
> > > the pdata for the non-OF tree but for the OF case you keep that struct
> around for no reason.
> > > You could safe some memory and use pdata directly for setup.
> > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from
> MFD.
> > For OF, 'pdata' is getting from device nodes properties. Why do we
> > have such design? Because it can make the handling more easy for
> > flowing routine. All necessary properties get from 'pdata', never care it is
> from OF or MFD. And someone are working on replacing OF interface with a
> firmware agnostic device_property* interface which will work with both OF and
> ACPI.
> > More information for this design, please contact Darren Hart
> <dvhart@linux.intel.com>. Darren Hart wrote to me:
> > "Generally speaking, rather than if/else blocks throughout the code
> > checking if it is enumerated via open firmware or as a platform device, a
> cleaner approach is to check if the pdata is null, and if so, populate the pdata
> from the open firmware description if present.
> > Then use the pdata throughout the driver.
> 
> But isn't it enough to hold on to this pdata thing through the probe function
> only? After probe is done you could drop that memory in the OF-case, right?
OK. If it is OF-case, pdata can be freed in the end of probe.


> > > > +	irq_set_handler_data(port->pp->irq, gpio);
> > >
> > > This does not look like it belongs here. It should only be used
> > > together with
> > > irq_set_chained_handler() or am I missing here something?
> > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
> irq_set_handler_data', it just sets the irq data.
> > Why do you think it must be used together with ' irq_set_chained_handler'?
> 
> because you do request_irq(…, driver_data). If you you look close to
> irq_set_chained_handler() it does not have such a member. Thus it uses
> irq_set_handler_data() for that same purpose.
> 
OK. I can improve it.
ÿôèº{.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] 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: 4747 bytes --]

On Thu, 2014-09-04 at 10:38 +0000, Chen, Alvin wrote:
> 
> > > Since we enable this module not only support OF devices, but also support
> > MFD devices, so we remove OF_GPIO dependenc
> > > For 'PCI', the original code is also not depended on PCI, and this patch also
> > not, do you think it is necessary?
> > 
> > if not PCI then you should add something likwe
> > 	"depends on OF_GPIO || MFD"
> > 
> > looking further, you need also HAS_IOMEM for things like
> > devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver
> > and make the block depend on it?
> > 
> I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD path only support Intel Quark.
> 
> Andriy, how do you think?

I think we don't need that dependency at all since OF_GPIO has stubs for
non-OF case. Am I missing something?

GPIOLIB dependency is implied in Kconfig, by the way.

P.S. See, for example, leds-gpio.c

> 
> > > > >  struct dwapb_gpio {
> > > > >  	struct	device		*dev;
> > > > >  	void __iomem		*regs;
> > > > >  	struct dwapb_gpio_port	*ports;
> > > > > -	unsigned int		nr_ports;
> > > > you could keep this the way it is
> > > It has been moved to 'pdata'.
> > 
> > I saw that. But there is no need keep a pointer to pdata across the whole driver
> > since only need nr_ports in the driver removal part of the whole driver.
> Got your idea. So can I just use a global static pdata pointer instead of adding it as a member of ' struct dwapb_gpio'?
> Since pdata is used in all 'probe' functions, and make pdata as global static make programming more easy.
> 
> > 
> > > > >  	struct irq_domain	*domain;
> > > > > +	const struct dwapb_gpio_platform_data	*pdata;
> > > >
> > > > and not making this a member of this struct. I've been going up and
> > > > down the source and I don't see the need to marry dwapb_gpio to
> > > > dwapb_gpio_platform_data.
> > > > That dwapb_gpio_port_property thing has a long name. Could you just
> > > > set it up, pass it for registration and the free it? You can't free
> > > > the pdata for the non-OF tree but for the OF case you keep that struct
> > around for no reason.
> > > > You could safe some memory and use pdata directly for setup.
> > > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from
> > MFD.
> > > For OF, 'pdata' is getting from device nodes properties. Why do we
> > > have such design? Because it can make the handling more easy for
> > > flowing routine. All necessary properties get from 'pdata', never care it is
> > from OF or MFD. And someone are working on replacing OF interface with a
> > firmware agnostic device_property* interface which will work with both OF and
> > ACPI.
> > > More information for this design, please contact Darren Hart
> > <dvhart@linux.intel.com>. Darren Hart wrote to me:
> > > "Generally speaking, rather than if/else blocks throughout the code
> > > checking if it is enumerated via open firmware or as a platform device, a
> > cleaner approach is to check if the pdata is null, and if so, populate the pdata
> > from the open firmware description if present.
> > > Then use the pdata throughout the driver.
> > 
> > But isn't it enough to hold on to this pdata thing through the probe function
> > only? After probe is done you could drop that memory in the OF-case, right?
> OK. If it is OF-case, pdata can be freed in the end of probe.
> 
> 
> > > > > +	irq_set_handler_data(port->pp->irq, gpio);
> > > >
> > > > This does not look like it belongs here. It should only be used
> > > > together with
> > > > irq_set_chained_handler() or am I missing here something?
> > > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
> > irq_set_handler_data', it just sets the irq data.
> > > Why do you think it must be used together with ' irq_set_chained_handler'?
> > 
> > because you do request_irq(…, driver_data). If you you look close to
> > irq_set_chained_handler() it does not have such a member. Thus it uses
> > irq_set_handler_data() for that same purpose.
> > 
> OK. I can improve it.


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

[Linus Walleij]

On Wed, Aug 27, 2014 at 7:46 PM, 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>

(...)

> +#if defined CONFIG_PM_SLEEP

I wonder whether it's worth #ifdef:in out such things, it clutters
the place.

> +/* Store GPIO context across system-wide suspend/resume transitions */
> +static struct gpio_saved_regs {

Call the struct:

struct dwapb_context

because that is easier to understand.

> +       unsigned long data;
> +       unsigned long dir;
> +       unsigned long int_en;
> +       unsigned long int_mask;
> +       unsigned long int_type;
> +       unsigned long int_pol;
> +       unsigned long int_deb;
> +} saved_regs;

Singleton huh?

Insert this into the dynamically allocated per-port or chip struct
instead.

+       dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
+       dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);

And port B, C, D?

This looks like a crude hack.

Yours,
Linus Walleij

RE: [PATCH 3/3] 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: 1525 bytes --]

> 
> > +#if defined CONFIG_PM_SLEEP
> 
> I wonder whether it's worth #ifdef:in out such things, it clutters the place.
OK. I will use '#ifdef'.
> 
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
> 
> Call the struct:
> 
> struct dwapb_context
> 
> because that is easier to understand.
> 
OK.

> > +       unsigned long data;
> > +       unsigned long dir;
> > +       unsigned long int_en;
> > +       unsigned long int_mask;
> > +       unsigned long int_type;
> > +       unsigned long int_pol;
> > +       unsigned long int_deb;
> > +} saved_regs;
> 
> Singleton huh?
> 
> Insert this into the dynamically allocated per-port or chip struct instead.
> 
How about the following?

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;

Since only portA can support irq, and the irq related registers are only for portA. Comparing to allocate for each port
dynamically, it is more directly and easy to understand. 


> +       dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
> +       dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);
> 
> And port B, C, D?
> 
> This looks like a crude hack.
I will add port B, C, D.
> 
> Yours,
> Linus Walleij
ÿôèº{.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] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Linus Walleij]

On Fri, Sep 5, 2014 at 4:09 AM, Chen, Alvin <alvin.chen@intel.com> wrote:

>> > +       unsigned long data;
>> > +       unsigned long dir;
>> > +       unsigned long int_en;
>> > +       unsigned long int_mask;
>> > +       unsigned long int_type;
>> > +       unsigned long int_pol;
>> > +       unsigned long int_deb;
>> > +} saved_regs;
>>
>> Singleton huh?
>>
>> Insert this into the dynamically allocated per-port or chip struct instead.
>>
> How about the following?
>
> 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;

NO because this is still a singleton variable. Put it into the
dynamically allocated structs.

> Comparing to allocate for each port
> dynamically, it is more directly and easy to understand.

No, I disagree. The overall design pattern in the kernel is to
allocate all state containers dynamically.

Yours,
Linus Walleij

RE: [PATCH 3/3] 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: 1044 bytes --]

> >>
> >> Insert this into the dynamically allocated per-port or chip struct instead.
> >>
> > How about the following?
> >
> > 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;
> 
> NO because this is still a singleton variable. Put it into the dynamically allocated
> structs.
> 
> > Comparing to allocate for each port
> > dynamically, it is more directly and easy to understand.
> 
> No, I disagree. The overall design pattern in the kernel is to allocate all state
> containers dynamically.
> 
OK. Please also help review the v2 I just sent out, and after getting more feedbacks, I will improve this part in the next version together.


ÿôèº{.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¥