[PATCH v4 1/2] gpio: loongson: add dts and acpi support

[PATCH v4 1/2] gpio: loongson: add dts and acpi support

[Yinbo Zhu]

Latest Loongson platforms such as the Loongson-2 SoC series describe
GPIO device resources with DTS or ACPI. Add such support to the
existing platform device driver.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v4:
		1. Fixup name spelling about Signed-off-by. 
		2. Drop "series" here and everywhere else.
		3. Fixup the copyright in driver.
		4. Drop the "else" in loongson_gpio_request.
		5. Use trinocular operation replace the related logic.
		6. Remove lable judgement in context about "lgpio->chip.to_irq = 
		   loongson_gpio_to_irq"
		7. Use dev_err replace pr_err in probe.
		8. Make legacy platform_data should be left out of this patch.
		9. Remove the mips config in gpio Kconfig.
Change in v3:
		1. Move the gpio platform data struct from arch/ into include/linux/
		   platform_data/.
		2. Replace platform_gpio_data with loongson_gpio_platform_data in .c.
		3. Add maintainer in MAINTAINERS file for include/linux/platform_data/
		   gpio-loongson.h and gpio-loongson.c
Change in v2:
		1. Fixup of_loongson_gpio_get_props and remove the parse logic about
	           "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
		   "loongson,gpio_base", "loongson,support_irq" then kernel driver will
		   initial them that depend compatible except "loongson,gpio_base".

 MAINTAINERS                  |   8 +
 drivers/gpio/Kconfig         |   6 +-
 drivers/gpio/gpio-loongson.c | 403 ++++++++++++++++++++++++++++-------
 3 files changed, 338 insertions(+), 79 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5114db9c8f32..838b920efcef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12051,6 +12051,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
 F:	drivers/soc/loongson/loongson2_guts.c
 
+LOONGSON GPIO DRIVER
+M:	Huacai Chen <chenhuacai@kernel.org>
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	drivers/gpio/gpio-loongson.c
+F:	include/linux/platform_data/gpio-loongson.h
+
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
 M:	Sathya Prakash <sathya.prakash@broadcom.com>
 M:	Sreekanth Reddy <sreekanth.reddy@broadcom.com>
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a01af1180616..b4423c058b1d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -376,10 +376,10 @@ config GPIO_LOGICVC
 	  programmable logic block.
 
 config GPIO_LOONGSON
-	bool "Loongson-2/3 GPIO support"
-	depends on CPU_LOONGSON2EF || CPU_LOONGSON64
+	bool "Loongson series GPIO support"
+	depends on LOONGARCH || COMPILE_TEST
 	help
-	  Driver for GPIO functionality on Loongson-2F/3A/3B processors.
+	  Driver for GPIO functionality on Loongson seires processors.
 
 config GPIO_LPC18XX
 	tristate "NXP LPC18XX/43XX GPIO support"
diff --git a/drivers/gpio/gpio-loongson.c b/drivers/gpio/gpio-loongson.c
index a42145873cc9..b9b191a541c0 100644
--- a/drivers/gpio/gpio-loongson.c
+++ b/drivers/gpio/gpio-loongson.c
@@ -1,13 +1,11 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-License-Identifier: GPL-2.0+
 /*
- *  Loongson-2F/3A/3B GPIO Support
+ * Loongson GPIO Support
  *
- *  Copyright (c) 2008 Richard Liu,  STMicroelectronics	 <richard.liu@st.com>
- *  Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com>
- *  Copyright (c) 2013 Hongbing Hu <huhb@lemote.com>
- *  Copyright (c) 2014 Huacai Chen <chenhc@lemote.com>
+ * Copyright (C) 2022 Loongson Technology Corporation Limited
  */
 
+#include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -17,119 +15,372 @@
 #include <linux/platform_device.h>
 #include <linux/bitops.h>
 #include <asm/types.h>
-#include <loongson.h>
+#include <asm/loongson.h>
 
-#define STLS2F_N_GPIO		4
-#define STLS3A_N_GPIO		16
+#define LOONGSON_GPIO_IN(x)		(x->base + x->in_offset)
+#define LOONGSON_GPIO_OUT(x)		(x->base + x->out_offset)
+#define LOONGSON_GPIO_OEN(x)		(x->base + x->conf_offset)
 
-#ifdef CONFIG_CPU_LOONGSON64
-#define LOONGSON_N_GPIO	STLS3A_N_GPIO
-#else
-#define LOONGSON_N_GPIO	STLS2F_N_GPIO
-#endif
+#define LOONGSON_GPIO_IN_BYTE(x, gpio)	(x->base +\
+					x->in_offset + gpio)
+#define LOONGSON_GPIO_OUT_BYTE(x, gpio)	(x->base +\
+					x->out_offset + gpio)
+#define LOONGSON_GPIO_OEN_BYTE(x, gpio)	(x->base +\
+					x->conf_offset + gpio)
 
-/*
- * Offset into the register where we read lines, we write them from offset 0.
- * This offset is the only thing that stand between us and using
- * GPIO_GENERIC.
- */
-#define LOONGSON_GPIO_IN_OFFSET	16
+struct loongson_gpio_chip {
+	struct gpio_chip	chip;
+	spinlock_t		lock;
+	void __iomem		*base;
+	int			conf_offset;
+	int			out_offset;
+	int			in_offset;
+	u16			*gsi_idx_map;
+	u16			mapsize;
+};
 
-static DEFINE_SPINLOCK(gpio_lock);
+static int loongson_gpio_request(
+			struct gpio_chip *chip, unsigned int pin)
+{
+	if (pin >= chip->ngpio)
+		return -EINVAL;
+
+	return 0;
+}
 
-static int loongson_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
+static inline void __set_direction(struct loongson_gpio_chip *lgpio,
+			unsigned int pin, int input)
 {
-	u32 val;
+	u64 qval;
+	u8  bval;
+
+	if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
+		qval = readq(LOONGSON_GPIO_OEN(lgpio));
+		if (input)
+			qval |= 1ULL << pin;
+		else
+			qval &= ~(1ULL << pin);
+		writeq(qval, LOONGSON_GPIO_OEN(lgpio));
+		return;
+	}
 
-	spin_lock(&gpio_lock);
-	val = LOONGSON_GPIODATA;
-	spin_unlock(&gpio_lock);
+	if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
+			!strncmp(lgpio->chip.label, "LOON0002", 8)) {
+		bval = input ? 1 : 0;
+		writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
+		return;
+	}
 
-	return !!(val & BIT(gpio + LOONGSON_GPIO_IN_OFFSET));
+	if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
+		if (input)
+			LOONGSON_GPIOIE |= BIT(pin);
+		else
+			LOONGSON_GPIOIE &= ~BIT(pin);
+		return;
+	}
 }
 
-static void loongson_gpio_set_value(struct gpio_chip *chip,
-		unsigned gpio, int value)
+static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
+			int high)
 {
-	u32 val;
+	u64 qval;
+	u8 bval;
 
-	spin_lock(&gpio_lock);
-	val = LOONGSON_GPIODATA;
-	if (value)
-		val |= BIT(gpio);
-	else
-		val &= ~BIT(gpio);
-	LOONGSON_GPIODATA = val;
-	spin_unlock(&gpio_lock);
+	if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
+		qval = readq(LOONGSON_GPIO_OUT(lgpio));
+		if (high)
+			qval |= 1ULL << pin;
+		else
+			qval &= ~(1ULL << pin);
+		writeq(qval, LOONGSON_GPIO_OUT(lgpio));
+		return;
+	}
+
+	if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
+			!strncmp(lgpio->chip.label, "LOON0002", 8)) {
+		bval = high ? 1 : 0;
+		writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
+		return;
+	}
+
+	if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
+		if (LOONGSON_GPIODATA)
+			LOONGSON_GPIODATA |= BIT(pin);
+		else
+			LOONGSON_GPIODATA &= ~BIT(pin);
+		return;
+	}
 }
 
-static int loongson_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+static int loongson_gpio_direction_input(
+				struct gpio_chip *chip, unsigned int pin)
 {
-	u32 temp;
+	unsigned long flags;
+	struct loongson_gpio_chip *lgpio =
+		container_of(chip, struct loongson_gpio_chip, chip);
 
-	spin_lock(&gpio_lock);
-	temp = LOONGSON_GPIOIE;
-	temp |= BIT(gpio);
-	LOONGSON_GPIOIE = temp;
-	spin_unlock(&gpio_lock);
+	spin_lock_irqsave(&lgpio->lock, flags);
+	__set_direction(lgpio, pin, 1);
+	spin_unlock_irqrestore(&lgpio->lock, flags);
 
 	return 0;
 }
 
