linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add pinctrl support to EN7581 SoC
@ 2024-08-22  9:40 Lorenzo Bianconi
  2024-08-22  9:40 ` [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller Lorenzo Bianconi
  2024-08-22  9:40 ` [PATCH v2 2/2] pinctrl: airoha: Add support for EN7581 SoC Lorenzo Bianconi
  0 siblings, 2 replies; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-22  9:40 UTC (permalink / raw)
  To: Lorenzo Bianconi, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno
  Cc: linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, benjamin.larsson, ansuelsmth

Introduce pinctrl driver for EN7581 SoC. Current EN7581 pinctrl driver
supports the following functionalities:
- pin multiplexing
- pin pull-up, pull-down, open-drain, current strength,
  {input,output}_enable, output_{low,high}
- gpio controller
- irq controller

---
Changes in v2:
- Fix compilation errors
- Collapse some register mappings for gpio and irq controllers
- update dt-bindings according to new register mapping
- fix some dt-bindings errors
- Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/

---
Lorenzo Bianconi (2):
      dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
      pinctrl: airoha: Add support for EN7581 SoC

 .../bindings/pinctrl/airoha,en7581-pinctrl.yaml    |  454 +++
 MAINTAINERS                                        |    7 +
 drivers/pinctrl/mediatek/Kconfig                   |   16 +-
 drivers/pinctrl/mediatek/Makefile                  |    1 +
 drivers/pinctrl/mediatek/pinctrl-airoha.c          | 3031 ++++++++++++++++++++
 5 files changed, 3508 insertions(+), 1 deletion(-)
