LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/2] This is a patch series for pinctrl driver of Sunplus SP7021 SoC.
@ 2021-12-07  4:17 Wells Lu
  2021-12-07  4:17 ` [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021 Wells Lu
  2021-12-07  4:17 ` [PATCH v3 2/2] pinctrl: Add driver " Wells Lu
  0 siblings, 2 replies; 9+ messages in thread
From: Wells Lu @ 2021-12-07  4:17 UTC (permalink / raw)
  To: linus.walleij, linux-gpio, linux-kernel, robh+dt, devicetree,
	linux-arm-kernel
  Cc: wells.lu, dvorkin, Wells Lu

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control
applications.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Wells Lu (2):
  dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021
  pinctrl: Add driver for Sunplus SP7021

 .../bindings/pinctrl/sunplus,sp7021-pinctrl.yaml   |  293 ++++++
 MAINTAINERS                                        |   10 +
 drivers/pinctrl/Kconfig                            |    1 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/sunplus/Kconfig                    |   21 +
 drivers/pinctrl/sunplus/Makefile                   |    5 +
 drivers/pinctrl/sunplus/sppctl.c                   | 1074 ++++++++++++++++++++
 drivers/pinctrl/sunplus/sppctl.h                   |  154 +++
 drivers/pinctrl/sunplus/sppctl_sp7021.c            |  536 ++++++++++
 include/dt-bindings/pinctrl/sppctl-sp7021.h        |  173 ++++
 include/dt-bindings/pinctrl/sppctl.h               |   40 +
 11 files changed, 2308 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml
 create mode 100644 drivers/pinctrl/sunplus/Kconfig
 create mode 100644 drivers/pinctrl/sunplus/Makefile
 create mode 100644 drivers/pinctrl/sunplus/sppctl.c
 create mode 100644 drivers/pinctrl/sunplus/sppctl.h
 create mode 100644 drivers/pinctrl/sunplus/sppctl_sp7021.c
 create mode 100644 include/dt-bindings/pinctrl/sppctl-sp7021.h
 create mode 100644 include/dt-bindings/pinctrl/sppctl.h

-- 
2.7.4


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

