[PATCH v5 11/13] dt-bindings: arm: airoha: Add binding for Airoha GPIO controller

[PATCH v5 11/13] dt-bindings: arm: airoha: Add binding for Airoha GPIO controller

[Felix Fietkau]

From: John Crispin <john@phrozen.org>

Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
GPIOs

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 .../bindings/gpio/airoha,en7523-gpio.yaml     | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/airoha,en7523-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/airoha,en7523-gpio.yaml b/Documentation/devicetree/bindings/gpio/airoha,en7523-gpio.yaml
new file mode 100644
index 000000000000..66c00ec85731
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/airoha,en7523-gpio.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/airoha,en7523-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7523 GPIO controller
+
+maintainers:
+  - John Crispin <john@phrozen.org>
+
+description: |
+  Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
+  GPIOs.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - const: airoha,en7523-gpio
+
+  reg:
+    description: |
+      The first tuple points to the input register.
+      The second and third tuple point to the direction registers
+      The fourth tuple points to the output register
+    maxItems: 4
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio@1fbf0200 {
+        compatible = "airoha,en7523-gpio";
+        reg = <0x1fbf0204 0x4>,
+              <0x1fbf0200 0x4>,
+              <0x1fbf0220 0x4>,
+              <0x1fbf0214 0x4>;
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
+
+    gpio1: gpio@1fbf0270 {
+        compatible = "airoha,en7523-gpio";
+        reg = <0x1fbf0270 0x4>,
+              <0x1fbf0260 0x4>,
+              <0x1fbf0264 0x4>,
+              <0x1fbf0278 0x4>;
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
+
+
+...
-- 
2.30.1

[PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Felix Fietkau]

From: John Crispin <john@phrozen.org>

Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
GPIOs. Each instance in DT is for an single bank.

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/gpio/Kconfig       |   9 +++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-en7523.c | 136 +++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 drivers/gpio/gpio-en7523.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 072ed610f9c6..e4a34272504f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -247,6 +247,15 @@ config GPIO_EM
 	help
 	  Say yes here to support GPIO on Renesas Emma Mobile SoCs.
 
+config GPIO_EN7523
+	tristate "Airoha GPIO support"
+	depends on ARCH_AIROHA
+	default ARCH_AIROHA
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the GPIO controller on Airoha EN7523.
+
 config GPIO_EP93XX
 	def_bool y
 	depends on ARCH_EP93XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 71ee9fc2ff83..d2269ee0948e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_DLN2)			+= gpio-dln2.o
 obj-$(CONFIG_GPIO_DWAPB)		+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EIC_SPRD)		+= gpio-eic-sprd.o
 obj-$(CONFIG_GPIO_EM)			+= gpio-em.o
+obj-$(CONFIG_GPIO_EN7523)		+= gpio-en7523.o
 obj-$(CONFIG_GPIO_EP93XX)		+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_EXAR)			+= gpio-exar.o
 obj-$(CONFIG_GPIO_F7188X)		+= gpio-f7188x.o