-static int loongson_gpio_direction_output(struct gpio_chip *chip,
-		unsigned gpio, int level)
+static int loongson_gpio_direction_output(
+				struct gpio_chip *chip, unsigned int pin,
+				int value)
 {
-	u32 temp;
+	struct loongson_gpio_chip *lgpio =
+		container_of(chip, struct loongson_gpio_chip, chip);
+	unsigned long flags;
 
-	loongson_gpio_set_value(chip, gpio, level);
-	spin_lock(&gpio_lock);
-	temp = LOONGSON_GPIOIE;
-	temp &= ~BIT(gpio);
-	LOONGSON_GPIOIE = temp;
-	spin_unlock(&gpio_lock);
+	spin_lock_irqsave(&lgpio->lock, flags);
+	__set_level(lgpio, pin, value);
+	__set_direction(lgpio, pin, 0);
+	spin_unlock_irqrestore(&lgpio->lock, flags);
 
 	return 0;
 }
 
+static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
+{
+	u64 qval;
+	u8  bval;
+	int val;
+	struct loongson_gpio_chip *lgpio =
+		container_of(chip, struct loongson_gpio_chip, chip);
+
+	if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
+		qval = readq(LOONGSON_GPIO_IN(lgpio));
+		return ((qval & (1ULL << pin)) != 0);
+	}
+
+	if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
+			!strncmp(lgpio->chip.label, "LOON0002", 8)) {
+		bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
+		return (bval & 1);
+	}
+
+	if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
+		val = LOONGSON_GPIODATA;
+		return !!(val & BIT(pin + lgpio->in_offset));
+	}
+
+	return -ENXIO;
+}
+
+static void loongson_gpio_set(struct gpio_chip *chip, unsigned int pin,
+			int value)
+{
+	unsigned long flags;
+	struct loongson_gpio_chip *lgpio =
+		container_of(chip, struct loongson_gpio_chip, chip);
+
+	spin_lock_irqsave(&lgpio->lock, flags);
+	__set_level(lgpio, pin, value);
+	spin_unlock_irqrestore(&lgpio->lock, flags);
+}
+
+static int loongson_gpio_to_irq(
+			struct gpio_chip *chip, unsigned int offset)
+{
+	struct platform_device *pdev =
+		container_of(chip->parent, struct platform_device, dev);
+	struct loongson_gpio_chip *lgpio =
+		container_of(chip, struct loongson_gpio_chip, chip);
+
+	if (offset >= chip->ngpio)
+		return -EINVAL;
+
+	if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
+		offset = lgpio->gsi_idx_map[offset];
+
+	return platform_get_irq(pdev, offset);
+}
+
+static int loongson_gpio_init(
+			struct device *dev, struct loongson_gpio_chip *lgpio,
+			struct device_node *np, void __iomem *base)
+{
+	lgpio->chip.request = loongson_gpio_request;
+	lgpio->chip.direction_input = loongson_gpio_direction_input;
+	lgpio->chip.get = loongson_gpio_get;
+	lgpio->chip.direction_output = loongson_gpio_direction_output;
+	lgpio->chip.set = loongson_gpio_set;
+	lgpio->chip.can_sleep = 0;
+	lgpio->chip.of_node = np;
+	lgpio->chip.parent = dev;
+	spin_lock_init(&lgpio->lock);
+	lgpio->base = (void __iomem *)base;
+	lgpio->chip.to_irq = loongson_gpio_to_irq;
+
+	gpiochip_add(&lgpio->chip);
+
+	return 0;
+}
+
+static void of_loongson_gpio_get_props(struct device_node *np,
+				  struct loongson_gpio_chip *lgpio)
+{
+	const char *name;
+
+	of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio);
+
+	of_property_read_string(np, "compatible", &name);
+	lgpio->chip.label = kstrdup(name, GFP_KERNEL);
+
+	if (!strcmp(name, "loongson,ls2k-gpio")) {
+		lgpio->conf_offset = 0x0;
+		lgpio->out_offset = 0x10;
+		lgpio->in_offset = 0x20;
+		return;
+	}
+
+	if (!strcmp(name, "loongson,ls7a-gpio")) {
+		lgpio->conf_offset = 0x800;
+		lgpio->out_offset = 0x900;
+		lgpio->in_offset = 0xa00;
+		return;
+	}
+}
+
+static void acpi_loongson_gpio_get_props(struct platform_device *pdev,
+				  struct loongson_gpio_chip *lgpio)
+{
+
+	struct device *dev = &pdev->dev;
+	int rval;
+
+	device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio);
+	device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base);
+	device_property_read_u32(dev, "conf_offset",
+					(u32 *)&lgpio->conf_offset);
+	device_property_read_u32(dev, "out_offset",
+					(u32 *)&lgpio->out_offset);
+	device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset);
+
+	rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0);
+	if (rval > 0) {
+		lgpio->gsi_idx_map =
+			kmalloc_array(rval, sizeof(*lgpio->gsi_idx_map),
+					GFP_KERNEL);
+		if (unlikely(!lgpio->gsi_idx_map)) {
+			dev_err(dev, "Alloc gsi_idx_map fail!\n");
+		} else {
+			lgpio->mapsize = rval;
+			device_property_read_u16_array(dev, "gsi_idx_map",
+					lgpio->gsi_idx_map, lgpio->mapsize);
+		}
+	}
+
+	lgpio->chip.label = kstrdup(pdev->name, GFP_KERNEL);
+}
+
+static void platform_loongson_gpio_get_props(struct platform_device *pdev,
+				  struct loongson_gpio_chip *lgpio)
+{
+}
+
 static int loongson_gpio_probe(struct platform_device *pdev)
 {
-	struct gpio_chip *gc;
+	struct resource *iores;
+	void __iomem *base;
+	struct loongson_gpio_chip *lgpio;
+	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
+	int ret = 0;
 
-	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
-	if (!gc)
+	lgpio = kzalloc(sizeof(struct loongson_gpio_chip), GFP_KERNEL);
+	if (!lgpio)
 		return -ENOMEM;
 
-	gc->label = "loongson-gpio-chip";
-	gc->base = 0;
-	gc->ngpio = LOONGSON_N_GPIO;
-	gc->get = loongson_gpio_get_value;
-	gc->set = loongson_gpio_set_value;
-	gc->direction_input = loongson_gpio_direction_input;
-	gc->direction_output = loongson_gpio_direction_output;
+	if (np)
+		of_loongson_gpio_get_props(np, lgpio);
+	else if (ACPI_COMPANION(&pdev->dev))
+		acpi_loongson_gpio_get_props(pdev, lgpio);
+	else
+		platform_loongson_gpio_get_props(pdev, lgpio);
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iores) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (!request_mem_region(iores->start, resource_size(iores),
+				pdev->name)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	base = ioremap(iores->start, resource_size(iores));
+	if (!base) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, lgpio);
+
+	loongson_gpio_init(dev, lgpio, np, base);
+
+	return 0;
+out:
+	dev_err(dev, "missing mandatory property\n");
+
+	return ret;
+}
+
+static int loongson_gpio_remove(struct platform_device *pdev)
+{
+	struct loongson_gpio_chip *lgpio = platform_get_drvdata(pdev);
+	struct resource		*mem;
 
-	return gpiochip_add_data(gc, NULL);
+	platform_set_drvdata(pdev, NULL);
+
+	gpiochip_remove(&lgpio->chip);
+	iounmap(lgpio->base);
+	kfree(lgpio->gsi_idx_map);
+	kfree(lgpio);
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(mem->start, resource_size(mem));
+
+	return 0;
 }
 