* [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021
  2021-12-07  4:17 [PATCH v3 0/2] This is a patch series for pinctrl driver of Sunplus SP7021 SoC Wells Lu
@ 2021-12-07  4:17 ` Wells Lu
  2021-12-09 23:30   ` Linus Walleij
  2021-12-07  4:17 ` [PATCH v3 2/2] pinctrl: Add driver " Wells Lu
  1 sibling, 1 reply; 9+ messages in thread
From: Wells Lu @ 2021-12-07  4:17 UTC (permalink / raw)
  To: linus.walleij, linux-gpio, linux-kernel, robh+dt, devicetree,
	linux-arm-kernel
  Cc: wells.lu, dvorkin, Wells Lu

Add dt-bindings header files and documentation for Sunplus SP7021 SoC.

Signed-off-by: Wells Lu <wellslutw@gmail.com>
---
Changes in V3
  - Addressed comments of Mr. Rob Herring.

 .../bindings/pinctrl/sunplus,sp7021-pinctrl.yaml   | 293 +++++++++++++++++++++
 MAINTAINERS                                        |   9 +
 include/dt-bindings/pinctrl/sppctl-sp7021.h        | 173 ++++++++++++
 include/dt-bindings/pinctrl/sppctl.h               |  40 +++
 4 files changed, 515 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml
 create mode 100644 include/dt-bindings/pinctrl/sppctl-sp7021.h
 create mode 100644 include/dt-bindings/pinctrl/sppctl.h

diff --git a/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml
new file mode 100644
index 0000000..fe2ab1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml
@@ -0,0 +1,293 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/sunplus,sp7021-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus SP7021 Pin Controller Device Tree Bindings
+
+maintainers:
+  - Dvorkin Dmitry <dvorkin@tibbo.com>
+  - Wells Lu <wellslutw@gmail.com>
+
+description: |
+  The Sunplus SP7021 pin controller is used to control SoC pins. Please
+  refer to pinctrl-bindings.txt in this directory for details of the common
+  pinctrl bindings used by client devices.
+
+  The device node of pin controller of Sunplus SP7021 has following
+  properties.
+
+properties:
+  compatible:
+    const: sunplus,sp7021-pctl
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  reg:
+    items:
+      - description: the MOON2 registers
+      - description: the GPIOXT registers
+      - description: the GPIOXT2 registers
+      - description: the FIRST registers
+      - description: the MOON1 registers
+
+  reg-names:
+    items:
+      - const: moon2
+      - const: gpioxt
+      - const: gpioxt2
+      - const: first
+      - const: moon1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+patternProperties:
+  '-pins$':
+    if:
+      type: object
+    then:
+      description: |
+        A pinctrl node should contain at least one subnodes representing the
+        pins or function-pins group available on the machine. Each subnode
+        will list the pins it needs, and how they should be configured.
+
+        Pinctrl node's client devices use subnodes for desired pin
+        configuration. Client device subnodes use below standard properties.
+
+      properties:
+        pins:
+          description: |
+            Define pins which are used by pinctrl node's client device.
+
+            It consists of one or more integers which represents the config
+            setting for corresponding pin. Please use macro SPPCTL_IOPAD to
+            define the integers for pins.
+
+            The first argument of the macro is pin number, the second is pin
+            type, the third is type of GPIO, the last is default output state
+            of GPIO.
+          $ref: /schemas/types.yaml#/definitions/uint32-array
+
+        function:
+          description: |
+            Define pin-function which is used by pinctrl node's client device.
+            The name should be one of string in the following enumeration.
+          $ref: "/schemas/types.yaml#/definitions/string"
+          enum: [ SPI_FLASH, SPI_FLASH_4BIT, SPI_NAND, CARD0_EMMC, SD_CARD,
+                  UA0, FPGA_IFX, HDMI_TX, LCDIF, USB0_OTG, USB1_OTG ]
+
+        groups:
+          description: |
+            Define pin-group in a specified pin-function.
+            The name should be one of string in the following enumeration.
+          $ref: "/schemas/types.yaml#/definitions/string"
+          enum: [ SPI_FLASH1, SPI_FLASH2, SPI_FLASH_4BIT1, SPI_FLASH_4BIT2,
+                  SPI_NAND, CARD0_EMMC, SD_CARD, UA0, FPGA_IFX, HDMI_TX1,
+                  HDMI_TX2, HDMI_TX3, LCDIF, USB0_OTG, USB1_OTG ]
+
+        zero_func:
+          description: |
+            Disabled pins which are not used by pinctrl node's client device.
+          $ref: /schemas/types.yaml#/definitions/uint32-array
+
+      additionalProperties: false
+
+      allOf:
+        - if:
+            properties:
+              function:
+                enum:
+                  - SPI_FLASH
+          then:
+            properties:
+              groups:
+                enum:
+                  - SPI_FLASH1
+                  - SPI_FLASH2
+        - if:
+            properties:
+              function:
+                enum:
+                  - SPI_FLASH_4BIT
+          then:
+            properties:
+              groups:
+                enum:
+                  - SPI_FLASH_4BIT1
+                  - SPI_FLASH_4BIT2
+        - if:
+            properties:
+              function:
+                enum:
+                  - SPI_NAND
+          then:
+            properties:
+              groups:
+                enum:
+                  - SPI_NAND
+        - if:
+            properties:
+              function:
+                enum:
+                  - CARD0_EMMC
+          then:
+            properties:
+              groups:
+                enum:
+                  - CARD0_EMMC
+        - if:
+            properties:
+              function:
+                enum:
+                  - SD_CARD
+          then:
+            properties:
+              groups:
+                enum:
+                  - SD_CARD
+        - if:
+            properties:
+              function:
+                enum:
+                  - UA0
+          then:
+            properties:
+              groups:
+                enum:
+                  - UA0
+        - if:
+            properties:
+              function:
+                enum:
+                  - FPGA_IFX
+          then:
+            properties:
+              groups:
+                enum:
+                  - FPGA_IFX
+        - if:
+            properties:
+              function:
+                enum:
+                  - HDMI_TX
+          then:
+            properties:
+              groups:
+                enum:
+                  - HDMI_TX1
+                  - HDMI_TX2
+                  - HDMI_TX3
+        - if:
+            properties:
+              function:
+                enum:
+                  - LCDIF
+          then:
+            properties:
+              groups:
+                enum:
+                  - LCDIF
+        - if:
+            properties:
+              function:
+                enum:
+                  - USB0_OTG
+          then:
+            properties:
+              groups:
+                enum:
+                  - USB0_OTG
+        - if:
+            properties:
+              function:
+                enum:
+                  - USB1_OTG
+          then:
+            properties:
+              groups:
+                enum:
+                  - USB1_OTG
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+  - clocks
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/pinctrl/sppctl-sp7021.h>
+
+    pinctl@9c000100 {
+        compatible = "sunplus,sp7021-pctl";
+        reg = <0x9c000100 0x100>, <0x9c000300 0x80>, <0x9c000380 0x80>,
+              <0x9c0032e4 0x1c>, <0x9c000080 0x20>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        clocks = <&clkc 0x83>;
+        resets = <&rstc 0x73>;
+
+        uart0-pins {
+            function = "UA0";
+            groups = "UA0";
+        };
+
+        spinand0-pins {
+            function = "SPI_NAND";
+            groups = "SPI_NAND";
+        };
+
+        uart1-pins {
+            pins = <
+                SPPCTL_IOPAD(11, SPPCTL_PCTL_G_PMUX, MUXF_UA1_TX, 0)
+                SPPCTL_IOPAD(10, SPPCTL_PCTL_G_PMUX, MUXF_UA1_RX, 0)
+            >;
+        };
+
+        uart2-pins {
+            pins = <
+                SPPCTL_IOPAD(20, SPPCTL_PCTL_G_PMUX, MUXF_UA1_TX, 0)
+                SPPCTL_IOPAD(21, SPPCTL_PCTL_G_PMUX, MUXF_UA1_RX, 0)
+                SPPCTL_IOPAD(22, SPPCTL_PCTL_G_PMUX, MUXF_UA1_RTS, 0)
+                SPPCTL_IOPAD(23, SPPCTL_PCTL_G_PMUX, MUXF_UA1_CTS, 0)
+            >;
+        };
+
+        emmc-pins {
+            function = "CARD0_EMMC";
+            groups = "CARD0_EMMC";
+        };
+
+        sdcard-pins {
+            function = "SD_CARD";
+            groups = "SD_CARD";
+            pins = < SPPCTL_IOPAD(91, SPPCTL_PCTL_G_GPIO, 0, 0) >;
+        };
+
+        hdmi_A_tx1-pins {
+            function = "HDMI_TX";
+            groups = "HDMI_TX1";
+        };
+        hdmi_A_tx2-pins {
+            function = "HDMI_TX";
+            groups = "HDMI_TX2";
+        };
+        hdmi_A_tx3-pins {
+            function = "HDMI_TX";
+            groups = "HDMI_TX3";
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index faa9c34..723bbe3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15128,6 +15128,15 @@ L:	linux-omap@vger.kernel.org
 S:	Maintained
 F:	drivers/pinctrl/pinctrl-single.c
 
+PIN CONTROLLER - SUNPLUS / TIBBO
+M:	Dvorkin Dmitry <dvorkin@tibbo.com>
+M:	Wells Lu <wellslutw@gmail.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+W:	https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
+F:	Documentation/devicetree/bindings/pinctrl/sunplus,*
+F:	include/dt-bindings/pinctrl/sppctl*
+
 PKTCDVD DRIVER
 M:	linux-block@vger.kernel.org
 S:	Orphan
diff --git a/include/dt-bindings/pinctrl/sppctl-sp7021.h b/include/dt-bindings/pinctrl/sppctl-sp7021.h
new file mode 100644
index 0000000..c9e6a07
--- /dev/null
+++ b/include/dt-bindings/pinctrl/sppctl-sp7021.h
@@ -0,0 +1,173 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/* Sunplus SP7021 dt-bindings Pinctrl header file
+ * Copyright (C) Sunplus Tech/Tibbo Tech. 2020
+ * Author: Dvorkin Dmitry <dvorkin@tibbo.com>
+ */
+
+#ifndef	__DT_BINDINGS_PINCTRL_SPPCTL_SP7021_H__
+#define	__DT_BINDINGS_PINCTRL_SPPCTL_SP7021_H__
+
+#include <dt-bindings/pinctrl/sppctl.h>
+
+/* Please don't change the order of the
+ * following defines. They are based on
+ * order of control register define of
+ * MOON2 and MOON4 registers.
+ */
+#define MUXF_GPIO			0
+#define MUXF_IOP			1
+#define MUXF_L2SW_CLK_OUT		2
+#define MUXF_L2SW_MAC_SMI_MDC		3
+#define MUXF_L2SW_LED_FLASH0		4
+#define MUXF_L2SW_LED_FLASH1		5
+#define MUXF_L2SW_LED_ON0		6
+#define MUXF_L2SW_LED_ON1		7
+#define MUXF_L2SW_MAC_SMI_MDIO		8
+#define MUXF_L2SW_P0_MAC_RMII_TXEN	9
+#define MUXF_L2SW_P0_MAC_RMII_TXD0	10
+#define MUXF_L2SW_P0_MAC_RMII_TXD1	11
+#define MUXF_L2SW_P0_MAC_RMII_CRSDV	12
+#define MUXF_L2SW_P0_MAC_RMII_RXD0	13
+#define MUXF_L2SW_P0_MAC_RMII_RXD1	14
+#define MUXF_L2SW_P0_MAC_RMII_RXER	15
+#define MUXF_L2SW_P1_MAC_RMII_TXEN	16
+#define MUXF_L2SW_P1_MAC_RMII_TXD0	17
+#define MUXF_L2SW_P1_MAC_RMII_TXD1	18
+#define MUXF_L2SW_P1_MAC_RMII_CRSDV	19
+#define MUXF_L2SW_P1_MAC_RMII_RXD0	20
+#define MUXF_L2SW_P1_MAC_RMII_RXD1	21
+#define MUXF_L2SW_P1_MAC_RMII_RXER	22
+#define MUXF_DAISY_MODE			23
+#define MUXF_SDIO_CLK			24
+#define MUXF_SDIO_CMD			25
+#define MUXF_SDIO_D0			26
+#define MUXF_SDIO_D1			27
+#define MUXF_SDIO_D2			28
+#define MUXF_SDIO_D3			29
+#define MUXF_PWM0			30
+#define MUXF_PWM1			31
+#define MUXF_PWM2			32
+#define MUXF_PWM3			33
+#define MUXF_PWM4			34
+#define MUXF_PWM5			35
+#define MUXF_PWM6			36
+#define MUXF_PWM7			37
+#define MUXF_ICM0_D			38
+#define MUXF_ICM1_D			39
+#define MUXF_ICM2_D			40
+#define MUXF_ICM3_D			41
+#define MUXF_ICM0_CLK			42
+#define MUXF_ICM1_CLK			43
+#define MUXF_ICM2_CLK			44
+#define MUXF_ICM3_CLK			45
+#define MUXF_SPIM0_INT			46
+#define MUXF_SPIM0_CLK			47
+#define MUXF_SPIM0_EN			48
+#define MUXF_SPIM0_DO			49
+#define MUXF_SPIM0_DI			50
+#define MUXF_SPIM1_INT			51
+#define MUXF_SPIM1_CLK			52
+#define MUXF_SPIM1_EN			53
+#define MUXF_SPIM1_DO			54
+#define MUXF_SPIM1_DI			55
+#define MUXF_SPIM2_INT			56
+#define MUXF_SPIM2_CLK			57
+#define MUXF_SPIM2_EN			58
+#define MUXF_SPIM2_DO			59
+#define MUXF_SPIM2_DI			60
+#define MUXF_SPIM3_INT			61
+#define MUXF_SPIM3_CLK			62
+#define MUXF_SPIM3_EN			63
+#define MUXF_SPIM3_DO			64
+#define MUXF_SPIM3_DI			65
+#define MUXF_SPI0S_INT			66
+#define MUXF_SPI0S_CLK			67
+#define MUXF_SPI0S_EN			68
+#define MUXF_SPI0S_DO			69
+#define MUXF_SPI0S_DI			70
+#define MUXF_SPI1S_INT			71
+#define MUXF_SPI1S_CLK			72
+#define MUXF_SPI1S_EN			73
+#define MUXF_SPI1S_DO			74
+#define MUXF_SPI1S_DI			75
+#define MUXF_SPI2S_INT			76
+#define MUXF_SPI2S_CLK			77
+#define MUXF_SPI2S_EN			78
+#define MUXF_SPI2S_DO			79
+#define MUXF_SPI2S_DI			80
+#define MUXF_SPI3S_INT			81
+#define MUXF_SPI3S_CLK			82
+#define MUXF_SPI3S_EN			83
+#define MUXF_SPI3S_DO			84
+#define MUXF_SPI3S_DI			85
+#define MUXF_I2CM0_CLK			86
+#define MUXF_I2CM0_DAT			87
+#define MUXF_I2CM1_CLK			88
+#define MUXF_I2CM1_DAT			89
+#define MUXF_I2CM2_CLK			90
+#define MUXF_I2CM2_DAT			91
+#define MUXF_I2CM3_CLK			92
+#define MUXF_I2CM3_DAT			93
+#define MUXF_UA1_TX			94
+#define MUXF_UA1_RX			95
+#define MUXF_UA1_CTS			96
+#define MUXF_UA1_RTS			97
+#define MUXF_UA2_TX			98
+#define MUXF_UA2_RX			99
+#define MUXF_UA2_CTS			100
+#define MUXF_UA2_RTS			101
+#define MUXF_UA3_TX			102
+#define MUXF_UA3_RX			103
+#define MUXF_UA3_CTS			104
+#define MUXF_UA3_RTS			105
+#define MUXF_UA4_TX			106
+#define MUXF_UA4_RX			107
+#define MUXF_UA4_CTS			108
+#define MUXF_UA4_RTS			109
+#define MUXF_TIMER0_INT			110
+#define MUXF_TIMER1_INT			111
+#define MUXF_TIMER2_INT			112
+#define MUXF_TIMER3_INT			113
+#define MUXF_GPIO_INT0			114
+#define MUXF_GPIO_INT1			115
+#define MUXF_GPIO_INT2			116
+#define MUXF_GPIO_INT3			117
+#define MUXF_GPIO_INT4			118
+#define MUXF_GPIO_INT5			119
+#define MUXF_GPIO_INT6			120
+#define MUXF_GPIO_INT7			121
+
+#define GROP_SPI_FLASH			122
+#define GROP_SPI_FLASH_4BIT		123
+#define GROP_SPI_NAND			124
+#define GROP_CARD0_EMMC			125
+#define GROP_SD_CARD			126
+#define GROP_UA0			127
+#define GROP_ACHIP_DEBUG		128
+#define GROP_ACHIP_UA2AXI		129
+#define GROP_FPGA_IFX			130
+#define GROP_HDMI_TX			131
+#define GROP_AUD_EXT_ADC_IFX0		132
+#define GROP_AUD_EXT_DAC_IFX0		133
+#define GROP_SPDIF_RX			134
+#define GROP_SPDIF_TX			135
+#define GROP_TDMTX_IFX0			136
+#define GROP_TDMRX_IFX0			137
+#define GROP_PDMRX_IFX0			138
+#define GROP_PCM_IEC_TX			139
+#define GROP_LCDIF			140
+#define GROP_DVD_DSP_DEBUG		141
+#define GROP_I2C_DEBUG			142
+#define GROP_I2C_SLAVE			143
+#define GROP_WAKEUP			144
+#define GROP_UART2AXI			145
+#define GROP_USB0_I2C			146
+#define GROP_USB1_I2C			147
+#define GROP_USB0_OTG			148
+#define GROP_USB1_OTG			149
+#define GROP_UPHY0_DEBUG		150
+#define GROP_UPHY1_DEBUG		151
+#define GROP_UPHY0_EXT			152
+#define GROP_PROBE_PORT			153
+
+#endif
diff --git a/include/dt-bindings/pinctrl/sppctl.h b/include/dt-bindings/pinctrl/sppctl.h
new file mode 100644
index 0000000..7e5c7a6
--- /dev/null
+++ b/include/dt-bindings/pinctrl/sppctl.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/* Sunplus dt-bindings Pinctrl header file
+ * Copyright (C) Sunplus Tech/Tibbo Tech. 2020
+ * Author: Dvorkin Dmitry <dvorkin@tibbo.com>
+ */
+
+#ifndef __DT_BINDINGS_PINCTRL_SPPCTL_H__
+#define __DT_BINDINGS_PINCTRL_SPPCTL_H__
+
+#define IOP_G_MASTE		(0x01 << 0)
+#define IOP_G_FIRST		(0x01 << 1)
+
+#define SPPCTL_PCTL_G_PMUX	(0x00        | IOP_G_MASTE)
+#define SPPCTL_PCTL_G_GPIO	(IOP_G_FIRST | IOP_G_MASTE)
+#define SPPCTL_PCTL_G_IOPP	(IOP_G_FIRST | 0x00)
+
+#define SPPCTL_PCTL_L_OUT	(0x01 << 0)	/* Output LOW        */
+#define SPPCTL_PCTL_L_OU1	(0x01 << 1)	/* Output HIGH       */
+#define SPPCTL_PCTL_L_INV	(0x01 << 2)	/* Input Invert      */
+#define SPPCTL_PCTL_L_ONV	(0x01 << 3)	/* Output Invert     */
+#define SPPCTL_PCTL_L_ODR	(0x01 << 4)	/* Output Open Drain */
+
+#define SPPCTL_PCTLE_P(v)	((v) << 24)
+#define SPPCTL_PCTLE_G(v)	((v) << 16)
+#define SPPCTL_PCTLE_F(v)	((v) << 8)
+#define SPPCTL_PCTLE_L(v)	((v) << 0)
+
+#define SPPCTL_PCTLD_P(v)	(((v) >> 24) & 0xff)
+#define SPPCTL_PCTLD_G(v)	(((v) >> 16) & 0xff)
+#define SPPCTL_PCTLD_F(v)	(((v) >>  8) & 0xff)
+#define SPPCTL_PCTLD_L(v)	(((v) >>  0) & 0xff)
+
+/*
+ * pack into 32-bit value:
+ * pin#(8bit), typ(8bit), function(8bit), flag(8bit)
+ */
+#define SPPCTL_IOPAD(pin, typ, fun, flg)	(((pin) << 24) | ((typ) << 16) | \
+						((fun) << 8) | (flg))
+
+#endif
-- 
2.7.4


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

* [PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
  2021-12-07  4:17 [PATCH v3 0/2] This is a patch series for pinctrl driver of Sunplus SP7021 SoC Wells Lu
  2021-12-07  4:17 ` [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021 Wells Lu
@ 2021-12-07  4:17 ` Wells Lu
  2021-12-10  1:24   ` Linus Walleij
  1 sibling, 1 reply; 9+ messages in thread
From: Wells Lu @ 2021-12-07  4:17 UTC (permalink / raw)
  To: linus.walleij, linux-gpio, linux-kernel, robh+dt, devicetree,
	linux-arm-kernel
  Cc: wells.lu, dvorkin, Wells Lu

Add driver for Sunplus SP7021.

Signed-off-by: Wells Lu <wellslutw@gmail.com>
---
Changes in V3
  - Addressed comments of Mr. Linus Walleij.
  - Remove sysfs functions.
  - Cleaned up code.

 MAINTAINERS                             |    1 +
 drivers/pinctrl/Kconfig                 |    1 +
 drivers/pinctrl/Makefile                |    1 +
 drivers/pinctrl/sunplus/Kconfig         |   21 +
 drivers/pinctrl/sunplus/Makefile        |    5 +
 drivers/pinctrl/sunplus/sppctl.c        | 1074 +++++++++++++++++++++++++++++++
 drivers/pinctrl/sunplus/sppctl.h        |  154 +++++
 drivers/pinctrl/sunplus/sppctl_sp7021.c |  536 +++++++++++++++
 8 files changed, 1793 insertions(+)
 create mode 100644 drivers/pinctrl/sunplus/Kconfig
 create mode 100644 drivers/pinctrl/sunplus/Makefile
 create mode 100644 drivers/pinctrl/sunplus/sppctl.c
 create mode 100644 drivers/pinctrl/sunplus/sppctl.h
 create mode 100644 drivers/pinctrl/sunplus/sppctl_sp7021.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 723bbe3..c19bbfd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15135,6 +15135,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 W:	https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
 F:	Documentation/devicetree/bindings/pinctrl/sunplus,*
+F:	drivers/pinctrl/sunplus/
 F:	include/dt-bindings/pinctrl/sppctl*
 
 PKTCDVD DRIVER
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 6a961d5..5aa4e29 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -459,6 +459,7 @@ source "drivers/pinctrl/samsung/Kconfig"
 source "drivers/pinctrl/spear/Kconfig"
 source "drivers/pinctrl/sprd/Kconfig"
 source "drivers/pinctrl/stm32/Kconfig"
+source "drivers/pinctrl/sunplus/Kconfig"
 source "drivers/pinctrl/sunxi/Kconfig"
 source "drivers/pinctrl/tegra/Kconfig"
 source "drivers/pinctrl/ti/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 5e63de2..9f1eef3 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PINCTRL_SAMSUNG)	+= samsung/
 obj-$(CONFIG_PINCTRL_SPEAR)	+= spear/
 obj-y				+= sprd/
 obj-$(CONFIG_PINCTRL_STM32)	+= stm32/
+obj-y				+= sunplus/
 obj-$(CONFIG_PINCTRL_SUNXI)	+= sunxi/
 obj-y				+= ti/
 obj-$(CONFIG_PINCTRL_UNIPHIER)	+= uniphier/
diff --git a/drivers/pinctrl/sunplus/Kconfig b/drivers/pinctrl/sunplus/Kconfig
new file mode 100644
index 0000000..d9291a7
--- /dev/null
+++ b/drivers/pinctrl/sunplus/Kconfig
@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Sunplus Pin control driver configuration
+#
+
+config PINCTRL_SPPCTL
+	bool "Sunplus SP7021 PinMux and GPIO driver"
+	depends on SOC_SP7021
+	depends on OF && HAS_IOMEM
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	select PINCONF
+	select PINMUX
+	select GPIOLIB
+	select OF_GPIO
+	help
+	  Say Y here to support Sunplus SP7021 pinmux controller.
+	  The driver is selected automatically by platform.
+	  This driver requires the pinctrl framework.
+	  GPIO is provided by the same driver.
diff --git a/drivers/pinctrl/sunplus/Makefile b/drivers/pinctrl/sunplus/Makefile
new file mode 100644
index 0000000..63482da
--- /dev/null
+++ b/drivers/pinctrl/sunplus/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the Sunplus Pin control drivers.
+#
+obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl.o sppctl_sp7021.o
diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c
new file mode 100644
index 0000000..f8b382e
--- /dev/null
+++ b/drivers/pinctrl/sunplus/sppctl.c
@@ -0,0 +1,1074 @@
+// SPDX-License-Identifier: GPL-2.0
+/* SP7021 Pin Controller Driver.
+ * Copyright (C) Sunplus Tech/Tibbo Tech.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+
+#include <dt-bindings/pinctrl/sppctl-sp7021.h>
+#include "../pinctrl-utils.h"
+#include "../core.h"
+#include "sppctl.h"
+
+static void sppctl_func_set(struct sppctl_pdata *pctl, u8 func, u8 val)
+{
+	u32 reg, offset;
+
+	/* Note that upper 16-bit word is mask
+	 * and lower 16-bit word is value.
+	 * Enable mask before write.
+	 */
+	reg = 0x007f0000 | val;	/* Set value and enable mask. */
+
+	if (func & 1)
+		reg <<= 8;
+
+	/* Convert function # to register offset. */
+	offset = func & ~1;
+	offset <<= 1;
+
+	dev_dbg(pctl->pctl_dev->dev, "%s(0x%x, 0x%x): offset: 0x%x, reg: 0x%08x\n",
+		__func__, func, val, offset, reg);
+	writel(reg, pctl->moon2_base + offset);
+}
+
+static u8 sppctl_func_get(struct sppctl_pdata *pctl, u8 func)
+{
+	u32 reg, offset;
+	u8 val;
+
+	/* Convert function # to register offset. */
+	offset = func & ~1;
+	offset <<= 1;
+
+	reg = readl(pctl->moon2_base + offset);
+	if (func & 1)
+		val = reg >> 8;
+	else
+		val = reg;
+	val &= 0x7f;
+
+	dev_dbg(pctl->pctl_dev->dev, "%s(0x%x): offset: 0x%x, reg: 0x%08X, val: 0x%x\n",
+		__func__, func, offset, reg, val);
+
+	return val;
+}
+
+static void sppctl_gmx_set(struct sppctl_pdata *pctl, u8 reg_off, u8 bit_off, u8 bit_sz,
+			   u8 val)
+{
+	u32 mask, reg;
+
+	/* Note that upper 16-bit word is mask
+	 * and lower 16-bit word is value.
+	 * Enable mask before write.
+	 */
+	mask = ~(~0 << bit_sz);
+	reg = (mask << 16) | (val & mask);
+	reg <<= bit_off;
+	writel(reg, pctl->moon1_base + (reg_off << 2));
+
+	dev_dbg(pctl->pctl_dev->dev, "%s(0x%x, 0x%x, 0x%x, 0x%x): reg: 0x%08X\n",
+		__func__, reg_off, bit_off, bit_sz, val, reg);
+}
+
+static int sppctl_first_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));
+
+	dev_dbg(chip->parent, "%s(%u): addr = %p, reg = %08x, val = %d\n",
+		__func__, offset, spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST +
+		R32_ROF(offset), reg, (int)R32_VAL(reg, R32_BOF(offset)));
+
+	return R32_VAL(reg, R32_BOF(offset));
+}
+
+static int sppctl_master_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	reg = readl(spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_MASTER + R16_ROF(offset));
+
+	dev_dbg(chip->parent, "%s(%u): addr = %p, reg = %08x, val = %d\n",
+		__func__, offset, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_MASTER +
+		R16_ROF(offset), reg, (int)R32_VAL(reg, R16_BOF(offset)));
+
+	return R32_VAL(reg, R16_BOF(offset));
+}
+
+static void sppctl_first_master_set(struct gpio_chip *chip, unsigned int offset,
+				    enum mux_f_mg first, enum mux_m_ig master)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* FIRST register */
+	if (first != mux_f_keep) {
+		reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));
+		dev_dbg(chip->parent, "First: %08x (%p)\n", reg, spp_gchip->first_base +
+			SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));
+		if (first != R32_VAL(reg, R32_BOF(offset))) {
+			if (first == mux_f_gpio)
+				reg |= BIT(R32_BOF(offset));
+			else
+				reg &= ~BIT(R32_BOF(offset));
+			dev_dbg(chip->parent, "First: %08x\n", reg);
+			writel(reg, spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST +
+			       R32_ROF(offset));
+		}
+	}
+
+	/* MASTER register */
+	if (master != mux_m_keep) {
+		reg = (BIT(R16_BOF(offset)) << 16);
+		if (master == mux_m_gpio)
+			reg |= BIT(R16_BOF(offset));
+		dev_dbg(chip->parent, "Master: %08x (%p)\n", reg, spp_gchip->gpioxt_base +
+			SPPCTL_GPIO_OFF_MASTER + R16_ROF(offset));
+		writel(reg, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_MASTER +
+		       R16_ROF(offset));
+	}
+}
+
+static void sppctl_gpio_input_inv_set(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
+	writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_IINV + R16_ROF(offset));
+}
+
+static void sppctl_gpio_output_inv_set(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
+	writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OINV + R16_ROF(offset));
+}
+
+static int sppctl_gpio_output_od_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	reg = readl(spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + R16_ROF(offset));
+
+	return R32_VAL(reg, R16_BOF(offset));
+}
+
+static void sppctl_gpio_output_od_set(struct gpio_chip *chip, unsigned int offset,
+				      unsigned int val)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | ((val & BIT(0)) << R16_BOF(offset));
+	writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + R16_ROF(offset));
+}
+
+static int sppctl_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	reg = readl(spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_OE + R16_ROF(offset));
+
+	return R32_VAL(reg, R16_BOF(offset)) ^ BIT(0);
+}
+
+static int sppctl_gpio_inv_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u16 inv_off;
+	u32 reg;
+
+	inv_off = SPPCTL_GPIO_OFF_IINV;
+	if (sppctl_gpio_get_direction(chip, offset) == 0)
+		inv_off = SPPCTL_GPIO_OFF_OINV;
+
+	reg = readl(spp_gchip->gpioxt2_base + inv_off + R16_ROF(offset));
+
+	return R32_VAL(reg, R16_BOF(offset));
+}
+
+static int sppctl_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16);
+	writel(reg, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_OE + R16_ROF(offset));
+
+	return 0;
+}
+
+static int sppctl_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int val)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
+	writel(reg, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_OE + R16_ROF(offset));
+
+	if (val < 0)
+		return 0;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | ((val & BIT(0)) << R16_BOF(offset));
+	writel(reg, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_OUT + R16_ROF(offset));
+
+	return 0;
+}
+
+static int sppctl_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	reg = readl(spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_IN + R32_ROF(offset));
+
+	return R32_VAL(reg, R32_BOF(offset));
+}
+
+static void sppctl_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+	reg = (BIT(R16_BOF(offset)) << 16) | (value & 0x0001) << R16_BOF(offset);
+	writel(reg, spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_OUT + R16_ROF(offset));
+}
+
+static int sppctl_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				  unsigned long config)
+{
+	enum pin_config_param param = pinconf_to_config_param(config);
+	struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
+	u16 arg = pinconf_to_config_argument(config);
+	int ret = 0;
+	u32 reg;
+
+	dev_dbg(chip->parent, "%s(%03d, %lX) param: %d, arg: %d\n", __func__,
+		offset, config, param, arg);
+
+	switch (param) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		/* Upper 16-bit word is mask. Lower 16-bit word is value. */
+		reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
+		writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + R16_ROF(offset));
+		break;
+
+	case PIN_CONFIG_INPUT_ENABLE:
+		dev_dbg(chip->parent, "%s(%03d, %lX) arg: %d\n", __func__,
+			offset, config, arg);
+		break;
+
+	case PIN_CONFIG_OUTPUT:
+		ret = sppctl_gpio_direction_output(chip, offset, 0);
+		break;
+
+	case PIN_CONFIG_PERSIST_STATE:
+		dev_dbg(chip->parent, "%s(%03d, %lX) not support, param: %d\n", __func__,
+			offset, config, param);
+		ret = -EOPNOTSUPP;
+		break;
+
+	default:
+		dev_dbg(chip->parent, "%s(%03d, %lX) unknown, param: %d\n", __func__,
+			offset, config, param);
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static void sppctl_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	const char *label;
+	int i;
+
+	for (i = 0; i < chip->ngpio; i++) {
+		label = gpiochip_is_requested(chip, i);
+		if (!label)
+			label = "";
+
+		seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base,
+			   chip->names[i], label);
+		seq_printf(s, " %c", sppctl_gpio_get_direction(chip, i) == 0 ? 'O' : 'I');
+		seq_printf(s, ":%d", sppctl_gpio_get(chip, i));
+		seq_printf(s, " %s", (sppctl_first_get(chip, i) ? "gpi" : "mux"));
+		seq_printf(s, " %s", (sppctl_master_get(chip, i) ? "gpi" : "iop"));
+		seq_printf(s, " %s", (sppctl_gpio_inv_get(chip, i) ? "inv" : "   "));
+		seq_printf(s, " %s", (sppctl_gpio_output_od_get(chip, i) ? "oDr" : ""));
+		seq_puts(s, "\n");
+	}
+}
+#endif
+
+static int sppctl_gpio_new(struct platform_device *pdev, struct sppctl_pdata *pctl)
+{
+	struct sppctl_gpio_chip *spp_gchip;
+	struct gpio_chip *gchip;
+	int err;
+
+	if (!of_find_property(pdev->dev.of_node, "gpio-controller", NULL)) {
+		dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");
+		return -EINVAL;
+	}
+
+	spp_gchip = devm_kzalloc(&pdev->dev, sizeof(*spp_gchip), GFP_KERNEL);
+	if (!spp_gchip)
+		return -ENOMEM;
+	pctl->spp_gchip = spp_gchip;
+
+	spp_gchip->gpioxt_base  = pctl->gpioxt_base;
+	spp_gchip->gpioxt2_base = pctl->gpioxt2_base;
+	spp_gchip->first_base   = pctl->first_base;
+
+	gchip =                    &spp_gchip->chip;
+	gchip->label =             SPPCTL_MODULE_NAME;
+	gchip->parent =            &pdev->dev;
+	gchip->owner =             THIS_MODULE;
+	gchip->request =           gpiochip_generic_request;
+	gchip->free =              gpiochip_generic_free;
+	gchip->get_direction =     sppctl_gpio_get_direction;
+	gchip->direction_input =   sppctl_gpio_direction_input;
+	gchip->direction_output =  sppctl_gpio_direction_output;
+	gchip->get =               sppctl_gpio_get;
+	gchip->set =               sppctl_gpio_set;
+	gchip->set_config =        sppctl_gpio_set_config;
+#ifdef CONFIG_DEBUG_FS
+	gchip->dbg_show =          sppctl_gpio_dbg_show;
+#endif
+	gchip->base =              0; /* it is main platform GPIO controller */
+	gchip->ngpio =             sppctl_gpio_list_sz;
+	gchip->names =             sppctl_gpio_list_s;
+	gchip->can_sleep =         0;
+	gchip->of_node =           pdev->dev.of_node;
+	gchip->of_gpio_n_cells =   2;
+
+	pctl->pctl_grange.npins = gchip->ngpio;
+	pctl->pctl_grange.base =  gchip->base;
+	pctl->pctl_grange.name =  gchip->label;
+	pctl->pctl_grange.gc =    gchip;
+
+	err = devm_gpiochip_add_data(&pdev->dev, gchip, spp_gchip);
+	if (err) {
+		dev_err_probe(&pdev->dev, err, "Failed to add gpiochip!\n");
+		return err;
+	}
+
+	return 0;
+}
+
+/* pinconf operations */
+static int stpctl_pin_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
+				 unsigned long *config)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int param = pinconf_to_config_param(*config);
+	unsigned int arg = 0;
+
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, pin);
+
+	switch (param) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (!sppctl_gpio_output_od_get(&pctl->spp_gchip->chip, pin))
+			return -EINVAL;
+		break;
+
+	case PIN_CONFIG_OUTPUT:
+		if (!sppctl_first_get(&pctl->spp_gchip->chip, pin))
+			return -EINVAL;
+		if (!sppctl_master_get(&pctl->spp_gchip->chip, pin))
+			return -EINVAL;
+		if (sppctl_gpio_get_direction(&pctl->spp_gchip->chip, pin) != 0)
+			return -EINVAL;
+		arg = sppctl_gpio_get(&pctl->spp_gchip->chip, pin);
+		break;
+
+	default:
+		dev_dbg(pctldev->dev, "%s(%d) skipping, param: 0x%x\n",
+			__func__, pin, param);
+		return -EOPNOTSUPP;
+	}
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int stpctl_pin_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
+				 unsigned long *configs, unsigned int num_configs)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	int i = 0;
+
+	dev_dbg(pctldev->dev, "%s(%d, %ld, %d)\n", __func__, pin, *configs, num_configs);
+
+	/* Special handling for IOP */
+	if (configs[i] == 0xFF) {
+		sppctl_first_master_set(&pctl->spp_gchip->chip, pin, mux_f_gpio, mux_m_iop);
+		return 0;
+	}
+
+	for (i = 0; i < num_configs; i++) {
+		if (configs[i] & SPPCTL_PCTL_L_OUT) {
+			dev_dbg(pctldev->dev, "%d: OUT\n", i);
+			sppctl_gpio_direction_output(&pctl->spp_gchip->chip, pin, 0);
+		}
+		if (configs[i] & SPPCTL_PCTL_L_OU1) {
+			dev_dbg(pctldev->dev, "%d: OU1\n", i);
+			sppctl_gpio_direction_output(&pctl->spp_gchip->chip, pin, 1);
+		}
+		if (configs[i] & SPPCTL_PCTL_L_INV) {
+			dev_dbg(pctldev->dev, "%d: INV\n", i);
+			sppctl_gpio_input_inv_set(&pctl->spp_gchip->chip, pin);
+		}
+		if (configs[i] & SPPCTL_PCTL_L_ONV) {
+			dev_dbg(pctldev->dev, "%d: ONV\n", i);
+			sppctl_gpio_output_inv_set(&pctl->spp_gchip->chip, pin);
+		}
+		if (configs[i] & SPPCTL_PCTL_L_ODR) {
+			dev_dbg(pctldev->dev, "%d: ODR\n", i);
+			sppctl_gpio_output_od_set(&pctl->spp_gchip->chip, pin, 1);
+		}
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static void stpctl_config_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+				   unsigned int offset)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
+	seq_printf(s, " %s", dev_name(pctldev->dev));
+}
+
+static void stpctl_config_group_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+					 unsigned int selector)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, selector);
+}
+
+static void stpctl_config_config_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+					  unsigned long config)
+{
+	dev_dbg(pctldev->dev, "%s(%ld)\n", __func__, config);
+}
+#endif
+
+static const struct pinconf_ops sppctl_pconf_ops = {
+	.is_generic                 = true,
+	.pin_config_get             = stpctl_pin_config_get,
+	.pin_config_set             = stpctl_pin_config_set,
+#ifdef CONFIG_DEBUG_FS
+	.pin_config_dbg_show        = stpctl_config_dbg_show,
+	.pin_config_group_dbg_show  = stpctl_config_group_dbg_show,
+	.pin_config_config_dbg_show = stpctl_config_config_dbg_show,
+#endif
+};
+
+/* pinmux operations */
+static int stpctl_request(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
+	return 0;
+}
+
+static int stpctl_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
+	return 0;
+}
+
+static int stpctl_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return sppctl_list_funcs_sz;
+}
+
+static const char *stpctl_get_function_name(struct pinctrl_dev *pctldev,
+					    unsigned int selector)
+{
+	return sppctl_list_funcs[selector].name;
+}
+
+static int stpctl_get_function_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+				      const char * const **groups, unsigned int *num_groups)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct sppctl_func *f = &sppctl_list_funcs[selector];
+
+	*num_groups = 0;
+	switch (f->freg) {
+	case f_off_i:
+	case f_off_0:   /* gen GPIO/IOP: all groups = all pins */
+		*num_groups = sppctl_gpio_list_sz;
+		*groups = sppctl_gpio_list_s;
+		break;
+
+	case f_off_m:   /* pin-mux */
+		*num_groups = sppctl_pmux_list_sz;
+		*groups = sppctl_pmux_list_s;
+		break;
+
+	case f_off_g:   /* pin-group */
+		if (!f->grps)
+			break;
+		*num_groups = f->gnum;
+		*groups = &pctl->groups_name[selector * SPPCTL_MAX_GROUPS];
+		break;
+
+	default:
+		dev_err(pctldev->dev, "%s(selector: %d) unknown fOFF %d\n", __func__,
+			selector, f->freg);
+		break;
+	}
+
+	dev_dbg(pctldev->dev, "%s(selector: %d) %d\n", __func__, selector, *num_groups);
+	return 0;
+}
+
+static int stpctl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
+			  unsigned int group_selector)
+{
+	const struct sppctl_func *f = &sppctl_list_funcs[func_selector];
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct grp2fp_map g2fpm = pctl->g2fp_maps[group_selector];
+	int i = -1, j = -1;
+
+	dev_dbg(pctldev->dev, "%s(func: %d, grp: %d)\n", __func__,
+		func_selector, group_selector);
+
+	switch (f->freg) {
+	case f_off_0:   /* GPIO. detouch from all funcs - ? */
+		for (i = 0; i < sppctl_list_funcs_sz; i++) {
+			if (sppctl_list_funcs[i].freg != f_off_m)
+				continue;
+			j++;
+			if (sppctl_func_get(pctl, j) != group_selector)
+				continue;
+			sppctl_func_set(pctl, j, 0);
+		}
+		break;
+
+	case f_off_m:   /* Mux */
+		sppctl_first_master_set(&pctl->spp_gchip->chip, group_selector,
+					mux_f_mux, mux_m_keep);
+		sppctl_func_set(pctl, func_selector - 2, (group_selector == 0) ?
+				group_selector : group_selector - 7);
+		break;
+
+	case f_off_g:   /* Group */
+		for (i = 0; i < f->grps[g2fpm.g_idx].pnum; i++)
+			sppctl_first_master_set(&pctl->spp_gchip->chip,
+						f->grps[g2fpm.g_idx].pins[i],
+						mux_f_mux, mux_m_keep);
+		sppctl_gmx_set(pctl, f->roff, f->boff, f->blen, f->grps[g2fpm.g_idx].gval);
+		break;
+
+	case f_off_i:   /* IOP */
+		sppctl_first_master_set(&pctl->spp_gchip->chip, group_selector,
+					mux_f_gpio, mux_m_iop);
+		break;
+
+	default:
+		dev_err(pctldev->dev, "%s(func_selector: %d) unknown f_off: %d\n",
+			__func__, func_selector, f->freg);
+		break;
+	}
+
+	return 0;
+}
+
+static int stpctl_gpio_request_enable(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range, unsigned int offset)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct pin_desc *pdesc;
+	int g_f, g_m;
+
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
+
+	g_f = sppctl_first_get(&pctl->spp_gchip->chip, offset);
+	g_m = sppctl_master_get(&pctl->spp_gchip->chip, offset);
+	if (g_f == mux_f_gpio && g_m == mux_m_gpio)
+		return 0;
+
+	pdesc = pin_desc_get(pctldev, offset);
+	if (pdesc->mux_owner)
+		return -EACCES;
+
+	sppctl_first_master_set(&pctl->spp_gchip->chip, offset, mux_f_gpio, mux_m_gpio);
+	return 0;
+}
+
+static void stpctl_gpio_disable_free(struct pinctrl_dev *pctldev,
+				     struct pinctrl_gpio_range *range, unsigned int offset)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
+}
+
+static int stpctl_gpio_set_direction(struct pinctrl_dev *pctldev,
+				     struct pinctrl_gpio_range *range, unsigned int offset,
+				     bool input)
+{
+	dev_dbg(pctldev->dev, "%s(%d,%d)\n", __func__, offset, input);
+	return 0;
+}
+
+static const struct pinmux_ops sppctl_pinmux_ops = {
+	.request             = stpctl_request,
+	.free                = stpctl_free,
+	.get_functions_count = stpctl_get_functions_count,
+	.get_function_name   = stpctl_get_function_name,
+	.get_function_groups = stpctl_get_function_groups,
+	.set_mux             = stpctl_set_mux,
+	.gpio_request_enable = stpctl_gpio_request_enable,
+	.gpio_disable_free   = stpctl_gpio_disable_free,
+	.gpio_set_direction  = stpctl_gpio_set_direction,
+	.strict              = 1
+};
+
+/* pinctrl operations */
+static int stpctl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->unq_grps_sz;
+}
+
+static const char *stpctl_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->unq_grps[selector];
+}
+
+static int stpctl_get_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
+				 const unsigned int **pins, unsigned int *num_pins)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct grp2fp_map g2fpm = pctl->g2fp_maps[selector];
+	const struct sppctl_func *f;
+
+	f = &sppctl_list_funcs[g2fpm.f_idx];
+	dev_dbg(pctldev->dev, "%s(%d), f_idx: %d, g_idx: %d, freg: %d\n",
+		__func__, selector, g2fpm.f_idx, g2fpm.g_idx, f->freg);
+
+	*num_pins = 0;
+
+	/* MUX | GPIO | IOP: 1 pin -> 1 group */
+	if (f->freg != f_off_g) {
+		*num_pins = 1;
+		*pins = &sppctl_pins_gpio[selector];
+		return 0;
+	}
+
+	/* IOP (several pins at once in a group) */
+	if (!f->grps)
+		return 0;
+	if (f->gnum < 1)
+		return 0;
+
+	*num_pins = f->grps[g2fpm.g_idx].pnum;
+	*pins = f->grps[g2fpm.g_idx].pins;
+
+	return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static void stpctl_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+				unsigned int offset)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const char *tmpp;
+	u8 g_f, g_m;
+
+	seq_printf(s, "%s", dev_name(pctldev->dev));
+	g_f = sppctl_first_get(&pctl->spp_gchip->chip, offset);
+	g_m = sppctl_master_get(&pctl->spp_gchip->chip, offset);
+
+	tmpp = "?";
+	if (g_f &&  g_m)
+		tmpp = "GPIO";
+	if (g_f && !g_m)
+		tmpp = " IOP";
+	if (!g_f)
+		tmpp = " MUX";
+	seq_printf(s, " %s", tmpp);
+}
+#endif
+
+static int stpctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
+				 struct pinctrl_map **map, unsigned int *num_maps)
+{
+	struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
+	int nmG = of_property_count_strings(np_config, "groups");
+	const struct sppctl_func *f = NULL;
+	struct device_node *parent;
+	unsigned long *configs;
+	struct property *prop;
+	const char *s_f, *s_g;
+	u8 p_p, p_g, p_f, p_l;
+	const __be32 *list;
+	u32 dt_pin, dt_fun;
+	int i, size = 0;
+
+	list = of_get_property(np_config, "pins", &size);
+
+	if (nmG <= 0)
+		nmG = 0;
+
+	parent = of_get_parent(np_config);
+	*num_maps = size / sizeof(*list);
+
+	/* Check if out of range or invalid? */
+	for (i = 0; i < (*num_maps); i++) {
+		dt_pin = be32_to_cpu(list[i]);
+		p_p = SPPCTL_PCTLD_P(dt_pin);
+		p_g = SPPCTL_PCTLD_G(dt_pin);
+
+		if (p_p >= sppctl_pins_all_sz) {
+			dev_dbg(pctldev->dev, "Invalid pin property at index %d (0x%08x)\n",
+				i, dt_pin);
+			return -EINVAL;
+		}
+	}
+
+	*map = kcalloc(*num_maps + nmG, sizeof(**map), GFP_KERNEL);
+	for (i = 0; i < (*num_maps); i++) {
+		dt_pin = be32_to_cpu(list[i]);
+		p_p = SPPCTL_PCTLD_P(dt_pin);
+		p_g = SPPCTL_PCTLD_G(dt_pin);
+		p_f = SPPCTL_PCTLD_F(dt_pin);
+		p_l = SPPCTL_PCTLD_L(dt_pin);
+		(*map)[i].name = parent->name;
+		dev_dbg(pctldev->dev, "map [%d]=%08x, p=%d, g=%d, f=%d, l=%d\n",
+			i, dt_pin, p_p, p_g, p_f, p_l);
+
+		if (p_g == SPPCTL_PCTL_G_GPIO) {
+			(*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			(*map)[i].data.configs.num_configs = 1;
+			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, p_p);
+			configs = kcalloc(1, sizeof(*configs), GFP_KERNEL);
+			*configs = p_l;
+			(*map)[i].data.configs.configs = configs;
+
+			dev_dbg(pctldev->dev, "%s(%d) = 0x%x\n",
+				(*map)[i].data.configs.group_or_pin, p_p, p_l);
+		} else if (p_g == SPPCTL_PCTL_G_IOPP) {
+			(*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			(*map)[i].data.configs.num_configs = 1;
+			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, p_p);
+			configs = kcalloc(1, sizeof(*configs), GFP_KERNEL);
+			*configs = 0xFF;
+			(*map)[i].data.configs.configs = configs;
+
+			dev_dbg(pctldev->dev, "%s(%d) = 0x%x\n",
+				(*map)[i].data.configs.group_or_pin, p_p, p_l);
+		} else {
+			(*map)[i].type = PIN_MAP_TYPE_MUX_GROUP;
+			(*map)[i].data.mux.function = sppctl_list_funcs[p_f].name;
+			(*map)[i].data.mux.group = pin_get_name(pctldev, p_p);
+
+			dev_dbg(pctldev->dev, "f->p: %s(%d)->%s(%d)\n",
+				(*map)[i].data.mux.function, p_f,
+				(*map)[i].data.mux.group, p_p);
+		}
+	}
+
+	/* Handle pin-group function. */
+	if (nmG > 0 && of_property_read_string(np_config, "function", &s_f) == 0) {
+		dev_dbg(pctldev->dev, "found func: %s\n", s_f);
+		of_property_for_each_string(np_config, "groups", prop, s_g) {
+			dev_dbg(pctldev->dev, " %s: %s\n", s_f, s_g);
+			(*map)[*num_maps].type = PIN_MAP_TYPE_MUX_GROUP;
+			(*map)[*num_maps].data.mux.function = s_f;
+			(*map)[*num_maps].data.mux.group = s_g;
+			dev_dbg(pctldev->dev, "f->g: %s->%s\n",
+				(*map)[*num_maps].data.mux.function,
+				(*map)[*num_maps].data.mux.group);
+			(*num_maps)++;
+		}
+	}
+
+	/* Handle zero function. */
+	list = of_get_property(np_config, "zero_func", &size);
+	if (list) {
+		for (i = 0; i < (size / sizeof(*list)); i++) {
+			dt_fun = be32_to_cpu(list[i]);
+			if (dt_fun >= sppctl_list_funcs_sz) {
+				dev_err(pctldev->dev, "Zero-func %d out of range!\n",
+					dt_fun);
+				continue;
+			}
+
+			f = &sppctl_list_funcs[dt_fun];
+			switch (f->freg) {
+			case f_off_m:
+				dev_dbg(pctldev->dev, "Zero-func: %d (%s)\n",
+					dt_fun, f->name);
+				sppctl_func_set(pctl, dt_fun - 2, 0);
+				break;
+
+			case f_off_g:
+				dev_dbg(pctldev->dev, "zero-group: %d (%s)\n",
+					dt_fun, f->name);
+				sppctl_gmx_set(pctl, f->roff, f->boff, f->blen, 0);
+				break;
+
+			default:
+				dev_err(pctldev->dev, "Wrong zero-group: %d (%s)\n",
+					dt_fun, f->name);
+				break;
+			}
+		}
+	}
+
+	of_node_put(parent);
+	dev_dbg(pctldev->dev, "%d pins mapped\n", *num_maps);
+	return 0;
+}
+
+static void stpctl_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
+			       unsigned int num_maps)
+{
+	dev_dbg(pctldev->dev, "%s(%d)\n", __func__, num_maps);
+	pinctrl_utils_free_map(pctldev, map, num_maps);
+}
+
+static const struct pinctrl_ops sppctl_pctl_ops = {
+	.get_groups_count = stpctl_get_groups_count,
+	.get_group_name   = stpctl_get_group_name,
+	.get_group_pins   = stpctl_get_group_pins,
+#ifdef CONFIG_DEBUG_FS
+	.pin_dbg_show     = stpctl_pin_dbg_show,
+#endif
+	.dt_node_to_map   = stpctl_dt_node_to_map,
+	.dt_free_map      = stpctl_dt_free_map,
+};
+
+/* platform driver functions */
+static int sppctl_group_groups(struct platform_device *pdev)
+{
+	struct sppctl_pdata *sppctl = pdev->dev.platform_data;
+	const char *name;
+	int i, k, j;
+
+	/* Fill array of all groups. */
+	sppctl->unq_grps = NULL;
+	sppctl->unq_grps_sz = sppctl_gpio_list_sz;
+
+	/* Calculate unique group names array size. */
+	for (i = 0; i < sppctl_list_funcs_sz; i++)
+		if (sppctl_list_funcs[i].freg == f_off_g)
+			sppctl->unq_grps_sz += sppctl_list_funcs[i].gnum;
+
+	/* Fill up unique group names array. */
+	sppctl->unq_grps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) *
+					sizeof(char *), GFP_KERNEL);
+	if (!sppctl->unq_grps)
+		return -ENOMEM;
+
+	sppctl->g2fp_maps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) *
+					 sizeof(struct grp2fp_map), GFP_KERNEL);
+	if (!sppctl->g2fp_maps)
+		return -ENOMEM;
+
+	sppctl->groups_name = devm_kzalloc(&pdev->dev, sppctl_list_funcs_sz *
+					   SPPCTL_MAX_GROUPS * sizeof(char *), GFP_KERNEL);
+	if (!sppctl->groups_name)
+		return -ENOMEM;
+
+	/* gpio */
+	for (i = 0; i < sppctl_gpio_list_sz; i++) {
+		sppctl->unq_grps[i] = sppctl_gpio_list_s[i];
+		sppctl->g2fp_maps[i].f_idx = 0;
+		sppctl->g2fp_maps[i].g_idx = i;
+	}
+
+	/* groups */
+	j = sppctl_gpio_list_sz;
+	for (i = 0; i < sppctl_list_funcs_sz; i++) {
+		if (sppctl_list_funcs[i].freg != f_off_g)
+			continue;
+
+		for (k = 0; k < sppctl_list_funcs[i].gnum; k++) {
+			name = sppctl_list_funcs[i].grps[k].name;
+			sppctl->groups_name[i * SPPCTL_MAX_GROUPS + k] = name;
+			sppctl->unq_grps[j] = name;
+			sppctl->g2fp_maps[j].f_idx = i;
+			sppctl->g2fp_maps[j].g_idx = k;
+			j++;
+		}
+	}
+
+	dev_dbg(&pdev->dev, "funcs: %zd unq_grps: %zd\n", sppctl_list_funcs_sz,
+		sppctl->unq_grps_sz);
+	return 0;
+}
+
+static int sppctl_pinctrl_init(struct platform_device *pdev)
+{
+	struct device_node *np = of_node_get(pdev->dev.of_node);
+	struct sppctl_pdata *sppctl = pdev->dev.platform_data;
+	int err;
+
+	/* Initialize pctl_desc */
+	sppctl->pctl_desc.owner   = THIS_MODULE;
+	sppctl->pctl_desc.name    = dev_name(&pdev->dev);
+	sppctl->pctl_desc.pins    = &sppctl_pins_all[0];
+	sppctl->pctl_desc.npins   = sppctl_pins_all_sz;
+	sppctl->pctl_desc.pctlops = &sppctl_pctl_ops;
+	sppctl->pctl_desc.confops = &sppctl_pconf_ops;
+	sppctl->pctl_desc.pmxops  = &sppctl_pinmux_ops;
+
+	err = sppctl_group_groups(pdev);
+	if (err) {
+		of_node_put(np);
+		return err;
+	}
+
+	err = devm_pinctrl_register_and_init(&pdev->dev, &sppctl->pctl_desc,
+					     sppctl, &sppctl->pctl_dev);
+	if (err) {
+		dev_err_probe(&pdev->dev, err, "Failed to register pinctrl!\n");
+		of_node_put(np);
+		return err;
+	}
+
+	pinctrl_enable(sppctl->pctl_dev);
+	return 0;
+}
+
+static int sppctl_resource_map(struct platform_device *pdev, struct sppctl_pdata *sppctl)
+{
+	struct resource *rp;
+	int ret;
+
+	/* MOON2 registers */
+	rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon2");
+	sppctl->moon2_base = devm_ioremap_resource(&pdev->dev, rp);
+	if (IS_ERR(sppctl->moon2_base)) {
+		ret = PTR_ERR(sppctl->moon2_base);
+		goto ioremap_failed;
+	}
+	dev_dbg(&pdev->dev, "MOON2:   %pr\n", rp);
+
+	/* GPIOXT registers */
+	rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpioxt");
+	sppctl->gpioxt_base = devm_ioremap_resource(&pdev->dev, rp);
+	if (IS_ERR(sppctl->gpioxt_base)) {
+		ret = PTR_ERR(sppctl->gpioxt_base);
+		goto ioremap_failed;
+	}
+	dev_dbg(&pdev->dev, "GPIOXT:  %pr\n", rp);
+
+	/* GPIOXT 2 registers */
+	rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpioxt2");
+	sppctl->gpioxt2_base = devm_ioremap_resource(&pdev->dev, rp);
+	if (IS_ERR(sppctl->gpioxt2_base)) {
+		ret = PTR_ERR(sppctl->gpioxt2_base);
+		goto ioremap_failed;
+	}
+	dev_dbg(&pdev->dev, "GPIOXT2: %pr\n", rp);
+
+	/* FIRST registers */
+	rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "first");
+	sppctl->first_base = devm_ioremap_resource(&pdev->dev, rp);
+	if (IS_ERR(sppctl->first_base)) {
+		ret = PTR_ERR(sppctl->first_base);
+		goto ioremap_failed;
+	}
+	dev_dbg(&pdev->dev, "FIRST:   %pr\n", rp);
+
+	/* MOON1 registers */
+	rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon1");
+	sppctl->moon1_base = devm_ioremap_resource(&pdev->dev, rp);
+	if (IS_ERR(sppctl->moon1_base)) {
+		ret = PTR_ERR(sppctl->moon1_base);
+		goto ioremap_failed;
+	}
+	dev_dbg(&pdev->dev, "MOON1:   %pr\n", rp);
+
+	return 0;
+
+ioremap_failed:
+	dev_err_probe(&pdev->dev, ret, "ioremap failed!\n");
+	return ret;
+}
+
+static int sppctl_probe(struct platform_device *pdev)
+{
+	struct sppctl_pdata *sppctl;
+	int ret;
+
+	sppctl = devm_kzalloc(&pdev->dev, sizeof(*sppctl), GFP_KERNEL);
+	if (!sppctl)
+		return -ENOMEM;
+	pdev->dev.platform_data = sppctl;
+
+	ret = sppctl_resource_map(pdev, sppctl);
+	if (ret)
+		return ret;
+
+	ret = sppctl_gpio_new(pdev, sppctl);
+	if (ret)
+		return ret;
+
+	ret = sppctl_pinctrl_init(pdev);
+	if (ret)
+		return ret;
+
+	pinctrl_add_gpio_range(sppctl->pctl_dev, &sppctl->pctl_grange);
+	dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech. (C) 2020");
+
+	return 0;
+}
+
+static int sppctl_remove(struct platform_device *pdev)
+{
+	struct sppctl_pdata *sppctl = pdev->dev.platform_data;
+
+	devm_pinctrl_unregister(&pdev->dev, sppctl->pctl_dev);
+	return 0;
+}
+
+static const struct of_device_id sppctl_match_table[] = {
+	{ .compatible = "sunplus,sp7021-pctl" },
+	{ /* zero */ }
+};
+
+static struct platform_driver sppctl_pinctrl_driver = {
+	.driver = {
+		.name           = SPPCTL_MODULE_NAME,
+		.owner          = THIS_MODULE,
+		.of_match_table = sppctl_match_table,
+	},
+	.probe  = sppctl_probe,
+	.remove = sppctl_remove,
+};
+builtin_platform_driver(sppctl_pinctrl_driver)
+
+MODULE_AUTHOR("Dvorkin Dmitry <dvorkin@tibbo.com>");
+MODULE_AUTHOR("Wells Lu <wellslutw@gmail.com>");
+MODULE_DESCRIPTION("Sunplus SP7021 Pin Control and GPIO driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pinctrl/sunplus/sppctl.h b/drivers/pinctrl/sunplus/sppctl.h
new file mode 100644
index 0000000..49a59fe
--- /dev/null
+++ b/drivers/pinctrl/sunplus/sppctl.h
@@ -0,0 +1,154 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* SP7021 Pin Controller Driver.
+ * Copyright (C) Sunplus Tech/Tibbo Tech.
+ */
+
+#ifndef __SPPCTL_H__
+#define __SPPCTL_H__
+
+#define SPPCTL_MODULE_NAME	"sppctl_sp7021"
+#define SPPCTL_MAX_GROUPS	5
+
+#define SPPCTL_GPIO_OFF_FIRST	0x00
+#define SPPCTL_GPIO_OFF_MASTER	0x00
+#define SPPCTL_GPIO_OFF_OE	0x20
+#define SPPCTL_GPIO_OFF_OUT	0x40
+#define SPPCTL_GPIO_OFF_IN	0x60
+#define SPPCTL_GPIO_OFF_IINV	0x00
+#define SPPCTL_GPIO_OFF_OINV	0x20
+#define SPPCTL_GPIO_OFF_OD	0x40
+
+/* (/16)*4 */
+#define R16_ROF(r)		(((r) >> 4) << 2)
+#define R16_BOF(r)		((r) % 16)
+
+/* (/32)*4 */
+#define R32_ROF(r)		(((r) >> 5) << 2)
+#define R32_BOF(r)		((r) % 32)
+#define R32_VAL(r, boff)	(((r) >> (boff)) & BIT(0))
+
+#define FNCE(n, r, o, bo, bl, g) { \
+	.name = n, \
+	.freg = r, \
+	.roff = o, \
+	.boff = bo, \
+	.blen = bl, \
+	.grps = (g), \
+	.gnum = ARRAY_SIZE(g), \
+}
+
+#define FNCN(n, r, o, bo, bl) { \
+	.name = n, \
+	.freg = r, \
+	.roff = o, \
+	.boff = bo, \
+	.blen = bl, \
+	.grps = NULL, \
+	.gnum = 0, \
+}
+
+#define EGRP(n, v, p) { \
+	.name = n, \
+	.gval = (v), \
+	.pins = (p), \
+	.pnum = ARRAY_SIZE(p), \
+}
+
+/* FIRST register:
+ *   0: MUX
+ *   1: GPIO/IOP
+ *   2: No change
+ */
+enum mux_f_mg {
+	mux_f_mux = 0,
+	mux_f_gpio = 1,
+	mux_f_keep = 2,
+};
+
+/* MASTER register:
+ *   0: IOP
+ *   1: GPIO
+ *   2: No change
+ */
+enum mux_m_ig {
+	mux_m_iop = 0,
+	mux_m_gpio = 1,
+	mux_m_keep = 2,
+};
+
+enum f_off {
+	f_off_0,	/* nowhere          */
+	f_off_m,	/* mux registers    */
+	f_off_g,	/* group registers  */
+	f_off_i,	/* iop registers    */
+};
+
+struct grp2fp_map {
+	u16 f_idx;      /* function index   */
+	u16 g_idx;      /* pins/group index */
+};
+
+struct sppctl_sdata {
+	u8 i;
+	u8 ridx;
+	struct sppctl_pdata *pdata;
+};
+
+struct sppctl_gpio_chip {
+	void __iomem *gpioxt_base;	/* MASTER, OE, OUT, IN */
+	void __iomem *gpioxt2_base;	/* I_INV, O_INV, OD    */
+	void __iomem *first_base;	/* GPIO_FIRST          */
+
+	struct gpio_chip chip;
+};
+
+struct sppctl_pdata {
+	/* base addresses */
+	void __iomem *moon2_base;	/* MOON2               */
+	void __iomem *gpioxt_base;	/* MASTER, OE, OUT, IN */
+	void __iomem *gpioxt2_base;	/* I_INV, O_INV, OD    */
+	void __iomem *first_base;	/* FIRST               */
+	void __iomem *moon1_base;	/* MOON1               */
+
+	/* pinctrl and gpio-chip */
+	struct pinctrl_desc pctl_desc;
+	struct pinctrl_dev *pctl_dev;
+	struct pinctrl_gpio_range pctl_grange;
+	struct sppctl_gpio_chip *spp_gchip;
+
+	/* others */
+	char const **unq_grps;
+	struct grp2fp_map *g2fp_maps;
+	size_t unq_grps_sz;
+	const char **groups_name;
+};
+
+struct sppctl_grp {
+	const char * const name;
+	const u8 gval;                  /* group number    */
+	const unsigned * const pins;    /* list of pins    */
+	const unsigned int pnum;        /* number of pins  */
+};
+
+struct sppctl_func {
+	const char * const name;
+	const enum f_off freg;          /* function register type */
+	const u8 roff;                  /* register offset        */
+	const u8 boff;                  /* bit offset             */
+	const u8 blen;                  /* bit length             */
+	const struct sppctl_grp * const grps; /* list of groups   */
+	const unsigned int gnum;        /* number of groups       */
+};
+
+extern const struct sppctl_func sppctl_list_funcs[];
+extern const char * const sppctl_pmux_list_s[];
+extern const char * const sppctl_gpio_list_s[];
+extern const struct pinctrl_pin_desc sppctl_pins_all[];
+extern const unsigned int sppctl_pins_gpio[];
+
+extern const size_t sppctl_list_funcs_sz;
+extern const size_t sppctl_pmux_list_sz;
+extern const size_t sppctl_gpio_list_sz;
+extern const size_t sppctl_pins_all_sz;
+
+#endif
diff --git a/drivers/pinctrl/sunplus/sppctl_sp7021.c b/drivers/pinctrl/sunplus/sppctl_sp7021.c
new file mode 100644
index 0000000..457342e
--- /dev/null
+++ b/drivers/pinctrl/sunplus/sppctl_sp7021.c
@@ -0,0 +1,536 @@
+// SPDX-License-Identifier: GPL-2.0
+/* SP7021 Pin Controller Driver.
+ * Copyright (C) Sunplus Tech/Tibbo Tech.
+ */
+
+#include <linux/gpio/driver.h>
+
+#include "sppctl.h"
+
+#define D_PIS(x, y)	"P" __stringify(x) "_0" __stringify(y)
+#define D(x, y)		((x) * 8 + (y))
+#define P(x, y)		PINCTRL_PIN(D(x, y), D_PIS(x, y))
+
+const char * const sppctl_gpio_list_s[] = {
+	D_PIS(0, 0),  D_PIS(0, 1),  D_PIS(0, 2),  D_PIS(0, 3),
+	D_PIS(0, 4),  D_PIS(0, 5),  D_PIS(0, 6),  D_PIS(0, 7),
+	D_PIS(1, 0),  D_PIS(1, 1),  D_PIS(1, 2),  D_PIS(1, 3),
+	D_PIS(1, 4),  D_PIS(1, 5),  D_PIS(1, 6),  D_PIS(1, 7),
+	D_PIS(2, 0),  D_PIS(2, 1),  D_PIS(2, 2),  D_PIS(2, 3),
+	D_PIS(2, 4),  D_PIS(2, 5),  D_PIS(2, 6),  D_PIS(2, 7),
+	D_PIS(3, 0),  D_PIS(3, 1),  D_PIS(3, 2),  D_PIS(3, 3),
+	D_PIS(3, 4),  D_PIS(3, 5),  D_PIS(3, 6),  D_PIS(3, 7),
+	D_PIS(4, 0),  D_PIS(4, 1),  D_PIS(4, 2),  D_PIS(4, 3),
+	D_PIS(4, 4),  D_PIS(4, 5),  D_PIS(4, 6),  D_PIS(4, 7),
+	D_PIS(5, 0),  D_PIS(5, 1),  D_PIS(5, 2),  D_PIS(5, 3),
+	D_PIS(5, 4),  D_PIS(5, 5),  D_PIS(5, 6),  D_PIS(5, 7),
+	D_PIS(6, 0),  D_PIS(6, 1),  D_PIS(6, 2),  D_PIS(6, 3),
+	D_PIS(6, 4),  D_PIS(6, 5),  D_PIS(6, 6),  D_PIS(6, 7),
+	D_PIS(7, 0),  D_PIS(7, 1),  D_PIS(7, 2),  D_PIS(7, 3),
+	D_PIS(7, 4),  D_PIS(7, 5),  D_PIS(7, 6),  D_PIS(7, 7),
+	D_PIS(8, 0),  D_PIS(8, 1),  D_PIS(8, 2),  D_PIS(8, 3),
+	D_PIS(8, 4),  D_PIS(8, 5),  D_PIS(8, 6),  D_PIS(8, 7),
+	D_PIS(9, 0),  D_PIS(9, 1),  D_PIS(9, 2),  D_PIS(9, 3),
+	D_PIS(9, 4),  D_PIS(9, 5),  D_PIS(9, 6),  D_PIS(9, 7),
+	D_PIS(10, 0), D_PIS(10, 1), D_PIS(10, 2), D_PIS(10, 3),
+	D_PIS(10, 4), D_PIS(10, 5), D_PIS(10, 6), D_PIS(10, 7),
+	D_PIS(11, 0), D_PIS(11, 1), D_PIS(11, 2), D_PIS(11, 3),
+	D_PIS(11, 4), D_PIS(11, 5), D_PIS(11, 6), D_PIS(11, 7),
+	D_PIS(12, 0), D_PIS(12, 1), D_PIS(12, 2)
+};
+
+const size_t sppctl_gpio_list_sz = ARRAY_SIZE(sppctl_gpio_list_s);
+
+/* function: GPIO. list of groups (pins) */
+const unsigned int sppctl_pins_gpio[] = {
+	D(0, 0), D(0, 1), D(0, 2), D(0, 3), D(0, 4), D(0, 5), D(0, 6), D(0, 7),
+	D(1, 0), D(1, 1), D(1, 2), D(1, 3), D(1, 4), D(1, 5), D(1, 6), D(1, 7),
+	D(2, 0), D(2, 1), D(2, 2), D(2, 3), D(2, 4), D(2, 5), D(2, 6), D(2, 7),
+	D(3, 0), D(3, 1), D(3, 2), D(3, 3), D(3, 4), D(3, 5), D(3, 6), D(3, 7),
+	D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4), D(4, 5), D(4, 6), D(4, 7),
+	D(5, 0), D(5, 1), D(5, 2), D(5, 3), D(5, 4), D(5, 5), D(5, 6), D(5, 7),
+	D(6, 0), D(6, 1), D(6, 2), D(6, 3), D(6, 4), D(6, 5), D(6, 6), D(6, 7),
+	D(7, 0), D(7, 1), D(7, 2), D(7, 3), D(7, 4), D(7, 5), D(7, 6), D(7, 7),
+	D(8, 0), D(8, 1), D(8, 2), D(8, 3), D(8, 4), D(8, 5), D(8, 6), D(8, 7),
+	D(9, 0), D(9, 1), D(9, 2), D(9, 3), D(9, 4), D(9, 5), D(9, 6), D(9, 7),
+	D(10, 0), D(10, 1), D(10, 2), D(10, 3), D(10, 4), D(10, 5), D(10, 6), D(10, 7),
+	D(11, 0), D(11, 1), D(11, 2), D(11, 3), D(11, 4), D(11, 5), D(11, 6), D(11, 7),
+	D(12, 0), D(12, 1), D(12, 2)
+};
+
+const struct pinctrl_pin_desc sppctl_pins_all[] = {
+	/* gpio and iop only */
+	P(0, 0), P(0, 1), P(0, 2), P(0, 3), P(0, 4), P(0, 5), P(0, 6), P(0, 7),
+	/* gpio, iop, muxable */
+	P(1, 0), P(1, 1), P(1, 2), P(1, 3), P(1, 4), P(1, 5), P(1, 6), P(1, 7),
+	P(2, 0), P(2, 1), P(2, 2), P(2, 3), P(2, 4), P(2, 5), P(2, 6), P(2, 7),
+	P(3, 0), P(3, 1), P(3, 2), P(3, 3), P(3, 4), P(3, 5), P(3, 6), P(3, 7),
+	P(4, 0), P(4, 1), P(4, 2), P(4, 3), P(4, 4), P(4, 5), P(4, 6), P(4, 7),
+	P(5, 0), P(5, 1), P(5, 2), P(5, 3), P(5, 4), P(5, 5), P(5, 6), P(5, 7),
+	P(6, 0), P(6, 1), P(6, 2), P(6, 3), P(6, 4), P(6, 5), P(6, 6), P(6, 7),
+	P(7, 0), P(7, 1), P(7, 2), P(7, 3), P(7, 4), P(7, 5), P(7, 6), P(7, 7),
+	P(8, 0), P(8, 1), P(8, 2), P(8, 3), P(8, 4), P(8, 5), P(8, 6), P(8, 7),
+	/* gpio (not wired) and iop only */
+	P(9, 0),  P(9, 1),  P(9, 2),  P(9, 3),  P(9, 4),  P(9, 5),  P(9, 6),  P(9, 7),
+	P(10, 0), P(10, 1), P(10, 2), P(10, 3), P(10, 4), P(10, 5), P(10, 6), P(10, 7),
+	P(11, 0), P(11, 1), P(11, 2), P(11, 3), P(11, 4), P(11, 5), P(11, 6), P(11, 7),
+	P(12, 0), P(12, 1), P(12, 2)
+};
+
+const size_t sppctl_pins_all_sz = ARRAY_SIZE(sppctl_pins_all);
+
+/* pmux groups: some pins are muxable. group = pin */
+const char * const sppctl_pmux_list_s[] = {
+	D_PIS(0, 0),
+	D_PIS(1, 0), D_PIS(1, 1), D_PIS(1, 2), D_PIS(1, 3),
+	D_PIS(1, 4), D_PIS(1, 5), D_PIS(1, 6), D_PIS(1, 7),
+	D_PIS(2, 0), D_PIS(2, 1), D_PIS(2, 2), D_PIS(2, 3),
+	D_PIS(2, 4), D_PIS(2, 5), D_PIS(2, 6), D_PIS(2, 7),
+	D_PIS(3, 0), D_PIS(3, 1), D_PIS(3, 2), D_PIS(3, 3),
+	D_PIS(3, 4), D_PIS(3, 5), D_PIS(3, 6), D_PIS(3, 7),
+	D_PIS(4, 0), D_PIS(4, 1), D_PIS(4, 2), D_PIS(4, 3),
+	D_PIS(4, 4), D_PIS(4, 5), D_PIS(4, 6), D_PIS(4, 7),
+	D_PIS(5, 0), D_PIS(5, 1), D_PIS(5, 2), D_PIS(5, 3),
+	D_PIS(5, 4), D_PIS(5, 5), D_PIS(5, 6), D_PIS(5, 7),
+	D_PIS(6, 0), D_PIS(6, 1), D_PIS(6, 2), D_PIS(6, 3),
+	D_PIS(6, 4), D_PIS(6, 5), D_PIS(6, 6), D_PIS(6, 7),
+	D_PIS(7, 0), D_PIS(7, 1), D_PIS(7, 2), D_PIS(7, 3),
+	D_PIS(7, 4), D_PIS(7, 5), D_PIS(7, 6), D_PIS(7, 7),
+	D_PIS(8, 0), D_PIS(8, 1), D_PIS(8, 2), D_PIS(8, 3),
+	D_PIS(8, 4), D_PIS(8, 5), D_PIS(8, 6), D_PIS(8, 7)
+};
+
+const size_t sppctl_pmux_list_sz = ARRAY_SIZE(sppctl_pmux_list_s);
+
+static const unsigned int pins_spif1[] = { D(10, 3), D(10, 4), D(10, 6), D(10, 7) };
+static const unsigned int pins_spif2[] = { D(9, 4), D(9, 6), D(9, 7), D(10, 1) };
+static const struct sppctl_grp sp7021grps_spif[] = {
+	EGRP("SPI_FLASH1", 1, pins_spif1),
+	EGRP("SPI_FLASH2", 2, pins_spif2)
+};
+
+static const unsigned int pins_spi41[] = { D(10, 2), D(10, 5) };
+static const unsigned int pins_spi42[] = { D(9, 5), D(9, 8) };
+static const struct sppctl_grp sp7021grps_spi4[] = {
+	EGRP("SPI_FLASH_4BIT1", 1, pins_spi41),
+	EGRP("SPI_FLASH_4BIT2", 2, pins_spi42)
+};
+
+static const unsigned int pins_snan[] = {
+	D(9, 4), D(9, 5), D(9, 6), D(9, 7), D(10, 0), D(10, 1)
+};
+
+static const struct sppctl_grp sp7021grps_snan[] = {
+	EGRP("SPI_NAND", 1, pins_snan)
+};
+
+static const unsigned int pins_emmc[] = {
+	D(9, 0), D(9, 1), D(9, 2), D(9, 3), D(9, 4), D(9, 5),
+	D(9, 6), D(9, 7), D(10, 0), D(10, 1) };
+
+static const struct sppctl_grp sp7021grps_emmc[] = {
+	EGRP("CARD0_EMMC", 1, pins_emmc)
+};
+
+static const unsigned int pins_sdsd[] = {
+	D(8, 1), D(8, 2), D(8, 3), D(8, 4), D(8, 5), D(8, 6)
+};
+
+static const struct sppctl_grp sp7021grps_sdsd[] = {
+	EGRP("SD_CARD", 1, pins_sdsd)
+};
+
+static const unsigned int pins_uar0[] = { D(11, 0), D(11, 1) };
+
+static const struct sppctl_grp sp7021grps_uar0[] = {
+	EGRP("UA0", 1, pins_uar0)
+};
+
+static const unsigned int pins_adbg1[] = { D(10, 2), D(10, 3) };
+static const unsigned int pins_adbg2[] = { D(7, 1), D(7, 2) };
+
+static const struct sppctl_grp sp7021grps_adbg[] = {
+	EGRP("ACHIP_DEBUG1", 1, pins_adbg1),
+	EGRP("ACHIP_DEBUG2", 2, pins_adbg2)
+};
+
+static const unsigned int pins_aua2axi1[] = { D(2, 0), D(2, 1), D(2, 2) };
+static const unsigned int pins_aua2axi2[] = { D(1, 0), D(1, 1), D(1, 2) };
+
+static const struct sppctl_grp sp7021grps_au2x[] = {
+	EGRP("ACHIP_UA2AXI1", 1, pins_aua2axi1),
+	EGRP("ACHIP_UA2AXI2", 2, pins_aua2axi2)
+};
+
+static const unsigned int pins_fpga[] = {
+	D(0, 2), D(0, 3), D(0, 4), D(0, 5), D(0, 6), D(0, 7),
+	D(1, 0), D(1, 1), D(1, 2), D(1, 3), D(1, 4), D(1, 5),
+	D(1, 6), D(1, 7), D(2, 0), D(2, 1), D(2, 2), D(2, 3),
+	D(2, 4), D(2, 5), D(2, 6), D(2, 7), D(3, 0), D(3, 1),
+	D(3, 2), D(3, 3), D(3, 4), D(3, 5), D(3, 6), D(3, 7),
+	D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4), D(4, 5),
+	D(4, 6), D(4, 7), D(5, 0), D(5, 1), D(5, 2)
+};
+
+static const struct sppctl_grp sp7021grps_fpga[] = {
+	EGRP("FPGA_IFX", 1, pins_fpga)
+};
+
+static const unsigned int pins_hdmi1[] = { D(10, 6), D(12, 2), D(12, 1) };
+static const unsigned int pins_hdmi2[] = { D(8, 3), D(8, 5), D(8, 6) };
+static const unsigned int pins_hdmi3[] = { D(7, 4), D(7, 6), D(7, 7) };
+
+static const struct sppctl_grp sp7021grps_hdmi[] = {
+	EGRP("HDMI_TX1", 1, pins_hdmi1),
+	EGRP("HDMI_TX2", 2, pins_hdmi2),
+	EGRP("HDMI_TX3", 3, pins_hdmi3)
+};
+
+static const unsigned int pins_eadc[] = {
+	D(1, 0), D(1, 1), D(1, 2), D(1, 3), D(1, 4), D(1, 5), D(1, 6)
+};
+
+static const struct sppctl_grp sp7021grps_eadc[] = {
+	EGRP("AUD_EXT_ADC_IFX0", 1, pins_eadc)
+};
+
+static const unsigned int pins_edac[] = {
+	D(2, 5), D(2, 6), D(2, 7), D(3, 0), D(3, 1), D(3, 2), D(3, 4)
+};
+
+static const struct sppctl_grp sp7021grps_edac[] = {
+	EGRP("AUD_EXT_DAC_IFX0", 1, pins_edac)
+};
+
+static const unsigned int pins_spdi[] = { D(2, 4) };
+
+static const struct sppctl_grp sp7021grps_spdi[] = {
+	EGRP("AUD_IEC_RX0", 1, pins_spdi)
+};
+
+static const unsigned int pins_spdo[] = { D(3, 6) };
+
+static const struct sppctl_grp sp7021grps_spdo[] = {
+	EGRP("AUD_IEC_TX0", 1, pins_spdo)
+};
+
+static const unsigned int pins_tdmt[] = {
+	D(2, 5), D(2, 6), D(2, 7), D(3, 0), D(3, 1), D(3, 2)
+};
+
+static const struct sppctl_grp sp7021grps_tdmt[] = {
+	EGRP("TDMTX_IFX0", 1, pins_tdmt)
+};
+
+static const unsigned int pins_tdmr[] = { D(1, 7), D(2, 0), D(2, 1), D(2, 2) };
+
+static const struct sppctl_grp sp7021grps_tdmr[] = {
+	EGRP("TDMRX_IFX0", 1, pins_tdmr)
+};
+
+static const unsigned int pins_pdmr[] = {
+	D(1, 7), D(2, 0), D(2, 1), D(2, 2), D(2, 3)
+};
+
+static const struct sppctl_grp sp7021grps_pdmr[] = {
+	EGRP("PDMRX_IFX0", 1, pins_pdmr)
+};
+
+static const unsigned int pins_pcmt[] = {
+	D(3, 7), D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4)
+};
+
+static const struct sppctl_grp sp7021grps_pcmt[] = {
+	EGRP("PCM_IEC_TX", 1, pins_pcmt)
+};
+
+static const unsigned int pins_lcdi[] = {
+	D(1, 4), D(1, 5),
+	D(1, 6), D(1, 7), D(2, 0), D(2, 1), D(2, 2), D(2, 3),
+	D(2, 4), D(2, 5), D(2, 6), D(2, 7), D(3, 0), D(3, 1),
+	D(3, 2), D(3, 3), D(3, 4), D(3, 5), D(3, 6), D(3, 7),
+	D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4), D(4, 5),
+	D(4, 6), D(4, 7)
+};
+
+static const struct sppctl_grp sp7021grps_lcdi[] = {
+	EGRP("LCDIF", 1, pins_lcdi)
+};
+
+static const unsigned int pins_dvdd[] = {
+	D(7, 0), D(7, 1), D(7, 2), D(7, 3), D(7, 4), D(7, 5), D(7, 6), D(7, 7),
+	D(8, 0), D(8, 1), D(8, 2), D(8, 3), D(8, 4), D(8, 5)
+};
+
+static const struct sppctl_grp sp7021grps_dvdd[] = {
+	EGRP("DVD_DSP_DEBUG", 1, pins_dvdd)
+};
+
+static const unsigned int pins_i2cd[] = { D(1, 0), D(1, 1) };
+
+static const struct sppctl_grp sp7021grps_i2cd[] = {
+	EGRP("I2C_DEBUG", 1, pins_i2cd)
+};
+
+static const unsigned int pins_i2cs[] = { D(0, 0), D(0, 1) };
+
+static const struct sppctl_grp sp7021grps_i2cs[] = {
+	EGRP("I2C_SLAVE", 1, pins_i2cs)
+};
+
+static const unsigned int pins_wakp[] = { D(10, 5) };
+
+static const struct sppctl_grp sp7021grps_wakp[] = {
+	EGRP("WAKEUP", 1, pins_wakp)
+};
+
+static const unsigned int pins_u2ax[] = { D(2, 0), D(2, 1), D(3, 0), D(3, 1) };
+
+static const struct sppctl_grp sp7021grps_u2ax[] = {
+	EGRP("UART2AXI", 1, pins_u2ax)
+};
+
+static const unsigned int pins_u0ic[] = {
+	D(0, 0), D(0, 1), D(0, 4), D(0, 5), D(1, 0), D(1, 1)
+};
+
+static const struct sppctl_grp sp7021grps_u0ic[] = {
+	EGRP("USB0_I2C", 1, pins_u0ic)
+};
+
+static const unsigned int pins_u1ic[] = {
+	D(0, 2), D(0, 3), D(0, 6), D(0, 7), D(1, 2), D(1, 3)
+};
+
+static const struct sppctl_grp sp7021grps_u1ic[] = {
+	EGRP("USB1_I2C", 1, pins_u1ic)
+};
+
+static const unsigned int pins_u0ot[] = { D(11, 2) };
+
+static const struct sppctl_grp sp7021grps_u0ot[] = {
+	EGRP("USB0_OTG", 1, pins_u0ot)
+};
+
+static const unsigned int pins_u1ot[] = { D(11, 3) };
+
+static const struct sppctl_grp sp7021grps_u1ot[] = {
+	EGRP("USB1_OTG", 1, pins_u1ot)
+};
+
+static const unsigned int pins_uphd[] = {
+	D(0, 1), D(0, 2), D(0, 3), D(7, 4), D(7, 5), D(7, 6),
+	D(7, 7), D(8, 0), D(8, 1), D(8, 2), D(8, 3),
+	D(9, 7), D(10, 2), D(10, 3), D(10, 4)
+};
+
+static const struct sppctl_grp sp7021grps_up0d[] = {
+	EGRP("UPHY0_DEBUG", 1, pins_uphd)
+};
+
+static const struct sppctl_grp sp7021grps_up1d[] = {
+	EGRP("UPHY1_DEBUG", 1, pins_uphd)
+};
+
+static const unsigned int pins_upex[] = {
+	D(0, 0), D(0, 1), D(0, 2), D(0, 3), D(0, 4), D(0, 5), D(0, 6), D(0, 7),
+	D(1, 0), D(1, 1), D(1, 2), D(1, 3), D(1, 4), D(1, 5), D(1, 6), D(1, 7),
+	D(2, 0), D(2, 1), D(2, 2), D(2, 3), D(2, 4), D(2, 5), D(2, 6), D(2, 7),
+	D(3, 0), D(3, 1), D(3, 2), D(3, 3), D(3, 4), D(3, 5), D(3, 6), D(3, 7),
+	D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4), D(4, 5), D(4, 6), D(4, 7),
+	D(5, 0), D(5, 1), D(5, 2), D(5, 3), D(5, 4), D(5, 5), D(5, 6), D(5, 7),
+	D(6, 0), D(6, 1), D(6, 2), D(6, 3), D(6, 4), D(6, 5), D(6, 6), D(6, 7),
+	D(7, 0), D(7, 1), D(7, 2), D(7, 3), D(7, 4), D(7, 5), D(7, 6), D(7, 7),
+	D(8, 0), D(8, 1), D(8, 2), D(8, 3), D(8, 4), D(8, 5), D(8, 6), D(8, 7),
+	D(9, 0), D(9, 1), D(9, 2), D(9, 3), D(9, 4), D(9, 5), D(9, 6), D(9, 7),
+	D(10, 0), D(10, 1), D(10, 2), D(10, 3), D(10, 4), D(10, 5), D(10, 6), D(10, 7)
+};
+
+static const struct sppctl_grp sp7021grps_upex[] = {
+	EGRP("UPHY0_EXT", 1, pins_upex)
+};
+
+static const unsigned int pins_prp1[] = {
+	D(0, 6), D(0, 7),
+	D(1, 0), D(1, 1), D(1, 2), D(1, 3), D(1, 4), D(1, 5), D(1, 6), D(1, 7),
+	D(2, 1), D(2, 2), D(2, 3), D(2, 4), D(2, 5), D(2, 6), D(2, 7),
+	D(3, 0), D(3, 1), D(3, 2)
+};
+
+static const unsigned int pins_prp2[] = {
+	D(3, 4), D(3, 6), D(3, 7),
+	D(4, 0), D(4, 1), D(4, 2), D(4, 3), D(4, 4), D(4, 5), D(4, 6), D(4, 7),
+	D(5, 0), D(5, 1), D(5, 2), D(5, 3), D(5, 4), D(5, 5), D(5, 6), D(5, 7),
+	D(6, 4)
+};
+
+static const struct sppctl_grp sp7021grps_prbp[] = {
+	EGRP("PROBE_PORT1", 1, pins_prp1),
+	EGRP("PROBE_PORT2", 2, pins_prp2)
+};
+
+const struct sppctl_func sppctl_list_funcs[] = {
+	FNCN("GPIO",            f_off_0, 0x00, 0, 0),
+	FNCN("IOP",             f_off_0, 0x00, 0, 0),
+
+	FNCN("L2SW_CLK_OUT",        f_off_m, 0x00, 0, 7),
+	FNCN("L2SW_MAC_SMI_MDC",    f_off_m, 0x00, 8, 7),
+	FNCN("L2SW_LED_FLASH0",     f_off_m, 0x01, 0, 7),
+	FNCN("L2SW_LED_FLASH1",     f_off_m, 0x01, 8, 7),
+	FNCN("L2SW_LED_ON0",        f_off_m, 0x02, 0, 7),
+	FNCN("L2SW_LED_ON1",        f_off_m, 0x02, 8, 7),
+	FNCN("L2SW_MAC_SMI_MDIO",   f_off_m, 0x03, 0, 7),
+	FNCN("L2SW_P0_MAC_RMII_TXEN",   f_off_m, 0x03, 8, 7),
+	FNCN("L2SW_P0_MAC_RMII_TXD0",   f_off_m, 0x04, 0, 7),
+	FNCN("L2SW_P0_MAC_RMII_TXD1",   f_off_m, 0x04, 8, 7),
+	FNCN("L2SW_P0_MAC_RMII_CRSDV",  f_off_m, 0x05, 0, 7),
+	FNCN("L2SW_P0_MAC_RMII_RXD0",   f_off_m, 0x05, 8, 7),
+	FNCN("L2SW_P0_MAC_RMII_RXD1",   f_off_m, 0x06, 0, 7),
+	FNCN("L2SW_P0_MAC_RMII_RXER",   f_off_m, 0x06, 8, 7),
+	FNCN("L2SW_P1_MAC_RMII_TXEN",   f_off_m, 0x07, 0, 7),
+	FNCN("L2SW_P1_MAC_RMII_TXD0",   f_off_m, 0x07, 8, 7),
+	FNCN("L2SW_P1_MAC_RMII_TXD1",   f_off_m, 0x08, 0, 7),
+	FNCN("L2SW_P1_MAC_RMII_CRSDV",  f_off_m, 0x08, 8, 7),
+	FNCN("L2SW_P1_MAC_RMII_RXD0",   f_off_m, 0x09, 0, 7),
+	FNCN("L2SW_P1_MAC_RMII_RXD1",   f_off_m, 0x09, 8, 7),
+	FNCN("L2SW_P1_MAC_RMII_RXER",   f_off_m, 0x0A, 0, 7),
+	FNCN("DAISY_MODE",      f_off_m, 0x0A, 8, 7),
+	FNCN("SDIO_CLK",        f_off_m, 0x0B, 0, 7),
+	FNCN("SDIO_CMD",        f_off_m, 0x0B, 8, 7),
+	FNCN("SDIO_D0",         f_off_m, 0x0C, 0, 7),
+	FNCN("SDIO_D1",         f_off_m, 0x0C, 8, 7),
+	FNCN("SDIO_D2",         f_off_m, 0x0D, 0, 7),
+	FNCN("SDIO_D3",         f_off_m, 0x0D, 8, 7),
+	FNCN("PWM0",            f_off_m, 0x0E, 0, 7),
+	FNCN("PWM1",            f_off_m, 0x0E, 8, 7),
+	FNCN("PWM2",            f_off_m, 0x0F, 0, 7),
+	FNCN("PWM3",            f_off_m, 0x0F, 8, 7),
+
+	FNCN("PWM4",            f_off_m, 0x10, 0, 7),
+	FNCN("PWM5",            f_off_m, 0x10, 8, 7),
+	FNCN("PWM6",            f_off_m, 0x11, 0, 7),
+	FNCN("PWM7",            f_off_m, 0x11, 8, 7),
+	FNCN("ICM0_D",          f_off_m, 0x12, 0, 7),    /* 4x Input captures */
+	FNCN("ICM1_D",          f_off_m, 0x12, 8, 7),
+	FNCN("ICM2_D",          f_off_m, 0x13, 0, 7),
+	FNCN("ICM3_D",          f_off_m, 0x13, 8, 7),
+	FNCN("ICM0_CLK",        f_off_m, 0x14, 0, 7),
+	FNCN("ICM1_CLK",        f_off_m, 0x14, 8, 7),
+	FNCN("ICM2_CLK",        f_off_m, 0x15, 0, 7),
+	FNCN("ICM3_CLK",        f_off_m, 0x15, 8, 7),
+	FNCN("SPIM0_INT",       f_off_m, 0x16, 0, 7),    /* 4x SPI masters */
+	FNCN("SPIM0_CLK",       f_off_m, 0x16, 8, 7),
+	FNCN("SPIM0_EN",        f_off_m, 0x17, 0, 7),
+	FNCN("SPIM0_DO",        f_off_m, 0x17, 8, 7),
+	FNCN("SPIM0_DI",        f_off_m, 0x18, 0, 7),
+	FNCN("SPIM1_INT",       f_off_m, 0x18, 8, 7),
+	FNCN("SPIM1_CLK",       f_off_m, 0x19, 0, 7),
+	FNCN("SPIM1_EN",        f_off_m, 0x19, 8, 7),
+	FNCN("SPIM1_DO",        f_off_m, 0x1A, 0, 7),
+	FNCN("SPIM1_DI",        f_off_m, 0x1A, 8, 7),
+	FNCN("SPIM2_INT",       f_off_m, 0x1B, 0, 7),
+	FNCN("SPIM2_CLK",       f_off_m, 0x1B, 8, 7),
+	FNCN("SPIM2_EN",        f_off_m, 0x1C, 0, 7),
+	FNCN("SPIM2_DO",        f_off_m, 0x1C, 8, 7),
+	FNCN("SPIM2_DI",        f_off_m, 0x1D, 0, 7),
+	FNCN("SPIM3_INT",       f_off_m, 0x1D, 8, 7),
+	FNCN("SPIM3_CLK",       f_off_m, 0x1E, 0, 7),
+	FNCN("SPIM3_EN",        f_off_m, 0x1E, 8, 7),
+	FNCN("SPIM3_DO",        f_off_m, 0x1F, 0, 7),
+	FNCN("SPIM3_DI",        f_off_m, 0x1F, 8, 7),
+
+	FNCN("SPI0S_INT",       f_off_m, 0x20, 0, 7),    /* 4x SPI slaves */
+	FNCN("SPI0S_CLK",       f_off_m, 0x20, 8, 7),
+	FNCN("SPI0S_EN",        f_off_m, 0x21, 0, 7),
+	FNCN("SPI0S_DO",        f_off_m, 0x21, 8, 7),
+	FNCN("SPI0S_DI",        f_off_m, 0x22, 0, 7),
+	FNCN("SPI1S_INT",       f_off_m, 0x22, 8, 7),
+	FNCN("SPI1S_CLK",       f_off_m, 0x23, 0, 7),
+	FNCN("SPI1S_EN",        f_off_m, 0x23, 8, 7),
+	FNCN("SPI1S_DO",        f_off_m, 0x24, 0, 7),
+	FNCN("SPI1S_DI",        f_off_m, 0x24, 8, 7),
+	FNCN("SPI2S_INT",       f_off_m, 0x25, 0, 7),
+	FNCN("SPI2S_CLK",       f_off_m, 0x25, 8, 7),
+	FNCN("SPI2S_EN",        f_off_m, 0x26, 0, 7),
+	FNCN("SPI2S_DO",        f_off_m, 0x26, 8, 7),
+	FNCN("SPI2S_DI",        f_off_m, 0x27, 0, 7),
+	FNCN("SPI3S_INT",       f_off_m, 0x27, 8, 7),
+	FNCN("SPI3S_CLK",       f_off_m, 0x28, 0, 7),
+	FNCN("SPI3S_EN",        f_off_m, 0x28, 8, 7),
+	FNCN("SPI3S_DO",        f_off_m, 0x29, 0, 7),
+	FNCN("SPI3S_DI",        f_off_m, 0x29, 8, 7),
+	FNCN("I2CM0_CLK",       f_off_m, 0x2A, 0, 7),    /* 4x I2C masters */
+	FNCN("I2CM0_DAT",       f_off_m, 0x2A, 8, 7),
+	FNCN("I2CM1_CLK",       f_off_m, 0x2B, 0, 7),
+	FNCN("I2CM1_DAT",       f_off_m, 0x2B, 8, 7),
+	FNCN("I2CM2_CLK",       f_off_m, 0x2C, 0, 7),
+	FNCN("I2CM2_DAT",       f_off_m, 0x2C, 8, 7),
+	FNCN("I2CM3_CLK",       f_off_m, 0x2D, 0, 7),
+	FNCN("I2CM3_DAT",       f_off_m, 0x2D, 8, 7),
+	FNCN("UA1_TX",          f_off_m, 0x2E, 0, 7),    /* 4x UARTS */
+	FNCN("UA1_RX",          f_off_m, 0x2E, 8, 7),
+	FNCN("UA1_CTS",         f_off_m, 0x2F, 0, 7),
+	FNCN("UA1_RTS",         f_off_m, 0x2F, 8, 7),
+
+	FNCN("UA2_TX",          f_off_m, 0x30, 0, 7),
+	FNCN("UA2_RX",          f_off_m, 0x30, 8, 7),
+	FNCN("UA2_CTS",         f_off_m, 0x31, 0, 7),
+	FNCN("UA2_RTS",         f_off_m, 0x31, 8, 7),
+	FNCN("UA3_TX",          f_off_m, 0x32, 0, 7),
+	FNCN("UA3_RX",          f_off_m, 0x32, 8, 7),
+	FNCN("UA3_CTS",         f_off_m, 0x33, 0, 7),
+	FNCN("UA3_RTS",         f_off_m, 0x33, 8, 7),
+	FNCN("UA4_TX",          f_off_m, 0x34, 0, 7),
+	FNCN("UA4_RX",          f_off_m, 0x34, 8, 7),
+	FNCN("UA4_CTS",         f_off_m, 0x35, 0, 7),
+	FNCN("UA4_RTS",         f_off_m, 0x35, 8, 7),
+	FNCN("TIMER0_INT",      f_off_m, 0x36, 0, 7),    /* 4x timer int. */
+	FNCN("TIMER1_INT",      f_off_m, 0x36, 8, 7),
+	FNCN("TIMER2_INT",      f_off_m, 0x37, 0, 7),
+	FNCN("TIMER3_INT",      f_off_m, 0x37, 8, 7),
+	FNCN("GPIO_INT0",       f_off_m, 0x38, 0, 7),    /* 8x GPIO int. */
+	FNCN("GPIO_INT1",       f_off_m, 0x38, 8, 7),
+	FNCN("GPIO_INT2",       f_off_m, 0x39, 0, 7),
+	FNCN("GPIO_INT3",       f_off_m, 0x39, 8, 7),
+	FNCN("GPIO_INT4",       f_off_m, 0x3A, 0, 7),
+	FNCN("GPIO_INT5",       f_off_m, 0x3A, 8, 7),
+	FNCN("GPIO_INT6",       f_off_m, 0x3B, 0, 7),
+	FNCN("GPIO_INT7",       f_off_m, 0x3B, 8, 7),
+
+	/* MOON1 register */
+	FNCE("SPI_FLASH",       f_off_g, 0x01,  0, 2, sp7021grps_spif),
+	FNCE("SPI_FLASH_4BIT",  f_off_g, 0x01,  2, 2, sp7021grps_spi4),
+	FNCE("SPI_NAND",        f_off_g, 0x01,  4, 1, sp7021grps_snan),
+	FNCE("CARD0_EMMC",      f_off_g, 0x01,  5, 1, sp7021grps_emmc),
+	FNCE("SD_CARD",         f_off_g, 0x01,  6, 1, sp7021grps_sdsd),
+	FNCE("UA0",             f_off_g, 0x01,  7, 1, sp7021grps_uar0),
+	FNCE("ACHIP_DEBUG",     f_off_g, 0x01,  8, 2, sp7021grps_adbg),
+	FNCE("ACHIP_UA2AXI",    f_off_g, 0x01, 10, 2, sp7021grps_au2x),
+	FNCE("FPGA_IFX",        f_off_g, 0x01, 12, 1, sp7021grps_fpga),
+	FNCE("HDMI_TX",         f_off_g, 0x01, 13, 2, sp7021grps_hdmi),
+
+	FNCE("AUD_EXT_ADC_IFX0", f_off_g, 0x01, 15, 1, sp7021grps_eadc),
+	FNCE("AUD_EXT_DAC_IFX0", f_off_g, 0x02,  0, 1, sp7021grps_edac),
+	FNCE("SPDIF_RX",        f_off_g, 0x02,  2, 1, sp7021grps_spdi),
+	FNCE("SPDIF_TX",        f_off_g, 0x02,  3, 1, sp7021grps_spdo),
+	FNCE("TDMTX_IFX0",      f_off_g, 0x02,  4, 1, sp7021grps_tdmt),
+	FNCE("TDMRX_IFX0",      f_off_g, 0x02,  5, 1, sp7021grps_tdmr),
+	FNCE("PDMRX_IFX0",      f_off_g, 0x02,  6, 1, sp7021grps_pdmr),
+	FNCE("PCM_IEC_TX",      f_off_g, 0x02,  7, 1, sp7021grps_pcmt),
+	FNCE("LCDIF",           f_off_g, 0x04,  6, 1, sp7021grps_lcdi),
+	FNCE("DVD_DSP_DEBUG",   f_off_g, 0x02,  8, 1, sp7021grps_dvdd),
+	FNCE("I2C_DEBUG",       f_off_g, 0x02,  9, 1, sp7021grps_i2cd),
+	FNCE("I2C_SLAVE",       f_off_g, 0x02, 10, 1, sp7021grps_i2cs),
+	FNCE("WAKEUP",          f_off_g, 0x02, 11, 1, sp7021grps_wakp),
+	FNCE("UART2AXI",        f_off_g, 0x02, 12, 2, sp7021grps_u2ax),
+	FNCE("USB0_I2C",        f_off_g, 0x02, 14, 2, sp7021grps_u0ic),
+	FNCE("USB1_I2C",        f_off_g, 0x03,  0, 2, sp7021grps_u1ic),
+	FNCE("USB0_OTG",        f_off_g, 0x03,  2, 1, sp7021grps_u0ot),
+	FNCE("USB1_OTG",        f_off_g, 0x03,  3, 1, sp7021grps_u1ot),
+	FNCE("UPHY0_DEBUG",     f_off_g, 0x03,  4, 1, sp7021grps_up0d),
+	FNCE("UPHY1_DEBUG",     f_off_g, 0x03,  5, 1, sp7021grps_up1d),
+	FNCE("UPHY0_EXT",       f_off_g, 0x03,  6, 1, sp7021grps_upex),
+	FNCE("PROBE_PORT",      f_off_g, 0x03,  7, 2, sp7021grps_prbp),
+};
+
+const size_t sppctl_list_funcs_sz = ARRAY_SIZE(sppctl_list_funcs);
-- 
2.7.4


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

