[PATCH 0/6] Add support for pinctrl/gpio on Armada 37xx

[PATCH 0/6] Add support for pinctrl/gpio on Armada 37xx

[Gregory CLEMENT]

Hi,

this series add support for the pin and gpio controllers present on
the Armada 37xx SoCs.

Each Armada 37xx SoC comes with 2 pin controllers: one on the south
bridge (managing 28 pins) and one on the north bridge (managing 36 pins).

At the hardware level the controller configure the pins by group and not
pin by pin.

The gpio controller is also capable to handle interrupt from gpio.

Gregory

Gregory CLEMENT (6):
  pinctrl: dt-bindings: Add documentation for Armada 37xx pin
    controllers
  pinctrl: armada-37xx: Add pin controller support for Armada 37xx
  pinctrl: armada-37xx: Add gpio support
  pinctrl: aramda-37xx: Add irqchip support
  ARM64: dts: marvell: Add pinctrl nodes for Armada 3700
  ARM64: dts: marvell: armada37xx: add pinctrl definition

 .../pinctrl/marvell,armada-37xx-pinctrl.txt        | 127 +++
 arch/arm64/Kconfig.platforms                       |   2 +
 arch/arm64/boot/dts/marvell/armada-3720-db.dts     |   8 +
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi       |  60 +-
 drivers/pinctrl/Makefile                           |   2 +-
 drivers/pinctrl/mvebu/Kconfig                      |   7 +
 drivers/pinctrl/mvebu/Makefile                     |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c        | 848 +++++++++++++++++++++
 8 files changed, 1052 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

-- 
2.11.0

[PATCH 0/6] Add support for pinctrl/gpio on Armada 37xx

[Gregory CLEMENT]

Hi,

this series add support for the pin and gpio controllers present on
the Armada 37xx SoCs.

Each Armada 37xx SoC comes with 2 pin controllers: one on the south
bridge (managing 28 pins) and one on the north bridge (managing 36 pins).

At the hardware level the controller configure the pins by group and not
pin by pin.

The gpio controller is also capable to handle interrupt from gpio.

Gregory

Gregory CLEMENT (6):
  pinctrl: dt-bindings: Add documentation for Armada 37xx pin
    controllers
  pinctrl: armada-37xx: Add pin controller support for Armada 37xx
  pinctrl: armada-37xx: Add gpio support
  pinctrl: aramda-37xx: Add irqchip support
  ARM64: dts: marvell: Add pinctrl nodes for Armada 3700
  ARM64: dts: marvell: armada37xx: add pinctrl definition

 .../pinctrl/marvell,armada-37xx-pinctrl.txt        | 127 +++
 arch/arm64/Kconfig.platforms                       |   2 +
 arch/arm64/boot/dts/marvell/armada-3720-db.dts     |   8 +
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi       |  60 +-
 drivers/pinctrl/Makefile                           |   2 +-
 drivers/pinctrl/mvebu/Kconfig                      |   7 +
 drivers/pinctrl/mvebu/Makefile                     |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c        | 848 +++++++++++++++++++++
 8 files changed, 1052 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

-- 
2.11.0

[PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Gregory CLEMENT]

Document the device tree binding for the pin controllers found on the
Armada 37xx SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../pinctrl/marvell,armada-37xx-pinctrl.txt        | 127 +++++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
new file mode 100644
index 000000000000..53a30a09d3fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
@@ -0,0 +1,127 @@
+* Marvell Armada 37xx SoC pinctrl
+
+Refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning
+of the phrase "pin configuration node".
+
+Each Armada 37xx SoC come with two pin controller one for the south
+bridge and the other for the north bridge.
+
+Required properties for pinctrl driver:
+- compatible:	"marvell,armada3710-sb-pinctrl" for the south bridge
+		"marvell,armada3710-nb-pinctrl" for the north bridge
+- reg: The first set of register are for pinctrl/gpio and the second
+  set for the interrupt controller
+- interrupts: list of the interrupt use by the gpio
+
+Available groups and functions for the North bridge:
+
+group: jtag
+ - pins 20-24
+ - functions jtag, gpio
+
+group sdio0
+ - pins 8-10
+ - functions sdio, gpio
+
+group emmc_nb
+ - pins 27-35
+ - functions emmc, gpio
+
+group pwm0
+ - pin 11 (GPIO1-11)
+ - functions pwm, gpio
+
+group pwm1
+ - pin 12
+ - functions pwm, gpio
+
+group pwm2
+ - pin 13
+ - functions pwm, gpio
+
+group pwm3
+ - pin 14
+ - functions pwm, gpio
+
+group pmic1
+ - pin 17
+ - functions pmic, gpio
+
+group pmic0
+ - pin 16
+ - functions pmic, gpio
+
+group i2c2
+ - pins 2-3
+ - functions i2c, gpio
+
+group i2c1
+ - pins 0-1
+ - functions i2c, gpio
+
+group spi_cs1
+ - pin 17
+ - functions spi, gpio
+
+group spi_cs2
+ - pin 18
+ - functions spi, gpio
+
+group spi_cs3
+ - pin 19
+ - functions spi, gpio
+
+group onewire
+ - pin 4
+ - functions onewire, gpio
+
+group uart1
+ - pins 25-26
+ - functions uart, gpio
+
+group spi_quad
+ - pins 15-16
+ - functions spi, gpio
+
+group uart_2
+ - pins 9-10
+ - functions uart, gpio
+
+Available groups and functions for the South bridge:
+
+group usb32_drvvbus0
+ - pin 36
+ - functions drvbus, gpio
+
+group usb2_drvvbus1
+ - pin 37
+ - functions drvbus, gpio
+
+group sdio_sb
+ - pins 60-64
+ - functions sdio, gpio
+
+group rgmii
+ - pins 42-55
+ - functions mii, gpio
+
+group pcie1
+ - pins 39-40
+ - functions pcie, gpio
+
+group ptp
+ - pins 56-58
+ - functions ptp, gpio
+
+group ptp_clk
+ - pin 57
+ - functions ptp, mii
+
+group ptp_trig
+ - pin 58
+ - functions ptp, mii
+
+group mii_col
+ - pin 59
+ - functions mii, mii_err
-- 
2.11.0

[PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Gregory CLEMENT]

Document the device tree binding for the pin controllers found on the
Armada 37xx SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../pinctrl/marvell,armada-37xx-pinctrl.txt        | 127 +++++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
new file mode 100644
index 000000000000..53a30a09d3fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
@@ -0,0 +1,127 @@
+* Marvell Armada 37xx SoC pinctrl
+
+Refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning
+of the phrase "pin configuration node".
+
+Each Armada 37xx SoC come with two pin controller one for the south
+bridge and the other for the north bridge.
+
+Required properties for pinctrl driver:
+- compatible:	"marvell,armada3710-sb-pinctrl" for the south bridge
+		"marvell,armada3710-nb-pinctrl" for the north bridge
+- reg: The first set of register are for pinctrl/gpio and the second
+  set for the interrupt controller
+- interrupts: list of the interrupt use by the gpio
+
+Available groups and functions for the North bridge:
+
+group: jtag
+ - pins 20-24
+ - functions jtag, gpio
+
+group sdio0
+ - pins 8-10
+ - functions sdio, gpio
+
+group emmc_nb
+ - pins 27-35
+ - functions emmc, gpio
+
+group pwm0
+ - pin 11 (GPIO1-11)
+ - functions pwm, gpio
+
+group pwm1
+ - pin 12
+ - functions pwm, gpio
+
+group pwm2
+ - pin 13
+ - functions pwm, gpio
+
+group pwm3
+ - pin 14
+ - functions pwm, gpio
+
+group pmic1
+ - pin 17
+ - functions pmic, gpio
+
+group pmic0
+ - pin 16
+ - functions pmic, gpio
+
+group i2c2
+ - pins 2-3
+ - functions i2c, gpio
+
+group i2c1
+ - pins 0-1
+ - functions i2c, gpio
+
+group spi_cs1
+ - pin 17
+ - functions spi, gpio
+
+group spi_cs2
+ - pin 18
+ - functions spi, gpio
+
+group spi_cs3
+ - pin 19
+ - functions spi, gpio
+
+group onewire
+ - pin 4
+ - functions onewire, gpio
+
+group uart1
+ - pins 25-26
+ - functions uart, gpio
+
+group spi_quad
+ - pins 15-16
+ - functions spi, gpio
+
+group uart_2
+ - pins 9-10
+ - functions uart, gpio
+
+Available groups and functions for the South bridge:
+
+group usb32_drvvbus0
+ - pin 36
+ - functions drvbus, gpio
+
+group usb2_drvvbus1
+ - pin 37
+ - functions drvbus, gpio
+
+group sdio_sb
+ - pins 60-64
+ - functions sdio, gpio
+
+group rgmii
+ - pins 42-55
+ - functions mii, gpio
+
+group pcie1
+ - pins 39-40
+ - functions pcie, gpio
+
+group ptp
+ - pins 56-58
+ - functions ptp, gpio
+
+group ptp_clk
+ - pin 57
+ - functions ptp, mii
+
+group ptp_trig
+ - pin 58
+ - functions ptp, mii
+
+group mii_col
+ - pin 59
+ - functions mii, mii_err
-- 
2.11.0