diff --git a/drivers/gpio/gpio-en7523.c b/drivers/gpio/gpio-en7523.c
new file mode 100644
index 000000000000..0032f99e366d
--- /dev/null
+++ b/drivers/gpio/gpio-en7523.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/gpio/driver.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+#define AIROHA_GPIO_MAX		32
+
+/**
+ * airoha_gpio_ctrl - Airoha GPIO driver data
+ *
+ * @gc: Associated gpio_chip instance.
+ * @data: The data register.
+ * @dir0: The direction register for the lower 16 pins.
+ * @dir1: The direction register for the higher 16 pins.
+ * @output: The output enable register.
+ */
+struct airoha_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *data;
+	void __iomem *dir[2];
+	void __iomem *output;
+};
+
+static struct airoha_gpio_ctrl *gc_to_ctrl(struct gpio_chip *gc)
+{
+	return container_of(gc, struct airoha_gpio_ctrl, gc);
+}
+
+static int airoha_dir_set(struct gpio_chip *gc, unsigned int gpio,
+			  int val, int out)
+{
+	struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc);
+	u32 dir = ioread32(ctrl->dir[gpio / 16]);
+	u32 output = ioread32(ctrl->output);
+	u32 mask = BIT((gpio % 16) * 2);
+
+	if (out) {
+		dir |= mask;
+		output |= BIT(gpio);
+	} else {
+		dir &= ~mask;
+		output &= ~BIT(gpio);
+	}
+
+	iowrite32(dir, ctrl->dir[gpio / 16]);
+	iowrite32(output, ctrl->output);
+
+	if (out)
+		gc->set(gc, gpio, val);
+
+	return 0;
+}
+
+static int airoha_dir_out(struct gpio_chip *gc, unsigned int gpio,
+			  int val)
+{
+	return airoha_dir_set(gc, gpio, val, 1);
+}
+
+static int airoha_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	return airoha_dir_set(gc, gpio, 0, 0);
+}
+
+static int airoha_get_dir(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc);
+	u32 dir = ioread32(ctrl->dir[gpio / 16]);
+	u32 mask = BIT((gpio % 16) * 2);
+
+	return dir & mask;
+}
+
+static const struct of_device_id airoha_gpio_of_match[] = {
+	{ .compatible = "airoha,en7523-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, airoha_gpio_of_match);
+
+static int airoha_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct airoha_gpio_ctrl *ctrl;
+	int err;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->data = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->data))
+		return PTR_ERR(ctrl->data);
+
+	ctrl->dir[0] = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(ctrl->dir[0]))
+		return PTR_ERR(ctrl->dir[0]);
+
+	ctrl->dir[1] = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(ctrl->dir[1]))
+		return PTR_ERR(ctrl->dir[1]);
+
+	ctrl->output = devm_platform_ioremap_resource(pdev, 3);
+	if (IS_ERR(ctrl->output))
+		return PTR_ERR(ctrl->output);
+
+	err = bgpio_init(&ctrl->gc, dev, 4, ctrl->data, NULL,
+			 NULL, NULL, NULL, 0);
+	if (err) {
+		dev_err(dev, "unable to init generic GPIO");
+		return err;
+	}
+
+	ctrl->gc.ngpio = AIROHA_GPIO_MAX;
+	ctrl->gc.owner = THIS_MODULE;
+	ctrl->gc.direction_output = airoha_dir_out;
+	ctrl->gc.direction_input = airoha_dir_in;
+	ctrl->gc.get_direction = airoha_get_dir;
+
+	return devm_gpiochip_add_data(dev, &ctrl->gc, ctrl);
+}
+
+static struct platform_driver airoha_gpio_driver = {
+	.driver = {
+		.name = "airoha-gpio",
+		.of_match_table	= airoha_gpio_of_match,
+	},
+	.probe = airoha_gpio_probe,
+};
+module_platform_driver(airoha_gpio_driver);
+
+MODULE_DESCRIPTION("Airoha GPIO support");
+MODULE_AUTHOR("John Crispin <john@phrozen.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.30.1

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Linus Walleij]

Hi Felix!

Thanks for your patch!

On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote:

> From: John Crispin <john@phrozen.org>
>
> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
> GPIOs. Each instance in DT is for an single bank.
>
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

(...)
> +config GPIO_EN7523
> +       tristate "Airoha GPIO support"
> +       depends on ARCH_AIROHA
> +       default ARCH_AIROHA
> +       select GPIO_GENERIC

Yes that looks applicable, but why isn't it used?

The few 32-bit registers look like an ideal candidate for
using the generic GPIO. Check similar drivers such as
drivers/gpio/gpio-ftgpio010.c and how it uses
bgpio_init() and the nice doc for bgpio_init() in
drivers/gpio/gpio-mmio.c.

If it's not working already with generic GPIO I do not
think it would be far fetched to fix it.

Yours,
Linus Walleij

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Felix Fietkau]

On 2021-12-02 02:47, Linus Walleij wrote:
> Hi Felix!
> 
> Thanks for your patch!
> 
> On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote:
> 
>> From: John Crispin <john@phrozen.org>
>>
>> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
>> GPIOs. Each instance in DT is for an single bank.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> 
> (...)
>> +config GPIO_EN7523
>> +       tristate "Airoha GPIO support"
>> +       depends on ARCH_AIROHA
>> +       default ARCH_AIROHA
>> +       select GPIO_GENERIC
> 
> Yes that looks applicable, but why isn't it used?
> 
> The few 32-bit registers look like an ideal candidate for
> using the generic GPIO. Check similar drivers such as
> drivers/gpio/gpio-ftgpio010.c and how it uses
> bgpio_init() and the nice doc for bgpio_init() in
> drivers/gpio/gpio-mmio.c.
I just looked at the datasheet and the driver code again, and I think 
EN7523 is too strange for proper generic GPIO support.

For each bank there are two control registers (not consecutive), which 
have 2-bit fields for every GPIO line to control direction. No idea why 
2 bits per line, because only values 0 and 1 are valid, the rest are 
reserved.
For lines configured as output, an extra output-enable bit also needs to 
be set in a separate register before output values can be written.

The code does use bgpio to read/write values, but that's about it.
I don't think it would do the generic GPIO code any good to support this 
weirdness.

- Felix

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[John Crispin]

