[PATCH v4 1/4] dt-bindings: mfd: add lubbock-cplds binding

[PATCH v4 1/4] dt-bindings: mfd: add lubbock-cplds binding

[Robert Jarzmik]

Add a binding for lubbock motherboard IO board.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v3: name change to lubbock-cplds,
          Lee's comments taken into account.
---
 .../devicetree/bindings/mfd/lubbock-cplds.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/lubbock-cplds.txt

diff --git a/Documentation/devicetree/bindings/mfd/lubbock-cplds.txt b/Documentation/devicetree/bindings/mfd/lubbock-cplds.txt
new file mode 100644
index 0000000..211ed9a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/lubbock-cplds.txt
@@ -0,0 +1,26 @@
+Intel XScale PXA255 development platform (Lubbock).
+
+This regroups all the CPLDs on the Lubbock motherboard, providing interrupt
+muxing, leds handling, ...
+
+Required properties:
+  - compatible : should be "intel,lubbock-cplds"
+
+  - interrupts : The first interrupt is the SoC input interrupt connected to the
+                 lubbock IO board interrupt multiplexer output. The only known
+                 working conifguration 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-cplds";
+	interrupts = <0 IRQ_TYPE_EDGE_FALLING 400 IRQ_TYPE_NONE>;
+	#interrupt-cells = <2>;
+	interrupt-parent = <&pxairq>;
+};
-- 
2.1.0

[PATCH v4 2/4] mfd: lubbock_cplds: 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
Since v3: take into account Lee's review + Russell's advice
            whitespace errors fixes
	    debug/info messages formatting
	    return X; empty lines
---
 drivers/mfd/Kconfig   |  10 +++
 drivers/mfd/Makefile  |   1 +
 drivers/mfd/lubbock.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 210 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..4077fc5