Re: [PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> Document the device tree binding for the pin controllers found on the
> Armada 37xx SoCs.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
(...)

> +Required properties for pinctrl driver:
> +- compatible:  "marvell,armada3710-sb-pinctrl" for the south bridge
> +               "marvell,armada3710-nb-pinctrl" for the north bridge
> +- reg: The first set of register are for pinctrl/gpio and the second
> +  set for the interrupt controller
> +- interrupts: list of the interrupt use by the gpio

While this makes sense on its own, it doesn't match the code you sent.

The code uses syscon and regmap inside a simple-mfd node, not reg.

Please clarify how this works so we can see what is going on.

The syscon part doesn't seem optional at all.

Yours,
Linus Walleij

[PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> Document the device tree binding for the pin controllers found on the
> Armada 37xx SoCs.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
(...)

> +Required properties for pinctrl driver:
> +- compatible:  "marvell,armada3710-sb-pinctrl" for the south bridge
> +               "marvell,armada3710-nb-pinctrl" for the north bridge
> +- reg: The first set of register are for pinctrl/gpio and the second
> +  set for the interrupt controller
> +- interrupts: list of the interrupt use by the gpio

While this makes sense on its own, it doesn't match the code you sent.

The code uses syscon and regmap inside a simple-mfd node, not reg.

Please clarify how this works so we can see what is going on.

The syscon part doesn't seem optional at all.

Yours,
Linus Walleij

Re: [PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Gregory CLEMENT]

Hi Linus,
 
 On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> Document the device tree binding for the pin controllers found on the
>> Armada 37xx SoCs.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> (...)
>
>> +Required properties for pinctrl driver:
>> +- compatible:  "marvell,armada3710-sb-pinctrl" for the south bridge
>> +               "marvell,armada3710-nb-pinctrl" for the north bridge
>> +- reg: The first set of register are for pinctrl/gpio and the second
>> +  set for the interrupt controller
>> +- interrupts: list of the interrupt use by the gpio
>
> While this makes sense on its own, it doesn't match the code you sent.
>
> The code uses syscon and regmap inside a simple-mfd node, not reg.
>
> Please clarify how this works so we can see what is going on.
>
> The syscon part doesn't seem optional at all.

OK done.

>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers

[Gregory CLEMENT]

Hi Linus,
 
 On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> Document the device tree binding for the pin controllers found on the
>> Armada 37xx SoCs.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> (...)
>
>> +Required properties for pinctrl driver:
>> +- compatible:  "marvell,armada3710-sb-pinctrl" for the south bridge
>> +               "marvell,armada3710-nb-pinctrl" for the north bridge
>> +- reg: The first set of register are for pinctrl/gpio and the second
>> +  set for the interrupt controller
>> +- interrupts: list of the interrupt use by the gpio
>
> While this makes sense on its own, it doesn't match the code you sent.
>
> The code uses syscon and regmap inside a simple-mfd node, not reg.
>
> Please clarify how this works so we can see what is going on.
>
> The syscon part doesn't seem optional at all.

OK done.

>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Gregory CLEMENT]

The Armada 37xx SoC come with 2 pin controllers: one on the south
bridge (managing 28 pins) and one on the north bridge (managing 36 pins).

At the hardware level the controller configure the pins by group and not
pin by pin. This constraint is reflected in the design of the driver:
only the group related functions are implemented.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/Kconfig.platforms                |   2 +
 drivers/pinctrl/Makefile                    |   2 +-
 drivers/pinctrl/mvebu/Kconfig               |   7 +
 drivers/pinctrl/mvebu/Makefile              |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 590 ++++++++++++++++++++++++++++
 5 files changed, 602 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 715ef1256838..0786e3e0f5c6 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -105,6 +105,8 @@ config ARCH_MVEBU
 	select ARMADA_37XX_CLK
 	select MVEBU_ODMI
 	select MVEBU_PIC
+	select PINCTRL
+	select PINCTRL_ARMADA_37XX
 	help
 	  This enables support for Marvell EBU familly, including:
 	   - Armada 3700 SoC Family
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 25d50a86981d..b89659b5bfa9 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -44,7 +44,7 @@ obj-y				+= bcm/
 obj-$(CONFIG_PINCTRL_BERLIN)	+= berlin/
 obj-y				+= freescale/
 obj-$(CONFIG_X86)		+= intel/
-obj-$(CONFIG_PINCTRL_MVEBU)	+= mvebu/
+obj-y				+= mvebu/
 obj-y				+= nomadik/
 obj-$(CONFIG_PINCTRL_PXA)	+= pxa/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
index 170602407c0d..98fcf1290acd 100644
--- a/drivers/pinctrl/mvebu/Kconfig
+++ b/drivers/pinctrl/mvebu/Kconfig
@@ -39,3 +39,10 @@ config PINCTRL_ORION
 	select PINCTRL_MVEBU
 
 endif
+
+config PINCTRL_ARMADA_37XX
+       bool
+       select PINMUX
+       select PINCONF
+       select GENERIC_PINCONF
+
diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
index 18270cd5ea43..60c245a60f39 100644
--- a/drivers/pinctrl/mvebu/Makefile
+++ b/drivers/pinctrl/mvebu/Makefile
@@ -1,4 +1,4 @@
-obj-y				+= pinctrl-mvebu.o
+obj-$(CONFIG_PINCTRL_MVEBU)	+= pinctrl-mvebu.o
 obj-$(CONFIG_PINCTRL_DOVE)	+= pinctrl-dove.o
 obj-$(CONFIG_PINCTRL_KIRKWOOD)	+= pinctrl-kirkwood.o
 obj-$(CONFIG_PINCTRL_ARMADA_370) += pinctrl-armada-370.o
@@ -6,4 +6,5 @@ obj-$(CONFIG_PINCTRL_ARMADA_375) += pinctrl-armada-375.o
 obj-$(CONFIG_PINCTRL_ARMADA_38X) += pinctrl-armada-38x.o
 obj-$(CONFIG_PINCTRL_ARMADA_39X) += pinctrl-armada-39x.o
 obj-$(CONFIG_PINCTRL_ARMADA_XP)  += pinctrl-armada-xp.o
+obj-$(CONFIG_PINCTRL_ARMADA_37XX)  += pinctrl-armada-37xx.o
 obj-$(CONFIG_PINCTRL_ORION)  += pinctrl-orion.o
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
new file mode 100644
index 000000000000..021bfe793af3
--- /dev/null
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -0,0 +1,590 @@
+/*
+ * Marvell 37xx SoC pinctrl driver
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "../pinctrl-utils.h"
+
+#define OUTPUT_EN	0x0
+#define OUTPUT_CTL	0x20
+#define SELECTION	0x30
+
+static int global_pin;
+
+#define NB_FUNCS 2
+#define GPIO_PER_REG	32
+
+struct armada_37xx_pin_group {
+	const char	*name;
+	unsigned int	start_pin;
+	unsigned int	npins;
+	u32		reg_mask;
+	unsigned int	extra_pin;
+	unsigned int	extra_npins;
+	const char	*funcs[NB_FUNCS];
+	unsigned int	*pins;
+};
+
+struct armada_37xx_pin_data {
+	u8				nr_pins;
+	char				*name;
+	struct armada_37xx_pin_group	*groups;
+	int				ngroups;
+};
+
+struct armada_37xx_pmx_func {
+	const char		*name;
+	const char		**groups;
+	unsigned int		ngroups;
+};
+
+struct armada_37xx_pinctrl {
+	struct regmap			*regmap;
+	struct armada_37xx_pin_data	*data;
+	struct device			*dev;
+	struct pinctrl_desc		pctl;
+	struct pinctrl_dev		*pctl_dev;
+	struct armada_37xx_pin_group	*groups;
+	unsigned int			ngroups;
+	struct armada_37xx_pmx_func	*funcs;
+	unsigned int			nfuncs;
+};
+
+#define PIN_GRP(_name, _start, _nr, _mask, _func1, _func2)	\
+	{					\
+		.name = _name,			\
+		.start_pin = _start,		\
+		.npins = _nr,			\
+		.reg_mask = _mask,		\
+		.funcs = {_func1, _func2}	\
+	}
+
+#define PIN_GRP_GPIO(_name, _start, _nr, _mask, _func1)	\
+	{					\
+		.name = _name,			\
+		.start_pin = _start,		\
+		.npins = _nr,			\
+		.reg_mask = _mask,		\
+		.funcs = {_func1, "gpio"}	\
+	}
+
+#define PIN_GRP_EXTRA(_name, _start, _nr, _mask, _start2, _nr2, _f1, _f2) \
+	{						\
+		.name = _name,				\
+		.start_pin = _start,			\
+		.npins = _nr,				\
+		.reg_mask = _mask,			\
+		.extra_pin = _start2,			\
+		.extra_npins = _nr2,			\
+		.funcs = {_f1, _f2}			\
+	}
+
+static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
+	PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
+	PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
+	PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
+	PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
+	PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
+	PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
+	PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
+	PIN_GRP_GPIO("pmic1", 17, 1, BIT(7), "pmic"),
+	PIN_GRP_GPIO("pmic0", 16, 1, BIT(8), "pmic"),
+	PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
+	PIN_GRP_GPIO("i2c1", 0, 2, BIT(10), "i2c"),
+	PIN_GRP_GPIO("spi_cs1", 17, 1, BIT(12), "spi"),
+	PIN_GRP_GPIO("spi_cs2", 18, 1, BIT(13), "spi"),
+	PIN_GRP_GPIO("spi_cs3", 19, 1, BIT(14), "spi"),
+	PIN_GRP_GPIO("onewire", 4, 1, BIT(16), "onewire"),
+	PIN_GRP_GPIO("uart1", 25, 2, BIT(17), "uart"),
+	PIN_GRP_GPIO("spi_quad", 15, 2, BIT(18), "spi"),
+	PIN_GRP_EXTRA("uart_2", 9, 2, BIT(19), 18, 2, "gpio", "uart"),
+};
+
+static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
+	PIN_GRP_GPIO("usb32_drvvbus0", 0, 1, BIT(0), "drvbus"),
+	PIN_GRP_GPIO("usb2_drvvbus1", 1, 1, BIT(1), "drvbus"),
+	PIN_GRP_GPIO("sdio_sb", 24, 5, BIT(2), "sdio"),
+	PIN_GRP_EXTRA("rgmii", 6, 14, BIT(3), 23, 1, "mii", "gpio"),
+	PIN_GRP_GPIO("pcie1", 3, 2, BIT(4), "pcie"),
+	PIN_GRP_GPIO("ptp", 20, 3, BIT(5), "ptp"),
+	PIN_GRP("ptp_clk", 21, 1, BIT(6), "ptp", "mii"),
+	PIN_GRP("ptp_trig", 22, 1, BIT(7), "ptp", "mii"),
+	PIN_GRP("mii_col", 23, 1, BIT(8), "mii", "mii_err"),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_nb = {
+	.nr_pins = 36,
+	.name = "GPIO1",
+	.groups = armada_37xx_nb_groups,
+	.ngroups = ARRAY_SIZE(armada_37xx_nb_groups),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_sb = {
+	.nr_pins = 29,
+	.name = "GPIO2",
+	.groups = armada_37xx_sb_groups,
+	.ngroups = ARRAY_SIZE(armada_37xx_sb_groups),
+};
+
+static int armada_37xx_get_func_reg(struct armada_37xx_pin_group *grp,
+				    const char *func)
+{
+	int f;
+
+	for (f = 0; f < NB_FUNCS; f++)
+		if (!strcmp(grp->funcs[f], func))
+			return f;
+
+	return -ENOTSUPP;
+}
+
+static struct armada_37xx_pin_group *armada_37xx_find_next_grp_by_pin(
+	struct armada_37xx_pinctrl *info, int pin, int *grp)
+{
+	while (*grp < info->ngroups) {
+		struct armada_37xx_pin_group *group = &info->groups[*grp];
+		int j;
+
+		*grp = *grp + 1;
+		for (j = 0; j < (group->npins + group->extra_npins); j++)
+			if (group->pins[j] == pin)
+				return group;
+	}
+	return NULL;
+}
+
+static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
+			    unsigned int selector, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
+			    unsigned int selector, unsigned long *configs,
+			    unsigned int num_configs)
+{
+	return -ENOTSUPP;
+}
+
+static struct pinconf_ops armada_37xx_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_group_get = armada_37xx_pin_config_group_get,
+	.pin_config_group_set = armada_37xx_pin_config_group_set,
+};
+
+static int armada_37xx_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->ngroups;
+}
+
+static const char *armada_37xx_get_group_name(struct pinctrl_dev *pctldev,
+					      unsigned int group)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->groups[group].name;
+}
+
+static int armada_37xx_get_group_pins(struct pinctrl_dev *pctldev,
+				      unsigned int selector,
+				      const unsigned int **pins,
+				      unsigned int *npins)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector >= info->ngroups)
+		return -EINVAL;
+
+	*pins = info->groups[selector].pins;
+	*npins = info->groups[selector].npins;
+
+	return 0;
+}
+
+static const struct pinctrl_ops armada_37xx_pctrl_ops = {
+	.get_groups_count	= armada_37xx_get_groups_count,
+	.get_group_name		= armada_37xx_get_group_name,
+	.get_group_pins		= armada_37xx_get_group_pins,
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
+	.dt_free_map		= pinctrl_utils_free_map,
+};
+
+/*
+ * Pinmux_ops handling
+ */
+
+static int armada_37xx_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->nfuncs;
+}
+
+static const char *armada_37xx_pmx_get_func_name(struct pinctrl_dev *pctldev,
+						 unsigned int selector)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->funcs[selector].name;
+}
+
+static int armada_37xx_pmx_get_groups(struct pinctrl_dev *pctldev,
+				      unsigned int selector,
+				      const char * const **groups,
+				      unsigned int * const num_groups)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = info->funcs[selector].groups;
+	*num_groups = info->funcs[selector].ngroups;
+
+	return 0;
+}
+
+static int armada_37xx_pmx_set_by_name(struct pinctrl_dev *pctldev,
+				       const char *name,
+				       struct armada_37xx_pin_group *grp)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int reg = SELECTION;
+	unsigned int mask = grp->reg_mask;
+	int ret, val;
+
+	dev_dbg(info->dev, "enable function %s group %s\n",
+		name, grp->name);
+
+	ret = armada_37xx_get_func_reg(grp, name);
+
+	if (ret < 0)
+		return ret;
+
+	val = ret ? mask : 0;
+
+	regmap_update_bits(info->regmap, reg, mask, val);
+
+	return 0;
+}
+
+static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       unsigned int group)
+{
+
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct armada_37xx_pin_group *grp = &info->groups[group];
+	const char *name = info->funcs[selector].name;
+
+	return armada_37xx_pmx_set_by_name(pctldev, name, grp);
+}
+
+static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
+					   unsigned int offset)
+{
+	unsigned int reg = OUTPUT_EN;
+	unsigned int mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	return regmap_update_bits(info->regmap, reg, mask, 0);
+}
+
+
+
+static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
+					    unsigned int offset, int value)
+{
+	unsigned int reg = OUTPUT_EN;
+	unsigned int mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	return regmap_update_bits(info->regmap, reg, mask, mask);
+}
+
+static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range,
+					      unsigned int offset, bool input)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
+		offset, range->name, offset, input ? "input" : "output");
+
+	if (input)
+		armada_37xx_pmx_direction_input(info, offset);
+	else
+		armada_37xx_pmx_direction_output(info, offset, 0);
+
+	return 0;
+}
+
+static int armada_37xx_gpio_request_enable(struct pinctrl_dev *pctldev,
+					   struct pinctrl_gpio_range *range,
+					   unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct armada_37xx_pin_group *group;
+	int grp = 0;
+
+	dev_dbg(info->dev, "requesting gpio %d\n", offset);
+
+	while ((group = armada_37xx_find_next_grp_by_pin(info, offset, &grp)))
+		armada_37xx_pmx_set_by_name(pctldev, "gpio", group);
+
+	return 0;
+}
+
+static const struct pinmux_ops armada_37xx_pmx_ops = {
+	.get_functions_count	= armada_37xx_pmx_get_funcs_count,
+	.get_function_name	= armada_37xx_pmx_get_func_name,
+	.get_function_groups	= armada_37xx_pmx_get_groups,
+	.set_mux		= armada_37xx_pmx_set,
+	.gpio_request_enable	= armada_37xx_gpio_request_enable,
+	.gpio_set_direction	= armada_37xx_pmx_gpio_set_direction,
+};
+
+static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
+			 const char *name)
+{
+	int i = 0;
+
+	if (*funcsize <= 0)
+		return -EOVERFLOW;
+
+	while (funcs->ngroups) {
+		/* function already there */
+		if (strcmp(funcs->name, name) == 0) {
+			funcs->ngroups++;
+
+			return -EEXIST;
+		}
+		funcs++;
+		i++;
+	}
+
+	/* append new unique function */
+	funcs->name = name;
+	funcs->ngroups = 1;
+	(*funcsize)--;
+
+	return 0;
+}
+
+static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
+{
+	int n, num = 0, funcsize = info->data->nr_pins;
+
+	for (n = 0; n < info->ngroups; n++) {
+		struct armada_37xx_pin_group *grp = &info->groups[n];
+		int i, j, f;
+
+		grp->pins = devm_kzalloc(info->dev,
+					 (grp->npins + grp->extra_npins) *
+					 sizeof(*grp->pins), GFP_KERNEL);
+		if (!grp->pins)
+			return -ENOMEM;
+
+		for (i = 0; i < grp->npins; i++)
+			grp->pins[i] = grp->start_pin + base + i;
+
+		for (j = 0; j < grp->extra_npins; j++)
+			grp->pins[i+j] = grp->extra_pin + base + j;
+
+		for (f = 0; f < NB_FUNCS; f++) {
+			int ret;
+			/* check for unique functions and count groups */
+			ret = _add_function(info->funcs, &funcsize,
+					    grp->funcs[f]);
+			if (ret == -EOVERFLOW)
+				dev_err(info->dev,
+					"More functions than pins(%d)\n",
+					info->data->nr_pins);
+			if (ret < 0)
+				continue;
+			num++;
+		}
+	}
+
+	info->nfuncs = num;
+
+	return 0;
+}
+
+static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
+{
+	struct armada_37xx_pmx_func *funcs = info->funcs;
+	int n;
+
+	for (n = 0; n < info->nfuncs; n++) {
+		const char *name = funcs[n].name;
+		const char **groups;
+		int g;
+
+		funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
+					       sizeof(*(funcs[n].groups)),
+					       GFP_KERNEL);
+		if (!funcs[n].groups)
+			return -ENOMEM;
+
+		groups = funcs[n].groups;
+
+		for (g = 0; g < info->ngroups; g++) {
+			struct armada_37xx_pin_group *gp = &info->groups[g];
+			int f;
+
+			for (f = 0; f < NB_FUNCS; f++) {
+				if (strcmp(gp->funcs[f], name) == 0) {
+					*groups = gp->name;
+					groups++;
+				}
+			}
+		}
+	}
+	return 0;
+}
+
+static int armada_37xx_pinctrl_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct armada_37xx_pin_data *pin_data = info->data;
+	struct pinctrl_desc *ctrldesc = &info->pctl;
+	struct pinctrl_pin_desc *pindesc, *pdesc;
+	int base_pin = global_pin;
+	int pin, ret;
+
+	info->groups = pin_data->groups;
+	info->ngroups = pin_data->ngroups;
+
+	ctrldesc->name = "armada_37xx-pinctrl";
+	ctrldesc->owner = THIS_MODULE;
+	ctrldesc->pctlops = &armada_37xx_pctrl_ops;
+	ctrldesc->pmxops = &armada_37xx_pmx_ops;
+	ctrldesc->confops = &armada_37xx_pinconf_ops;
+
+	pindesc = devm_kzalloc(&pdev->dev, sizeof(*pindesc) *
+			       pin_data->nr_pins, GFP_KERNEL);
+	if (!pindesc)
+		return -ENOMEM;
+
+	ctrldesc->pins = pindesc;
+	ctrldesc->npins = pin_data->nr_pins;
+
+	pdesc = pindesc;
+	for (pin = 0; pin < pin_data->nr_pins; pin++, global_pin++) {
+		pdesc->number = global_pin;
+		pdesc->name = kasprintf(GFP_KERNEL, "%s-%d",
+					pin_data->name, pin);
+		pdesc++;
+	}
+
+	/*
+	 * we allocate functions for number of pins and hope there are
+	 * fewer unique functions than pins available
+	 */
+	info->funcs = devm_kzalloc(&pdev->dev, pin_data->nr_pins *
+			   sizeof(struct armada_37xx_pmx_func), GFP_KERNEL);
+	if (!info->funcs)
+		return -ENOMEM;
+
+
+	ret = armada_37xx_fill_group(info, base_pin);
+	if (ret)
+		return ret;
+
+	ret = armada_37xx_fill_func(info);
+	if (ret)
+		return ret;
+
+	info->pctl_dev = devm_pinctrl_register(&pdev->dev, ctrldesc, info);
+	if (IS_ERR(info->pctl_dev)) {
+		dev_err(&pdev->dev, "could not register pinctrl driver\n");
+		return PTR_ERR(info->pctl_dev);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id armada_37xx_pinctrl_of_match[] = {
+	{
+		.compatible = "marvell,armada3710-sb-pinctrl",
+		.data = (void *)&armada_37xx_pin_sb,
+	},
+	{
+		.compatible = "marvell,armada3710-nb-pinctrl",
+		.data = (void *)&armada_37xx_pin_nb,
+	},
+	{ },
+};
+
+static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct armada_37xx_pinctrl *info;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *regmap;
+	int ret;
+
+	info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
+			    GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = dev;
+
+	regmap = syscon_node_to_regmap(np);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "cannot get regmap\n");
+		return PTR_ERR(regmap);
+	}
+	info->regmap = regmap;
+
+	match = of_match_node(armada_37xx_pinctrl_of_match, np);
+	info->data = (struct armada_37xx_pin_data *)match->data;
+
+	ret = armada_37xx_pinctrl_register(pdev, info);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, info);
+
+	return 0;
+}
+
+static struct platform_driver armada_37xx_pinctrl_driver = {
+	.driver = {
+		.name = "armada-37xx-pinctrl",
+		.of_match_table = armada_37xx_pinctrl_of_match,
+	},
+	.probe = armada_37xx_pinctrl_probe,
+};
+
+builtin_platform_driver(armada_37xx_pinctrl_driver);
-- 
2.11.0

[PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Gregory CLEMENT]

The Armada 37xx SoC come with 2 pin controllers: one on the south
bridge (managing 28 pins) and one on the north bridge (managing 36 pins).

At the hardware level the controller configure the pins by group and not
pin by pin. This constraint is reflected in the design of the driver:
only the group related functions are implemented.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/Kconfig.platforms                |   2 +
 drivers/pinctrl/Makefile                    |   2 +-
 drivers/pinctrl/mvebu/Kconfig               |   7 +
 drivers/pinctrl/mvebu/Makefile              |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 590 ++++++++++++++++++++++++++++
 5 files changed, 602 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 715ef1256838..0786e3e0f5c6 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -105,6 +105,8 @@ config ARCH_MVEBU
 	select ARMADA_37XX_CLK
 	select MVEBU_ODMI
 	select MVEBU_PIC
+	select PINCTRL
+	select PINCTRL_ARMADA_37XX
 	help
 	  This enables support for Marvell EBU familly, including:
 	   - Armada 3700 SoC Family
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 25d50a86981d..b89659b5bfa9 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -44,7 +44,7 @@ obj-y				+= bcm/
 obj-$(CONFIG_PINCTRL_BERLIN)	+= berlin/
 obj-y				+= freescale/
 obj-$(CONFIG_X86)		+= intel/
-obj-$(CONFIG_PINCTRL_MVEBU)	+= mvebu/
+obj-y				+= mvebu/
 obj-y				+= nomadik/
 obj-$(CONFIG_PINCTRL_PXA)	+= pxa/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
index 170602407c0d..98fcf1290acd 100644
--- a/drivers/pinctrl/mvebu/Kconfig
+++ b/drivers/pinctrl/mvebu/Kconfig
@@ -39,3 +39,10 @@ config PINCTRL_ORION
 	select PINCTRL_MVEBU
 
 endif
+
+config PINCTRL_ARMADA_37XX
+       bool
+       select PINMUX
+       select PINCONF
+       select GENERIC_PINCONF
+
diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
index 18270cd5ea43..60c245a60f39 100644
--- a/drivers/pinctrl/mvebu/Makefile
+++ b/drivers/pinctrl/mvebu/Makefile
@@ -1,4 +1,4 @@
-obj-y				+= pinctrl-mvebu.o
+obj-$(CONFIG_PINCTRL_MVEBU)	+= pinctrl-mvebu.o
 obj-$(CONFIG_PINCTRL_DOVE)	+= pinctrl-dove.o
 obj-$(CONFIG_PINCTRL_KIRKWOOD)	+= pinctrl-kirkwood.o
 obj-$(CONFIG_PINCTRL_ARMADA_370) += pinctrl-armada-370.o
@@ -6,4 +6,5 @@ obj-$(CONFIG_PINCTRL_ARMADA_375) += pinctrl-armada-375.o
 obj-$(CONFIG_PINCTRL_ARMADA_38X) += pinctrl-armada-38x.o
 obj-$(CONFIG_PINCTRL_ARMADA_39X) += pinctrl-armada-39x.o
 obj-$(CONFIG_PINCTRL_ARMADA_XP)  += pinctrl-armada-xp.o
