[PATCH v2 0/2] add SAMA5D2 PIOBU GPIO driver

[PATCH v2 0/2] add SAMA5D2 PIOBU GPIO driver

[Andrei.Stefanescu]

On SAMA5D2 SoC the PIOBU pins do not lose their
voltage during Backup/Self-refresh mode. This can
be useful, for example, when the voltage must remain
positive for a peripheral during Backup/Self-refresh
mode (suspend-to ram is the Linux equivalent state).

v2:
- make driver be a subnode of the syscon node
- change Kconfig to depend on MFD_SYSCON and select GPIO_SYSCON
- change include header from linux/gpio.h to linux/gpio/driver.h
- include linux/bits.h header
- change intrusion in comment to tamper
- fix kerneldoc of functions
- replace GPIOF_DIR_* flags with 0/1
- replace ?: statement with if-else
- remove the use of sama5d2_piobu_template_chip
- retrieve syscon via syscon_node_to_regmap(pdev->dev.parent->of_node);

Note that PIOBU_REG_SIZE is used to determine the register to write to
with regmap:
	reg = PIOBU_BASE + pin * PIOBU_REG_SIZE;

Also, no irq capability implemented.

Andrei Stefanescu (2):
  dt-bindings: gpio: add SAMA5D2 PIOBU support
  gpio: add driver for SAMA5D2 PIOBU pins

 .../bindings/gpio/gpio-sama5d2-piobu.txt           |  31 +++
 MAINTAINERS                                        |   7 +
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-sama5d2-piobu.c                  | 253 +++++++++++++++++++++
 5 files changed, 303 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
 create mode 100644 drivers/gpio/gpio-sama5d2-piobu.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: gpio: add SAMA5D2 PIOBU support

[Andrei.Stefanescu]

This patch describes the compatible and the device tree
bindings necessary for the SAMA5D2 PIOBU GPIO.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 .../bindings/gpio/gpio-sama5d2-piobu.txt           | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
new file mode 100644
index 0000000..2e260e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
@@ -0,0 +1,31 @@
+GPIO controller for SAMA5D2 PIOBU pins.
+
+These pins have the property of not losing their voltage
+during Backup/Self-refresh mode.
+
+These bindings should be set to a node in the dtsi file.
+
+Required properties:
+- compatible:		"syscon", "microchip,sama5d2-piobu"
+- #gpio-cells:		There are 2. The pin number is the
+			first, the second represents additional
+			parameters such as GPIO_ACTIVE_HIGH/LOW.
+- gpio-controller:	Marks the port as GPIO controller.
+
+Note that the driver uses syscon and should be the child of
+the syscon node.
+
+Example:
+
+	secumod@fc040000 {
+		compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd";
+		status = "okay";
+		reg = <0xfc040000 0x100>;
+
+		pioBU: piobu {
+			status = "okay";
+			compatible = "microchip,sama5d2-piobu";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
-- 
2.7.4

[PATCH v2 2/2] gpio: add driver for SAMA5D2 PIOBU pins

[Andrei.Stefanescu]

PIOBU pins do not lose their voltage during Backup/Self-refresh.
This patch adds a simple GPIO controller for them and a
maintainer for the driver.

This driver adds support for using the pins as GPIO
offering the possibility to read/set the voltage.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 MAINTAINERS                       |   7 ++
 drivers/gpio/Kconfig              |  11 ++
 drivers/gpio/Makefile             |   1 +
 drivers/gpio/gpio-sama5d2-piobu.c | 253 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 272 insertions(+)
 create mode 100644 drivers/gpio/gpio-sama5d2-piobu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f485597..88369f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9760,6 +9760,13 @@ M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 S:	Supported
 F:	drivers/power/reset/at91-sama5d2_shdwc.c
 
+MICROCHIP SAMA5D2-COMPATIBLE PIOBU GPIO
+M:	Andrei Stefanescu <andrei.stefanescu@microchip.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+L:	linux-gpio@vger.kernel.org
+F:	drivers/gpio/gpio-sama5d2-piobu.c
+F:	Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
+
 MICROCHIP SPI DRIVER
 M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 S:	Supported
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 833a1b5..1c41fac 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -429,6 +429,17 @@ config GPIO_REG
 	  A 32-bit single register GPIO fixed in/out implementation.  This
 	  can be used to represent any register as a set of GPIO signals.
 
+config GPIO_SAMA5D2_PIOBU
+	tristate "SAMA5D2 PIOBU GPIO support"
+	depends on MFD_SYSCON
+	select GPIO_SYSCON
+	help
+	  Say yes here to use the PIOBU pins as GPIOs.
+
+	  PIOBU pins on the SAMA5D2 can be used as GPIOs.
+	  The difference from regular GPIOs is that they
+	  maintain their value during backup/self-refresh.
+
 config GPIO_SIOX
 	tristate "SIOX GPIO support"
 	depends on SIOX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 671c447..f18d345 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_RDC321X)	+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
 obj-$(CONFIG_GPIO_REG)		+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
+obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
 obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SNPS_CREG)	+= gpio-creg-snps.o