+static const struct of_device_id loongson_gpio_dt_ids[] = {
+	{ .compatible = "loongson,ls2k-gpio"},
+	{ .compatible = "loongson,ls7a-gpio"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids);
+
+static const struct acpi_device_id loongson_gpio_acpi_match[] = {
+	{"LOON0002"},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
+
 static struct platform_driver loongson_gpio_driver = {
 	.driver = {
 		.name = "loongson-gpio",
+		.owner = THIS_MODULE,
+		.of_match_table = loongson_gpio_dt_ids,
+		.acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match),
 	},
 	.probe = loongson_gpio_probe,
+	.remove = loongson_gpio_remove,
 };
 
 static int __init loongson_gpio_setup(void)
 {
-	struct platform_device *pdev;
-	int ret;
-
-	ret = platform_driver_register(&loongson_gpio_driver);
-	if (ret) {
-		pr_err("error registering loongson GPIO driver\n");
-		return ret;
-	}
-
-	pdev = platform_device_register_simple("loongson-gpio", -1, NULL, 0);
-	return PTR_ERR_OR_ZERO(pdev);
+	return platform_driver_register(&loongson_gpio_driver);
 }
 postcore_initcall(loongson_gpio_setup);
+
+static void __exit loongson_gpio_exit(void)
+{
+	platform_driver_unregister(&loongson_gpio_driver);
+}
+
+MODULE_DESCRIPTION("Loongson gpio driver");
+MODULE_LICENSE("GPL");
-- 
2.31.1

[PATCH v4 2/2] dt-bindings: gpio: add loongson gpio

[Yinbo Zhu]

Add the Loongson platform gpio binding with DT schema format using
json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Change in v4:
		1. Remove the string "series".
		2. Add the reviewed-by information.
Change in v3:
		1. Separate some changes of MAINTAINERS file and enter the first patch.
Change in v2:
		1. Drop "loongson,gpio_base" and "gpio-ranges" will cover it.
		1. Drop "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
		   "loongson,support_irq" and kernel driver will initial them that depend
		   compatible in kernel.
		3. Fixup maintainer for this driver.

 .../bindings/gpio/loongson,ls-gpio.yaml       | 126 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
new file mode 100644
index 000000000000..fb86e8ce6349
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
@@ -0,0 +1,126 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/loongson,ls-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson GPIO controller.
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls2k-gpio
+      - loongson,ls7a-gpio
+
+  reg:
+    maxItems: 1
+
+  ngpios:
+    minimum: 1
+    maximum: 64
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  gpio-ranges: true
+
+  interrupts:
+    minItems: 1
+    maxItems: 64
+
+required:
+  - compatible
+  - reg
+  - ngpios
+  - "#gpio-cells"
+  - gpio-controller
+  - gpio-ranges
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gpio0: gpio@1fe00500 {
+      compatible = "loongson,ls2k-gpio";
+      reg = <0x1fe00500 0x38>;
+      ngpios = <64>;
+      #gpio-cells = <2>;
+      gpio-controller;
+      gpio-ranges = <&pctrl 0 0 15>,
+                    <&pctrl 16 16 15>,
+                    <&pctrl 32 32 10>,
+                    <&pctrl 44 44 20>;
+      interrupt-parent = <&liointc1>;
+      interrupts = <28 IRQ_TYPE_LEVEL_LOW>,
+                   <29 IRQ_TYPE_LEVEL_LOW>,
+                   <30 IRQ_TYPE_LEVEL_LOW>,
+                   <30 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <26 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <>,
+                   <>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>,
+                   <27 IRQ_TYPE_LEVEL_LOW>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 838b920efcef..62c5499e4159 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12056,6 +12056,7 @@ M:	Huacai Chen <chenhuacai@kernel.org>
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-gpio@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
 F:	drivers/gpio/gpio-loongson.c
 F:	include/linux/platform_data/gpio-loongson.h
 
-- 
2.31.1

Re: [PATCH v4 1/2] gpio: loongson: add dts and acpi support

[Bartosz Golaszewski]

On Thu, Nov 17, 2022 at 4:59 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>
> Latest Loongson platforms such as the Loongson-2 SoC series describe
> GPIO device resources with DTS or ACPI. Add such support to the
> existing platform device driver.
>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v4:
>                 1. Fixup name spelling about Signed-off-by.
>                 2. Drop "series" here and everywhere else.
>                 3. Fixup the copyright in driver.
>                 4. Drop the "else" in loongson_gpio_request.
>                 5. Use trinocular operation replace the related logic.
>                 6. Remove lable judgement in context about "lgpio->chip.to_irq =
>                    loongson_gpio_to_irq"
>                 7. Use dev_err replace pr_err in probe.
>                 8. Make legacy platform_data should be left out of this patch.
>                 9. Remove the mips config in gpio Kconfig.
> Change in v3:
>                 1. Move the gpio platform data struct from arch/ into include/linux/
>                    platform_data/.
>                 2. Replace platform_gpio_data with loongson_gpio_platform_data in .c.
>                 3. Add maintainer in MAINTAINERS file for include/linux/platform_data/
>                    gpio-loongson.h and gpio-loongson.c
> Change in v2:
>                 1. Fixup of_loongson_gpio_get_props and remove the parse logic about
>                    "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
>                    "loongson,gpio_base", "loongson,support_irq" then kernel driver will
>                    initial them that depend compatible except "loongson,gpio_base".
>
>  MAINTAINERS                  |   8 +
>  drivers/gpio/Kconfig         |   6 +-
>  drivers/gpio/gpio-loongson.c | 403 ++++++++++++++++++++++++++++-------
>  3 files changed, 338 insertions(+), 79 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5114db9c8f32..838b920efcef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12051,6 +12051,14 @@ S:     Maintained
>  F:     Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
>  F:     drivers/soc/loongson/loongson2_guts.c
>
> +LOONGSON GPIO DRIVER
> +M:     Huacai Chen <chenhuacai@kernel.org>
> +M:     Yinbo Zhu <zhuyinbo@loongson.cn>
> +L:     linux-gpio@vger.kernel.org
> +S:     Maintained
> +F:     drivers/gpio/gpio-loongson.c
> +F:     include/linux/platform_data/gpio-loongson.h
> +
>  LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
>  M:     Sathya Prakash <sathya.prakash@broadcom.com>
>  M:     Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a01af1180616..b4423c058b1d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -376,10 +376,10 @@ config GPIO_LOGICVC
>           programmable logic block.
>
>  config GPIO_LOONGSON
> -       bool "Loongson-2/3 GPIO support"
> -       depends on CPU_LOONGSON2EF || CPU_LOONGSON64
> +       bool "Loongson series GPIO support"
> +       depends on LOONGARCH || COMPILE_TEST
>         help
> -         Driver for GPIO functionality on Loongson-2F/3A/3B processors.
> +         Driver for GPIO functionality on Loongson seires processors.
>
>  config GPIO_LPC18XX
>         tristate "NXP LPC18XX/43XX GPIO support"
> diff --git a/drivers/gpio/gpio-loongson.c b/drivers/gpio/gpio-loongson.c
> index a42145873cc9..b9b191a541c0 100644
> --- a/drivers/gpio/gpio-loongson.c
> +++ b/drivers/gpio/gpio-loongson.c
> @@ -1,13 +1,11 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
> - *  Loongson-2F/3A/3B GPIO Support
> + * Loongson GPIO Support
>   *
> - *  Copyright (c) 2008 Richard Liu,  STMicroelectronics         <richard.liu@st.com>
> - *  Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com>
> - *  Copyright (c) 2013 Hongbing Hu <huhb@lemote.com>
> - *  Copyright (c) 2014 Huacai Chen <chenhc@lemote.com>

Why are you removing other people's copyright notices?

> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
> @@ -17,119 +15,372 @@
>  #include <linux/platform_device.h>
>  #include <linux/bitops.h>
>  #include <asm/types.h>
> -#include <loongson.h>
> +#include <asm/loongson.h>
>
> -#define STLS2F_N_GPIO          4
> -#define STLS3A_N_GPIO          16
> +#define LOONGSON_GPIO_IN(x)            (x->base + x->in_offset)
> +#define LOONGSON_GPIO_OUT(x)           (x->base + x->out_offset)
> +#define LOONGSON_GPIO_OEN(x)           (x->base + x->conf_offset)
>
> -#ifdef CONFIG_CPU_LOONGSON64
> -#define LOONGSON_N_GPIO        STLS3A_N_GPIO
> -#else
> -#define LOONGSON_N_GPIO        STLS2F_N_GPIO
> -#endif
> +#define LOONGSON_GPIO_IN_BYTE(x, gpio) (x->base +\
> +                                       x->in_offset + gpio)
> +#define LOONGSON_GPIO_OUT_BYTE(x, gpio)        (x->base +\
> +                                       x->out_offset + gpio)
> +#define LOONGSON_GPIO_OEN_BYTE(x, gpio)        (x->base +\
> +                                       x->conf_offset + gpio)
>
> -/*
> - * Offset into the register where we read lines, we write them from offset 0.
> - * This offset is the only thing that stand between us and using
> - * GPIO_GENERIC.
> - */
> -#define LOONGSON_GPIO_IN_OFFSET        16
> +struct loongson_gpio_chip {
> +       struct gpio_chip        chip;
> +       spinlock_t              lock;
> +       void __iomem            *base;
> +       int                     conf_offset;
> +       int                     out_offset;
> +       int                     in_offset;
> +       u16                     *gsi_idx_map;
> +       u16                     mapsize;
> +};
>
> -static DEFINE_SPINLOCK(gpio_lock);
> +static int loongson_gpio_request(
> +                       struct gpio_chip *chip, unsigned int pin)
> +{
> +       if (pin >= chip->ngpio)
> +               return -EINVAL;
> +
> +       return 0;
> +}
>
> -static int loongson_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
> +                       unsigned int pin, int input)
>  {
> -       u32 val;
> +       u64 qval;
> +       u8  bval;
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
> +               qval = readq(LOONGSON_GPIO_OEN(lgpio));
> +               if (input)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OEN(lgpio));
> +               return;
> +       }
>
> -       spin_lock(&gpio_lock);
> -       val = LOONGSON_GPIODATA;
> -       spin_unlock(&gpio_lock);
> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {

This is ugly - can you instead use chip data associated with the
relevant compatible to store whatever custom info you need? See the
data field in struct of_device_id and acpi_device_id. Same elsewhere,
I really dislike this label parsing everywhere.

> +               bval = input ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
> +               return;
> +       }
>
> -       return !!(val & BIT(gpio + LOONGSON_GPIO_IN_OFFSET));
> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
> +               if (input)
> +                       LOONGSON_GPIOIE |= BIT(pin);
> +               else
> +                       LOONGSON_GPIOIE &= ~BIT(pin);
> +               return;
> +       }
>  }
>
> -static void loongson_gpio_set_value(struct gpio_chip *chip,
> -               unsigned gpio, int value)
> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
> +                       int high)
>  {
> -       u32 val;
> +       u64 qval;
> +       u8 bval;
>
> -       spin_lock(&gpio_lock);
> -       val = LOONGSON_GPIODATA;
> -       if (value)
> -               val |= BIT(gpio);
> -       else
> -               val &= ~BIT(gpio);
> -       LOONGSON_GPIODATA = val;
> -       spin_unlock(&gpio_lock);
> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
> +               qval = readq(LOONGSON_GPIO_OUT(lgpio));
> +               if (high)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OUT(lgpio));
> +               return;
> +       }
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {
> +               bval = high ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
> +               return;
> +       }
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
> +               if (LOONGSON_GPIODATA)
> +                       LOONGSON_GPIODATA |= BIT(pin);
> +               else
> +                       LOONGSON_GPIODATA &= ~BIT(pin);
> +               return;
> +       }
>  }
>
> -static int loongson_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> +static int loongson_gpio_direction_input(
> +                               struct gpio_chip *chip, unsigned int pin)
>  {
> -       u32 temp;
> +       unsigned long flags;
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
>
> -       spin_lock(&gpio_lock);
> -       temp = LOONGSON_GPIOIE;
> -       temp |= BIT(gpio);
> -       LOONGSON_GPIOIE = temp;
> -       spin_unlock(&gpio_lock);
> +       spin_lock_irqsave(&lgpio->lock, flags);
> +       __set_direction(lgpio, pin, 1);
> +       spin_unlock_irqrestore(&lgpio->lock, flags);
>
>         return 0;
>  }
>
> -static int loongson_gpio_direction_output(struct gpio_chip *chip,
> -               unsigned gpio, int level)
> +static int loongson_gpio_direction_output(
> +                               struct gpio_chip *chip, unsigned int pin,
> +                               int value)
>  {
> -       u32 temp;
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +       unsigned long flags;
>
> -       loongson_gpio_set_value(chip, gpio, level);
> -       spin_lock(&gpio_lock);
> -       temp = LOONGSON_GPIOIE;
> -       temp &= ~BIT(gpio);
> -       LOONGSON_GPIOIE = temp;
> -       spin_unlock(&gpio_lock);
> +       spin_lock_irqsave(&lgpio->lock, flags);
> +       __set_level(lgpio, pin, value);
> +       __set_direction(lgpio, pin, 0);
> +       spin_unlock_irqrestore(&lgpio->lock, flags);
>
>         return 0;
>  }
>
> +static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
> +{
> +       u64 qval;
> +       u8  bval;
> +       int val;
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
> +               qval = readq(LOONGSON_GPIO_IN(lgpio));
> +               return ((qval & (1ULL << pin)) != 0);
> +       }
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {
> +               bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
> +               return (bval & 1);
> +       }
> +
> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
> +               val = LOONGSON_GPIODATA;
> +               return !!(val & BIT(pin + lgpio->in_offset));
> +       }
> +
> +       return -ENXIO;
> +}
> +
> +static void loongson_gpio_set(struct gpio_chip *chip, unsigned int pin,
> +                       int value)
> +{
> +       unsigned long flags;
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       spin_lock_irqsave(&lgpio->lock, flags);
> +       __set_level(lgpio, pin, value);
> +       spin_unlock_irqrestore(&lgpio->lock, flags);
> +}
> +
> +static int loongson_gpio_to_irq(
> +                       struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct platform_device *pdev =
> +               container_of(chip->parent, struct platform_device, dev);
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (offset >= chip->ngpio)
> +               return -EINVAL;
> +
> +       if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
> +               offset = lgpio->gsi_idx_map[offset];
> +
> +       return platform_get_irq(pdev, offset);
> +}
> +
> +static int loongson_gpio_init(
> +                       struct device *dev, struct loongson_gpio_chip *lgpio,
> +                       struct device_node *np, void __iomem *base)
> +{
> +       lgpio->chip.request = loongson_gpio_request;
> +       lgpio->chip.direction_input = loongson_gpio_direction_input;
> +       lgpio->chip.get = loongson_gpio_get;
> +       lgpio->chip.direction_output = loongson_gpio_direction_output;
> +       lgpio->chip.set = loongson_gpio_set;
> +       lgpio->chip.can_sleep = 0;
> +       lgpio->chip.of_node = np;
> +       lgpio->chip.parent = dev;
> +       spin_lock_init(&lgpio->lock);
> +       lgpio->base = (void __iomem *)base;
> +       lgpio->chip.to_irq = loongson_gpio_to_irq;
> +
> +       gpiochip_add(&lgpio->chip);
> +
> +       return 0;
> +}
> +
> +static void of_loongson_gpio_get_props(struct device_node *np,
> +                                 struct loongson_gpio_chip *lgpio)
> +{
> +       const char *name;
> +
> +       of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio);
> +
> +       of_property_read_string(np, "compatible", &name);
> +       lgpio->chip.label = kstrdup(name, GFP_KERNEL);
> +
> +       if (!strcmp(name, "loongson,ls2k-gpio")) {
> +               lgpio->conf_offset = 0x0;
> +               lgpio->out_offset = 0x10;
> +               lgpio->in_offset = 0x20;
> +               return;
> +       }
> +
> +       if (!strcmp(name, "loongson,ls7a-gpio")) {
> +               lgpio->conf_offset = 0x800;
> +               lgpio->out_offset = 0x900;
> +               lgpio->in_offset = 0xa00;
> +               return;
> +       }
> +}
> +
> +static void acpi_loongson_gpio_get_props(struct platform_device *pdev,
> +                                 struct loongson_gpio_chip *lgpio)
> +{
> +
> +       struct device *dev = &pdev->dev;
> +       int rval;
> +
> +       device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio);
> +       device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base);
> +       device_property_read_u32(dev, "conf_offset",
> +                                       (u32 *)&lgpio->conf_offset);
> +       device_property_read_u32(dev, "out_offset",
> +                                       (u32 *)&lgpio->out_offset);
> +       device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset);
> +
> +       rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0);
> +       if (rval > 0) {
> +               lgpio->gsi_idx_map =
> +                       kmalloc_array(rval, sizeof(*lgpio->gsi_idx_map),
> +                                       GFP_KERNEL);
> +               if (unlikely(!lgpio->gsi_idx_map)) {
> +                       dev_err(dev, "Alloc gsi_idx_map fail!\n");
> +               } else {
> +                       lgpio->mapsize = rval;
> +                       device_property_read_u16_array(dev, "gsi_idx_map",
> +                                       lgpio->gsi_idx_map, lgpio->mapsize);
> +               }
> +       }
> +
> +       lgpio->chip.label = kstrdup(pdev->name, GFP_KERNEL);
> +}
> +
> +static void platform_loongson_gpio_get_props(struct platform_device *pdev,
> +                                 struct loongson_gpio_chip *lgpio)
> +{
> +}