+obj-$(CONFIG_PINCTRL_ARMADA_37XX)  += pinctrl-armada-37xx.o
 obj-$(CONFIG_PINCTRL_ORION)  += pinctrl-orion.o
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
new file mode 100644
index 000000000000..021bfe793af3
--- /dev/null
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -0,0 +1,590 @@
+/*
+ * Marvell 37xx SoC pinctrl driver
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "../pinctrl-utils.h"
+
+#define OUTPUT_EN	0x0
+#define OUTPUT_CTL	0x20
+#define SELECTION	0x30
+
+static int global_pin;
+
+#define NB_FUNCS 2
+#define GPIO_PER_REG	32
+
+struct armada_37xx_pin_group {
+	const char	*name;
+	unsigned int	start_pin;
+	unsigned int	npins;
+	u32		reg_mask;
+	unsigned int	extra_pin;
+	unsigned int	extra_npins;
+	const char	*funcs[NB_FUNCS];
+	unsigned int	*pins;
+};
+
+struct armada_37xx_pin_data {
+	u8				nr_pins;
+	char				*name;
+	struct armada_37xx_pin_group	*groups;
+	int				ngroups;
+};
+
+struct armada_37xx_pmx_func {
+	const char		*name;
+	const char		**groups;
+	unsigned int		ngroups;
+};
+
+struct armada_37xx_pinctrl {
+	struct regmap			*regmap;
+	struct armada_37xx_pin_data	*data;
+	struct device			*dev;
+	struct pinctrl_desc		pctl;
+	struct pinctrl_dev		*pctl_dev;
+	struct armada_37xx_pin_group	*groups;
+	unsigned int			ngroups;
+	struct armada_37xx_pmx_func	*funcs;
+	unsigned int			nfuncs;
+};
+
+#define PIN_GRP(_name, _start, _nr, _mask, _func1, _func2)	\
+	{					\
+		.name = _name,			\
+		.start_pin = _start,		\
+		.npins = _nr,			\
+		.reg_mask = _mask,		\
+		.funcs = {_func1, _func2}	\
+	}
+
+#define PIN_GRP_GPIO(_name, _start, _nr, _mask, _func1)	\
+	{					\
+		.name = _name,			\
+		.start_pin = _start,		\
+		.npins = _nr,			\
+		.reg_mask = _mask,		\
+		.funcs = {_func1, "gpio"}	\
+	}
+
+#define PIN_GRP_EXTRA(_name, _start, _nr, _mask, _start2, _nr2, _f1, _f2) \
+	{						\
+		.name = _name,				\
+		.start_pin = _start,			\
+		.npins = _nr,				\
+		.reg_mask = _mask,			\
+		.extra_pin = _start2,			\
+		.extra_npins = _nr2,			\
+		.funcs = {_f1, _f2}			\
+	}
+
+static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
+	PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
+	PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
+	PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
+	PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
+	PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
+	PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
+	PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
+	PIN_GRP_GPIO("pmic1", 17, 1, BIT(7), "pmic"),
+	PIN_GRP_GPIO("pmic0", 16, 1, BIT(8), "pmic"),
+	PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
+	PIN_GRP_GPIO("i2c1", 0, 2, BIT(10), "i2c"),
+	PIN_GRP_GPIO("spi_cs1", 17, 1, BIT(12), "spi"),
+	PIN_GRP_GPIO("spi_cs2", 18, 1, BIT(13), "spi"),
+	PIN_GRP_GPIO("spi_cs3", 19, 1, BIT(14), "spi"),
+	PIN_GRP_GPIO("onewire", 4, 1, BIT(16), "onewire"),
+	PIN_GRP_GPIO("uart1", 25, 2, BIT(17), "uart"),
+	PIN_GRP_GPIO("spi_quad", 15, 2, BIT(18), "spi"),
+	PIN_GRP_EXTRA("uart_2", 9, 2, BIT(19), 18, 2, "gpio", "uart"),
+};
+
+static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
+	PIN_GRP_GPIO("usb32_drvvbus0", 0, 1, BIT(0), "drvbus"),
+	PIN_GRP_GPIO("usb2_drvvbus1", 1, 1, BIT(1), "drvbus"),
+	PIN_GRP_GPIO("sdio_sb", 24, 5, BIT(2), "sdio"),
+	PIN_GRP_EXTRA("rgmii", 6, 14, BIT(3), 23, 1, "mii", "gpio"),
+	PIN_GRP_GPIO("pcie1", 3, 2, BIT(4), "pcie"),
+	PIN_GRP_GPIO("ptp", 20, 3, BIT(5), "ptp"),
+	PIN_GRP("ptp_clk", 21, 1, BIT(6), "ptp", "mii"),
+	PIN_GRP("ptp_trig", 22, 1, BIT(7), "ptp", "mii"),
+	PIN_GRP("mii_col", 23, 1, BIT(8), "mii", "mii_err"),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_nb = {
+	.nr_pins = 36,
+	.name = "GPIO1",
+	.groups = armada_37xx_nb_groups,
+	.ngroups = ARRAY_SIZE(armada_37xx_nb_groups),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_sb = {
+	.nr_pins = 29,
+	.name = "GPIO2",
+	.groups = armada_37xx_sb_groups,
+	.ngroups = ARRAY_SIZE(armada_37xx_sb_groups),
+};
+
+static int armada_37xx_get_func_reg(struct armada_37xx_pin_group *grp,
+				    const char *func)
+{
+	int f;
+
+	for (f = 0; f < NB_FUNCS; f++)
+		if (!strcmp(grp->funcs[f], func))
+			return f;
+
+	return -ENOTSUPP;
+}
+
+static struct armada_37xx_pin_group *armada_37xx_find_next_grp_by_pin(
+	struct armada_37xx_pinctrl *info, int pin, int *grp)
+{
+	while (*grp < info->ngroups) {
+		struct armada_37xx_pin_group *group = &info->groups[*grp];
+		int j;
+
+		*grp = *grp + 1;
+		for (j = 0; j < (group->npins + group->extra_npins); j++)
+			if (group->pins[j] == pin)
+				return group;
+	}
+	return NULL;
+}
+
+static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
+			    unsigned int selector, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
+			    unsigned int selector, unsigned long *configs,
+			    unsigned int num_configs)
+{
+	return -ENOTSUPP;
+}
+
+static struct pinconf_ops armada_37xx_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_group_get = armada_37xx_pin_config_group_get,
+	.pin_config_group_set = armada_37xx_pin_config_group_set,
+};
+
+static int armada_37xx_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->ngroups;
+}
+
+static const char *armada_37xx_get_group_name(struct pinctrl_dev *pctldev,
+					      unsigned int group)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->groups[group].name;
+}
+
+static int armada_37xx_get_group_pins(struct pinctrl_dev *pctldev,
+				      unsigned int selector,
+				      const unsigned int **pins,
+				      unsigned int *npins)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector >= info->ngroups)
+		return -EINVAL;
+
+	*pins = info->groups[selector].pins;
+	*npins = info->groups[selector].npins;
+
+	return 0;
+}
+
+static const struct pinctrl_ops armada_37xx_pctrl_ops = {
+	.get_groups_count	= armada_37xx_get_groups_count,
+	.get_group_name		= armada_37xx_get_group_name,
+	.get_group_pins		= armada_37xx_get_group_pins,
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
+	.dt_free_map		= pinctrl_utils_free_map,
+};
+
+/*
+ * Pinmux_ops handling
+ */
+
+static int armada_37xx_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->nfuncs;
+}
+
+static const char *armada_37xx_pmx_get_func_name(struct pinctrl_dev *pctldev,
+						 unsigned int selector)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->funcs[selector].name;
+}
+
+static int armada_37xx_pmx_get_groups(struct pinctrl_dev *pctldev,
+				      unsigned int selector,
+				      const char * const **groups,
+				      unsigned int * const num_groups)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = info->funcs[selector].groups;
+	*num_groups = info->funcs[selector].ngroups;
+
+	return 0;
+}
+
+static int armada_37xx_pmx_set_by_name(struct pinctrl_dev *pctldev,
+				       const char *name,
+				       struct armada_37xx_pin_group *grp)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int reg = SELECTION;
+	unsigned int mask = grp->reg_mask;
+	int ret, val;
+
+	dev_dbg(info->dev, "enable function %s group %s\n",
+		name, grp->name);
+
+	ret = armada_37xx_get_func_reg(grp, name);
+
+	if (ret < 0)
+		return ret;
+
+	val = ret ? mask : 0;
+
+	regmap_update_bits(info->regmap, reg, mask, val);
+
+	return 0;
+}
+
+static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       unsigned int group)
+{
+
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct armada_37xx_pin_group *grp = &info->groups[group];
+	const char *name = info->funcs[selector].name;
+
+	return armada_37xx_pmx_set_by_name(pctldev, name, grp);
+}
+
+static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
+					   unsigned int offset)
+{
+	unsigned int reg = OUTPUT_EN;
+	unsigned int mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	return regmap_update_bits(info->regmap, reg, mask, 0);
+}
+
+
+
+static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
+					    unsigned int offset, int value)
+{
+	unsigned int reg = OUTPUT_EN;
+	unsigned int mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	return regmap_update_bits(info->regmap, reg, mask, mask);
+}
+
+static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range,
+					      unsigned int offset, bool input)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
+		offset, range->name, offset, input ? "input" : "output");
+
+	if (input)
+		armada_37xx_pmx_direction_input(info, offset);
+	else
+		armada_37xx_pmx_direction_output(info, offset, 0);
+
+	return 0;
+}
+
+static int armada_37xx_gpio_request_enable(struct pinctrl_dev *pctldev,
+					   struct pinctrl_gpio_range *range,
+					   unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct armada_37xx_pin_group *group;
+	int grp = 0;
+
+	dev_dbg(info->dev, "requesting gpio %d\n", offset);
+
+	while ((group = armada_37xx_find_next_grp_by_pin(info, offset, &grp)))
+		armada_37xx_pmx_set_by_name(pctldev, "gpio", group);
+
+	return 0;
+}
+
+static const struct pinmux_ops armada_37xx_pmx_ops = {
+	.get_functions_count	= armada_37xx_pmx_get_funcs_count,
+	.get_function_name	= armada_37xx_pmx_get_func_name,
+	.get_function_groups	= armada_37xx_pmx_get_groups,
+	.set_mux		= armada_37xx_pmx_set,
+	.gpio_request_enable	= armada_37xx_gpio_request_enable,
+	.gpio_set_direction	= armada_37xx_pmx_gpio_set_direction,
+};
+
+static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
+			 const char *name)
+{
+	int i = 0;
+
+	if (*funcsize <= 0)
+		return -EOVERFLOW;
+
+	while (funcs->ngroups) {
+		/* function already there */
+		if (strcmp(funcs->name, name) == 0) {
+			funcs->ngroups++;
+
+			return -EEXIST;
+		}
+		funcs++;
+		i++;
+	}
+
+	/* append new unique function */
+	funcs->name = name;
+	funcs->ngroups = 1;
+	(*funcsize)--;
+
+	return 0;
+}
+
+static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
+{
+	int n, num = 0, funcsize = info->data->nr_pins;
+
+	for (n = 0; n < info->ngroups; n++) {
+		struct armada_37xx_pin_group *grp = &info->groups[n];
+		int i, j, f;
+
+		grp->pins = devm_kzalloc(info->dev,
+					 (grp->npins + grp->extra_npins) *
+					 sizeof(*grp->pins), GFP_KERNEL);
+		if (!grp->pins)
+			return -ENOMEM;
+
+		for (i = 0; i < grp->npins; i++)
+			grp->pins[i] = grp->start_pin + base + i;
+
+		for (j = 0; j < grp->extra_npins; j++)
+			grp->pins[i+j] = grp->extra_pin + base + j;
+
+		for (f = 0; f < NB_FUNCS; f++) {
+			int ret;
+			/* check for unique functions and count groups */
+			ret = _add_function(info->funcs, &funcsize,
+					    grp->funcs[f]);
+			if (ret == -EOVERFLOW)
+				dev_err(info->dev,
+					"More functions than pins(%d)\n",
+					info->data->nr_pins);
+			if (ret < 0)
+				continue;
+			num++;
+		}
+	}
+
+	info->nfuncs = num;
+
+	return 0;
+}
+
+static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
+{
+	struct armada_37xx_pmx_func *funcs = info->funcs;
+	int n;
+
+	for (n = 0; n < info->nfuncs; n++) {
+		const char *name = funcs[n].name;
+		const char **groups;
+		int g;
+
+		funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
+					       sizeof(*(funcs[n].groups)),
+					       GFP_KERNEL);
+		if (!funcs[n].groups)
+			return -ENOMEM;
+
+		groups = funcs[n].groups;
+
+		for (g = 0; g < info->ngroups; g++) {
+			struct armada_37xx_pin_group *gp = &info->groups[g];
+			int f;
+
+			for (f = 0; f < NB_FUNCS; f++) {
+				if (strcmp(gp->funcs[f], name) == 0) {
+					*groups = gp->name;
+					groups++;
+				}
+			}
+		}
+	}
+	return 0;
+}
+
+static int armada_37xx_pinctrl_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct armada_37xx_pin_data *pin_data = info->data;
+	struct pinctrl_desc *ctrldesc = &info->pctl;
+	struct pinctrl_pin_desc *pindesc, *pdesc;
+	int base_pin = global_pin;
+	int pin, ret;
+
+	info->groups = pin_data->groups;
+	info->ngroups = pin_data->ngroups;
+
+	ctrldesc->name = "armada_37xx-pinctrl";
+	ctrldesc->owner = THIS_MODULE;
+	ctrldesc->pctlops = &armada_37xx_pctrl_ops;
+	ctrldesc->pmxops = &armada_37xx_pmx_ops;
+	ctrldesc->confops = &armada_37xx_pinconf_ops;
+
+	pindesc = devm_kzalloc(&pdev->dev, sizeof(*pindesc) *
+			       pin_data->nr_pins, GFP_KERNEL);
+	if (!pindesc)
+		return -ENOMEM;
+
+	ctrldesc->pins = pindesc;
+	ctrldesc->npins = pin_data->nr_pins;
+
+	pdesc = pindesc;
+	for (pin = 0; pin < pin_data->nr_pins; pin++, global_pin++) {
+		pdesc->number = global_pin;
+		pdesc->name = kasprintf(GFP_KERNEL, "%s-%d",
+					pin_data->name, pin);
+		pdesc++;
+	}
+
+	/*
+	 * we allocate functions for number of pins and hope there are
+	 * fewer unique functions than pins available
+	 */
+	info->funcs = devm_kzalloc(&pdev->dev, pin_data->nr_pins *
+			   sizeof(struct armada_37xx_pmx_func), GFP_KERNEL);
+	if (!info->funcs)
+		return -ENOMEM;
+
+
+	ret = armada_37xx_fill_group(info, base_pin);
+	if (ret)
+		return ret;
+
+	ret = armada_37xx_fill_func(info);
+	if (ret)
+		return ret;
+
+	info->pctl_dev = devm_pinctrl_register(&pdev->dev, ctrldesc, info);
+	if (IS_ERR(info->pctl_dev)) {
+		dev_err(&pdev->dev, "could not register pinctrl driver\n");
+		return PTR_ERR(info->pctl_dev);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id armada_37xx_pinctrl_of_match[] = {
+	{
+		.compatible = "marvell,armada3710-sb-pinctrl",
+		.data = (void *)&armada_37xx_pin_sb,
+	},
+	{
+		.compatible = "marvell,armada3710-nb-pinctrl",
+		.data = (void *)&armada_37xx_pin_nb,
+	},
+	{ },
+};
+
+static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct armada_37xx_pinctrl *info;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *regmap;
+	int ret;
+
+	info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
+			    GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = dev;
+
+	regmap = syscon_node_to_regmap(np);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "cannot get regmap\n");
+		return PTR_ERR(regmap);
+	}
+	info->regmap = regmap;
+
+	match = of_match_node(armada_37xx_pinctrl_of_match, np);
+	info->data = (struct armada_37xx_pin_data *)match->data;
+
+	ret = armada_37xx_pinctrl_register(pdev, info);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, info);
+
+	return 0;
+}
+
+static struct platform_driver armada_37xx_pinctrl_driver = {
+	.driver = {
+		.name = "armada-37xx-pinctrl",
+		.of_match_table = armada_37xx_pinctrl_of_match,
+	},
+	.probe = armada_37xx_pinctrl_probe,
+};
+
+builtin_platform_driver(armada_37xx_pinctrl_driver);
-- 
2.11.0

Re: [PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> The Armada 37xx SoC come with 2 pin controllers: one on the south
> bridge (managing 28 pins) and one on the north bridge (managing 36 pins).
>
> At the hardware level the controller configure the pins by group and not
> pin by pin. This constraint is reflected in the design of the driver:
> only the group related functions are implemented.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Overall this looks good.

> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 715ef1256838..0786e3e0f5c6 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -105,6 +105,8 @@ config ARCH_MVEBU
>         select ARMADA_37XX_CLK
>         select MVEBU_ODMI
>         select MVEBU_PIC
> +       select PINCTRL
> +       select PINCTRL_ARMADA_37XX

Do you already select MFD_SYSCON? It seems to be required.

I can't merge patches to ARM SoC and prefer not to. Split this into a separate
patch for ARM SoC in the Armada/Marvell tree.

> -obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
> +obj-y                          += mvebu/

Just make sure everything is guarded with proper symbols.

> +config PINCTRL_ARMADA_37XX
> +       bool
> +       select PINMUX
> +       select PINCONF
> +       select GENERIC_PINCONF

Nice!

> +struct armada_37xx_pin_group {
> +       const char      *name;
> +       unsigned int    start_pin;
> +       unsigned int    npins;
> +       u32             reg_mask;
> +       unsigned int    extra_pin;
> +       unsigned int    extra_npins;
> +       const char      *funcs[NB_FUNCS];
> +       unsigned int    *pins;
> +};

I would prefer if you add some kerneldoc to this struct.
Especially the extra_pin things are not evident so explain this
in detail here.

> +struct armada_37xx_pin_data {
> +       u8                              nr_pins;
> +       char                            *name;
> +       struct armada_37xx_pin_group    *groups;
> +       int                             ngroups;
> +};
> +
> +struct armada_37xx_pmx_func {
> +       const char              *name;
> +       const char              **groups;
> +       unsigned int            ngroups;
> +};
> +
> +struct armada_37xx_pinctrl {
> +       struct regmap                   *regmap;
> +       struct armada_37xx_pin_data     *data;
> +       struct device                   *dev;
> +       struct pinctrl_desc             pctl;
> +       struct pinctrl_dev              *pctl_dev;
> +       struct armada_37xx_pin_group    *groups;
> +       unsigned int                    ngroups;
> +       struct armada_37xx_pmx_func     *funcs;
> +       unsigned int                    nfuncs;
> +};

You do not need to document these. They are self-explanatory.

> +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
> +                           unsigned int selector, unsigned long *config)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
> +                           unsigned int selector, unsigned long *configs,
> +                           unsigned int num_configs)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static struct pinconf_ops armada_37xx_pinconf_ops = {
> +       .is_generic = true,
> +       .pin_config_group_get = armada_37xx_pin_config_group_get,
> +       .pin_config_group_set = armada_37xx_pin_config_group_set,
> +};

