[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Robert Jarzmik]

Add a binding for lubbock motherboard IO board.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt

diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
new file mode 100644
index 0000000..33c9e21
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
@@ -0,0 +1,26 @@
+Intel's pxa255 system development platform motherboard.
+
+This is the motherboard, or IO board, of the pxa25x development system,
+supporting a lubbock pxa25x SoC board.
+
+Required properties:
+  - compatible : One of the following chip-specific strings:
+        "intel,lubbock-io"
+  - interrupts : The first interrupt is the line the /IRQ signal the IO board
+                 multiplex is connected to. The only known case is GPIO0 on the
+                 pxa25x SoC.
+
+Optional properties:
+  - interrupts : The second optional interrupt is the base of the interrupts
+                 multiplexed by the lubbock motherboard. If unspecified, the
+                 range is fully dynamic, and the irqdomain will generate its
+                 interrupt base on the fly.
+
+Example:
+
+mb: lubbock-mb@0 {
+	compatible = "intel,lubbock-io";
+	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
+	#interrupt-cells = <2>;
+        interrupt-parent = <&pxairq>;
+};
-- 
2.1.0

[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Robert Jarzmik]

Add a binding for lubbock motherboard IO board.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt

diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
new file mode 100644
index 0000000..33c9e21
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
@@ -0,0 +1,26 @@
+Intel's pxa255 system development platform motherboard.
+
+This is the motherboard, or IO board, of the pxa25x development system,
+supporting a lubbock pxa25x SoC board.
+
+Required properties:
+  - compatible : One of the following chip-specific strings:
+        "intel,lubbock-io"
+  - interrupts : The first interrupt is the line the /IRQ signal the IO board
+                 multiplex is connected to. The only known case is GPIO0 on the
+                 pxa25x SoC.
+
+Optional properties:
+  - interrupts : The second optional interrupt is the base of the interrupts
+                 multiplexed by the lubbock motherboard. If unspecified, the
+                 range is fully dynamic, and the irqdomain will generate its
+                 interrupt base on the fly.
+
+Example:
+
+mb: lubbock-mb at 0 {
+	compatible = "intel,lubbock-io";
+	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
+	#interrupt-cells = <2>;
+        interrupt-parent = <&pxairq>;
+};
-- 
2.1.0

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lubbock () board is the IO motherboard of the Intel PXA25x Development
Platform, which supports the Lubbock pxa25x soc board.

Historically, this support was in arch/arm/mach-pxa/lubbock.c. When
gpio-pxa was moved to drivers/pxa, it became a driver, and its
initialization and probing happened at postcore initcall. The lubbock
code used to install the chained lubbock interrupt handler at init_irq()
time.

The consequence of the gpio-pxa change is that the installed chained irq
handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
removing :
 - the handler
 - the falling edge detection setting of GPIO0, which revealed the
   interrupt request from the lubbock IO board.

As a fix, move the gpio0 chained handler setup to a place where we have
the guarantee that pxa_gpio_probe() was called before, so that lubbock
handler becomes the true IRQ chained handler of GPIO0, demuxing the
lubbock IO board interrupts.

This patch moves all that handling to a mfd driver. It's only purpose
for the time being is the interrupt handling, but in the future it
should encompass all the motherboard CPLDs handling :
 - leds
 - switches
 - hexleds

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: change the name from cottula to lubbock_io
            Dmitry pointed out the Cottula was the pxa25x family name,
	    lubbock was the pxa25x development board name. Therefore the
	    name was changed to lubbock_io (lubbock IO board)
	  change the resources to bi-irq ioresource
	    Discussion between Arnd and Robert to change the gpio
	    request by a irq request.
Since v2: take into account Mark's review
      	    Use irq flags from resources (DT case and pdata case).
	    Change of name from lubbock_io to lubbock-io
---
 drivers/mfd/Kconfig   |  10 +++
 drivers/mfd/Makefile  |   1 +
 drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 drivers/mfd/lubbock.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b731..4d8939f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -91,6 +91,16 @@ config MFD_AXP20X
 	  components like regulators or the PEK (Power Enable Key) under the
 	  corresponding menus.
 
+config MFD_LUBBOCK
+	bool "Lubbock Motherboard"
+	def_bool ARCH_LUBBOCK
+	select MFD_CORE
+	help
+	  This driver supports the Lubbock multifunction chip found on the
+	  pxa25x development platform system (named Lubbock). This IO board
+	  supports the interrupts handling, ethernet controller, flash chips,
+	  etc ...
+
 config MFD_CROS_EC
 	tristate "ChromeOS Embedded Controller"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e2..aff1f4f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
+obj-$(CONFIG_MFD_LUBBOCK)	+= lubbock.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c
new file mode 100644
index 0000000..6025135
--- /dev/null
+++ b/drivers/mfd/lubbock.c
@@ -0,0 +1,196 @@
+/*
+ * Intel Cotulla MFD - lubbock motherboard
+ *
+ * Copyright (C) 2014 Robert Jarzmik
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+
+#define COT_IRQ_MASK_EN 0xc0
+#define COT_IRQ_SET_CLR 0xd0
+
+#define LUBBOCK_NB_IRQ	8
+
+struct lubbock {
+	void __iomem	*base;
+	int irq;
+	unsigned int irq_mask;
+	struct gpio_desc *gpio0;
+	struct irq_domain *irqdomain;
+};
+
+static irqreturn_t lubbock_irq_handler(int in_irq, void *d)
+{
+	struct lubbock *cot = d;
+	unsigned long pending;
+	unsigned int bit;
+
+	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
+	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
+		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));
+	return IRQ_HANDLED;
+}
+
+static void lubbock_irq_mask_ack(struct irq_data *d)
+{
+	struct lubbock *cot = irq_data_get_irq_chip_data(d);
+	unsigned int lubbock_irq = irqd_to_hwirq(d);
+	unsigned int set, bit = BIT(lubbock_irq);
+
+	cot->irq_mask &= ~bit;
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	set = readl(cot->base + COT_IRQ_SET_CLR);
+	writel(set & ~bit, cot->base + COT_IRQ_SET_CLR);
+}
+
+static void lubbock_irq_unmask(struct irq_data *d)
+{
+	struct lubbock *cot = irq_data_get_irq_chip_data(d);
+	unsigned int lubbock_irq = irqd_to_hwirq(d);
+	unsigned int bit = BIT(lubbock_irq);
+
+	cot->irq_mask |= bit;
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+}
+
+static struct irq_chip lubbock_irq_chip = {
+	.name		= "lubbock",
+	.irq_mask_ack	= lubbock_irq_mask_ack,
+	.irq_unmask	= lubbock_irq_unmask,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct lubbock *cot = d->host_data;
+
+	irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, cot);
+	return 0;
+}
+
+static const struct irq_domain_ops lubbock_irq_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.map = lubbock_irq_domain_map,
+};
+
+static int lubbock_resume(struct platform_device *pdev)
+{
+	struct lubbock *cot = platform_get_drvdata(pdev);
+
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	return 0;
+}
+
+static int lubbock_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct lubbock *cot;
+	int ret;
+	unsigned int base_irq = 0;
+	unsigned long irqflags;
+
+	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
+	if (!cot)
+		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res) {
+		cot->irq = (unsigned int)res->start;
+		irqflags = res->flags;
+	}
+	if (!cot->irq)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+	if (res)
+		base_irq = (unsigned int)res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cot->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(cot->base))
+		return PTR_ERR(cot->base);
+
+	platform_set_drvdata(pdev, cot);
+
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	writel(0, cot->base + COT_IRQ_SET_CLR);
+	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
+			       irqflags, dev_name(&pdev->dev), cot);
+	if (ret == -ENOSYS)
+		return -EPROBE_DEFER;
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
+			ret);
+		return ret;
+	}
+	irq_set_irq_wake(cot->irq, 1);
+
+	cot->irqdomain =
+		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
+				      &lubbock_irq_domain_ops, cot);
+	if (!cot->irqdomain)
+		return -ENODEV;
+
+	ret = 0;
+	if (base_irq)
+		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
+						 LUBBOCK_NB_IRQ);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
+			base_irq, base_irq + LUBBOCK_NB_IRQ);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
+		 cot->base, cot->irq, base_irq);
+
+	return 0;
+}
+
+static int lubbock_remove(struct platform_device *pdev)
+{
+	struct lubbock *cot = platform_get_drvdata(pdev);
+
+	irq_set_chip_and_handler(cot->irq, NULL, NULL);
+	return 0;
+}
+
+static const struct of_device_id lubbock_id_table[] = {
+	{ .compatible = "intel,lubbock-io", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lubbock_id_table);
+
+static struct platform_driver lubbock_driver = {
+	.driver		= {
+		.name	= "lubbock_io",
+		.of_match_table = of_match_ptr(lubbock_id_table),
+	},
+	.probe		= lubbock_probe,
+	.remove		= lubbock_remove,
+	.resume		= lubbock_resume,
+};
+
+module_platform_driver(lubbock_driver);
+
+MODULE_DESCRIPTION("Lubbock driver");
+MODULE_AUTHOR("Robert Jarzmik");
+MODULE_LICENSE("GPL");
-- 
2.1.0

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lubbock () board is the IO motherboard of the Intel PXA25x Development
Platform, which supports the Lubbock pxa25x soc board.

Historically, this support was in arch/arm/mach-pxa/lubbock.c. When
gpio-pxa was moved to drivers/pxa, it became a driver, and its
initialization and probing happened at postcore initcall. The lubbock
code used to install the chained lubbock interrupt handler at init_irq()
time.

The consequence of the gpio-pxa change is that the installed chained irq
handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
removing :
 - the handler
 - the falling edge detection setting of GPIO0, which revealed the
   interrupt request from the lubbock IO board.

As a fix, move the gpio0 chained handler setup to a place where we have
the guarantee that pxa_gpio_probe() was called before, so that lubbock
handler becomes the true IRQ chained handler of GPIO0, demuxing the
lubbock IO board interrupts.

This patch moves all that handling to a mfd driver. It's only purpose
for the time being is the interrupt handling, but in the future it
should encompass all the motherboard CPLDs handling :
 - leds
 - switches
 - hexleds

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: change the name from cottula to lubbock_io
            Dmitry pointed out the Cottula was the pxa25x family name,
	    lubbock was the pxa25x development board name. Therefore the
	    name was changed to lubbock_io (lubbock IO board)
	  change the resources to bi-irq ioresource
	    Discussion between Arnd and Robert to change the gpio
	    request by a irq request.
Since v2: take into account Mark's review
      	    Use irq flags from resources (DT case and pdata case).
	    Change of name from lubbock_io to lubbock-io
---
 drivers/mfd/Kconfig   |  10 +++
 drivers/mfd/Makefile  |   1 +
 drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 drivers/mfd/lubbock.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b731..4d8939f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -91,6 +91,16 @@ config MFD_AXP20X
 	  components like regulators or the PEK (Power Enable Key) under the
 	  corresponding menus.
 
+config MFD_LUBBOCK
+	bool "Lubbock Motherboard"
+	def_bool ARCH_LUBBOCK
+	select MFD_CORE
+	help
+	  This driver supports the Lubbock multifunction chip found on the
+	  pxa25x development platform system (named Lubbock). This IO board
+	  supports the interrupts handling, ethernet controller, flash chips,
+	  etc ...
+
 config MFD_CROS_EC
 	tristate "ChromeOS Embedded Controller"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e2..aff1f4f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
+obj-$(CONFIG_MFD_LUBBOCK)	+= lubbock.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c
new file mode 100644
index 0000000..6025135
--- /dev/null
+++ b/drivers/mfd/lubbock.c
@@ -0,0 +1,196 @@
+/*
+ * Intel Cotulla MFD - lubbock motherboard
+ *
+ * Copyright (C) 2014 Robert Jarzmik
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+
+#define COT_IRQ_MASK_EN 0xc0
+#define COT_IRQ_SET_CLR 0xd0
+
+#define LUBBOCK_NB_IRQ	8
+
+struct lubbock {
+	void __iomem	*base;
+	int irq;
+	unsigned int irq_mask;
+	struct gpio_desc *gpio0;
+	struct irq_domain *irqdomain;
+};
+
+static irqreturn_t lubbock_irq_handler(int in_irq, void *d)
+{
+	struct lubbock *cot = d;
+	unsigned long pending;
+	unsigned int bit;
+
+	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
+	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
+		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));
+	return IRQ_HANDLED;
+}
+
+static void lubbock_irq_mask_ack(struct irq_data *d)
+{
+	struct lubbock *cot = irq_data_get_irq_chip_data(d);
+	unsigned int lubbock_irq = irqd_to_hwirq(d);
+	unsigned int set, bit = BIT(lubbock_irq);
+
+	cot->irq_mask &= ~bit;
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	set = readl(cot->base + COT_IRQ_SET_CLR);
+	writel(set & ~bit, cot->base + COT_IRQ_SET_CLR);
+}
+
+static void lubbock_irq_unmask(struct irq_data *d)
+{
+	struct lubbock *cot = irq_data_get_irq_chip_data(d);
+	unsigned int lubbock_irq = irqd_to_hwirq(d);
+	unsigned int bit = BIT(lubbock_irq);
+
+	cot->irq_mask |= bit;
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+}
+
+static struct irq_chip lubbock_irq_chip = {
+	.name		= "lubbock",
+	.irq_mask_ack	= lubbock_irq_mask_ack,
+	.irq_unmask	= lubbock_irq_unmask,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct lubbock *cot = d->host_data;
+
+	irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, cot);
+	return 0;
+}
+
+static const struct irq_domain_ops lubbock_irq_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.map = lubbock_irq_domain_map,
+};
+
+static int lubbock_resume(struct platform_device *pdev)
+{
+	struct lubbock *cot = platform_get_drvdata(pdev);
+
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	return 0;
+}
+
+static int lubbock_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct lubbock *cot;
+	int ret;
+	unsigned int base_irq = 0;
+	unsigned long irqflags;
+
+	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
+	if (!cot)
+		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res) {
+		cot->irq = (unsigned int)res->start;
+		irqflags = res->flags;
+	}
+	if (!cot->irq)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+	if (res)
+		base_irq = (unsigned int)res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cot->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(cot->base))
+		return PTR_ERR(cot->base);
+
+	platform_set_drvdata(pdev, cot);
+
+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	writel(0, cot->base + COT_IRQ_SET_CLR);
+	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
+			       irqflags, dev_name(&pdev->dev), cot);
+	if (ret == -ENOSYS)
+		return -EPROBE_DEFER;
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
+			ret);
+		return ret;
+	}
+	irq_set_irq_wake(cot->irq, 1);
+
+	cot->irqdomain =
+		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
+				      &lubbock_irq_domain_ops, cot);
+	if (!cot->irqdomain)
+		return -ENODEV;
+
+	ret = 0;
+	if (base_irq)
+		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
+						 LUBBOCK_NB_IRQ);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
+			base_irq, base_irq + LUBBOCK_NB_IRQ);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
+		 cot->base, cot->irq, base_irq);
+
+	return 0;
+}
+
+static int lubbock_remove(struct platform_device *pdev)
+{
+	struct lubbock *cot = platform_get_drvdata(pdev);
+
+	irq_set_chip_and_handler(cot->irq, NULL, NULL);
+	return 0;
+}
+
+static const struct of_device_id lubbock_id_table[] = {
+	{ .compatible = "intel,lubbock-io", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lubbock_id_table);
+
+static struct platform_driver lubbock_driver = {
+	.driver		= {
+		.name	= "lubbock_io",
+		.of_match_table = of_match_ptr(lubbock_id_table),
+	},
+	.probe		= lubbock_probe,
+	.remove		= lubbock_remove,
+	.resume		= lubbock_resume,
+};
+
+module_platform_driver(lubbock_driver);
+
+MODULE_DESCRIPTION("Lubbock driver");
+MODULE_AUTHOR("Robert Jarzmik");
+MODULE_LICENSE("GPL");
-- 
2.1.0

[PATCH v3 3/3] ARM: pxa: lubbock: use new lubbock_io driver

[Robert Jarzmik]

As the interrupt handling was transferred to the lubbock_io driver, make
the switch in lubbock platform code.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: change the name from cottula to lubbock_io
            Dmitry pointed out the Cottula was the pxa25x family name,
	    lubbock was the pxa25x development board name. Therefore the
	    name was changed to lubbock_io (lubbock IO board)
	  change the resources to bi-irq ioresource
	    Discussion between Arnd and Robert to change the gpio
	    request by a irq request.
---
 arch/arm/mach-pxa/include/mach/lubbock.h |   7 +-
 arch/arm/mach-pxa/lubbock.c              | 108 +++++++++----------------------
 2 files changed, 33 insertions(+), 82 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/lubbock.h b/arch/arm/mach-pxa/include/mach/lubbock.h
index 958cd6af..f602e6a 100644
--- a/arch/arm/mach-pxa/include/mach/lubbock.h
+++ b/arch/arm/mach-pxa/include/mach/lubbock.h
@@ -37,7 +37,9 @@
 #define LUB_GP			__LUB_REG(LUBBOCK_FPGA_PHYS + 0x100)
 
 /* Board specific IRQs */