On 02.12.21 18:59, Felix Fietkau wrote:
> 
> On 2021-12-02 02:47, Linus Walleij wrote:
>> Hi Felix!
>>
>> Thanks for your patch!
>>
>> On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote:
>>
>>> From: John Crispin <john@phrozen.org>
>>>
>>> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two 
>>> banks of 32
>>> GPIOs. Each instance in DT is for an single bank.
>>>
>>> Signed-off-by: John Crispin <john@phrozen.org>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>
>> (...)
>>> +config GPIO_EN7523
>>> +       tristate "Airoha GPIO support"
>>> +       depends on ARCH_AIROHA
>>> +       default ARCH_AIROHA
>>> +       select GPIO_GENERIC
>>
>> Yes that looks applicable, but why isn't it used?
>>
>> The few 32-bit registers look like an ideal candidate for
>> using the generic GPIO. Check similar drivers such as
>> drivers/gpio/gpio-ftgpio010.c and how it uses
>> bgpio_init() and the nice doc for bgpio_init() in
>> drivers/gpio/gpio-mmio.c.
> I just looked at the datasheet and the driver code again, and I think 
> EN7523 is too strange for proper generic GPIO support.
> 
> For each bank there are two control registers (not consecutive), which 
> have 2-bit fields for every GPIO line to control direction. No idea why 
> 2 bits per line, because only values 0 and 1 are valid, the rest are 
> reserved.
> For lines configured as output, an extra output-enable bit also needs to 
> be set in a separate register before output values can be written.
> 
> The code does use bgpio to read/write values, but that's about it.
> I don't think it would do the generic GPIO code any good to support this 
> weirdness.
> 
> - Felix
Hi Linus,
I sent an email to you 16.06.21 explaining all of this and you replied, 
telling me that this approach is the most reasonable one to take.
	John

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Linus Walleij]

On Thu, Dec 2, 2021 at 8:02 PM John Crispin <john@phrozen.org> wrote:

> Hi Linus,
> I sent an email to you 16.06.21 explaining all of this and you replied,
> telling me that this approach is the most reasonable one to take.
>         John

Sadly I don't remember that, too long ago!

Anyways let's just go ahead with this.

Yours,
Linus Walleij

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Linus Walleij]

On Thu, Dec 2, 2021 at 6:59 PM Felix Fietkau <nbd@nbd.name> wrote:

> I just looked at the datasheet and the driver code again, and I think
> EN7523 is too strange for proper generic GPIO support.

Yup I see that John explained it to me in the past, no problem.
Let's do it like this then, but just drop select GPIO_GENERIC
from KConfig since it's not used.

With that oneliner change:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Felix Fietkau]

On 2021-12-05 00:57, Linus Walleij wrote:
> On Thu, Dec 2, 2021 at 6:59 PM Felix Fietkau <nbd@nbd.name> wrote:
> 
>> I just looked at the datasheet and the driver code again, and I think
>> EN7523 is too strange for proper generic GPIO support.
> 
> Yup I see that John explained it to me in the past, no problem.
> Let's do it like this then, but just drop select GPIO_GENERIC
> from KConfig since it's not used.
> 
> With that oneliner change:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij
The generic GPIO API is still used for the data register. The driver 
calls bgpio_init(), so you suggested oneliner change won't work.

- Felix

Re: [PATCH v5 12/13] gpio: Add support for Airoha EN7523 GPIO controller

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 1:08 AM Felix Fietkau <nbd@nbd.name> wrote:
>
> From: John Crispin <john@phrozen.org>
>
> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32
> GPIOs. Each instance in DT is for an single bank.

a single

...

> +/**
> + * airoha_gpio_ctrl - Airoha GPIO driver data

> + *

Unnecessary blank line.

> + * @gc: Associated gpio_chip instance.
> + * @data: The data register.
> + * @dir0: The direction register for the lower 16 pins.
> + * @dir1: The direction register for the higher 16 pins.
> + * @output: The output enable register.
> + */

...

> +static int airoha_dir_set(struct gpio_chip *gc, unsigned int gpio,
> +                         int val, int out)
> +{
> +       struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc);
> +       u32 dir = ioread32(ctrl->dir[gpio / 16]);
> +       u32 output = ioread32(ctrl->output);
> +       u32 mask = BIT((gpio % 16) * 2);
> +
> +       if (out) {
> +               dir |= mask;
> +               output |= BIT(gpio);
> +       } else {
> +               dir &= ~mask;
> +               output &= ~BIT(gpio);
> +       }
> +
> +       iowrite32(dir, ctrl->dir[gpio / 16]);
> +       iowrite32(output, ctrl->output);

> +       if (out)
> +               gc->set(gc, gpio, val);

Needs a fix or a comment to explain why it's fine that there is a
glitch possible.

> +       return 0;
> +}

...

> +       err = bgpio_init(&ctrl->gc, dev, 4, ctrl->data, NULL,
> +                        NULL, NULL, NULL, 0);
> +       if (err) {

> +               dev_err(dev, "unable to init generic GPIO");
> +               return err;

return dev_err_probe(...);

> +       }

-- 
With Best Regards,
Andy Shevchenko