Drop it, if you don't use it.

> +
>  static int loongson_gpio_probe(struct platform_device *pdev)
>  {
> -       struct gpio_chip *gc;
> +       struct resource *iores;
> +       void __iomem *base;
> +       struct loongson_gpio_chip *lgpio;
> +       struct device_node *np = pdev->dev.of_node;
>         struct device *dev = &pdev->dev;
> +       int ret = 0;
>
> -       gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> -       if (!gc)
> +       lgpio = kzalloc(sizeof(struct loongson_gpio_chip), GFP_KERNEL);

Why stop using devres? Same elsewhere.

> +       if (!lgpio)
>                 return -ENOMEM;
>
> -       gc->label = "loongson-gpio-chip";
> -       gc->base = 0;
> -       gc->ngpio = LOONGSON_N_GPIO;
> -       gc->get = loongson_gpio_get_value;
> -       gc->set = loongson_gpio_set_value;
> -       gc->direction_input = loongson_gpio_direction_input;
> -       gc->direction_output = loongson_gpio_direction_output;
> +       if (np)
> +               of_loongson_gpio_get_props(np, lgpio);
> +       else if (ACPI_COMPANION(&pdev->dev))
> +               acpi_loongson_gpio_get_props(pdev, lgpio);
> +       else
> +               platform_loongson_gpio_get_props(pdev, lgpio);

I would prefer if you used generic device properties - like you
already do for acpi. They work for OF too thanks to the fwnode
abstraction.

> +
> +       iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!iores) {
> +               ret = -ENODEV;
> +               goto out;
> +       }
> +
> +       if (!request_mem_region(iores->start, resource_size(iores),
> +                               pdev->name)) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       base = ioremap(iores->start, resource_size(iores));
> +       if (!base) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }

Are you aware of the gpio-regmap interface? It seems to me that this
code could benefit from using it. There are already some good examples
in the tree.

> +
> +       platform_set_drvdata(pdev, lgpio);
> +
> +       loongson_gpio_init(dev, lgpio, np, base);
> +
> +       return 0;
> +out:
> +       dev_err(dev, "missing mandatory property\n");
> +

So if request_mem_region() or ioremap() fails, we're printing this
error message? Doesn't seem right.

> +       return ret;
> +}
> +
> +static int loongson_gpio_remove(struct platform_device *pdev)
> +{
> +       struct loongson_gpio_chip *lgpio = platform_get_drvdata(pdev);
> +       struct resource         *mem;
>
> -       return gpiochip_add_data(gc, NULL);
> +       platform_set_drvdata(pdev, NULL);
> +
> +       gpiochip_remove(&lgpio->chip);
> +       iounmap(lgpio->base);
> +       kfree(lgpio->gsi_idx_map);
> +       kfree(lgpio);
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       release_mem_region(mem->start, resource_size(mem));
> +

This entire function can be dropped if you used devres.

> +       return 0;
>  }
>
> +static const struct of_device_id loongson_gpio_dt_ids[] = {
> +       { .compatible = "loongson,ls2k-gpio"},
> +       { .compatible = "loongson,ls7a-gpio"},
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids);
> +
> +static const struct acpi_device_id loongson_gpio_acpi_match[] = {
> +       {"LOON0002"},
> +       {}
> +};
> +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
> +
>  static struct platform_driver loongson_gpio_driver = {
>         .driver = {
>                 .name = "loongson-gpio",
> +               .owner = THIS_MODULE,
> +               .of_match_table = loongson_gpio_dt_ids,
> +               .acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match),
>         },
>         .probe = loongson_gpio_probe,
> +       .remove = loongson_gpio_remove,
>  };
>
>  static int __init loongson_gpio_setup(void)
>  {
> -       struct platform_device *pdev;
> -       int ret;
> -
> -       ret = platform_driver_register(&loongson_gpio_driver);
> -       if (ret) {
> -               pr_err("error registering loongson GPIO driver\n");
> -               return ret;
> -       }
> -
> -       pdev = platform_device_register_simple("loongson-gpio", -1, NULL, 0);
> -       return PTR_ERR_OR_ZERO(pdev);
> +       return platform_driver_register(&loongson_gpio_driver);
>  }
>  postcore_initcall(loongson_gpio_setup);
> +
> +static void __exit loongson_gpio_exit(void)
> +{
> +       platform_driver_unregister(&loongson_gpio_driver);
> +}

This module cannot be unloaded so you can drop this. The __exit macro
will make the compiler drop it anyway.

Bart

> +
> +MODULE_DESCRIPTION("Loongson gpio driver");
> +MODULE_LICENSE("GPL");
> --
> 2.31.1
>

Re: [PATCH v4 1/2] gpio: loongson: add dts and acpi support

[Yinbo Zhu]