Don't we support just leaving group set/get uninitialized? Too bad in that case.

> +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
> +                        const char *name)

No _foo opaque underscore prefix please. Git this a reasonable name
like armada_37xx_add_function() or so.

Apart from that it looks good.

Yours,
Linus Walleij

[PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> The Armada 37xx SoC come with 2 pin controllers: one on the south
> bridge (managing 28 pins) and one on the north bridge (managing 36 pins).
>
> At the hardware level the controller configure the pins by group and not
> pin by pin. This constraint is reflected in the design of the driver:
> only the group related functions are implemented.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Overall this looks good.

> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 715ef1256838..0786e3e0f5c6 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -105,6 +105,8 @@ config ARCH_MVEBU
>         select ARMADA_37XX_CLK
>         select MVEBU_ODMI
>         select MVEBU_PIC
> +       select PINCTRL
> +       select PINCTRL_ARMADA_37XX

Do you already select MFD_SYSCON? It seems to be required.

I can't merge patches to ARM SoC and prefer not to. Split this into a separate
patch for ARM SoC in the Armada/Marvell tree.

> -obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
> +obj-y                          += mvebu/

Just make sure everything is guarded with proper symbols.

> +config PINCTRL_ARMADA_37XX
> +       bool
> +       select PINMUX
> +       select PINCONF
> +       select GENERIC_PINCONF

Nice!

> +struct armada_37xx_pin_group {
> +       const char      *name;
> +       unsigned int    start_pin;
> +       unsigned int    npins;
> +       u32             reg_mask;
> +       unsigned int    extra_pin;
> +       unsigned int    extra_npins;
> +       const char      *funcs[NB_FUNCS];
> +       unsigned int    *pins;
> +};

I would prefer if you add some kerneldoc to this struct.
Especially the extra_pin things are not evident so explain this
in detail here.

> +struct armada_37xx_pin_data {
> +       u8                              nr_pins;
> +       char                            *name;
> +       struct armada_37xx_pin_group    *groups;
> +       int                             ngroups;
> +};
> +
> +struct armada_37xx_pmx_func {
> +       const char              *name;
> +       const char              **groups;
> +       unsigned int            ngroups;
> +};
> +
> +struct armada_37xx_pinctrl {
> +       struct regmap                   *regmap;
> +       struct armada_37xx_pin_data     *data;
> +       struct device                   *dev;
> +       struct pinctrl_desc             pctl;
> +       struct pinctrl_dev              *pctl_dev;
> +       struct armada_37xx_pin_group    *groups;
> +       unsigned int                    ngroups;
> +       struct armada_37xx_pmx_func     *funcs;
> +       unsigned int                    nfuncs;
> +};

You do not need to document these. They are self-explanatory.

> +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
> +                           unsigned int selector, unsigned long *config)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
> +                           unsigned int selector, unsigned long *configs,
> +                           unsigned int num_configs)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static struct pinconf_ops armada_37xx_pinconf_ops = {
> +       .is_generic = true,
> +       .pin_config_group_get = armada_37xx_pin_config_group_get,
> +       .pin_config_group_set = armada_37xx_pin_config_group_set,
> +};

Don't we support just leaving group set/get uninitialized? Too bad in that case.

> +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
> +                        const char *name)

No _foo opaque underscore prefix please. Git this a reasonable name
like armada_37xx_add_function() or so.

Apart from that it looks good.

Yours,
Linus Walleij

Re: [PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Gregory CLEMENT]

Hi Linus,
 
 On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> The Armada 37xx SoC come with 2 pin controllers: one on the south
>> bridge (managing 28 pins) and one on the north bridge (managing 36 pins).
>>
>> At the hardware level the controller configure the pins by group and not
>> pin by pin. This constraint is reflected in the design of the driver:
>> only the group related functions are implemented.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Overall this looks good.
>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 715ef1256838..0786e3e0f5c6 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -105,6 +105,8 @@ config ARCH_MVEBU
>>         select ARMADA_37XX_CLK
>>         select MVEBU_ODMI
>>         select MVEBU_PIC
>> +       select PINCTRL
>> +       select PINCTRL_ARMADA_37XX
>
> Do you already select MFD_SYSCON? It seems to be required.

I added the dependency

>
> I can't merge patches to ARM SoC and prefer not to. Split this into a separate
> patch for ARM SoC in the Armada/Marvell tree.
>
>> -obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
>> +obj-y                          += mvebu/
>

Done it was split.

> Just make sure everything is guarded with proper symbols.

I checked it.

>
>> +struct armada_37xx_pin_group {
>> +       const char      *name;
>> +       unsigned int    start_pin;
>> +       unsigned int    npins;
>> +       u32             reg_mask;
>> +       unsigned int    extra_pin;
>> +       unsigned int    extra_npins;
>> +       const char      *funcs[NB_FUNCS];
>> +       unsigned int    *pins;
>> +};
>
> I would prefer if you add some kerneldoc to this struct.
> Especially the extra_pin things are not evident so explain this
> in detail here.

done.

>> +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
>> +                           unsigned int selector, unsigned long *config)
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
>> +                           unsigned int selector, unsigned long *configs,
>> +                           unsigned int num_configs)
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static struct pinconf_ops armada_37xx_pinconf_ops = {
>> +       .is_generic = true,
>> +       .pin_config_group_get = armada_37xx_pin_config_group_get,
>> +       .pin_config_group_set = armada_37xx_pin_config_group_set,
>> +};
>
> Don't we support just leaving group set/get uninitialized? Too bad in that case.
>

I am not sure to follow you here. On this controller some of the pin
cannot be configured individually but only inside a group. That's why I
set this function not supported. Did I miss something?

>> +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
>> +                        const char *name)
>
> No _foo opaque underscore prefix please. Git this a reasonable name
> like armada_37xx_add_function() or so.

OK done

>
> Apart from that it looks good.

Thanks,

Gregory
-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

[Gregory CLEMENT]

Hi Linus,
 
 On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> The Armada 37xx SoC come with 2 pin controllers: one on the south
>> bridge (managing 28 pins) and one on the north bridge (managing 36 pins).
>>
>> At the hardware level the controller configure the pins by group and not
>> pin by pin. This constraint is reflected in the design of the driver:
>> only the group related functions are implemented.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Overall this looks good.
>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 715ef1256838..0786e3e0f5c6 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -105,6 +105,8 @@ config ARCH_MVEBU
>>         select ARMADA_37XX_CLK
>>         select MVEBU_ODMI
>>         select MVEBU_PIC
>> +       select PINCTRL
>> +       select PINCTRL_ARMADA_37XX
>
> Do you already select MFD_SYSCON? It seems to be required.

I added the dependency

>
> I can't merge patches to ARM SoC and prefer not to. Split this into a separate
> patch for ARM SoC in the Armada/Marvell tree.
>
>> -obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
>> +obj-y                          += mvebu/
>

Done it was split.

> Just make sure everything is guarded with proper symbols.

I checked it.

>
>> +struct armada_37xx_pin_group {
>> +       const char      *name;
>> +       unsigned int    start_pin;
>> +       unsigned int    npins;
>> +       u32             reg_mask;
>> +       unsigned int    extra_pin;
>> +       unsigned int    extra_npins;
>> +       const char      *funcs[NB_FUNCS];
>> +       unsigned int    *pins;
>> +};
>
> I would prefer if you add some kerneldoc to this struct.
> Especially the extra_pin things are not evident so explain this
> in detail here.

done.

>> +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev,
>> +                           unsigned int selector, unsigned long *config)
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev,
>> +                           unsigned int selector, unsigned long *configs,
>> +                           unsigned int num_configs)
>> +{
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static struct pinconf_ops armada_37xx_pinconf_ops = {
>> +       .is_generic = true,
>> +       .pin_config_group_get = armada_37xx_pin_config_group_get,
>> +       .pin_config_group_set = armada_37xx_pin_config_group_set,
>> +};
>
> Don't we support just leaving group set/get uninitialized? Too bad in that case.
>

I am not sure to follow you here. On this controller some of the pin
cannot be configured individually but only inside a group. That's why I
set this function not supported. Did I miss something?

>> +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
>> +                        const char *name)
>
> No _foo opaque underscore prefix please. Git this a reasonable name
> like armada_37xx_add_function() or so.

OK done

>
> Apart from that it looks good.

Thanks,

Gregory
-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

GPIO management is pretty simple and is part of the same IP than the pin
controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
the pinctrl-armada-37xx.c file, it also allows sharing common functions
between the gpiolib and the pinctrl drivers.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 113 ++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 021bfe793af3..4d9571b49ad1 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -10,6 +10,7 @@
  * without any warranty of any kind, whether express or implied.
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -24,6 +25,8 @@
 #include "../pinctrl-utils.h"
 
 #define OUTPUT_EN	0x0
+#define INPUT_VAL	0x10
+#define OUTPUT_VAL	0x18
 #define OUTPUT_CTL	0x20
 #define SELECTION	0x30
 
@@ -60,6 +63,7 @@ struct armada_37xx_pinctrl {
 	struct regmap			*regmap;
 	struct armada_37xx_pin_data	*data;
 	struct device			*dev;
+	struct gpio_chip		gpio_chip;
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
 	struct armada_37xx_pin_group	*groups;
@@ -297,9 +301,10 @@ static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
 	return armada_37xx_pmx_set_by_name(pctldev, name, grp);
 }
 
-static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
-					   unsigned int offset)
+static int armada_37xx_gpio_direction_input(struct gpio_chip *chip,
+					    unsigned int offset)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -312,11 +317,28 @@ static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, 0);
 }
 
+static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
+					  unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_EN;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
 
+	return (val & mask) == 0;
+}
 
-static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
-					    unsigned int offset, int value)
+static int armada_37xx_gpio_direction_output(struct gpio_chip *chip,
+					     unsigned int offset, int value)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -329,19 +351,54 @@ static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, mask);
 }
 
+static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = INPUT_VAL;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
+
+	return (val & mask) != 0;
+}
+
+static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				 int value)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_VAL;
+	unsigned int mask, val;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+	val = value ? mask : 0;
+
+	regmap_update_bits(info->regmap, reg, mask, val);
+}
+
 static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					      struct pinctrl_gpio_range *range,
 					      unsigned int offset, bool input)
 {
 	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = range->gc;
 
 	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
 		offset, range->name, offset, input ? "input" : "output");
 
 	if (input)
-		armada_37xx_pmx_direction_input(info, offset);
+		armada_37xx_gpio_direction_input(chip, offset);
 	else
-		armada_37xx_pmx_direction_output(info, offset, 0);
+		armada_37xx_gpio_direction_output(chip, offset, 0);
 
 	return 0;
 }
@@ -371,6 +428,39 @@ static const struct pinmux_ops armada_37xx_pmx_ops = {
 	.gpio_set_direction	= armada_37xx_pmx_gpio_set_direction,
 };
 
+static const struct gpio_chip armada_37xx_gpiolib_chip = {
+	.request = gpiochip_generic_request,
+	.free = gpiochip_generic_free,
+	.set = armada_37xx_gpio_set,
+	.get = armada_37xx_gpio_get,
+	.get_direction	= armada_37xx_gpio_get_direction,
+	.direction_input = armada_37xx_gpio_direction_input,
+	.direction_output = armada_37xx_gpio_direction_output,
+	.owner = THIS_MODULE,
+};
+
+static int armada_37xx_gpiolib_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct gpio_chip *gc;
+	int ret;
+
+	info->gpio_chip = armada_37xx_gpiolib_chip;
+
+	gc = &info->gpio_chip;
+	gc->ngpio = info->data->nr_pins;
+	gc->parent = &pdev->dev;
+	gc->base = -1;
+	gc->of_node = info->dev->of_node;
+	gc->label = info->data->name;
+
+	ret = gpiochip_add_data(gc, info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
 			 const char *name)
 {
@@ -551,7 +641,7 @@ static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct regmap *regmap;
-	int ret;
+	int ret, pinbase = global_pin;
 
 	info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
 			    GFP_KERNEL);
@@ -570,10 +660,19 @@ static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	match = of_match_node(armada_37xx_pinctrl_of_match, np);
 	info->data = (struct armada_37xx_pin_data *)match->data;
 
+	ret = armada_37xx_gpiolib_register(pdev, info);
+	if (ret)
+		return ret;
+
 	ret = armada_37xx_pinctrl_register(pdev, info);
 	if (ret)
 		return ret;
 
+	ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
+				     pinbase, info->data->nr_pins);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, info);
 
 	return 0;
-- 
2.11.0

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

GPIO management is pretty simple and is part of the same IP than the pin
controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
the pinctrl-armada-37xx.c file, it also allows sharing common functions
between the gpiolib and the pinctrl drivers.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 113 ++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 021bfe793af3..4d9571b49ad1 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -10,6 +10,7 @@
  * without any warranty of any kind, whether express or implied.
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -24,6 +25,8 @@
 #include "../pinctrl-utils.h"
 
 #define OUTPUT_EN	0x0
+#define INPUT_VAL	0x10
+#define OUTPUT_VAL	0x18
 #define OUTPUT_CTL	0x20
 #define SELECTION	0x30
 