-#define LUBBOCK_IRQ(x)		(IRQ_BOARD_START + (x))
+#define LUBBOCK_NR_IRQS		IRQ_BOARD_START
+
+#define LUBBOCK_IRQ(x)		(LUBBOCK_NR_IRQS + (x))
 #define LUBBOCK_SD_IRQ		LUBBOCK_IRQ(0)
 #define LUBBOCK_SA1111_IRQ	LUBBOCK_IRQ(1)
 #define LUBBOCK_USB_IRQ		LUBBOCK_IRQ(2)  /* usb connect */
@@ -47,8 +49,7 @@
 #define LUBBOCK_USB_DISC_IRQ	LUBBOCK_IRQ(6)  /* usb disconnect */
 #define LUBBOCK_LAST_IRQ	LUBBOCK_IRQ(6)
 
-#define LUBBOCK_SA1111_IRQ_BASE	(IRQ_BOARD_START + 16)
-#define LUBBOCK_NR_IRQS		(IRQ_BOARD_START + 16 + 55)
+#define LUBBOCK_SA1111_IRQ_BASE	(LUBBOCK_NR_IRQS + 16)
 
 #ifndef __ASSEMBLY__
 extern void lubbock_set_misc_wr(unsigned int mask, unsigned int set);
diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index d8a1be6..4ef957a 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -12,6 +12,7 @@
  *  published by the Free Software Foundation.
  */
 #include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -123,84 +124,6 @@ void lubbock_set_misc_wr(unsigned int mask, unsigned int set)
 }
 EXPORT_SYMBOL(lubbock_set_misc_wr);
 