在 2022/11/17 下午5:39, Bartosz Golaszewski 写道:
> On Thu, Nov 17, 2022 at 4:59 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>>
>> Latest Loongson platforms such as the Loongson-2 SoC series describe
>> GPIO device resources with DTS or ACPI. Add such support to the
>> existing platform device driver.
>>
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
>> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
>> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>> Change in v4:
>>                  1. Fixup name spelling about Signed-off-by.
>>                  2. Drop "series" here and everywhere else.
>>                  3. Fixup the copyright in driver.
>>                  4. Drop the "else" in loongson_gpio_request.
>>                  5. Use trinocular operation replace the related logic.
>>                  6. Remove lable judgement in context about "lgpio->chip.to_irq =
>>                     loongson_gpio_to_irq"
>>                  7. Use dev_err replace pr_err in probe.
>>                  8. Make legacy platform_data should be left out of this patch.
>>                  9. Remove the mips config in gpio Kconfig.
>> Change in v3:
>>                  1. Move the gpio platform data struct from arch/ into include/linux/
>>                     platform_data/.
>>                  2. Replace platform_gpio_data with loongson_gpio_platform_data in .c.
>>                  3. Add maintainer in MAINTAINERS file for include/linux/platform_data/
>>                     gpio-loongson.h and gpio-loongson.c
>> Change in v2:
>>                  1. Fixup of_loongson_gpio_get_props and remove the parse logic about
>>                     "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
>>                     "loongson,gpio_base", "loongson,support_irq" then kernel driver will
>>                     initial them that depend compatible except "loongson,gpio_base".
>>
>>   MAINTAINERS                  |   8 +
>>   drivers/gpio/Kconfig         |   6 +-
>>   drivers/gpio/gpio-loongson.c | 403 ++++++++++++++++++++++++++++-------
>>   3 files changed, 338 insertions(+), 79 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5114db9c8f32..838b920efcef 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12051,6 +12051,14 @@ S:     Maintained
>>   F:     Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
>>   F:     drivers/soc/loongson/loongson2_guts.c
>>
>> +LOONGSON GPIO DRIVER
>> +M:     Huacai Chen <chenhuacai@kernel.org>
>> +M:     Yinbo Zhu <zhuyinbo@loongson.cn>
>> +L:     linux-gpio@vger.kernel.org
>> +S:     Maintained
>> +F:     drivers/gpio/gpio-loongson.c
>> +F:     include/linux/platform_data/gpio-loongson.h
>> +
>>   LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
>>   M:     Sathya Prakash <sathya.prakash@broadcom.com>
>>   M:     Sreekanth Reddy <sreekanth.reddy@broadcom.com>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index a01af1180616..b4423c058b1d 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -376,10 +376,10 @@ config GPIO_LOGICVC
>>            programmable logic block.
>>
>>   config GPIO_LOONGSON
>> -       bool "Loongson-2/3 GPIO support"
>> -       depends on CPU_LOONGSON2EF || CPU_LOONGSON64
>> +       bool "Loongson series GPIO support"
>> +       depends on LOONGARCH || COMPILE_TEST
>>          help
>> -         Driver for GPIO functionality on Loongson-2F/3A/3B processors.
>> +         Driver for GPIO functionality on Loongson seires processors.
>>
>>   config GPIO_LPC18XX
>>          tristate "NXP LPC18XX/43XX GPIO support"
>> diff --git a/drivers/gpio/gpio-loongson.c b/drivers/gpio/gpio-loongson.c
>> index a42145873cc9..b9b191a541c0 100644
>> --- a/drivers/gpio/gpio-loongson.c
>> +++ b/drivers/gpio/gpio-loongson.c
>> @@ -1,13 +1,11 @@
>> -// SPDX-License-Identifier: GPL-2.0-or-later
>> +// SPDX-License-Identifier: GPL-2.0+
>>   /*
>> - *  Loongson-2F/3A/3B GPIO Support
>> + * Loongson GPIO Support
>>    *
>> - *  Copyright (c) 2008 Richard Liu,  STMicroelectronics         <richard.liu@st.com>
>> - *  Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com>
>> - *  Copyright (c) 2013 Hongbing Hu <huhb@lemote.com>
>> - *  Copyright (c) 2014 Huacai Chen <chenhc@lemote.com>
> 
> Why are you removing other people's copyright notices?
I will add a new file.
> 
>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>>    */
>>
>> +#include <linux/acpi.h>
>>   #include <linux/kernel.h>
>>   #include <linux/init.h>
>>   #include <linux/module.h>
>> @@ -17,119 +15,372 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/bitops.h>
>>   #include <asm/types.h>
>> -#include <loongson.h>
>> +#include <asm/loongson.h>
>>
>> -#define STLS2F_N_GPIO          4
>> -#define STLS3A_N_GPIO          16
>> +#define LOONGSON_GPIO_IN(x)            (x->base + x->in_offset)
>> +#define LOONGSON_GPIO_OUT(x)           (x->base + x->out_offset)
>> +#define LOONGSON_GPIO_OEN(x)           (x->base + x->conf_offset)
>>
>> -#ifdef CONFIG_CPU_LOONGSON64
>> -#define LOONGSON_N_GPIO        STLS3A_N_GPIO
>> -#else
>> -#define LOONGSON_N_GPIO        STLS2F_N_GPIO
>> -#endif
>> +#define LOONGSON_GPIO_IN_BYTE(x, gpio) (x->base +\
>> +                                       x->in_offset + gpio)
>> +#define LOONGSON_GPIO_OUT_BYTE(x, gpio)        (x->base +\
>> +                                       x->out_offset + gpio)
>> +#define LOONGSON_GPIO_OEN_BYTE(x, gpio)        (x->base +\
>> +                                       x->conf_offset + gpio)
>>
>> -/*
>> - * Offset into the register where we read lines, we write them from offset 0.
>> - * This offset is the only thing that stand between us and using
>> - * GPIO_GENERIC.
>> - */
>> -#define LOONGSON_GPIO_IN_OFFSET        16
>> +struct loongson_gpio_chip {
>> +       struct gpio_chip        chip;
>> +       spinlock_t              lock;
>> +       void __iomem            *base;
>> +       int                     conf_offset;
>> +       int                     out_offset;
>> +       int                     in_offset;
>> +       u16                     *gsi_idx_map;
>> +       u16                     mapsize;
>> +};
>>
>> -static DEFINE_SPINLOCK(gpio_lock);
>> +static int loongson_gpio_request(
>> +                       struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       if (pin >= chip->ngpio)
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
>>
>> -static int loongson_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
>> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
>> +                       unsigned int pin, int input)
>>   {
>> -       u32 val;
>> +       u64 qval;
>> +       u8  bval;
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
>> +               qval = readq(LOONGSON_GPIO_OEN(lgpio));
>> +               if (input)
>> +                       qval |= 1ULL << pin;
>> +               else
>> +                       qval &= ~(1ULL << pin);
>> +               writeq(qval, LOONGSON_GPIO_OEN(lgpio));
>> +               return;
>> +       }
>>
>> -       spin_lock(&gpio_lock);
>> -       val = LOONGSON_GPIODATA;
>> -       spin_unlock(&gpio_lock);
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
>> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {
> 
> This is ugly - can you instead use chip data associated with the
> relevant compatible to store whatever custom info you need? See the
> data field in struct of_device_id and acpi_device_id. Same elsewhere,
> I really dislike this label parsing everywhere.
okay, I got it.
> 
>> +               bval = input ? 1 : 0;
>> +               writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
>> +               return;
>> +       }
>>
>> -       return !!(val & BIT(gpio + LOONGSON_GPIO_IN_OFFSET));
>> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
>> +               if (input)
>> +                       LOONGSON_GPIOIE |= BIT(pin);
>> +               else
>> +                       LOONGSON_GPIOIE &= ~BIT(pin);
>> +               return;
>> +       }
>>   }
>>
>> -static void loongson_gpio_set_value(struct gpio_chip *chip,
>> -               unsigned gpio, int value)
>> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
>> +                       int high)
>>   {
>> -       u32 val;
>> +       u64 qval;
>> +       u8 bval;
>>
>> -       spin_lock(&gpio_lock);
>> -       val = LOONGSON_GPIODATA;
>> -       if (value)
>> -               val |= BIT(gpio);
>> -       else
>> -               val &= ~BIT(gpio);
>> -       LOONGSON_GPIODATA = val;
>> -       spin_unlock(&gpio_lock);
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
>> +               qval = readq(LOONGSON_GPIO_OUT(lgpio));
>> +               if (high)
>> +                       qval |= 1ULL << pin;
>> +               else
>> +                       qval &= ~(1ULL << pin);
>> +               writeq(qval, LOONGSON_GPIO_OUT(lgpio));
>> +               return;
>> +       }
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
>> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {
>> +               bval = high ? 1 : 0;
>> +               writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
>> +               return;
>> +       }
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
>> +               if (LOONGSON_GPIODATA)
>> +                       LOONGSON_GPIODATA |= BIT(pin);
>> +               else
>> +                       LOONGSON_GPIODATA &= ~BIT(pin);
>> +               return;
>> +       }
>>   }
>>
>> -static int loongson_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
>> +static int loongson_gpio_direction_input(
>> +                               struct gpio_chip *chip, unsigned int pin)
>>   {
>> -       u32 temp;
>> +       unsigned long flags;
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>>
>> -       spin_lock(&gpio_lock);
>> -       temp = LOONGSON_GPIOIE;
>> -       temp |= BIT(gpio);
>> -       LOONGSON_GPIOIE = temp;
>> -       spin_unlock(&gpio_lock);
>> +       spin_lock_irqsave(&lgpio->lock, flags);
>> +       __set_direction(lgpio, pin, 1);
>> +       spin_unlock_irqrestore(&lgpio->lock, flags);
>>
>>          return 0;
>>   }
>>
>> -static int loongson_gpio_direction_output(struct gpio_chip *chip,
>> -               unsigned gpio, int level)
>> +static int loongson_gpio_direction_output(
>> +                               struct gpio_chip *chip, unsigned int pin,
>> +                               int value)
>>   {
>> -       u32 temp;
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +       unsigned long flags;
>>
>> -       loongson_gpio_set_value(chip, gpio, level);
>> -       spin_lock(&gpio_lock);
>> -       temp = LOONGSON_GPIOIE;
>> -       temp &= ~BIT(gpio);
>> -       LOONGSON_GPIOIE = temp;
>> -       spin_unlock(&gpio_lock);
>> +       spin_lock_irqsave(&lgpio->lock, flags);
>> +       __set_level(lgpio, pin, value);
>> +       __set_direction(lgpio, pin, 0);
>> +       spin_unlock_irqrestore(&lgpio->lock, flags);
>>
>>          return 0;
>>   }
>>
>> +static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       u64 qval;
>> +       u8  bval;
>> +       int val;
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls2k-gpio")) {
>> +               qval = readq(LOONGSON_GPIO_IN(lgpio));
>> +               return ((qval & (1ULL << pin)) != 0);
>> +       }
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,ls7a-gpio") ||
>> +                       !strncmp(lgpio->chip.label, "LOON0002", 8)) {
>> +               bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
>> +               return (bval & 1);
>> +       }
>> +
>> +       if (!strcmp(lgpio->chip.label, "loongson,platform-gpio")) {
>> +               val = LOONGSON_GPIODATA;
>> +               return !!(val & BIT(pin + lgpio->in_offset));
>> +       }
>> +
>> +       return -ENXIO;
>> +}
>> +
>> +static void loongson_gpio_set(struct gpio_chip *chip, unsigned int pin,
>> +                       int value)
>> +{
>> +       unsigned long flags;
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +
>> +       spin_lock_irqsave(&lgpio->lock, flags);
>> +       __set_level(lgpio, pin, value);
>> +       spin_unlock_irqrestore(&lgpio->lock, flags);
>> +}
>> +
>> +static int loongson_gpio_to_irq(
>> +                       struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct platform_device *pdev =
>> +               container_of(chip->parent, struct platform_device, dev);
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +
>> +       if (offset >= chip->ngpio)
>> +               return -EINVAL;
>> +
>> +       if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
>> +               offset = lgpio->gsi_idx_map[offset];
>> +
>> +       return platform_get_irq(pdev, offset);
>> +}
>> +
>> +static int loongson_gpio_init(
>> +                       struct device *dev, struct loongson_gpio_chip *lgpio,
>> +                       struct device_node *np, void __iomem *base)
>> +{
>> +       lgpio->chip.request = loongson_gpio_request;
>> +       lgpio->chip.direction_input = loongson_gpio_direction_input;
>> +       lgpio->chip.get = loongson_gpio_get;
>> +       lgpio->chip.direction_output = loongson_gpio_direction_output;
>> +       lgpio->chip.set = loongson_gpio_set;
>> +       lgpio->chip.can_sleep = 0;
>> +       lgpio->chip.of_node = np;
>> +       lgpio->chip.parent = dev;
>> +       spin_lock_init(&lgpio->lock);
>> +       lgpio->base = (void __iomem *)base;
>> +       lgpio->chip.to_irq = loongson_gpio_to_irq;
>> +
>> +       gpiochip_add(&lgpio->chip);
>> +
>> +       return 0;
>> +}
>> +
>> +static void of_loongson_gpio_get_props(struct device_node *np,
>> +                                 struct loongson_gpio_chip *lgpio)
>> +{
>> +       const char *name;
>> +
>> +       of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio);
>> +
>> +       of_property_read_string(np, "compatible", &name);
>> +       lgpio->chip.label = kstrdup(name, GFP_KERNEL);
>> +
>> +       if (!strcmp(name, "loongson,ls2k-gpio")) {
>> +               lgpio->conf_offset = 0x0;
>> +               lgpio->out_offset = 0x10;
>> +               lgpio->in_offset = 0x20;
>> +               return;
>> +       }
>> +
>> +       if (!strcmp(name, "loongson,ls7a-gpio")) {
>> +               lgpio->conf_offset = 0x800;
>> +               lgpio->out_offset = 0x900;
>> +               lgpio->in_offset = 0xa00;
>> +               return;
>> +       }
>> +}
>> +
>> +static void acpi_loongson_gpio_get_props(struct platform_device *pdev,
>> +                                 struct loongson_gpio_chip *lgpio)
>> +{
>> +
>> +       struct device *dev = &pdev->dev;
>> +       int rval;
>> +
>> +       device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio);
>> +       device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base);
>> +       device_property_read_u32(dev, "conf_offset",
>> +                                       (u32 *)&lgpio->conf_offset);
>> +       device_property_read_u32(dev, "out_offset",
>> +                                       (u32 *)&lgpio->out_offset);
>> +       device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset);
>> +
>> +       rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0);
>> +       if (rval > 0) {
>> +               lgpio->gsi_idx_map =
>> +                       kmalloc_array(rval, sizeof(*lgpio->gsi_idx_map),
>> +                                       GFP_KERNEL);
>> +               if (unlikely(!lgpio->gsi_idx_map)) {
>> +                       dev_err(dev, "Alloc gsi_idx_map fail!\n");
>> +               } else {
>> +                       lgpio->mapsize = rval;
>> +                       device_property_read_u16_array(dev, "gsi_idx_map",
>> +                                       lgpio->gsi_idx_map, lgpio->mapsize);
>> +               }
>> +       }
>> +
>> +       lgpio->chip.label = kstrdup(pdev->name, GFP_KERNEL);
>> +}
>> +
>> +static void platform_loongson_gpio_get_props(struct platform_device *pdev,
>> +                                 struct loongson_gpio_chip *lgpio)
>> +{
>> +}
> 
> Drop it, if you don't use it.
okay, I got it.
> 
>> +
>>   static int loongson_gpio_probe(struct platform_device *pdev)
>>   {
>> -       struct gpio_chip *gc;
>> +       struct resource *iores;
>> +       void __iomem *base;
>> +       struct loongson_gpio_chip *lgpio;
>> +       struct device_node *np = pdev->dev.of_node;
>>          struct device *dev = &pdev->dev;
>> +       int ret = 0;
>>
>> -       gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
>> -       if (!gc)
>> +       lgpio = kzalloc(sizeof(struct loongson_gpio_chip), GFP_KERNEL);
> 
> Why stop using devres? Same elsewhere.
okay, I had use it devres.
> 
>> +       if (!lgpio)
>>                  return -ENOMEM;
>>
>> -       gc->label = "loongson-gpio-chip";
>> -       gc->base = 0;
>> -       gc->ngpio = LOONGSON_N_GPIO;
>> -       gc->get = loongson_gpio_get_value;
>> -       gc->set = loongson_gpio_set_value;
>> -       gc->direction_input = loongson_gpio_direction_input;
>> -       gc->direction_output = loongson_gpio_direction_output;
>> +       if (np)
>> +               of_loongson_gpio_get_props(np, lgpio);
>> +       else if (ACPI_COMPANION(&pdev->dev))
>> +               acpi_loongson_gpio_get_props(pdev, lgpio);
>> +       else
>> +               platform_loongson_gpio_get_props(pdev, lgpio);
> 
> I would prefer if you used generic device properties - like you
> already do for acpi. They work for OF too thanks to the fwnode
> abstraction.
Some of these attributes I tried to define in the dts, but they are not
generic, so I put them in xxx_ device_ id data field.
> 
>> +
>> +       iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!iores) {
>> +               ret = -ENODEV;
>> +               goto out;
>> +       }
>> +
>> +       if (!request_mem_region(iores->start, resource_size(iores),
>> +                               pdev->name)) {
>> +               ret = -EBUSY;
>> +               goto out;
>> +       }
>> +
>> +       base = ioremap(iores->start, resource_size(iores));
>> +       if (!base) {
>> +               ret = -ENOMEM;
>> +               goto out;
>> +       }
> 
> Are you aware of the gpio-regmap interface? It seems to me that this
> code could benefit from using it. There are already some good examples
> in the tree.
I tried to use regmap, but I found that my driver needs to access 64 bit
registers, and regmap cannot cover it.
> 
>> +
>> +       platform_set_drvdata(pdev, lgpio);
>> +
>> +       loongson_gpio_init(dev, lgpio, np, base);
>> +
>> +       return 0;
>> +out:
>> +       dev_err(dev, "missing mandatory property\n");
>> +
> 
> So if request_mem_region() or ioremap() fails, we're printing this
> error message? Doesn't seem right.
> 
>> +       return ret;
>> +}
>> +
>> +static int loongson_gpio_remove(struct platform_device *pdev)
>> +{
>> +       struct loongson_gpio_chip *lgpio = platform_get_drvdata(pdev);
>> +       struct resource         *mem;
>>
>> -       return gpiochip_add_data(gc, NULL);
>> +       platform_set_drvdata(pdev, NULL);
>> +
>> +       gpiochip_remove(&lgpio->chip);
>> +       iounmap(lgpio->base);
>> +       kfree(lgpio->gsi_idx_map);
>> +       kfree(lgpio);
>> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       release_mem_region(mem->start, resource_size(mem));
>> +
> 
> This entire function can be dropped if you used devres.
> 
>> +       return 0;
>>   }
>>
>> +static const struct of_device_id loongson_gpio_dt_ids[] = {
>> +       { .compatible = "loongson,ls2k-gpio"},
>> +       { .compatible = "loongson,ls7a-gpio"},
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids);
>> +
>> +static const struct acpi_device_id loongson_gpio_acpi_match[] = {
>> +       {"LOON0002"},
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
>> +
>>   static struct platform_driver loongson_gpio_driver = {
>>          .driver = {
>>                  .name = "loongson-gpio",
>> +               .owner = THIS_MODULE,
>> +               .of_match_table = loongson_gpio_dt_ids,
>> +               .acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match),
>>          },
>>          .probe = loongson_gpio_probe,
>> +       .remove = loongson_gpio_remove,
>>   };
>>
>>   static int __init loongson_gpio_setup(void)
>>   {
>> -       struct platform_device *pdev;
>> -       int ret;
>> -
>> -       ret = platform_driver_register(&loongson_gpio_driver);
>> -       if (ret) {
>> -               pr_err("error registering loongson GPIO driver\n");
>> -               return ret;
>> -       }
>> -
>> -       pdev = platform_device_register_simple("loongson-gpio", -1, NULL, 0);
>> -       return PTR_ERR_OR_ZERO(pdev);
>> +       return platform_driver_register(&loongson_gpio_driver);
>>   }
>>   postcore_initcall(loongson_gpio_setup);
>> +
>> +static void __exit loongson_gpio_exit(void)
>> +{
>> +       platform_driver_unregister(&loongson_gpio_driver);
>> +}
> 
> This module cannot be unloaded so you can drop this. The __exit macro
> will make the compiler drop it anyway.
okay I got it.
> 
> Bart
> 
>> +
>> +MODULE_DESCRIPTION("Loongson gpio driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.31.1
>>

Re: [PATCH v4 1/2] gpio: loongson: add dts and acpi support

[Arnd Bergmann]

On Thu, Nov 17, 2022, at 04:59, Yinbo Zhu wrote:
> 
>  config GPIO_LOONGSON
> -	bool "Loongson-2/3 GPIO support"
> -	depends on CPU_LOONGSON2EF || CPU_LOONGSON64
> +	bool "Loongson series GPIO support"
> +	depends on LOONGARCH || COMPILE_TEST

This looks like it will introduce a regression for users of the
older machines CPU_LOONGSON2EF and CPU_LOONGSON64 machines.

While the driver previously called 'platform_device_register_simple'
to create the platform device itself, this call is no longer
done anywhere, so it also cannot work here, but whatever was
working should not be broken. I can see two possible ways to do
this:

a) create the platform_device in the mips code in a way that
the driver can handle it as before