@@ -60,6 +63,7 @@ struct armada_37xx_pinctrl {
 	struct regmap			*regmap;
 	struct armada_37xx_pin_data	*data;
 	struct device			*dev;
+	struct gpio_chip		gpio_chip;
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
 	struct armada_37xx_pin_group	*groups;
@@ -297,9 +301,10 @@ static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
 	return armada_37xx_pmx_set_by_name(pctldev, name, grp);
 }
 
-static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
-					   unsigned int offset)
+static int armada_37xx_gpio_direction_input(struct gpio_chip *chip,
+					    unsigned int offset)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -312,11 +317,28 @@ static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, 0);
 }
 
+static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
+					  unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_EN;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
 
+	return (val & mask) == 0;
+}
 
-static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
-					    unsigned int offset, int value)
+static int armada_37xx_gpio_direction_output(struct gpio_chip *chip,
+					     unsigned int offset, int value)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -329,19 +351,54 @@ static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, mask);
 }
 
+static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = INPUT_VAL;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
+
+	return (val & mask) != 0;
+}
+
+static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				 int value)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_VAL;
+	unsigned int mask, val;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+	val = value ? mask : 0;
+
+	regmap_update_bits(info->regmap, reg, mask, val);
+}
+
 static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					      struct pinctrl_gpio_range *range,
 					      unsigned int offset, bool input)
 {
 	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = range->gc;
 
 	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
 		offset, range->name, offset, input ? "input" : "output");
 
 	if (input)
-		armada_37xx_pmx_direction_input(info, offset);
+		armada_37xx_gpio_direction_input(chip, offset);
 	else
-		armada_37xx_pmx_direction_output(info, offset, 0);
+		armada_37xx_gpio_direction_output(chip, offset, 0);
 
 	return 0;
 }
@@ -371,6 +428,39 @@ static const struct pinmux_ops armada_37xx_pmx_ops = {
 	.gpio_set_direction	= armada_37xx_pmx_gpio_set_direction,
 };
 
+static const struct gpio_chip armada_37xx_gpiolib_chip = {
+	.request = gpiochip_generic_request,
+	.free = gpiochip_generic_free,
+	.set = armada_37xx_gpio_set,
+	.get = armada_37xx_gpio_get,
+	.get_direction	= armada_37xx_gpio_get_direction,
+	.direction_input = armada_37xx_gpio_direction_input,
+	.direction_output = armada_37xx_gpio_direction_output,
+	.owner = THIS_MODULE,
+};
+
+static int armada_37xx_gpiolib_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct gpio_chip *gc;
+	int ret;
+
+	info->gpio_chip = armada_37xx_gpiolib_chip;
+
+	gc = &info->gpio_chip;
+	gc->ngpio = info->data->nr_pins;
+	gc->parent = &pdev->dev;
+	gc->base = -1;
+	gc->of_node = info->dev->of_node;
+	gc->label = info->data->name;
+
+	ret = gpiochip_add_data(gc, info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
 			 const char *name)
 {
@@ -551,7 +641,7 @@ static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct regmap *regmap;
-	int ret;
+	int ret, pinbase = global_pin;
 
 	info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
 			    GFP_KERNEL);
@@ -570,10 +660,19 @@ static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	match = of_match_node(armada_37xx_pinctrl_of_match, np);
 	info->data = (struct armada_37xx_pin_data *)match->data;
 
+	ret = armada_37xx_gpiolib_register(pdev, info);
+	if (ret)
+		return ret;
+
 	ret = armada_37xx_pinctrl_register(pdev, info);
 	if (ret)
 		return ret;
 
+	ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
+				     pinbase, info->data->nr_pins);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, info);
 
 	return 0;
-- 
2.11.0

Re: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> GPIO management is pretty simple and is part of the same IP than the pin
> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
> the pinctrl-armada-37xx.c file, it also allows sharing common functions
> between the gpiolib and the pinctrl drivers.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Some remarks:

> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
> +                                         unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = OUTPUT_EN;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }

Add a comment saying we never have more than two registers?
If there would be three registers this would fail, right?

> +       mask = BIT(offset);
> +
> +       regmap_read(info->regmap, reg, &val);
>
> +       return (val & mask) == 0;

Use this:

return !(val & mask);

> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = INPUT_VAL;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }
> +       mask = BIT(offset);

This code is repeating. Break out a static (inline?) helper to do this.

> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
> +                                       struct armada_37xx_pinctrl *info)

Nit: gpiochip_register or so is more to the point.

> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
> +                                    pinbase, info->data->nr_pins);
> +       if (ret)
> +               return ret;

Why do you do this?

Why not just put the ranges into the device tree? We already support
this in the gpiolib core and it is helpful.

See Documentation/devicetree/bindings/gpio/gpio.txt
and other DTS files for gpio-ranges.

Yours,
Linus Walleij

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> GPIO management is pretty simple and is part of the same IP than the pin
> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
> the pinctrl-armada-37xx.c file, it also allows sharing common functions
> between the gpiolib and the pinctrl drivers.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Some remarks:

> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
> +                                         unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = OUTPUT_EN;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }

Add a comment saying we never have more than two registers?
If there would be three registers this would fail, right?

> +       mask = BIT(offset);
> +
> +       regmap_read(info->regmap, reg, &val);
>
> +       return (val & mask) == 0;

Use this:

return !(val & mask);

> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = INPUT_VAL;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }
> +       mask = BIT(offset);

This code is repeating. Break out a static (inline?) helper to do this.

> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
> +                                       struct armada_37xx_pinctrl *info)

Nit: gpiochip_register or so is more to the point.

> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
> +                                    pinbase, info->data->nr_pins);
> +       if (ret)
> +               return ret;

Why do you do this?

Why not just put the ranges into the device tree? We already support
this in the gpiolib core and it is helpful.

See Documentation/devicetree/bindings/gpio/gpio.txt
and other DTS files for gpio-ranges.

Yours,
Linus Walleij

Re: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

Hi Linus,
 
 On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> GPIO management is pretty simple and is part of the same IP than the pin
>> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
>> the pinctrl-armada-37xx.c file, it also allows sharing common functions
>> between the gpiolib and the pinctrl drivers.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Some remarks:
>
>> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
>> +                                         unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = OUTPUT_EN;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>
> Add a comment saying we never have more than two registers?
> If there would be three registers this would fail, right?

I added the comment

>
>> +       mask = BIT(offset);
>> +
>> +       regmap_read(info->regmap, reg, &val);
>>
>> +       return (val & mask) == 0;
>
> Use this:
>
> return !(val & mask);

done but I could tou explain the advantage of doing it?

>
>> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = INPUT_VAL;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>> +       mask = BIT(offset);
>
> This code is repeating. Break out a static (inline?) helper to do
> this.

done

>
>> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
>> +                                       struct armada_37xx_pinctrl *info)
>
> Nit: gpiochip_register or so is more to the point.
>
>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>> +                                    pinbase, info->data->nr_pins);
>> +       if (ret)
>> +               return ret;
>
> Why do you do this?
>
> Why not just put the ranges into the device tree? We already support
> this in the gpiolib core and it is helpful.
>
> See Documentation/devicetree/bindings/gpio/gpio.txt
> and other DTS files for gpio-ranges.

Following your review, I tried to use it but it didn't work for
me. When the second pin controller was probed then there was collision
for the gpio number. I tried several combination without any luck.

So for now I left it aside.

I can show you the errors message I get and the binding I used if you
are interested.


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

Hi Linus,
 
 On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> GPIO management is pretty simple and is part of the same IP than the pin
>> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
>> the pinctrl-armada-37xx.c file, it also allows sharing common functions
>> between the gpiolib and the pinctrl drivers.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Some remarks:
>
>> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
>> +                                         unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = OUTPUT_EN;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>
> Add a comment saying we never have more than two registers?
> If there would be three registers this would fail, right?

I added the comment

>
>> +       mask = BIT(offset);
>> +
>> +       regmap_read(info->regmap, reg, &val);
>>
>> +       return (val & mask) == 0;
>
> Use this:
>
> return !(val & mask);

done but I could tou explain the advantage of doing it?

>
>> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = INPUT_VAL;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>> +       mask = BIT(offset);
>
> This code is repeating. Break out a static (inline?) helper to do
> this.

done

>
>> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
>> +                                       struct armada_37xx_pinctrl *info)
>
> Nit: gpiochip_register or so is more to the point.
>
>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>> +                                    pinbase, info->data->nr_pins);
>> +       if (ret)
>> +               return ret;
>
> Why do you do this?
>
> Why not just put the ranges into the device tree? We already support
> this in the gpiolib core and it is helpful.
>
> See Documentation/devicetree/bindings/gpio/gpio.txt
> and other DTS files for gpio-ranges.

Following your review, I tried to use it but it didn't work for
me. When the second pin controller was probed then there was collision
for the gpio number. I tried several combination without any luck.

So for now I left it aside.

I can show you the errors message I get and the binding I used if you
are interested.


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Linus Walleij]

On Wed, Mar 22, 2017 at 12:54 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>  On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

>>> +       mask = BIT(offset);
>>> +
>>> +       regmap_read(info->regmap, reg, &val);
>>>
>>> +       return (val & mask) == 0;
>>
>> Use this:
>>
>> return !(val & mask);
>
> done but I could tou explain the advantage of doing it?

The other controllers do it so makes the code easier to
perceptualize.

>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>> +                                    pinbase, info->data->nr_pins);
>>> +       if (ret)
>>> +               return ret;
>>
>> Why do you do this?
>>
>> Why not just put the ranges into the device tree? We already support
>> this in the gpiolib core and it is helpful.
>>
>> See Documentation/devicetree/bindings/gpio/gpio.txt
>> and other DTS files for gpio-ranges.
>
> Following your review, I tried to use it but it didn't work for
> me. When the second pin controller was probed then there was collision
> for the gpio number. I tried several combination without any luck.

That sounds like a bug. Are you using dynamic GPIO number
assignments?

Yours,
Linus Walleij

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Linus Walleij]

On Wed, Mar 22, 2017 at 12:54 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>  On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

>>> +       mask = BIT(offset);
>>> +
>>> +       regmap_read(info->regmap, reg, &val);
>>>
>>> +       return (val & mask) == 0;
>>
>> Use this:
>>
>> return !(val & mask);
>
> done but I could tou explain the advantage of doing it?

The other controllers do it so makes the code easier to
perceptualize.

>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>> +                                    pinbase, info->data->nr_pins);
>>> +       if (ret)
>>> +               return ret;
>>
>> Why do you do this?
>>
>> Why not just put the ranges into the device tree? We already support
>> this in the gpiolib core and it is helpful.
>>
>> See Documentation/devicetree/bindings/gpio/gpio.txt
>> and other DTS files for gpio-ranges.
>
> Following your review, I tried to use it but it didn't work for
> me. When the second pin controller was probed then there was collision
> for the gpio number. I tried several combination without any luck.

That sounds like a bug. Are you using dynamic GPIO number
assignments?

Yours,
Linus Walleij

Re: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

Hi Linus,
 
 On jeu., mars 23 2017, Linus Walleij <linus.walleij@linaro.org> wrote:


>>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>>> +                                    pinbase, info->data->nr_pins);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Why do you do this?
>>>
>>> Why not just put the ranges into the device tree? We already support
>>> this in the gpiolib core and it is helpful.
>>>
>>> See Documentation/devicetree/bindings/gpio/gpio.txt
>>> and other DTS files for gpio-ranges.
>>
>> Following your review, I tried to use it but it didn't work for
>> me. When the second pin controller was probed then there was collision
>> for the gpio number. I tried several combination without any luck.
>
> That sounds like a bug. Are you using dynamic GPIO number
> assignments?

I managed to use it, the issue was that I register the gpio before the
pinctrl. The call to gpiochip_add_pin_range() was done before both
registration in my initial call so it was not a problem. When I switched
to the gpio-ranges, this call was done from the gpiochip_add_data(), and
in this case it has tt be called after devm_pinctrl_register().

So I am about sending a new version using gpio-ranges.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/6] pinctrl: armada-37xx: Add gpio support

[Gregory CLEMENT]

Hi Linus,
 
 On jeu., mars 23 2017, Linus Walleij <linus.walleij@linaro.org> wrote:


>>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>>> +                                    pinbase, info->data->nr_pins);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Why do you do this?
>>>
>>> Why not just put the ranges into the device tree? We already support
>>> this in the gpiolib core and it is helpful.
>>>
>>> See Documentation/devicetree/bindings/gpio/gpio.txt
>>> and other DTS files for gpio-ranges.
>>
>> Following your review, I tried to use it but it didn't work for
>> me. When the second pin controller was probed then there was collision
>> for the gpio number. I tried several combination without any luck.
>
> That sounds like a bug. Are you using dynamic GPIO number
> assignments?

I managed to use it, the issue was that I register the gpio before the
pinctrl. The call to gpiochip_add_pin_range() was done before both
registration in my initial call so it was not a problem. When I switched
to the gpio-ranges, this call was done from the gpiochip_add_data(), and
in this case it has tt be called after devm_pinctrl_register().

So I am about sending a new version using gpio-ranges.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

The Armada 37xx SoCs can handle interrupt through GPIO. However it can
only manage the edge ones.

The way the interrupt are managed are classical so we can use the generic
interrupt chip model.

The only unusual "feature" is that many interrupts are connected to the
parent interrupt controller. But we do not take advantage of this and use
the chained irq with all of them.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 159 ++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 4d9571b49ad1..4c714d0beb88 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -13,7 +13,9 @@
 #include <linux/gpio/driver.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -30,6 +32,11 @@
 #define OUTPUT_CTL	0x20
 #define SELECTION	0x30
 
