Skip to contentSkip to conversation

add sama5d2 PIOBU GPIO driver

PATCH Superseded [email protected] 3 participants 8 messages Nov 7, 2018 – Nov 15, 2018

Subsystems (3)
Device tree bindings GPIO Pin controller - Microchip AT91

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover add sama5d2 PIOBU GPIO driver

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
Signature · 1 line
2.7.4
author
#1 lore raw

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

This patch describes the compatible and the device tree bindings necessary for the sama5d2 PIOBU GPIO controller driver.

Signed-off-by Andrei Stefanescu <[email protected]>
---
 .../bindings/gpio/gpio-sama5d2-piobu.txt           | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +23 -0
new file mode 100644
index 0000000..791ac51
@@ -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 at fc040000 {
+		compatible = "syscon", "microchip,sama5d2-piobu";
+		reg = <0xfc040000 0x100>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
Signature · 1 line
2.7.4
maintainer
#2 lore raw

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

Quoted 3 lines · Andrei.Stefanescu at microchip.com

This patch describes the compatible and the device tree bindings necessary for the sama5d2 PIOBU GPIO controller driver.

Bindings describe the h/w, not drivers.

Quoted 13 lines
Signed-off-by Andrei Stefanescu <[email protected]>
---
 .../bindings/gpio/gpio-sama5d2-piobu.txt           | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +0 -0
new file mode 100644
index 0000000..791ac51

Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt line 9

Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +9 -0
@@ -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"

Why the syscon?

syscon should be last if it remains.

Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt line 13

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +4 -0
+- #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.

No interrupt capability?

Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt line 17

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +4 -0
+
+Example:
+
+	secumod at fc040000 {

gpio at ...

Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +6 -0
+		compatible = "syscon", "microchip,sama5d2-piobu";
+		reg = <0xfc040000 0x100>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
Signature · 1 line
2.7.4
#3 lore raw

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

In reply to [email protected]
Quoted 2 lines · On Wed, Nov 7, 2018 at 4:12 PM <[email protected]> wrote:

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

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

Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt line 23

Quoted 7 lines
Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file +7 -0
+       secumod at 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

author
#4 lore raw

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

In reply to [email protected]

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

Signed-off-by Andrei Stefanescu <[email protected]>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
index f485597..88369f1 100644
@@ -9760,6 +9760,13 @@ M:	Nicolas Ferre <[email protected]>
 S:	Supported
 F:	drivers/power/reset/at91-sama5d2_shdwc.c
 
+MICROCHIP SAMA5D2-COMPATIBLE PIOBU GPIO
+M:	Andrei Stefanescu <[email protected]>
+L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
+L:	linux-gpio at 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 <[email protected]>
 S:	Supported
Signature · 1 line
2.7.4
author
#6 lore raw

PATCH 3/3 gpio: add driver for sama5d2 PIOBU pins

In reply to [email protected]

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 <[email protected]>
---
 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
drivers/gpio/Kconfig +10 -0
index 833a1b5..e562e32 100644
@@ -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
drivers/gpio/Makefile +1 -0
index 671c447..f18d345 100644
@@ -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
drivers/gpio/gpio-sama5d2-piobu.c new file +254 -0

254 lines not on this page · raw message

Signature · 1 line
2.7.4
#7 lore raw

Re: [PATCH 3/3] gpio: add driver for sama5d2 PIOBU pins

Hi Andrei,

thanks for your patch!

Quoted 7 lines · On Wed, Nov 7, 2018 at 4:12 PM <[email protected]> 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 <[email protected]>

(...)

drivers/gpio/Kconfig config GPIO_REG · line 434

Quoted 3 lines
drivers/gpio/Kconfig +3 -0
+config GPIO_SAMA5D2_PIOBU
+       tristate "SAMA5D2 PIOBU GPIO support"
+       depends on GPIO_SYSCON

Should it not be:
depends on MFD_SYSCON
select GPIO_SYSCON

Quoted 1 line

+#include <linux/gpio.h>

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

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

drivers/gpio/gpio-sama5d2-piobu.c line 20

Quoted 2 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +2 -0
+#define PIOBU_NUM 8
+#define PIOBU_REG_SIZE 4

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

Quoted 5 lines

+/*
+ * 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

Quoted 3 lines

+/*
+ * 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

drivers/gpio/gpio-sama5d2-piobu.c line 131

Quoted 9 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +9 -0
+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.

drivers/gpio/gpio-sama5d2-piobu.c line 163

Quoted 9 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +9 -0
+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.

drivers/gpio/gpio-sama5d2-piobu.c line 177

Quoted 4 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +4 -0
+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.

Quoted 10 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +10 -0
+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,

(...)

drivers/gpio/gpio-sama5d2-piobu.c line 207

Quoted 4 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +4 -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.

drivers/gpio/gpio-sama5d2-piobu.c line 210

Quoted 2 lines
drivers/gpio/gpio-sama5d2-piobu.c new file +2 -0
+       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