[PATCH 0/3] add sama5d2 PIOBU GPIO driver

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

This patch series:
- documents the driver's necessary device tree bindings
- adds a MAINTAINER for the driver,
- provides a basic GPIO controller driver for them

Andrei Stefanescu (3):
  dt-bindings: gpio: add sama5d2 PIOBU support
  MAINTAINERS: add maintainer for sama5d2 PIOBU GPIO driver
  gpio: added driver for sama5d2 PIOBU pins

 .../bindings/gpio/gpio-sama5d2-piobu.txt           |  23 ++
 MAINTAINERS                                        |   7 +
 drivers/gpio/Kconfig                               |  10 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-sama5d2-piobu.c                  | 254 +++++++++++++++++++++
 5 files changed, 295 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 1/3] 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
controller driver.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 .../bindings/gpio/gpio-sama5d2-piobu.txt           | 23 ++++++++++++++++++++++
 1 file changed, 23 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..791ac51
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
@@ -0,0 +1,23 @@
+GPIO controller for SAMA5D2 PIOBU pins.
+
+This 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.
+
+Example:
+
+	secumod@fc040000 {
+		compatible = "syscon", "microchip,sama5d2-piobu";
+		reg = <0xfc040000 0x100>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
-- 
2.7.4

Re: [PATCH 1/3] dt-bindings: gpio: add sama5d2 PIOBU support

[Linus Walleij]

On Wed, Nov 7, 2018 at 4:12 PM <Andrei.Stefanescu@microchip.com> wrote:

> +Required properties:
> +- compatible:          "syscon", "microchip,sama5d2-piobu"

Looks like "syscon" should be dropped, it is using a syscon,
but it is no syscon.

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

Isn't it easiest to just make this driver be a subnode of the
syscon so it can just refer to its parent to find the
syscon it needs?

Just state that in the bindings and make an example where
it is inside the syscon.

Yours,
Linus Walleij

[PATCH 2/3] MAINTAINERS: add maintainer for sama5d2 PIOBU GPIO driver

[Andrei.Stefanescu]

This patch adds a maintainer for the sama5d2 GPIO controller
driver.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

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

Re: [PATCH 2/3] MAINTAINERS: add maintainer for sama5d2 PIOBU GPIO driver

[Linus Walleij]

On Wed, Nov 7, 2018 at 4:12 PM <Andrei.Stefanescu@microchip.com> wrote:

> This patch adds a maintainer for the sama5d2 GPIO controller
> driver.
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>

Just merge this in with the driver patch, thanks!

Yours,
Linus Walleij

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

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>
---
 drivers/gpio/Kconfig              |  10 ++
 drivers/gpio/Makefile             |   1 +
 drivers/gpio/gpio-sama5d2-piobu.c | 254 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/gpio/gpio-sama5d2-piobu.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 833a1b5..e562e32 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -429,6 +429,16 @@ 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 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..4ded370
--- /dev/null
+++ b/drivers/gpio/gpio-sama5d2-piobu.c
@@ -0,0 +1,254 @@
+// 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/gpio.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 intrusion detection
+ * normal mode protection register for intrusion 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 sama5d2_piobu_set_direction call
+ *
+ * Do not consider pin for intrusion detection (normal and backup modes)
+ * Do not consider pin as intrusion 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, ret;
+	struct sama5d2_piobu *piobu = container_of(chip, struct sama5d2_piobu,
+						   chip);
+
+	reg = PIOBU_BASE + pin * PIOBU_REG_SIZE;
+	ret = regmap_update_bits(piobu->regmap, reg, mask, value);
+	return ret;
+}
+
+/*
+ * 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) ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
+}
+
+/*
+ * 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 == GPIOF_DIR_IN)
+		ret = sama5d2_piobu_read_value(chip, pin, PIOBU_PDS);
+	else if (ret == GPIOF_DIR_OUT)
+		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)
+{
+	value = (value == 0) ? PIOBU_LOW : PIOBU_HIGH;
+	sama5d2_piobu_write_value(chip, pin, PIOBU_SOD, value);
+}
+
+const struct gpio_chip sama5d2_piobu_template_chip = {
+	.owner = THIS_MODULE,
+	.get_direction = sama5d2_piobu_get_direction,
+	.direction_input = sama5d2_piobu_direction_input,
+	.direction_output = sama5d2_piobu_direction_output,
+	.get = sama5d2_piobu_get,
+	.set = sama5d2_piobu_set,
+	.base = -1,
+	.ngpio = PIOBU_NUM,
+	.can_sleep = 0,
+
+};
+
+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 = sama5d2_piobu_template_chip;
+	piobu->chip.label = pdev->name;
+	piobu->chip.parent = &pdev->dev;
+	piobu->chip.of_node = pdev->dev.of_node;
+
+	piobu->regmap =
+		syscon_regmap_lookup_by_compatible("microchip,sama5d2-piobu");
+	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 3/3] gpio: add driver for sama5d2 PIOBU pins

[Linus Walleij]

Hi Andrei,

thanks for your patch!

On Wed, Nov 7, 2018 at 4:12 PM <Andrei.Stefanescu@microchip.com> wrote:

> PIOBU pins do not lose their voltage during Backup/Self-refresh.
> This patch adds a simple GPIO controller for them.
>
> 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>

(...)
> +config GPIO_SAMA5D2_PIOBU
> +       tristate "SAMA5D2 PIOBU GPIO support"
> +       depends on GPIO_SYSCON

Should it not be:
depends on MFD_SYSCON
select GPIO_SYSCON

> +#include <linux/gpio.h>

Drivers should only
#include <linux/gpio/driver.h>

Also add:
#include <linux/bits.h>
because you use BIT() macros.

> +#define PIOBU_NUM 8
> +#define PIOBU_REG_SIZE 4

Isnt register size 4 implied by syscon, I think it is hardwired
to 32 bits.

> +/*
> + * sama5d2_piobu_setup_pin - prepares a pin for sama5d2_piobu_set_direction call
> + *
> + * Do not consider pin for intrusion detection (normal and backup modes)
> + * Do not consider pin as intrusion wakeup interrupt source

Intrusion ... sounds like some burglar alarm :D

> +/*
> + * sama5d2_piobu_get_direction - gpiochip get_direction
> + */