+#define IRQ_EN		0x0
+#define IRQ_POL		0x08
+#define IRQ_STATUS	0x10
+#define IRQ_WKUP	0x18
+
 static int global_pin;
 
 #define NB_FUNCS 2
@@ -64,6 +71,8 @@ struct armada_37xx_pinctrl {
 	struct armada_37xx_pin_data	*data;
 	struct device			*dev;
 	struct gpio_chip		gpio_chip;
+	struct irq_chip			irq_chip;
+	struct irq_domain		*domain;
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
 	struct armada_37xx_pin_group	*groups;
@@ -385,6 +394,14 @@ static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	regmap_update_bits(info->regmap, reg, mask, val);
 }
 
+static int armada_37xx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+
+	return irq_create_mapping(info->domain, offset);
+}
+
+
 static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					      struct pinctrl_gpio_range *range,
 					      unsigned int offset, bool input)
@@ -436,9 +453,148 @@ static const struct gpio_chip armada_37xx_gpiolib_chip = {
 	.get_direction	= armada_37xx_gpio_get_direction,
 	.direction_input = armada_37xx_gpio_direction_input,
 	.direction_output = armada_37xx_gpio_direction_output,
+	.to_irq = armada_37xx_gpio_to_irq,
 	.owner = THIS_MODULE,
 };
 