---
base-commit: defaf1a2113a22b00dfa1abc0fd2014820eaf065
change-id: 20240818-en7581-pinctrl-1bf120154be0
prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-22  9:40 [PATCH v2 0/2] Add pinctrl support to EN7581 SoC Lorenzo Bianconi
@ 2024-08-22  9:40 ` Lorenzo Bianconi
  2024-08-22 16:06   ` Conor Dooley
  2024-08-22  9:40 ` [PATCH v2 2/2] pinctrl: airoha: Add support for EN7581 SoC Lorenzo Bianconi
  1 sibling, 1 reply; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-22  9:40 UTC (permalink / raw)
  To: Lorenzo Bianconi, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno
  Cc: linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, benjamin.larsson, ansuelsmth

Introduce device-tree binding documentation for Airoha EN7581 pinctrl
controller.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/pinctrl/airoha,en7581-pinctrl.yaml    | 454 +++++++++++++++++++++
 1 file changed, 454 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
new file mode 100644
index 000000000000..a4b58bc30416
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
@@ -0,0 +1,454 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/airoha,en7581-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Pin Controller
+
+maintainers:
+  - Lorenzo Bianconi <lorenzo@kernel.org>
+
+description:
+  The Airoha's EN7581 Pin controller is used to control SoC pins.
+
+allOf:
+  - $ref: pinctrl.yaml#
+
+properties:
+  compatible:
+    enum:
+      - airoha,en7581-pinctrl
+
+  reg:
+    items:
+      - description: IOMUX base address
+      - description: LED IOMUX base address
+      - description: GPIO flash mode base address
+      - description: GPIO flash mode extended base address
+      - description: IO pin configuration base address
+      - description: PCIE reset open-drain base address
+      - description: GPIO bank0 register base address
+      - description: GPIO bank0 second control register base address
+      - description: GPIO bank1 second control register base address
+      - description: GPIO bank1 register base address
+
+  reg-names:
+    items:
+      - const: iomux
+      - const: led-iomux
+      - const: gpio-flash-mode
+      - const: gpio-flash-mode-ext
+      - const: ioconf
+      - const: pcie-rst-od
+      - const: gpio-bank0
+      - const: gpio-ctrl1
+      - const: gpio-ctrl2
+      - const: gpio-bank1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description:
+      Number of cells in GPIO specifier. Since the generic GPIO binding is
+      used, the amount of cells must be specified as 2. See the below mentioned
+      gpio binding representation for description of particular cells.
+
+  gpio-ranges:
+    maxItems: 1
+    description:
+      GPIO valid number range.
+
+  interrupt-controller: true
+
+  interrupts:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 2
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '^.*mux.*$':
+        type: object
+        additionalProperties: false
+        description:
+          pinmux configuration nodes.
+
+        $ref: /schemas/pinctrl/pinmux-node.yaml
+        properties:
+          function:
+            description:
+              A string containing the name of the function to mux to the group.
+            enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi,
+                   pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0,
+                   phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1,
+                   phy3_led1, phy4_led1]
+          groups:
+            description:
+              An array of strings. Each string contains the name of a group.
+        required:
+          - function
+          - groups
+
+        allOf:
+          - if:
+              properties:
+                function:
+                  const: pon
+            then:
+              properties:
+                groups:
+                  enum: [pon]
+          - if:
+              properties:
+                function:
+                  const: tod_1pps
+            then:
+              properties:
+                groups:
+                  enum: [pon_tod_1pps, gsw_tod_1pps]
+          - if:
+              properties:
+                function:
+                  const: sipo
+            then:
+              properties:
+                groups:
+                  enum: [sipo, sipo_rclk]
+          - if:
+              properties:
+                function:
+                  const: mdio
+            then:
+              properties:
+                groups:
+                  enum: [mdio]
+          - if:
+              properties:
+                function:
+                  const: uart
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4,
+                           uart5]
+                  maxItems: 2
+          - if:
+              properties:
+                function:
+                  const: i2c
+            then:
+              properties:
+                groups:
+                  enum: [i2c1]
+          - if:
+              properties:
+                function:
+                  const: jtag
+            then:
+              properties:
+                groups:
+                  enum: [jtag_udi, jtag_dfd]
+          - if:
+              properties:
+                function:
+                  const: pcm
+            then:
+              properties:
+                groups:
+                  enum: [pcm1, pcm2]
+          - if:
+              properties:
+                function:
+                  const: spi
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [spi_quad, spi_cs1]
+                  maxItems: 2
+          - if:
+              properties:
+                function:
+                  const: pcm_spi
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1,
+                           pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3,
+                           pcm_spi_cs4]
+                  maxItems: 7
+          - if:
+              properties:
+                function:
+                  const: i2c
+            then:
+              properties:
+                groups:
+                  enum: [i2s]
+          - if:
+              properties:
+                function:
+                  const: emmc
+            then:
+              properties:
+                groups:
+                  enum: [emmc]
+          - if:
+              properties:
+                function:
+                  const: pnand
+            then:
+              properties:
+                groups:
+                  enum: [pnand]
+          - if:
+              properties:
+                function:
+                  const: pcie_reset
+            then:
+              properties:
+                groups:
+                  enum: [pcie_reset0, pcie_reset1, pcie_reset2]
+          - if:
+              properties:
+                function:
+                  const: pwm
+            then:
+              properties:
+                groups:
+                  enum: [gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6,
+                         gpio7, gpio8, gpio9, gpio10, gpio11, gpio12, gpio13,
+                         gpio14, gpio15, gpio16, gpio17, gpio18, gpio19,
+                         gpio20, gpio21, gpio22, gpio23, gpio24, gpio25,
+                         gpio26, gpio27, gpio28, gpio29, gpio30, gpio31,
+                         gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
+                         gpio42, gpio43, gpio44, gpio45, gpio46, gpio47]
+          - if:
+              properties:
+                function:
+                  const: phy1_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy2_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy3_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy4_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy1_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy2_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy3_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy4_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+
+      '^.*conf.*$':
+        type: object
+        additionalProperties: false
+        description:
+          pinconf configuration nodes.
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+
+        properties:
+          pins:
+            description:
+              An array of strings. Each string contains the name of a pin.
+            items:
+              enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk,
+                     spi_mosi, spi_miso, gpio0, gpio1, gpio2, gpio3, gpio4,
+                     gpio5, gpio6, gpio7, gpio8, gpio9, gpio10, gpio11, gpio12,
+                     gpio13, gpio14, gpio15, gpio16, gpio17, gpio18, gpio19,
+                     gpio20, gpio21, gpio22, gpio23, gpio24, gpio25, gpio26,
+                     gpio27, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33,
+                     gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40,
+                     gpio41, gpio42, gpio43, gpio44, gpio45, gpio46,
+                     pcie_reset0, pcie_reset1, pcie_reset2]
+            minItems: 1
+            maxItems: 58
+
+          bias-disable: true
+          bias-pull-up: true
+          bias-pull-down: true
+          input-enable: true
+          output-enable: true
+          output-low: true
+          output-high: true
+          drive-open-drain: true
+
+          drive-strength:
+            description:
+              Selects the drive strength for MIO pins, in mA.
+            enum: [2, 4, 6, 8]
+
+        required:
+          - pins
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pio: pinctrl@1fa20214 {
+        compatible = "airoha,en7581-pinctrl";
+        reg = <0x0 0x1fa20214 0x0 0x30>,
+              <0x0 0x1fa2027c 0x0 0x8>,
+              <0x0 0x1fbf0234 0x0 0x4>,
+              <0x0 0x1fbf0268 0x0 0x4>,
+              <0x0 0x1fa2001c 0x0 0x50>,
+              <0x0 0x1fa2018c 0x0 0x4>,
+              <0x0 0x1fbf0200 0x0 0x18>,
+              <0x0 0x1fbf0220 0x0 0x4>,
+              <0x0 0x1fbf0260 0x0 0x8>,
+              <0x0 0x1fbf0270 0x0 0x28>;
+        reg-names = "iomux", "led-iomux",
+                    "gpio-flash-mode", "gpio-flash-mode-ext",
+                    "ioconf", "pcie-rst-od",
+                    "gpio-bank0", "gpio-ctrl1",
+                    "gpio-ctrl2", "gpio-bank1";
+
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&pio 0 13 47>;
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&gic>;
+        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
+
+        pcie1-rst-pins {
+          conf {
+            pins = "pcie_reset1";
+            drive-open-drain = <1>;
+          };
+        };
+
+        pwm-pins {
+          mux {
+            function = "pwm";
+            groups = "gpio18";
+          };
+        };
+
+        spi-pins {
+          mux {
+            function = "spi";
+            groups = "spi_quad", "spi_cs1";
+          };
+        };
+
+        uart2-pins {
+          mux {
+            function = "uart";
+            groups = "uart2", "uart2_cts_rts";
+          };
+        };
+
+        uar5-pins {
+          mux {
+            function = "uart";
+            groups = "uart5";
+          };
+        };
+
+        mmc-pins {
+          mux {
+            function = "emmc";
+            groups = "emmc";
+          };
+        };
+
+        mdio-pins {
+          mux {
+            function = "mdio";
+            groups = "mdio";
+          };
+
+          conf {
+            pins = "gpio2";
+            output-enable;
+          };
+        };
+
+        gswp1-led0-pins {
+          mux {
+            function = "phy1_led0";
+            groups = "gpio33";
+          };
+        };
+
+        gswp2-led1-pins {
+          mux {
+            function = "phy2_led1";
+            groups = "gpio44";
+          };
+        };
+      };
+    };

-- 
2.46.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/2] pinctrl: airoha: Add support for EN7581 SoC
  2024-08-22  9:40 [PATCH v2 0/2] Add pinctrl support to EN7581 SoC Lorenzo Bianconi
  2024-08-22  9:40 ` [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller Lorenzo Bianconi
@ 2024-08-22  9:40 ` Lorenzo Bianconi
  1 sibling, 0 replies; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-22  9:40 UTC (permalink / raw)
  To: Lorenzo Bianconi, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno
  Cc: linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, benjamin.larsson, ansuelsmth

Introduce pinctrl driver for EN7581 SoC. Current EN7581 pinctrl driver
supports the following functionalities:
- pin multiplexing
- pin pull-up, pull-down, open-drain, current strength,
  {input,output}_enable, output_{low,high}
- gpio controller
- irq controller

Tested-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
Co-developed-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 MAINTAINERS                               |    7 +
 drivers/pinctrl/mediatek/Kconfig          |   16 +-
 drivers/pinctrl/mediatek/Makefile         |    1 +
 drivers/pinctrl/mediatek/pinctrl-airoha.c | 3031 +++++++++++++++++++++++++++++
 4 files changed, 3054 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e87e..75c0ae2224e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17967,6 +17967,13 @@ F:	drivers/pinctrl/
 F:	include/dt-bindings/pinctrl/
 F:	include/linux/pinctrl/
 
+PIN CONTROLLER - AIROHA
+M:	Lorenzo Bianconi <lorenzo@kernel.org>
+L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
+F:	drivers/pinctrl/mediatek/pinctrl-airoha.c
+
 PIN CONTROLLER - AMD
 M:	Basavaraj Natikar <Basavaraj.Natikar@amd.com>
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
index 7af287252834..43bbf606ee7d 100644
--- a/drivers/pinctrl/mediatek/Kconfig
+++ b/drivers/pinctrl/mediatek/Kconfig
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 menu "MediaTek pinctrl drivers"
-	depends on ARCH_MEDIATEK || RALINK || COMPILE_TEST
+	depends on ARCH_MEDIATEK || ARCH_AIROHA || RALINK || COMPILE_TEST
 
 config EINT_MTK
 	tristate "MediaTek External Interrupt Support"
@@ -126,6 +126,20 @@ config PINCTRL_MT8127
 	select PINCTRL_MTK
 
 # For ARMv8 SoCs
+config PINCTRL_AIROHA
+	tristate "Airoha pin control"
+	depends on OF
+	depends on ARM64 || COMPILE_TEST
+	select PINMUX
+	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support pin controller and gpio driver
+	  on Airoha EN7581 SoC.
+
 config PINCTRL_MT2712
 	bool "MediaTek MT2712 pin control"
 	depends on OF
diff --git a/drivers/pinctrl/mediatek/Makefile b/drivers/pinctrl/mediatek/Makefile
index 680f7e8526e0..1405d434218e 100644
--- a/drivers/pinctrl/mediatek/Makefile
+++ b/drivers/pinctrl/mediatek/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCTRL_MTK_MOORE)		+= pinctrl-moore.o
 obj-$(CONFIG_PINCTRL_MTK_PARIS)		+= pinctrl-paris.o
 
 # SoC Drivers
+obj-$(CONFIG_PINCTRL_AIROHA)		+= pinctrl-airoha.o
 obj-$(CONFIG_PINCTRL_MT7620)		+= pinctrl-mt7620.o
 obj-$(CONFIG_PINCTRL_MT7621)		+= pinctrl-mt7621.o
 obj-$(CONFIG_PINCTRL_MT76X8)		+= pinctrl-mt76x8.o
diff --git a/drivers/pinctrl/mediatek/pinctrl-airoha.c b/drivers/pinctrl/mediatek/pinctrl-airoha.c
new file mode 100644
index 000000000000..5e5bbc9157bc
--- /dev/null
+++ b/drivers/pinctrl/mediatek/pinctrl-airoha.c
@@ -0,0 +1,3031 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Author: Lorenzo Bianconi <lorenzo@kernel.org>
+ * Author: Benjamin Larsson <benjamin.larsson@genexis.eu>
+ * Author: Markus Gothe <markus.gothe@genexis.eu>
+ */
+
+#include <dt-bindings/pinctrl/mt65xx.h>
+#include <linux/bitfield.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+#include "../core.h"
+#include "../pinconf.h"
+#include "../pinmux.h"
+
+#define PINCTRL_PIN_GROUP(id)						\
+	PINCTRL_PINGROUP(#id, id##_pins, ARRAY_SIZE(id##_pins))
+
+#define PINCTRL_FUNC_DESC(id)						\
+	{								\
+		.desc = {						\
+			.func = {					\
+				.name = #id,				\
+				.groups = id##_groups,			\
+				.ngroups = ARRAY_SIZE(id##_groups),	\
+			}						\
+		},							\
+		.groups = id##_func_group,				\
+		.group_size = ARRAY_SIZE(id##_func_group),		\
+	}
+
+#define PINCTRL_CONF_DESC(p, offset, mask)				\
+	{								\
+		.pin = p,						\
+		.reg = { offset, mask },				\
+	}
+
+/* MUX */
+#define REG_GPIO_2ND_I2C_MODE			0x00
+#define GPIO_MDC_IO_MASTER_MODE_MODE		BIT(14)
+#define GPIO_I2C_MASTER_MODE_MODE		BIT(13)
+#define GPIO_I2S_MODE_MASK			BIT(12)
+#define GPIO_I2C_SLAVE_MODE_MODE		BIT(11)
+#define GPIO_LAN3_LED1_MODE_MASK		BIT(10)
+#define GPIO_LAN3_LED0_MODE_MASK		BIT(9)
+#define GPIO_LAN2_LED1_MODE_MASK		BIT(8)
+#define GPIO_LAN2_LED0_MODE_MASK		BIT(7)
+#define GPIO_LAN1_LED1_MODE_MASK		BIT(6)
+#define GPIO_LAN1_LED0_MODE_MASK		BIT(5)
+#define GPIO_LAN0_LED1_MODE_MASK		BIT(4)
+#define GPIO_LAN0_LED0_MODE_MASK		BIT(3)
+#define PON_TOD_1PPS_MODE_MASK			BIT(2)
+#define GSW_TOD_1PPS_MODE_MASK			BIT(1)
+#define GPIO_2ND_I2C_MODE_MASK			BIT(0)
+
+#define REG_GPIO_SPI_CS1_MODE			0x04
+#define GPIO_PCM_SPI_CS4_MODE_MASK		BIT(21)
+#define GPIO_PCM_SPI_CS3_MODE_MASK		BIT(20)
+#define GPIO_PCM_SPI_CS2_MODE_P156_MASK		BIT(19)
+#define GPIO_PCM_SPI_CS2_MODE_P128_MASK		BIT(18)
+#define GPIO_PCM_SPI_CS1_MODE_MASK		BIT(17)
+#define GPIO_PCM_SPI_MODE_MASK			BIT(16)
+#define GPIO_PCM2_MODE_MASK			BIT(13)
+#define GPIO_PCM1_MODE_MASK			BIT(12)
+#define GPIO_PCM_INT_MODE_MASK			BIT(9)
+#define GPIO_PCM_RESET_MODE_MASK		BIT(8)
+#define GPIO_SPI_QUAD_MODE_MASK			BIT(4)
+#define GPIO_SPI_CS4_MODE_MASK			BIT(3)
+#define GPIO_SPI_CS3_MODE_MASK			BIT(2)
+#define GPIO_SPI_CS2_MODE_MASK			BIT(1)
+#define GPIO_SPI_CS1_MODE_MASK			BIT(0)
+
+#define REG_GPIO_PON_MODE			0x08
+#define GPIO_PARALLEL_NAND_MODE_MASK		BIT(14)
+#define GPIO_SGMII_MDIO_MODE_MASK		BIT(13)
+#define GPIO_PCIE_RESET2_MASK			BIT(12)
+#define SIPO_RCLK_MODE_MASK			BIT(11)
+#define GPIO_PCIE_RESET1_MASK			BIT(10)
+#define GPIO_PCIE_RESET0_MASK			BIT(9)
+#define GPIO_UART5_MODE_MASK			BIT(8)
+#define GPIO_UART4_MODE_MASK			BIT(7)
+#define GPIO_HSUART_CTS_RTS_MODE_MASK		BIT(6)
+#define GPIO_HSUART_MODE_MASK			BIT(5)
+#define GPIO_UART2_CTS_RTS_MODE_MASK		BIT(4)
+#define GPIO_UART2_MODE_MASK			BIT(3)
+#define GPIO_SIPO_MODE_MASK			BIT(2)
+#define GPIO_EMMC_MODE_MASK			BIT(1)
+#define GPIO_PON_MODE_MASK			BIT(0)
+
+#define REG_NPU_UART_EN				0x10
+#define JTAG_UDI_EN_MASK			BIT(4)
+#define JTAG_DFD_EN_MASK			BIT(3)
+
+#define REG_FORCE_GPIO0_EN			0x14
+#define REG_FORCE_GPIO32_EN			0x18
+#define REG_INIC_MDIO_SLV_MOD			0x1c
+
+/* LED MAP */
+#define REG_LAN_LED0_MAPPING			0x00
+#define REG_LAN_LED1_MAPPING			0x04
+
+#define LAN4_LED_MAPPING_MASK			GENMASK(18, 16)
+#define LAN4_PHY4_LED_MAP			BIT(18)
+#define LAN4_PHY2_LED_MAP			BIT(17)
+#define LAN4_PHY1_LED_MAP			BIT(16)
+#define LAN4_PHY0_LED_MAP			0
+#define LAN4_PHY3_LED_MAP			GENMASK(17, 16)
+
+#define LAN3_LED_MAPPING_MASK			GENMASK(14, 12)
+#define LAN3_PHY4_LED_MAP			BIT(14)
+#define LAN3_PHY2_LED_MAP			BIT(13)
+#define LAN3_PHY1_LED_MAP			BIT(12)
+#define LAN3_PHY0_LED_MAP			0
+#define LAN3_PHY3_LED_MAP			GENMASK(13, 12)
+
+#define LAN2_LED_MAPPING_MASK			GENMASK(10, 8)
+#define LAN2_PHY4_LED_MAP			BIT(12)
+#define LAN2_PHY2_LED_MAP			BIT(11)
+#define LAN2_PHY1_LED_MAP			BIT(10)
+#define LAN2_PHY0_LED_MAP			0
+#define LAN2_PHY3_LED_MAP			GENMASK(11, 10)
+
+#define LAN1_LED_MAPPING_MASK			GENMASK(6, 4)
+#define LAN1_PHY4_LED_MAP			BIT(6)
+#define LAN1_PHY2_LED_MAP			BIT(5)
+#define LAN1_PHY1_LED_MAP			BIT(4)
+#define LAN1_PHY0_LED_MAP			0
+#define LAN1_PHY3_LED_MAP			GENMASK(5, 4)
+
+#define LAN0_LED_MAPPING_MASK			GENMASK(2, 0)
+#define LAN0_PHY4_LED_MAP			BIT(3)
+#define LAN0_PHY2_LED_MAP			BIT(2)
+#define LAN0_PHY1_LED_MAP			BIT(1)
+#define LAN0_PHY0_LED_MAP			0
+#define LAN0_PHY3_LED_MAP			GENMASK(2, 1)
+
+/* CONF */
+#define REG_I2C_SDA_E2				0x00
+#define SPI_MISO_E2_MASK			BIT(14)
+#define SPI_MOSI_E2_MASK			BIT(13)
+#define SPI_CLK_E2_MASK				BIT(12)
+#define SPI_CS0_E2_MASK				BIT(11)
+#define PCIE2_RESET_E2_MASK			BIT(10)
+#define PCIE1_RESET_E2_MASK			BIT(9)
+#define PCIE0_RESET_E2_MASK			BIT(8)
+#define UART1_RXD_E2_MASK			BIT(3)
+#define UART1_TXD_E2_MASK			BIT(2)
+#define I2C_SCL_E2_MASK				BIT(1)
+#define I2C_SDA_E2_MASK				BIT(0)
+
+#define REG_I2C_SDA_E4				0x04
+#define SPI_MISO_E4_MASK			BIT(14)
+#define SPI_MOSI_E4_MASK			BIT(13)
+#define SPI_CLK_E4_MASK				BIT(12)
+#define SPI_CS0_E4_MASK				BIT(11)
+#define PCIE2_RESET_E4_MASK			BIT(10)
+#define PCIE1_RESET_E4_MASK			BIT(9)
+#define PCIE0_RESET_E4_MASK			BIT(8)
+#define UART1_RXD_E4_MASK			BIT(3)
+#define UART1_TXD_E4_MASK			BIT(2)
+#define I2C_SCL_E4_MASK				BIT(1)
+#define I2C_SDA_E4_MASK				BIT(0)
+
+#define REG_GPIO_L_E2				0x08
+#define REG_GPIO_L_E4				0x0c
+#define REG_GPIO_H_E2				0x10
+#define REG_GPIO_H_E4				0x14
+
+#define REG_I2C_SDA_PU				0x28
+#define SPI_MISO_PU_MASK			BIT(14)
+#define SPI_MOSI_PU_MASK			BIT(13)
+#define SPI_CLK_PU_MASK				BIT(12)
+#define SPI_CS0_PU_MASK				BIT(11)
+#define PCIE2_RESET_PU_MASK			BIT(10)
+#define PCIE1_RESET_PU_MASK			BIT(9)
+#define PCIE0_RESET_PU_MASK			BIT(8)
+#define UART1_RXD_PU_MASK			BIT(3)
+#define UART1_TXD_PU_MASK			BIT(2)
+#define I2C_SCL_PU_MASK				BIT(1)
+#define I2C_SDA_PU_MASK				BIT(0)
+
+#define REG_I2C_SDA_PD				0x2c
+#define SPI_MISO_PD_MASK			BIT(14)
+#define SPI_MOSI_PD_MASK			BIT(13)
+#define SPI_CLK_PD_MASK				BIT(12)
+#define SPI_CS0_PD_MASK				BIT(11)
+#define PCIE2_RESET_PD_MASK			BIT(10)
+#define PCIE1_RESET_PD_MASK			BIT(9)
+#define PCIE0_RESET_PD_MASK			BIT(8)
+#define UART1_RXD_PD_MASK			BIT(3)
+#define UART1_TXD_PD_MASK			BIT(2)
+#define I2C_SCL_PD_MASK				BIT(1)
+#define I2C_SDA_PD_MASK				BIT(0)
+
+#define REG_GPIO_L_PU				0x30
+#define REG_GPIO_L_PD				0x34
+#define REG_GPIO_H_PU				0x38
+#define REG_GPIO_H_PD				0x3c
+
+#define REG_PCIE_RESET_OD			0x00
+#define PCIE2_RESET_OD_MASK			BIT(2)
+#define PCIE1_RESET_OD_MASK			BIT(1)
+#define PCIE0_RESET_OD_MASK			BIT(0)
+
+/* PWM MODE CONF */
+#define REG_GPIO_FLASH_MODE_CFG			0x00
+#define GPIO15_FLASH_MODE_CFG			BIT(15)
+#define GPIO14_FLASH_MODE_CFG			BIT(14)
+#define GPIO13_FLASH_MODE_CFG			BIT(13)
+#define GPIO12_FLASH_MODE_CFG			BIT(12)
+#define GPIO11_FLASH_MODE_CFG			BIT(11)
+#define GPIO10_FLASH_MODE_CFG			BIT(10)
+#define GPIO9_FLASH_MODE_CFG			BIT(9)
+#define GPIO8_FLASH_MODE_CFG			BIT(8)
+#define GPIO7_FLASH_MODE_CFG			BIT(7)
+#define GPIO6_FLASH_MODE_CFG			BIT(6)
+#define GPIO5_FLASH_MODE_CFG			BIT(5)
+#define GPIO4_FLASH_MODE_CFG			BIT(4)
+#define GPIO3_FLASH_MODE_CFG			BIT(3)
+#define GPIO2_FLASH_MODE_CFG			BIT(2)
+#define GPIO1_FLASH_MODE_CFG			BIT(1)
+#define GPIO0_FLASH_MODE_CFG			BIT(0)
+
+/* PWM MODE CONF EXT */
+#define REG_GPIO_FLASH_MODE_CFG_EXT		0x00
+#define GPIO51_FLASH_MODE_CFG			BIT(31)
+#define GPIO50_FLASH_MODE_CFG			BIT(30)
+#define GPIO49_FLASH_MODE_CFG			BIT(29)
+#define GPIO48_FLASH_MODE_CFG			BIT(28)
+#define GPIO47_FLASH_MODE_CFG			BIT(27)
+#define GPIO46_FLASH_MODE_CFG			BIT(26)
+#define GPIO45_FLASH_MODE_CFG			BIT(25)
+#define GPIO44_FLASH_MODE_CFG			BIT(24)
+#define GPIO43_FLASH_MODE_CFG			BIT(23)
+#define GPIO42_FLASH_MODE_CFG			BIT(22)
+#define GPIO41_FLASH_MODE_CFG			BIT(21)
+#define GPIO40_FLASH_MODE_CFG			BIT(20)
+#define GPIO39_FLASH_MODE_CFG			BIT(19)
+#define GPIO38_FLASH_MODE_CFG			BIT(18)
+#define GPIO37_FLASH_MODE_CFG			BIT(17)
+#define GPIO36_FLASH_MODE_CFG			BIT(16)
+#define GPIO31_FLASH_MODE_CFG			BIT(15)
+#define GPIO30_FLASH_MODE_CFG			BIT(14)
+#define GPIO29_FLASH_MODE_CFG			BIT(13)
+#define GPIO28_FLASH_MODE_CFG			BIT(12)
+#define GPIO27_FLASH_MODE_CFG			BIT(11)
+#define GPIO26_FLASH_MODE_CFG			BIT(10)
+#define GPIO25_FLASH_MODE_CFG			BIT(9)
+#define GPIO24_FLASH_MODE_CFG			BIT(8)
+#define GPIO23_FLASH_MODE_CFG			BIT(7)
+#define GPIO22_FLASH_MODE_CFG			BIT(6)
+#define GPIO21_FLASH_MODE_CFG			BIT(5)
+#define GPIO20_FLASH_MODE_CFG			BIT(4)
+#define GPIO19_FLASH_MODE_CFG			BIT(3)
+#define GPIO18_FLASH_MODE_CFG			BIT(2)
+#define GPIO17_FLASH_MODE_CFG			BIT(1)
+#define GPIO16_FLASH_MODE_CFG			BIT(0)
+
+/* GPIOs */
+#define REG_GPIO_CTRL				0x00
+#define REG_GPIO_DATA				0x04
+#define REG_GPIO_INT				0x08
+#define REG_GPIO_INT_EDGE			0x0c
+#define REG_GPIO_INT_LEVEL			0x10
+#define REG_GPIO_OE				0x14
+
+#define REG_GPIO_CTRL1				0x00
+
+#define REG_GPIO_CTRL2				0x00
+#define REG_GPIO_CTRL3				0x04
+
+#define REG_GPIO_DATA1				0x00
+#define REG_GPIO_OE1				0x08
+#define REG_GPIO_INT1				0x0c
+#define REG_GPIO_INT_EDGE1			0x10
+#define REG_GPIO_INT_EDGE2			0x14
+#define REG_GPIO_INT_EDGE3			0x18
+#define REG_GPIO_INT_LEVEL1			0x1c
+#define REG_GPIO_INT_LEVEL2			0x20
+#define REG_GPIO_INT_LEVEL3			0x24
+
+#define AIROHA_NUM_GPIOS			64
+#define AIROHA_GPIO_BANK_SIZE			(AIROHA_NUM_GPIOS / 2)
+#define AIROHA_REG_GPIOCTRL_NUM_GPIO		(AIROHA_NUM_GPIOS / 4)
+
+struct airoha_pinctrl_reg {
+	u32 offset;
+	u32 mask;
+};
+
+enum airoha_pinctrl_mux_func {
+	AIROHA_FUNC_MUX,
+	AIROHA_FUNC_LED_MUX,
+	AIROHA_FUNC_PWM_MUX,
+	AIROHA_FUNC_PWM_EXT_MUX,
+};
+
+struct airoha_pinctrl_func_group {
+	const char *name;
+	struct {
+		enum airoha_pinctrl_mux_func mux;
+		u32 offset;
+		u32 mask;
+		u32 val;
+	} regmap[2];
+	int regmap_size;
+};
+
+struct airoha_pinctrl_func {
+	const struct function_desc desc;
+	const struct airoha_pinctrl_func_group *groups;
+	u8 group_size;
+};
+
+struct airoha_pinctrl_conf {
+	u32 pin;
+	struct airoha_pinctrl_reg reg;
+};
+
+struct airoha_pinctrl_gpiochip {
+	struct gpio_chip chip;
+
+	void __iomem *data[2];
+	void __iomem *dir[4];
+	void __iomem *out[2];
+
+	/* protect concurrent register accesses */
+	spinlock_t lock;
+	void __iomem *status[2];
+	void __iomem *level[4];
+	void __iomem *edge[4];
+
+	u32 irq_type[AIROHA_NUM_GPIOS];
+};
+
+struct airoha_pinctrl {
+	struct pinctrl_dev *ctrl;
+
+	/* protect concurrent register accesses */
+	struct mutex mutex;
+	struct {
+		void __iomem *mux[4];
+		void __iomem *conf;
+		void __iomem *pcie_rst;
+	} regs;
+
+	struct airoha_pinctrl_gpiochip gpiochip;
+};
+
+static struct pinctrl_pin_desc airoha_pinctrl_pins[] = {
+	PINCTRL_PIN(0, "uart1_txd"),
+	PINCTRL_PIN(1, "uart1_rxd"),
+	PINCTRL_PIN(2, "i2c_scl"),
+	PINCTRL_PIN(3, "i2c_sda"),
+	PINCTRL_PIN(4, "spi_cs0"),
+	PINCTRL_PIN(5, "spi_clk"),
+	PINCTRL_PIN(6, "spi_mosi"),
+	PINCTRL_PIN(7, "spi_miso"),
+	PINCTRL_PIN(13, "gpio0"),
+	PINCTRL_PIN(14, "gpio1"),
+	PINCTRL_PIN(15, "gpio2"),
+	PINCTRL_PIN(16, "gpio3"),
+	PINCTRL_PIN(17, "gpio4"),
+	PINCTRL_PIN(18, "gpio5"),
+	PINCTRL_PIN(19, "gpio6"),
+	PINCTRL_PIN(20, "gpio7"),
+	PINCTRL_PIN(21, "gpio8"),
+	PINCTRL_PIN(22, "gpio9"),
+	PINCTRL_PIN(23, "gpio10"),
+	PINCTRL_PIN(24, "gpio11"),
+	PINCTRL_PIN(25, "gpio12"),
+	PINCTRL_PIN(26, "gpio13"),
+	PINCTRL_PIN(27, "gpio14"),
+	PINCTRL_PIN(28, "gpio15"),
+	PINCTRL_PIN(29, "gpio16"),
+	PINCTRL_PIN(30, "gpio17"),
+	PINCTRL_PIN(31, "gpio18"),
+	PINCTRL_PIN(32, "gpio19"),
+	PINCTRL_PIN(33, "gpio20"),
+	PINCTRL_PIN(34, "gpio21"),
+	PINCTRL_PIN(35, "gpio22"),
+	PINCTRL_PIN(36, "gpio23"),
+	PINCTRL_PIN(37, "gpio24"),
+	PINCTRL_PIN(38, "gpio25"),
+	PINCTRL_PIN(39, "gpio26"),
+	PINCTRL_PIN(40, "gpio27"),
+	PINCTRL_PIN(41, "gpio28"),
+	PINCTRL_PIN(42, "gpio29"),
+	PINCTRL_PIN(43, "gpio30"),
+	PINCTRL_PIN(44, "gpio31"),
+	PINCTRL_PIN(45, "gpio32"),
+	PINCTRL_PIN(46, "gpio33"),
+	PINCTRL_PIN(47, "gpio34"),
+	PINCTRL_PIN(48, "gpio35"),
+	PINCTRL_PIN(49, "gpio36"),
+	PINCTRL_PIN(50, "gpio37"),
+	PINCTRL_PIN(51, "gpio38"),
+	PINCTRL_PIN(52, "gpio39"),
+	PINCTRL_PIN(53, "gpio40"),
+	PINCTRL_PIN(54, "gpio41"),
+	PINCTRL_PIN(55, "gpio42"),
+	PINCTRL_PIN(56, "gpio43"),
+	PINCTRL_PIN(57, "gpio44"),
+	PINCTRL_PIN(58, "gpio45"),
+	PINCTRL_PIN(59, "gpio46"),
+	PINCTRL_PIN(61, "pcie_reset0"),
+	PINCTRL_PIN(62, "pcie_reset1"),
+	PINCTRL_PIN(63, "pcie_reset2"),
+};
+
+static const int pon_pins[] = { 49, 50, 51, 52, 53, 54 };
+static const int pon_tod_1pps_pins[] = { 46 };
+static const int gsw_tod_1pps_pins[] = { 46 };
+static const int sipo_pins[] = { 16, 17 };
+static const int sipo_rclk_pins[] = { 16, 17, 43 };
+static const int mdio_pins[] = { 14, 15 };
+static const int uart2_pins[] = { 48, 55 };
+static const int uart2_cts_rts_pins[] = { 46, 47 };
+static const int hsuart_pins[] = { 28, 29 };
+static const int hsuart_cts_rts_pins[] = { 26, 27 };
+static const int uart4_pins[] = { 38, 39 };
+static const int uart5_pins[] = { 18, 19 };
+static const int i2c0_pins[] = { 2, 3 };
+static const int i2c1_pins[] = { 14, 15 };
+static const int jtag_udi_pins[] = { 16, 17, 18, 19, 20 };
+static const int jtag_dfd_pins[] = { 16, 17, 18, 19, 20 };
+static const int i2s_pins[] = { 26, 27, 28, 29 };
+static const int pcm1_pins[] = { 22, 23, 24, 25 };
+static const int pcm2_pins[] = { 18, 19, 20, 21 };
+static const int spi_quad_pins[] = { 32, 33 };
+static const int spi_pins[] = { 4, 5, 6, 7 };
+static const int spi_cs1_pins[] = { 34 };
+static const int pcm_spi_pins[] = { 18, 19, 20, 21, 22, 23, 24, 25 };
+static const int pcm_spi_int_pins[] = { 14 };
+static const int pcm_spi_rst_pins[] = { 15 };
+static const int pcm_spi_cs1_pins[] = { 43 };
+static const int pcm_spi_cs2_pins[] = { 40 };
+static const int pcm_spi_cs2_p128_pins[] = { 40 };
+static const int pcm_spi_cs2_p156_pins[] = { 40 };
+static const int pcm_spi_cs3_pins[] = { 41 };
+static const int pcm_spi_cs4_pins[] = { 42 };
+static const int emmc_pins[] = { 4, 5, 6, 30, 31, 32, 33, 34, 35, 36, 37 };
+static const int pnand_pins[] = { 4, 5, 6, 7, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42 };
+static const int gpio0_pins[] = { 13 };
+static const int gpio1_pins[] = { 14 };
+static const int gpio2_pins[] = { 15 };
+static const int gpio3_pins[] = { 16 };
+static const int gpio4_pins[] = { 17 };
+static const int gpio5_pins[] = { 18 };
+static const int gpio6_pins[] = { 19 };
+static const int gpio7_pins[] = { 20 };
+static const int gpio8_pins[] = { 21 };
+static const int gpio9_pins[] = { 22 };
+static const int gpio10_pins[] = { 23 };
+static const int gpio11_pins[] = { 24 };
+static const int gpio12_pins[] = { 25 };
+static const int gpio13_pins[] = { 26 };
+static const int gpio14_pins[] = { 27 };
+static const int gpio15_pins[] = { 28 };
+static const int gpio16_pins[] = { 29 };
+static const int gpio17_pins[] = { 30 };
+static const int gpio18_pins[] = { 31 };
+static const int gpio19_pins[] = { 32 };
+static const int gpio20_pins[] = { 33 };
+static const int gpio21_pins[] = { 34 };
+static const int gpio22_pins[] = { 35 };
+static const int gpio23_pins[] = { 36 };
+static const int gpio24_pins[] = { 37 };
+static const int gpio25_pins[] = { 38 };
+static const int gpio26_pins[] = { 39 };
+static const int gpio27_pins[] = { 40 };
+static const int gpio28_pins[] = { 41 };
+static const int gpio29_pins[] = { 42 };
+static const int gpio30_pins[] = { 43 };
+static const int gpio31_pins[] = { 44 };
+static const int gpio33_pins[] = { 46 };
+static const int gpio34_pins[] = { 47 };
+static const int gpio35_pins[] = { 48 };
+static const int gpio36_pins[] = { 49 };
+static const int gpio37_pins[] = { 50 };
+static const int gpio38_pins[] = { 51 };
+static const int gpio39_pins[] = { 52 };
+static const int gpio40_pins[] = { 53 };
+static const int gpio41_pins[] = { 54 };
+static const int gpio42_pins[] = { 55 };
+static const int gpio43_pins[] = { 56 };
+static const int gpio44_pins[] = { 57 };
+static const int gpio45_pins[] = { 58 };
+static const int gpio46_pins[] = { 59 };
+static const int pcie_reset0_pins[] = { 61 };
+static const int pcie_reset1_pins[] = { 62 };
+static const int pcie_reset2_pins[] = { 63 };
+
+static const struct pingroup airoha_pinctrl_groups[] = {
+	PINCTRL_PIN_GROUP(pon),
+	PINCTRL_PIN_GROUP(pon_tod_1pps),
+	PINCTRL_PIN_GROUP(gsw_tod_1pps),
+	PINCTRL_PIN_GROUP(sipo),
+	PINCTRL_PIN_GROUP(sipo_rclk),
+	PINCTRL_PIN_GROUP(mdio),
+	PINCTRL_PIN_GROUP(uart2),
+	PINCTRL_PIN_GROUP(uart2_cts_rts),
+	PINCTRL_PIN_GROUP(hsuart),
+	PINCTRL_PIN_GROUP(hsuart_cts_rts),
+	PINCTRL_PIN_GROUP(uart4),
+	PINCTRL_PIN_GROUP(uart5),
+	PINCTRL_PIN_GROUP(i2c0),
+	PINCTRL_PIN_GROUP(i2c1),
+	PINCTRL_PIN_GROUP(jtag_udi),
+	PINCTRL_PIN_GROUP(jtag_dfd),
+	PINCTRL_PIN_GROUP(i2s),
+	PINCTRL_PIN_GROUP(pcm1),
+	PINCTRL_PIN_GROUP(pcm2),
+	PINCTRL_PIN_GROUP(spi),
+	PINCTRL_PIN_GROUP(spi_quad),
+	PINCTRL_PIN_GROUP(spi_cs1),
+	PINCTRL_PIN_GROUP(pcm_spi),
+	PINCTRL_PIN_GROUP(pcm_spi_int),
+	PINCTRL_PIN_GROUP(pcm_spi_rst),
+	PINCTRL_PIN_GROUP(pcm_spi_cs1),
+	PINCTRL_PIN_GROUP(pcm_spi_cs2_p128),
+	PINCTRL_PIN_GROUP(pcm_spi_cs2_p156),
+	PINCTRL_PIN_GROUP(pcm_spi_cs2),
+	PINCTRL_PIN_GROUP(pcm_spi_cs3),
+	PINCTRL_PIN_GROUP(pcm_spi_cs4),
+	PINCTRL_PIN_GROUP(emmc),
+	PINCTRL_PIN_GROUP(pnand),
+	PINCTRL_PIN_GROUP(gpio0),
+	PINCTRL_PIN_GROUP(gpio1),
+	PINCTRL_PIN_GROUP(gpio2),
+	PINCTRL_PIN_GROUP(gpio3),
+	PINCTRL_PIN_GROUP(gpio4),
+	PINCTRL_PIN_GROUP(gpio5),
+	PINCTRL_PIN_GROUP(gpio6),
+	PINCTRL_PIN_GROUP(gpio7),
+	PINCTRL_PIN_GROUP(gpio8),
+	PINCTRL_PIN_GROUP(gpio9),
+	PINCTRL_PIN_GROUP(gpio10),
+	PINCTRL_PIN_GROUP(gpio11),
+	PINCTRL_PIN_GROUP(gpio12),
+	PINCTRL_PIN_GROUP(gpio13),
+	PINCTRL_PIN_GROUP(gpio14),
+	PINCTRL_PIN_GROUP(gpio15),
+	PINCTRL_PIN_GROUP(gpio16),
+	PINCTRL_PIN_GROUP(gpio17),
+	PINCTRL_PIN_GROUP(gpio18),
+	PINCTRL_PIN_GROUP(gpio19),
+	PINCTRL_PIN_GROUP(gpio20),
+	PINCTRL_PIN_GROUP(gpio21),
+	PINCTRL_PIN_GROUP(gpio22),
+	PINCTRL_PIN_GROUP(gpio23),
+	PINCTRL_PIN_GROUP(gpio24),
+	PINCTRL_PIN_GROUP(gpio25),
+	PINCTRL_PIN_GROUP(gpio26),
+	PINCTRL_PIN_GROUP(gpio27),
+	PINCTRL_PIN_GROUP(gpio28),
+	PINCTRL_PIN_GROUP(gpio29),
+	PINCTRL_PIN_GROUP(gpio30),
+	PINCTRL_PIN_GROUP(gpio31),
+	PINCTRL_PIN_GROUP(gpio33),
+	PINCTRL_PIN_GROUP(gpio34),
+	PINCTRL_PIN_GROUP(gpio35),
+	PINCTRL_PIN_GROUP(gpio36),
+	PINCTRL_PIN_GROUP(gpio37),
+	PINCTRL_PIN_GROUP(gpio38),
+	PINCTRL_PIN_GROUP(gpio39),
+	PINCTRL_PIN_GROUP(gpio40),
+	PINCTRL_PIN_GROUP(gpio41),
+	PINCTRL_PIN_GROUP(gpio42),
+	PINCTRL_PIN_GROUP(gpio43),
+	PINCTRL_PIN_GROUP(gpio44),
+	PINCTRL_PIN_GROUP(gpio45),
+	PINCTRL_PIN_GROUP(gpio46),
+	PINCTRL_PIN_GROUP(pcie_reset0),
+	PINCTRL_PIN_GROUP(pcie_reset1),
+	PINCTRL_PIN_GROUP(pcie_reset2),
+};
+
+static const char *const pon_groups[] = { "pon" };
+static const char *const tod_1pps_groups[] = { "pon_tod_1pps", "gsw_tod_1pps" };
+static const char *const sipo_groups[] = { "sipo", "sipo_rclk" };
+static const char *const mdio_groups[] = { "mdio" };
+static const char *const uart_groups[] = { "uart2", "uart2_cts_rts", "hsuart",
+					   "hsuart_cts_rts", "uart4",
+					   "uart5" };
+static const char *const i2c_groups[] = { "i2c1" };
+static const char *const jtag_groups[] = { "jtag_udi", "jtag_dfd" };
+static const char *const pcm_groups[] = { "pcm1", "pcm2" };
+static const char *const spi_groups[] = { "spi_quad", "spi_cs1" };
+static const char *const pcm_spi_groups[] = { "pcm_spi", "pcm_spi_int",
+					      "pcm_spi_rst", "pcm_spi_cs1",
+					      "pcm_spi_cs2_p156",
+					      "pcm_spi_cs2_p128",
+					      "pcm_spi_cs3", "pcm_spi_cs4" };
+static const char *const i2s_groups[] = { "i2s" };
+static const char *const emmc_groups[] = { "emmc" };
+static const char *const pnand_groups[] = { "pnand" };
+static const char *const pcie_reset_groups[] = { "pcie_reset0", "pcie_reset1",
+						 "pcie_reset2" };
+static const char *const pwm_groups[] = { "gpio0", "gpio1",
+					  "gpio2", "gpio3",
+					  "gpio4", "gpio5",
+					  "gpio6", "gpio7",
+					  "gpio8", "gpio9",
+					  "gpio10", "gpio11",
+					  "gpio12", "gpio13",
+					  "gpio14", "gpio15",
+					  "gpio16", "gpio17",
+					  "gpio18", "gpio19",
+					  "gpio20", "gpio21",
+					  "gpio22", "gpio23",
+					  "gpio24", "gpio25",
+					  "gpio26", "gpio27",
+					  "gpio28", "gpio29",
+					  "gpio30", "gpio31",
+					  "gpio36", "gpio37",
+					  "gpio38", "gpio39",
+					  "gpio40", "gpio41",
+					  "gpio42", "gpio43",
+					  "gpio44", "gpio45",
+					  "gpio46", "gpio47" };
+static const char *const phy1_led0_groups[] = { "gpio33", "gpio34",
+						"gpio35", "gpio42" };
+static const char *const phy2_led0_groups[] = { "gpio33", "gpio34",
+						"gpio35", "gpio42" };
+static const char *const phy3_led0_groups[] = { "gpio33", "gpio34",
+						"gpio35", "gpio42" };
+static const char *const phy4_led0_groups[] = { "gpio33", "gpio34",
+						"gpio35", "gpio42" };
+static const char *const phy1_led1_groups[] = { "gpio43", "gpio44",
+						"gpio45", "gpio46" };
+static const char *const phy2_led1_groups[] = { "gpio43", "gpio44",
+						"gpio45", "gpio46" };
+static const char *const phy3_led1_groups[] = { "gpio43", "gpio44",
+						"gpio45", "gpio46" };
+static const char *const phy4_led1_groups[] = { "gpio43", "gpio44",
+						"gpio45", "gpio46" };
+
+static const struct airoha_pinctrl_func_group pon_func_group[] = {
+	{
+		.name = "pon",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_PON_MODE_MASK,
+			GPIO_PON_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group tod_1pps_func_group[] = {
+	{
+		.name = "pon_tod_1pps",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			PON_TOD_1PPS_MODE_MASK,
+			PON_TOD_1PPS_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gsw_tod_1pps",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GSW_TOD_1PPS_MODE_MASK,
+			GSW_TOD_1PPS_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group sipo_func_group[] = {
+	{
+		.name = "sipo",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_SIPO_MODE_MASK | SIPO_RCLK_MODE_MASK,
+			GPIO_SIPO_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "sipo_rclk",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_SIPO_MODE_MASK | SIPO_RCLK_MODE_MASK,
+			GPIO_SIPO_MODE_MASK | SIPO_RCLK_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group mdio_func_group[] = {
+	{
+		.name = "mdio",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_SGMII_MDIO_MODE_MASK,
+			GPIO_SGMII_MDIO_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_MDC_IO_MASTER_MODE_MODE,
+			GPIO_MDC_IO_MASTER_MODE_MODE
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group uart_func_group[] = {
+	{
+		.name = "uart2",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_UART2_MODE_MASK,
+			GPIO_UART2_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "uart2_cts_rts",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_UART2_MODE_MASK | GPIO_UART2_CTS_RTS_MODE_MASK,
+			GPIO_UART2_MODE_MASK | GPIO_UART2_CTS_RTS_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "hsuart",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
+			GPIO_HSUART_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+	{
+		.name = "hsuart_cts_rts",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
+			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "uart4",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_UART4_MODE_MASK,
+			GPIO_UART4_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "uart5",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_UART5_MODE_MASK,
+			GPIO_UART5_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group i2c_func_group[] = {
+	{
+		.name = "i2c1",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_2ND_I2C_MODE_MASK,
+			GPIO_2ND_I2C_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group jtag_func_group[] = {
+	{
+		.name = "jtag_udi",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_NPU_UART_EN,
+			JTAG_UDI_EN_MASK,
+			JTAG_UDI_EN_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "jtag_dfd",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_NPU_UART_EN,
+			JTAG_DFD_EN_MASK,
+			JTAG_DFD_EN_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group pcm_func_group[] = {
+	{
+		.name = "pcm1",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM1_MODE_MASK,
+			GPIO_PCM1_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm2",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM2_MODE_MASK,
+			GPIO_PCM2_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group spi_func_group[] = {
+	{
+		.name = "spi_quad",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_SPI_QUAD_MODE_MASK,
+			GPIO_SPI_QUAD_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "spi_cs1",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_SPI_CS1_MODE_MASK,
+			GPIO_SPI_CS1_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "spi_cs2",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_SPI_CS2_MODE_MASK,
+			GPIO_SPI_CS2_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "spi_cs3",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_SPI_CS3_MODE_MASK,
+			GPIO_SPI_CS3_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "spi_cs4",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_SPI_CS4_MODE_MASK,
+			GPIO_SPI_CS4_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group pcm_spi_func_group[] = {
+	{
+		.name = "pcm_spi",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_MODE_MASK,
+			GPIO_PCM_SPI_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_int",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_INT_MODE_MASK,
+			GPIO_PCM_INT_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_rst",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_RESET_MODE_MASK,
+			GPIO_PCM_RESET_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_cs1",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_CS1_MODE_MASK,
+			GPIO_PCM_SPI_CS1_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_cs2_p128",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_CS2_MODE_P128_MASK,
+			GPIO_PCM_SPI_CS2_MODE_P128_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_cs2_p156",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_CS2_MODE_P156_MASK,
+			GPIO_PCM_SPI_CS2_MODE_P156_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_cs3",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_CS3_MODE_MASK,
+			GPIO_PCM_SPI_CS3_MODE_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcm_spi_cs4",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_SPI_CS1_MODE,
+			GPIO_PCM_SPI_CS4_MODE_MASK,
+			GPIO_PCM_SPI_CS4_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group i2s_func_group[] = {
+	{
+		.name = "i2s",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_I2S_MODE_MASK,
+			GPIO_I2S_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group emmc_func_group[] = {
+	{
+		.name = "emmc",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_EMMC_MODE_MASK,
+			GPIO_EMMC_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group pnand_func_group[] = {
+	{
+		.name = "pnand",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_PARALLEL_NAND_MODE_MASK,
+			GPIO_PARALLEL_NAND_MODE_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group pcie_reset_func_group[] = {
+	{
+		.name = "pcie_reset0",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_PCIE_RESET0_MASK,
+			GPIO_PCIE_RESET0_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcie_reset1",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_PCIE_RESET1_MASK,
+			GPIO_PCIE_RESET1_MASK
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "pcie_reset2",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_PON_MODE,
+			GPIO_PCIE_RESET2_MASK,
+			GPIO_PCIE_RESET2_MASK
+		},
+		.regmap_size = 1,
+	},
+};
+
+/* PWM */
+static const struct airoha_pinctrl_func_group pwm_func_group[] = {
+	{
+		.name = "gpio0",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO0_FLASH_MODE_CFG,
+			GPIO0_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio1",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO1_FLASH_MODE_CFG,
+			GPIO1_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio2",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO2_FLASH_MODE_CFG,
+			GPIO2_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio3",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO3_FLASH_MODE_CFG,
+			GPIO3_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio4",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO4_FLASH_MODE_CFG,
+			GPIO4_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio5",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO5_FLASH_MODE_CFG,
+			GPIO5_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio6",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO6_FLASH_MODE_CFG,
+			GPIO6_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio7",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO7_FLASH_MODE_CFG,
+			GPIO7_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio8",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO8_FLASH_MODE_CFG,
+			GPIO8_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio9",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO9_FLASH_MODE_CFG,
+			GPIO9_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio10",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO10_FLASH_MODE_CFG,
+			GPIO10_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio11",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO11_FLASH_MODE_CFG,
+			GPIO11_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio12",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO12_FLASH_MODE_CFG,
+			GPIO12_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio13",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO13_FLASH_MODE_CFG,
+			GPIO13_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio14",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO14_FLASH_MODE_CFG,
+			GPIO14_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio15",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_MUX,
+			REG_GPIO_FLASH_MODE_CFG,
+			GPIO15_FLASH_MODE_CFG,
+			GPIO15_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio16",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO16_FLASH_MODE_CFG,
+			GPIO16_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio17",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO17_FLASH_MODE_CFG,
+			GPIO17_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio18",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO18_FLASH_MODE_CFG,
+			GPIO18_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio19",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO19_FLASH_MODE_CFG,
+			GPIO19_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio20",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO20_FLASH_MODE_CFG,
+			GPIO20_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio21",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO21_FLASH_MODE_CFG,
+			GPIO21_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio22",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO22_FLASH_MODE_CFG,
+			GPIO22_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio23",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO23_FLASH_MODE_CFG,
+			GPIO23_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio24",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO24_FLASH_MODE_CFG,
+			GPIO24_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio25",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO25_FLASH_MODE_CFG,
+			GPIO25_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio26",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO26_FLASH_MODE_CFG,
+			GPIO26_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio27",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO27_FLASH_MODE_CFG,
+			GPIO27_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio28",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO28_FLASH_MODE_CFG,
+			GPIO28_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio29",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO29_FLASH_MODE_CFG,
+			GPIO29_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio30",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO30_FLASH_MODE_CFG,
+			GPIO30_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio31",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO31_FLASH_MODE_CFG,
+			GPIO31_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio36",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO36_FLASH_MODE_CFG,
+			GPIO36_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio37",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO37_FLASH_MODE_CFG,
+			GPIO37_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio38",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO38_FLASH_MODE_CFG,
+			GPIO38_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio39",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO39_FLASH_MODE_CFG,
+			GPIO39_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio40",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO40_FLASH_MODE_CFG,
+			GPIO40_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio41",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO41_FLASH_MODE_CFG,
+			GPIO41_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio42",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO42_FLASH_MODE_CFG,
+			GPIO42_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio43",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO43_FLASH_MODE_CFG,
+			GPIO43_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio44",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO44_FLASH_MODE_CFG,
+			GPIO44_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio45",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO45_FLASH_MODE_CFG,
+			GPIO45_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio46",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO46_FLASH_MODE_CFG,
+			GPIO46_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	}, {
+		.name = "gpio47",
+		.regmap[0] = {
+			AIROHA_FUNC_PWM_EXT_MUX,
+			REG_GPIO_FLASH_MODE_CFG_EXT,
+			GPIO47_FLASH_MODE_CFG,
+			GPIO47_FLASH_MODE_CFG
+		},
+		.regmap_size = 1,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy1_led0_func_group[] = {
+	{
+		.name = "gpio33",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED0_MODE_MASK,
+			GPIO_LAN0_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio34",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED0_MODE_MASK,
+			GPIO_LAN1_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio35",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED0_MODE_MASK,
+			GPIO_LAN2_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio42",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy2_led0_func_group[] = {
+	{
+		.name = "gpio33",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED0_MODE_MASK,
+			GPIO_LAN0_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio34",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED0_MODE_MASK,
+			GPIO_LAN1_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio35",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED0_MODE_MASK,
+			GPIO_LAN2_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio42",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy3_led0_func_group[] = {
+	{
+		.name = "gpio33",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED0_MODE_MASK,
+			GPIO_LAN0_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio34",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED0_MODE_MASK,
+			GPIO_LAN1_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio35",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED0_MODE_MASK,
+			GPIO_LAN2_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio42",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy4_led0_func_group[] = {
+	{
+		.name = "gpio33",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED0_MODE_MASK,
+			GPIO_LAN0_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio34",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED0_MODE_MASK,
+			GPIO_LAN1_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio35",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED0_MODE_MASK,
+			GPIO_LAN2_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio42",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED0_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy1_led1_func_group[] = {
+	{
+		.name = "gpio43",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED1_MODE_MASK,
+			GPIO_LAN0_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio44",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED1_MODE_MASK,
+			GPIO_LAN1_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio45",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED1_MODE_MASK,
+			GPIO_LAN2_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio46",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY1_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy2_led1_func_group[] = {
+	{
+		.name = "gpio43",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED1_MODE_MASK,
+			GPIO_LAN0_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio44",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED1_MODE_MASK,
+			GPIO_LAN1_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio45",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED1_MODE_MASK,
+			GPIO_LAN2_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio46",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY2_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy3_led1_func_group[] = {
+	{
+		.name = "gpio43",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED1_MODE_MASK,
+			GPIO_LAN0_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio44",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED1_MODE_MASK,
+			GPIO_LAN1_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio45",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED1_MODE_MASK,
+			GPIO_LAN2_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio46",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY3_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func_group phy4_led1_func_group[] = {
+	{
+		.name = "gpio43",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN0_LED1_MODE_MASK,
+			GPIO_LAN0_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN1_LED_MAPPING_MASK,
+			LAN1_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio44",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN1_LED1_MODE_MASK,
+			GPIO_LAN1_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN2_LED_MAPPING_MASK,
+			LAN2_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio45",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN2_LED1_MODE_MASK,
+			GPIO_LAN2_LED1_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN3_LED_MAPPING_MASK,
+			LAN3_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	}, {
+		.name = "gpio46",
+		.regmap[0] = {
+			AIROHA_FUNC_MUX,
+			REG_GPIO_2ND_I2C_MODE,
+			GPIO_LAN3_LED0_MODE_MASK,
+			GPIO_LAN3_LED0_MODE_MASK
+		},
+		.regmap[1] = {
+			AIROHA_FUNC_LED_MUX,
+			REG_LAN_LED1_MAPPING,
+			LAN4_LED_MAPPING_MASK,
+			LAN4_PHY4_LED_MAP
+		},
+		.regmap_size = 2,
+	},
+};
+
+static const struct airoha_pinctrl_func airoha_pinctrl_funcs[] = {
+	PINCTRL_FUNC_DESC(pon),
+	PINCTRL_FUNC_DESC(tod_1pps),
+	PINCTRL_FUNC_DESC(sipo),
+	PINCTRL_FUNC_DESC(mdio),
+	PINCTRL_FUNC_DESC(uart),
+	PINCTRL_FUNC_DESC(i2c),
+	PINCTRL_FUNC_DESC(jtag),
+	PINCTRL_FUNC_DESC(pcm),
+	PINCTRL_FUNC_DESC(spi),
+	PINCTRL_FUNC_DESC(pcm_spi),
+	PINCTRL_FUNC_DESC(i2s),
+	PINCTRL_FUNC_DESC(emmc),
+	PINCTRL_FUNC_DESC(pnand),
+	PINCTRL_FUNC_DESC(pcie_reset),
+	PINCTRL_FUNC_DESC(pwm),
+	PINCTRL_FUNC_DESC(phy1_led0),
+	PINCTRL_FUNC_DESC(phy2_led0),
+	PINCTRL_FUNC_DESC(phy3_led0),
+	PINCTRL_FUNC_DESC(phy4_led0),
+	PINCTRL_FUNC_DESC(phy1_led1),
+	PINCTRL_FUNC_DESC(phy2_led1),
+	PINCTRL_FUNC_DESC(phy3_led1),
+	PINCTRL_FUNC_DESC(phy4_led1),
+};
+
+static const struct airoha_pinctrl_conf airoha_pinctrl_pullup_conf[] = {
+	PINCTRL_CONF_DESC(0, REG_I2C_SDA_PU, UART1_TXD_PU_MASK),
+	PINCTRL_CONF_DESC(1, REG_I2C_SDA_PU, UART1_RXD_PU_MASK),
+	PINCTRL_CONF_DESC(2, REG_I2C_SDA_PU, I2C_SDA_PU_MASK),
+	PINCTRL_CONF_DESC(3, REG_I2C_SDA_PU, I2C_SCL_PU_MASK),
+	PINCTRL_CONF_DESC(4, REG_I2C_SDA_PU, SPI_CS0_PU_MASK),
+	PINCTRL_CONF_DESC(5, REG_I2C_SDA_PU, SPI_CLK_PU_MASK),
+	PINCTRL_CONF_DESC(6, REG_I2C_SDA_PU, SPI_MOSI_PU_MASK),
+	PINCTRL_CONF_DESC(7, REG_I2C_SDA_PU, SPI_MISO_PU_MASK),
+	PINCTRL_CONF_DESC(13, REG_GPIO_L_PU, BIT(0)),
+	PINCTRL_CONF_DESC(14, REG_GPIO_L_PU, BIT(1)),
+	PINCTRL_CONF_DESC(15, REG_GPIO_L_PU, BIT(2)),
+	PINCTRL_CONF_DESC(16, REG_GPIO_L_PU, BIT(3)),
+	PINCTRL_CONF_DESC(17, REG_GPIO_L_PU, BIT(4)),
+	PINCTRL_CONF_DESC(18, REG_GPIO_L_PU, BIT(5)),
+	PINCTRL_CONF_DESC(19, REG_GPIO_L_PU, BIT(6)),
+	PINCTRL_CONF_DESC(20, REG_GPIO_L_PU, BIT(7)),
+	PINCTRL_CONF_DESC(21, REG_GPIO_L_PU, BIT(8)),
+	PINCTRL_CONF_DESC(22, REG_GPIO_L_PU, BIT(9)),
+	PINCTRL_CONF_DESC(23, REG_GPIO_L_PU, BIT(10)),
+	PINCTRL_CONF_DESC(24, REG_GPIO_L_PU, BIT(11)),
+	PINCTRL_CONF_DESC(25, REG_GPIO_L_PU, BIT(12)),
+	PINCTRL_CONF_DESC(26, REG_GPIO_L_PU, BIT(13)),
+	PINCTRL_CONF_DESC(27, REG_GPIO_L_PU, BIT(14)),
+	PINCTRL_CONF_DESC(28, REG_GPIO_L_PU, BIT(15)),
+	PINCTRL_CONF_DESC(29, REG_GPIO_L_PU, BIT(16)),
+	PINCTRL_CONF_DESC(30, REG_GPIO_L_PU, BIT(17)),
+	PINCTRL_CONF_DESC(31, REG_GPIO_L_PU, BIT(18)),
+	PINCTRL_CONF_DESC(32, REG_GPIO_L_PU, BIT(18)),
+	PINCTRL_CONF_DESC(33, REG_GPIO_L_PU, BIT(20)),
+	PINCTRL_CONF_DESC(34, REG_GPIO_L_PU, BIT(21)),
+	PINCTRL_CONF_DESC(35, REG_GPIO_L_PU, BIT(22)),
+	PINCTRL_CONF_DESC(36, REG_GPIO_L_PU, BIT(23)),
+	PINCTRL_CONF_DESC(37, REG_GPIO_L_PU, BIT(24)),
+	PINCTRL_CONF_DESC(38, REG_GPIO_L_PU, BIT(25)),
+	PINCTRL_CONF_DESC(39, REG_GPIO_L_PU, BIT(26)),
+	PINCTRL_CONF_DESC(40, REG_GPIO_L_PU, BIT(27)),
+	PINCTRL_CONF_DESC(41, REG_GPIO_L_PU, BIT(28)),
+	PINCTRL_CONF_DESC(42, REG_GPIO_L_PU, BIT(29)),
+	PINCTRL_CONF_DESC(43, REG_GPIO_L_PU, BIT(30)),
+	PINCTRL_CONF_DESC(44, REG_GPIO_L_PU, BIT(31)),
+	PINCTRL_CONF_DESC(45, REG_GPIO_H_PU, BIT(0)),
+	PINCTRL_CONF_DESC(46, REG_GPIO_H_PU, BIT(1)),
+	PINCTRL_CONF_DESC(47, REG_GPIO_H_PU, BIT(2)),
+	PINCTRL_CONF_DESC(48, REG_GPIO_H_PU, BIT(3)),
+	PINCTRL_CONF_DESC(49, REG_GPIO_H_PU, BIT(4)),
+	PINCTRL_CONF_DESC(50, REG_GPIO_H_PU, BIT(5)),
+	PINCTRL_CONF_DESC(51, REG_GPIO_H_PU, BIT(6)),
+	PINCTRL_CONF_DESC(52, REG_GPIO_H_PU, BIT(7)),
+	PINCTRL_CONF_DESC(53, REG_GPIO_H_PU, BIT(8)),
+	PINCTRL_CONF_DESC(54, REG_GPIO_H_PU, BIT(9)),
+	PINCTRL_CONF_DESC(55, REG_GPIO_H_PU, BIT(10)),
+	PINCTRL_CONF_DESC(56, REG_GPIO_H_PU, BIT(11)),
+	PINCTRL_CONF_DESC(57, REG_GPIO_H_PU, BIT(12)),
+	PINCTRL_CONF_DESC(58, REG_GPIO_H_PU, BIT(13)),
+	PINCTRL_CONF_DESC(59, REG_GPIO_H_PU, BIT(14)),
+	PINCTRL_CONF_DESC(61, REG_I2C_SDA_PU, PCIE0_RESET_PU_MASK),
+	PINCTRL_CONF_DESC(62, REG_I2C_SDA_PU, PCIE1_RESET_PU_MASK),
+	PINCTRL_CONF_DESC(63, REG_I2C_SDA_PU, PCIE2_RESET_PU_MASK),
+};
+
+static const struct airoha_pinctrl_conf airoha_pinctrl_pulldown_conf[] = {
+	PINCTRL_CONF_DESC(0, REG_I2C_SDA_PD, UART1_TXD_PD_MASK),
+	PINCTRL_CONF_DESC(1, REG_I2C_SDA_PD, UART1_RXD_PD_MASK),
+	PINCTRL_CONF_DESC(2, REG_I2C_SDA_PD, I2C_SDA_PD_MASK),
+	PINCTRL_CONF_DESC(3, REG_I2C_SDA_PD, I2C_SCL_PD_MASK),
+	PINCTRL_CONF_DESC(4, REG_I2C_SDA_PD, SPI_CS0_PD_MASK),
+	PINCTRL_CONF_DESC(5, REG_I2C_SDA_PD, SPI_CLK_PD_MASK),
+	PINCTRL_CONF_DESC(6, REG_I2C_SDA_PD, SPI_MOSI_PD_MASK),
+	PINCTRL_CONF_DESC(7, REG_I2C_SDA_PD, SPI_MISO_PD_MASK),
+	PINCTRL_CONF_DESC(13, REG_GPIO_L_PD, BIT(0)),
+	PINCTRL_CONF_DESC(14, REG_GPIO_L_PD, BIT(1)),
+	PINCTRL_CONF_DESC(15, REG_GPIO_L_PD, BIT(2)),
+	PINCTRL_CONF_DESC(16, REG_GPIO_L_PD, BIT(3)),
+	PINCTRL_CONF_DESC(17, REG_GPIO_L_PD, BIT(4)),
+	PINCTRL_CONF_DESC(18, REG_GPIO_L_PD, BIT(5)),
+	PINCTRL_CONF_DESC(19, REG_GPIO_L_PD, BIT(6)),
+	PINCTRL_CONF_DESC(20, REG_GPIO_L_PD, BIT(7)),
+	PINCTRL_CONF_DESC(21, REG_GPIO_L_PD, BIT(8)),
+	PINCTRL_CONF_DESC(22, REG_GPIO_L_PD, BIT(9)),
+	PINCTRL_CONF_DESC(23, REG_GPIO_L_PD, BIT(10)),
+	PINCTRL_CONF_DESC(24, REG_GPIO_L_PD, BIT(11)),
+	PINCTRL_CONF_DESC(25, REG_GPIO_L_PD, BIT(12)),
+	PINCTRL_CONF_DESC(26, REG_GPIO_L_PD, BIT(13)),
+	PINCTRL_CONF_DESC(27, REG_GPIO_L_PD, BIT(14)),
+	PINCTRL_CONF_DESC(28, REG_GPIO_L_PD, BIT(15)),
+	PINCTRL_CONF_DESC(29, REG_GPIO_L_PD, BIT(16)),
+	PINCTRL_CONF_DESC(30, REG_GPIO_L_PD, BIT(17)),
+	PINCTRL_CONF_DESC(31, REG_GPIO_L_PD, BIT(18)),
+	PINCTRL_CONF_DESC(32, REG_GPIO_L_PD, BIT(18)),
+	PINCTRL_CONF_DESC(33, REG_GPIO_L_PD, BIT(20)),
+	PINCTRL_CONF_DESC(34, REG_GPIO_L_PD, BIT(21)),
+	PINCTRL_CONF_DESC(35, REG_GPIO_L_PD, BIT(22)),
+	PINCTRL_CONF_DESC(36, REG_GPIO_L_PD, BIT(23)),
+	PINCTRL_CONF_DESC(37, REG_GPIO_L_PD, BIT(24)),
+	PINCTRL_CONF_DESC(38, REG_GPIO_L_PD, BIT(25)),
+	PINCTRL_CONF_DESC(39, REG_GPIO_L_PD, BIT(26)),
+	PINCTRL_CONF_DESC(40, REG_GPIO_L_PD, BIT(27)),
+	PINCTRL_CONF_DESC(41, REG_GPIO_L_PD, BIT(28)),
+	PINCTRL_CONF_DESC(42, REG_GPIO_L_PD, BIT(29)),
+	PINCTRL_CONF_DESC(43, REG_GPIO_L_PD, BIT(30)),
+	PINCTRL_CONF_DESC(44, REG_GPIO_L_PD, BIT(31)),
+	PINCTRL_CONF_DESC(45, REG_GPIO_H_PD, BIT(0)),
+	PINCTRL_CONF_DESC(46, REG_GPIO_H_PD, BIT(1)),
+	PINCTRL_CONF_DESC(47, REG_GPIO_H_PD, BIT(2)),
+	PINCTRL_CONF_DESC(48, REG_GPIO_H_PD, BIT(3)),
+	PINCTRL_CONF_DESC(49, REG_GPIO_H_PD, BIT(4)),
+	PINCTRL_CONF_DESC(50, REG_GPIO_H_PD, BIT(5)),
+	PINCTRL_CONF_DESC(51, REG_GPIO_H_PD, BIT(6)),
+	PINCTRL_CONF_DESC(52, REG_GPIO_H_PD, BIT(7)),
+	PINCTRL_CONF_DESC(53, REG_GPIO_H_PD, BIT(8)),
+	PINCTRL_CONF_DESC(54, REG_GPIO_H_PD, BIT(9)),
+	PINCTRL_CONF_DESC(55, REG_GPIO_H_PD, BIT(10)),
+	PINCTRL_CONF_DESC(56, REG_GPIO_H_PD, BIT(11)),
+	PINCTRL_CONF_DESC(57, REG_GPIO_H_PD, BIT(12)),
+	PINCTRL_CONF_DESC(58, REG_GPIO_H_PD, BIT(13)),
+	PINCTRL_CONF_DESC(59, REG_GPIO_H_PD, BIT(14)),
+	PINCTRL_CONF_DESC(61, REG_I2C_SDA_PD, PCIE0_RESET_PD_MASK),
+	PINCTRL_CONF_DESC(62, REG_I2C_SDA_PD, PCIE1_RESET_PD_MASK),
+	PINCTRL_CONF_DESC(63, REG_I2C_SDA_PD, PCIE2_RESET_PD_MASK),
+};
+
+static const struct airoha_pinctrl_conf airoha_pinctrl_drive_e2_conf[] = {
+	PINCTRL_CONF_DESC(0, REG_I2C_SDA_E2, UART1_TXD_E2_MASK),
+	PINCTRL_CONF_DESC(1, REG_I2C_SDA_E2, UART1_RXD_E2_MASK),
+	PINCTRL_CONF_DESC(2, REG_I2C_SDA_E2, I2C_SDA_E2_MASK),
+	PINCTRL_CONF_DESC(3, REG_I2C_SDA_E2, I2C_SCL_E2_MASK),
+	PINCTRL_CONF_DESC(4, REG_I2C_SDA_E2, SPI_CS0_E2_MASK),
+	PINCTRL_CONF_DESC(5, REG_I2C_SDA_E2, SPI_CLK_E2_MASK),
+	PINCTRL_CONF_DESC(6, REG_I2C_SDA_E2, SPI_MOSI_E2_MASK),
+	PINCTRL_CONF_DESC(7, REG_I2C_SDA_E2, SPI_MISO_E2_MASK),
+	PINCTRL_CONF_DESC(13, REG_GPIO_L_E2, BIT(0)),
+	PINCTRL_CONF_DESC(14, REG_GPIO_L_E2, BIT(1)),
+	PINCTRL_CONF_DESC(15, REG_GPIO_L_E2, BIT(2)),
+	PINCTRL_CONF_DESC(16, REG_GPIO_L_E2, BIT(3)),
+	PINCTRL_CONF_DESC(17, REG_GPIO_L_E2, BIT(4)),
+	PINCTRL_CONF_DESC(18, REG_GPIO_L_E2, BIT(5)),
+	PINCTRL_CONF_DESC(19, REG_GPIO_L_E2, BIT(6)),
+	PINCTRL_CONF_DESC(20, REG_GPIO_L_E2, BIT(7)),
+	PINCTRL_CONF_DESC(21, REG_GPIO_L_E2, BIT(8)),
+	PINCTRL_CONF_DESC(22, REG_GPIO_L_E2, BIT(9)),
+	PINCTRL_CONF_DESC(23, REG_GPIO_L_E2, BIT(10)),
+	PINCTRL_CONF_DESC(24, REG_GPIO_L_E2, BIT(11)),
+	PINCTRL_CONF_DESC(25, REG_GPIO_L_E2, BIT(12)),
+	PINCTRL_CONF_DESC(26, REG_GPIO_L_E2, BIT(13)),
+	PINCTRL_CONF_DESC(27, REG_GPIO_L_E2, BIT(14)),
+	PINCTRL_CONF_DESC(28, REG_GPIO_L_E2, BIT(15)),
+	PINCTRL_CONF_DESC(29, REG_GPIO_L_E2, BIT(16)),
+	PINCTRL_CONF_DESC(30, REG_GPIO_L_E2, BIT(17)),
+	PINCTRL_CONF_DESC(31, REG_GPIO_L_E2, BIT(18)),
+	PINCTRL_CONF_DESC(32, REG_GPIO_L_E2, BIT(18)),
+	PINCTRL_CONF_DESC(33, REG_GPIO_L_E2, BIT(20)),
+	PINCTRL_CONF_DESC(34, REG_GPIO_L_E2, BIT(21)),
+	PINCTRL_CONF_DESC(35, REG_GPIO_L_E2, BIT(22)),
+	PINCTRL_CONF_DESC(36, REG_GPIO_L_E2, BIT(23)),
+	PINCTRL_CONF_DESC(37, REG_GPIO_L_E2, BIT(24)),
+	PINCTRL_CONF_DESC(38, REG_GPIO_L_E2, BIT(25)),
+	PINCTRL_CONF_DESC(39, REG_GPIO_L_E2, BIT(26)),
+	PINCTRL_CONF_DESC(40, REG_GPIO_L_E2, BIT(27)),
+	PINCTRL_CONF_DESC(41, REG_GPIO_L_E2, BIT(28)),
+	PINCTRL_CONF_DESC(42, REG_GPIO_L_E2, BIT(29)),
+	PINCTRL_CONF_DESC(43, REG_GPIO_L_E2, BIT(30)),
+	PINCTRL_CONF_DESC(44, REG_GPIO_L_E2, BIT(31)),
+	PINCTRL_CONF_DESC(45, REG_GPIO_H_E2, BIT(0)),
+	PINCTRL_CONF_DESC(46, REG_GPIO_H_E2, BIT(1)),
+	PINCTRL_CONF_DESC(47, REG_GPIO_H_E2, BIT(2)),
+	PINCTRL_CONF_DESC(48, REG_GPIO_H_E2, BIT(3)),
+	PINCTRL_CONF_DESC(49, REG_GPIO_H_E2, BIT(4)),
+	PINCTRL_CONF_DESC(50, REG_GPIO_H_E2, BIT(5)),
+	PINCTRL_CONF_DESC(51, REG_GPIO_H_E2, BIT(6)),
+	PINCTRL_CONF_DESC(52, REG_GPIO_H_E2, BIT(7)),
+	PINCTRL_CONF_DESC(53, REG_GPIO_H_E2, BIT(8)),
+	PINCTRL_CONF_DESC(54, REG_GPIO_H_E2, BIT(9)),
+	PINCTRL_CONF_DESC(55, REG_GPIO_H_E2, BIT(10)),
+	PINCTRL_CONF_DESC(56, REG_GPIO_H_E2, BIT(11)),
+	PINCTRL_CONF_DESC(57, REG_GPIO_H_E2, BIT(12)),
+	PINCTRL_CONF_DESC(58, REG_GPIO_H_E2, BIT(13)),
+	PINCTRL_CONF_DESC(59, REG_GPIO_H_E2, BIT(14)),
+	PINCTRL_CONF_DESC(61, REG_I2C_SDA_E2, PCIE0_RESET_E2_MASK),
+	PINCTRL_CONF_DESC(62, REG_I2C_SDA_E2, PCIE1_RESET_E2_MASK),
+	PINCTRL_CONF_DESC(63, REG_I2C_SDA_E2, PCIE2_RESET_E2_MASK),
+};
+
+static const struct airoha_pinctrl_conf airoha_pinctrl_drive_e4_conf[] = {
+	PINCTRL_CONF_DESC(0, REG_I2C_SDA_E4, UART1_TXD_E4_MASK),
+	PINCTRL_CONF_DESC(1, REG_I2C_SDA_E4, UART1_RXD_E4_MASK),
+	PINCTRL_CONF_DESC(2, REG_I2C_SDA_E4, I2C_SDA_E4_MASK),
+	PINCTRL_CONF_DESC(3, REG_I2C_SDA_E4, I2C_SCL_E4_MASK),
+	PINCTRL_CONF_DESC(4, REG_I2C_SDA_E4, SPI_CS0_E4_MASK),
+	PINCTRL_CONF_DESC(5, REG_I2C_SDA_E4, SPI_CLK_E4_MASK),
+	PINCTRL_CONF_DESC(6, REG_I2C_SDA_E4, SPI_MOSI_E4_MASK),
+	PINCTRL_CONF_DESC(7, REG_I2C_SDA_E4, SPI_MISO_E4_MASK),
+	PINCTRL_CONF_DESC(13, REG_GPIO_L_E4, BIT(0)),
+	PINCTRL_CONF_DESC(14, REG_GPIO_L_E4, BIT(1)),
+	PINCTRL_CONF_DESC(15, REG_GPIO_L_E4, BIT(2)),
+	PINCTRL_CONF_DESC(16, REG_GPIO_L_E4, BIT(3)),
+	PINCTRL_CONF_DESC(17, REG_GPIO_L_E4, BIT(4)),
+	PINCTRL_CONF_DESC(18, REG_GPIO_L_E4, BIT(5)),
+	PINCTRL_CONF_DESC(19, REG_GPIO_L_E4, BIT(6)),
+	PINCTRL_CONF_DESC(20, REG_GPIO_L_E4, BIT(7)),
+	PINCTRL_CONF_DESC(21, REG_GPIO_L_E4, BIT(8)),
+	PINCTRL_CONF_DESC(22, REG_GPIO_L_E4, BIT(9)),
+	PINCTRL_CONF_DESC(23, REG_GPIO_L_E4, BIT(10)),
+	PINCTRL_CONF_DESC(24, REG_GPIO_L_E4, BIT(11)),
+	PINCTRL_CONF_DESC(25, REG_GPIO_L_E4, BIT(12)),
+	PINCTRL_CONF_DESC(26, REG_GPIO_L_E4, BIT(13)),
+	PINCTRL_CONF_DESC(27, REG_GPIO_L_E4, BIT(14)),
+	PINCTRL_CONF_DESC(28, REG_GPIO_L_E4, BIT(15)),
+	PINCTRL_CONF_DESC(29, REG_GPIO_L_E4, BIT(16)),
+	PINCTRL_CONF_DESC(30, REG_GPIO_L_E4, BIT(17)),
+	PINCTRL_CONF_DESC(31, REG_GPIO_L_E4, BIT(18)),
+	PINCTRL_CONF_DESC(32, REG_GPIO_L_E4, BIT(18)),
+	PINCTRL_CONF_DESC(33, REG_GPIO_L_E4, BIT(20)),
+	PINCTRL_CONF_DESC(34, REG_GPIO_L_E4, BIT(21)),
+	PINCTRL_CONF_DESC(35, REG_GPIO_L_E4, BIT(22)),
+	PINCTRL_CONF_DESC(36, REG_GPIO_L_E4, BIT(23)),
+	PINCTRL_CONF_DESC(37, REG_GPIO_L_E4, BIT(24)),
+	PINCTRL_CONF_DESC(38, REG_GPIO_L_E4, BIT(25)),
+	PINCTRL_CONF_DESC(39, REG_GPIO_L_E4, BIT(26)),
+	PINCTRL_CONF_DESC(40, REG_GPIO_L_E4, BIT(27)),
+	PINCTRL_CONF_DESC(41, REG_GPIO_L_E4, BIT(28)),
+	PINCTRL_CONF_DESC(42, REG_GPIO_L_E4, BIT(29)),
+	PINCTRL_CONF_DESC(43, REG_GPIO_L_E4, BIT(30)),
+	PINCTRL_CONF_DESC(44, REG_GPIO_L_E4, BIT(31)),
+	PINCTRL_CONF_DESC(45, REG_GPIO_H_E4, BIT(0)),
+	PINCTRL_CONF_DESC(46, REG_GPIO_H_E4, BIT(1)),
+	PINCTRL_CONF_DESC(47, REG_GPIO_H_E4, BIT(2)),
+	PINCTRL_CONF_DESC(48, REG_GPIO_H_E4, BIT(3)),
+	PINCTRL_CONF_DESC(49, REG_GPIO_H_E4, BIT(4)),
+	PINCTRL_CONF_DESC(50, REG_GPIO_H_E4, BIT(5)),
+	PINCTRL_CONF_DESC(51, REG_GPIO_H_E4, BIT(6)),
+	PINCTRL_CONF_DESC(52, REG_GPIO_H_E4, BIT(7)),
+	PINCTRL_CONF_DESC(53, REG_GPIO_H_E4, BIT(8)),
+	PINCTRL_CONF_DESC(54, REG_GPIO_H_E4, BIT(9)),
+	PINCTRL_CONF_DESC(55, REG_GPIO_H_E4, BIT(10)),
+	PINCTRL_CONF_DESC(56, REG_GPIO_H_E4, BIT(11)),
+	PINCTRL_CONF_DESC(57, REG_GPIO_H_E4, BIT(12)),
+	PINCTRL_CONF_DESC(58, REG_GPIO_H_E4, BIT(13)),
+	PINCTRL_CONF_DESC(59, REG_GPIO_H_E4, BIT(14)),
+	PINCTRL_CONF_DESC(61, REG_I2C_SDA_E4, PCIE0_RESET_E4_MASK),
+	PINCTRL_CONF_DESC(62, REG_I2C_SDA_E4, PCIE1_RESET_E4_MASK),
+	PINCTRL_CONF_DESC(63, REG_I2C_SDA_E4, PCIE2_RESET_E4_MASK),
+};
+
+static const struct airoha_pinctrl_conf airoha_pinctrl_pcie_rst_od_conf[] = {
+	PINCTRL_CONF_DESC(61, REG_PCIE_RESET_OD, PCIE0_RESET_OD_MASK),
+	PINCTRL_CONF_DESC(62, REG_PCIE_RESET_OD, PCIE1_RESET_OD_MASK),
+	PINCTRL_CONF_DESC(63, REG_PCIE_RESET_OD, PCIE2_RESET_OD_MASK),
+};
+
+static u32 airoha_pinctrl_rmw_unlock(void __iomem *addr, u32 mask, u32 val)
+{
+	val |= (readl(addr) & ~mask);
+	writel(val, addr);
+
+	return val;
+}
+
+#define airoha_pinctrl_set_unlock(addr, val)					\
+	airoha_pinctrl_rmw_unlock((addr), 0, (val))
+#define airoha_pinctrl_clear_unlock(addr, mask)					\
+	airoha_pinctrl_rmw_unlock((addr), (mask), (0))
+
+static u32 airoha_pinctrl_rmw(struct airoha_pinctrl *pinctrl,
+			      void __iomem *addr, u32 mask, u32 val)
+{
+	mutex_lock(&pinctrl->mutex);
+	val = airoha_pinctrl_rmw_unlock(addr, mask, val);
+	mutex_unlock(&pinctrl->mutex);
+
+	return val;
+}
+
+static void airoha_pinctrl_gpio_set_direction(struct airoha_pinctrl *pinctrl,
+					      unsigned int gpio, bool input)
+{
+	u32 mask, index;
+
+	/* set output enable */
+	mask = BIT(gpio % AIROHA_GPIO_BANK_SIZE);
+	index = gpio / AIROHA_GPIO_BANK_SIZE;
+	airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.out[index],
+			   mask, !input ? mask : 0);
+
+	/* set gpio direction */
+	mask = BIT(2 * (gpio % AIROHA_REG_GPIOCTRL_NUM_GPIO));
+	index = gpio / AIROHA_REG_GPIOCTRL_NUM_GPIO;
+	airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.dir[index],
+			   mask, !input ? mask : 0);
+}
+
+static void airoha_pinctrl_gpio_set_value(struct airoha_pinctrl *pinctrl,
+					  unsigned int gpio, bool value)
+{
+	u8 index = gpio / AIROHA_GPIO_BANK_SIZE;
+	u32 pin = gpio % AIROHA_GPIO_BANK_SIZE;
+
+	airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.data[index],
+			   BIT(pin), value ? BIT(pin) : 0);
+}
+
+static int airoha_pinctrl_gpio_get_direction(struct airoha_pinctrl *pinctrl,
+					     unsigned int gpio)
+{
+	u32 val, mask = BIT(2 * (gpio % AIROHA_REG_GPIOCTRL_NUM_GPIO));
+	u8 index = gpio / AIROHA_REG_GPIOCTRL_NUM_GPIO;
+
+	val = (readl(pinctrl->gpiochip.dir[index]) & mask);
+
+	return val ? PIN_CONFIG_OUTPUT_ENABLE : PIN_CONFIG_INPUT_ENABLE;
+}
+
+static int airoha_pinmux_set_mux(struct pinctrl_dev *pctrl_dev,
+				 unsigned int selector,
+				 unsigned int group)
+{
+	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct airoha_pinctrl_func *func;
+	struct function_desc *desc;
+	struct group_desc *grp;
+	int i;
+
+	desc = pinmux_generic_get_function(pctrl_dev, selector);
+	if (!desc)
+		return -EINVAL;
+
+	grp = pinctrl_generic_get_group(pctrl_dev, group);
+	if (!grp)
+		return -EINVAL;
+
+	dev_dbg(pctrl_dev->dev, "enable function %s group %s\n",
+		desc->func.name, grp->grp.name);
+
+	func = desc->data;
+	for (i = 0; i < func->group_size; i++) {
+		const struct airoha_pinctrl_func_group *group;
+		int j;
+
+		group = &func->groups[i];
+		if (strcmp(group->name, grp->grp.name))
+			continue;
+
+		for (j = 0; j < group->regmap_size; j++) {
+			void __iomem *base;
+
+			base = pinctrl->regs.mux[group->regmap[j].mux];
+			airoha_pinctrl_rmw(pinctrl,
+					   base + group->regmap[j].offset,
+					   group->regmap[j].mask,
+					   group->regmap[j].val);
+		}
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int airoha_pinmux_gpio_set_direction(struct pinctrl_dev *pctrl_dev,
+					    struct pinctrl_gpio_range *range,
+					    unsigned int pin, bool input)
+{
+	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	int gpio = pin - range->pin_base;
+
+	airoha_pinctrl_gpio_set_direction(pinctrl, gpio, input);
+
+	return 0;
+}
+
+static int airoha_pinctrl_get_gpio_from_pin(struct pinctrl_dev *pctrl_dev,
+					    int pin)
+{
+	struct pinctrl_gpio_range *range;
+	int gpio;
+
+	range = pinctrl_find_gpio_range_from_pin_nolock(pctrl_dev, pin);
+	if (!range)
+		return -EINVAL;
+
+	gpio = pin - range->pin_base;
+	if (gpio < 0)
+		return -EINVAL;
+
+	return gpio;
+}
+
+static const struct airoha_pinctrl_reg *
+airoha_pinctrl_get_conf_reg(const struct airoha_pinctrl_conf *conf,
+			    int conf_size, int pin)
+{
+	int i;
+
+	for (i = 0; i < conf_size; i++) {
+		if (conf[i].pin == pin)
+			return &conf[i].reg;
+	}
+
+	return NULL;
+}
+
+static int airoha_pinctrl_get_conf(void __iomem *base,
+				   const struct airoha_pinctrl_conf *conf,
+				   int conf_size, int pin, u32 *val)
+{
+	const struct airoha_pinctrl_reg *reg;
+
+	reg = airoha_pinctrl_get_conf_reg(conf, conf_size, pin);
+	if (!reg)
+		return -EINVAL;
+
+	*val = readl(base + reg->offset);
+	*val = (*val & reg->mask) >> __bf_shf(reg->mask);
+
+	return 0;
+}
+
+static int airoha_pinctrl_set_conf(struct airoha_pinctrl *pinctrl,
+				   void __iomem *base,
+				   const struct airoha_pinctrl_conf *conf,
+				   int conf_size, int pin, u32 val)
+{
+	const struct airoha_pinctrl_reg *reg = NULL;
+
+	reg = airoha_pinctrl_get_conf_reg(conf, conf_size, pin);
+	if (!reg)
+		return -EINVAL;
+
+	airoha_pinctrl_rmw(pinctrl, base + reg->offset, reg->mask,
+			   val << __bf_shf(reg->mask));
+
+	return 0;
+}
+
+#define airoha_pinctrl_get_pullup_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_get_conf(((pinctrl)->regs.conf),				\
+				airoha_pinctrl_pullup_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_pullup_conf),		\
+				(pin), (val))
+#define airoha_pinctrl_get_pulldown_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_get_conf(((pinctrl)->regs.conf),				\
+				airoha_pinctrl_pulldown_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_pulldown_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_get_drive_e2_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_get_conf(((pinctrl)->regs.conf),				\
+				airoha_pinctrl_drive_e2_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_drive_e2_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_get_drive_e4_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_get_conf(((pinctrl)->regs.conf),				\
+				airoha_pinctrl_drive_e4_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_drive_e4_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_get_pcie_rst_od_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_get_conf(((pinctrl)->regs.pcie_rst),			\
+				airoha_pinctrl_pcie_rst_od_conf,		\
+				ARRAY_SIZE(airoha_pinctrl_pcie_rst_od_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_set_pullup_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_set_conf((pinctrl), ((pinctrl)->regs.conf),		\
+				airoha_pinctrl_pullup_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_pullup_conf),		\
+				(pin), (val))
+#define airoha_pinctrl_set_pulldown_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_set_conf((pinctrl), ((pinctrl)->regs.conf),		\
+				airoha_pinctrl_pulldown_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_pulldown_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_set_drive_e2_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_set_conf((pinctrl), ((pinctrl)->regs.conf),		\
+				airoha_pinctrl_drive_e2_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_drive_e2_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_set_drive_e4_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_set_conf((pinctrl), ((pinctrl)->regs.conf),		\
+				airoha_pinctrl_drive_e4_conf,			\
+				ARRAY_SIZE(airoha_pinctrl_drive_e4_conf),	\
+				(pin), (val))
+#define airoha_pinctrl_set_pcie_rst_od_conf(pinctrl, pin, val)			\
+	airoha_pinctrl_set_conf((pinctrl), ((pinctrl)->regs.pcie_rst),		\
+				airoha_pinctrl_pcie_rst_od_conf,		\
+				ARRAY_SIZE(airoha_pinctrl_pcie_rst_od_conf),	\
+				(pin), (val))
+
+static int airoha_pinconf_get(struct pinctrl_dev *pctrl_dev,
+			      unsigned int pin, unsigned long *config)
+{
+	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 arg;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_PULL_UP: {
+		u32 pull_up, pull_down;
+
+		if (airoha_pinctrl_get_pullup_conf(pinctrl, pin, &pull_up) ||
+		    airoha_pinctrl_get_pulldown_conf(pinctrl, pin, &pull_down))
+			return -EINVAL;
+
+		if (param == PIN_CONFIG_BIAS_PULL_UP &&
+		    !(pull_up && !pull_down))
+			return -EINVAL;
+		else if (param == PIN_CONFIG_BIAS_PULL_DOWN &&
+			 !(pull_down && !pull_up))
+			return -EINVAL;
+		else if (pull_up || pull_down)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	}
+	case PIN_CONFIG_DRIVE_STRENGTH: {
+		u32 e2, e4;
+
+		if (airoha_pinctrl_get_drive_e2_conf(pinctrl, pin, &e2) ||
+		    airoha_pinctrl_get_drive_e4_conf(pinctrl, pin, &e4))
+			return -EINVAL;
+
+		arg = e4 << 1 | e2;
+		break;
+	}
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (airoha_pinctrl_get_pcie_rst_od_conf(pinctrl, pin, &arg))
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_OUTPUT_ENABLE:
+	case PIN_CONFIG_INPUT_ENABLE: {
+		int gpio = airoha_pinctrl_get_gpio_from_pin(pctrl_dev, pin);
+
+		if (gpio < 0)
+			return gpio;
+
+		arg = airoha_pinctrl_gpio_get_direction(pinctrl, gpio);
+		if (arg != param)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int airoha_pinconf_set(struct pinctrl_dev *pctrl_dev,
+			      unsigned int pin, unsigned long *configs,
+			      unsigned int num_configs)
+{
+	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	int i;
+
+	for (i = 0; i < num_configs; i++) {
+		u32 param = pinconf_to_config_param(configs[i]);
+		u32 arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
+			airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
+			airoha_pinctrl_set_pullup_conf(pinctrl, pin, 1);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 1);
+			airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH: {
+			u32 e2 = 0, e4 = 0;
+
+			switch (arg) {
+			case MTK_DRIVE_2mA:
+				break;
+			case MTK_DRIVE_4mA:
+				e2 = 1;
+				break;
+			case MTK_DRIVE_6mA:
+				e4 = 1;
+				break;
+			case MTK_DRIVE_8mA:
+				e2 = 1;
+				e4 = 1;
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			airoha_pinctrl_set_drive_e2_conf(pinctrl, pin, e2);
+			airoha_pinctrl_set_drive_e4_conf(pinctrl, pin, e4);
+			break;
+		}
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			airoha_pinctrl_set_pcie_rst_od_conf(pinctrl, pin, !!arg);
+			break;
+		case PIN_CONFIG_OUTPUT_ENABLE:
+		case PIN_CONFIG_INPUT_ENABLE:
+		case PIN_CONFIG_OUTPUT: {
+			int gpio = airoha_pinctrl_get_gpio_from_pin(pctrl_dev, pin);
+			bool input = param == PIN_CONFIG_INPUT_ENABLE;
+
+			if (gpio < 0)
+				return gpio;
+
+			airoha_pinctrl_gpio_set_direction(pinctrl, gpio, input);
+			if (param == PIN_CONFIG_OUTPUT)
+				airoha_pinctrl_gpio_set_value(pinctrl, gpio, !!arg);
+			break;
+		}
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
+static int airoha_pinconf_group_get(struct pinctrl_dev *pctrl_dev,
+				    unsigned int group, unsigned long *config)
+{
+	u32 cur_config = 0;
+	int i;
+
+	for (i = 0; i < airoha_pinctrl_groups[group].npins; i++) {
+		if (airoha_pinconf_get(pctrl_dev,
+				       airoha_pinctrl_groups[group].pins[i],
+				       config))
+			return -EOPNOTSUPP;
+
+		if (i && cur_config != *config)
+			return -EOPNOTSUPP;
+
+		cur_config = *config;
+	}
+
+	return 0;
+}
+
+static int airoha_pinconf_group_set(struct pinctrl_dev *pctrl_dev,
+				    unsigned int group, unsigned long *configs,
+				    unsigned int num_configs)
+{
+	int i;
+
+	for (i = 0; i < airoha_pinctrl_groups[group].npins; i++) {
+		int err;
+
+		err = airoha_pinconf_set(pctrl_dev,
+					 airoha_pinctrl_groups[group].pins[i],
+					 configs, num_configs);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops airoha_confops = {
+	.is_generic = true,
+	.pin_config_get = airoha_pinconf_get,
+	.pin_config_set = airoha_pinconf_set,
+	.pin_config_group_get = airoha_pinconf_group_get,
+	.pin_config_group_set = airoha_pinconf_group_set,
+	.pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static const struct pinctrl_ops airoha_pctlops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static const struct pinmux_ops airoha_pmxops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.gpio_set_direction = airoha_pinmux_gpio_set_direction,
+	.set_mux = airoha_pinmux_set_mux,
+	.strict = true,
+};
+
+static struct pinctrl_desc airoha_pinctrl_desc = {
+	.name = KBUILD_MODNAME,
+	.owner = THIS_MODULE,
+	.pctlops = &airoha_pctlops,
+	.pmxops = &airoha_pmxops,
+	.confops = &airoha_confops,
+	.pins = airoha_pinctrl_pins,
+	.npins = ARRAY_SIZE(airoha_pinctrl_pins),
+};
+
+static void airoha_pinctrl_gpio_set(struct gpio_chip *chip, unsigned int gpio,
+				    int value)
+{
+	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);
+
+	airoha_pinctrl_gpio_set_value(pinctrl, gpio, value);
+}
+
+static int airoha_pinctrl_gpio_get(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);
+	u8 index = gpio / AIROHA_GPIO_BANK_SIZE;
+	u32 pin = gpio % AIROHA_GPIO_BANK_SIZE;
+
+	return !!(readl(pinctrl->gpiochip.data[index]) & BIT(pin));
+}
+
+static int airoha_pinctrl_gpio_direction_output(struct gpio_chip *chip,
+						unsigned int gpio, int value)
+{
+	int err;
+
+	err = pinctrl_gpio_direction_output(chip, gpio);
+	if (err)
+		return err;
+
+	airoha_pinctrl_gpio_set(chip, gpio, value);
+
+	return 0;
+}
+
+static void airoha_pinctrl_gpio_irq_unmask(struct irq_data *data)
+{
+	u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_GPIO;
+	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_GPIO;
+	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
+	struct airoha_pinctrl_gpiochip *gpiochip;
+	u32 val = BIT(2 * offset);
+	unsigned long flags;
+
+	gpiochip = irq_data_get_irq_chip_data(data);
+	if (WARN_ON_ONCE(data->hwirq >= ARRAY_SIZE(gpiochip->irq_type)))
+		return;
+
+	spin_lock_irqsave(&gpiochip->lock, flags);
+
+	switch (gpiochip->irq_type[data->hwirq] & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_LEVEL_LOW:
+		val = val << 1;
+		fallthrough;
+	case IRQ_TYPE_LEVEL_HIGH:
+		airoha_pinctrl_rmw_unlock(gpiochip->level[index], mask, val);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		val = val << 1;
+		fallthrough;
+	case IRQ_TYPE_EDGE_RISING:
+		airoha_pinctrl_rmw_unlock(gpiochip->edge[index], mask, val);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		airoha_pinctrl_set_unlock(gpiochip->edge[index], mask);
+		break;
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&gpiochip->lock, flags);
+}
+
+static void airoha_pinctrl_gpio_irq_mask(struct irq_data *data)
+{
+	u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_GPIO;
+	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_GPIO;
+	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
+	struct airoha_pinctrl_gpiochip *gpiochip;
+	unsigned long flags;
+
+	gpiochip = irq_data_get_irq_chip_data(data);
+
+	spin_lock_irqsave(&gpiochip->lock, flags);
+
+	airoha_pinctrl_clear_unlock(gpiochip->edge[index], mask);
+	airoha_pinctrl_clear_unlock(gpiochip->level[index], mask);
+
+	spin_unlock_irqrestore(&gpiochip->lock, flags);
+}
+
+static int airoha_pinctrl_gpio_irq_type(struct irq_data *data,
+					unsigned int type)
+{
+	struct airoha_pinctrl_gpiochip *gpiochip;
+	unsigned long flags;
+
+	gpiochip = irq_data_get_irq_chip_data(data);
+	if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
+		return -EINVAL;
+
+	spin_lock_irqsave(&gpiochip->lock, flags);
+
+	if (type == IRQ_TYPE_PROBE) {
+		if (gpiochip->irq_type[data->hwirq])
+			goto unlock;
+
+		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+	}
+	gpiochip->irq_type[data->hwirq] = type & IRQ_TYPE_SENSE_MASK;
+unlock:
+	spin_unlock_irqrestore(&gpiochip->lock, flags);
+
+	return 0;
+}
+
+static irqreturn_t airoha_pinctrl_gpio_irq_handler(int irq, void *data)
+{
+	struct airoha_pinctrl_gpiochip *gpiochip;
+	bool handled = false;
+	int i;
+
+	gpiochip = container_of(data, struct airoha_pinctrl_gpiochip, chip);
+	for (i = 0; i < ARRAY_SIZE(gpiochip->status); i++) {
+		unsigned long status = readl(gpiochip->status[i]);
+		struct gpio_irq_chip *girq = &gpiochip->chip.irq;
+		int irq;
+
+		for_each_set_bit(irq, &status, AIROHA_GPIO_BANK_SIZE) {
+			u32 offset = irq + i * AIROHA_GPIO_BANK_SIZE;
+
+			generic_handle_irq(irq_find_mapping(girq->domain,
+							    offset));
+			writel(BIT(irq), gpiochip->status[i]);
+		}
+		handled |= !!status;
+	}
+
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
+				       struct platform_device *pdev)
+{
+	struct gpio_chip *chip = &pinctrl->gpiochip.chip;
+	struct gpio_irq_chip *girq = &chip->irq;
+	struct device *dev = &pdev->dev;
+	void __iomem *ptr;
+	int irq, err;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "gpio-bank0");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->gpiochip.data[0] = ptr + REG_GPIO_DATA;
+	pinctrl->gpiochip.dir[0] = ptr + REG_GPIO_CTRL;
+	pinctrl->gpiochip.out[0] = ptr + REG_GPIO_OE;
+
+	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
+		pinctrl->gpiochip.status[0] = ptr + REG_GPIO_INT;
+		pinctrl->gpiochip.level[0] = ptr + REG_GPIO_INT_LEVEL;
+		pinctrl->gpiochip.edge[0] = ptr + REG_GPIO_INT_EDGE;
+	}
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "gpio-ctrl1");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->gpiochip.dir[1] = ptr + REG_GPIO_CTRL1;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "gpio-bank1");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->gpiochip.data[1] = ptr + REG_GPIO_DATA1;
+	pinctrl->gpiochip.out[1] = ptr + REG_GPIO_OE1;
+
+	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
+		pinctrl->gpiochip.status[1] = ptr + REG_GPIO_INT1;
+		pinctrl->gpiochip.level[1] = ptr + REG_GPIO_INT_LEVEL1;
+		pinctrl->gpiochip.level[2] = ptr + REG_GPIO_INT_LEVEL2;
+		pinctrl->gpiochip.level[3] = ptr + REG_GPIO_INT_LEVEL3;
+		pinctrl->gpiochip.edge[1] = ptr + REG_GPIO_INT_EDGE1;
+		pinctrl->gpiochip.edge[2] = ptr + REG_GPIO_INT_EDGE2;
+		pinctrl->gpiochip.edge[3] = ptr + REG_GPIO_INT_EDGE3;
+	}
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "gpio-ctrl2");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->gpiochip.dir[2] = ptr + REG_GPIO_CTRL2;
+	pinctrl->gpiochip.dir[3] = ptr + REG_GPIO_CTRL3;
+
+	chip->parent = dev;
+	chip->label = dev_name(dev);
+	chip->request = gpiochip_generic_request;
+	chip->free = gpiochip_generic_free;
+	chip->direction_input = pinctrl_gpio_direction_input;
+	chip->direction_output = airoha_pinctrl_gpio_direction_output;
+	chip->set = airoha_pinctrl_gpio_set;
+	chip->get = airoha_pinctrl_gpio_get;
+	chip->base = -1;
+	chip->ngpio = AIROHA_NUM_GPIOS;
+
+	if (!of_property_read_bool(dev->of_node, "interrupt-controller"))
+		goto out;
+
+	spin_lock_init(&pinctrl->gpiochip.lock);
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	err = devm_request_irq(dev, irq, airoha_pinctrl_gpio_irq_handler,
+			       IRQF_SHARED, dev_name(dev), chip);
+	if (err) {
+		dev_err(dev, "error requesting irq %d: %d\n", irq, err);
+		return err;
+	}
+
+	girq->chip = devm_kzalloc(dev, sizeof(*girq->chip), GFP_KERNEL);
+	if (!girq->chip)
+		return -ENOMEM;
+
+	girq->chip->name = dev_name(dev);
+	girq->chip->irq_unmask = airoha_pinctrl_gpio_irq_unmask;
+	girq->chip->irq_mask = airoha_pinctrl_gpio_irq_mask;
+	girq->chip->irq_mask_ack = airoha_pinctrl_gpio_irq_mask;
+	girq->chip->irq_set_type = airoha_pinctrl_gpio_irq_type;
+	girq->chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_IMMUTABLE;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_simple_irq;
+out:
+	return devm_gpiochip_add_data(dev, chip, pinctrl);
+}
+
+static int airoha_pinctrl_ioreg_map(struct airoha_pinctrl *pinctrl,
+				    struct platform_device *pdev)
+{
+	void __iomem *ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "iomux");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.mux[0] = ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "led-iomux");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.mux[1] = ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "gpio-flash-mode");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.mux[2] = ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev,
+						    "gpio-flash-mode-ext");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.mux[3] = ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "ioconf");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.conf = ptr;
+
+	ptr = devm_platform_ioremap_resource_byname(pdev, "pcie-rst-od");
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
+
+	pinctrl->regs.pcie_rst = ptr;
+
+	return 0;
+}
+
+static int airoha_pinctrl_probe(struct platform_device *pdev)
+{
+	struct airoha_pinctrl *pinctrl;
+	int err, i;
+
+	pinctrl = devm_kzalloc(&pdev->dev, sizeof(*pinctrl), GFP_KERNEL);
+	if (!pinctrl)
+		return -ENOMEM;
+
+	mutex_init(&pinctrl->mutex);
+
+	err = airoha_pinctrl_ioreg_map(pinctrl, pdev);
+	if (err)
+		return err;
+
+	err = devm_pinctrl_register_and_init(&pdev->dev, &airoha_pinctrl_desc,
+					     pinctrl, &pinctrl->ctrl);
+	if (err)
+		return err;
+
+	/* build pin groups */
+	for (i = 0; i < ARRAY_SIZE(airoha_pinctrl_groups); i++) {
+		const struct pingroup *grp = &airoha_pinctrl_groups[i];
+
+		err = pinctrl_generic_add_group(pinctrl->ctrl, grp->name,
+						grp->pins, grp->npins,
+						(void *)grp);
+		if (err < 0) {
+			dev_err(&pdev->dev, "Failed to register group %s\n",
+				grp->name);
+			return err;
+		}
+	}
+
+	/* build functions */
+	for (i = 0; i < ARRAY_SIZE(airoha_pinctrl_funcs); i++) {
+		const struct airoha_pinctrl_func *func;
+
+		func = &airoha_pinctrl_funcs[i];
+		err = pinmux_generic_add_function(pinctrl->ctrl,
+						  func->desc.func.name,
+						  func->desc.func.groups,
+						  func->desc.func.ngroups,
+						  (void *)func);
+		if (err < 0) {
+			dev_err(&pdev->dev, "Failed to register function %s\n",
+				func->desc.func.name);
+			return err;
+		}
+	}
+
+	err = pinctrl_enable(pinctrl->ctrl);
+	if (err)
+		return err;
+
+	/* build gpio-chip */
+	return airoha_pinctrl_add_gpiochip(pinctrl, pdev);
+}
+
+static const struct of_device_id of_airoha_pinctrl_match[] = {
+	{ .compatible = "airoha,en7581-pinctrl" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver airoha_pinctrl_driver = {
+	.probe = airoha_pinctrl_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = of_airoha_pinctrl_match,
+	},
+};
+module_platform_driver(airoha_pinctrl_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@kernel.org>");
+MODULE_AUTHOR("Benjamin Larsson <benjamin.larsson@genexis.eu>");
+MODULE_AUTHOR("Markus Gothe <markus.gothe@genexis.eu>");
+MODULE_DESCRIPTION("Pinctrl driver for Airoha SoC");

-- 
2.46.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-22  9:40 ` [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller Lorenzo Bianconi
@ 2024-08-22 16:06   ` Conor Dooley
  2024-08-22 19:02     ` Lorenzo Bianconi
  2024-08-22 20:50     ` Benjamin Larsson
  0 siblings, 2 replies; 20+ messages in thread
From: Conor Dooley @ 2024-08-22 16:06 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Sean Wang, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, benjamin.larsson, ansuelsmth

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> controller.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> +  reg:
> +    items:
> +      - description: IOMUX base address
> +      - description: LED IOMUX base address
> +      - description: GPIO flash mode base address
> +      - description: GPIO flash mode extended base address
> +      - description: IO pin configuration base address
> +      - description: PCIE reset open-drain base address
> +      - description: GPIO bank0 register base address
> +      - description: GPIO bank0 second control register base address
> +      - description: GPIO bank1 second control register base address
> +      - description: GPIO bank1 register base address

> +      pinctrl@1fa20214 {
> +        compatible = "airoha,en7581-pinctrl";
> +        reg = <0x0 0x1fa20214 0x0 0x30>,
> +              <0x0 0x1fa2027c 0x0 0x8>,
> +              <0x0 0x1fbf0234 0x0 0x4>,
> +              <0x0 0x1fbf0268 0x0 0x4>,
> +              <0x0 0x1fa2001c 0x0 0x50>,
> +              <0x0 0x1fa2018c 0x0 0x4>,
> +              <0x0 0x1fbf0200 0x0 0x18>,
> +              <0x0 0x1fbf0220 0x0 0x4>,
> +              <0x0 0x1fbf0260 0x0 0x8>,
> +              <0x0 0x1fbf0270 0x0 0x28>;
> +        reg-names = "iomux", "led-iomux",
> +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> +                    "ioconf", "pcie-rst-od",
> +                    "gpio-bank0", "gpio-ctrl1",
> +                    "gpio-ctrl2", "gpio-bank1";

before looking at v1:
I would really like to see an explanation for why this is a correct
model of the hardware as part of the commit message. To me this screams
syscon/MFD and instead of describing this as a child of a syscon and
using regmap to access it you're doing whatever this is...

after looking at v1:
AFAICT the PWM driver does not currently exist in mainline, so I am now
doubly of the opinion that this needs to be an MFD and a wee bit annoyed
that you didn't include any rationale in your cover letter or w/e for
not going with an MFD given there was discussion on the topic in v1.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-22 16:06   ` Conor Dooley
@ 2024-08-22 19:02     ` Lorenzo Bianconi
  2024-08-22 20:50     ` Benjamin Larsson
  1 sibling, 0 replies; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-22 19:02 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Sean Wang, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, benjamin.larsson, ansuelsmth

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Aug 22, Conor Dooley wrote:
> On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> > Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> > controller.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > +  reg:
> > +    items:
> > +      - description: IOMUX base address
> > +      - description: LED IOMUX base address
> > +      - description: GPIO flash mode base address
> > +      - description: GPIO flash mode extended base address
> > +      - description: IO pin configuration base address
> > +      - description: PCIE reset open-drain base address
> > +      - description: GPIO bank0 register base address
> > +      - description: GPIO bank0 second control register base address
> > +      - description: GPIO bank1 second control register base address
> > +      - description: GPIO bank1 register base address
> 
> > +      pinctrl@1fa20214 {
> > +        compatible = "airoha,en7581-pinctrl";
> > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > +              <0x0 0x1fa2027c 0x0 0x8>,
> > +              <0x0 0x1fbf0234 0x0 0x4>,
> > +              <0x0 0x1fbf0268 0x0 0x4>,
> > +              <0x0 0x1fa2001c 0x0 0x50>,
> > +              <0x0 0x1fa2018c 0x0 0x4>,
> > +              <0x0 0x1fbf0200 0x0 0x18>,
> > +              <0x0 0x1fbf0220 0x0 0x4>,
> > +              <0x0 0x1fbf0260 0x0 0x8>,
> > +              <0x0 0x1fbf0270 0x0 0x28>;
> > +        reg-names = "iomux", "led-iomux",
> > +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> > +                    "ioconf", "pcie-rst-od",
> > +                    "gpio-bank0", "gpio-ctrl1",
> > +                    "gpio-ctrl2", "gpio-bank1";
> 
> before looking at v1:
> I would really like to see an explanation for why this is a correct
> model of the hardware as part of the commit message. To me this screams
> syscon/MFD and instead of describing this as a child of a syscon and
> using regmap to access it you're doing whatever this is...
> 
> after looking at v1:
> AFAICT the PWM driver does not currently exist in mainline, so I am now
> doubly of the opinion that this needs to be an MFD and a wee bit annoyed
> that you didn't include any rationale in your cover letter or w/e for
> not going with an MFD given there was discussion on the topic in v1.

based on the reply from Rob I was thinking it is fine to just reduce the number
of IO mappings, sorry for that.

> 
> Thanks,
> Conor.

clock, pinctrl and pwm controllers need to map 3 main memory areas:
- chip-scu: <0x0 0x1fa20000 0x0 0x384>
  it is used by the clock driver for fixed freq clock configuration,
  by the pinctrl driver for io-muxing (and by the future pon drivers)
- scu: <0x1fb00020 0x0 0x94c>
  it is used by the clock/rst driver
- gpio: <0x1fbf0200 0x0 0xbc>
  it is used by the pinctrl driver to implement gpio/irq controller and
  by the pwm driver.

I guess we can model chip_scu as single syscon node used by clock and pinctrl
while pwm can be a child of the pinctrl node. Something like:

...

chip_scu: chip-scu@1fa20000 {
	compatible = "airoha,en7581-chip-scu", "syscon";
	reg = <0x0 0x1fa20000 0x0 0x384>;
};

scuclk: clock-controller@1fb00020 {
	compatible = "airoha,en7581-scu";
	reg = <0x1fb00020 0x0 0x94c>;
	airoha,chip-scu = <&chip_scu>;
	...
};

pio: pinctrl@1fbf0200 {
	compatible = "airoha,en7581-pinctrl", "simple-mfd", "syscon";
	reg = <0x1fbf0200 0x0 0xbc>;
	airoha,chip-scu = <&chip_scu>;
	....

	pwm {
		compatible = "airoha,en7581-pwm";
		...
	};
};

...

Does it work for you?

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-22 16:06   ` Conor Dooley
  2024-08-22 19:02     ` Lorenzo Bianconi
@ 2024-08-22 20:50     ` Benjamin Larsson
  2024-08-23 16:14       ` Conor Dooley
  1 sibling, 1 reply; 20+ messages in thread
From: Benjamin Larsson @ 2024-08-22 20:50 UTC (permalink / raw)
  To: Conor Dooley, Lorenzo Bianconi
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Sean Wang, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream, ansuelsmth

On 22/08/2024 18:06, Conor Dooley wrote:


Hi.

> before looking at v1:
> I would really like to see an explanation for why this is a correct
> model of the hardware as part of the commit message. To me this screams
> syscon/MFD and instead of describing this as a child of a syscon and
> using regmap to access it you're doing whatever this is...

Can you post a link to a good example dts that uses syscon/MFD ?

It is not only pinctrl, pwm and gpio that are entangled in each other. A 
good example would help with developing a proper implementation.

MvH

Benjamin Larsson


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-23 16:14       ` Conor Dooley
@ 2024-08-23 15:08         ` Christian Marangi
  2024-08-23 21:17           ` Lorenzo Bianconi
  0 siblings, 1 reply; 20+ messages in thread
From: Christian Marangi @ 2024-08-23 15:08 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Benjamin Larsson, Lorenzo Bianconi, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > On 22/08/2024 18:06, Conor Dooley wrote:
> > 
> > 
> > Hi.
> > 
> > > before looking at v1:
> > > I would really like to see an explanation for why this is a correct
> > > model of the hardware as part of the commit message. To me this screams
> > > syscon/MFD and instead of describing this as a child of a syscon and
> > > using regmap to access it you're doing whatever this is...
> > 
> > Can you post a link to a good example dts that uses syscon/MFD ?
> > 
> > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > good example would help with developing a proper implementation.
> 
> Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> example. I would suggest to start by looking at drivers within gpio or
> pinctrl that use syscon_to_regmap() where the argument is sourced from
> either of_node->parent or dev.parent->of_node (which you use depends on
> whether or not you have a child node or not).
> 
> I recently had some questions myself for Rob about child nodes for mfd
> devices and when they were suitable to use:
> https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> 
> Following Rob's line of thought, I'd kinda expect an mfd driver to create
> the devices for gpio and pwm using devm_mfd_add_devices() and the
> pinctrl to have a child node.

Just to not get confused and staring to focus on the wrong kind of
API/too complex solution, I would suggest to check the example from
Lorenzo.

The pinctrl/gpio is an entire separate block and is mapped separately.
What is problematic is that chip SCU is a mix and address are not in
order and is required by many devices. (clock, pinctrl, gpio...)

IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
single big region and in our case we need to map 2 different one (gpio
AND chip SCU) (or for clock SCU and chip SCU)

Similar problem is present in many other place and syscon is just for
the task.

Simple proposed solution is:
- chip SCU entirely mapped and we use syscon
- pinctrl mapped and reference chip SCU by phandle
- pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs

Hope this can clear any confusion.

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-22 20:50     ` Benjamin Larsson
@ 2024-08-23 16:14       ` Conor Dooley
  2024-08-23 15:08         ` Christian Marangi
  0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2024-08-23 16:14 UTC (permalink / raw)
  To: Benjamin Larsson
  Cc: Lorenzo Bianconi, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream, ansuelsmth

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> On 22/08/2024 18:06, Conor Dooley wrote:
> 
> 
> Hi.
> 
> > before looking at v1:
> > I would really like to see an explanation for why this is a correct
> > model of the hardware as part of the commit message. To me this screams
> > syscon/MFD and instead of describing this as a child of a syscon and
> > using regmap to access it you're doing whatever this is...
> 
> Can you post a link to a good example dts that uses syscon/MFD ?
> 
> It is not only pinctrl, pwm and gpio that are entangled in each other. A
> good example would help with developing a proper implementation.

Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
example. I would suggest to start by looking at drivers within gpio or
pinctrl that use syscon_to_regmap() where the argument is sourced from
either of_node->parent or dev.parent->of_node (which you use depends on
whether or not you have a child node or not).

I recently had some questions myself for Rob about child nodes for mfd
devices and when they were suitable to use:
https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/

Following Rob's line of thought, I'd kinda expect an mfd driver to create
the devices for gpio and pwm using devm_mfd_add_devices() and the
pinctrl to have a child node.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-23 15:08         ` Christian Marangi
@ 2024-08-23 21:17           ` Lorenzo Bianconi
  2024-08-26 17:07             ` Conor Dooley
  0 siblings, 1 reply; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-23 21:17 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Conor Dooley, Benjamin Larsson, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

[-- Attachment #1: Type: text/plain, Size: 3422 bytes --]

> On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > 
> > > 
> > > Hi.
> > > 
> > > > before looking at v1:
> > > > I would really like to see an explanation for why this is a correct
> > > > model of the hardware as part of the commit message. To me this screams
> > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > using regmap to access it you're doing whatever this is...
> > > 
> > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > 
> > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > good example would help with developing a proper implementation.
> > 
> > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > example. I would suggest to start by looking at drivers within gpio or
> > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > either of_node->parent or dev.parent->of_node (which you use depends on
> > whether or not you have a child node or not).
> > 
> > I recently had some questions myself for Rob about child nodes for mfd
> > devices and when they were suitable to use:
> > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > 
> > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > pinctrl to have a child node.
> 
> Just to not get confused and staring to focus on the wrong kind of
> API/too complex solution, I would suggest to check the example from
> Lorenzo.
> 
> The pinctrl/gpio is an entire separate block and is mapped separately.
> What is problematic is that chip SCU is a mix and address are not in
> order and is required by many devices. (clock, pinctrl, gpio...)
> 
> IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> single big region and in our case we need to map 2 different one (gpio
> AND chip SCU) (or for clock SCU and chip SCU)
> 
> Similar problem is present in many other place and syscon is just for
> the task.
> 
> Simple proposed solution is:
> - chip SCU entirely mapped and we use syscon
> - pinctrl mapped and reference chip SCU by phandle
> - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> 
> Hope this can clear any confusion.
> 
> -- 
> 	Ansuel

To clarify the hw architecture we are discussing about 3 memory regions:
- chip_scu: <0x1fa20000 0x384>
- scu: <0x1fb00020 0x94c>
- gpio: <0x1fbf0200 0xbc>

The memory regions above are used by the following IC blocks:
- clock: chip_scu and scu
- pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
- pwm: gpio

clock and pinctrl devices share the chip_scu memory region but they need to
access even other separated memory areas (scu and gpio respectively).
pwm needs to just read/write few gpio registers.
As pointed out in my previous email, we can define the chip_scu block as
syscon node that can be accessed via phandle by clock and pinctrl drivers.
clock driver will map scu area while pinctrl one will map gpio memory block.
pwm can be just a child of pinctrl node.
What do you think about this approach? Can we address the requirements above
via classic mfd driver?

Regards,
Lorenzo



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-23 21:17           ` Lorenzo Bianconi
@ 2024-08-26 17:07             ` Conor Dooley
  2024-08-27  7:38               ` Benjamin Larsson
  2024-08-27  8:46               ` Lorenzo Bianconi
  0 siblings, 2 replies; 20+ messages in thread
From: Conor Dooley @ 2024-08-26 17:07 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Christian Marangi, Benjamin Larsson, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

[-- Attachment #1: Type: text/plain, Size: 5485 bytes --]

On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > 
> > > > 
> > > > Hi.
> > > > 
> > > > > before looking at v1:
> > > > > I would really like to see an explanation for why this is a correct
> > > > > model of the hardware as part of the commit message. To me this screams
> > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > using regmap to access it you're doing whatever this is...
> > > > 
> > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > 
> > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > good example would help with developing a proper implementation.
> > > 
> > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > example. I would suggest to start by looking at drivers within gpio or
> > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > whether or not you have a child node or not).
> > > 
> > > I recently had some questions myself for Rob about child nodes for mfd
> > > devices and when they were suitable to use:
> > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > 
> > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > pinctrl to have a child node.
> > 
> > Just to not get confused and staring to focus on the wrong kind of
> > API/too complex solution, I would suggest to check the example from
> > Lorenzo.
> > 
> > The pinctrl/gpio is an entire separate block and is mapped separately.
> > What is problematic is that chip SCU is a mix and address are not in
> > order and is required by many devices. (clock, pinctrl, gpio...)
> > 
> > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > single big region and in our case we need to map 2 different one (gpio
> > AND chip SCU) (or for clock SCU and chip SCU)
> > 
> > Similar problem is present in many other place and syscon is just for
> > the task.
> > 
> > Simple proposed solution is:
> > - chip SCU entirely mapped and we use syscon

That seems reasonable.

> > - pinctrl mapped and reference chip SCU by phandle

ref by phandle shouldn't be needed here, looking up by compatible should
suffice, no?

> > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs

The pwm is not a child of the pinctrl though, they're both subfunctions of
the register region they happen to both be in. I don't agree with that
appraisal, sounds like an MFD to me.

> > Hope this can clear any confusion.
> 
> To clarify the hw architecture we are discussing about 3 memory regions:
> - chip_scu: <0x1fa20000 0x384>
> - scu: <0x1fb00020 0x94c>
                  ^
I'm highly suspicious of a register region that begins at 0x20. What is
at 0x1fb00000?

> - gpio: <0x1fbf0200 0xbc>

Do you have a link to the register map documentation for this hardware?

> The memory regions above are used by the following IC blocks:
> - clock: chip_scu and scu

What is the differentiation between these two different regions? Do they
provide different clocks? Are registers from both of them required in
order to provide particular clocks?

> - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio

Ditto here. Are these actually two different sets of iomuxes, or are
registers from both required to mux a particular pin?

> - pwm: gpio
> 
> clock and pinctrl devices share the chip_scu memory region but they need to
> access even other separated memory areas (scu and gpio respectively).
> pwm needs to just read/write few gpio registers.
> As pointed out in my previous email, we can define the chip_scu block as
> syscon node that can be accessed via phandle by clock and pinctrl drivers.
> clock driver will map scu area while pinctrl one will map gpio memory block.
> pwm can be just a child of pinctrl node.

As I mentioned above, the last statement here I disagree with. As
someone that's currently in the process of fixing making a mess of this
exact kind of thing, I'm going to strongly advocate not taking shortcuts
like this.

IMO, all three of these regions need to be described as syscons in some
form, how exactly it's hard to say without a better understanding of the
breakdown between regions.

If, for example, the chip_scu only provides a few "helper" bits, I'd
expect something like

syscon@1fa20000 {
	compatible = "chip-scu", "syscon";
	reg = <0x1fa2000 0x384>;
};

syscon@1fb00000 {
	compatible = "scu", "syscon", "simple-mfd";
	#clock-cells = 1;
};

syscon@1fbf0200 {
	compatible = "gpio-scu", "syscon", "simple-mfd";
	#pwm-cells = 1;

	pinctrl@x {
		compatible = "pinctrl";
		reg = x;
		#pinctrl-cells = 1;
		#gpio-cells = 1;
	};
};

And you could look up the chip-scu by its compatible from the clock or
pinctrl drivers. Perhaps the "helper" bits assumption is incorrect
however, and both the scu and chip scu provide independent clocks?

> What do you think about this approach? Can we address the requirements above
> via classic mfd driver?



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-26 17:07             ` Conor Dooley
@ 2024-08-27  7:38               ` Benjamin Larsson
  2024-08-27  8:46               ` Lorenzo Bianconi
  1 sibling, 0 replies; 20+ messages in thread
From: Benjamin Larsson @ 2024-08-27  7:38 UTC (permalink / raw)
  To: Conor Dooley, Lorenzo Bianconi
  Cc: Christian Marangi, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

Hi.

On 26/08/2024 19:07, Conor Dooley wrote:
> To clarify the hw architecture we are discussing about 3 memory regions:
>> - chip_scu: <0x1fa20000 0x384>
>> - scu: <0x1fb00020 0x94c>
>                    ^
> I'm highly suspicious of a register region that begins at 0x20. What is
> at 0x1fb00000?

Unknown, no documentation of those registers.


>
>> - gpio: <0x1fbf0200 0xbc>
> Do you have a link to the register map documentation for this hardware?

There is no public documentation, what is available is the current 
driver source and this (less useful) partial map here:

https://github.com/gchmiel/en7512_kernel5/blob/master/linux-5-new/arch/mips/include/asm/tc3162/tc3162.h#L860

Registers with FMAP and FLAP are parts of the PWM functionality.

>
>> The memory regions above are used by the following IC blocks:
>> - clock: chip_scu and scu
> What is the differentiation between these two different regions? Do they
> provide different clocks? Are registers from both of them required in
> order to provide particular clocks?

chip-scu contains the registers the clock driver handles. But scu has 
registers with the word clock in the description but both regions does 
not seem to be needed for a specific clock.

chip-scu contains pinctrl, iomux and clocks

scu contains random bits and functional muxes for serdes

>
>> - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
> Ditto here. Are these actually two different sets of iomuxes, or are
> registers from both required to mux a particular pin?

io-muxes for pins are done in chip-scu, pwm muxing is done in the gpio 
register range together with chip-scu (ensure there are no conflicts).

MvH

Benjamin Larsson


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-26 17:07             ` Conor Dooley
  2024-08-27  7:38               ` Benjamin Larsson
@ 2024-08-27  8:46               ` Lorenzo Bianconi
  2024-08-27 14:35                 ` Rob Herring
  1 sibling, 1 reply; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-27  8:46 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Lorenzo Bianconi, Christian Marangi, Benjamin Larsson,
	Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Sean Wang, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-mediatek, linux-gpio, devicetree, linux-arm-kernel,
	upstream

>
> On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > >
> > > > >
> > > > > Hi.
> > > > >
> > > > > > before looking at v1:
> > > > > > I would really like to see an explanation for why this is a correct
> > > > > > model of the hardware as part of the commit message. To me this screams
> > > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > > using regmap to access it you're doing whatever this is...
> > > > >
> > > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > >
> > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > > good example would help with developing a proper implementation.
> > > >
> > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > > example. I would suggest to start by looking at drivers within gpio or
> > > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > > whether or not you have a child node or not).
> > > >
> > > > I recently had some questions myself for Rob about child nodes for mfd
> > > > devices and when they were suitable to use:
> > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > >
> > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > > pinctrl to have a child node.
> > >
> > > Just to not get confused and staring to focus on the wrong kind of
> > > API/too complex solution, I would suggest to check the example from
> > > Lorenzo.
> > >
> > > The pinctrl/gpio is an entire separate block and is mapped separately.
> > > What is problematic is that chip SCU is a mix and address are not in
> > > order and is required by many devices. (clock, pinctrl, gpio...)
> > >
> > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > > single big region and in our case we need to map 2 different one (gpio
> > > AND chip SCU) (or for clock SCU and chip SCU)
> > >
> > > Similar problem is present in many other place and syscon is just for
> > > the task.
> > >
> > > Simple proposed solution is:
> > > - chip SCU entirely mapped and we use syscon
>
> That seems reasonable.

ack

>
> > > - pinctrl mapped and reference chip SCU by phandle
>
> ref by phandle shouldn't be needed here, looking up by compatible should
> suffice, no?

ack, I think it would be fine

>
> > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
>
> The pwm is not a child of the pinctrl though, they're both subfunctions of
> the register region they happen to both be in. I don't agree with that
> appraisal, sounds like an MFD to me.

ack

>
> > > Hope this can clear any confusion.
> >
> > To clarify the hw architecture we are discussing about 3 memory regions:
> > - chip_scu: <0x1fa20000 0x384>
> > - scu: <0x1fb00020 0x94c>
>                   ^
> I'm highly suspicious of a register region that begins at 0x20. What is
> at 0x1fb00000?

sorry, my fault

>
> > - gpio: <0x1fbf0200 0xbc>
>
> Do you have a link to the register map documentation for this hardware?
>
> > The memory regions above are used by the following IC blocks:
> > - clock: chip_scu and scu
>
> What is the differentiation between these two different regions? Do they
> provide different clocks? Are registers from both of them required in
> order to provide particular clocks?

In chip-scu and scu memory regions we have heterogeneous registers.
Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
clock divider, switch clock source and divider, main bus clock source
and divider, ...) while in scu (regarding clock configuration) we have
pcie clock regs (e.g. reset and other registers). This is the reason
why, in en7581-scu driver, we need both of them, but we can access
chip-scu via the compatible string (please take a look at the dts
below).

>
> > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
>
> Ditto here. Are these actually two different sets of iomuxes, or are
> registers from both required to mux a particular pin?

Most of the io-muxes configuration registers are in chip-scu block,
just pwm ones are in gpio memory block.
Gpio block is mainly used for gpio_chip and irq_chip functionalities.

>
> > - pwm: gpio
> >
> > clock and pinctrl devices share the chip_scu memory region but they need to
> > access even other separated memory areas (scu and gpio respectively).
> > pwm needs to just read/write few gpio registers.
> > As pointed out in my previous email, we can define the chip_scu block as
> > syscon node that can be accessed via phandle by clock and pinctrl drivers.
> > clock driver will map scu area while pinctrl one will map gpio memory block.
> > pwm can be just a child of pinctrl node.
>
> As I mentioned above, the last statement here I disagree with. As
> someone that's currently in the process of fixing making a mess of this
> exact kind of thing, I'm going to strongly advocate not taking shortcuts
> like this.
>
> IMO, all three of these regions need to be described as syscons in some
> form, how exactly it's hard to say without a better understanding of the
> breakdown between regions.
>
> If, for example, the chip_scu only provides a few "helper" bits, I'd
> expect something like
>
> syscon@1fa20000 {
>         compatible = "chip-scu", "syscon";
>         reg = <0x1fa2000 0x384>;
> };
>
> syscon@1fb00000 {
>         compatible = "scu", "syscon", "simple-mfd";
>         #clock-cells = 1;
> };
>
> syscon@1fbf0200 {
>         compatible = "gpio-scu", "syscon", "simple-mfd";
>         #pwm-cells = 1;
>
>         pinctrl@x {
>                 compatible = "pinctrl";
>                 reg = x;
>                 #pinctrl-cells = 1;
>                 #gpio-cells = 1;
>         };
> };
>

ack, so we could use the following dts nodes for the discussed memory
regions (chip-scu, scu and gpio):

syscon@1fa20000 {
    compatible = "airoha,chip-scu", "syscon";
    reg = <0x0 0x1fa2000 0x0 0x384>;
};

clock-controller@1fb00000 {
    compatible = "airoha,en7581-scu", "syscon";
    reg = <0x0 0x1fb00000 0x0 0x94c>;
    #clock-cells = <1>;
    #reset-cells = <1>;
};

mfd@1fbf0200 {
    compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
    reg = <0x0 0x1fbf0200 0x0 0xc0>;

    pio: pinctrl {
        compatible = "airoha,en7581-pinctrl"
        gpio-controller;
        #gpio-cells = <2>;

        interrupt-controller;
        #interrupt-cells = <2>;
        interrupt-parent = <&gic>;
        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
    }

    pwm: pwm {
        compatible = "airoha,en7581-pwm";
        #pwm-cells = <3>;
    }
};

What do you think?
If we all agree on the approach above, we can proceed with the
required changes in the clk/pinctrl and pwm drivers.

Regards,
Lorenzo

> And you could look up the chip-scu by its compatible from the clock or
> pinctrl drivers. Perhaps the "helper" bits assumption is incorrect
> however, and both the scu and chip scu provide independent clocks?
>
> > What do you think about this approach? Can we address the requirements above
> > via classic mfd driver?
>
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-27  8:46               ` Lorenzo Bianconi
@ 2024-08-27 14:35                 ` Rob Herring
  2024-08-27 18:29                   ` Christian Marangi
  0 siblings, 1 reply; 20+ messages in thread
From: Rob Herring @ 2024-08-27 14:35 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Conor Dooley, Lorenzo Bianconi, Christian Marangi,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
<lorenzo.bianconi83@gmail.com> wrote:
>
> >
> > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > > >
> > > > > >
> > > > > > Hi.
> > > > > >
> > > > > > > before looking at v1:
> > > > > > > I would really like to see an explanation for why this is a correct
> > > > > > > model of the hardware as part of the commit message. To me this screams
> > > > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > > > using regmap to access it you're doing whatever this is...
> > > > > >
> > > > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > > >
> > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > > > good example would help with developing a proper implementation.
> > > > >
> > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > > > example. I would suggest to start by looking at drivers within gpio or
> > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > > > whether or not you have a child node or not).
> > > > >
> > > > > I recently had some questions myself for Rob about child nodes for mfd
> > > > > devices and when they were suitable to use:
> > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > > >
> > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > > > pinctrl to have a child node.
> > > >
> > > > Just to not get confused and staring to focus on the wrong kind of
> > > > API/too complex solution, I would suggest to check the example from
> > > > Lorenzo.
> > > >
> > > > The pinctrl/gpio is an entire separate block and is mapped separately.
> > > > What is problematic is that chip SCU is a mix and address are not in
> > > > order and is required by many devices. (clock, pinctrl, gpio...)
> > > >
> > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > > > single big region and in our case we need to map 2 different one (gpio
> > > > AND chip SCU) (or for clock SCU and chip SCU)
> > > >
> > > > Similar problem is present in many other place and syscon is just for
> > > > the task.
> > > >
> > > > Simple proposed solution is:
> > > > - chip SCU entirely mapped and we use syscon
> >
> > That seems reasonable.
>
> ack
>
> >
> > > > - pinctrl mapped and reference chip SCU by phandle
> >
> > ref by phandle shouldn't be needed here, looking up by compatible should
> > suffice, no?
>
> ack, I think it would be fine
>
> >
> > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> >
> > The pwm is not a child of the pinctrl though, they're both subfunctions of
> > the register region they happen to both be in. I don't agree with that
> > appraisal, sounds like an MFD to me.
>
> ack
>
> >
> > > > Hope this can clear any confusion.
> > >
> > > To clarify the hw architecture we are discussing about 3 memory regions:
> > > - chip_scu: <0x1fa20000 0x384>
> > > - scu: <0x1fb00020 0x94c>
> >                   ^
> > I'm highly suspicious of a register region that begins at 0x20. What is
> > at 0x1fb00000?
>
> sorry, my fault
>
> >
> > > - gpio: <0x1fbf0200 0xbc>
> >
> > Do you have a link to the register map documentation for this hardware?
> >
> > > The memory regions above are used by the following IC blocks:
> > > - clock: chip_scu and scu
> >
> > What is the differentiation between these two different regions? Do they
> > provide different clocks? Are registers from both of them required in
> > order to provide particular clocks?
>
> In chip-scu and scu memory regions we have heterogeneous registers.
> Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
> clock divider, switch clock source and divider, main bus clock source
> and divider, ...) while in scu (regarding clock configuration) we have
> pcie clock regs (e.g. reset and other registers). This is the reason
> why, in en7581-scu driver, we need both of them, but we can access
> chip-scu via the compatible string (please take a look at the dts
> below).
>
> >
> > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
> >
> > Ditto here. Are these actually two different sets of iomuxes, or are
> > registers from both required to mux a particular pin?
>
> Most of the io-muxes configuration registers are in chip-scu block,
> just pwm ones are in gpio memory block.
> Gpio block is mainly used for gpio_chip and irq_chip functionalities.
>
> >
> > > - pwm: gpio
> > >
> > > clock and pinctrl devices share the chip_scu memory region but they need to
> > > access even other separated memory areas (scu and gpio respectively).
> > > pwm needs to just read/write few gpio registers.
> > > As pointed out in my previous email, we can define the chip_scu block as
> > > syscon node that can be accessed via phandle by clock and pinctrl drivers.
> > > clock driver will map scu area while pinctrl one will map gpio memory block.
> > > pwm can be just a child of pinctrl node.
> >
> > As I mentioned above, the last statement here I disagree with. As
> > someone that's currently in the process of fixing making a mess of this
> > exact kind of thing, I'm going to strongly advocate not taking shortcuts
> > like this.
> >
> > IMO, all three of these regions need to be described as syscons in some
> > form, how exactly it's hard to say without a better understanding of the
> > breakdown between regions.
> >
> > If, for example, the chip_scu only provides a few "helper" bits, I'd
> > expect something like
> >
> > syscon@1fa20000 {
> >         compatible = "chip-scu", "syscon";
> >         reg = <0x1fa2000 0x384>;
> > };
> >
> > syscon@1fb00000 {
> >         compatible = "scu", "syscon", "simple-mfd";
> >         #clock-cells = 1;
> > };
> >
> > syscon@1fbf0200 {
> >         compatible = "gpio-scu", "syscon", "simple-mfd";
> >         #pwm-cells = 1;
> >
> >         pinctrl@x {
> >                 compatible = "pinctrl";
> >                 reg = x;
> >                 #pinctrl-cells = 1;
> >                 #gpio-cells = 1;
> >         };
> > };
> >
>
> ack, so we could use the following dts nodes for the discussed memory
> regions (chip-scu, scu and gpio):
>
> syscon@1fa20000 {
>     compatible = "airoha,chip-scu", "syscon";
>     reg = <0x0 0x1fa2000 0x0 0x384>;
> };
>
> clock-controller@1fb00000 {
>     compatible = "airoha,en7581-scu", "syscon";
>     reg = <0x0 0x1fb00000 0x0 0x94c>;
>     #clock-cells = <1>;
>     #reset-cells = <1>;
> };
>
> mfd@1fbf0200 {
>     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>
>     pio: pinctrl {
>         compatible = "airoha,en7581-pinctrl"
>         gpio-controller;
>         #gpio-cells = <2>;
>
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupt-parent = <&gic>;
>         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>     }
>
>     pwm: pwm {
>         compatible = "airoha,en7581-pwm";
>         #pwm-cells = <3>;
>     }
> };

I think this can be simplified down to this:

mfd@1fbf0200 {
    compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
is this h/w block called?
    reg = <0x0 0x1fbf0200 0x0 0xc0>;
    gpio-controller;
    #gpio-cells = <2>;
    interrupt-controller;
    #interrupt-cells = <2>;
    interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;

    #pwm-cells = <3>;

    pio: pinctrl {
        foo-pins {};
        bar-pins {};
    };
};

Maybe we keep the compatible in 'pinctrl'...

Rob

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-27 14:35                 ` Rob Herring
@ 2024-08-27 18:29                   ` Christian Marangi
  2024-08-29  6:20                     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: Christian Marangi @ 2024-08-27 18:29 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lorenzo Bianconi, Conor Dooley, Lorenzo Bianconi,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:
> On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
> <lorenzo.bianconi83@gmail.com> wrote:
> >
> > >
> > > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi.
> > > > > > >
> > > > > > > > before looking at v1:
> > > > > > > > I would really like to see an explanation for why this is a correct
> > > > > > > > model of the hardware as part of the commit message. To me this screams
> > > > > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > > > > using regmap to access it you're doing whatever this is...
> > > > > > >
> > > > > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > > > >
> > > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > > > > good example would help with developing a proper implementation.
> > > > > >
> > > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > > > > example. I would suggest to start by looking at drivers within gpio or
> > > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > > > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > > > > whether or not you have a child node or not).
> > > > > >
> > > > > > I recently had some questions myself for Rob about child nodes for mfd
> > > > > > devices and when they were suitable to use:
> > > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > > > >
> > > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > > > > pinctrl to have a child node.
> > > > >
> > > > > Just to not get confused and staring to focus on the wrong kind of
> > > > > API/too complex solution, I would suggest to check the example from
> > > > > Lorenzo.
> > > > >
> > > > > The pinctrl/gpio is an entire separate block and is mapped separately.
> > > > > What is problematic is that chip SCU is a mix and address are not in
> > > > > order and is required by many devices. (clock, pinctrl, gpio...)
> > > > >
> > > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > > > > single big region and in our case we need to map 2 different one (gpio
> > > > > AND chip SCU) (or for clock SCU and chip SCU)
> > > > >
> > > > > Similar problem is present in many other place and syscon is just for
> > > > > the task.
> > > > >
> > > > > Simple proposed solution is:
> > > > > - chip SCU entirely mapped and we use syscon
> > >
> > > That seems reasonable.
> >
> > ack
> >
> > >
> > > > > - pinctrl mapped and reference chip SCU by phandle
> > >
> > > ref by phandle shouldn't be needed here, looking up by compatible should
> > > suffice, no?
> >
> > ack, I think it would be fine
> >
> > >
> > > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> > >
> > > The pwm is not a child of the pinctrl though, they're both subfunctions of
> > > the register region they happen to both be in. I don't agree with that
> > > appraisal, sounds like an MFD to me.
> >
> > ack
> >
> > >
> > > > > Hope this can clear any confusion.
> > > >
> > > > To clarify the hw architecture we are discussing about 3 memory regions:
> > > > - chip_scu: <0x1fa20000 0x384>
> > > > - scu: <0x1fb00020 0x94c>
> > >                   ^
> > > I'm highly suspicious of a register region that begins at 0x20. What is
> > > at 0x1fb00000?
> >
> > sorry, my fault
> >
> > >
> > > > - gpio: <0x1fbf0200 0xbc>
> > >
> > > Do you have a link to the register map documentation for this hardware?
> > >
> > > > The memory regions above are used by the following IC blocks:
> > > > - clock: chip_scu and scu
> > >
> > > What is the differentiation between these two different regions? Do they
> > > provide different clocks? Are registers from both of them required in
> > > order to provide particular clocks?
> >
> > In chip-scu and scu memory regions we have heterogeneous registers.
> > Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
> > clock divider, switch clock source and divider, main bus clock source
> > and divider, ...) while in scu (regarding clock configuration) we have
> > pcie clock regs (e.g. reset and other registers). This is the reason
> > why, in en7581-scu driver, we need both of them, but we can access
> > chip-scu via the compatible string (please take a look at the dts
> > below).
> >
> > >
> > > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
> > >
> > > Ditto here. Are these actually two different sets of iomuxes, or are
> > > registers from both required to mux a particular pin?
> >
> > Most of the io-muxes configuration registers are in chip-scu block,
> > just pwm ones are in gpio memory block.
> > Gpio block is mainly used for gpio_chip and irq_chip functionalities.
> >
> > >
> > > > - pwm: gpio
> > > >
> > > > clock and pinctrl devices share the chip_scu memory region but they need to
> > > > access even other separated memory areas (scu and gpio respectively).
> > > > pwm needs to just read/write few gpio registers.
> > > > As pointed out in my previous email, we can define the chip_scu block as
> > > > syscon node that can be accessed via phandle by clock and pinctrl drivers.
> > > > clock driver will map scu area while pinctrl one will map gpio memory block.
> > > > pwm can be just a child of pinctrl node.
> > >
> > > As I mentioned above, the last statement here I disagree with. As
> > > someone that's currently in the process of fixing making a mess of this
> > > exact kind of thing, I'm going to strongly advocate not taking shortcuts
> > > like this.
> > >
> > > IMO, all three of these regions need to be described as syscons in some
> > > form, how exactly it's hard to say without a better understanding of the
> > > breakdown between regions.
> > >
> > > If, for example, the chip_scu only provides a few "helper" bits, I'd
> > > expect something like
> > >
> > > syscon@1fa20000 {
> > >         compatible = "chip-scu", "syscon";
> > >         reg = <0x1fa2000 0x384>;
> > > };
> > >
> > > syscon@1fb00000 {
> > >         compatible = "scu", "syscon", "simple-mfd";
> > >         #clock-cells = 1;
> > > };
> > >
> > > syscon@1fbf0200 {
> > >         compatible = "gpio-scu", "syscon", "simple-mfd";
> > >         #pwm-cells = 1;
> > >
> > >         pinctrl@x {
> > >                 compatible = "pinctrl";
> > >                 reg = x;
> > >                 #pinctrl-cells = 1;
> > >                 #gpio-cells = 1;
> > >         };
> > > };
> > >
> >
> > ack, so we could use the following dts nodes for the discussed memory
> > regions (chip-scu, scu and gpio):
> >
> > syscon@1fa20000 {
> >     compatible = "airoha,chip-scu", "syscon";
> >     reg = <0x0 0x1fa2000 0x0 0x384>;
> > };
> >
> > clock-controller@1fb00000 {
> >     compatible = "airoha,en7581-scu", "syscon";
> >     reg = <0x0 0x1fb00000 0x0 0x94c>;
> >     #clock-cells = <1>;
> >     #reset-cells = <1>;
> > };
> >
> > mfd@1fbf0200 {
> >     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
> >     reg = <0x0 0x1fbf0200 0x0 0xc0>;
> >
> >     pio: pinctrl {
> >         compatible = "airoha,en7581-pinctrl"
> >         gpio-controller;
> >         #gpio-cells = <2>;
> >
> >         interrupt-controller;
> >         #interrupt-cells = <2>;
> >         interrupt-parent = <&gic>;
> >         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> >     }
> >
> >     pwm: pwm {
> >         compatible = "airoha,en7581-pwm";
> >         #pwm-cells = <3>;
> >     }
> > };
> 
> I think this can be simplified down to this:
> 
> mfd@1fbf0200 {
>     compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
> is this h/w block called?
>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>     gpio-controller;
>     #gpio-cells = <2>;
>     interrupt-controller;
>     #interrupt-cells = <2>;
>     interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 
>     #pwm-cells = <3>;
> 
>     pio: pinctrl {
>         foo-pins {};
>         bar-pins {};
>     };
> };
> 
> Maybe we keep the compatible in 'pinctrl'...
>

Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
on how to implement this.

In Documentation the block is called GPIO Controller. As explained it does
expose pinctrl function AND pwm (with regs in the middle)

Is this semplification really needed? It does pose some problem driver
wise (on where to put the driver, in what subsystem) and also on the
yaml side with mixed property for pinctrl and pwm controller.

I feel mixing the 2 thing might cause some confusion on the 2 block
device that are well separated aside from the unlucky position of the
regs.

The suggested MFD implementation would consist of
- main node with MFD (map the entire GPIO controller regs)
-   2 child for PWM and pinctrl (no regs)

- driver in mfd/
- driver in pinctrl/
- driver in pwm/

An alternative is the previous solution with pinctrl mapping all the
GPIO controller regs and PWM a child but Conor suggested that a MFD
structure might be better suited for the task. We have both implemented
and ready to be submitted. Hope we can find a common decision on how to
implement this simple but annoying block of devices.

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-27 18:29                   ` Christian Marangi
@ 2024-08-29  6:20                     ` Krzysztof Kozlowski
  2024-08-30  8:50                       ` Christian Marangi
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-29  6:20 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Rob Herring, Lorenzo Bianconi, Conor Dooley, Lorenzo Bianconi,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote:
> On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:
> > On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
> > <lorenzo.bianconi83@gmail.com> wrote:
> > >
> > > >
> > > > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > > > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > > > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi.
> > > > > > > >
> > > > > > > > > before looking at v1:
> > > > > > > > > I would really like to see an explanation for why this is a correct
> > > > > > > > > model of the hardware as part of the commit message. To me this screams
> > > > > > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > > > > > using regmap to access it you're doing whatever this is...
> > > > > > > >
> > > > > > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > > > > >
> > > > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > > > > > good example would help with developing a proper implementation.
> > > > > > >
> > > > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > > > > > example. I would suggest to start by looking at drivers within gpio or
> > > > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > > > > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > > > > > whether or not you have a child node or not).
> > > > > > >
> > > > > > > I recently had some questions myself for Rob about child nodes for mfd
> > > > > > > devices and when they were suitable to use:
> > > > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > > > > >
> > > > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > > > > > pinctrl to have a child node.
> > > > > >
> > > > > > Just to not get confused and staring to focus on the wrong kind of
> > > > > > API/too complex solution, I would suggest to check the example from
> > > > > > Lorenzo.
> > > > > >
> > > > > > The pinctrl/gpio is an entire separate block and is mapped separately.
> > > > > > What is problematic is that chip SCU is a mix and address are not in
> > > > > > order and is required by many devices. (clock, pinctrl, gpio...)
> > > > > >
> > > > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > > > > > single big region and in our case we need to map 2 different one (gpio
> > > > > > AND chip SCU) (or for clock SCU and chip SCU)
> > > > > >
> > > > > > Similar problem is present in many other place and syscon is just for
> > > > > > the task.
> > > > > >
> > > > > > Simple proposed solution is:
> > > > > > - chip SCU entirely mapped and we use syscon
> > > >
> > > > That seems reasonable.
> > >
> > > ack
> > >
> > > >
> > > > > > - pinctrl mapped and reference chip SCU by phandle
> > > >
> > > > ref by phandle shouldn't be needed here, looking up by compatible should
> > > > suffice, no?
> > >
> > > ack, I think it would be fine
> > >
> > > >
> > > > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> > > >
> > > > The pwm is not a child of the pinctrl though, they're both subfunctions of
> > > > the register region they happen to both be in. I don't agree with that
> > > > appraisal, sounds like an MFD to me.
> > >
> > > ack
> > >
> > > >
> > > > > > Hope this can clear any confusion.
> > > > >
> > > > > To clarify the hw architecture we are discussing about 3 memory regions:
> > > > > - chip_scu: <0x1fa20000 0x384>
> > > > > - scu: <0x1fb00020 0x94c>
> > > >                   ^
> > > > I'm highly suspicious of a register region that begins at 0x20. What is
> > > > at 0x1fb00000?
> > >
> > > sorry, my fault
> > >
> > > >
> > > > > - gpio: <0x1fbf0200 0xbc>
> > > >
> > > > Do you have a link to the register map documentation for this hardware?
> > > >
> > > > > The memory regions above are used by the following IC blocks:
> > > > > - clock: chip_scu and scu
> > > >
> > > > What is the differentiation between these two different regions? Do they
> > > > provide different clocks? Are registers from both of them required in
> > > > order to provide particular clocks?
> > >
> > > In chip-scu and scu memory regions we have heterogeneous registers.
> > > Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
> > > clock divider, switch clock source and divider, main bus clock source
> > > and divider, ...) while in scu (regarding clock configuration) we have
> > > pcie clock regs (e.g. reset and other registers). This is the reason
> > > why, in en7581-scu driver, we need both of them, but we can access
> > > chip-scu via the compatible string (please take a look at the dts
> > > below).
> > >
> > > >
> > > > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
> > > >
> > > > Ditto here. Are these actually two different sets of iomuxes, or are
> > > > registers from both required to mux a particular pin?
> > >
> > > Most of the io-muxes configuration registers are in chip-scu block,
> > > just pwm ones are in gpio memory block.
> > > Gpio block is mainly used for gpio_chip and irq_chip functionalities.
> > >
> > > >
> > > > > - pwm: gpio
> > > > >
> > > > > clock and pinctrl devices share the chip_scu memory region but they need to
> > > > > access even other separated memory areas (scu and gpio respectively).
> > > > > pwm needs to just read/write few gpio registers.
> > > > > As pointed out in my previous email, we can define the chip_scu block as
> > > > > syscon node that can be accessed via phandle by clock and pinctrl drivers.
> > > > > clock driver will map scu area while pinctrl one will map gpio memory block.
> > > > > pwm can be just a child of pinctrl node.
> > > >
> > > > As I mentioned above, the last statement here I disagree with. As
> > > > someone that's currently in the process of fixing making a mess of this
> > > > exact kind of thing, I'm going to strongly advocate not taking shortcuts
> > > > like this.
> > > >
> > > > IMO, all three of these regions need to be described as syscons in some
> > > > form, how exactly it's hard to say without a better understanding of the
> > > > breakdown between regions.
> > > >
> > > > If, for example, the chip_scu only provides a few "helper" bits, I'd
> > > > expect something like
> > > >
> > > > syscon@1fa20000 {
> > > >         compatible = "chip-scu", "syscon";
> > > >         reg = <0x1fa2000 0x384>;
> > > > };
> > > >
> > > > syscon@1fb00000 {
> > > >         compatible = "scu", "syscon", "simple-mfd";
> > > >         #clock-cells = 1;
> > > > };
> > > >
> > > > syscon@1fbf0200 {
> > > >         compatible = "gpio-scu", "syscon", "simple-mfd";
> > > >         #pwm-cells = 1;
> > > >
> > > >         pinctrl@x {
> > > >                 compatible = "pinctrl";
> > > >                 reg = x;
> > > >                 #pinctrl-cells = 1;
> > > >                 #gpio-cells = 1;
> > > >         };
> > > > };
> > > >
> > >
> > > ack, so we could use the following dts nodes for the discussed memory
> > > regions (chip-scu, scu and gpio):
> > >
> > > syscon@1fa20000 {
> > >     compatible = "airoha,chip-scu", "syscon";
> > >     reg = <0x0 0x1fa2000 0x0 0x384>;
> > > };
> > >
> > > clock-controller@1fb00000 {
> > >     compatible = "airoha,en7581-scu", "syscon";
> > >     reg = <0x0 0x1fb00000 0x0 0x94c>;
> > >     #clock-cells = <1>;
> > >     #reset-cells = <1>;
> > > };
> > >
> > > mfd@1fbf0200 {
> > >     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
> > >     reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > >
> > >     pio: pinctrl {
> > >         compatible = "airoha,en7581-pinctrl"
> > >         gpio-controller;
> > >         #gpio-cells = <2>;
> > >
> > >         interrupt-controller;
> > >         #interrupt-cells = <2>;
> > >         interrupt-parent = <&gic>;
> > >         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > >     }
> > >
> > >     pwm: pwm {
> > >         compatible = "airoha,en7581-pwm";
> > >         #pwm-cells = <3>;
> > >     }
> > > };
> > 
> > I think this can be simplified down to this:
> > 
> > mfd@1fbf0200 {
> >     compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
> > is this h/w block called?
> >     reg = <0x0 0x1fbf0200 0x0 0xc0>;
> >     gpio-controller;
> >     #gpio-cells = <2>;
> >     interrupt-controller;
> >     #interrupt-cells = <2>;
> >     interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > 
> >     #pwm-cells = <3>;
> > 
> >     pio: pinctrl {
> >         foo-pins {};
> >         bar-pins {};
> >     };
> > };
> > 
> > Maybe we keep the compatible in 'pinctrl'...
> >
> 
> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
> on how to implement this.
> 
> In Documentation the block is called GPIO Controller. As explained it does
> expose pinctrl function AND pwm (with regs in the middle)
> 
> Is this semplification really needed? It does pose some problem driver
> wise (on where to put the driver, in what subsystem) and also on the