Check all your kerneldoc comments please,

/**
 * this_is_a_function() - This is a function
 */

See
Documentation/doc-guide/kernel-doc.rst

> +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) ? GPIOF_DIR_IN : GPIOF_DIR_OUT;

Please do not use these flags in drivers: they are for consumers.
Just return 0/1.

> +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 == GPIOF_DIR_IN)
> +               ret = sama5d2_piobu_read_value(chip, pin, PIOBU_PDS);
> +       else if (ret == GPIOF_DIR_OUT)
> +               ret = sama5d2_piobu_read_value(chip, pin, PIOBU_SOD);

Dito.

> +static void sama5d2_piobu_set(struct gpio_chip *chip, unsigned int pin,
> +                             int value)
> +{
> +       value = (value == 0) ? PIOBU_LOW : PIOBU_HIGH;

That looks unorthodox. Just use an if() statement.

> +const struct gpio_chip sama5d2_piobu_template_chip = {
> +       .owner = THIS_MODULE,
> +       .get_direction = sama5d2_piobu_get_direction,
> +       .direction_input = sama5d2_piobu_direction_input,
> +       .direction_output = sama5d2_piobu_direction_output,
> +       .get = sama5d2_piobu_get,
> +       .set = sama5d2_piobu_set,
> +       .base = -1,
> +       .ngpio = PIOBU_NUM,
> +       .can_sleep = 0,
(...)
> +       piobu->chip = sama5d2_piobu_template_chip;
> +       piobu->chip.label = pdev->name;
> +       piobu->chip.parent = &pdev->dev;
> +       piobu->chip.of_node = pdev->dev.of_node;

This is just overcomplicating things. Instead of assigning the template
just assign all function here in the probe() code, get rid of the
templaye. It's easier to follow.

> +       piobu->regmap =
> +               syscon_regmap_lookup_by_compatible("microchip,sama5d2-piobu");

So maybe just put this inside your syscon node and use:
syscon_node_to_regmap(pdev->dev.parent->of_node);

I might have more comments on the next version, but keep at it!

Yours,
Linus Walleij