+static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 reg, mask = d->mask;
+
+	irq_gc_lock(gc);
+	reg  = irq_reg_readl(gc, IRQ_WKUP);
+	if (on)
+		reg |= mask;
+	else
+		reg &= ~mask;
+	irq_reg_writel(gc, reg, IRQ_WKUP);
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 reg, mask = d->mask;
+
+	irq_gc_lock(gc);
+	reg  = irq_reg_readl(gc, IRQ_POL);
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		reg &= ~mask;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		reg |= mask;
+		break;
+	default:
+		irq_gc_unlock(gc);
+		return -EINVAL;
+	}
+	irq_reg_writel(gc, reg, IRQ_POL);
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+
+static void armada_37xx_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *d = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int n;
+
+	chained_irq_enter(chip, desc);
+
+	for (n = 0; n < d->revmap_size; n += 32) {
+		struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n);
+		u32 stat = readl_relaxed(gc->reg_base + IRQ_STATUS);
+
+		while (stat) {
+			u32 hwirq = ffs(stat) - 1;
+			u32 virq = irq_find_mapping(d, gc->irq_base + hwirq);
+
+			generic_handle_irq(virq);
+			stat &= ~(1 << hwirq);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int armada_37xx_irqchip_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct device_node *np = info->dev->of_node;
+	int nrirqs = info->data->nr_pins;
+	struct irq_domain *domain;
+	struct resource res;
+	void __iomem *base;
+	int ret, i, nr_irq_parent;
+
+	nr_irq_parent = of_irq_count(np);
+
+	if (!nr_irq_parent) {
+		dev_err(&pdev->dev, "Invalid or no IRQ\n");
+		return 0;
+	}
+
+	if (of_address_to_resource(np, 1, &res)) {
+		dev_err(info->dev, "cannot find IO resource\n");
+		return -ENOENT;
+	}
+
+	base = devm_ioremap_resource(info->dev, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	domain = irq_domain_add_linear(np, nrirqs,
+				       &irq_generic_chip_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
+	info->domain = domain;
+
+	ret = irq_alloc_domain_generic_chips(domain, GPIO_PER_REG, 1,
+			     np->name, handle_level_irq,
+			     IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0,
+			     IRQ_GC_INIT_MASK_CACHE);
+	if (ret) {
+		dev_err(info->dev, "%s: could not alloc generic chips\n",
+			np->full_name);
+		irq_domain_remove(domain);
+	}
+
+	for (i = 0; i < DIV_ROUND_UP(nrirqs, GPIO_PER_REG); i++) {
+		struct irq_chip_generic *gc;
+
+		gc = irq_get_domain_generic_chip(domain, i * GPIO_PER_REG);
+		gc->reg_base = base + i *  sizeof(u32);
+		gc->chip_types[0].regs.mask = IRQ_EN;
+		gc->chip_types[0].regs.ack = IRQ_STATUS;
+		gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
+		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
+		gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
+		gc->chip_types[0].chip.irq_set_wake = armada_37xx_irq_set_wake;
+		gc->chip_types[0].chip.irq_set_type = armada_37xx_irq_set_type;
+	}
+
+	for (i = 0; i < nr_irq_parent; i++) {
+		int irq = platform_get_irq(pdev, i);
+
+		if (irq < 0)
+			continue;
+
+		irq_set_chained_handler_and_data(irq, armada_37xx_irq_handler,
+						 domain);
+	}
+
+	for (i = 0 ; i < nrirqs ; i++)
+		irq_create_mapping(domain, i);
+
+	return 0;
+}
+
 static int armada_37xx_gpiolib_register(struct platform_device *pdev,
 					struct armada_37xx_pinctrl *info)
 {
@@ -457,6 +613,9 @@ static int armada_37xx_gpiolib_register(struct platform_device *pdev,
 	ret = gpiochip_add_data(gc, info);
 	if (ret)
 		return ret;
+	ret = armada_37xx_irqchip_register(pdev, info);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.11.0

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

The Armada 37xx SoCs can handle interrupt through GPIO. However it can
only manage the edge ones.

The way the interrupt are managed are classical so we can use the generic
interrupt chip model.

The only unusual "feature" is that many interrupts are connected to the
parent interrupt controller. But we do not take advantage of this and use
the chained irq with all of them.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 159 ++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 4d9571b49ad1..4c714d0beb88 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -13,7 +13,9 @@
 #include <linux/gpio/driver.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -30,6 +32,11 @@
 #define OUTPUT_CTL	0x20
 #define SELECTION	0x30
 
+#define IRQ_EN		0x0
+#define IRQ_POL		0x08
+#define IRQ_STATUS	0x10
+#define IRQ_WKUP	0x18
+
 static int global_pin;
 
 #define NB_FUNCS 2
@@ -64,6 +71,8 @@ struct armada_37xx_pinctrl {
 	struct armada_37xx_pin_data	*data;
 	struct device			*dev;
 	struct gpio_chip		gpio_chip;
+	struct irq_chip			irq_chip;
+	struct irq_domain		*domain;
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
 	struct armada_37xx_pin_group	*groups;
@@ -385,6 +394,14 @@ static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	regmap_update_bits(info->regmap, reg, mask, val);
 }
 
+static int armada_37xx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+
+	return irq_create_mapping(info->domain, offset);
+}
+
+
 static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					      struct pinctrl_gpio_range *range,
 					      unsigned int offset, bool input)
@@ -436,9 +453,148 @@ static const struct gpio_chip armada_37xx_gpiolib_chip = {
 	.get_direction	= armada_37xx_gpio_get_direction,
 	.direction_input = armada_37xx_gpio_direction_input,
 	.direction_output = armada_37xx_gpio_direction_output,
+	.to_irq = armada_37xx_gpio_to_irq,
 	.owner = THIS_MODULE,
 };
 
+static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 reg, mask = d->mask;
+
+	irq_gc_lock(gc);
+	reg  = irq_reg_readl(gc, IRQ_WKUP);
+	if (on)
+		reg |= mask;
+	else
+		reg &= ~mask;
+	irq_reg_writel(gc, reg, IRQ_WKUP);
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 reg, mask = d->mask;
+
+	irq_gc_lock(gc);
+	reg  = irq_reg_readl(gc, IRQ_POL);
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		reg &= ~mask;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		reg |= mask;
+		break;
+	default:
+		irq_gc_unlock(gc);
+		return -EINVAL;
+	}
+	irq_reg_writel(gc, reg, IRQ_POL);
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+
+static void armada_37xx_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *d = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int n;
+
+	chained_irq_enter(chip, desc);
+
+	for (n = 0; n < d->revmap_size; n += 32) {
+		struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n);
+		u32 stat = readl_relaxed(gc->reg_base + IRQ_STATUS);
+
+		while (stat) {
+			u32 hwirq = ffs(stat) - 1;
+			u32 virq = irq_find_mapping(d, gc->irq_base + hwirq);
+
+			generic_handle_irq(virq);
+			stat &= ~(1 << hwirq);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int armada_37xx_irqchip_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct device_node *np = info->dev->of_node;
+	int nrirqs = info->data->nr_pins;
+	struct irq_domain *domain;
+	struct resource res;
+	void __iomem *base;
+	int ret, i, nr_irq_parent;
+
+	nr_irq_parent = of_irq_count(np);
+
+	if (!nr_irq_parent) {
+		dev_err(&pdev->dev, "Invalid or no IRQ\n");
+		return 0;
+	}
+
+	if (of_address_to_resource(np, 1, &res)) {
+		dev_err(info->dev, "cannot find IO resource\n");
+		return -ENOENT;
+	}
+
+	base = devm_ioremap_resource(info->dev, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	domain = irq_domain_add_linear(np, nrirqs,
+				       &irq_generic_chip_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
+	info->domain = domain;
+
+	ret = irq_alloc_domain_generic_chips(domain, GPIO_PER_REG, 1,
+			     np->name, handle_level_irq,
+			     IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0,
+			     IRQ_GC_INIT_MASK_CACHE);
+	if (ret) {
+		dev_err(info->dev, "%s: could not alloc generic chips\n",
+			np->full_name);
+		irq_domain_remove(domain);
+	}
+
+	for (i = 0; i < DIV_ROUND_UP(nrirqs, GPIO_PER_REG); i++) {
+		struct irq_chip_generic *gc;
+
+		gc = irq_get_domain_generic_chip(domain, i * GPIO_PER_REG);
+		gc->reg_base = base + i *  sizeof(u32);
+		gc->chip_types[0].regs.mask = IRQ_EN;
+		gc->chip_types[0].regs.ack = IRQ_STATUS;
+		gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
+		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
+		gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
+		gc->chip_types[0].chip.irq_set_wake = armada_37xx_irq_set_wake;
+		gc->chip_types[0].chip.irq_set_type = armada_37xx_irq_set_type;
+	}
+
+	for (i = 0; i < nr_irq_parent; i++) {
+		int irq = platform_get_irq(pdev, i);
+
+		if (irq < 0)
+			continue;
+
+		irq_set_chained_handler_and_data(irq, armada_37xx_irq_handler,
+						 domain);
+	}
+
+	for (i = 0 ; i < nrirqs ; i++)
+		irq_create_mapping(domain, i);
+
+	return 0;
+}
+
 static int armada_37xx_gpiolib_register(struct platform_device *pdev,
 					struct armada_37xx_pinctrl *info)
 {
@@ -457,6 +613,9 @@ static int armada_37xx_gpiolib_register(struct platform_device *pdev,
 	ret = gpiochip_add_data(gc, info);
 	if (ret)
 		return ret;
+	ret = armada_37xx_irqchip_register(pdev, info);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.11.0

Re: [PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
> only manage the edge ones.
>
> The way the interrupt are managed are classical so we can use the generic
> interrupt chip model.
>
> The only unusual "feature" is that many interrupts are connected to the
> parent interrupt controller. But we do not take advantage of this and use
> the chained irq with all of them.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

So this is very simple and should use GPIOLIB_IRQCHIP.

Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
for inspiration.

> @@ -64,6 +71,8 @@ struct armada_37xx_pinctrl {
>         struct armada_37xx_pin_data     *data;
>         struct device                   *dev;
>         struct gpio_chip                gpio_chip;
> +       struct irq_chip                 irq_chip;
> +       struct irq_domain               *domain;

You don't need a domain when using GPIOLIB_IRQCHIP

> +static int armada_37xx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +
> +       return irq_create_mapping(info->domain, offset);
> +}

Nor this.

The irqchip code should be pretty much the same but you need to
dereference gpio_chip from chip data and pick the irqchip from
there.

Yours,
Linus Walleij

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
> only manage the edge ones.
>
> The way the interrupt are managed are classical so we can use the generic
> interrupt chip model.
>
> The only unusual "feature" is that many interrupts are connected to the
> parent interrupt controller. But we do not take advantage of this and use
> the chained irq with all of them.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

So this is very simple and should use GPIOLIB_IRQCHIP.

Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
for inspiration.

> @@ -64,6 +71,8 @@ struct armada_37xx_pinctrl {
>         struct armada_37xx_pin_data     *data;
>         struct device                   *dev;
>         struct gpio_chip                gpio_chip;
> +       struct irq_chip                 irq_chip;
> +       struct irq_domain               *domain;

You don't need a domain when using GPIOLIB_IRQCHIP

> +static int armada_37xx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +
> +       return irq_create_mapping(info->domain, offset);
> +}

Nor this.

The irqchip code should be pretty much the same but you need to
dereference gpio_chip from chip data and pick the irqchip from
there.

Yours,
Linus Walleij

Re: [PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

Hi Linus,
 
 On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
>> only manage the edge ones.
>>
>> The way the interrupt are managed are classical so we can use the generic
>> interrupt chip model.
>>
>> The only unusual "feature" is that many interrupts are connected to the
>> parent interrupt controller. But we do not take advantage of this and use
>> the chained irq with all of them.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> So this is very simple and should use GPIOLIB_IRQCHIP.
>
> Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
> at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
> ("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
> for inspiration.

I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
my code, during the development on the v2 I did a commit only for this
change and here it is the diffstat:
110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

In my first version I used the generic irqchip so I was able to benefit
of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
again some of the functions such as .mask, .unmask an ack.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

Hi Linus,
 
 On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
>> only manage the edge ones.
>>
>> The way the interrupt are managed are classical so we can use the generic
>> interrupt chip model.
>>
>> The only unusual "feature" is that many interrupts are connected to the
>> parent interrupt controller. But we do not take advantage of this and use
>> the chained irq with all of them.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> So this is very simple and should use GPIOLIB_IRQCHIP.
>
> Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
> at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
> ("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
> for inspiration.

I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
my code, during the development on the v2 I did a commit only for this
change and here it is the diffstat:
110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

In my first version I used the generic irqchip so I was able to benefit
of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
again some of the functions such as .mask, .unmask an ack.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Linus Walleij]

On Wed, Mar 22, 2017 at 1:02 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>  On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>
>>> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
>>> only manage the edge ones.
>>>
>>> The way the interrupt are managed are classical so we can use the generic
>>> interrupt chip model.
>>>
>>> The only unusual "feature" is that many interrupts are connected to the
>>> parent interrupt controller. But we do not take advantage of this and use
>>> the chained irq with all of them.
>>>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>> So this is very simple and should use GPIOLIB_IRQCHIP.
>>
>> Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
>> at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
>> ("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
>> for inspiration.
>
> I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
> my code, during the development on the v2 I did a commit only for this
> change and here it is the diffstat:
> 110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
>
> In my first version I used the generic irqchip so I was able to benefit
> of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
> again some of the functions such as .mask, .unmask an ack.

I still think this is better, so thanks.

If you have ideas of how we could combine generic irqchip with
GPIOLIB_IRQCHIP in an efficient manner, I'd be happy to hear
about it? I want gpiolib core to be involved setting things like
the .irq_request/release_resources() for example, so if you
would proceed with this solution, I would have had you add those
duplicating the gpiolib helpers instead.

Yours,
Linus Walleij

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Linus Walleij]

On Wed, Mar 22, 2017 at 1:02 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>  On ven., d?c. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>
>>> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
>>> only manage the edge ones.
>>>
>>> The way the interrupt are managed are classical so we can use the generic
>>> interrupt chip model.
>>>
>>> The only unusual "feature" is that many interrupts are connected to the
>>> parent interrupt controller. But we do not take advantage of this and use
>>> the chained irq with all of them.
>>>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>> So this is very simple and should use GPIOLIB_IRQCHIP.
>>
>> Begin with select GPIOLIB_IRQCHIP in your Kconfig and then look
>> at conversions such as commit 85ae9e512f437cd09bf61564bdba29ab88bab3e3
>> ("pinctrl: bcm2835: switch to GPIOLIB_IRQCHIP")
>> for inspiration.
>
> I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
> my code, during the development on the v2 I did a commit only for this
> change and here it is the diffstat:
> 110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
>
> In my first version I used the generic irqchip so I was able to benefit
> of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
> again some of the functions such as .mask, .unmask an ack.

I still think this is better, so thanks.

If you have ideas of how we could combine generic irqchip with
GPIOLIB_IRQCHIP in an efficient manner, I'd be happy to hear
about it? I want gpiolib core to be involved setting things like
the .irq_request/release_resources() for example, so if you
would proceed with this solution, I would have had you add those
duplicating the gpiolib helpers instead.

Yours,
Linus Walleij

Re: [PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

Hi Linus,
 
 On jeu., mars 23 2017, Linus Walleij <linus.walleij@linaro.org> wrote:

>> I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
>> my code, during the development on the v2 I did a commit only for this
>> change and here it is the diffstat:
>> 110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
>>
>> In my first version I used the generic irqchip so I was able to benefit
>> of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
>> again some of the functions such as .mask, .unmask an ack.
>
> I still think this is better, so thanks.

Yes, I understand that from the point of view of the gpio subsystem it
is better to share as code as possible between the driver.

>
> If you have ideas of how we could combine generic irqchip with
> GPIOLIB_IRQCHIP in an efficient manner, I'd be happy to hear
> about it? I want gpiolib core to be involved setting things like
> the .irq_request/release_resources() for example, so if you
> would proceed with this solution, I would have had you add those
> duplicating the gpiolib helpers instead.

Indeed it could be interesting to be able to combine both, however it
does not seem an easy task: at first sight both subsystem would need to
be modified. It is obviously beyond the scope of this driver, but I will
try to have a look on it to see what I could propose in a separate
series.

Gregory

>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support

[Gregory CLEMENT]

Hi Linus,
 
 On jeu., mars 23 2017, Linus Walleij <linus.walleij@linaro.org> wrote:

>> I switched to the use of GPIOLIB_IRQCHIP however it didn't really simplify
>> my code, during the development on the v2 I did a commit only for this
>> change and here it is the diffstat:
>> 110     72      drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
>>
>> In my first version I used the generic irqchip so I was able to benefit
>> of this framework, by switching to GPIOLIB_IRQCHIP I had to implement
>> again some of the functions such as .mask, .unmask an ack.
>
> I still think this is better, so thanks.

Yes, I understand that from the point of view of the gpio subsystem it
is better to share as code as possible between the driver.

>
> If you have ideas of how we could combine generic irqchip with
> GPIOLIB_IRQCHIP in an efficient manner, I'd be happy to hear
> about it? I want gpiolib core to be involved setting things like
> the .irq_request/release_resources() for example, so if you
> would proceed with this solution, I would have had you add those
> duplicating the gpiolib helpers instead.

Indeed it could be interesting to be able to combine both, however it
does not seem an easy task: at first sight both subsystem would need to
be modified. It is obviously beyond the scope of this driver, but I will
try to have a look on it to see what I could propose in a separate
series.

Gregory

>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 5/6] ARM64: dts: marvell: Add pinctrl nodes for Armada 3700

[Gregory CLEMENT]

Add the nodes for the two pin controller present in the Armada 37xx SoCs.

Initially the node was named gpio1 using the same name that for the
register range in the datasheet. However renaming it pinctr_nb (nb for
North Bridge) makes more sens.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 30 +++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 926a4024c3dd..682ec009121c 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -157,16 +157,40 @@
 				#clock-cells = <1>;
 			};
 
-			gpio1: gpio@13800 {
-				compatible = "marvell,mvebu-gpio-3700",
+			pinctrl_nb: pinctrl-nb@13800 {
+				compatible = "marvell,armada3710-nb-pinctrl",
 				"syscon", "simple-mfd";
-				reg = <0x13800 0x500>;
+				reg = <0x13800 0x100>, <0x13C00 0x20>;
+				interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
 
 				xtalclk: xtal-clk {
 					compatible = "marvell,armada-3700-xtal-clock";
 					clock-output-names = "xtal";
 					#clock-cells = <0>;
 				};
+				};
+			};
+
+			pinctrl_sb: pinctrl-sb@18800 {
+				compatible = "marvell,armada3710-sb-pinctrl",
+				"syscon", "simple-mfd";
+				reg = <0x18800 0x100>, <0x18C00 0x20>;
+				interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			eth0: ethernet@30000 {
-- 
2.11.0

[PATCH 5/6] ARM64: dts: marvell: Add pinctrl nodes for Armada 3700

[Gregory CLEMENT]

Add the nodes for the two pin controller present in the Armada 37xx SoCs.

Initially the node was named gpio1 using the same name that for the
register range in the datasheet. However renaming it pinctr_nb (nb for
North Bridge) makes more sens.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 30 +++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 926a4024c3dd..682ec009121c 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -157,16 +157,40 @@
 				#clock-cells = <1>;
 			};
 
-			gpio1: gpio at 13800 {
-				compatible = "marvell,mvebu-gpio-3700",
+			pinctrl_nb: pinctrl-nb at 13800 {
+				compatible = "marvell,armada3710-nb-pinctrl",
 				"syscon", "simple-mfd";
-				reg = <0x13800 0x500>;
+				reg = <0x13800 0x100>, <0x13C00 0x20>;
+				interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
 
 				xtalclk: xtal-clk {
 					compatible = "marvell,armada-3700-xtal-clock";
 					clock-output-names = "xtal";
 					#clock-cells = <0>;
 				};
+				};
+			};
+
+			pinctrl_sb: pinctrl-sb at 18800 {
+				compatible = "marvell,armada3710-sb-pinctrl",
+				"syscon", "simple-mfd";
+				reg = <0x18800 0x100>, <0x18C00 0x20>;
+				interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			eth0: ethernet at 30000 {
-- 
2.11.0

[PATCH 6/6] ARM64: dts: marvell: armada37xx: add pinctrl definition

[Gregory CLEMENT]

Start to populate the device tree of the Armada 37xx with the pincontrol
configuration used on the board providing a dts.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-db.dts |  8 +++++++
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi   | 32 +++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index 935d37f82ce8..cf262b8ec4ad 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -63,6 +63,8 @@
 };
 
 &i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_pins>;
 	status = "okay";
 };
 
@@ -73,6 +75,8 @@
 
 &spi0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi_quad_pins>;
 
 	m25p80@0 {
 		compatible = "jedec,spi-nor";
@@ -103,6 +107,8 @@
 
 /* Exported on the micro USB connector CON32 through an FTDI */
 &uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>;
 	status = "okay";
 };
 
@@ -128,6 +134,8 @@
 };
 
 &eth0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins>;
 	phy-mode = "rgmii-id";
 	phy = <&phy0>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 682ec009121c..120e1b82c8b0 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -179,8 +179,32 @@
 					clock-output-names = "xtal";
 					#clock-cells = <0>;
 				};
+
+				spi_quad_pins: spi-quad-pins {
+					groups = "spi_quad";
+					function = "spi";
 				};
-			};
+
+				i2c1_pins: i2c1-pins {
+					groups = "i2c1";
+					function = "i2c";
+				};
+
+				i2c2_pins: i2c2-pins {
+					groups = "i2c2";
+					function = "i2c";
+				};
+
+				uart1_pins: uart1-pins {
+					groups = "uart1";
+					function = "uart";
+				};
+
+				uart2_pins: uart2-pins {
+					groups = "uart2";
+					function = "uart";
+				};
+};
 
 			pinctrl_sb: pinctrl-sb@18800 {
 				compatible = "marvell,armada3710-sb-pinctrl",
@@ -191,6 +215,12 @@
 					     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
+
+				rgmii_pins: mii-pins {
+					groups = "rgmii";
+					function = "mii";
+				};
+
 			};
 
 			eth0: ethernet@30000 {
-- 
2.11.0

[PATCH 6/6] ARM64: dts: marvell: armada37xx: add pinctrl definition

[Gregory CLEMENT]

Start to populate the device tree of the Armada 37xx with the pincontrol
configuration used on the board providing a dts.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-db.dts |  8 +++++++
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi   | 32 +++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index 935d37f82ce8..cf262b8ec4ad 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -63,6 +63,8 @@
 };
 
 &i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_pins>;
 	status = "okay";
 };
 
@@ -73,6 +75,8 @@
 
 &spi0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi_quad_pins>;
 
 	m25p80 at 0 {
 		compatible = "jedec,spi-nor";
@@ -103,6 +107,8 @@
 
 /* Exported on the micro USB connector CON32 through an FTDI */
 &uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>;
 	status = "okay";
 };
 
@@ -128,6 +134,8 @@
 };
 
 &eth0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins>;
 	phy-mode = "rgmii-id";
 	phy = <&phy0>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 682ec009121c..120e1b82c8b0 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -179,8 +179,32 @@
 					clock-output-names = "xtal";
 					#clock-cells = <0>;
 				};
+
+				spi_quad_pins: spi-quad-pins {
+					groups = "spi_quad";
+					function = "spi";
 				};
-			};
+
+				i2c1_pins: i2c1-pins {
+					groups = "i2c1";
+					function = "i2c";
+				};
+
+				i2c2_pins: i2c2-pins {
+					groups = "i2c2";
+					function = "i2c";
+				};
+
+				uart1_pins: uart1-pins {
+					groups = "uart1";
+					function = "uart";
+				};
+
+				uart2_pins: uart2-pins {
+					groups = "uart2";
+					function = "uart";
+				};
+};
 
 			pinctrl_sb: pinctrl-sb at 18800 {
 				compatible = "marvell,armada3710-sb-pinctrl",
@@ -191,6 +215,12 @@
 					     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
+
+				rgmii_pins: mii-pins {
+					groups = "rgmii";
+					function = "mii";
+				};
+
 			};
 
 			eth0: ethernet at 30000 {
-- 
2.11.0

Re: [PATCH 6/6] ARM64: dts: marvell: armada37xx: add pinctrl definition

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:25 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> Start to populate the device tree of the Armada 37xx with the pincontrol
> configuration used on the board providing a dts.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

>  arch/arm64/boot/dts/marvell/armada-3720-db.dts |  8 +++++++

Maybe you want to take this opportunity to add gpio-line-names = ...;
to the armada 3720 pinctrl/gpiochip nodes?

Yours,
Linus Walleij

[PATCH 6/6] ARM64: dts: marvell: armada37xx: add pinctrl definition

[Linus Walleij]

On Thu, Dec 22, 2016 at 6:25 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> Start to populate the device tree of the Armada 37xx with the pincontrol
> configuration used on the board providing a dts.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

>  arch/arm64/boot/dts/marvell/armada-3720-db.dts |  8 +++++++

Maybe you want to take this opportunity to add gpio-line-names = ...;
to the armada 3720 pinctrl/gpiochip nodes?

Yours,
Linus Walleij