b) duplicate the entire driver and leave the old code untouched.

The second one is probably easier here, but the first one would
be nicer in the end, depending on how much of the original
code remains.

>  	help
> -	  Driver for GPIO functionality on Loongson-2F/3A/3B processors.
> +	  Driver for GPIO functionality on Loongson seires processors.

s/seires/series/

> +static void of_loongson_gpio_get_props(struct device_node *np,
> +				  struct loongson_gpio_chip *lgpio)
> +{
> +	const char *name;
> +
> +	of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio);

This does not work: chip.ngpio is a 16-bit field, so you
cannot overwrite it using a 32-bit pointer dereference. Just
use a local variable as an intermediate

> +	of_property_read_string(np, "compatible", &name);
> +	lgpio->chip.label = kstrdup(name, GFP_KERNEL);
> +	if (!strcmp(name, "loongson,ls2k-gpio")) {
> +		lgpio->conf_offset = 0x0;

This probably works, but is not reliable since "compatible"
is an enumeration rather than a single string. Using
of_device_is_compatible() would work here, or even better
you can have a configuration that is referenced from
the 'data' field of the 'of_device_id'

> +static void acpi_loongson_gpio_get_props(struct platform_device *pdev,
> +				  struct loongson_gpio_chip *lgpio)
> +{
> +
> +	struct device *dev = &pdev->dev;
> +	int rval;
> +
> +	device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio);
> +	device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base);
> +	device_property_read_u32(dev, "conf_offset",
> +					(u32 *)&lgpio->conf_offset);
> +	device_property_read_u32(dev, "out_offset",
> +					(u32 *)&lgpio->out_offset);
> +	device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset);