diff --git a/drivers/gpio/gpio-sama5d2-piobu.c b/drivers/gpio/gpio-sama5d2-piobu.c
new file mode 100644
index 0000000..db0cb5d
--- /dev/null
+++ b/drivers/gpio/gpio-sama5d2-piobu.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SAMA5D2 PIOBU GPIO controller
+ *
+ * Copyright (C) 2018 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Andrei Stefanescu <andrei.stefanescu@microchip.com>
+ *
+ */
+#include <linux/bits.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define PIOBU_NUM 8
+#define PIOBU_REG_SIZE 4
+
+/*
+ * backup mode protection register for tamper detection
+ * normal mode protection register for tamper detection
+ * wakeup signal generation
+ */
+#define PIOBU_BMPR 0x7C
+#define PIOBU_NMPR 0x80
+#define PIOBU_WKPR 0x90
+
+#define PIOBU_BASE 0x18 /* PIOBU offset from SECUMOD base register address. */
+
+#define PIOBU_DET_OFFSET 16
+
+/* In the datasheet this bit is called OUTPUT */
+#define PIOBU_DIRECTION BIT(8)
+#define PIOBU_OUT BIT(8)
+#define PIOBU_IN 0
+
+#define PIOBU_SOD BIT(9)
+#define PIOBU_PDS BIT(10)
+
+#define PIOBU_HIGH BIT(9)
+#define PIOBU_LOW 0
+
+struct sama5d2_piobu {
+	struct gpio_chip chip;
+	struct regmap *regmap;
+};
+
+/**
+ * sama5d2_piobu_setup_pin() - prepares a pin for set_direction call
+ *
+ * Do not consider pin for tamper detection (normal and backup modes)
+ * Do not consider pin as tamper wakeup interrupt source
+ */
+static int sama5d2_piobu_setup_pin(struct gpio_chip *chip, unsigned int pin)
+{
+	int ret;
+	struct sama5d2_piobu *piobu = container_of(chip, struct sama5d2_piobu,
+						   chip);
+	unsigned int mask = BIT(PIOBU_DET_OFFSET + pin);
+
+	ret = regmap_update_bits(piobu->regmap, PIOBU_BMPR, mask, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(piobu->regmap, PIOBU_NMPR, mask, 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(piobu->regmap, PIOBU_WKPR, mask, 0);
+}
+
+/**
+ * sama5d2_piobu_write_value() - writes value & mask at the pin's PIOBU register
+ */
+static int sama5d2_piobu_write_value(struct gpio_chip *chip, unsigned int pin,
+				     unsigned int mask, unsigned int value)
+{
+	int reg;
+	struct sama5d2_piobu *piobu = container_of(chip, struct sama5d2_piobu,
+						   chip);
+
+	reg = PIOBU_BASE + pin * PIOBU_REG_SIZE;
+
+	return regmap_update_bits(piobu->regmap, reg, mask, value);
+}
+
+/**
+ * sama5d2_piobu_read_value() - read the value with masking from the pin's PIOBU
+ *			      register
+ */
+static int sama5d2_piobu_read_value(struct gpio_chip *chip, unsigned int pin,
+				    unsigned int mask)
+{
+	struct sama5d2_piobu *piobu = container_of(chip, struct sama5d2_piobu,
+						   chip);
+	unsigned int val, reg;
+	int ret;
+
+	reg = PIOBU_BASE + pin * PIOBU_REG_SIZE;
+	ret = regmap_read(piobu->regmap, reg, &val);
+	if (ret < 0)
+		return ret;
+
+	return val & mask;
+}
+
+/**
+ * sama5d2_piobu_set_direction() - mark pin as input or output
+ */
+static int sama5d2_piobu_set_direction(struct gpio_chip *chip,
+				       unsigned int direction,
+				       unsigned int pin)
+{
+	return sama5d2_piobu_write_value(chip, pin, PIOBU_DIRECTION, direction);
+}
+
+/**
+ * sama5d2_piobu_get_direction() - gpiochip get_direction
+ */
+static int sama5d2_piobu_get_direction(struct gpio_chip *chip,
+				       unsigned int pin)
+{
+	int ret = sama5d2_piobu_read_value(chip, pin, PIOBU_DIRECTION);
+
+	if (ret < 0)
+		return ret;
+
+	return (ret == PIOBU_IN) ? 1 : 0;
+}
+
+/**
+ * sama5d2_piobu_direction_input() - gpiochip direction_input
+ */
+static int sama5d2_piobu_direction_input(struct gpio_chip *chip,
+					 unsigned int pin)
+{
+	return sama5d2_piobu_set_direction(chip, PIOBU_IN, pin);
+}
+
+/**
+ * sama5d2_piobu_direction_output() - gpiochip direction_output
+ */
+static int sama5d2_piobu_direction_output(struct gpio_chip *chip,
+					  unsigned int pin, int value)
+{
+	return sama5d2_piobu_set_direction(chip, PIOBU_OUT, pin);
+}
+
+/**
+ * sama5d2_piobu_get() - gpiochip get
+ */
+static int sama5d2_piobu_get(struct gpio_chip *chip, unsigned int pin)
+{
+	/* if pin is input, read value from PDS else read from SOD */
+	int ret = sama5d2_piobu_get_direction(chip, pin);
+
+	if (ret == 1)
+		ret = sama5d2_piobu_read_value(chip, pin, PIOBU_PDS);
+	else if (!ret)
+		ret = sama5d2_piobu_read_value(chip, pin, PIOBU_SOD);
+
+	if (ret < 0)
+		return ret;
+
+	return !!ret;
+}
+
+/**
+ * sama5d2_piobu_set() - gpiochip set
+ */
+static void sama5d2_piobu_set(struct gpio_chip *chip, unsigned int pin,
+			      int value)
+{
+	if (!value)
+		value = PIOBU_LOW;
+	else
+		value = PIOBU_HIGH;
+
+	sama5d2_piobu_write_value(chip, pin, PIOBU_SOD, value);
+}
+
+static int sama5d2_piobu_probe(struct platform_device *pdev)
+{
+	struct sama5d2_piobu *piobu;
+	int ret, i;
+
+	piobu = devm_kzalloc(&pdev->dev, sizeof(*piobu), GFP_KERNEL);
+	if (!piobu)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, piobu);
+	piobu->chip.label = pdev->name;
+	piobu->chip.parent = &pdev->dev;
+	piobu->chip.of_node = pdev->dev.of_node;
+	piobu->chip.owner = THIS_MODULE,
+	piobu->chip.get_direction = sama5d2_piobu_get_direction,
+	piobu->chip.direction_input = sama5d2_piobu_direction_input,
+	piobu->chip.direction_output = sama5d2_piobu_direction_output,
+	piobu->chip.get = sama5d2_piobu_get,
+	piobu->chip.set = sama5d2_piobu_set,
+	piobu->chip.base = -1,
+	piobu->chip.ngpio = PIOBU_NUM,
+	piobu->chip.can_sleep = 0,
+
+	piobu->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(piobu->regmap)) {
+		dev_err(&pdev->dev, "Failed to get syscon regmap %ld\n",
+			PTR_ERR(piobu->regmap));
+		return PTR_ERR(piobu->regmap);
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &piobu->chip, piobu);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add gpiochip %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < PIOBU_NUM; ++i) {
+		ret = sama5d2_piobu_setup_pin(&piobu->chip, i);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to setup pin: %d %d\n",
+				i, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id sama5d2_piobu_ids[] = {
+	{ .compatible = "microchip,sama5d2-piobu" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sama5d2_piobu_ids);
+
+static struct platform_driver sama5d2_piobu_driver = {
+	.driver = {
+		.name		= "sama5d2-piobu",
+		.of_match_table	= of_match_ptr(sama5d2_piobu_ids)
+	},
+	.probe = sama5d2_piobu_probe,
+};
+
+module_platform_driver(sama5d2_piobu_driver);
+
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("SAMA5D2 PIOBU controller driver");
+MODULE_AUTHOR("Andrei Stefanescu <andrei.stefanescu@microchip.com>");
-- 
2.7.4

Re: [PATCH v2 0/2] add SAMA5D2 PIOBU GPIO driver

[Linus Walleij]

Hi Andrei,

On Tue, Nov 20, 2018 at 9:08 AM <Andrei.Stefanescu@microchip.com> wrote:

> On SAMA5D2 SoC the PIOBU pins do not lose their
> voltage during Backup/Self-refresh mode. This can
> be useful, for example, when the voltage must remain
> positive for a peripheral during Backup/Self-refresh
> mode (suspend-to ram is the Linux equivalent state).
>
> v2:

This version looks good to me, I'm just waiting for a DT
binding review.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] dt-bindings: gpio: add SAMA5D2 PIOBU support

[Andrei.Stefanescu]

Hello Rob,

Could you please tell me if there are any improvements
to be made to the patch?

Best regards,
Andrei

On 20.11.2018 10:08, Andrei Stefanescu - M50506 wrote:
> This patch describes the compatible and the device tree
> bindings necessary for the SAMA5D2 PIOBU GPIO.
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
> ---
>   .../bindings/gpio/gpio-sama5d2-piobu.txt           | 31 ++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
> new file mode 100644
> index 0000000..2e260e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
> @@ -0,0 +1,31 @@
> +GPIO controller for SAMA5D2 PIOBU pins.
> +
> +These pins have the property of not losing their voltage
> +during Backup/Self-refresh mode.
> +
> +These bindings should be set to a node in the dtsi file.
> +
> +Required properties:
> +- compatible:		"syscon", "microchip,sama5d2-piobu"
> +- #gpio-cells:		There are 2. The pin number is the
> +			first, the second represents additional
> +			parameters such as GPIO_ACTIVE_HIGH/LOW.
> +- gpio-controller:	Marks the port as GPIO controller.
> +
> +Note that the driver uses syscon and should be the child of
> +the syscon node.
> +
> +Example:
> +
> +	secumod@fc040000 {
> +		compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd";
> +		status = "okay";
> +		reg = <0xfc040000 0x100>;
> +
> +		pioBU: piobu {
> +			status = "okay";
> +			compatible = "microchip,sama5d2-piobu";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};

Re: [PATCH v2 1/2] dt-bindings: gpio: add SAMA5D2 PIOBU support

[Rob Herring]

On Tue, Nov 20, 2018 at 08:08:36AM +0000, Andrei.Stefanescu@microchip.com wrote:
> This patch describes the compatible and the device tree
> bindings necessary for the SAMA5D2 PIOBU GPIO.
> 
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
> ---
>  .../bindings/gpio/gpio-sama5d2-piobu.txt           | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
> new file mode 100644
> index 0000000..2e260e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt

microchip,sama5d2-piobu.txt for the file name.

> @@ -0,0 +1,31 @@
> +GPIO controller for SAMA5D2 PIOBU pins.
> +
> +These pins have the property of not losing their voltage
> +during Backup/Self-refresh mode.
> +
> +These bindings should be set to a node in the dtsi file.
> +
> +Required properties:
> +- compatible:		"syscon", "microchip,sama5d2-piobu"

syscon should be removed.

> +- #gpio-cells:		There are 2. The pin number is the
> +			first, the second represents additional
> +			parameters such as GPIO_ACTIVE_HIGH/LOW.
> +- gpio-controller:	Marks the port as GPIO controller.
> +
> +Note that the driver uses syscon and should be the child of
> +the syscon node.

child of the "atmel,sama5d2-secumod" node to be more specific.

But why do you need a child node? The parent can be a gpio provider. 
What other nodes does this need?

> +
> +Example:
> +
> +	secumod@fc040000 {
> +		compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd";

This is not documented as being a simple-mfd.

> +		status = "okay";
> +		reg = <0xfc040000 0x100>;
> +
> +		pioBU: piobu {

gpio {

Is there not a register range you can put here?

> +			status = "okay";
> +			compatible = "microchip,sama5d2-piobu";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] dt-bindings: gpio: add SAMA5D2 PIOBU support

[Andrei.Stefanescu]

Hello Rob,

Thank you for your feedback.

I will add a bit of context regarding the secumod. The 
"atmel,sama5d2-secumod"
compatible string is not used for loading a driver. It is used by atmel 
securam
driver (drivers/misc/sram.c) which has access to secumod's registers via:

     syscon_regmap_lookup_by_compatible("atmel,sama5d2-secumod")

So the secumod exports multiple hardware functions, eg: the securam, the 
PIOBU
pins which can be used as GPIO pins.

My initial patch had the "microchip,sama5d2-piobu" compatible appended 
to the
secumod's compatible e.g.:

secumod@fc040000 {
     compatible = "syscon", "microchip,sama5d2-piobu";
...

Taking into consideration Linus Walleij's advice I arrived to the current
version. I thought this was a good idea because it separates the secumod 
node
from the GPIO controller node. Please notice that securam node is already
separated from secumod node (it is a separate node in the device tree).

I have a few questions because I am not sure of the best approach:

1. Is it ok to have the GPIO controller as a child node?
2. I used simple-mfd because it was the only way I could think of in 
order to
    get the driver probed.
3. Should I add a register range? I thought that because the driver uses 
syscon
    it is not necessary to add the register range. Also, the register 
range would
    have been a subset of the secumod's register range.

Re: [PATCH v2 1/2] dt-bindings: gpio: add SAMA5D2 PIOBU support

[Rob Herring]

On Wed, Dec 5, 2018 at 5:06 AM <Andrei.Stefanescu@microchip.com> wrote:
>
> Hello Rob,
>
> Thank you for your feedback.
>
> I will add a bit of context regarding the secumod. The
> "atmel,sama5d2-secumod"
> compatible string is not used for loading a driver. It is used by atmel
> securam
> driver (drivers/misc/sram.c) which has access to secumod's registers via:
>
>      syscon_regmap_lookup_by_compatible("atmel,sama5d2-secumod")
>
> So the secumod exports multiple hardware functions, eg: the securam, the
> PIOBU
> pins which can be used as GPIO pins.

Yes, I understand why you want to design it this way, but don't design
your bindings around how 1 OS happens to currently work.

>
> My initial patch had the "microchip,sama5d2-piobu" compatible appended
> to the
> secumod's compatible e.g.:
>
> secumod@fc040000 {
>      compatible = "syscon", "microchip,sama5d2-piobu";

That doesn't look appended to me. The documentation says it should
look like this:

compatible = "atmel,sama5d2-secumod", "syscon";

> ...
>
> Taking into consideration Linus Walleij's advice I arrived to the current
> version. I thought this was a good idea because it separates the secumod
> node
> from the GPIO controller node. Please notice that securam node is already
> separated from secumod node (it is a separate node in the device tree).
>
> I have a few questions because I am not sure of the best approach:
>
> 1. Is it ok to have the GPIO controller as a child node?

Is it a separate h/w block and do you have other child nodes that need
their own resources in DT (interrupts, clocks, etc.)? If these aren't
the case, then no you shouldn't. As it stands, you don't have any
other child nodes, so no you shouldn't have a child node here. Just
add to properties to the existing binding:

secumod@fc040000 {
        compatible = "atmel,sama5d2-secumod", "syscon";
        reg = <0xfc040000 0x100>;
        #gpio-cells = <2>;
        gpio-controller;
};

Now perhaps you have 10 other functions in this block like pinctrl,
clocks, phys, power domains, etc. Then child nodes would be warranted.
But if that's the case, then please write a complete binding for the
secumod block.

> 2. I used simple-mfd because it was the only way I could think of in
> order to
>     get the driver probed.

Sounds like an OS problem that has little to do with the binding. In
any case, if you change a binding which you have, then it needs to be
documented.

> 3. Should I add a register range? I thought that because the driver uses
> syscon
>     it is not necessary to add the register range. Also, the register
> range would
>     have been a subset of the secumod's register range.

If it has one, then yes you should. If it doesn't then it is probably
not a separate h/w block and shouldn't have a child node to begin
with.

Rob