Sorry, but no, dt-bindings do not affect the driver at all in such way.
Nothing changes in your driver in such aspect, no dilemma where to put
it (the same place as before).

> yaml side with mixed property for pinctrl and pwm controller.

Hardware people mixed it, not we...

> 
> I feel mixing the 2 thing might cause some confusion on the 2 block
> device that are well separated aside from the unlucky position of the
> regs.

I think the feedback you got is that essentially these are parts of the
same device. Are you saying that hardware is really two separate
devices?

> 
> The suggested MFD implementation would consist of
> - main node with MFD (map the entire GPIO controller regs)
> -   2 child for PWM and pinctrl (no regs)
> 
> - driver in mfd/
> - driver in pinctrl/
> - driver in pwm/

This has nothing to do with bindings, except that you will need one
driver somewhere which adds child devices, but you still could do
everything as two drivers (as before).

Anyway how to structure drivers is rarely good argument about bindings.

> 
> An alternative is the previous solution with pinctrl mapping all the
> GPIO controller regs and PWM a child but Conor suggested that a MFD
> structure might be better suited for the task. We have both implemented
> and ready to be submitted. Hope we can find a common decision on how to
> implement this simple but annoying block of devices.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-29  6:20                     ` Krzysztof Kozlowski
@ 2024-08-30  8:50                       ` Christian Marangi
  2024-08-30 10:28                         ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: Christian Marangi @ 2024-08-30  8:50 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Rob Herring, Lorenzo Bianconi, Conor Dooley, Lorenzo Bianconi,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On Thu, Aug 29, 2024 at 08:20:10AM +0200, Krzysztof Kozlowski wrote:
> On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote:
> > On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:
> > > On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
> > > <lorenzo.bianconi83@gmail.com> wrote:
> > > >
> > > > >
> > > > > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
> > > > > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
> > > > > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
> > > > > > > > > On 22/08/2024 18:06, Conor Dooley wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi.
> > > > > > > > >
> > > > > > > > > > before looking at v1:
> > > > > > > > > > I would really like to see an explanation for why this is a correct
> > > > > > > > > > model of the hardware as part of the commit message. To me this screams
> > > > > > > > > > syscon/MFD and instead of describing this as a child of a syscon and
> > > > > > > > > > using regmap to access it you're doing whatever this is...
> > > > > > > > >
> > > > > > > > > Can you post a link to a good example dts that uses syscon/MFD ?
> > > > > > > > >
> > > > > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A
> > > > > > > > > good example would help with developing a proper implementation.
> > > > > > > >
> > > > > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
> > > > > > > > example. I would suggest to start by looking at drivers within gpio or
> > > > > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from
> > > > > > > > either of_node->parent or dev.parent->of_node (which you use depends on
> > > > > > > > whether or not you have a child node or not).
> > > > > > > >
> > > > > > > > I recently had some questions myself for Rob about child nodes for mfd
> > > > > > > > devices and when they were suitable to use:
> > > > > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
> > > > > > > >
> > > > > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create
> > > > > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the
> > > > > > > > pinctrl to have a child node.
> > > > > > >
> > > > > > > Just to not get confused and staring to focus on the wrong kind of
> > > > > > > API/too complex solution, I would suggest to check the example from
> > > > > > > Lorenzo.
> > > > > > >
> > > > > > > The pinctrl/gpio is an entire separate block and is mapped separately.
> > > > > > > What is problematic is that chip SCU is a mix and address are not in
> > > > > > > order and is required by many devices. (clock, pinctrl, gpio...)
> > > > > > >
> > > > > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
> > > > > > > single big region and in our case we need to map 2 different one (gpio
> > > > > > > AND chip SCU) (or for clock SCU and chip SCU)
> > > > > > >
> > > > > > > Similar problem is present in many other place and syscon is just for
> > > > > > > the task.
> > > > > > >
> > > > > > > Simple proposed solution is:
> > > > > > > - chip SCU entirely mapped and we use syscon
> > > > >
> > > > > That seems reasonable.
> > > >
> > > > ack
> > > >
> > > > >
> > > > > > > - pinctrl mapped and reference chip SCU by phandle
> > > > >
> > > > > ref by phandle shouldn't be needed here, looking up by compatible should
> > > > > suffice, no?
> > > >
> > > > ack, I think it would be fine
> > > >
> > > > >
> > > > > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> > > > >
> > > > > The pwm is not a child of the pinctrl though, they're both subfunctions of
> > > > > the register region they happen to both be in. I don't agree with that
> > > > > appraisal, sounds like an MFD to me.
> > > >
> > > > ack
> > > >
> > > > >
> > > > > > > Hope this can clear any confusion.
> > > > > >
> > > > > > To clarify the hw architecture we are discussing about 3 memory regions:
> > > > > > - chip_scu: <0x1fa20000 0x384>
> > > > > > - scu: <0x1fb00020 0x94c>
> > > > >                   ^
> > > > > I'm highly suspicious of a register region that begins at 0x20. What is
> > > > > at 0x1fb00000?
> > > >
> > > > sorry, my fault
> > > >
> > > > >
> > > > > > - gpio: <0x1fbf0200 0xbc>
> > > > >
> > > > > Do you have a link to the register map documentation for this hardware?
> > > > >
> > > > > > The memory regions above are used by the following IC blocks:
> > > > > > - clock: chip_scu and scu
> > > > >
> > > > > What is the differentiation between these two different regions? Do they
> > > > > provide different clocks? Are registers from both of them required in
> > > > > order to provide particular clocks?
> > > >
> > > > In chip-scu and scu memory regions we have heterogeneous registers.
> > > > Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
> > > > clock divider, switch clock source and divider, main bus clock source
> > > > and divider, ...) while in scu (regarding clock configuration) we have
> > > > pcie clock regs (e.g. reset and other registers). This is the reason
> > > > why, in en7581-scu driver, we need both of them, but we can access
> > > > chip-scu via the compatible string (please take a look at the dts
> > > > below).
> > > >
> > > > >
> > > > > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
> > > > >
> > > > > Ditto here. Are these actually two different sets of iomuxes, or are
> > > > > registers from both required to mux a particular pin?
> > > >
> > > > Most of the io-muxes configuration registers are in chip-scu block,
> > > > just pwm ones are in gpio memory block.
> > > > Gpio block is mainly used for gpio_chip and irq_chip functionalities.
> > > >
> > > > >
> > > > > > - pwm: gpio
> > > > > >
> > > > > > clock and pinctrl devices share the chip_scu memory region but they need to
> > > > > > access even other separated memory areas (scu and gpio respectively).
> > > > > > pwm needs to just read/write few gpio registers.
> > > > > > As pointed out in my previous email, we can define the chip_scu block as
> > > > > > syscon node that can be accessed via phandle by clock and pinctrl drivers.
> > > > > > clock driver will map scu area while pinctrl one will map gpio memory block.
> > > > > > pwm can be just a child of pinctrl node.
> > > > >
> > > > > As I mentioned above, the last statement here I disagree with. As
> > > > > someone that's currently in the process of fixing making a mess of this
> > > > > exact kind of thing, I'm going to strongly advocate not taking shortcuts
> > > > > like this.
> > > > >
> > > > > IMO, all three of these regions need to be described as syscons in some
> > > > > form, how exactly it's hard to say without a better understanding of the
> > > > > breakdown between regions.
> > > > >
> > > > > If, for example, the chip_scu only provides a few "helper" bits, I'd
> > > > > expect something like
> > > > >
> > > > > syscon@1fa20000 {
> > > > >         compatible = "chip-scu", "syscon";
> > > > >         reg = <0x1fa2000 0x384>;
> > > > > };
> > > > >
> > > > > syscon@1fb00000 {
> > > > >         compatible = "scu", "syscon", "simple-mfd";
> > > > >         #clock-cells = 1;
> > > > > };
> > > > >
> > > > > syscon@1fbf0200 {
> > > > >         compatible = "gpio-scu", "syscon", "simple-mfd";
> > > > >         #pwm-cells = 1;
> > > > >
> > > > >         pinctrl@x {
> > > > >                 compatible = "pinctrl";
> > > > >                 reg = x;
> > > > >                 #pinctrl-cells = 1;
> > > > >                 #gpio-cells = 1;
> > > > >         };
> > > > > };
> > > > >
> > > >
> > > > ack, so we could use the following dts nodes for the discussed memory
> > > > regions (chip-scu, scu and gpio):
> > > >
> > > > syscon@1fa20000 {
> > > >     compatible = "airoha,chip-scu", "syscon";
> > > >     reg = <0x0 0x1fa2000 0x0 0x384>;
> > > > };
> > > >
> > > > clock-controller@1fb00000 {
> > > >     compatible = "airoha,en7581-scu", "syscon";
> > > >     reg = <0x0 0x1fb00000 0x0 0x94c>;
> > > >     #clock-cells = <1>;
> > > >     #reset-cells = <1>;
> > > > };
> > > >
> > > > mfd@1fbf0200 {
> > > >     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
> > > >     reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > > >
> > > >     pio: pinctrl {
> > > >         compatible = "airoha,en7581-pinctrl"
> > > >         gpio-controller;
> > > >         #gpio-cells = <2>;
> > > >
> > > >         interrupt-controller;
> > > >         #interrupt-cells = <2>;
> > > >         interrupt-parent = <&gic>;
> > > >         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > > >     }
> > > >
> > > >     pwm: pwm {
> > > >         compatible = "airoha,en7581-pwm";
> > > >         #pwm-cells = <3>;
> > > >     }
> > > > };
> > > 
> > > I think this can be simplified down to this:
> > > 
> > > mfd@1fbf0200 {
> > >     compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
> > > is this h/w block called?
> > >     reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > >     gpio-controller;
> > >     #gpio-cells = <2>;
> > >     interrupt-controller;
> > >     #interrupt-cells = <2>;
> > >     interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > > 
> > >     #pwm-cells = <3>;
> > > 
> > >     pio: pinctrl {
> > >         foo-pins {};
> > >         bar-pins {};
> > >     };
> > > };
> > > 
> > > Maybe we keep the compatible in 'pinctrl'...
> > >
> > 
> > Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
> > on how to implement this.
> > 
> > In Documentation the block is called GPIO Controller. As explained it does
> > expose pinctrl function AND pwm (with regs in the middle)
> > 
> > Is this semplification really needed? It does pose some problem driver
> > wise (on where to put the driver, in what subsystem) and also on the
> 
> Sorry, but no, dt-bindings do not affect the driver at all in such way.
> Nothing changes in your driver in such aspect, no dilemma where to put
> it (the same place as before).
>