This looks worrying: While you addressed the feedback in the
DT binding, the ACPI version still uses the old format, which
the binding is different depending on the firmware.

A modern driver should not set the "gpio_base" any more, and
the firmware should not care either.

The other fields appear to correspond to the ones that the DT
version decides based on the device identifier. There isn't
really a point in doing this differently, so pick one version
or the other and then use the same method for both DT and ACPI.

> +static void platform_loongson_gpio_get_props(struct platform_device *pdev,
> +				  struct loongson_gpio_chip *lgpio)
> +{
> +}

> +	if (np)
> +		of_loongson_gpio_get_props(np, lgpio);
> +	else if (ACPI_COMPANION(&pdev->dev))
> +		acpi_loongson_gpio_get_props(pdev, lgpio);
> +	else
> +		platform_loongson_gpio_get_props(pdev, lgpio);

The third branch is clearly broken now as it fails to assign
anything. Using device_property_read_u32() etc should really
work in all three cases, so if you fold the
of_loongson_gpio_get_props and acpi_loongson_gpio_get_props
functions into one, that will solve the third case as well.

> +static const struct of_device_id loongson_gpio_dt_ids[] = {
> +	{ .compatible = "loongson,ls2k-gpio"},
> +	{ .compatible = "loongson,ls7a-gpio"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids);
> +
> +static const struct acpi_device_id loongson_gpio_acpi_match[] = {
> +	{"LOON0002"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
> +
>  static struct platform_driver loongson_gpio_driver = {
>  	.driver = {
>  		.name = "loongson-gpio",
> +		.owner = THIS_MODULE,
> +		.of_match_table = loongson_gpio_dt_ids,
> +		.acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match),
>  	},

The ACPI_PTR() macro here means that you get an "unused variable"
warning when the driver is build with CONFIG_ACPI disabled.
I think you should just reference the variable directly. If you
want to save a few bytes, you can keep the ACPI_PTR() here
and enclose the struct definition in #ifdef CONFIG_ACPI.

    Arnd

Re: [PATCH v4 1/2] gpio: loongson: add dts and acpi support

[Yinbo Zhu]

Hi Arnd,

I had adop your advice and as v5 series patch.
and about move the legacy gpio driver to other deposition that I have
internal talk in loongson team and think it should be okay.

BRs,
Yinbo.
在 2022/11/17 下午5:55, Arnd Bergmann 写道:
> On Thu, Nov 17, 2022, at 04:59, Yinbo Zhu wrote:
>>
>>   config GPIO_LOONGSON
>> -	bool "Loongson-2/3 GPIO support"
>> -	depends on CPU_LOONGSON2EF || CPU_LOONGSON64
>> +	bool "Loongson series GPIO support"
>> +	depends on LOONGARCH || COMPILE_TEST
> 
> This looks like it will introduce a regression for users of the
> older machines CPU_LOONGSON2EF and CPU_LOONGSON64 machines.
> 
> While the driver previously called 'platform_device_register_simple'
> to create the platform device itself, this call is no longer
> done anywhere, so it also cannot work here, but whatever was
> working should not be broken. I can see two possible ways to do
> this:
> 
> a) create the platform_device in the mips code in a way that
> the driver can handle it as before
> 
> b) duplicate the entire driver and leave the old code untouched.
> 
> The second one is probably easier here, but the first one would
> be nicer in the end, depending on how much of the original
> code remains.
> 
>>   	help
>> -	  Driver for GPIO functionality on Loongson-2F/3A/3B processors.
>> +	  Driver for GPIO functionality on Loongson seires processors.
> 
> s/seires/series/
> 
>> +static void of_loongson_gpio_get_props(struct device_node *np,
>> +				  struct loongson_gpio_chip *lgpio)
>> +{
>> +	const char *name;
>> +
>> +	of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio);
> 
> This does not work: chip.ngpio is a 16-bit field, so you
> cannot overwrite it using a 32-bit pointer dereference. Just
> use a local variable as an intermediate
> 
>> +	of_property_read_string(np, "compatible", &name);
>> +	lgpio->chip.label = kstrdup(name, GFP_KERNEL);
>> +	if (!strcmp(name, "loongson,ls2k-gpio")) {
>> +		lgpio->conf_offset = 0x0;
> 
> This probably works, but is not reliable since "compatible"
> is an enumeration rather than a single string. Using
> of_device_is_compatible() would work here, or even better
> you can have a configuration that is referenced from
> the 'data' field of the 'of_device_id'
> 
>> +static void acpi_loongson_gpio_get_props(struct platform_device *pdev,
>> +				  struct loongson_gpio_chip *lgpio)
>> +{
>> +
>> +	struct device *dev = &pdev->dev;
>> +	int rval;
>> +
>> +	device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio);
>> +	device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base);
>> +	device_property_read_u32(dev, "conf_offset",
>> +					(u32 *)&lgpio->conf_offset);
>> +	device_property_read_u32(dev, "out_offset",
>> +					(u32 *)&lgpio->out_offset);
>> +	device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset);
> 
> This looks worrying: While you addressed the feedback in the
> DT binding, the ACPI version still uses the old format, which
> the binding is different depending on the firmware.
> 
> A modern driver should not set the "gpio_base" any more, and
> the firmware should not care either.
> 
> The other fields appear to correspond to the ones that the DT
> version decides based on the device identifier. There isn't
> really a point in doing this differently, so pick one version
> or the other and then use the same method for both DT and ACPI.
> 
>> +static void platform_loongson_gpio_get_props(struct platform_device *pdev,
>> +				  struct loongson_gpio_chip *lgpio)
>> +{
>> +}
> 
>> +	if (np)
>> +		of_loongson_gpio_get_props(np, lgpio);
>> +	else if (ACPI_COMPANION(&pdev->dev))
>> +		acpi_loongson_gpio_get_props(pdev, lgpio);
>> +	else
>> +		platform_loongson_gpio_get_props(pdev, lgpio);
> 
> The third branch is clearly broken now as it fails to assign
> anything. Using device_property_read_u32() etc should really
> work in all three cases, so if you fold the
> of_loongson_gpio_get_props and acpi_loongson_gpio_get_props
> functions into one, that will solve the third case as well.
> 
>> +static const struct of_device_id loongson_gpio_dt_ids[] = {
>> +	{ .compatible = "loongson,ls2k-gpio"},
>> +	{ .compatible = "loongson,ls7a-gpio"},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids);
>> +
>> +static const struct acpi_device_id loongson_gpio_acpi_match[] = {
>> +	{"LOON0002"},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
>> +
>>   static struct platform_driver loongson_gpio_driver = {
>>   	.driver = {
>>   		.name = "loongson-gpio",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = loongson_gpio_dt_ids,
>> +		.acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match),
>>   	},
> 
> The ACPI_PTR() macro here means that you get an "unused variable"
> warning when the driver is build with CONFIG_ACPI disabled.
> I think you should just reference the variable directly. If you
> want to save a few bytes, you can keep the ACPI_PTR() here
> and enclose the struct definition in #ifdef CONFIG_ACPI.
> 
>      Arnd
>