--- /dev/null
+++ b/drivers/mfd/lubbock.c
@@ -0,0 +1,199 @@
+/*
+ * 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;
+
+	base_irq = platform_get_irq(pdev, 1);
+	if (base_irq < 0)
+		base_irq = 0;
+
+	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%d: %d\n",
+			cot->irq, 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;
+
+	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;
+		}
+	}
+
+	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-cplds", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lubbock_id_table);
+
+static struct platform_driver lubbock_driver = {
+	.driver		= {
+		.name	= "lubbock_cplds",
+		.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 MFD driver");
+MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>");
+MODULE_LICENSE("GPL");
-- 
2.1.0

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Lee Jones]

On Sat, 24 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.
> 
> 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)

Are you sure this is what you want to do?  We don't usually support
'boards' per say.  Instead we support 'devices', then pull each of
those devices together using some h/w description mechanism.

Board files are so yesterday!

Besides, this is MFD, where we support single pieces of silicon which
happen to support multiple devices.  I definitely don't want to support
boards here.

You might want to re-think the naming and your (sales) pitch.

> 	  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
> Since v3: take into account Lee's review + Russell's advice
>             whitespace errors fixes
> 	    debug/info messages formatting
> 	    return X; empty lines
> ---
>  drivers/mfd/Kconfig   |  10 +++
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/lubbock.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 210 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..4077fc5
> --- /dev/null
> +++ b/drivers/mfd/lubbock.c
> @@ -0,0 +1,199 @@
> +/*
> + * 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>

Why have you included this?  I don't see the use of the MFD framework
anywhere.  So what makes this an MFD?

I'm going to stop here, as I think I need more of an explanation so
what you're trying to achieve with this driver.

[...]

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

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[robert.jarzmik at free.fr]

----- Mail original -----
De: "Lee Jones" <lee.jones@linaro.org>
?: "Robert Jarzmik" <robert.jarzmik@free.fr>
Cc: "Rob Herring" <robh+dt@kernel.org>, "Pawel Moll" <pawel.moll@arm.com>, "Mark Rutland" <mark.rutland@arm.com>, "Ian Campbell" <ijc+devicetree@hellion.org.uk>, "Kumar Gala" <galak@codeaurora.org>, "Daniel Mack" <daniel@zonque.org>, "Haojian Zhuang" <haojian.zhuang@gmail.com>, "Samuel Ortiz" <sameo@linux.intel.com>, "Grant Likely" <grant.likely@linaro.org>, devicetree at vger.kernel.org, linux-kernel at vger.kernel.org, linux-arm-kernel at lists.infradead.org, "Arnd Bergmann" <arnd@arndb.de>, "Russell King - ARM Linux" <linux@arm.linux.org.uk>, "Dmitry Eremin-Solenikov" <dbaryshkov@gmail.com>
Envoy?: Lundi 16 F?vrier 2015 14:05:49
Objet: Re: [PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

On Sat, 24 Jan 2015, Robert Jarzmik wrote:

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

> Are you sure this is what you want to do?  We don't usually support
> 'boards' per say.  Instead we support 'devices', then pull each of
> those devices together using some h/w description mechanism.

Do you know that :
 1) anything under "---" in a commit message is thrown away
 2) after v2, we _both_ agreed that the accurate name is "cplds"
    which exactly what is in this patch
    (see device registering with lubbock_cplds).
 3) there is no more mention of "board" anywhere in the patch core

> Besides, this is MFD, where we support single pieces of silicon which
> happen to support multiple devices.  I definitely don't want to support
> boards here.
> You might want to re-think the naming and your (sales) pitch.
I might need help. As for the (sales), no comment.

>> +#include <linux/mfd/core.h>
> Why have you included this?  I don't see the use of the MFD framework
> anywhere.  So what makes this an MFD?
I thought cplds were to be handled by an MFD driver.

> I'm going to stop here, as I think I need more of an explanation so
> what you're trying to achieve with this driver.
Why ? I think things were clear that this driver handles the CPLDs on
lubbock board, namely u46 and u52. I don't understand what is wrong
with this patch so that you don't want to go forward.

--

Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Lee Jones]

On Mon, 16 Feb 2015, robert.jarzmik at free.fr wrote:

> ----- Mail original -----
> De: "Lee Jones" <lee.jones@linaro.org>
> ?: "Robert Jarzmik" <robert.jarzmik@free.fr>
> Cc: "Rob Herring" <robh+dt@kernel.org>, "Pawel Moll" <pawel.moll@arm.com>, "Mark Rutland" <mark.rutland@arm.com>, "Ian Campbell" <ijc+devicetree@hellion.org.uk>, "Kumar Gala" <galak@codeaurora.org>, "Daniel Mack" <daniel@zonque.org>, "Haojian Zhuang" <haojian.zhuang@gmail.com>, "Samuel Ortiz" <sameo@linux.intel.com>, "Grant Likely" <grant.likely@linaro.org>, devicetree at vger.kernel.org, linux-kernel at vger.kernel.org, linux-arm-kernel at lists.infradead.org, "Arnd Bergmann" <arnd@arndb.de>, "Russell King - ARM Linux" <linux@arm.linux.org.uk>, "Dmitry Eremin-Solenikov" <dbaryshkov@gmail.com>
> Envoy?: Lundi 16 F?vrier 2015 14:05:49
> Objet: Re: [PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

What's all this?  Please configure your mail client correctly.

For advice, see:

  Documentation/email-clients.txt

> On Sat, 24 Jan 2015, Robert Jarzmik wrote:
> 
> > ---
> > 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)
> 
> > Are you sure this is what you want to do?  We don't usually support
> > 'boards' per say.  Instead we support 'devices', then pull each of
> > those devices together using some h/w description mechanism.
> 
> Do you know that :
>  1) anything under "---" in a commit message is thrown away

Yes.  But I don't remember reading this passage before.  Just becuase
this is no longer v2, it doesn't void any comments regarding v1
changelogs.

>  2) after v2, we _both_ agreed that the accurate name is "cplds"
>     which exactly what is in this patch
>     (see device registering with lubbock_cplds).

There is no mention of this decision in the changelogs.  If it's not
in the change logs, it didn't happen. ;)

I'm still concerned with the fact that the driver file is named using
and is populated by lots of instances of a "board" name.  I'm sure you
would share my thoughts is someone submitted a driver called
beaglebone.c or raspberrypi.c to MFD.

>  3) there is no more mention of "board" anywhere in the patch core

No, just the name of one.

> > Besides, this is MFD, where we support single pieces of silicon which
> > happen to support multiple devices.  I definitely don't want to support
> > boards here.
> > You might want to re-think the naming and your (sales) pitch.
> I might need help. As for the (sales), no comment.

By pitch, I mean to convince me that this belongs in MFD.

> >> +#include <linux/mfd/core.h>
> > Why have you included this?  I don't see the use of the MFD framework
> > anywhere.  So what makes this an MFD?
> I thought cplds were to be handled by an MFD driver.

Not to my knowledge, although we do appear to have one.  That doesn't
necessarily mean that it's the right place for it though.  I'm not
entirely sure how CPLDs even work on a functional level.

> > I'm going to stop here, as I think I need more of an explanation so
> > what you're trying to achieve with this driver.
> Why ? I think things were clear that this driver handles the CPLDs on
> lubbock board, namely u46 and u52. I don't understand what is wrong
> with this patch so that you don't want to go forward.

Understanding was lost when I learned that the whole file was centred
around the premise of board support.  I understand now that this is a
driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
really a CPLD driver, shouldn't be named after the manufacturer/model
number of the chip, rather than the PCB which it's connected to?

I'm also concerned that this driver has no functional CPLD code
contained.  All you're doing is defining an IRQ domain.  Why then
isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
driver?

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

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

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

> What's all this?  Please configure your mail client correctly.
>
> For advice, see:
>
>   Documentation/email-clients.txt
While at day work, I have only access to web mail ...

>>  2) after v2, we _both_ agreed that the accurate name is "cplds"
>>     which exactly what is in this patch
>>     (see device registering with lubbock_cplds).
>
> There is no mention of this decision in the changelogs.  If it's not
> in the change logs, it didn't happen. ;)
Ah right.

> I'm still concerned with the fact that the driver file is named using
> and is populated by lots of instances of a "board" name.  I'm sure you
> would share my thoughts is someone submitted a driver called
> beaglebone.c or raspberrypi.c to MFD.
I understand your concern. Arnd, a thought about it ? The only viable
alternative would be to move it to arch/arm/plat-pxa AFAIS.

>> > Besides, this is MFD, where we support single pieces of silicon which
>> > happen to support multiple devices.  I definitely don't want to support
>> > boards here.
>> > You might want to re-think the naming and your (sales) pitch.
>> I might need help. As for the (sales), no comment.
>
> By pitch, I mean to convince me that this belongs in MFD.
I've been trying.

>> I thought cplds were to be handled by an MFD driver.
>
> Not to my knowledge, although we do appear to have one.  That doesn't
> necessarily mean that it's the right place for it though.  I'm not
> entirely sure how CPLDs even work on a functional level.
As I said, I understand your concern.

>> > I'm going to stop here, as I think I need more of an explanation so
>> > what you're trying to achieve with this driver.
>> Why ? I think things were clear that this driver handles the CPLDs on
>> lubbock board, namely u46 and u52. I don't understand what is wrong
>> with this patch so that you don't want to go forward.
>
> Understanding was lost when I learned that the whole file was centred
> around the premise of board support.  I understand now that this is a
> driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
> really a CPLD driver, shouldn't be named after the manufacturer/model
> number of the chip, rather than the PCB which it's connected to?
>
> I'm also concerned that this driver has no functional CPLD code
> contained.  All you're doing is defining an IRQ domain.  Why then
> isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
> driver?
Is not only a irqchip because, as explained at the bottom of the commit message,
quoting myself :
  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

When these parts will be added, and they'll have to be handled by this driver
because of iospace sharing, and same IP (cplds) providing the hardware support,
the irqchip way will be just impossible to follow.

As I said above, I understand your concern. If we all reach a concensus this
should be moved to the pxa tree, so be it. But I'd like other opinion here.

Cheers.

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Lee Jones]

Arnd, Greg,

  Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?

On Mon, 16 Feb 2015, Robert Jarzmik wrote:
> Lee Jones wrote:
> > What's all this?  Please configure your mail client correctly.
> >
> > For advice, see:
> >
> >   Documentation/email-clients.txt
> While at day work, I have only access to web mail ...

Probably best to hold off your reply until you get home then.  It's
not just a formatting issue, you also exposed many raw email addresses
to the internet, which are easily harvested up by web crawlers.

See this:
  https://lkml.org/lkml/2015/2/16/247

> >>  2) after v2, we _both_ agreed that the accurate name is "cplds"
> >>     which exactly what is in this patch
> >>     (see device registering with lubbock_cplds).
> >
> > There is no mention of this decision in the changelogs.  If it's not
> > in the change logs, it didn't happen. ;)
> Ah right.
> 
> > I'm still concerned with the fact that the driver file is named using
> > and is populated by lots of instances of a "board" name.  I'm sure you
> > would share my thoughts is someone submitted a driver called
> > beaglebone.c or raspberrypi.c to MFD.
> I understand your concern. Arnd, a thought about it ? The only viable
> alternative would be to move it to arch/arm/plat-pxa AFAIS.

I don't think this is correct either.  CPLD handling would probably be
slightly less out of place in drivers/misc, but perhaps a new
subsystem for PLDs/CPLDs/FPGAs would be more appropriate
drivers/programmables or similar maybe.

> >> > Besides, this is MFD, where we support single pieces of silicon which
> >> > happen to support multiple devices.  I definitely don't want to support
> >> > boards here.
> >> > You might want to re-think the naming and your (sales) pitch.
> >> I might need help. As for the (sales), no comment.
> >
> > By pitch, I mean to convince me that this belongs in MFD.
> I've been trying.

I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
mean I'm going to leave you on the curb.  I'd like to help you get it
into a better home.

[...]

> > Understanding was lost when I learned that the whole file was centred
> > around the premise of board support.  I understand now that this is a
> > driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
> > really a CPLD driver, shouldn't be named after the manufacturer/model
> > number of the chip, rather than the PCB which it's connected to?
> >
> > I'm also concerned that this driver has no functional CPLD code
> > contained.  All you're doing is defining an IRQ domain.  Why then
> > isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
> > driver?
> Is not only a irqchip because, as explained at the bottom of the commit message,
> quoting myself :
>   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

I had a conversation about this on IRC yesterday and some good
points/questions were posed.  This is a difficult area, because you
can program these things to do whatever you like.  Depending on the
'intention' (and it is only an intention -- someone else can come
along and reprogram these devices on a whim), the CPLD code could live
anywhere.  If you wanted to put watchdog functionality in there, then
there is an argument for it to live in drivers/watchdog, etc etc.  So
just because the plan is to support a few (i.e. more than one) simple
devices, it doesn't necessarily mean that the handling should be done
in MFD.

Yesterday I was asked "Are you wanting to restrict drivers in
drivers/mfd to those that make use of MFD_CORE functionality?".  My
answer to that was "No, however; I only want devices which
_intrinsically_ operate in multiple subsystems", which these
programmables no not do.

FYI, you're not on your own here.  There is at least one of these
devices in the kernel already and upon a short inspection there
appears to be a number of Out-of-Tree (OoT) drivers out there which
will require a home in Mainline sooner or later.

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

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Nicolas Pitre]

On Tue, 17 Feb 2015, Lee Jones wrote:

> Arnd, Greg,
> 
>   Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?

FWIW...

The Lubbock is an ancient development board (circa 2003) using a CPLD to 
multiplex a couple things on the board.  I really doubt anyone would 
reprogram this CPLD at this point. So I'd treat it just like another 
interrupt controller + random peripherals that will never change.  And 
yes, maybe a more appropriate name is needed.

And I happen to have one such beast on my desk wasting significant 
realestate and picking up dust.  I don't think I booted it even once in 
the last 3 years.  I'm seriously considering a trip to the recycling 
facility to dispose of it unless someone wants it and is ready to pay 
for the shipping.

> On Mon, 16 Feb 2015, Robert Jarzmik wrote:
> > Lee Jones wrote:
> > > What's all this?  Please configure your mail client correctly.
> > >
> > > For advice, see:
> > >
> > >   Documentation/email-clients.txt
> > While at day work, I have only access to web mail ...
> 
> Probably best to hold off your reply until you get home then.  It's
> not just a formatting issue, you also exposed many raw email addresses
> to the internet, which are easily harvested up by web crawlers.
> 
> See this:
>   https://lkml.org/lkml/2015/2/16/247
> 
> > >>  2) after v2, we _both_ agreed that the accurate name is "cplds"
> > >>     which exactly what is in this patch
> > >>     (see device registering with lubbock_cplds).
> > >
> > > There is no mention of this decision in the changelogs.  If it's not
> > > in the change logs, it didn't happen. ;)
> > Ah right.
> > 
> > > I'm still concerned with the fact that the driver file is named using
> > > and is populated by lots of instances of a "board" name.  I'm sure you
> > > would share my thoughts is someone submitted a driver called
> > > beaglebone.c or raspberrypi.c to MFD.
> > I understand your concern. Arnd, a thought about it ? The only viable
> > alternative would be to move it to arch/arm/plat-pxa AFAIS.
> 
> I don't think this is correct either.  CPLD handling would probably be
> slightly less out of place in drivers/misc, but perhaps a new
> subsystem for PLDs/CPLDs/FPGAs would be more appropriate
> drivers/programmables or similar maybe.
> 
> > >> > Besides, this is MFD, where we support single pieces of silicon which
> > >> > happen to support multiple devices.  I definitely don't want to support
> > >> > boards here.
> > >> > You might want to re-think the naming and your (sales) pitch.
> > >> I might need help. As for the (sales), no comment.
> > >
> > > By pitch, I mean to convince me that this belongs in MFD.
> > I've been trying.
> 
> I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
> mean I'm going to leave you on the curb.  I'd like to help you get it
> into a better home.
> 
> [...]
> 
> > > Understanding was lost when I learned that the whole file was centred
> > > around the premise of board support.  I understand now that this is a
> > > driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
> > > really a CPLD driver, shouldn't be named after the manufacturer/model
> > > number of the chip, rather than the PCB which it's connected to?
> > >
> > > I'm also concerned that this driver has no functional CPLD code
> > > contained.  All you're doing is defining an IRQ domain.  Why then
> > > isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
> > > driver?
> > Is not only a irqchip because, as explained at the bottom of the commit message,
> > quoting myself :
> >   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
> 
> I had a conversation about this on IRC yesterday and some good
> points/questions were posed.  This is a difficult area, because you
> can program these things to do whatever you like.  Depending on the
> 'intention' (and it is only an intention -- someone else can come
> along and reprogram these devices on a whim), the CPLD code could live
> anywhere.  If you wanted to put watchdog functionality in there, then
> there is an argument for it to live in drivers/watchdog, etc etc.  So
> just because the plan is to support a few (i.e. more than one) simple
> devices, it doesn't necessarily mean that the handling should be done
> in MFD.
> 
> Yesterday I was asked "Are you wanting to restrict drivers in
> drivers/mfd to those that make use of MFD_CORE functionality?".  My
> answer to that was "No, however; I only want devices which
> _intrinsically_ operate in multiple subsystems", which these
> programmables no not do.
> 
> FYI, you're not on your own here.  There is at least one of these
> devices in the kernel already and upon a short inspection there
> appears to be a number of Out-of-Tree (OoT) drivers out there which
> will require a home in Mainline sooner or later.
> 
> -- 
> 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 linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Lee Jones]

On Tue, 17 Feb 2015, Nicolas Pitre wrote:
> On Tue, 17 Feb 2015, Lee Jones wrote:
> 
> > Arnd, Greg,
> > 
> >   Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?
> 
> FWIW...
> 
> The Lubbock is an ancient development board (circa 2003) using a CPLD to 
> multiplex a couple things on the board.  I really doubt anyone would 
> reprogram this CPLD at this point. So I'd treat it just like another 
> interrupt controller + random peripherals that will never change.  And 
> yes, maybe a more appropriate name is needed.

Understood.  However, I'm tempted to occupy some higher ground here
and say that there will always be (good?) excuses to shoehorn drivers
into subsystems which they don't truly belong.  Rather than make
exceptions I'd rather see an implementation which would also serve
subsequent attempts to upstream these types of devices.

> And I happen to have one such beast on my desk wasting significant 
> realestate and picking up dust.  I don't think I booted it even once in 
> the last 3 years.  I'm seriously considering a trip to the recycling 
> facility to dispose of it unless someone wants it and is ready to pay 
> for the shipping.

Shameless.  I think you were looking for Craig's List. ;)

> > On Mon, 16 Feb 2015, Robert Jarzmik wrote:
> > > Lee Jones wrote:
> > > > What's all this?  Please configure your mail client correctly.
> > > >
> > > > For advice, see:
> > > >
> > > >   Documentation/email-clients.txt
> > > While at day work, I have only access to web mail ...
> > 
> > Probably best to hold off your reply until you get home then.  It's
> > not just a formatting issue, you also exposed many raw email addresses
> > to the internet, which are easily harvested up by web crawlers.
> > 
> > See this:
> >   https://lkml.org/lkml/2015/2/16/247
> > 
> > > >>  2) after v2, we _both_ agreed that the accurate name is "cplds"
> > > >>     which exactly what is in this patch
> > > >>     (see device registering with lubbock_cplds).
> > > >
> > > > There is no mention of this decision in the changelogs.  If it's not
> > > > in the change logs, it didn't happen. ;)
> > > Ah right.
> > > 
> > > > I'm still concerned with the fact that the driver file is named using
> > > > and is populated by lots of instances of a "board" name.  I'm sure you
> > > > would share my thoughts is someone submitted a driver called
> > > > beaglebone.c or raspberrypi.c to MFD.
> > > I understand your concern. Arnd, a thought about it ? The only viable
> > > alternative would be to move it to arch/arm/plat-pxa AFAIS.
> > 
> > I don't think this is correct either.  CPLD handling would probably be
> > slightly less out of place in drivers/misc, but perhaps a new
> > subsystem for PLDs/CPLDs/FPGAs would be more appropriate
> > drivers/programmables or similar maybe.
> > 
> > > >> > Besides, this is MFD, where we support single pieces of silicon which
> > > >> > happen to support multiple devices.  I definitely don't want to support
> > > >> > boards here.
> > > >> > You might want to re-think the naming and your (sales) pitch.
> > > >> I might need help. As for the (sales), no comment.
> > > >
> > > > By pitch, I mean to convince me that this belongs in MFD.
> > > I've been trying.
> > 
> > I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
> > mean I'm going to leave you on the curb.  I'd like to help you get it
> > into a better home.
> > 
> > [...]
> > 
> > > > Understanding was lost when I learned that the whole file was centred
> > > > around the premise of board support.  I understand now that this is a
> > > > driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
> > > > really a CPLD driver, shouldn't be named after the manufacturer/model
> > > > number of the chip, rather than the PCB which it's connected to?
> > > >
> > > > I'm also concerned that this driver has no functional CPLD code
> > > > contained.  All you're doing is defining an IRQ domain.  Why then
> > > > isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
> > > > driver?
> > > Is not only a irqchip because, as explained at the bottom of the commit message,
> > > quoting myself :
> > >   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
> > 
> > I had a conversation about this on IRC yesterday and some good
> > points/questions were posed.  This is a difficult area, because you
> > can program these things to do whatever you like.  Depending on the
> > 'intention' (and it is only an intention -- someone else can come
> > along and reprogram these devices on a whim), the CPLD code could live
> > anywhere.  If you wanted to put watchdog functionality in there, then
> > there is an argument for it to live in drivers/watchdog, etc etc.  So
> > just because the plan is to support a few (i.e. more than one) simple
> > devices, it doesn't necessarily mean that the handling should be done
> > in MFD.
> > 
> > Yesterday I was asked "Are you wanting to restrict drivers in
> > drivers/mfd to those that make use of MFD_CORE functionality?".  My
> > answer to that was "No, however; I only want devices which
> > _intrinsically_ operate in multiple subsystems", which these
> > programmables no not do.
> > 
> > FYI, you're not on your own here.  There is at least one of these
> > devices in the kernel already and upon a short inspection there
> > appears to be a number of Out-of-Tree (OoT) drivers out there which
> > will require a home in Mainline sooner or later.
> > 


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

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

Lee Jones <lee.jones@linaro.org> writes:
>> > Arnd, Greg,
>> > 
>> >   Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?

Hi Arnd and Greg,

I have this driver I'm upstreaming, which comes out of
arch/arm/mach-pxa/lubbock.c. As for the reason it is extracted, see submitted
commit [1] for reference.

The main question is : where does it belong in the kernel ?

The driver is :
 - for the CPLDs on the Lubbock development platform, which is more or less an
   old motherboard for Intel Xscale pxa255 SoC (see [2] for more details)
 - these CPLDs control :
   - interrupt muxing towards the SoC
   - several leds
   - switches read back
   For the whole patch, see [4]

Lee's position is that it doesn't belong to drivers/mfd, see [3].

So where should I submit it ? And more generally, where should CPLDs drivers be
pushed in the kernel tree ?

If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
but it has to land somewhere, I don't want lubbock to remain broken.

Cheers.

--
Robert

[1] Reason of extraction / commit message
    mfd: lubbock_cplds: add lubbock IO board
    
    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>

[2] Board description by Nicolas
>> The Lubbock is an ancient development board (circa 2003) using a CPLD to 
>> multiplex a couple things on the board.  I really doubt anyone would 
>> reprogram this CPLD at this point. So I'd treat it just like another 
>> interrupt controller + random peripherals that will never change.  And 
>> yes, maybe a more appropriate name is needed.

[3] Lee's position
>> > I don't think this is correct either.  CPLD handling would probably be
>> > slightly less out of place in drivers/misc, but perhaps a new
>> > subsystem for PLDs/CPLDs/FPGAs would be more appropriate
>> > drivers/programmables or similar maybe.
>> >
...
>> > I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
>> > mean I'm going to leave you on the curb.  I'd like to help you get it
>> > into a better home.
>> > 
>> > [...]
>> > > Is not only a irqchip because, as explained at the bottom of the commit message,
>> > > quoting myself :
>> > >   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
>> > 
>> > I had a conversation about this on IRC yesterday and some good
>> > points/questions were posed.  This is a difficult area, because you
>> > can program these things to do whatever you like.  Depending on the
>> > 'intention' (and it is only an intention -- someone else can come
>> > along and reprogram these devices on a whim), the CPLD code could live
>> > anywhere.  If you wanted to put watchdog functionality in there, then
>> > there is an argument for it to live in drivers/watchdog, etc etc.  So
>> > just because the plan is to support a few (i.e. more than one) simple
>> > devices, it doesn't necessarily mean that the handling should be done
>> > in MFD.
>> > 
>> > Yesterday I was asked "Are you wanting to restrict drivers in
>> > drivers/mfd to those that make use of MFD_CORE functionality?".  My
>> > answer to that was "No, however; I only want devices which
>> > _intrinsically_ operate in multiple subsystems", which these
>> > programmables no not do.
>> > 
>> > FYI, you're not on your own here.  There is at least one of these
>> > devices in the kernel already and upon a short inspection there
>> > appears to be a number of Out-of-Tree (OoT) drivers out there which
>> > will require a home in Mainline sooner or later.
>> > 

[4] Whole patch
https://lkml.org/lkml/2015/1/24/90

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Hi Arnd and Greg,
It's been a week, backlog ping ?

>
> I have this driver I'm upstreaming, which comes out of
> arch/arm/mach-pxa/lubbock.c. As for the reason it is extracted, see submitted
> commit [1] for reference.
>
> The main question is : where does it belong in the kernel ?
>
> The driver is :
>  - for the CPLDs on the Lubbock development platform, which is more or less an
>    old motherboard for Intel Xscale pxa255 SoC (see [2] for more details)
>  - these CPLDs control :
>    - interrupt muxing towards the SoC
>    - several leds
>    - switches read back
>    For the whole patch, see [4]
>
> Lee's position is that it doesn't belong to drivers/mfd, see [3].
>
> So where should I submit it ? And more generally, where should CPLDs drivers be
> pushed in the kernel tree ?
>
> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
> but it has to land somewhere, I don't want lubbock to remain broken.
>
> Cheers.
>
> --
> Robert
>
> [1] Reason of extraction / commit message
>     mfd: lubbock_cplds: add lubbock IO board
>     
>     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>
>
> [2] Board description by Nicolas
>>> The Lubbock is an ancient development board (circa 2003) using a CPLD to 
>>> multiplex a couple things on the board.  I really doubt anyone would 
>>> reprogram this CPLD at this point. So I'd treat it just like another 
>>> interrupt controller + random peripherals that will never change.  And 
>>> yes, maybe a more appropriate name is needed.
>
> [3] Lee's position
>>> > I don't think this is correct either.  CPLD handling would probably be
>>> > slightly less out of place in drivers/misc, but perhaps a new
>>> > subsystem for PLDs/CPLDs/FPGAs would be more appropriate
>>> > drivers/programmables or similar maybe.
>>> >
> ...
>>> > I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
>>> > mean I'm going to leave you on the curb.  I'd like to help you get it
>>> > into a better home.
>>> > 
>>> > [...]
>>> > > Is not only a irqchip because, as explained at the bottom of the commit message,
>>> > > quoting myself :
>>> > >   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
>>> > 
>>> > I had a conversation about this on IRC yesterday and some good
>>> > points/questions were posed.  This is a difficult area, because you
>>> > can program these things to do whatever you like.  Depending on the
>>> > 'intention' (and it is only an intention -- someone else can come
>>> > along and reprogram these devices on a whim), the CPLD code could live
>>> > anywhere.  If you wanted to put watchdog functionality in there, then
>>> > there is an argument for it to live in drivers/watchdog, etc etc.  So
>>> > just because the plan is to support a few (i.e. more than one) simple
>>> > devices, it doesn't necessarily mean that the handling should be done
>>> > in MFD.
>>> > 
>>> > Yesterday I was asked "Are you wanting to restrict drivers in
>>> > drivers/mfd to those that make use of MFD_CORE functionality?".  My
>>> > answer to that was "No, however; I only want devices which
>>> > _intrinsically_ operate in multiple subsystems", which these
>>> > programmables no not do.
>>> > 
>>> > FYI, you're not on your own here.  There is at least one of these
>>> > devices in the kernel already and upon a short inspection there
>>> > appears to be a number of Out-of-Tree (OoT) drivers out there which
>>> > will require a home in Mainline sooner or later.
>>> > 
>
> [4] Whole patch
> https://lkml.org/lkml/2015/1/24/90

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Greg Kroah-Hartman]

On Sat, Feb 28, 2015 at 10:57:30AM +0100, Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Hi Arnd and Greg,
> It's been a week, backlog ping ?

If only my backlog was just one week...

And I'm not the mfd maintainer...

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Sat, Feb 28, 2015 at 10:57:30AM +0100, Robert Jarzmik wrote:
>> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>> 
>> > Hi Arnd and Greg,
>> It's been a week, backlog ping ?
>
> If only my backlog was just one week...
Ah, that's bad :)

> And I'm not the mfd maintainer...
I know. The question was for the "all drivers" maintainer ... ie. you.

The question was where should CPLD drivers end up, given that the mfd maintainer
pushed back.

Yet I understand you're overflowed, and I'll take it into arch/arm/plat-pxa. I
don't think it's the right place, but it's my only fallback.

Cheers.

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Greg Kroah-Hartman]

On Fri, Feb 20, 2015 at 05:02:57PM +0100, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> >> > Arnd, Greg,
> >> > 
> >> >   Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?
> 
> Hi Arnd and Greg,
> 
> I have this driver I'm upstreaming, which comes out of
> arch/arm/mach-pxa/lubbock.c. As for the reason it is extracted, see submitted
> commit [1] for reference.
> 
> The main question is : where does it belong in the kernel ?
> 
> The driver is :
>  - for the CPLDs on the Lubbock development platform, which is more or less an
>    old motherboard for Intel Xscale pxa255 SoC (see [2] for more details)
>  - these CPLDs control :
>    - interrupt muxing towards the SoC
>    - several leds
>    - switches read back
>    For the whole patch, see [4]
> 
> Lee's position is that it doesn't belong to drivers/mfd, see [3].
> 
> So where should I submit it ? And more generally, where should CPLDs drivers be
> pushed in the kernel tree ?
> 
> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
> but it has to land somewhere, I don't want lubbock to remain broken.

drivers/platform/arm ?

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Fri, Feb 20, 2015 at 05:02:57PM +0100, Robert Jarzmik wrote:
>> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
>> but it has to land somewhere, I don't want lubbock to remain broken.
>
> drivers/platform/arm ?
Most certainly.

I'll submit that to drivers/platform/arm/pxa, and maintain that pxa tree. As for
drivers/platform/arm, do you want also maintainers to step up, or will you take
the review/merge burden ?

Cheers.

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Greg Kroah-Hartman]

On Thu, Mar 26, 2015 at 10:38:54PM +0100, Robert Jarzmik wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Feb 20, 2015 at 05:02:57PM +0100, Robert Jarzmik wrote:
> >> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
> >> but it has to land somewhere, I don't want lubbock to remain broken.
> >
> > drivers/platform/arm ?
> Most certainly.
> 
> I'll submit that to drivers/platform/arm/pxa, and maintain that pxa tree. As for
> drivers/platform/arm, do you want also maintainers to step up, or will you take
> the review/merge burden ?

I have enough review/merge burden to do anything with ARM, sorry.

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Arnd Bergmann]

On Thursday 26 March 2015, Robert Jarzmik wrote:
> 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Feb 20, 2015 at 05:02:57PM +0100, Robert Jarzmik wrote:
> >> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
> >> but it has to land somewhere, I don't want lubbock to remain broken.
> >
> > drivers/platform/arm ?
> Most certainly.
> 
> I'll submit that to drivers/platform/arm/pxa, and maintain that pxa tree. As for
> drivers/platform/arm, do you want also maintainers to step up, or will you take
> the review/merge burden ?
> 

I'd much prefer not to add drivers/platform/arm, which would make it too easy
to add random stuff there. What is the problem with leaving it in mach-pxa?

	Arnd

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> On Thursday 26 March 2015, Robert Jarzmik wrote:
>> 
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Fri, Feb 20, 2015 at 05:02:57PM +0100, Robert Jarzmik wrote:
>> >> If there is no solution, I'll fallback through arch/arm/plat-pxa, not very nice,
>> >> but it has to land somewhere, I don't want lubbock to remain broken.
>> >
>> > drivers/platform/arm ?
>> Most certainly.
>> 
>> I'll submit that to drivers/platform/arm/pxa, and maintain that pxa tree. As for
>> drivers/platform/arm, do you want also maintainers to step up, or will you take
>> the review/merge burden ?
>> 
>
> I'd much prefer not to add drivers/platform/arm, which would make it too easy
> to add random stuff there. What is the problem with leaving it in mach-pxa?
Hi Arnd,

It's not as much a problem as a generic question : does a driver belong to
arch/* ?

Personaly it would have been far simpler for me to have it through the pxa tree,
but I want to be sure it's the right place. Others will follow, pxa mainstone is
such a candidate.

I was thinking so far that arch/arm/mach-* was for machine description,
ie. wirings, interconnections, initial setup etc ... The "driver" part, ie. code
really driving dynamics in IPs was as per my understanding in drivers/...

Now I can create arch/arm/mach-pxa/lubbock_cplds.c, that won't make any
difference to me, provided that it's the right thing to do.

Cheers.

-- 
Robert

[PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Arnd Bergmann]

On Saturday 28 March 2015, Robert Jarzmik wrote:
> It's not as much a problem as a generic question : does a driver belong to
> arch/* ?
> 
> Personaly it would have been far simpler for me to have it through the pxa tree,
> but I want to be sure it's the right place. Others will follow, pxa mainstone is
> such a candidate.
> 
> I was thinking so far that arch/arm/mach-* was for machine description,
> ie. wirings, interconnections, initial setup etc ... The "driver" part, ie. code
> really driving dynamics in IPs was as per my understanding in drivers/...
> 
> Now I can create arch/arm/mach-pxa/lubbock_cplds.c, that won't make any
> difference to me, provided that it's the right thing to do.

If we had a lot of these, we would probably put them somewhere under drivers.
and find a maintainer for them. Given that this is an exceptional case for
an older machine, my feeling is that leaving the code in mach-pxa is the least
effort for now.

	Arnd

[PATCH v4 3/4] ARM: pxa: lubbock: use new lubbock_cplds driver

[Robert Jarzmik]

As the interrupt handling was transferred to the lubbock_cplds 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.
Since v3: rename lubbock_io into lubbock_cplds
---
 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..a0c91d8 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_cplds_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_cplds_device = {
+	.name		= "lubbock_cplds",
+	.id		= -1,
+	.resource	= &lubbock_cplds_resources[0],
+	.num_resources	= 3,
+};
+
+
 static struct platform_device *devices[] __initdata = {
 	&sa1111_device,
 	&smc91x_device,
 	&lubbock_flash_device[0],
 	&lubbock_flash_device[1],
+	&lubbock_cplds_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 v4 4/4] MAINTAINERS: add entry for lubbock-cplds

[Robert Jarzmik]

Add entry Lubbock cplds driver into the pxa platform scope.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ddb9ac8..8c6a49d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7681,6 +7681,7 @@ T:	git git://github.com/hzhuang1/linux.git
 T:	git git://github.com/rjarzmik/linux.git
 S:	Maintained
 F:	arch/arm/mach-pxa/
+F:	drivers/mfd/lubbock.c
 F:	drivers/pcmcia/pxa2xx*
 F:	drivers/spi/spi-pxa2xx*
 F:	drivers/usb/gadget/udc/pxa2*
-- 
2.1.0

[PATCH v4 1/4] dt-bindings: mfd: add lubbock-cplds binding

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Add a binding for lubbock motherboard IO board.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> Since v3: name change to lubbock-cplds,
>           Lee's comments taken into account.

Hi Lee,

I hope I have handled all the comments. Is this v4 good for you for mfd tree
staging ?

Cheers.

--
Robert