Ok, from the proposed node structure, is it problematic to move the
gpio-controller and -cells in the pinctrl node? And also the pwm-cells
to the pwm node?
This is similar to how it's done by broadcom GPIO MFD [1] that also
expose pinctrl and other device in the same register block as MFD
childs.

This would be the final node block.

                mfd@1fbf0200 {
                        compatible = "airoha,en7581-gpio-mfd";
                        reg = <0x0 0x1fbf0200 0x0 0xc0>;

                        interrupt-parent = <&gic>;
                        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;

                        pio: pinctrl {
                                compatible = "airoha,en7581-pinctrl";

                                gpio-controller;
                                #gpio-cells = <2>;

                                interrupt-controller;
                                #interrupt-cells = <2>;
                        };

                        pwm: pwm {
                                compatible = "airoha,en7581-pwm";

                                #pwm-cells = <3>;
                                status = "disabled";
                        };
                };

I also link the implementation of the MFD driver [2]

[1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml
[2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.c

> > yaml side with mixed property for pinctrl and pwm controller.
> 
> Hardware people mixed it, not we...
> 
> > 
> > I feel mixing the 2 thing might cause some confusion on the 2 block
> > device that are well separated aside from the unlucky position of the
> > regs.
> 
> I think the feedback you got is that essentially these are parts of the
> same device. Are you saying that hardware is really two separate
> devices?
> 
> > 
> > The suggested MFD implementation would consist of
> > - main node with MFD (map the entire GPIO controller regs)
> > -   2 child for PWM and pinctrl (no regs)
> > 
> > - driver in mfd/
> > - driver in pinctrl/
> > - driver in pwm/
> 
> This has nothing to do with bindings, except that you will need one
> driver somewhere which adds child devices, but you still could do
> everything as two drivers (as before).
> 
> Anyway how to structure drivers is rarely good argument about bindings.
> 
> > 
> > An alternative is the previous solution with pinctrl mapping all the
> > GPIO controller regs and PWM a child but Conor suggested that a MFD
> > structure might be better suited for the task. We have both implemented
> > and ready to be submitted. Hope we can find a common decision on how to
> > implement this simple but annoying block of devices.
> 
> Best regards,
> Krzysztof
> 

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-30  8:50                       ` Christian Marangi
@ 2024-08-30 10:28                         ` Krzysztof Kozlowski
  2024-08-30 10:55                           ` Lorenzo Bianconi
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-30 10:28 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Rob Herring, Lorenzo Bianconi, Conor Dooley, Lorenzo Bianconi,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On 30/08/2024 10:50, Christian Marangi wrote:
> On Thu, Aug 29, 2024 at 08:20:10AM +0200, Krzysztof Kozlowski wrote:
>> On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote:
>>> On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:
>>>> On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
>>>> <lorenzo.bianconi83@gmail.com> wrote:
>>>>>
>>>>>>
>>>>>> On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
>>>>>>>> On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
>>>>>>>>> On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
>>>>>>>>>> On 22/08/2024 18:06, Conor Dooley wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi.
>>>>>>>>>>
>>>>>>>>>>> before looking at v1:
>>>>>>>>>>> I would really like to see an explanation for why this is a correct
>>>>>>>>>>> model of the hardware as part of the commit message. To me this screams
>>>>>>>>>>> syscon/MFD and instead of describing this as a child of a syscon and
>>>>>>>>>>> using regmap to access it you're doing whatever this is...
>>>>>>>>>>
>>>>>>>>>> Can you post a link to a good example dts that uses syscon/MFD ?
>>>>>>>>>>
>>>>>>>>>> It is not only pinctrl, pwm and gpio that are entangled in each other. A
>>>>>>>>>> good example would help with developing a proper implementation.
>>>>>>>>>
>>>>>>>>> Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
>>>>>>>>> example. I would suggest to start by looking at drivers within gpio or
>>>>>>>>> pinctrl that use syscon_to_regmap() where the argument is sourced from
>>>>>>>>> either of_node->parent or dev.parent->of_node (which you use depends on
>>>>>>>>> whether or not you have a child node or not).
>>>>>>>>>
>>>>>>>>> I recently had some questions myself for Rob about child nodes for mfd
>>>>>>>>> devices and when they were suitable to use:
>>>>>>>>> https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/
>>>>>>>>>
>>>>>>>>> Following Rob's line of thought, I'd kinda expect an mfd driver to create
>>>>>>>>> the devices for gpio and pwm using devm_mfd_add_devices() and the
>>>>>>>>> pinctrl to have a child node.
>>>>>>>>
>>>>>>>> Just to not get confused and staring to focus on the wrong kind of
>>>>>>>> API/too complex solution, I would suggest to check the example from
>>>>>>>> Lorenzo.
>>>>>>>>
>>>>>>>> The pinctrl/gpio is an entire separate block and is mapped separately.
>>>>>>>> What is problematic is that chip SCU is a mix and address are not in
>>>>>>>> order and is required by many devices. (clock, pinctrl, gpio...)
>>>>>>>>
>>>>>>>> IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
>>>>>>>> single big region and in our case we need to map 2 different one (gpio
>>>>>>>> AND chip SCU) (or for clock SCU and chip SCU)
>>>>>>>>
>>>>>>>> Similar problem is present in many other place and syscon is just for
>>>>>>>> the task.
>>>>>>>>
>>>>>>>> Simple proposed solution is:
>>>>>>>> - chip SCU entirely mapped and we use syscon
>>>>>>
>>>>>> That seems reasonable.
>>>>>
>>>>> ack
>>>>>
>>>>>>
>>>>>>>> - pinctrl mapped and reference chip SCU by phandle
>>>>>>
>>>>>> ref by phandle shouldn't be needed here, looking up by compatible should
>>>>>> suffice, no?
>>>>>
>>>>> ack, I think it would be fine
>>>>>
>>>>>>
>>>>>>>> - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
>>>>>>
>>>>>> The pwm is not a child of the pinctrl though, they're both subfunctions of
>>>>>> the register region they happen to both be in. I don't agree with that
>>>>>> appraisal, sounds like an MFD to me.
>>>>>
>>>>> ack
>>>>>
>>>>>>
>>>>>>>> Hope this can clear any confusion.
>>>>>>>
>>>>>>> To clarify the hw architecture we are discussing about 3 memory regions:
>>>>>>> - chip_scu: <0x1fa20000 0x384>
>>>>>>> - scu: <0x1fb00020 0x94c>
>>>>>>                   ^
>>>>>> I'm highly suspicious of a register region that begins at 0x20. What is
>>>>>> at 0x1fb00000?
>>>>>
>>>>> sorry, my fault
>>>>>
>>>>>>
>>>>>>> - gpio: <0x1fbf0200 0xbc>
>>>>>>
>>>>>> Do you have a link to the register map documentation for this hardware?
>>>>>>
>>>>>>> The memory regions above are used by the following IC blocks:
>>>>>>> - clock: chip_scu and scu
>>>>>>
>>>>>> What is the differentiation between these two different regions? Do they
>>>>>> provide different clocks? Are registers from both of them required in
>>>>>> order to provide particular clocks?
>>>>>
>>>>> In chip-scu and scu memory regions we have heterogeneous registers.
>>>>> Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
>>>>> clock divider, switch clock source and divider, main bus clock source
>>>>> and divider, ...) while in scu (regarding clock configuration) we have
>>>>> pcie clock regs (e.g. reset and other registers). This is the reason
>>>>> why, in en7581-scu driver, we need both of them, but we can access
>>>>> chip-scu via the compatible string (please take a look at the dts
>>>>> below).
>>>>>
>>>>>>
>>>>>>> - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
>>>>>>
>>>>>> Ditto here. Are these actually two different sets of iomuxes, or are
>>>>>> registers from both required to mux a particular pin?
>>>>>
>>>>> Most of the io-muxes configuration registers are in chip-scu block,
>>>>> just pwm ones are in gpio memory block.
>>>>> Gpio block is mainly used for gpio_chip and irq_chip functionalities.
>>>>>
>>>>>>
>>>>>>> - pwm: gpio
>>>>>>>
>>>>>>> clock and pinctrl devices share the chip_scu memory region but they need to
>>>>>>> access even other separated memory areas (scu and gpio respectively).
>>>>>>> pwm needs to just read/write few gpio registers.
>>>>>>> As pointed out in my previous email, we can define the chip_scu block as
>>>>>>> syscon node that can be accessed via phandle by clock and pinctrl drivers.
>>>>>>> clock driver will map scu area while pinctrl one will map gpio memory block.
>>>>>>> pwm can be just a child of pinctrl node.
>>>>>>
>>>>>> As I mentioned above, the last statement here I disagree with. As
>>>>>> someone that's currently in the process of fixing making a mess of this
>>>>>> exact kind of thing, I'm going to strongly advocate not taking shortcuts
>>>>>> like this.
>>>>>>
>>>>>> IMO, all three of these regions need to be described as syscons in some
>>>>>> form, how exactly it's hard to say without a better understanding of the
>>>>>> breakdown between regions.
>>>>>>
>>>>>> If, for example, the chip_scu only provides a few "helper" bits, I'd
>>>>>> expect something like
>>>>>>
>>>>>> syscon@1fa20000 {
>>>>>>         compatible = "chip-scu", "syscon";
>>>>>>         reg = <0x1fa2000 0x384>;
>>>>>> };
>>>>>>
>>>>>> syscon@1fb00000 {
>>>>>>         compatible = "scu", "syscon", "simple-mfd";
>>>>>>         #clock-cells = 1;
>>>>>> };
>>>>>>
>>>>>> syscon@1fbf0200 {
>>>>>>         compatible = "gpio-scu", "syscon", "simple-mfd";
>>>>>>         #pwm-cells = 1;
>>>>>>
>>>>>>         pinctrl@x {
>>>>>>                 compatible = "pinctrl";
>>>>>>                 reg = x;
>>>>>>                 #pinctrl-cells = 1;
>>>>>>                 #gpio-cells = 1;
>>>>>>         };
>>>>>> };
>>>>>>
>>>>>
>>>>> ack, so we could use the following dts nodes for the discussed memory
>>>>> regions (chip-scu, scu and gpio):
>>>>>
>>>>> syscon@1fa20000 {
>>>>>     compatible = "airoha,chip-scu", "syscon";
>>>>>     reg = <0x0 0x1fa2000 0x0 0x384>;
>>>>> };
>>>>>
>>>>> clock-controller@1fb00000 {
>>>>>     compatible = "airoha,en7581-scu", "syscon";
>>>>>     reg = <0x0 0x1fb00000 0x0 0x94c>;
>>>>>     #clock-cells = <1>;
>>>>>     #reset-cells = <1>;
>>>>> };
>>>>>
>>>>> mfd@1fbf0200 {
>>>>>     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
>>>>>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>>
>>>>>     pio: pinctrl {
>>>>>         compatible = "airoha,en7581-pinctrl"
>>>>>         gpio-controller;
>>>>>         #gpio-cells = <2>;
>>>>>
>>>>>         interrupt-controller;
>>>>>         #interrupt-cells = <2>;
>>>>>         interrupt-parent = <&gic>;
>>>>>         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>>     }
>>>>>
>>>>>     pwm: pwm {
>>>>>         compatible = "airoha,en7581-pwm";
>>>>>         #pwm-cells = <3>;
>>>>>     }
>>>>> };
>>>>
>>>> I think this can be simplified down to this:
>>>>
>>>> mfd@1fbf0200 {
>>>>     compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
>>>> is this h/w block called?
>>>>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>     gpio-controller;
>>>>     #gpio-cells = <2>;
>>>>     interrupt-controller;
>>>>     #interrupt-cells = <2>;
>>>>     interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>>     #pwm-cells = <3>;
>>>>
>>>>     pio: pinctrl {
>>>>         foo-pins {};
>>>>         bar-pins {};
>>>>     };
>>>> };
>>>>
>>>> Maybe we keep the compatible in 'pinctrl'...
>>>>
>>>
>>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
>>> on how to implement this.
>>>
>>> In Documentation the block is called GPIO Controller. As explained it does
>>> expose pinctrl function AND pwm (with regs in the middle)
>>>
>>> Is this semplification really needed? It does pose some problem driver
>>> wise (on where to put the driver, in what subsystem) and also on the
>>
>> Sorry, but no, dt-bindings do not affect the driver at all in such way.
>> Nothing changes in your driver in such aspect, no dilemma where to put
>> it (the same place as before).
>>
> 
> Ok, from the proposed node structure, is it problematic to move the
> gpio-controller and -cells in the pinctrl node? And also the pwm-cells
> to the pwm node?

The move is just unnecessary and not neat. You design DTS based on your
drivers architecture and this is exactly what we want to avoid.

> This is similar to how it's done by broadcom GPIO MFD [1] that also

There are 'reg' fields, which is the main problem here. I don't like
that arguments because it entirely misses the discussions - about that
binding or other bindings - happening prior to merge.

> expose pinctrl and other device in the same register block as MFD
> childs.
> 
> This would be the final node block.
> 
>                 mfd@1fbf0200 {
>                         compatible = "airoha,en7581-gpio-mfd";
>                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
> 
>                         interrupt-parent = <&gic>;
>                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 
>                         pio: pinctrl {
>                                 compatible = "airoha,en7581-pinctrl";
> 
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
> 
>                                 interrupt-controller;
>                                 #interrupt-cells = <2>;

No resources here...

>                         };
> 
>                         pwm: pwm {
>                                 compatible = "airoha,en7581-pwm";
> 
>                                 #pwm-cells = <3>;
>                                 status = "disabled";

And why is it disabled? No external resources. There is no benefit of
this node.

>                         };
>                 };
> 
> I also link the implementation of the MFD driver [2]
> 
> [1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml
> [2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.c


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-30 10:28                         ` Krzysztof Kozlowski
@ 2024-08-30 10:55                           ` Lorenzo Bianconi
  2024-08-30 11:01                             ` Conor Dooley
  0 siblings, 1 reply; 20+ messages in thread
From: Lorenzo Bianconi @ 2024-08-30 10:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Christian Marangi, Rob Herring, Lorenzo Bianconi, Conor Dooley,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

[...]

> >>>
> >>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
> >>> on how to implement this.
> >>>
> >>> In Documentation the block is called GPIO Controller. As explained it does
> >>> expose pinctrl function AND pwm (with regs in the middle)
> >>>
> >>> Is this semplification really needed? It does pose some problem driver
> >>> wise (on where to put the driver, in what subsystem) and also on the
> >>
> >> Sorry, but no, dt-bindings do not affect the driver at all in such way.
> >> Nothing changes in your driver in such aspect, no dilemma where to put
> >> it (the same place as before).
> >>
> > 
> > Ok, from the proposed node structure, is it problematic to move the
> > gpio-controller and -cells in the pinctrl node? And also the pwm-cells
> > to the pwm node?
> 
> The move is just unnecessary and not neat. You design DTS based on your
> drivers architecture and this is exactly what we want to avoid.
> 
> > This is similar to how it's done by broadcom GPIO MFD [1] that also
> 
> There are 'reg' fields, which is the main problem here. I don't like
> that arguments because it entirely misses the discussions - about that
> binding or other bindings - happening prior to merge.
> 
> > expose pinctrl and other device in the same register block as MFD
> > childs.
> > 
> > This would be the final node block.
> > 
> >                 mfd@1fbf0200 {
> >                         compatible = "airoha,en7581-gpio-mfd";
> >                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > 
> >                         interrupt-parent = <&gic>;
> >                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > 
> >                         pio: pinctrl {
> >                                 compatible = "airoha,en7581-pinctrl";
> > 
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> > 
> >                                 interrupt-controller;
> >                                 #interrupt-cells = <2>;
> 
> No resources here...

ack. iiuc, all the properties will be in the parent node (mfd) and we will
have just the compatible strings in the child ones, right? Something like:

		mfd@1fbf0200 {
			compatible = "airoha,en7581-gpio-mfd";
			reg = <0x0 0x1fbf0200 0x0 0xc0>;
			gpio-controller;
			#gpio-cells = <2>;

			...
			#pwm-cells = <3>;

			pio: pinctrl {
				compatible = "airoha,en7581-pinctrl";
			};

			pwm: pwm {
				compatible = "airoha,en7581-pwm";
			};
		};

> 
> >                         };
> > 
> >                         pwm: pwm {
> >                                 compatible = "airoha,en7581-pwm";
> > 
> >                                 #pwm-cells = <3>;
> >                                 status = "disabled";
> 
> And why is it disabled? No external resources. There is no benefit of
> this node.

This is just a copy-paster error.

Regards,
Lorenzo

> 
> >                         };
> >                 };
> > 
> > I also link the implementation of the MFD driver [2]
> > 
> > [1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml
> > [2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.c
> 
> 
> Best regards,
> Krzysztof
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-30 10:55                           ` Lorenzo Bianconi
@ 2024-08-30 11:01                             ` Conor Dooley
  2024-08-30 11:03                               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2024-08-30 11:01 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Krzysztof Kozlowski, Christian Marangi, Rob Herring,
	Lorenzo Bianconi, Conor Dooley, Benjamin Larsson, Linus Walleij,
	Krzysztof Kozlowski, Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On Fri, Aug 30, 2024 at 12:55:32PM +0200, Lorenzo Bianconi wrote:
> [...]
> 
> > >>>
> > >>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
> > >>> on how to implement this.
> > >>>
> > >>> In Documentation the block is called GPIO Controller. As explained it does
> > >>> expose pinctrl function AND pwm (with regs in the middle)
> > >>>
> > >>> Is this semplification really needed? It does pose some problem driver
> > >>> wise (on where to put the driver, in what subsystem) and also on the
> > >>
> > >> Sorry, but no, dt-bindings do not affect the driver at all in such way.
> > >> Nothing changes in your driver in such aspect, no dilemma where to put
> > >> it (the same place as before).
> > >>
> > > 
> > > Ok, from the proposed node structure, is it problematic to move the
> > > gpio-controller and -cells in the pinctrl node? And also the pwm-cells
> > > to the pwm node?
> > 
> > The move is just unnecessary and not neat. You design DTS based on your
> > drivers architecture and this is exactly what we want to avoid.
> > 
> > > This is similar to how it's done by broadcom GPIO MFD [1] that also
> > 
> > There are 'reg' fields, which is the main problem here. I don't like
> > that arguments because it entirely misses the discussions - about that
> > binding or other bindings - happening prior to merge.
> > 
> > > expose pinctrl and other device in the same register block as MFD
> > > childs.
> > > 
> > > This would be the final node block.
> > > 
> > >                 mfd@1fbf0200 {
> > >                         compatible = "airoha,en7581-gpio-mfd";
> > >                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > > 
> > >                         interrupt-parent = <&gic>;
> > >                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > > 
> > >                         pio: pinctrl {
> > >                                 compatible = "airoha,en7581-pinctrl";
> > > 
> > >                                 gpio-controller;
> > >                                 #gpio-cells = <2>;
> > > 
> > >                                 interrupt-controller;
> > >                                 #interrupt-cells = <2>;
> > 
> > No resources here...
> 
> ack. iiuc, all the properties will be in the parent node (mfd) and we will
> have just the compatible strings in the child ones, right? Something like:
> 
> 		mfd@1fbf0200 {
> 			compatible = "airoha,en7581-gpio-mfd";
> 			reg = <0x0 0x1fbf0200 0x0 0xc0>;
> 			gpio-controller;
> 			#gpio-cells = <2>;
> 
> 			...
> 			#pwm-cells = <3>;
> 
> 			pio: pinctrl {
> 				compatible = "airoha,en7581-pinctrl";
> 			};
> 
> 			pwm: pwm {
> 				compatible = "airoha,en7581-pwm";
> 			};
> 		};


Didn't Rob basically tell you how to do it earlier in the thread?
What you've got now makes no sense, the compatibles only exist in that
to probe drivers, which you can do from the mfd driver with
mfd_add_devices() or w/e that function is called.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
  2024-08-30 11:01                             ` Conor Dooley
@ 2024-08-30 11:03                               ` Krzysztof Kozlowski
  0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-30 11:03 UTC (permalink / raw)
  To: Conor Dooley, Lorenzo Bianconi
  Cc: Christian Marangi, Rob Herring, Lorenzo Bianconi, Conor Dooley,
	Benjamin Larsson, Linus Walleij, Krzysztof Kozlowski,
	Conor Dooley, Sean Wang, Matthias Brugger,
	AngeloGioacchino Del Regno, linux-mediatek, linux-gpio,
	devicetree, linux-arm-kernel, upstream

On 30/08/2024 13:01, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 12:55:32PM +0200, Lorenzo Bianconi wrote:
>> [...]
>>
>>>>>>
>>>>>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
>>>>>> on how to implement this.
>>>>>>
>>>>>> In Documentation the block is called GPIO Controller. As explained it does
>>>>>> expose pinctrl function AND pwm (with regs in the middle)
>>>>>>
>>>>>> Is this semplification really needed? It does pose some problem driver
>>>>>> wise (on where to put the driver, in what subsystem) and also on the
>>>>>
>>>>> Sorry, but no, dt-bindings do not affect the driver at all in such way.
>>>>> Nothing changes in your driver in such aspect, no dilemma where to put
>>>>> it (the same place as before).
>>>>>
>>>>
>>>> Ok, from the proposed node structure, is it problematic to move the
>>>> gpio-controller and -cells in the pinctrl node? And also the pwm-cells
>>>> to the pwm node?
>>>
>>> The move is just unnecessary and not neat. You design DTS based on your
>>> drivers architecture and this is exactly what we want to avoid.
>>>
>>>> This is similar to how it's done by broadcom GPIO MFD [1] that also
>>>
>>> There are 'reg' fields, which is the main problem here. I don't like
>>> that arguments because it entirely misses the discussions - about that
>>> binding or other bindings - happening prior to merge.
>>>
>>>> expose pinctrl and other device in the same register block as MFD
>>>> childs.
>>>>
>>>> This would be the final node block.
>>>>
>>>>                 mfd@1fbf0200 {
>>>>                         compatible = "airoha,en7581-gpio-mfd";
>>>>                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>
>>>>                         interrupt-parent = <&gic>;
>>>>                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>>                         pio: pinctrl {
>>>>                                 compatible = "airoha,en7581-pinctrl";
>>>>
>>>>                                 gpio-controller;
>>>>                                 #gpio-cells = <2>;
>>>>
>>>>                                 interrupt-controller;
>>>>                                 #interrupt-cells = <2>;
>>>
>>> No resources here...
>>
>> ack. iiuc, all the properties will be in the parent node (mfd) and we will
>> have just the compatible strings in the child ones, right? Something like:
>>
>> 		mfd@1fbf0200 {
>> 			compatible = "airoha,en7581-gpio-mfd";
>> 			reg = <0x0 0x1fbf0200 0x0 0xc0>;
>> 			gpio-controller;
>> 			#gpio-cells = <2>;
>>
>> 			...
>> 			#pwm-cells = <3>;
>>
>> 			pio: pinctrl {
>> 				compatible = "airoha,en7581-pinctrl";
>> 			};
>>
>> 			pwm: pwm {
>> 				compatible = "airoha,en7581-pwm";
>> 			};
>> 		};
> 
> 
> Didn't Rob basically tell you how to do it earlier in the thread?
> What you've got now makes no sense, the compatibles only exist in that
> to probe drivers, which you can do from the mfd driver with
> mfd_add_devices() or w/e that function is called.

Yep, we are making circles.

I will repeat:
"The move is just unnecessary and not neat. You design DTS based on your
drivers architecture and this is exactly what we want to avoid."

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2024-08-30 11:03 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-22  9:40 [PATCH v2 0/2] Add pinctrl support to EN7581 SoC Lorenzo Bianconi
2024-08-22  9:40 ` [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller Lorenzo Bianconi
2024-08-22 16:06   ` Conor Dooley
2024-08-22 19:02     ` Lorenzo Bianconi
2024-08-22 20:50     ` Benjamin Larsson
2024-08-23 16:14       ` Conor Dooley
2024-08-23 15:08         ` Christian Marangi
2024-08-23 21:17           ` Lorenzo Bianconi
2024-08-26 17:07             ` Conor Dooley
2024-08-27  7:38               ` Benjamin Larsson
2024-08-27  8:46               ` Lorenzo Bianconi
2024-08-27 14:35                 ` Rob Herring
2024-08-27 18:29                   ` Christian Marangi
2024-08-29  6:20                     ` Krzysztof Kozlowski
2024-08-30  8:50                       ` Christian Marangi
2024-08-30 10:28                         ` Krzysztof Kozlowski
2024-08-30 10:55                           ` Lorenzo Bianconi
2024-08-30 11:01                             ` Conor Dooley
2024-08-30 11:03                               ` Krzysztof Kozlowski
2024-08-22  9:40 ` [PATCH v2 2/2] pinctrl: airoha: Add support for EN7581 SoC Lorenzo Bianconi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).