* Re: [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021
  2021-12-07  4:17 ` [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021 Wells Lu
@ 2021-12-09 23:30   ` Linus Walleij
  2021-12-10 14:07     ` 呂芳騰
       [not found]     ` <CAFnkrskBVLk0q77Xkk=E07gxmWSNfRtwznz9yv1fd07u0P2HgQ@mail.gmail.com>
  0 siblings, 2 replies; 9+ messages in thread
From: Linus Walleij @ 2021-12-09 23:30 UTC (permalink / raw)
  To: Wells Lu
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	wells.lu, dvorkin

On Tue, Dec 7, 2021 at 5:17 AM Wells Lu <wellslutw@gmail.com> wrote:

> Add dt-bindings header files and documentation for Sunplus SP7021 SoC.
>
> Signed-off-by: Wells Lu <wellslutw@gmail.com>

> +patternProperties:
> +  '-pins$':
> +    if:
> +      type: object
> +    then:
> +      description: |
> +        A pinctrl node should contain at least one subnodes representing the
> +        pins or function-pins group available on the machine. Each subnode
> +        will list the pins it needs, and how they should be configured.
> +
> +        Pinctrl node's client devices use subnodes for desired pin
> +        configuration. Client device subnodes use below standard properties.

I don't understand this if type object stuff here, Rob, help...

> +      properties:
> +        pins:
> +          description: |
> +            Define pins which are used by pinctrl node's client device.
(...)
> +          $ref: /schemas/types.yaml#/definitions/uint32-array

Why can this not $ref the standard binings in
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml

See for example
Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml
for a nice example of how to use this.

Yours,
Linus Walleij

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

* Re: [PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
  2021-12-07  4:17 ` [PATCH v3 2/2] pinctrl: Add driver " Wells Lu
@ 2021-12-10  1:24   ` Linus Walleij
  2021-12-10 18:33     ` 呂芳騰
       [not found]     ` <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
  0 siblings, 2 replies; 9+ messages in thread
From: Linus Walleij @ 2021-12-10  1:24 UTC (permalink / raw)
  To: Wells Lu
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	wells.lu, dvorkin

Hi Wells,

this is improving! Keep working on this driver. I now naturally have
more comments :)

On Tue, Dec 7, 2021 at 5:17 AM Wells Lu <wellslutw@gmail.com> wrote:

> +static void sppctl_func_set(struct sppctl_pdata *pctl, u8 func, u8 val)
> +{
> +       u32 reg, offset;
> +
> +       /* Note that upper 16-bit word is mask
> +        * and lower 16-bit word is value.
> +        * Enable mask before write.
> +        */
> +       reg = 0x007f0000 | val; /* Set value and enable mask. */

Define these types of masks and use them like this:

#include <linux/bits.h>

#define SPPCTL_FUNC_MASK GENMASK(22, 16)

Also switch the order with the mask to the right please:

reg = val & SPPCTL_FUNC_MASK;

> +       if (func & 1)

I would write

#define SSPCTL_FUNC_FLAG BIT(0)

if (func & SSPCTL_FUNC_FLAG)

Use the name that bit has in your documentation for the
define so we know what is going on.

> +               reg <<= 8;

Likewise

#define SSPCTL_FUNC_UPPER_SHIFT 8

reg <<= SSPCTL_FUNC_UPPER_SHIFT;


Can also be a comment. The general idea is to break out as many
of these magic numbers as possible to #defines and give them
some names from the reference manual, so we understand them
instead of the numbers being magic.

> +       /* Convert function # to register offset. */
> +       offset = func & ~1;

Step 1 write:
offset = func & GENMASK(31, 1);

> +       offset <<= 1;

I would write:
offset *= 2;
because we are dealing with an offset and not an arithmetic
operation. It will be the same to the compiler.

But the best is to just merge all this and write (if I'm not wrong):

#include <linux/bitfield.h>

/*
 * Bit 1 .. 31 gives the function, index this into a 32-bit offset by
* multiplying by four to find the register.
 */
offset = FIELD_GET(GENMASK(31, 1), func);
offset *= 4;

This gets pretty clear. We see that we remove BIT(0) and use the
rest as offset index and there are four bytes per register.

(Beware of bugs in my pseudocode, check it!)

> +static u8 sppctl_func_get(struct sppctl_pdata *pctl, u8 func)
> +{
> +       u32 reg, offset;
> +       u8 val;
> +
> +       /* Convert function # to register offset. */
> +       offset = func & ~1;
> +       offset <<= 1;

Same comments.

> +       reg = readl(pctl->moon2_base + offset);
> +       if (func & 1)
> +               val = reg >> 8;
> +       else
> +               val = reg;
> +       val &= 0x7f;

#define SSPCTL_*_MASK for this 0x7f so we understand it.

> +static void sppctl_gmx_set(struct sppctl_pdata *pctl, u8 reg_off, u8 bit_off, u8 bit_sz,
> +                          u8 val)
> +{
> +       u32 mask, reg;
> +
> +       /* Note that upper 16-bit word is mask
> +        * and lower 16-bit word is value.
> +        * Enable mask before write.
> +        */
> +       mask = ~(~0 << bit_sz);
> +       reg = (mask << 16) | (val & mask);
> +       reg <<= bit_off;

Please familiarize yourself with <linux/bitfield.h> and use things like
FIELD_PREP() for this (I think, atleast).

> +static int sppctl_first_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> +       u32 reg;
> +
> +       reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));

So R32_ROF() is register offset.

> +
> +       dev_dbg(chip->parent, "%s(%u): addr = %p, reg = %08x, val = %d\n",
> +               __func__, offset, spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST +
> +               R32_ROF(offset), reg, (int)R32_VAL(reg, R32_BOF(offset)));
> +
> +       return R32_VAL(reg, R32_BOF(offset));

And R32_BOF is register bit offset.

I think these macros just make it hard to read because the reader has to
go to another file and look it up and then figure out what does ROF and
BOF actually mean (no explanation given).

I would just inline the stuff.

u32 reg = (offset / 32) * 4;
u32 bit = offset % 32;

reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + reg);

// Some debug code

return !!(reg & BIT(bit));

> +static void sppctl_gpio_output_inv_set(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> +       u32 reg;
> +
> +       /* Upper 16-bit word is mask. Lower 16-bit word is value. */
> +       reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
> +       writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OINV + R16_ROF(offset));
> +}

Same comments about the BOF and ROF.

This layout with "mask and value" in registers needs to be explained
somewhere it looks complex. I don't understand why a machine register
contains a mask for example.

> +static int stpctl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> +                         unsigned int group_selector)
> +{
> +       const struct sppctl_func *f = &sppctl_list_funcs[func_selector];
> +       struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       struct grp2fp_map g2fpm = pctl->g2fp_maps[group_selector];
> +       int i = -1, j = -1;

Please do not initialize loop variable i to -1, just declare it.

> +       dev_dbg(pctldev->dev, "%s(func: %d, grp: %d)\n", __func__,
> +               func_selector, group_selector);
> +
> +       switch (f->freg) {
> +       case f_off_0:   /* GPIO. detouch from all funcs - ? */
> +               for (i = 0; i < sppctl_list_funcs_sz; i++) {
> +                       if (sppctl_list_funcs[i].freg != f_off_m)
> +                               continue;
> +                       j++;

Insert a comment that j is set to -1 so this will be zero here after the first
iteration.

> +                       if (sppctl_func_get(pctl, j) != group_selector)
> +                               continue;
> +                       sppctl_func_set(pctl, j, 0);
> +               }
> +               break;
> +
> +       case f_off_m:   /* Mux */
> +               sppctl_first_master_set(&pctl->spp_gchip->chip, group_selector,
> +                                       mux_f_mux, mux_m_keep);
> +               sppctl_func_set(pctl, func_selector - 2, (group_selector == 0) ?
> +                               group_selector : group_selector - 7);

-2 and -7? Why? Add some comments or maybe #define these
constants?

> +static int stpctl_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                     struct pinctrl_gpio_range *range, unsigned int offset)
> +{
> +       struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       struct pin_desc *pdesc;
> +       int g_f, g_m;
> +
> +       dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
> +
> +       g_f = sppctl_first_get(&pctl->spp_gchip->chip, offset);
> +       g_m = sppctl_master_get(&pctl->spp_gchip->chip, offset);
> +       if (g_f == mux_f_gpio && g_m == mux_m_gpio)
> +               return 0;
> +
> +       pdesc = pin_desc_get(pctldev, offset);
> +       if (pdesc->mux_owner)
> +               return -EACCES;

Do not reimplement the pinmux core please.

What you want to achieve here is "strict pinmux", i.e. setting the field
"strict" in struct pinmux_ops to true. Then you can just delete this
check.

> +static const struct pinmux_ops sppctl_pinmux_ops = {
> +       .request             = stpctl_request,
> +       .free                = stpctl_free,

These are just set to empty functions. Delete these entries
and the empty functions.

> +       .get_functions_count = stpctl_get_functions_count,
> +       .get_function_name   = stpctl_get_function_name,
> +       .get_function_groups = stpctl_get_function_groups,
> +       .set_mux             = stpctl_set_mux,
> +       .gpio_request_enable = stpctl_gpio_request_enable,
> +       .gpio_disable_free   = stpctl_gpio_disable_free,
> +       .gpio_set_direction  = stpctl_gpio_set_direction,
> +       .strict              = 1

Use "true" rather than 1. (And do not reimplement the check.)

> +static int sppctl_remove(struct platform_device *pdev)
> +{
> +       struct sppctl_pdata *sppctl = pdev->dev.platform_data;
> +
> +       devm_pinctrl_unregister(&pdev->dev, sppctl->pctl_dev);

This defies the idea with devm_* calls. Drop remove() entirely because
devm_ allocated resources go away by themselves.

> +++ b/drivers/pinctrl/sunplus/sppctl.h
(...)
> +/* (/16)*4 */
> +#define R16_ROF(r)             (((r) >> 4) << 2)
> +#define R16_BOF(r)             ((r) % 16)
> +/* (/32)*4 */
> +#define R32_ROF(r)             (((r) >> 5) << 2)
> +#define R32_BOF(r)             ((r) % 32)

As mentioned I prefer explicit inlined code for these.
The bit shifting here makes it really hard to know what is going
on, the compiler will get it right if you use the right types
and just write (n / 32) * 4. Please do not try to help the compiler
optimizing it just leads to code that is hard to read.

> +#define R32_VAL(r, boff)       (((r) >> (boff)) & BIT(0))

To check the value of a certain bit use this pattern:

if (val & BIT(n))

To return a boolean clamped bit (return 0/1) do this idiom:

return !!(val & BIT(n));

Other than these things I didn't notice anything more this
time, but I might find even more stuff, but hey it's getting there!

Yours,
Linus Walleij

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

* Re: [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021
  2021-12-09 23:30   ` Linus Walleij
@ 2021-12-10 14:07     ` 呂芳騰
       [not found]     ` <CAFnkrskBVLk0q77Xkk=E07gxmWSNfRtwznz9yv1fd07u0P2HgQ@mail.gmail.com>
  1 sibling, 0 replies; 9+ messages in thread
From: 呂芳騰 @ 2021-12-10 14:07 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	Wells Lu, Dvorkin Dmitry

Re-send this email because it was rejected due to the wrong email format!

Hi Linus,

Thank you very much for your review!

Please see my answers below:

> > Add dt-bindings header files and documentation for Sunplus SP7021 SoC.
> >
> > Signed-off-by: Wells Lu <wellslutw@gmail.com>
>
> > +patternProperties:
> > +  '-pins$':
> > +    if:
> > +      type: object
> > +    then:
> > +      description: |
> > +        A pinctrl node should contain at least one subnodes representing the
> > +        pins or function-pins group available on the machine. Each subnode
> > +        will list the pins it needs, and how they should be configured.
> > +
> > +        Pinctrl node's client devices use subnodes for desired pin
> > +        configuration. Client device subnodes use below standard properties.
>
> I don't understand this if type object stuff here, Rob, help...

I'll remove "if type object" stuff next patch since
pinctrl node has no properties with "-pins" suffix.
except sub nodes.


> > +      properties:
> > +        pins:
> > +          description: |
> > +            Define pins which are used by pinctrl node's client device.
> (...)
> > +          $ref: /schemas/types.yaml#/definitions/uint32-array
>
> Why can this not $ref the standard binings in
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
>
> See for example
> Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml
> for a nice example of how to use this.
> Yours,
> Linus Walleij
The pins' types and control of SP7021 are a bit complex.
Let me explain:

SP7021 has 99 digital GPIO pins which are numbered from
GPIO 0 to 98. All are multiplexed with some special function
pins. There are 3 types special function pins:

(1) function-pins:
    For example, if control-field SPI_FLASH_SEL is set to 1,
    GPIO 83, 84, 86 and 87 will be pins of SPI-NOR flash.
    If it is set to 2, GPIO 76, 78, 79 and 81 will be pins of
    SPI-NOR flash.

    For example 2, if control-bit UA0_SEL is set to 1,
    GPIO 88 and 89 will be TX and RX pins of UART_0
    (UART channel 0).

    For example 3, if control-bit EMMC_SEL is set to 1,
    GPIO 72, 73, 74, 75, 76, 77, 78, 79, 80, 81 will be
    pins of an eMMC device.

    The driver defines properties "function" and "groups"
    to select this kind of pins-function.

(2) fully pin-mux pins:
    GPIO 8 to 71 are 'fully pin-mux' pins.
    Pins of peripherals of SP7021 (ex: UART_1, UART_2,
    UART_3, UART_4, I2C_0, I2C_1, ..., SPI_0, SPI_1, ...
    GPIO_INT0, GPIO_INT1, .., RMII_of_Ethernet, and etc.)
    can be set to any pins of fully pin-mux pins.

    EX1 (UART channel 1):
    If control-field UA1_TX_SEL is set to 3, TX pin of
    UART_1 will be routed to GPIO 10 (3 - 1 + 8 = 10)
    If control-field UA1_RX_SEL is set to 4, RX pin of
    UART_1 will be routed to GPIO 11 (4 - 1 + 8 = 11)
    If control-field UA1_RTS_SEL is set to 5, RTS pin of
    UART_1 will be routed to GPIO 12 (5 - 1 + 8 = 12)
    If control-field UA1_CTS_SEL is set to 6, CTS pin of
    UART_1 will be routed to GPIO 13 (6 - 1 + 8 = 13)

    EX2 (I2C channel 0):
    If control-field I2C0_CLK_SEL is set to 20, CLK pin
    of I2C_0 will be routed to GPIO 27 (20 - 1 + 8 = 27)
    If control-field I2C0_DATA_SEL is set to 21, DATA pin
    of I2C_0 will be routed to GPIO 28 (21 - 1 + 9 = 28)

    Totally, SP7021 has 120 peripheral pins. The
    peripheral pins can be routed to any of 64  'fully
     pin-mux' pins. So total combinations are:
           120 x 64 = 7680
     This is why we cannot enumerate all combinations.

(3) I/O processor pins
    SP7021 has a built-in I/O processor.
    Any GPIO pins (GPIO 0 to 98) can be set to pins of
    I/O processor.

Property 'pins' is defined as uint32 array. Each item
defines a pin as below:
Bit 32~24 defines GPIO number. Its range is 0 ~ 98.
Bit 23~16 defines types: digital GPIO pins (3), IO processor pins (2)
or fully pin-mux pins (1)
Bit 15~8 defines pins of peripherals which are defined in
'include/dt-binging/pinctrl/sppctl.h'.
Bit 7~0 defines types or initial-state of digital GPIO pins.


Best regards,
Wells

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

* Re: [PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
  2021-12-10  1:24   ` Linus Walleij
@ 2021-12-10 18:33     ` 呂芳騰
       [not found]     ` <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
  1 sibling, 0 replies; 9+ messages in thread
From: 呂芳騰 @ 2021-12-10 18:33 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	Wells Lu, Dvorkin Dmitry

Resend the email because it was rejected due to wrong format!

Hi Linus,

Thank you very much for your review.

Please see my answers below:

> Hi Wells,
>
> this is improving! Keep working on this driver. I now naturally have
> more comments :)
>
> On Tue, Dec 7, 2021 at 5:17 AM Wells Lu <wellslutw@gmail.com> wrote:
>
> > +static void sppctl_func_set(struct sppctl_pdata *pctl, u8 func, u8 val)
> > +{
> > +       u32 reg, offset;
> > +
> > +       /* Note that upper 16-bit word is mask
> > +        * and lower 16-bit word is value.
> > +        * Enable mask before write.
> > +        */
> > +       reg = 0x007f0000 | val; /* Set value and enable mask. */
>
> Define these types of masks and use them like this:
>
> #include <linux/bits.h>
>
> #define SPPCTL_FUNC_MASK GENMASK(22, 16)
>
> Also switch the order with the mask to the right please:
>
> reg = val & SPPCTL_FUNC_MASK;

Yes, I'll modify code next patch.


> > +       if (func & 1)
>
> I would write
>
> #define SSPCTL_FUNC_FLAG BIT(0)
>
> if (func & SSPCTL_FUNC_FLAG)
>
> Use the name that bit has in your documentation for the
> define so we know what is going on.

Actually, 'if (func & 1)' is not used to test bit 0,
but test 'func' is an odd number or not.
If 'func' is even number, control-field is at bit 6 ~ 0.
Its corresponding mask-field is at bit 22 ~ 16.
If 'func' is odd number, control-field is at bit 14 ~ 8.
Its corresponding mask-field is at bit 30 ~ 24.

Control and mask fields of 'func' are arranged as shown
below:

func # | register control-field  mask-field
-------+------------------------------------
   0   | reg[0]     ( 6:0)        (22 :  6)
   1   | reg[0]     (14:8)        (30 : 24)
   2   | reg[1]     ( 6:0)        (22 :  6)
   3   | reg[1]     (14:8)        (30 : 24)

> > +               reg <<= 8;
>
> Likewise
>
> #define SSPCTL_FUNC_UPPER_SHIFT 8
>
> reg <<= SSPCTL_FUNC_UPPER_SHIFT;
> Can also be a comment. The general idea is to break out as many
> of these magic numbers as possible to #defines and give them
> some names from the reference manual, so we understand them
> instead of the numbers being magic.

Yes, I'll modify codes, add defines and more comments next patch.


> > +       /* Convert function # to register offset. */
> > +       offset = func & ~1;
>
> Step 1 write:
> offset = func & GENMASK(31, 1);
>
> > +       offset <<= 1;
>
> I would write:
> offset *= 2;
> because we are dealing with an offset and not an arithmetic
> operation. It will be the same to the compiler.
>
> But the best is to just merge all this and write (if I'm not wrong):
>
> #include <linux/bitfield.h>
>
> /*
>  * Bit 1 .. 31 gives the function, index this into a 32-bit offset by
> * multiplying by four to find the register.
>  */
> offset = FIELD_GET(GENMASK(31, 1), func);
> offset *= 4;
>
> This gets pretty clear. We see that we remove BIT(0) and use the
> rest as offset index and there are four bytes per register.
>
> (Beware of bugs in my pseudo code, check it!)

Thank you for example code!
Yes, I will use BIT, GENMASK, FIELD_GET, and FIELD_PREP
macros next patch.


> > +static u8 sppctl_func_get(struct sppctl_pdata *pctl, u8 func)
> > +{
> > +       u32 reg, offset;
> > +       u8 val;
> > +
> > +       /* Convert function # to register offset. */
> > +       offset = func & ~1;
> > +       offset <<= 1;
>
> Same comments.

Yes, I got it. I'll modify codes next patch.


> > +       reg = readl(pctl->moon2_base + offset);
> > +       if (func & 1)
> > +               val = reg >> 8;
> > +       else
> > +               val = reg;
> > +       val &= 0x7f;
>
> #define SSPCTL_*_MASK for this 0x7f so we understand it.

Yes, I'll do it next patch.


> > +static void sppctl_gmx_set(struct sppctl_pdata *pctl, u8 reg_off, u8 bit_off, u8 bit_sz,
> > +                          u8 val)
> > +{
> > +       u32 mask, reg;
> > +
> > +       /* Note that upper 16-bit word is mask
> > +        * and lower 16-bit word is value.
> > +        * Enable mask before write.
> > +        */
> > +       mask = ~(~0 << bit_sz);
> > +       reg = (mask << 16) | (val & mask);
> > +       reg <<= bit_off;
>
> Please familiarize yourself with <linux/bitfield.h> and use things like
> FIELD_PREP() for this (I think, at least).

Yes, I will use BIT, GENMASK, FIELD_GET, and FIELD_PREP
macros next patch.

> > +static int sppctl_first_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> > +       u32 reg;
> > +
> > +       reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));
>
> So R32_ROF() is register offset.

Yes, it converts 'offset' to register offset (w.r.t. base register)

R32_ROF() is for 32-bit width registers
R16_ROF() is for 16-bit width registers (higher 16-bit of the register is mask).


> > +
> > +       dev_dbg(chip->parent, "%s(%u): addr = %p, reg = %08x, val = %d\n",
> > +               __func__, offset, spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST +
> > +               R32_ROF(offset), reg, (int)R32_VAL(reg, R32_BOF(offset)));
> > +
> > +       return R32_VAL(reg, R32_BOF(offset));
>
> And R32_BOF is register bit offset.
>
> I think these macros just make it hard to read because the reader has to
> go to another file and look it up and then figure out what does ROF and
> BOF actually mean (no explanation given).
>
> I would just inline the stuff.
>
> u32 reg = (offset / 32) * 4;
> u32 bit = offset % 32;
>
> reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + reg);
>
> // Some debug code
>
> return !!(reg & BIT(bit));

Yes, I'll modify codes to follow your suggestions next patch.


> > +static void sppctl_gpio_output_inv_set(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> > +       u32 reg;
> > +
> > +       /* Upper 16-bit word is mask. Lower 16-bit word is value. */
> > +       reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
> > +       writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OINV + R16_ROF(offset));
> > +}
>
> Same comments about the BOF and ROF.
>
> This layout with "mask and value" in registers needs to be explained
> somewhere it looks complex. I don't understand why a machine register
> contains a mask for example.

This is a hardware mechanism for protecting some important registers from
being overwritten accidentally. The corresponding mask bit should be set
first and then the control-bits or fields can be written. The design is
originally requested from car makers.


> > +static int stpctl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> > +                         unsigned int group_selector)
> > +{
> > +       const struct sppctl_func *f = &sppctl_list_funcs[func_selector];
> > +       struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> > +       struct grp2fp_map g2fpm = pctl->g2fp_maps[group_selector];
> > +       int i = -1, j = -1;
>
> Please do not initialize loop variable i to -1, just declare it.

Yes, I'll remove the initialization for loop variables next patch


> > +       dev_dbg(pctldev->dev, "%s(func: %d, grp: %d)\n", __func__,
> > +               func_selector, group_selector);
> > +
> > +       switch (f->freg) {
> > +       case f_off_0:   /* GPIO. detouch from all funcs - ? */
> > +               for (i = 0; i < sppctl_list_funcs_sz; i++) {
> > +                       if (sppctl_list_funcs[i].freg != f_off_m)
> > +                               continue;
> > +                       j++;
>
> Insert a comment that j is set to -1 so this will be zero here after the first
> iteration.

Yes, I'll add comments next patch.


> > +                       if (sppctl_func_get(pctl, j) != group_selector)
> > +                               continue;
> > +                       sppctl_func_set(pctl, j, 0);
> > +               }
> > +               break;
> > +
> > +       case f_off_m:   /* Mux */
> > +               sppctl_first_master_set(&pctl->spp_gchip->chip, group_selector,
> > +                                       mux_f_mux, mux_m_keep);
> > +               sppctl_func_set(pctl, func_selector - 2, (group_selector == 0) ?
> > +                               group_selector : group_selector - 7);
>
> -2 and -7? Why? Add some comments or maybe #define these
> constants?

Yes, I'll defines and add some comments for the magic numbers
next patch.


> > +static int stpctl_gpio_request_enable(struct pinctrl_dev *pctldev,
> > +                                     struct pinctrl_gpio_range *range, unsigned int offset)
> > +{
> > +       struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> > +       struct pin_desc *pdesc;
> > +       int g_f, g_m;
> > +
> > +       dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
> > +
> > +       g_f = sppctl_first_get(&pctl->spp_gchip->chip, offset);
> > +       g_m = sppctl_master_get(&pctl->spp_gchip->chip, offset);
> > +       if (g_f == mux_f_gpio && g_m == mux_m_gpio)
> > +               return 0;
> > +
> > +       pdesc = pin_desc_get(pctldev, offset);
> > +       if (pdesc->mux_owner)
> > +               return -EACCES;
>
> Do not reimplement the pinmux core please.
>
> What you want to achieve here is "strict pinmux", i.e. setting the field
> "strict" in struct pinmux_ops to true. Then you can just delete this
> check.

Yes, I'll remove the if-statement next patch.


> > +static const struct pinmux_ops sppctl_pinmux_ops = {
> > +       .request             = stpctl_request,
> > +       .free                = stpctl_free,
>
> These are just set to empty functions. Delete these entries
> and the empty functions.

Yes, I'll remove all 'empty' functions next patch.


> > +       .get_functions_count = stpctl_get_functions_count,
> > +       .get_function_name   = stpctl_get_function_name,
> > +       .get_function_groups = stpctl_get_function_groups,
> > +       .set_mux             = stpctl_set_mux,
> > +       .gpio_request_enable = stpctl_gpio_request_enable,
> > +       .gpio_disable_free   = stpctl_gpio_disable_free,
> > +       .gpio_set_direction  = stpctl_gpio_set_direction,
> > +       .strict              = 1
>
> Use "true" rather than 1. (And do not reimplement the check.)

Yes, I'll do it next patch.


> > +static int sppctl_remove(struct platform_device *pdev)
> > +{
> > +       struct sppctl_pdata *sppctl = pdev->dev.platform_data;
> > +
> > +       devm_pinctrl_unregister(&pdev->dev, sppctl->pctl_dev);
>
> This defies the idea with devm_* calls. Drop remove() entirely because
> devm_ allocated resources go away by themselves.

Yes, I'll remove devm_pinctrl_unregister() next patch.
Sorry for buggy code. Fortunately, pinctrl driver
will never be removed.


> > +++ b/drivers/pinctrl/sunplus/sppctl.h
> (...)
> > +/* (/16)*4 */
> > +#define R16_ROF(r)             (((r) >> 4) << 2)
> > +#define R16_BOF(r)             ((r) % 16)
> > +/* (/32)*4 */
> > +#define R32_ROF(r)             (((r) >> 5) << 2)
> > +#define R32_BOF(r)             ((r) % 32)
>
> As mentioned I prefer explicit inlined code for these.
> The bit shifting here makes it really hard to know what is going
> on, the compiler will get it right if you use the right types
> and just write (n / 32) * 4. Please do not try to help the compiler
> optimizing it just leads to code that is hard to read.
>
> > +#define R32_VAL(r, boff)       (((r) >> (boff)) & BIT(0))
>
> To check the value of a certain bit use this pattern:
>
> if (val & BIT(n))
>
> To return a boolean clamped bit (return 0/1) do this idiom:
>
> return !!(val & BIT(n));

Yes, I'll remove the four macros.
Write explicit inline code.


> Other than these things I didn't notice anything more this
> time, but I might find even more stuff, but hey it's getting there!
> Yours,
> Linus Walleij
Thanks, I'll review the whole code again and fix
the similar improper coding you pointed out.


Best regards,
Wells Lu

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

* Re: [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021
       [not found]     ` <CAFnkrskBVLk0q77Xkk=E07gxmWSNfRtwznz9yv1fd07u0P2HgQ@mail.gmail.com>
@ 2021-12-10 23:36       ` Linus Walleij
  0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2021-12-10 23:36 UTC (permalink / raw)
  To: 呂芳騰
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	Wells Lu, Dvorkin Dmitry

On Fri, Dec 10, 2021 at 1:41 PM 呂芳騰 <wellslutw@gmail.com> wrote:

> SP7021 has 99 digital GPIO pins which are numbered from
> GPIO 0 to 98. All are multiplexed with some special function
> pins. There are 3 types special function pins:
>
> (1) function-pins:
>     For example, if control-field SPI_FLASH_SEL is set to 1,
>     GPIO 83, 84, 86 and 87 will be pins of SPI-NOR flash.
>     If it is set to 2, GPIO 76, 78, 79 and 81 will be pins of
>     SPI-NOR flash.
>
>     For example 2, if control-bit UA0_SEL is set to 1,
>     GPIO 88 and 89 will be TX and RX pins of UART_0
>     (UART channel 0).
>
>     For example 3, if control-bit EMMC_SEL is set to 1,
>     GPIO 72, 73, 74, 75, 76, 77, 78, 79, 80, 81 will be
>     pins of an eMMC device.
>
>     The driver defines properties "function" and "groups"
>     to select this kind of pins-function.
>
> (2) fully pin-mux pins:
>     GPIO 8 to 71 are 'fully pin-mux' pins.
>     Pins of peripherals of SP7021 (ex: UART_1, UART_2,
>     UART_3, UART_4, I2C_0, I2C_1, ..., SPI_0, SPI_1, ...
>     GPIO_INT0, GPIO_INT1, .., RMII_of_Ethernet, and etc.)
>     can be set to any pins of fully pin-mux pins.
>
>     EX1 (UART channel 1):
>     If control-field UA1_TX_SEL is set to 3, TX pin of
>     UART_1 will be routed to GPIO 10 (3 - 1 + 8 = 10)
>     If control-field UA1_RX_SEL is set to 4, RX pin of
>     UART_1 will be routed to GPIO 11 (4 - 1 + 8 = 11)
>     If control-field UA1_RTS_SEL is set to 5, RTS pin of
>     UART_1 will be routed to GPIO 12 (5 - 1 + 8 = 12)
>     If control-field UA1_CTS_SEL is set to 6, CTS pin of
>     UART_1 will be routed to GPIO 13 (6 - 1 + 8 = 13)
>
>     EX2 (I2C channel 0):
>     If control-field I2C0_CLK_SEL is set to 20, CLK pin
>     of I2C_0 will be routed to GPIO 27 (20 - 1 + 8 = 27)
>     If control-field I2C0_DATA_SEL is set to 21, DATA pin
>     of I2C_0 will be routed to GPIO 28 (21 - 1 + 9 = 28)
>
>     Totally, SP7021 has 120 peripheral pins. The
>     peripheral pins can be routed to any of 64  'fully
>      pin-mux' pins. So total combinations are:
>            120 x 64 = 7680
>      This is why we cannot enumerate all combinations.

Please copy some version of the above into the top level
description in the bindings. People writing DTS files need to
know what they are doing and why this looks so funny
and if the explanation is right there in the bindings document,
that is helpful.

This is pretty cool. It is what is called a "phone exchange mux",
(anyone can call any number) which is a type that the pin
control subsystem simply assumed did not exist because it
would take up a lot of silicon space. But here it is.

So:

>> > +      properties:
>> > +        pins:
>> > +          description: |
>> > +            Define pins which are used by pinctrl node's client device.
>> (...)
>> > +          $ref: /schemas/types.yaml#/definitions/uint32-array
>>
>> Why can this not $ref the standard binings in
>> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml

If this does not conform with the standard definition of pins from
pinmux-node.yaml then you need to vendor prefix them, this will
then be something like sunplus,pins.

Yours,
Linus Walleij

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

* Re: [PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
       [not found]     ` <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
@ 2021-12-10 23:45       ` Linus Walleij
  0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2021-12-10 23:45 UTC (permalink / raw)
  To: 呂芳騰
  Cc: linux-gpio, linux-kernel, robh+dt, devicetree, linux-arm-kernel,
	Wells Lu, Dvorkin Dmitry

Hi Wells,

I understand things better now!

On Fri, Dec 10, 2021 at 6:34 PM 呂芳騰 <wellslutw@gmail.com> wrote:

> > #define SSPCTL_FUNC_FLAG BIT(0)
> >
> > if (func & SSPCTL_FUNC_FLAG)
> >
> > Use the name that bit has in your documentation for the
> > define so we know what is going on.
>
> Actually, 'if (func & 1)' is not used to test bit 0,
> but test 'func' is an odd number or not.
> If 'func' is even number, control-field is at bit 6 ~ 0.
> Its corresponding mask-field is at bit 22 ~ 16.
> If 'func' is odd number, control-field is at bit 14 ~ 8.
> Its corresponding mask-field is at bit 30 ~ 24.

Aha! That makes sense. Just put in exactly that comment in
a block comment in the code so people maintaining this
can see what is going on here and why you are checking
for an odd/even number.

> Control and mask fields of 'func' are arranged as shown
> below:
>
> func # | register control-field  mask-field
> -------+------------------------------------
>    0   | reg[0]     ( 6:0)        (22 :  6)
>    1   | reg[0]     (14:8)        (30 : 24)
>    2   | reg[1]     ( 6:0)        (22 :  6)
>    3   | reg[1]     (14:8)        (30 : 24)

This is also nice to actually have in the code. Be generous
with comments I like that.

> > This layout with "mask and value" in registers needs to be explained
> > somewhere it looks complex. I don't understand why a machine register
> > contains a mask for example.
>
> This is a hardware mechanism for protecting some important registers from
> being overwritten accidentally. The corresponding mask bit should be set
> first and then the control-bits or fields can be written. The design is
> originally requested from car makers.

Wow that is very very interesting! So the feature is there to stop a
program that goes astray and just write random numbers all over the
memory from doing something harmful.

Insert some comments about this at the top of the file or at the first
time you use a mask.

Thanks for keeping fixing this up despite my sometimes confused
comments. Most of my comment is about making the code easy
to read by people that will maintain it, so my rule of thumb is if I
have a hard time to understand it then others may have a hard time
with it too.

Yours,
Linus Walleij

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

end of thread, other threads:[~2021-12-10 23:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-07  4:17 [PATCH v3 0/2] This is a patch series for pinctrl driver of Sunplus SP7021 SoC Wells Lu
2021-12-07  4:17 ` [PATCH v3 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021 Wells Lu
2021-12-09 23:30   ` Linus Walleij
2021-12-10 14:07     ` 呂芳騰
     [not found]     ` <CAFnkrskBVLk0q77Xkk=E07gxmWSNfRtwznz9yv1fd07u0P2HgQ@mail.gmail.com>
2021-12-10 23:36       ` Linus Walleij
2021-12-07  4:17 ` [PATCH v3 2/2] pinctrl: Add driver " Wells Lu
2021-12-10  1:24   ` Linus Walleij
2021-12-10 18:33     ` 呂芳騰
     [not found]     ` <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
2021-12-10 23:45       ` Linus Walleij

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).