-static unsigned long lubbock_irq_enabled;
-
-static void lubbock_mask_irq(struct irq_data *d)
-{
-	int lubbock_irq = (d->irq - LUBBOCK_IRQ(0));
-	LUB_IRQ_MASK_EN = (lubbock_irq_enabled &= ~(1 << lubbock_irq));
-}
-
-static void lubbock_unmask_irq(struct irq_data *d)
-{
-	int lubbock_irq = (d->irq - LUBBOCK_IRQ(0));
-	/* the irq can be acknowledged only if deasserted, so it's done here */
-	LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq);
-	LUB_IRQ_MASK_EN = (lubbock_irq_enabled |= (1 << lubbock_irq));
-}
-
-static struct irq_chip lubbock_irq_chip = {
-	.name		= "FPGA",
-	.irq_ack	= lubbock_mask_irq,
-	.irq_mask	= lubbock_mask_irq,
-	.irq_unmask	= lubbock_unmask_irq,
-};
-
-static void lubbock_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	unsigned long pending = LUB_IRQ_SET_CLR & lubbock_irq_enabled;
-	do {
-		/* clear our parent irq */
-		desc->irq_data.chip->irq_ack(&desc->irq_data);
-		if (likely(pending)) {
-			irq = LUBBOCK_IRQ(0) + __ffs(pending);
-			generic_handle_irq(irq);
-		}
-		pending = LUB_IRQ_SET_CLR & lubbock_irq_enabled;
-	} while (pending);
-}
-
-static void __init lubbock_init_irq(void)
-{
-	int irq;
-
-	pxa25x_init_irq();
-
-	/* setup extra lubbock irqs */
-	for (irq = LUBBOCK_IRQ(0); irq <= LUBBOCK_LAST_IRQ; irq++) {
-		irq_set_chip_and_handler(irq, &lubbock_irq_chip,
-					 handle_level_irq);
-		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
-	}
-
-	irq_set_chained_handler(PXA_GPIO_TO_IRQ(0), lubbock_irq_handler);
-	irq_set_irq_type(PXA_GPIO_TO_IRQ(0), IRQ_TYPE_EDGE_FALLING);
-}
-
-#ifdef CONFIG_PM
-
-static void lubbock_irq_resume(void)
-{
-	LUB_IRQ_MASK_EN = lubbock_irq_enabled;
-}
-
-static struct syscore_ops lubbock_irq_syscore_ops = {
-	.resume = lubbock_irq_resume,
-};
-
-static int __init lubbock_irq_device_init(void)
-{
-	if (machine_is_lubbock()) {
-		register_syscore_ops(&lubbock_irq_syscore_ops);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-device_initcall(lubbock_irq_device_init);
-
-#endif
-
 static int lubbock_udc_is_connected(void)
 {
 	return (LUB_MISC_RD & (1 << 9)) == 0;
@@ -383,11 +306,38 @@ static struct platform_device lubbock_flash_device[2] = {
 	},
 };
 
+static struct resource lubbock_io_resources[] = {
+	[0] = {
+		.start	= LUBBOCK_FPGA_PHYS,
+		.end	= LUBBOCK_FPGA_PHYS + 256 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= PXA_GPIO_TO_IRQ(0),
+		.end	= LUBBOCK_IRQ(0),
+		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_LOWEDGE,
+	},
+	[2] = {
+		.start	= LUBBOCK_IRQ(0),
+		.end	= LUBBOCK_IRQ(0),
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device lubbock_io_device = {
+	.name		= "lubbock_io",
+	.id		= -1,
+	.resource	= &lubbock_io_resources[0],
+	.num_resources	= 3,
+};
+
+
 static struct platform_device *devices[] __initdata = {
 	&sa1111_device,
 	&smc91x_device,
 	&lubbock_flash_device[0],
 	&lubbock_flash_device[1],
+	&lubbock_io_device,
 };
 
 static struct pxafb_mode_info sharp_lm8v31_mode = {
@@ -648,7 +598,7 @@ MACHINE_START(LUBBOCK, "Intel DBPXA250 Development Platform (aka Lubbock)")
 	/* Maintainer: MontaVista Software Inc. */
 	.map_io		= lubbock_map_io,
 	.nr_irqs	= LUBBOCK_NR_IRQS,
-	.init_irq	= lubbock_init_irq,
+	.init_irq	= pxa25x_init_irq,
 	.handle_irq	= pxa25x_handle_irq,
 	.init_time	= pxa_timer_init,
 	.init_machine	= lubbock_init,
-- 
2.1.0

[PATCH v3 3/3] ARM: pxa: lubbock: use new lubbock_io driver

[Robert Jarzmik]

As the interrupt handling was transferred to the lubbock_io driver, make
the switch in lubbock platform code.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: change the name from cottula to lubbock_io
            Dmitry pointed out the Cottula was the pxa25x family name,
	    lubbock was the pxa25x development board name. Therefore the
	    name was changed to lubbock_io (lubbock IO board)
	  change the resources to bi-irq ioresource
	    Discussion between Arnd and Robert to change the gpio
	    request by a irq request.
---
 arch/arm/mach-pxa/include/mach/lubbock.h |   7 +-
 arch/arm/mach-pxa/lubbock.c              | 108 +++++++++----------------------
 2 files changed, 33 insertions(+), 82 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/lubbock.h b/arch/arm/mach-pxa/include/mach/lubbock.h
index 958cd6af..f602e6a 100644
--- a/arch/arm/mach-pxa/include/mach/lubbock.h
+++ b/arch/arm/mach-pxa/include/mach/lubbock.h
@@ -37,7 +37,9 @@
 #define LUB_GP			__LUB_REG(LUBBOCK_FPGA_PHYS + 0x100)
 
 /* Board specific IRQs */
-#define LUBBOCK_IRQ(x)		(IRQ_BOARD_START + (x))
+#define LUBBOCK_NR_IRQS		IRQ_BOARD_START
+
+#define LUBBOCK_IRQ(x)		(LUBBOCK_NR_IRQS + (x))
 #define LUBBOCK_SD_IRQ		LUBBOCK_IRQ(0)
 #define LUBBOCK_SA1111_IRQ	LUBBOCK_IRQ(1)
 #define LUBBOCK_USB_IRQ		LUBBOCK_IRQ(2)  /* usb connect */
@@ -47,8 +49,7 @@
 #define LUBBOCK_USB_DISC_IRQ	LUBBOCK_IRQ(6)  /* usb disconnect */
 #define LUBBOCK_LAST_IRQ	LUBBOCK_IRQ(6)
 
-#define LUBBOCK_SA1111_IRQ_BASE	(IRQ_BOARD_START + 16)
-#define LUBBOCK_NR_IRQS		(IRQ_BOARD_START + 16 + 55)
+#define LUBBOCK_SA1111_IRQ_BASE	(LUBBOCK_NR_IRQS + 16)
 
 #ifndef __ASSEMBLY__
 extern void lubbock_set_misc_wr(unsigned int mask, unsigned int set);
diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index d8a1be6..4ef957a 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -12,6 +12,7 @@
  *  published by the Free Software Foundation.
  */
 #include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -123,84 +124,6 @@ void lubbock_set_misc_wr(unsigned int mask, unsigned int set)
 }
 EXPORT_SYMBOL(lubbock_set_misc_wr);
 
-static unsigned long lubbock_irq_enabled;
-
-static void lubbock_mask_irq(struct irq_data *d)
-{
-	int lubbock_irq = (d->irq - LUBBOCK_IRQ(0));
-	LUB_IRQ_MASK_EN = (lubbock_irq_enabled &= ~(1 << lubbock_irq));
-}
-
-static void lubbock_unmask_irq(struct irq_data *d)
-{
-	int lubbock_irq = (d->irq - LUBBOCK_IRQ(0));
-	/* the irq can be acknowledged only if deasserted, so it's done here */
-	LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq);
-	LUB_IRQ_MASK_EN = (lubbock_irq_enabled |= (1 << lubbock_irq));
-}
-
-static struct irq_chip lubbock_irq_chip = {
-	.name		= "FPGA",
-	.irq_ack	= lubbock_mask_irq,
-	.irq_mask	= lubbock_mask_irq,
-	.irq_unmask	= lubbock_unmask_irq,
-};
-
-static void lubbock_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	unsigned long pending = LUB_IRQ_SET_CLR & lubbock_irq_enabled;
-	do {
-		/* clear our parent irq */
-		desc->irq_data.chip->irq_ack(&desc->irq_data);
-		if (likely(pending)) {
-			irq = LUBBOCK_IRQ(0) + __ffs(pending);
-			generic_handle_irq(irq);
-		}
-		pending = LUB_IRQ_SET_CLR & lubbock_irq_enabled;
-	} while (pending);
-}
-
-static void __init lubbock_init_irq(void)
-{
-	int irq;
-
-	pxa25x_init_irq();
-
-	/* setup extra lubbock irqs */
-	for (irq = LUBBOCK_IRQ(0); irq <= LUBBOCK_LAST_IRQ; irq++) {
-		irq_set_chip_and_handler(irq, &lubbock_irq_chip,
-					 handle_level_irq);
-		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
-	}
-
-	irq_set_chained_handler(PXA_GPIO_TO_IRQ(0), lubbock_irq_handler);
-	irq_set_irq_type(PXA_GPIO_TO_IRQ(0), IRQ_TYPE_EDGE_FALLING);
-}
-
-#ifdef CONFIG_PM
-
-static void lubbock_irq_resume(void)
-{
-	LUB_IRQ_MASK_EN = lubbock_irq_enabled;
-}
-
-static struct syscore_ops lubbock_irq_syscore_ops = {
-	.resume = lubbock_irq_resume,
-};
-
-static int __init lubbock_irq_device_init(void)
-{
-	if (machine_is_lubbock()) {
-		register_syscore_ops(&lubbock_irq_syscore_ops);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-device_initcall(lubbock_irq_device_init);
-
-#endif
-
 static int lubbock_udc_is_connected(void)
 {
 	return (LUB_MISC_RD & (1 << 9)) == 0;
@@ -383,11 +306,38 @@ static struct platform_device lubbock_flash_device[2] = {
 	},
 };
 
+static struct resource lubbock_io_resources[] = {
+	[0] = {
+		.start	= LUBBOCK_FPGA_PHYS,
+		.end	= LUBBOCK_FPGA_PHYS + 256 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= PXA_GPIO_TO_IRQ(0),
+		.end	= LUBBOCK_IRQ(0),
+		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_LOWEDGE,
+	},
+	[2] = {
+		.start	= LUBBOCK_IRQ(0),
+		.end	= LUBBOCK_IRQ(0),
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device lubbock_io_device = {
+	.name		= "lubbock_io",
+	.id		= -1,
+	.resource	= &lubbock_io_resources[0],
+	.num_resources	= 3,
+};
+
+
 static struct platform_device *devices[] __initdata = {
 	&sa1111_device,
 	&smc91x_device,
 	&lubbock_flash_device[0],
 	&lubbock_flash_device[1],
+	&lubbock_io_device,
 };
 
 static struct pxafb_mode_info sharp_lm8v31_mode = {
@@ -648,7 +598,7 @@ MACHINE_START(LUBBOCK, "Intel DBPXA250 Development Platform (aka Lubbock)")
 	/* Maintainer: MontaVista Software Inc. */
 	.map_io		= lubbock_map_io,
 	.nr_irqs	= LUBBOCK_NR_IRQS,
-	.init_irq	= lubbock_init_irq,
+	.init_irq	= pxa25x_init_irq,
 	.handle_irq	= pxa25x_handle_irq,
 	.init_time	= pxa_timer_init,
 	.init_machine	= lubbock_init,
-- 
2.1.0

Re: [PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Lee Jones]

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

> Add a binding for lubbock motherboard IO board.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> new file mode 100644
> index 0000000..33c9e21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> @@ -0,0 +1,26 @@
> +Intel's pxa255 system development platform motherboard.

s/pxa25/PXA25/

"system development platform motherboard" doesn't quite sit right with me.

How about "Intel Xscale PXA255 development platform (Lubbock)"?

> +This is the motherboard, or IO board, of the pxa25x development system,
> +supporting a lubbock pxa25x SoC board.

Again, this sounds weird.

> +Required properties:
> +  - compatible : One of the following chip-specific strings:
> +        "intel,lubbock-io"

An odd thing to say with only one entry.

Also, please line it up to the right of the ':' above.

> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
> +                 multiplex is connected to. The only known case is GPIO0 on the
> +                 pxa25x SoC.

Can you get someone to help you re-word this into a more fluid
sentence?

> +Optional properties:
> +  - interrupts : The second optional interrupt is the base of the interrupts
> +                 multiplexed by the lubbock motherboard. If unspecified, the
> +                 range is fully dynamic, and the irqdomain will generate its
> +                 interrupt base on the fly.
> +
> +Example:
> +
> +mb: lubbock-mb@0 {
> +	compatible = "intel,lubbock-io";
> +	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
> +	#interrupt-cells = <2>;
> +        interrupt-parent = <&pxairq>;

Whitespace error.

> +};

I'm guessing mb means motherboard?  I think it's unusual for a
motherboard to have it's own driver.  Usually we provide drivers for
the individual components/peripherals situated on the board.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Lee Jones]

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

> Add a binding for lubbock motherboard IO board.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> new file mode 100644
> index 0000000..33c9e21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> @@ -0,0 +1,26 @@
> +Intel's pxa255 system development platform motherboard.

s/pxa25/PXA25/

"system development platform motherboard" doesn't quite sit right with me.

How about "Intel Xscale PXA255 development platform (Lubbock)"?

> +This is the motherboard, or IO board, of the pxa25x development system,
> +supporting a lubbock pxa25x SoC board.

Again, this sounds weird.

> +Required properties:
> +  - compatible : One of the following chip-specific strings:
> +        "intel,lubbock-io"

An odd thing to say with only one entry.

Also, please line it up to the right of the ':' above.

> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
> +                 multiplex is connected to. The only known case is GPIO0 on the
> +                 pxa25x SoC.

Can you get someone to help you re-word this into a more fluid
sentence?

> +Optional properties:
> +  - interrupts : The second optional interrupt is the base of the interrupts
> +                 multiplexed by the lubbock motherboard. If unspecified, the
> +                 range is fully dynamic, and the irqdomain will generate its
> +                 interrupt base on the fly.
> +
> +Example:
> +
> +mb: lubbock-mb at 0 {
> +	compatible = "intel,lubbock-io";
> +	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
> +	#interrupt-cells = <2>;
> +        interrupt-parent = <&pxairq>;

Whitespace error.

> +};

I'm guessing mb means motherboard?  I think it's unusual for a
motherboard to have it's own driver.  Usually we provide drivers for
the individual components/peripherals situated on the board.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

> Lubbock () board is the IO motherboard of the Intel PXA25x Development
> Platform, which supports the Lubbock pxa25x soc board.
> 
> Historically, this support was in arch/arm/mach-pxa/lubbock.c. When
> gpio-pxa was moved to drivers/pxa, it became a driver, and its
> initialization and probing happened at postcore initcall. The lubbock
> code used to install the chained lubbock interrupt handler at init_irq()
> time.
> 
> The consequence of the gpio-pxa change is that the installed chained irq
> handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
> removing :
>  - the handler
>  - the falling edge detection setting of GPIO0, which revealed the
>    interrupt request from the lubbock IO board.
> 
> As a fix, move the gpio0 chained handler setup to a place where we have
> the guarantee that pxa_gpio_probe() was called before, so that lubbock
> handler becomes the true IRQ chained handler of GPIO0, demuxing the
> lubbock IO board interrupts.

How is this guaranteed?

> This patch moves all that handling to a mfd driver. It's only purpose
> for the time being is the interrupt handling, but in the future it
> should encompass all the motherboard CPLDs handling :
>  - leds
>  - switches
>  - hexleds
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> Since v1: change the name from cottula to lubbock_io
>             Dmitry pointed out the Cottula was the pxa25x family name,
> 	    lubbock was the pxa25x development board name. Therefore the
> 	    name was changed to lubbock_io (lubbock IO board)
> 	  change the resources to bi-irq ioresource
> 	    Discussion between Arnd and Robert to change the gpio
> 	    request by a irq request.
> Since v2: take into account Mark's review
>       	    Use irq flags from resources (DT case and pdata case).
> 	    Change of name from lubbock_io to lubbock-io
> ---
>  drivers/mfd/Kconfig   |  10 +++
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 207 insertions(+)
>  create mode 100644 drivers/mfd/lubbock.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b731..4d8939f 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -91,6 +91,16 @@ config MFD_AXP20X
>  	  components like regulators or the PEK (Power Enable Key) under the
>  	  corresponding menus.
>  
> +config MFD_LUBBOCK
> +	bool "Lubbock Motherboard"
> +	def_bool ARCH_LUBBOCK
> +	select MFD_CORE
> +	help
> +	  This driver supports the Lubbock multifunction chip found on the
> +	  pxa25x development platform system (named Lubbock). This IO board
> +	  supports the interrupts handling, ethernet controller, flash chips,
> +	  etc ...
> +
>  config MFD_CROS_EC
>  	tristate "ChromeOS Embedded Controller"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e2..aff1f4f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
>  obj-$(CONFIG_MFD_SM501)		+= sm501.o
>  obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> +obj-$(CONFIG_MFD_LUBBOCK)	+= lubbock.o
>  obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
>  obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
>  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
> diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c
> new file mode 100644
> index 0000000..6025135
> --- /dev/null
> +++ b/drivers/mfd/lubbock.c
> @@ -0,0 +1,196 @@
> +/*
> + * Intel Cotulla MFD - lubbock motherboard
> + *
> + * Copyright (C) 2014 Robert Jarzmik
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.

Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.

> + *

Superfluous '\n'.

> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>

Can this be built as a module?

If so, why isn't it a tristate?

> +#include <linux/of_platform.h>
> +
> +#define COT_IRQ_MASK_EN 0xc0
> +#define COT_IRQ_SET_CLR 0xd0
> +
> +#define LUBBOCK_NB_IRQ	8
> +
> +struct lubbock {
> +	void __iomem	*base;

Random spacing.

> +	int irq;
> +	unsigned int irq_mask;
> +	struct gpio_desc *gpio0;
> +	struct irq_domain *irqdomain;
> +};
> +
> +static irqreturn_t lubbock_irq_handler(int in_irq, void *d)
> +{
> +	struct lubbock *cot = d;
> +	unsigned long pending;
> +	unsigned int bit;
> +
> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));

I'd like to see a '\n' here.

> +	return IRQ_HANDLED;
> +}
> +
> +static void lubbock_irq_mask_ack(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int set, bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask &= ~bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	set = readl(cot->base + COT_IRQ_SET_CLR);
> +	writel(set & ~bit, cot->base + COT_IRQ_SET_CLR);
> +}
> +
> +static void lubbock_irq_unmask(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask |= bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +}
> +
> +static struct irq_chip lubbock_irq_chip = {
> +	.name		= "lubbock",
> +	.irq_mask_ack	= lubbock_irq_mask_ack,
> +	.irq_unmask	= lubbock_irq_unmask,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				   irq_hw_number_t hwirq)
> +{
> +	struct lubbock *cot = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, cot);

Again, I'd prefer some separation between code and the return.

(same in all cases below).

> +	return 0;
> +}
> +
> +static const struct irq_domain_ops lubbock_irq_domain_ops = {
> +	.xlate = irq_domain_xlate_twocell,
> +	.map = lubbock_irq_domain_map,
> +};
> +
> +static int lubbock_resume(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	return 0;
> +}
> +
> +static int lubbock_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct lubbock *cot;
> +	int ret;
> +	unsigned int base_irq = 0;
> +	unsigned long irqflags;
> +
> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
> +	if (!cot)
> +		return -ENOMEM;

'\n' here.

> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq()?

> +	if (res) {
> +		cot->irq = (unsigned int)res->start;
> +		irqflags = res->flags;
> +	}
> +	if (!cot->irq)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);

platform_get_irq()?

> +	if (res)
> +		base_irq = (unsigned int)res->start;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	cot->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(cot->base))
> +		return PTR_ERR(cot->base);
> +
> +	platform_set_drvdata(pdev, cot);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	writel(0, cot->base + COT_IRQ_SET_CLR);

'\n'

> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> +			       irqflags, dev_name(&pdev->dev), cot);
> +	if (ret == -ENOSYS)
> +		return -EPROBE_DEFER;

I haven't seen anyone do this after devm_request_irq() before.

Why is it required here?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> +			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

> +		return ret;
> +	}
> +	irq_set_irq_wake(cot->irq, 1);
> +
> +	cot->irqdomain =
> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
> +				      &lubbock_irq_domain_ops, cot);

As a personal preference, I would prefer to see:

	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
					       LUBBOCK_NB_IRQ,
					       &lubbock_irq_domain_ops, cot);

> +	if (!cot->irqdomain)
> +		return -ENODEV;
> +
> +	ret = 0;

'ret' will be zero here, or we would have returned already.

> +	if (base_irq)
> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
> +						 LUBBOCK_NB_IRQ);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
> +		return ret;
> +	}

Is this solely the check from irq_create_strict_mappings(), therefore
it should be inside the previous if () { ... }.

> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
> +		 cot->base, cot->irq, base_irq);

Please remove this line.

> +	return 0;
> +}
> +
> +static int lubbock_remove(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	irq_set_chip_and_handler(cot->irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static const struct of_device_id lubbock_id_table[] = {
> +	{ .compatible = "intel,lubbock-io", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lubbock_id_table);
> +
> +static struct platform_driver lubbock_driver = {
> +	.driver		= {
> +		.name	= "lubbock_io",
> +		.of_match_table = of_match_ptr(lubbock_id_table),
> +	},
> +	.probe		= lubbock_probe,
> +	.remove		= lubbock_remove,
> +	.resume		= lubbock_resume,
> +};
> +
> +module_platform_driver(lubbock_driver);
> +
> +MODULE_DESCRIPTION("Lubbock driver");

"Lubbock MFD Driver"?

> +MODULE_AUTHOR("Robert Jarzmik");

Email.

> +MODULE_LICENSE("GPL");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

> Lubbock () board is the IO motherboard of the Intel PXA25x Development
> Platform, which supports the Lubbock pxa25x soc board.
> 
> Historically, this support was in arch/arm/mach-pxa/lubbock.c. When
> gpio-pxa was moved to drivers/pxa, it became a driver, and its
> initialization and probing happened at postcore initcall. The lubbock
> code used to install the chained lubbock interrupt handler at init_irq()
> time.
> 
> The consequence of the gpio-pxa change is that the installed chained irq
> handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
> removing :
>  - the handler
>  - the falling edge detection setting of GPIO0, which revealed the
>    interrupt request from the lubbock IO board.
> 
> As a fix, move the gpio0 chained handler setup to a place where we have
> the guarantee that pxa_gpio_probe() was called before, so that lubbock
> handler becomes the true IRQ chained handler of GPIO0, demuxing the
> lubbock IO board interrupts.

How is this guaranteed?

> This patch moves all that handling to a mfd driver. It's only purpose
> for the time being is the interrupt handling, but in the future it
> should encompass all the motherboard CPLDs handling :
>  - leds
>  - switches
>  - hexleds
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> ---
> Since v1: change the name from cottula to lubbock_io
>             Dmitry pointed out the Cottula was the pxa25x family name,
> 	    lubbock was the pxa25x development board name. Therefore the
> 	    name was changed to lubbock_io (lubbock IO board)
> 	  change the resources to bi-irq ioresource
> 	    Discussion between Arnd and Robert to change the gpio
> 	    request by a irq request.
> Since v2: take into account Mark's review
>       	    Use irq flags from resources (DT case and pdata case).
> 	    Change of name from lubbock_io to lubbock-io
> ---
>  drivers/mfd/Kconfig   |  10 +++
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 207 insertions(+)
>  create mode 100644 drivers/mfd/lubbock.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b731..4d8939f 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -91,6 +91,16 @@ config MFD_AXP20X
>  	  components like regulators or the PEK (Power Enable Key) under the
>  	  corresponding menus.
>  
> +config MFD_LUBBOCK
> +	bool "Lubbock Motherboard"
> +	def_bool ARCH_LUBBOCK
> +	select MFD_CORE
> +	help
> +	  This driver supports the Lubbock multifunction chip found on the
> +	  pxa25x development platform system (named Lubbock). This IO board
> +	  supports the interrupts handling, ethernet controller, flash chips,
> +	  etc ...
> +
>  config MFD_CROS_EC
>  	tristate "ChromeOS Embedded Controller"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e2..aff1f4f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
>  obj-$(CONFIG_MFD_SM501)		+= sm501.o
>  obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> +obj-$(CONFIG_MFD_LUBBOCK)	+= lubbock.o
>  obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
>  obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
>  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
> diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c
> new file mode 100644
> index 0000000..6025135
> --- /dev/null
> +++ b/drivers/mfd/lubbock.c
> @@ -0,0 +1,196 @@
> +/*
> + * Intel Cotulla MFD - lubbock motherboard
> + *
> + * Copyright (C) 2014 Robert Jarzmik
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.

Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.

> + *

Superfluous '\n'.

> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>

Can this be built as a module?

If so, why isn't it a tristate?

> +#include <linux/of_platform.h>
> +
> +#define COT_IRQ_MASK_EN 0xc0
> +#define COT_IRQ_SET_CLR 0xd0
> +
> +#define LUBBOCK_NB_IRQ	8
> +
> +struct lubbock {
> +	void __iomem	*base;

Random spacing.

> +	int irq;
> +	unsigned int irq_mask;
> +	struct gpio_desc *gpio0;
> +	struct irq_domain *irqdomain;
> +};
> +
> +static irqreturn_t lubbock_irq_handler(int in_irq, void *d)
> +{
> +	struct lubbock *cot = d;
> +	unsigned long pending;
> +	unsigned int bit;
> +
> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));

I'd like to see a '\n' here.

> +	return IRQ_HANDLED;
> +}
> +
> +static void lubbock_irq_mask_ack(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int set, bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask &= ~bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	set = readl(cot->base + COT_IRQ_SET_CLR);
> +	writel(set & ~bit, cot->base + COT_IRQ_SET_CLR);
> +}
> +
> +static void lubbock_irq_unmask(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask |= bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +}
> +
> +static struct irq_chip lubbock_irq_chip = {
> +	.name		= "lubbock",
> +	.irq_mask_ack	= lubbock_irq_mask_ack,
> +	.irq_unmask	= lubbock_irq_unmask,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				   irq_hw_number_t hwirq)
> +{
> +	struct lubbock *cot = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, cot);

Again, I'd prefer some separation between code and the return.

(same in all cases below).

> +	return 0;
> +}
> +
> +static const struct irq_domain_ops lubbock_irq_domain_ops = {
> +	.xlate = irq_domain_xlate_twocell,
> +	.map = lubbock_irq_domain_map,
> +};
> +
> +static int lubbock_resume(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	return 0;
> +}
> +
> +static int lubbock_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct lubbock *cot;
> +	int ret;
> +	unsigned int base_irq = 0;
> +	unsigned long irqflags;
> +
> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
> +	if (!cot)
> +		return -ENOMEM;

'\n' here.

> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq()?

> +	if (res) {
> +		cot->irq = (unsigned int)res->start;
> +		irqflags = res->flags;
> +	}
> +	if (!cot->irq)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);

platform_get_irq()?

> +	if (res)
> +		base_irq = (unsigned int)res->start;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	cot->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(cot->base))
> +		return PTR_ERR(cot->base);
> +
> +	platform_set_drvdata(pdev, cot);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	writel(0, cot->base + COT_IRQ_SET_CLR);

'\n'

> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> +			       irqflags, dev_name(&pdev->dev), cot);
> +	if (ret == -ENOSYS)
> +		return -EPROBE_DEFER;

I haven't seen anyone do this after devm_request_irq() before.

Why is it required here?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> +			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

> +		return ret;
> +	}
> +	irq_set_irq_wake(cot->irq, 1);
> +
> +	cot->irqdomain =
> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
> +				      &lubbock_irq_domain_ops, cot);

As a personal preference, I would prefer to see:

	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
					       LUBBOCK_NB_IRQ,
					       &lubbock_irq_domain_ops, cot);

> +	if (!cot->irqdomain)
> +		return -ENODEV;
> +
> +	ret = 0;

'ret' will be zero here, or we would have returned already.

> +	if (base_irq)
> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
> +						 LUBBOCK_NB_IRQ);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
> +		return ret;
> +	}

Is this solely the check from irq_create_strict_mappings(), therefore
it should be inside the previous if () { ... }.

> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
> +		 cot->base, cot->irq, base_irq);

Please remove this line.

> +	return 0;
> +}
> +
> +static int lubbock_remove(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	irq_set_chip_and_handler(cot->irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static const struct of_device_id lubbock_id_table[] = {
> +	{ .compatible = "intel,lubbock-io", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lubbock_id_table);
> +
> +static struct platform_driver lubbock_driver = {
> +	.driver		= {
> +		.name	= "lubbock_io",
> +		.of_match_table = of_match_ptr(lubbock_id_table),
> +	},
> +	.probe		= lubbock_probe,
> +	.remove		= lubbock_remove,
> +	.resume		= lubbock_resume,
> +};
> +
> +module_platform_driver(lubbock_driver);
> +
> +MODULE_DESCRIPTION("Lubbock driver");

"Lubbock MFD Driver"?

> +MODULE_AUTHOR("Robert Jarzmik");

Email.

> +MODULE_LICENSE("GPL");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

> Lubbock () board is the IO motherboard of the Intel PXA25x Development
> Platform, which supports the Lubbock pxa25x soc board.
> 
> Historically, this support was in arch/arm/mach-pxa/lubbock.c. When
> gpio-pxa was moved to drivers/pxa, it became a driver, and its
> initialization and probing happened at postcore initcall. The lubbock
> code used to install the chained lubbock interrupt handler at init_irq()
> time.
> 
> The consequence of the gpio-pxa change is that the installed chained irq
> handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
> removing :
>  - the handler
>  - the falling edge detection setting of GPIO0, which revealed the
>    interrupt request from the lubbock IO board.
> 
> As a fix, move the gpio0 chained handler setup to a place where we have
> the guarantee that pxa_gpio_probe() was called before, so that lubbock
> handler becomes the true IRQ chained handler of GPIO0, demuxing the
> lubbock IO board interrupts.

How is this guaranteed?

> This patch moves all that handling to a mfd driver. It's only purpose
> for the time being is the interrupt handling, but in the future it
> should encompass all the motherboard CPLDs handling :
>  - leds
>  - switches
>  - hexleds
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> Since v1: change the name from cottula to lubbock_io
>             Dmitry pointed out the Cottula was the pxa25x family name,
> 	    lubbock was the pxa25x development board name. Therefore the
> 	    name was changed to lubbock_io (lubbock IO board)
> 	  change the resources to bi-irq ioresource
> 	    Discussion between Arnd and Robert to change the gpio
> 	    request by a irq request.
> Since v2: take into account Mark's review
>       	    Use irq flags from resources (DT case and pdata case).
> 	    Change of name from lubbock_io to lubbock-io
> ---
>  drivers/mfd/Kconfig   |  10 +++
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 207 insertions(+)
>  create mode 100644 drivers/mfd/lubbock.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b731..4d8939f 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -91,6 +91,16 @@ config MFD_AXP20X
>  	  components like regulators or the PEK (Power Enable Key) under the
>  	  corresponding menus.
>  
> +config MFD_LUBBOCK
> +	bool "Lubbock Motherboard"
> +	def_bool ARCH_LUBBOCK
> +	select MFD_CORE
> +	help
> +	  This driver supports the Lubbock multifunction chip found on the
> +	  pxa25x development platform system (named Lubbock). This IO board
> +	  supports the interrupts handling, ethernet controller, flash chips,
> +	  etc ...
> +
>  config MFD_CROS_EC
>  	tristate "ChromeOS Embedded Controller"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e2..aff1f4f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
>  obj-$(CONFIG_MFD_SM501)		+= sm501.o
>  obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> +obj-$(CONFIG_MFD_LUBBOCK)	+= lubbock.o
>  obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
>  obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
>  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
> diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c
> new file mode 100644
> index 0000000..6025135
> --- /dev/null
> +++ b/drivers/mfd/lubbock.c
> @@ -0,0 +1,196 @@
> +/*
> + * Intel Cotulla MFD - lubbock motherboard
> + *
> + * Copyright (C) 2014 Robert Jarzmik
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.

Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.

> + *

Superfluous '\n'.

> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>

Can this be built as a module?

If so, why isn't it a tristate?

> +#include <linux/of_platform.h>
> +
> +#define COT_IRQ_MASK_EN 0xc0
> +#define COT_IRQ_SET_CLR 0xd0
> +
> +#define LUBBOCK_NB_IRQ	8
> +
> +struct lubbock {
> +	void __iomem	*base;

Random spacing.

> +	int irq;
> +	unsigned int irq_mask;
> +	struct gpio_desc *gpio0;
> +	struct irq_domain *irqdomain;
> +};
> +
> +static irqreturn_t lubbock_irq_handler(int in_irq, void *d)
> +{
> +	struct lubbock *cot = d;
> +	unsigned long pending;
> +	unsigned int bit;
> +
> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));

I'd like to see a '\n' here.

> +	return IRQ_HANDLED;
> +}
> +
> +static void lubbock_irq_mask_ack(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int set, bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask &= ~bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	set = readl(cot->base + COT_IRQ_SET_CLR);
> +	writel(set & ~bit, cot->base + COT_IRQ_SET_CLR);
> +}
> +
> +static void lubbock_irq_unmask(struct irq_data *d)
> +{
> +	struct lubbock *cot = irq_data_get_irq_chip_data(d);
> +	unsigned int lubbock_irq = irqd_to_hwirq(d);
> +	unsigned int bit = BIT(lubbock_irq);
> +
> +	cot->irq_mask |= bit;
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +}
> +
> +static struct irq_chip lubbock_irq_chip = {
> +	.name		= "lubbock",
> +	.irq_mask_ack	= lubbock_irq_mask_ack,
> +	.irq_unmask	= lubbock_irq_unmask,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				   irq_hw_number_t hwirq)
> +{
> +	struct lubbock *cot = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, cot);

Again, I'd prefer some separation between code and the return.

(same in all cases below).

> +	return 0;
> +}
> +
> +static const struct irq_domain_ops lubbock_irq_domain_ops = {
> +	.xlate = irq_domain_xlate_twocell,
> +	.map = lubbock_irq_domain_map,
> +};
> +
> +static int lubbock_resume(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	return 0;
> +}
> +
> +static int lubbock_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct lubbock *cot;
> +	int ret;
> +	unsigned int base_irq = 0;
> +	unsigned long irqflags;
> +
> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
> +	if (!cot)
> +		return -ENOMEM;

'\n' here.

> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq()?

> +	if (res) {
> +		cot->irq = (unsigned int)res->start;
> +		irqflags = res->flags;
> +	}
> +	if (!cot->irq)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);

platform_get_irq()?

> +	if (res)
> +		base_irq = (unsigned int)res->start;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	cot->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(cot->base))
> +		return PTR_ERR(cot->base);
> +
> +	platform_set_drvdata(pdev, cot);
> +
> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
> +	writel(0, cot->base + COT_IRQ_SET_CLR);

'\n'

> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> +			       irqflags, dev_name(&pdev->dev), cot);
> +	if (ret == -ENOSYS)
> +		return -EPROBE_DEFER;

I haven't seen anyone do this after devm_request_irq() before.

Why is it required here?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> +			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

> +		return ret;
> +	}
> +	irq_set_irq_wake(cot->irq, 1);
> +
> +	cot->irqdomain =
> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
> +				      &lubbock_irq_domain_ops, cot);

As a personal preference, I would prefer to see:

	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
					       LUBBOCK_NB_IRQ,
					       &lubbock_irq_domain_ops, cot);

> +	if (!cot->irqdomain)
> +		return -ENODEV;
> +
> +	ret = 0;

'ret' will be zero here, or we would have returned already.

> +	if (base_irq)
> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
> +						 LUBBOCK_NB_IRQ);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
> +		return ret;
> +	}

Is this solely the check from irq_create_strict_mappings(), therefore
it should be inside the previous if () { ... }.

> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
> +		 cot->base, cot->irq, base_irq);

Please remove this line.

> +	return 0;
> +}
> +
> +static int lubbock_remove(struct platform_device *pdev)
> +{
> +	struct lubbock *cot = platform_get_drvdata(pdev);
> +
> +	irq_set_chip_and_handler(cot->irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static const struct of_device_id lubbock_id_table[] = {
> +	{ .compatible = "intel,lubbock-io", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lubbock_id_table);
> +
> +static struct platform_driver lubbock_driver = {
> +	.driver		= {
> +		.name	= "lubbock_io",
> +		.of_match_table = of_match_ptr(lubbock_id_table),
> +	},
> +	.probe		= lubbock_probe,
> +	.remove		= lubbock_remove,
> +	.resume		= lubbock_resume,
> +};
> +
> +module_platform_driver(lubbock_driver);
> +
> +MODULE_DESCRIPTION("Lubbock driver");

"Lubbock MFD Driver"?

> +MODULE_AUTHOR("Robert Jarzmik");

Email.

> +MODULE_LICENSE("GPL");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> As a fix, move the gpio0 chained handler setup to a place where we have
>> the guarantee that pxa_gpio_probe() was called before, so that lubbock
>> handler becomes the true IRQ chained handler of GPIO0, demuxing the
>> lubbock IO board interrupts.
>
> How is this guaranteed?
In the following chunk :
	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
			       irqflags, dev_name(&pdev->dev), cot);
	if (ret == -ENOSYS)
		return -EPROBE_DEFER;

See __setup_irq(), and see what happens if the irq chip is not set (which
happens on pxa platform when the gpio driver is not registered).

>> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.
>
> Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.
OK, your tree, your rules.

> Superfluous '\n'.
Yep.

> Can this be built as a module?
Not in its current form.

> If so, why isn't it a tristate?
See above.

Now the question is : should it be buildable as a module ? I was thinking it
shouldn't because without this driver lubbock becomes a bit useless (most of its
peripherals are on the motherboard).

>> +struct lubbock {
>> +	void __iomem	*base;
>
> Random spacing.
Right.

>> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
>> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
>> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));
>
> I'd like to see a '\n' here.
OK.

> Again, I'd prefer some separation between code and the return.
>
> (same in all cases below).
OK.
>
>> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
>> +	if (!cot)
>> +		return -ENOMEM;
>
> '\n' here.
OK.

>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> platform_get_irq()?
No. I need the flags.

>> +	if (res) {
>> +		cot->irq = (unsigned int)res->start;
>> +		irqflags = res->flags;
>> +	}
>> +	if (!cot->irq)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
>
> platform_get_irq()?
Yes, certainly.

>> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
>> +	writel(0, cot->base + COT_IRQ_SET_CLR);
>
> '\n'
OK.
>> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>> +			       irqflags, dev_name(&pdev->dev), cot);
>> +	if (ret == -ENOSYS)
>> +		return -EPROBE_DEFER;
>
> I haven't seen anyone do this after devm_request_irq() before.
> Why is it required here?
Explained above.

>
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
>> +			ret);
>
> I'm not keen on this type of formatting.  Besides the system will
> print out the returned error on failure.
Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

>> +	cot->irqdomain =
>> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
>> +				      &lubbock_irq_domain_ops, cot);
>
> As a personal preference, I would prefer to see:
>
> 	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
> 					       LUBBOCK_NB_IRQ,
> 					       &lubbock_irq_domain_ops, cot);
Your tree, your rules. OK.

>
>> +	if (!cot->irqdomain)
>> +		return -ENODEV;
>> +
>> +	ret = 0;
>
> 'ret' will be zero here, or we would have returned already.
Good catch. OK.

>> +	if (base_irq)
>> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
>> +						 LUBBOCK_NB_IRQ);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
>> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
>> +		return ret;
>> +	}
>
> Is this solely the check from irq_create_strict_mappings(), therefore
> it should be inside the previous if () { ... }.
As you wish.

>> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
>> +		 cot->base, cot->irq, base_irq);
>
> Please remove this line.
OK.

>> +MODULE_DESCRIPTION("Lubbock driver");
>
> "Lubbock MFD Driver"?
Yes.

>
>> +MODULE_AUTHOR("Robert Jarzmik");
>
> Email.
Sure

Thanks for the review.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> As a fix, move the gpio0 chained handler setup to a place where we have
>> the guarantee that pxa_gpio_probe() was called before, so that lubbock
>> handler becomes the true IRQ chained handler of GPIO0, demuxing the
>> lubbock IO board interrupts.
>
> How is this guaranteed?
In the following chunk :
	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
			       irqflags, dev_name(&pdev->dev), cot);
	if (ret == -ENOSYS)
		return -EPROBE_DEFER;

See __setup_irq(), and see what happens if the irq chip is not set (which
happens on pxa platform when the gpio driver is not registered).

>> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.
>
> Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.
OK, your tree, your rules.

> Superfluous '\n'.
Yep.

> Can this be built as a module?
Not in its current form.

> If so, why isn't it a tristate?
See above.

Now the question is : should it be buildable as a module ? I was thinking it
shouldn't because without this driver lubbock becomes a bit useless (most of its
peripherals are on the motherboard).

>> +struct lubbock {
>> +	void __iomem	*base;
>
> Random spacing.
Right.

>> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
>> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
>> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));
>
> I'd like to see a '\n' here.
OK.

> Again, I'd prefer some separation between code and the return.
>
> (same in all cases below).
OK.
>
>> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
>> +	if (!cot)
>> +		return -ENOMEM;
>
> '\n' here.
OK.

>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> platform_get_irq()?
No. I need the flags.

>> +	if (res) {
>> +		cot->irq = (unsigned int)res->start;
>> +		irqflags = res->flags;
>> +	}
>> +	if (!cot->irq)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
>
> platform_get_irq()?
Yes, certainly.

>> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
>> +	writel(0, cot->base + COT_IRQ_SET_CLR);
>
> '\n'
OK.
>> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>> +			       irqflags, dev_name(&pdev->dev), cot);
>> +	if (ret == -ENOSYS)
>> +		return -EPROBE_DEFER;
>
> I haven't seen anyone do this after devm_request_irq() before.
> Why is it required here?
Explained above.

>
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
>> +			ret);
>
> I'm not keen on this type of formatting.  Besides the system will
> print out the returned error on failure.
Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

>> +	cot->irqdomain =
>> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
>> +				      &lubbock_irq_domain_ops, cot);
>
> As a personal preference, I would prefer to see:
>
> 	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
> 					       LUBBOCK_NB_IRQ,
> 					       &lubbock_irq_domain_ops, cot);
Your tree, your rules. OK.

>
>> +	if (!cot->irqdomain)
>> +		return -ENODEV;
>> +
>> +	ret = 0;
>
> 'ret' will be zero here, or we would have returned already.
Good catch. OK.

>> +	if (base_irq)
>> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
>> +						 LUBBOCK_NB_IRQ);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
>> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
>> +		return ret;
>> +	}
>
> Is this solely the check from irq_create_strict_mappings(), therefore
> it should be inside the previous if () { ... }.
As you wish.

>> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
>> +		 cot->base, cot->irq, base_irq);
>
> Please remove this line.
OK.

>> +MODULE_DESCRIPTION("Lubbock driver");
>
> "Lubbock MFD Driver"?
Yes.

>
>> +MODULE_AUTHOR("Robert Jarzmik");
>
> Email.
Sure

Thanks for the review.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> As a fix, move the gpio0 chained handler setup to a place where we have
>> the guarantee that pxa_gpio_probe() was called before, so that lubbock
>> handler becomes the true IRQ chained handler of GPIO0, demuxing the
>> lubbock IO board interrupts.
>
> How is this guaranteed?
In the following chunk :
	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
			       irqflags, dev_name(&pdev->dev), cot);
	if (ret == -ENOSYS)
		return -EPROBE_DEFER;

See __setup_irq(), and see what happens if the irq chip is not set (which
happens on pxa platform when the gpio driver is not registered).

>> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.
>
> Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.
OK, your tree, your rules.

> Superfluous '\n'.
Yep.

> Can this be built as a module?
Not in its current form.

> If so, why isn't it a tristate?
See above.

Now the question is : should it be buildable as a module ? I was thinking it
shouldn't because without this driver lubbock becomes a bit useless (most of its
peripherals are on the motherboard).

>> +struct lubbock {
>> +	void __iomem	*base;
>
> Random spacing.
Right.

>> +	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
>> +	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
>> +		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));
>
> I'd like to see a '\n' here.
OK.

> Again, I'd prefer some separation between code and the return.
>
> (same in all cases below).
OK.
>
>> +	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
>> +	if (!cot)
>> +		return -ENOMEM;
>
> '\n' here.
OK.

>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> platform_get_irq()?
No. I need the flags.

>> +	if (res) {
>> +		cot->irq = (unsigned int)res->start;
>> +		irqflags = res->flags;
>> +	}
>> +	if (!cot->irq)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
>
> platform_get_irq()?
Yes, certainly.

>> +	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
>> +	writel(0, cot->base + COT_IRQ_SET_CLR);
>
> '\n'
OK.
>> +	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>> +			       irqflags, dev_name(&pdev->dev), cot);
>> +	if (ret == -ENOSYS)
>> +		return -EPROBE_DEFER;
>
> I haven't seen anyone do this after devm_request_irq() before.
> Why is it required here?
Explained above.

>
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
>> +			ret);
>
> I'm not keen on this type of formatting.  Besides the system will
> print out the returned error on failure.
Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

>> +	cot->irqdomain =
>> +		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
>> +				      &lubbock_irq_domain_ops, cot);
>
> As a personal preference, I would prefer to see:
>
> 	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
> 					       LUBBOCK_NB_IRQ,
> 					       &lubbock_irq_domain_ops, cot);
Your tree, your rules. OK.

>
>> +	if (!cot->irqdomain)
>> +		return -ENODEV;
>> +
>> +	ret = 0;
>
> 'ret' will be zero here, or we would have returned already.
Good catch. OK.

>> +	if (base_irq)
>> +		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
>> +						 LUBBOCK_NB_IRQ);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
>> +			base_irq, base_irq + LUBBOCK_NB_IRQ);
>> +		return ret;
>> +	}
>
> Is this solely the check from irq_create_strict_mappings(), therefore
> it should be inside the previous if () { ... }.
As you wish.

>> +	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
>> +		 cot->base, cot->irq, base_irq);
>
> Please remove this line.
OK.

>> +MODULE_DESCRIPTION("Lubbock driver");
>
> "Lubbock MFD Driver"?
Yes.

>
>> +MODULE_AUTHOR("Robert Jarzmik");
>
> Email.
Sure

Thanks for the review.

-- 
Robert

Re: [PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> Add a binding for lubbock motherboard IO board.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> ---
>>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> new file mode 100644
>> index 0000000..33c9e21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> @@ -0,0 +1,26 @@
>> +Intel's pxa255 system development platform motherboard.
>
> s/pxa25/PXA25/
Ok.
>
> "system development platform motherboard" doesn't quite sit right with me.
>
> How about "Intel Xscale PXA255 development platform (Lubbock)"?
OK.

>
>> +This is the motherboard, or IO board, of the pxa25x development system,
>> +supporting a lubbock pxa25x SoC board.
>
> Again, this sounds weird.
Would it sound better with :
  This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
  muxing, leds handling, ...
>
>> +Required properties:
>> +  - compatible : One of the following chip-specific strings:
>> +        "intel,lubbock-io"
>
> An odd thing to say with only one entry.
Right, a simple "should be" would fit better.
>
>> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
>> +                 multiplex is connected to. The only known case is GPIO0 on the
>> +                 pxa25x SoC.
>
> Can you get someone to help you re-word this into a more fluid
> sentence?
Aouch, how about this :
interrupts : The first interrupt is the SoC input interrupt connected to the
             lubbock IO board interrupt multiplexer output. The only known
             working configuration is GPIO0 on the pxa25x SoC.

>
>> +Optional properties:
>> +  - interrupts : The second optional interrupt is the base of the interrupts
>> +                 multiplexed by the lubbock motherboard. If unspecified, the
>> +                 range is fully dynamic, and the irqdomain will generate its
>> +                 interrupt base on the fly.
>> +
>> +Example:
>> +
>> +mb: lubbock-mb@0 {
>> +	compatible = "intel,lubbock-io";
>> +	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
>> +	#interrupt-cells = <2>;
>> +        interrupt-parent = <&pxairq>;
>
> Whitespace error.
Right.

> I'm guessing mb means motherboard?  I think it's unusual for a
> motherboard to have it's own driver.  Usually we provide drivers for
> the individual components/peripherals situated on the board.
Yes, mb for motherboard. Well, this driver is actually for the CPLDs on the
motherboard.  Would "lubbock-cplds" be a better choice ?

Cheers.

-- 
Robert

Re: [PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Robert Jarzmik]

Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> Add a binding for lubbock motherboard IO board.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>> ---
>>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> new file mode 100644
>> index 0000000..33c9e21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> @@ -0,0 +1,26 @@
>> +Intel's pxa255 system development platform motherboard.
>
> s/pxa25/PXA25/
Ok.
>
> "system development platform motherboard" doesn't quite sit right with me.
>
> How about "Intel Xscale PXA255 development platform (Lubbock)"?
OK.

>
>> +This is the motherboard, or IO board, of the pxa25x development system,
>> +supporting a lubbock pxa25x SoC board.
>
> Again, this sounds weird.
Would it sound better with :
  This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
  muxing, leds handling, ...
>
>> +Required properties:
>> +  - compatible : One of the following chip-specific strings:
>> +        "intel,lubbock-io"
>
> An odd thing to say with only one entry.
Right, a simple "should be" would fit better.
>
>> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
>> +                 multiplex is connected to. The only known case is GPIO0 on the
>> +                 pxa25x SoC.
>
> Can you get someone to help you re-word this into a more fluid
> sentence?
Aouch, how about this :
interrupts : The first interrupt is the SoC input interrupt connected to the
             lubbock IO board interrupt multiplexer output. The only known
             working configuration is GPIO0 on the pxa25x SoC.

>
>> +Optional properties:
>> +  - interrupts : The second optional interrupt is the base of the interrupts
>> +                 multiplexed by the lubbock motherboard. If unspecified, the
>> +                 range is fully dynamic, and the irqdomain will generate its
>> +                 interrupt base on the fly.
>> +
>> +Example:
>> +
>> +mb: lubbock-mb@0 {
>> +	compatible = "intel,lubbock-io";
>> +	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
>> +	#interrupt-cells = <2>;
>> +        interrupt-parent = <&pxairq>;
>
> Whitespace error.
Right.

> I'm guessing mb means motherboard?  I think it's unusual for a
> motherboard to have it's own driver.  Usually we provide drivers for
> the individual components/peripherals situated on the board.
Yes, mb for motherboard. Well, this driver is actually for the CPLDs on the
motherboard.  Would "lubbock-cplds" be a better choice ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Fri, 16 Jan 2015, Robert Jarzmik wrote:
>
>> Add a binding for lubbock motherboard IO board.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> ---
>>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> new file mode 100644
>> index 0000000..33c9e21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
>> @@ -0,0 +1,26 @@
>> +Intel's pxa255 system development platform motherboard.
>
> s/pxa25/PXA25/
Ok.
>
> "system development platform motherboard" doesn't quite sit right with me.
>
> How about "Intel Xscale PXA255 development platform (Lubbock)"?
OK.

>
>> +This is the motherboard, or IO board, of the pxa25x development system,
>> +supporting a lubbock pxa25x SoC board.
>
> Again, this sounds weird.
Would it sound better with :
  This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
  muxing, leds handling, ...
>
>> +Required properties:
>> +  - compatible : One of the following chip-specific strings:
>> +        "intel,lubbock-io"
>
> An odd thing to say with only one entry.
Right, a simple "should be" would fit better.
>
>> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
>> +                 multiplex is connected to. The only known case is GPIO0 on the
>> +                 pxa25x SoC.
>
> Can you get someone to help you re-word this into a more fluid
> sentence?
Aouch, how about this :
interrupts : The first interrupt is the SoC input interrupt connected to the
             lubbock IO board interrupt multiplexer output. The only known
             working configuration is GPIO0 on the pxa25x SoC.

>
>> +Optional properties:
>> +  - interrupts : The second optional interrupt is the base of the interrupts
>> +                 multiplexed by the lubbock motherboard. If unspecified, the
>> +                 range is fully dynamic, and the irqdomain will generate its
>> +                 interrupt base on the fly.
>> +
>> +Example:
>> +
>> +mb: lubbock-mb at 0 {
>> +	compatible = "intel,lubbock-io";
>> +	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
>> +	#interrupt-cells = <2>;
>> +        interrupt-parent = <&pxairq>;
>
> Whitespace error.
Right.

> I'm guessing mb means motherboard?  I think it's unusual for a
> motherboard to have it's own driver.  Usually we provide drivers for
> the individual components/peripherals situated on the board.
Yes, mb for motherboard. Well, this driver is actually for the CPLDs on the
motherboard.  Would "lubbock-cplds" be a better choice ?

Cheers.

-- 
Robert

Re: [PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Lee Jones]

On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Fri, 16 Jan 2015, Robert Jarzmik wrote:
> >
> >> Add a binding for lubbock motherboard IO board.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >> ---
> >>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> new file mode 100644
> >> index 0000000..33c9e21
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> @@ -0,0 +1,26 @@

[...]

> >> +This is the motherboard, or IO board, of the pxa25x development system,
> >> +supporting a lubbock pxa25x SoC board.
> >
> > Again, this sounds weird.
> Would it sound better with :
>   This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
>   muxing, leds handling, ...

Sounds a lot better, yes.

[...]

> >> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
> >> +                 multiplex is connected to. The only known case is GPIO0 on the
> >> +                 pxa25x SoC.
> >
> > Can you get someone to help you re-word this into a more fluid
> > sentence?
> Aouch, how about this :

:)

> interrupts : The first interrupt is the SoC input interrupt connected to the
>              lubbock IO board interrupt multiplexer output. The only known
>              working configuration is GPIO0 on the pxa25x SoC.

Perfect.

[...]

> > I'm guessing mb means motherboard?  I think it's unusual for a
> > motherboard to have it's own driver.  Usually we provide drivers for
> > the individual components/peripherals situated on the board.
> Yes, mb for motherboard. Well, this driver is actually for the CPLDs on the
> motherboard.  Would "lubbock-cplds" be a better choice ?

That would fit better with my current understanding of what requires a
driver and what does not.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 1/3] dt-bindings: mfd: add lubbock-io binding

[Lee Jones]

On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Fri, 16 Jan 2015, Robert Jarzmik wrote:
> >
> >> Add a binding for lubbock motherboard IO board.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >> ---
> >>  .../devicetree/bindings/mfd/lubbock-io.txt         | 26 ++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mfd/lubbock-io.txt b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> new file mode 100644
> >> index 0000000..33c9e21
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/lubbock-io.txt
> >> @@ -0,0 +1,26 @@

[...]

> >> +This is the motherboard, or IO board, of the pxa25x development system,
> >> +supporting a lubbock pxa25x SoC board.
> >
> > Again, this sounds weird.
> Would it sound better with :
>   This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
>   muxing, leds handling, ...

Sounds a lot better, yes.

[...]

> >> +  - interrupts : The first interrupt is the line the /IRQ signal the IO board
> >> +                 multiplex is connected to. The only known case is GPIO0 on the
> >> +                 pxa25x SoC.
> >
> > Can you get someone to help you re-word this into a more fluid
> > sentence?
> Aouch, how about this :

:)

> interrupts : The first interrupt is the SoC input interrupt connected to the
>              lubbock IO board interrupt multiplexer output. The only known
>              working configuration is GPIO0 on the pxa25x SoC.

Perfect.

[...]

> > I'm guessing mb means motherboard?  I think it's unusual for a
> > motherboard to have it's own driver.  Usually we provide drivers for
> > the individual components/peripherals situated on the board.
> Yes, mb for motherboard. Well, this driver is actually for the CPLDs on the
> motherboard.  Would "lubbock-cplds" be a better choice ?

That would fit better with my current understanding of what requires a
driver and what does not.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Fri, 16 Jan 2015, Robert Jarzmik wrote:

[...]

> >> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >
> > platform_get_irq()?
> No. I need the flags.

Where are they used?

[...]

> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> >> +			ret);
> >
> > I'm not keen on this type of formatting.  Besides the system will
> > print out the returned error on failure.
> Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
> driver with -ENODEV, how will I know it will come from this request_irq() or
> another part of the code ... Well I can remove it if you want, but I think it's
> an error.

I'm not asking you to remove the entire message, just the junk at the
end.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> > On Fri, 16 Jan 2015, Robert Jarzmik wrote:

[...]

> >> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >
> > platform_get_irq()?
> No. I need the flags.

Where are they used?

[...]

> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> >> +			ret);
> >
> > I'm not keen on this type of formatting.  Besides the system will
> > print out the returned error on failure.
> Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
> driver with -ENODEV, how will I know it will come from this request_irq() or
> another part of the code ... Well I can remove it if you want, but I think it's
> an error.

I'm not asking you to remove the entire message, just the junk at the
end.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Fri, 16 Jan 2015, Robert Jarzmik wrote:

[...]

> >> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >
> > platform_get_irq()?
> No. I need the flags.

Where are they used?

[...]

> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> >> +			ret);
> >
> > I'm not keen on this type of formatting.  Besides the system will
> > print out the returned error on failure.
> Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
> driver with -ENODEV, how will I know it will come from this request_irq() or
> another part of the code ... Well I can remove it if you want, but I think it's
> an error.

I'm not asking you to remove the entire message, just the junk at the
end.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Tue, Jan 20, 2015 at 10:29:19AM +0000, Lee Jones wrote:
> On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> > >> +	if (ret) {
> > >> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> > >> +			ret);
> > >
> > > I'm not keen on this type of formatting.  Besides the system will
> > > print out the returned error on failure.
> > Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
> > driver with -ENODEV, how will I know it will come from this request_irq() or
> > another part of the code ... Well I can remove it if you want, but I think it's
> > an error.
> 
> I'm not asking you to remove the entire message, just the junk at the
> end.

No.  Leave it.  If request_irq() returns -ENODEV or -ENXIO, you'll
just get the "Couldn't request main irq" message but without the
error code printed.

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

but I guess printing the IRQ number no longer makes sense with todays
dynamic mapping of logical IRQ numbers, as it is no longer meaningful.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Tue, Jan 20, 2015 at 10:29:19AM +0000, Lee Jones wrote:
> On Mon, 19 Jan 2015, Robert Jarzmik wrote:
> > >> +	if (ret) {
> > >> +		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
> > >> +			ret);
> > >
> > > I'm not keen on this type of formatting.  Besides the system will
> > > print out the returned error on failure.
> > Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
> > driver with -ENODEV, how will I know it will come from this request_irq() or
> > another part of the code ... Well I can remove it if you want, but I think it's
> > an error.
> 
> I'm not asking you to remove the entire message, just the junk at the
> end.

No.  Leave it.  If request_irq() returns -ENODEV or -ENXIO, you'll
just get the "Couldn't request main irq" message but without the
error code printed.

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

but I guess printing the IRQ number no longer makes sense with todays
dynamic mapping of logical IRQ numbers, as it is no longer meaningful.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> What I'd suggest (and always have done) is:
>
> 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> 		irq, ret);
I like it, it's even more compact, I'll use it for next patch version.

> but I guess printing the IRQ number no longer makes sense with todays
> dynamic mapping of logical IRQ numbers, as it is no longer meaningful.
Yes ... we're not yet there with pxa gpio interrupts, maybe it will come
eventually one day.

For Lee:
> > > platform_get_irq()?
> > No. I need the flags.
> Where are they used?
A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

Cheers.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> writes:

> What I'd suggest (and always have done) is:
>
> 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> 		irq, ret);
I like it, it's even more compact, I'll use it for next patch version.

> but I guess printing the IRQ number no longer makes sense with todays
> dynamic mapping of logical IRQ numbers, as it is no longer meaningful.
Yes ... we're not yet there with pxa gpio interrupts, maybe it will come
eventually one day.

For Lee:
> > > platform_get_irq()?
> > No. I need the flags.
> Where are they used?
A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

Cheers.

-- 
Robert

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> What I'd suggest (and always have done) is:
>
> 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> 		irq, ret);
I like it, it's even more compact, I'll use it for next patch version.

> but I guess printing the IRQ number no longer makes sense with todays
> dynamic mapping of logical IRQ numbers, as it is no longer meaningful.
Yes ... we're not yet there with pxa gpio interrupts, maybe it will come
eventually one day.

For Lee:
> > > platform_get_irq()?
> > No. I need the flags.
> Where are they used?
A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

Cheers.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> > > > platform_get_irq()?
> > > No. I need the flags.
> > Where are they used?
> A couple of lines below, using local "irqflags" variable :
>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>                               irqflags, dev_name(&pdev->dev), cot);

No, I mean where are they _used_?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> > > > platform_get_irq()?
> > > No. I need the flags.
> > Where are they used?
> A couple of lines below, using local "irqflags" variable :
>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>                               irqflags, dev_name(&pdev->dev), cot);

No, I mean where are they _used_?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
>> > > > platform_get_irq()?
>> > > No. I need the flags.
>> > Where are they used?
>> A couple of lines below, using local "irqflags" variable :
>>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>>                               irqflags, dev_name(&pdev->dev), cot);
>
> No, I mean where are they _used_?
I don't get your point.

For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()
??? And if you think ahead, then there are used all around in
kernel/irq/manage.c, as you probably know.

Cheers.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
>> > > > platform_get_irq()?
>> > > No. I need the flags.
>> > Where are they used?
>> A couple of lines below, using local "irqflags" variable :
>>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>>                               irqflags, dev_name(&pdev->dev), cot);
>
> No, I mean where are they _used_?
I don't get your point.

For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()
??? And if you think ahead, then there are used all around in
kernel/irq/manage.c, as you probably know.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:

> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
>> > > > platform_get_irq()?
>> > > No. I need the flags.
>> > Where are they used?
>> A couple of lines below, using local "irqflags" variable :
>>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
>>                               irqflags, dev_name(&pdev->dev), cot);
>
> No, I mean where are they _used_?
I don't get your point.

For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()
??? And if you think ahead, then there are used all around in
kernel/irq/manage.c, as you probably know.

Cheers.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > What I'd suggest (and always have done) is:
> >
> > 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> > 		irq, ret);
> I like it, it's even more compact, I'll use it for next patch version.

BTW, this is an example why I have the policy of always ensuring that
the kernel messages print sufficient diagnostics.  Right now, I have
a problem - since I rebooted my firewall a few nights ago, I now get
on one of my machines:

  rt6_redirect: source isn't a valid nexthop for redirect target

and it spews that for a few minutes every 26 hours or so.  No further
information, and it leaves you wondering "well, what was the invalid
next hop?  What was the source?"

Pretty much the only way to try and find out is to leave a tcpdump or
wireshark running for 24 hours to try and get a dump - which is not
that easy if you don't have lots of disk space.  So, right now, I have
no way to diagnose the above.

If it printed that information, then I'd be able to see what the
addresses were, and I'd probably be able to come up with a tcpdump
filter which didn't involve logging all IPv6 traffic.

Kernel messages need to be smart.  If not, they might as well just be
"The kernel encountered a problem. Abort, Retry or Fail?"

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > What I'd suggest (and always have done) is:
> >
> > 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> > 		irq, ret);
> I like it, it's even more compact, I'll use it for next patch version.

BTW, this is an example why I have the policy of always ensuring that
the kernel messages print sufficient diagnostics.  Right now, I have
a problem - since I rebooted my firewall a few nights ago, I now get
on one of my machines:

  rt6_redirect: source isn't a valid nexthop for redirect target

and it spews that for a few minutes every 26 hours or so.  No further
information, and it leaves you wondering "well, what was the invalid
next hop?  What was the source?"

Pretty much the only way to try and find out is to leave a tcpdump or
wireshark running for 24 hours to try and get a dump - which is not
that easy if you don't have lots of disk space.  So, right now, I have
no way to diagnose the above.

If it printed that information, then I'd be able to see what the
addresses were, and I'd probably be able to come up with a tcpdump
filter which didn't involve logging all IPv6 traffic.

Kernel messages need to be smart.  If not, they might as well just be
"The kernel encountered a problem. Abort, Retry or Fail?"

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:16:45AM +0000, Lee Jones wrote:
> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> > > > > platform_get_irq()?
> > > > No. I need the flags.
> > > Where are they used?
> > A couple of lines below, using local "irqflags" variable :
> >        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >                               irqflags, dev_name(&pdev->dev), cot);
> 
> No, I mean where are they _used_?

They're used in request_irq, and request_irq uses it to set the interrupt
type (edge or level) as specified in the IRQ resource.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:16:45AM +0000, Lee Jones wrote:
> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> > > > > platform_get_irq()?
> > > > No. I need the flags.
> > > Where are they used?
> > A couple of lines below, using local "irqflags" variable :
> >        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >                               irqflags, dev_name(&pdev->dev), cot);
> 
> No, I mean where are they _used_?

They're used in request_irq, and request_irq uses it to set the interrupt
type (edge or level) as specified in the IRQ resource.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:16:45AM +0000, Lee Jones wrote:
> On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> > > > > platform_get_irq()?
> > > > No. I need the flags.
> > > Where are they used?
> > A couple of lines below, using local "irqflags" variable :
> >        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >                               irqflags, dev_name(&pdev->dev), cot);
> 
> No, I mean where are they _used_?

They're used in request_irq, and request_irq uses it to set the interrupt
type (edge or level) as specified in the IRQ resource.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> >> > > > platform_get_irq()?
> >> > > No. I need the flags.
> >> > Where are they used?
> >> A couple of lines below, using local "irqflags" variable :
> >>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >>                               irqflags, dev_name(&pdev->dev), cot);
> >
> > No, I mean where are they _used_?
> I don't get your point.
> 
> For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()

No, this is where the argument is being _passed_.

> ??? And if you think ahead, then there are used all around in
> kernel/irq/manage.c, as you probably know.

Perhaps some context would help.

Looking at one of the other patches in the series it appears the flag
you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
where this is being _used_ (think 'consumed, rather than passed.  I
only see a single entry in drivers/pnp/interface.c.

That's what got me thinking... are you sure you're a) making use of
this flag and b) assuming the answer to 'a' is "no" and if everyting's
working correctly in the code's current state, are you sure you need
them at all? 

I think to set an edge trigger on an IRQ, you should instead do so via
irq_set_irq_type(), or have a missed a line or two?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> 
> > On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> >> > > > platform_get_irq()?
> >> > > No. I need the flags.
> >> > Where are they used?
> >> A couple of lines below, using local "irqflags" variable :
> >>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >>                               irqflags, dev_name(&pdev->dev), cot);
> >
> > No, I mean where are they _used_?
> I don't get your point.
> 
> For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()

No, this is where the argument is being _passed_.

> ??? And if you think ahead, then there are used all around in
> kernel/irq/manage.c, as you probably know.

Perhaps some context would help.

Looking at one of the other patches in the series it appears the flag
you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
where this is being _used_ (think 'consumed, rather than passed.  I
only see a single entry in drivers/pnp/interface.c.

That's what got me thinking... are you sure you're a) making use of
this flag and b) assuming the answer to 'a' is "no" and if everyting's
working correctly in the code's current state, are you sure you need
them at all? 

I think to set an edge trigger on an IRQ, you should instead do so via
irq_set_irq_type(), or have a missed a line or two?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > On Wed, 21 Jan 2015, Robert Jarzmik wrote:
> >> > > > platform_get_irq()?
> >> > > No. I need the flags.
> >> > Where are they used?
> >> A couple of lines below, using local "irqflags" variable :
> >>        ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
> >>                               irqflags, dev_name(&pdev->dev), cot);
> >
> > No, I mean where are they _used_?
> I don't get your point.
> 
> For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq()

No, this is where the argument is being _passed_.

> ??? And if you think ahead, then there are used all around in
> kernel/irq/manage.c, as you probably know.

Perhaps some context would help.

Looking at one of the other patches in the series it appears the flag
you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
where this is being _used_ (think 'consumed, rather than passed.  I
only see a single entry in drivers/pnp/interface.c.

That's what got me thinking... are you sure you're a) making use of
this flag and b) assuming the answer to 'a' is "no" and if everyting's
working correctly in the code's current state, are you sure you need
them at all? 

I think to set an edge trigger on an IRQ, you should instead do so via
irq_set_irq_type(), or have a missed a line or two?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 12:35:51PM +0000, Lee Jones wrote:
> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?

Setting the IRQ type in request_irq() is perfectly acceptable.

include/linux/interrupt.h:
/*
 * These correspond to the IORESOURCE_IRQ_* defines in
 * linux/ioport.h to select the interrupt line behaviour.  When
 * requesting an interrupt without specifying a IRQF_TRIGGER, the
 * setting should be assumed to be "as already configured", which
 * may be as per machine or firmware initialisation.
 */
#define IRQF_TRIGGER_NONE       0x00000000
#define IRQF_TRIGGER_RISING     0x00000001
#define IRQF_TRIGGER_FALLING    0x00000002
#define IRQF_TRIGGER_HIGH       0x00000004
#define IRQF_TRIGGER_LOW        0x00000008


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 12:35:51PM +0000, Lee Jones wrote:
> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?

Setting the IRQ type in request_irq() is perfectly acceptable.

include/linux/interrupt.h:
/*
 * These correspond to the IORESOURCE_IRQ_* defines in
 * linux/ioport.h to select the interrupt line behaviour.  When
 * requesting an interrupt without specifying a IRQF_TRIGGER, the
 * setting should be assumed to be "as already configured", which
 * may be as per machine or firmware initialisation.
 */
#define IRQF_TRIGGER_NONE       0x00000000
#define IRQF_TRIGGER_RISING     0x00000001
#define IRQF_TRIGGER_FALLING    0x00000002
#define IRQF_TRIGGER_HIGH       0x00000004
#define IRQF_TRIGGER_LOW        0x00000008


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 12:35:51PM +0000, Lee Jones wrote:
> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?

Setting the IRQ type in request_irq() is perfectly acceptable.

include/linux/interrupt.h:
/*
 * These correspond to the IORESOURCE_IRQ_* defines in
 * linux/ioport.h to select the interrupt line behaviour.  When
 * requesting an interrupt without specifying a IRQF_TRIGGER, the
 * setting should be assumed to be "as already configured", which
 * may be as per machine or firmware initialisation.
 */
#define IRQF_TRIGGER_NONE       0x00000000
#define IRQF_TRIGGER_RISING     0x00000001
#define IRQF_TRIGGER_FALLING    0x00000002
#define IRQF_TRIGGER_HIGH       0x00000004
#define IRQF_TRIGGER_LOW        0x00000008


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[robert.jarzmik]

> ----- Mail original -----
> De: "Lee Jones" <lee.jones@linaro.org>
First of all, this is my web mail interface, so please be kind with
my mail formatting ...

> Looking at one of the other patches in the series it appears the flag
> you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
> where this is being _used_ (think 'consumed, rather than passed.  I
> only see a single entry in drivers/pnp/interface.c.
Look at this call chain :
request_irq()
  setup_irq()
    __setup_irq()
      __irq_set_trigger()
        pxa_gpio_irq_type() (aka. chip->irq_set_type)
          => hardware register manipulation

Moreover, you should be aware of the bijection existing between :
 - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING
 - it's HIGHEDGE twin
 - it is noted in : include/linux/interrupt.h

> That's what got me thinking... are you sure you're a) making use of
this flag
Yes, I'm quite sure.

> b) assuming the answer to 'a' is "no"
I won't :)

> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?
__setup_irq(), that's the entry point.

Cheers.

--
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[robert.jarzmik-GANU6spQydw]

> ----- Mail original -----
> De: "Lee Jones" <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
First of all, this is my web mail interface, so please be kind with
my mail formatting ...

> Looking at one of the other patches in the series it appears the flag
> you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
> where this is being _used_ (think 'consumed, rather than passed.  I
> only see a single entry in drivers/pnp/interface.c.
Look at this call chain :
request_irq()
  setup_irq()
    __setup_irq()
      __irq_set_trigger()
        pxa_gpio_irq_type() (aka. chip->irq_set_type)
          => hardware register manipulation

Moreover, you should be aware of the bijection existing between :
 - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING
 - it's HIGHEDGE twin
 - it is noted in : include/linux/interrupt.h

> That's what got me thinking... are you sure you're a) making use of
this flag
Yes, I'm quite sure.

> b) assuming the answer to 'a' is "no"
I won't :)

> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?
__setup_irq(), that's the entry point.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[robert.jarzmik at free.fr]

> ----- Mail original -----
> De: "Lee Jones" <lee.jones@linaro.org>
First of all, this is my web mail interface, so please be kind with
my mail formatting ...

> Looking at one of the other patches in the series it appears the flag
> you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
> where this is being _used_ (think 'consumed, rather than passed.  I
> only see a single entry in drivers/pnp/interface.c.
Look at this call chain :
request_irq()
  setup_irq()
    __setup_irq()
      __irq_set_trigger()
        pxa_gpio_irq_type() (aka. chip->irq_set_type)
          => hardware register manipulation

Moreover, you should be aware of the bijection existing between :
 - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING
 - it's HIGHEDGE twin
 - it is noted in : include/linux/interrupt.h

> That's what got me thinking... are you sure you're a) making use of
this flag
Yes, I'm quite sure.

> b) assuming the answer to 'a' is "no"
I won't :)

> I think to set an edge trigger on an IRQ, you should instead do so via
> irq_set_irq_type(), or have a missed a line or two?
__setup_irq(), that's the entry point.

Cheers.

--
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, robert.jarzmik@free.fr wrote:

> > ----- Mail original -----
> > De: "Lee Jones" <lee.jones@linaro.org>
> First of all, this is my web mail interface, so please be kind with
> my mail formatting ...

I have no idea what you want me to change or do differently?

Perhaps it might be more prudent for you to switch to a quality
mailer?

> > Looking at one of the other patches in the series it appears the flag
> > you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
> > where this is being _used_ (think 'consumed, rather than passed.  I
> > only see a single entry in drivers/pnp/interface.c.
> Look at this call chain :
> request_irq()
>   setup_irq()
>     __setup_irq()
>       __irq_set_trigger()
>         pxa_gpio_irq_type() (aka. chip->irq_set_type)
>           => hardware register manipulation
> 
> Moreover, you should be aware of the bijection existing between :
>  - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING
>  - it's HIGHEDGE twin
>  - it is noted in : include/linux/interrupt.h
> 
> > That's what got me thinking... are you sure you're a) making use of
> this flag
> Yes, I'm quite sure.
> 
> > b) assuming the answer to 'a' is "no"
> I won't :)
> 
> > I think to set an edge trigger on an IRQ, you should instead do so via
> > irq_set_irq_type(), or have a missed a line or two?
> __setup_irq(), that's the entry point.

Very well, Russell and yourself have convinced me.  If you fixup the
remainder of comments, I'm happy.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Lee Jones]

On Wed, 21 Jan 2015, robert.jarzmik at free.fr wrote:

> > ----- Mail original -----
> > De: "Lee Jones" <lee.jones@linaro.org>
> First of all, this is my web mail interface, so please be kind with
> my mail formatting ...

I have no idea what you want me to change or do differently?

Perhaps it might be more prudent for you to switch to a quality
mailer?

> > Looking at one of the other patches in the series it appears the flag
> > you're trying to capture is IORESOURCE_IRQ_LOWEDGE.  When I grep for
> > where this is being _used_ (think 'consumed, rather than passed.  I
> > only see a single entry in drivers/pnp/interface.c.
> Look at this call chain :
> request_irq()
>   setup_irq()
>     __setup_irq()
>       __irq_set_trigger()
>         pxa_gpio_irq_type() (aka. chip->irq_set_type)
>           => hardware register manipulation
> 
> Moreover, you should be aware of the bijection existing between :
>  - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING
>  - it's HIGHEDGE twin
>  - it is noted in : include/linux/interrupt.h
> 
> > That's what got me thinking... are you sure you're a) making use of
> this flag
> Yes, I'm quite sure.
> 
> > b) assuming the answer to 'a' is "no"
> I won't :)
> 
> > I think to set an edge trigger on an IRQ, you should instead do so via
> > irq_set_irq_type(), or have a missed a line or two?
> __setup_irq(), that's the entry point.

Very well, Russell and yourself have convinced me.  If you fixup the
remainder of comments, I'm happy.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Joe Perches]

(adding netdev)

On Wed, 2015-01-21 at 09:44 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:
> > Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> > 
> > > What I'd suggest (and always have done) is:
> > >
> > > 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> > > 		irq, ret);
> > I like it, it's even more compact, I'll use it for next patch version.
> 
> BTW, this is an example why I have the policy of always ensuring that
> the kernel messages print sufficient diagnostics.  Right now, I have
> a problem - since I rebooted my firewall a few nights ago, I now get
> on one of my machines:
> 
>   rt6_redirect: source isn't a valid nexthop for redirect target
> 
> and it spews that for a few minutes every 26 hours or so.  No further
> information, and it leaves you wondering "well, what was the invalid
> next hop?  What was the source?"
> 
> Pretty much the only way to try and find out is to leave a tcpdump or
> wireshark running for 24 hours to try and get a dump - which is not
> that easy if you don't have lots of disk space.  So, right now, I have
> no way to diagnose the above.
> 
> If it printed that information, then I'd be able to see what the
> addresses were, and I'd probably be able to come up with a tcpdump
> filter which didn't involve logging all IPv6 traffic.
> 
> Kernel messages need to be smart.  If not, they might as well just be
> "The kernel encountered a problem. Abort, Retry or Fail?"
> 

unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Joe Perches]

(adding netdev)

On Wed, 2015-01-21 at 09:44 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:
> > Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> > 
> > > What I'd suggest (and always have done) is:
> > >
> > > 	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
> > > 		irq, ret);
> > I like it, it's even more compact, I'll use it for next patch version.
> 
> BTW, this is an example why I have the policy of always ensuring that
> the kernel messages print sufficient diagnostics.  Right now, I have
> a problem - since I rebooted my firewall a few nights ago, I now get
> on one of my machines:
> 
>   rt6_redirect: source isn't a valid nexthop for redirect target
> 
> and it spews that for a few minutes every 26 hours or so.  No further
> information, and it leaves you wondering "well, what was the invalid
> next hop?  What was the source?"
> 
> Pretty much the only way to try and find out is to leave a tcpdump or
> wireshark running for 24 hours to try and get a dump - which is not
> that easy if you don't have lots of disk space.  So, right now, I have
> no way to diagnose the above.
> 
> If it printed that information, then I'd be able to see what the
> addresses were, and I'd probably be able to come up with a tcpdump
> filter which didn't involve logging all IPv6 traffic.
> 
> Kernel messages need to be smart.  If not, they might as well just be
> "The kernel encountered a problem. Abort, Retry or Fail?"
> 

Re: unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> (adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> (adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> (adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Joe Perches]

On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> > (adding netdev)
> 
> I wasn't actually reporting that as an issue; I was using it as an
> example.  It's from a very old kernel (2.6.27.21) which I run on one
> of my old x86 machines.

It's still the same code.
If the message can be improved, why not do it?

unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Joe Perches]

On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> > (adding netdev)
> 
> I wasn't actually reporting that as an issue; I was using it as an
> example.  It's from a very old kernel (2.6.27.21) which I run on one
> of my old x86 machines.

It's still the same code.
If the message can be improved, why not do it?

Re: unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:40:44AM -0800, Joe Perches wrote:
> On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:
> > On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> > > (adding netdev)
> > 
> > I wasn't actually reporting that as an issue; I was using it as an
> > example.  It's from a very old kernel (2.6.27.21) which I run on one
> > of my old x86 machines.
> 
> It's still the same code.
> If the message can be improved, why not do it?

I assume you're taking the responsibility to test anything that comes
out of this then?

I'm not; I tried updating the kernel on the machine to 2.6.32 many
years ago and that was a no-go because of userspace (udev)
incompatibilities.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

unclear ipv6 redirect message (was Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board)

[Russell King - ARM Linux]

On Wed, Jan 21, 2015 at 08:40:44AM -0800, Joe Perches wrote:
> On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:
> > On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> > > (adding netdev)
> > 
> > I wasn't actually reporting that as an issue; I was using it as an
> > example.  It's from a very old kernel (2.6.27.21) which I run on one
> > of my old x86 machines.
> 
> It's still the same code.
> If the message can be improved, why not do it?

I assume you're taking the responsibility to test anything that comes
out of this then?

I'm not; I tried updating the kernel on the machine to 2.6.32 many
years ago and that was a no-go because of userspace (udev)
incompatibilities.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:
> Very well, Russell and yourself have convinced me.  If you fixup the
> remainder of comments, I'm happy.
Cool.

Let me a couple of days to gather my wits, cross-check I have not forgotten a
comment, make some testing on the board and then post v4.

Cheers.

-- 
Robert

Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> Very well, Russell and yourself have convinced me.  If you fixup the
> remainder of comments, I'm happy.
Cool.

Let me a couple of days to gather my wits, cross-check I have not forgotten a
comment, make some testing on the board and then post v4.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:
> Very well, Russell and yourself have convinced me.  If you fixup the
> remainder of comments, I'm happy.
Cool.

Let me a couple of days to gather my wits, cross-check I have not forgotten a
comment, make some testing on the board and then post v4.

Cheers.

-- 
Robert