[RESEND PATCH 0/3] Add initial support for Canaan Kendryte K230 pinctrl

[RESEND PATCH 0/3] Add initial support for Canaan Kendryte K230 pinctrl

[Ze Huang]

Resend the patches to the same thread and fix incorrect patch ID.

This patch series introduces support for the pinctrl driver of the Canaan
K230 SoC. The K230 SoC features 64 IO pins, each of which can be configured
for up to five different functions.

The controller manages the entire pin configuration and multiplexing
through a single register, which control features such as schmitt trigger,
drive strength, bias pull-up/down, input/output enable, power source, and
mux mode.

The changes have been tested on the K230 development board.

The pin function definition can be found here [1], and most of the DTS data
was converted from the vendor's code [2].

Link: https://developer.canaan-creative.com/k230/dev/_downloads/a53655a81951bc8a440ae647be286e75/K230_PINOUT_V1.1_20230321.xlsx [1]
Link: https://github.com/kendryte/k230_sdk/blob/main/src/little/uboot/arch/riscv/dts/k230_canmv.dts [2]

Ze Huang (3):
  dt-bindings: pinctrl: Add support for canaan,k230 SoC
  pinctrl: canaan: Add support for k230 SoC
  riscv: dts: canaan: Add k230's pinctrl node

 .../bindings/pinctrl/canaan,k230-pinctrl.yaml | 128 ++++
 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi  | 316 ++++++++
 arch/riscv/boot/dts/canaan/k230-pinctrl.h     |  18 +
 arch/riscv/boot/dts/canaan/k230.dtsi          |   2 +
 drivers/pinctrl/Kconfig                       |  10 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-k230.c                | 674 ++++++++++++++++++
 7 files changed, 1149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
 create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
 create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h
 create mode 100644 drivers/pinctrl/pinctrl-k230.c

base-commit: 0eea987088a22d73d81e968de7347cdc7e594f72
prerequisite-patch-id: 2401703b57448c9ea2c3dc7650b4502491a28944
prerequisite-patch-id: 50ccf1104191cdf22f9077880d3dc781b190a3c8
prerequisite-patch-id: f8b983b301d0c14f1448b9e4c321262a509e061e
prerequisite-patch-id: ced4a01ccd8ddab2fd308d543ddf47bd1641518a
prerequisite-patch-id: c2144cf468c57b856830a61615ba6ba501e8ec58
prerequisite-patch-id: 704efc6e76814e1877748959d7319d558c8386c1
-- 
2.46.1

[RESEND PATCH 1/3] dt-bindings: pinctrl: Add support for canaan,k230 SoC

[Ze Huang]

Add device tree binding details for Canaan K230 pinctrl device.

Signed-off-by: Ze Huang <18771902331@163.com>
---
 .../bindings/pinctrl/canaan,k230-pinctrl.yaml | 128 ++++++++++++++++++
 1 file changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
new file mode 100644
index 000000000000..979c5bd71e3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
@@ -0,0 +1,128 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/canaan,k230-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Canaan Kendryte K230 Pin Controller
+
+maintainers:
+  - Ze Huang <18771902331@163.com>
+
+description:
+  The Canaan Kendryte K230 platform includes 64 IO pins, each capable of
+  multiplexing up to 5 different functions. Pin function configuration is
+  performed on a per-pin basis.
+
+properties:
+  compatible:
+    const: canaan,k230-pinctrl
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+    description:
+      A pinctrl node should contain at least one subnode representing the
+      pinctrl groups available on the machine.
+
+    patternProperties:
+      '-cfg$':
+        type: object
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+        additionalProperties: false
+        description:
+          Each subnode will list the pins it needs, and how they should
+          be configured, with regard to muxer configuration, bias, input
+          enable/disable, input schmitt trigger, slew-rate enable/disable,
+          slew-rate, drive strength.
+
+        properties:
+          pinmux:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            description:
+              The list of GPIOs and their mux settings that properties in
+              the node apply to. This should be set with the macro
+              'K230_PINMUX(pin, mode)'
+
+          bias-disable: true
+
+          bias-pull-up: true
+
+          bias-pull-down: true
+
+          drive-strength:
+            minimum: 0
+            maximum: 15
+
+          input-enable: true
+
+          output-enable: true
+
+          input-schmitt-enable: true
+
+          slew-rate:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: |
+              slew rate control enable
+              0: disable
+              1: enable
+
+            enum: [0, 1]
+
+          power-source:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: |
+              Specifies the power source voltage for the IO bank that the
+              pin belongs to. Each bank of IO pins operate at a specific,
+              fixed voltage levels. Incorrect voltage configuration can
+              damage the chip. The defined constants represent the
+              possible voltage configurations:
+
+              - K230_MSC_3V3 (value 0): 3.3V power supply
+              - K230_MSC_1V8 (value 1): 1.8V power supply
+
+              The following banks have the corresponding voltage
+              configurations:
+
+              - bank IO0 to IO1: Fixed at 1.8V
+              - bank IO2 to IO13: Fixed at 1.8V
+              - bank IO14 to IO25: Fixed at 1.8V
+              - bank IO26 to IO37: Fixed at 1.8V
+              - bank IO38 to IO49: Fixed at 1.8V
+              - bank IO50 to IO61: Fixed at 3.3V
+              - bank IO62 to IO63: Fixed at 1.8V
+
+            enum: [0, 1]
+
+        required:
+          - pinmux
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pinctrl: pinctrl@91105000 {
+        compatible = "canaan,k230-pinctrl";
+        reg = <0x91105000 0x100>;
+
+        uart2_pins: uart2-pins {
+            uart2-pins-cfg {
+                pinmux = <0x503>, /* uart2 txd */
+                         <0x603>; /* uart2 rxd */
+                slew-rate = <0>;
+                drive-strength = <4>;
+                power-source = <1>;
+                input-enable;
+                output-enable;
+                bias-disable;
+            };
+        };
+    };
-- 
2.46.1

[RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC

[Ze Huang]

Configuration of the K230 is similar to that of the K210. However, in K210,
the 256 functions for each pin are shared, whereas in K230, multiplex
functions are different for every pin.

Signed-off-by: Ze Huang <18771902331@163.com>
---
 drivers/pinctrl/Kconfig        |  10 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-k230.c | 674 +++++++++++++++++++++++++++++++++
 3 files changed, 685 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-k230.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 1be05efccc29..ff85dd8757fe 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -246,6 +246,16 @@ config PINCTRL_K210
 	  Add support for the Canaan Kendryte K210 RISC-V SOC Field
 	  Programmable IO Array (FPIOA) controller.
 
+config PINCTRL_K230
+	bool "Pinctrl driver for the Canaan Kendryte K230 SoC"
+	depends on OF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	select REGMAP_MMIO
+	help
+	  Add support for the Canaan Kendryte K230 RISC-V SOC pin controller.
+
 config PINCTRL_KEEMBAY
 	tristate "Pinctrl driver for Intel Keem Bay SoC"
 	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 2152539b53d5..66e7a04ecfa4 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_PINCTRL_EQUILIBRIUM)   += pinctrl-equilibrium.o
 obj-$(CONFIG_PINCTRL_GEMINI)	+= pinctrl-gemini.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_K210)	+= pinctrl-k210.o
+obj-$(CONFIG_PINCTRL_K230)	+= pinctrl-k230.o
 obj-$(CONFIG_PINCTRL_KEEMBAY)	+= pinctrl-keembay.o
 obj-$(CONFIG_PINCTRL_LANTIQ)	+= pinctrl-lantiq.o
 obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
diff --git a/drivers/pinctrl/pinctrl-k230.c b/drivers/pinctrl/pinctrl-k230.c
new file mode 100644
index 000000000000..a935f1f55e66
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-k230.c
@@ -0,0 +1,674 @@
+// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+/*
+ * Copyright (C) 2024 Canaan Bright Sight Co. Ltd
+ * Copyright (C) 2024 Ze Huang <18771902331@163.com>
+ */
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "core.h"
+#include "pinconf.h"
+
+#define K230_NPINS 64
+
+#define K230_SHIFT_ST		(0)
+#define K230_SHIFT_DS		(1)
+#define K230_SHIFT_BIAS		(5)
+#define K230_SHIFT_PD		(5)
+#define K230_SHIFT_PU		(6)
+#define K230_SHIFT_OE		(7)
+#define K230_SHIFT_IE		(8)
+#define K230_SHIFT_MSC		(9)
+#define K230_SHIFT_SL		(10)
+#define K230_SHIFT_SEL		(11)
+
+#define K230_PC_ST		BIT(0)
+#define K230_PC_DS		GENMASK(4, 1)
+#define K230_PC_PD		BIT(5)
+#define K230_PC_PU		BIT(6)
+#define K230_PC_BIAS		GENMASK(6, 5)
+#define K230_PC_OE		BIT(7)
+#define K230_PC_IE		BIT(8)
+#define K230_PC_MSC		BIT(9)
+#define K230_PC_SL		BIT(10)
+#define K230_PC_SEL		GENMASK(13, 11)
+
+struct k230_pin_conf {
+	unsigned int		func;
+	unsigned long		*configs;
+	unsigned int		nconfigs;
+};
+
+struct k230_pin_group {
+	const char		*name;
+	unsigned int		*pins;
+	unsigned int		num_pins;
+
+	struct k230_pin_conf	*data;
+};
+
+struct k230_pmx_func {
+	const char		*name;
+	const char		**groups;
+	unsigned int		*group_idx;
+	unsigned int		ngroups;
+};
+
+struct k230_pinctrl {
+	struct pinctrl_desc	pctl;
+	struct pinctrl_dev	*pctl_dev;
+	struct regmap		*regmap_base;
+	void __iomem		*base;
+	struct k230_pin_group	*groups;
+	unsigned int		ngroups;
+	struct k230_pmx_func	*functions;
+	unsigned int		nfunctions;
+};
+
+static struct regmap_config k230_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.max_register	= 0x100,
+	.reg_stride	= 4,
+};
+
+static int k230_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->ngroups;
+}
+
+static const char *k230_get_group_name(struct pinctrl_dev *pctldev,
+				       unsigned int selector)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->groups[selector].name;
+}
+
+static int k230_get_group_pins(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       const unsigned int **pins,
+			       unsigned int *num_pins)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector >= info->ngroups)
+		return -EINVAL;
+
+	*pins = info->groups[selector].pins;
+	*num_pins = info->groups[selector].num_pins;
+
+	return 0;
+}
+
+static inline const struct k230_pmx_func *k230_name_to_funtion(
+		const struct k230_pinctrl *info, const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < info->nfunctions; i++) {
+		if (!strcmp(info->functions[i].name, name))
+			return &info->functions[i];
+	}
+
+	return NULL;
+}
+
+static int k230_pinctrl_parse_groups(struct device_node *np,
+				     struct k230_pin_group *grp,
+				     struct k230_pinctrl *info,
+				     unsigned int index)
+{
+	struct device *dev = info->pctl_dev->dev;
+	const __be32 *list;
+	int size, i, ret;
+
+	grp->name = np->name;
+
+	list = of_get_property(np, "pinmux", &size);
+	size /= sizeof(*list);
+
+	grp->num_pins = size;
+	grp->pins = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->pins),
+				 GFP_KERNEL);
+	grp->data = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->data),
+				 GFP_KERNEL);
+	if (!grp->pins || !grp->data)
+		return -ENOMEM;
+
+	for (i = 0; i < size; i++) {
+		unsigned int mux_data = be32_to_cpu(*list++);
+
+		grp->pins[i] = (mux_data >> 8);
+		grp->data[i].func = (mux_data & 0xff);
+
+		ret = pinconf_generic_parse_dt_config(np, NULL,
+						      &grp->data[i].configs,
+						      &grp->data[i].nconfigs);
+		if (ret)
+			return ret;
+	}
+	of_node_put(np);
+
+	return 0;
+}
+
+static void k230_pinctrl_child_count(struct k230_pinctrl *info,
+				     struct device_node *np)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(np, child) {
+		info->nfunctions++;
+		info->ngroups += of_get_child_count(child);
+	}
+}
+
+static int k230_pinctrl_parse_functions(struct device_node *np,
+					struct k230_pinctrl *info,
+					unsigned int index)
+{
+	struct device *dev = info->pctl_dev->dev;
+	struct k230_pmx_func *func;
+	struct k230_pin_group *grp;
+	struct device_node *child;
+	static unsigned int idx, i;
+	int ret;
+
+	func = &info->functions[index];
+
+	func->name = np->name;
+	func->ngroups = of_get_child_count(np);
+	if (func->ngroups <= 0)
+		return 0;
+
+	func->groups = devm_kcalloc(dev, func->ngroups,
+				    sizeof(*func->groups), GFP_KERNEL);
+	func->group_idx = devm_kcalloc(dev, func->ngroups,
+				       sizeof(*func->group_idx), GFP_KERNEL);
+	if (!func->groups || !func->group_idx)
+		return -ENOMEM;
+
+	i = 0;
+
+	for_each_child_of_node(np, child) {
+		func->groups[i] = child->name;
+		func->group_idx[i] = idx;
+		grp = &info->groups[idx];
+		idx++;
+		ret = k230_pinctrl_parse_groups(child, grp, info, i++);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int k230_pinctrl_parse_dt(struct platform_device *pdev,
+				 struct k230_pinctrl *info)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *child;
+	unsigned int i;
+	int ret;
+
+	k230_pinctrl_child_count(info, np);
+
+	info->functions = devm_kcalloc(dev, info->nfunctions,
+				       sizeof(*info->functions), GFP_KERNEL);
+	info->groups = devm_kcalloc(dev, info->ngroups,
+				    sizeof(*info->groups), GFP_KERNEL);
+	if (!info->functions || !info->groups)
+		return -ENOMEM;
+
+	i = 0;
+
+	for_each_child_of_node(np, child) {
+		ret = k230_pinctrl_parse_functions(child, info, i++);
+		if (ret) {
+			dev_err(dev, "failed to parse function\n");
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static struct pinctrl_pin_desc k230_pins[] = {
+	PINCTRL_PIN(0,  "IO0"),  PINCTRL_PIN(1,  "IO1"),  PINCTRL_PIN(2,  "IO2"),
+	PINCTRL_PIN(3,  "IO3"),  PINCTRL_PIN(4,  "IO4"),  PINCTRL_PIN(5,  "IO5"),
+	PINCTRL_PIN(6,  "IO6"),  PINCTRL_PIN(7,  "IO7"),  PINCTRL_PIN(8,  "IO8"),
+	PINCTRL_PIN(9,  "IO9"),  PINCTRL_PIN(10, "IO10"), PINCTRL_PIN(11, "IO11"),
+	PINCTRL_PIN(12, "IO12"), PINCTRL_PIN(13, "IO13"), PINCTRL_PIN(14, "IO14"),
+	PINCTRL_PIN(15, "IO15"), PINCTRL_PIN(16, "IO16"), PINCTRL_PIN(17, "IO17"),
+	PINCTRL_PIN(18, "IO18"), PINCTRL_PIN(19, "IO19"), PINCTRL_PIN(20, "IO20"),
+	PINCTRL_PIN(21, "IO21"), PINCTRL_PIN(22, "IO22"), PINCTRL_PIN(23, "IO23"),
+	PINCTRL_PIN(24, "IO24"), PINCTRL_PIN(25, "IO25"), PINCTRL_PIN(26, "IO26"),
+	PINCTRL_PIN(27, "IO27"), PINCTRL_PIN(28, "IO28"), PINCTRL_PIN(29, "IO29"),
+	PINCTRL_PIN(30, "IO30"), PINCTRL_PIN(31, "IO31"), PINCTRL_PIN(32, "IO32"),
+	PINCTRL_PIN(33, "IO33"), PINCTRL_PIN(34, "IO34"), PINCTRL_PIN(35, "IO35"),
+	PINCTRL_PIN(36, "IO36"), PINCTRL_PIN(37, "IO37"), PINCTRL_PIN(38, "IO38"),
+	PINCTRL_PIN(39, "IO39"), PINCTRL_PIN(40, "IO40"), PINCTRL_PIN(41, "IO41"),
+	PINCTRL_PIN(42, "IO42"), PINCTRL_PIN(43, "IO43"), PINCTRL_PIN(44, "IO44"),
+	PINCTRL_PIN(45, "IO45"), PINCTRL_PIN(46, "IO46"), PINCTRL_PIN(47, "IO47"),
+	PINCTRL_PIN(48, "IO48"), PINCTRL_PIN(49, "IO49"), PINCTRL_PIN(50, "IO50"),
+	PINCTRL_PIN(51, "IO51"), PINCTRL_PIN(52, "IO52"), PINCTRL_PIN(53, "IO53"),
+	PINCTRL_PIN(54, "IO54"), PINCTRL_PIN(55, "IO55"), PINCTRL_PIN(56, "IO56"),
+	PINCTRL_PIN(57, "IO57"), PINCTRL_PIN(58, "IO58"), PINCTRL_PIN(59, "IO59"),
+	PINCTRL_PIN(60, "IO60"), PINCTRL_PIN(61, "IO61"), PINCTRL_PIN(62, "IO62"),
+	PINCTRL_PIN(63, "IO63")
+};
+
+static void k230_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
+				      struct seq_file *s, unsigned int offset)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	u32 val, mode, bias, drive, input, output, slew, schmitt, power;
+	struct k230_pin_group *grp = k230_pins[offset].drv_data;
+	static const char * const biasing[] = {
+			"pull none", "pull down", "pull up", "" };
+	static const char * const enable[] = {
+			"disable", "enable" };
+	static const char * const power_source[] = {
+			"3V3", "1V8" };
+	int ret;
+
+	ret = regmap_read(info->regmap_base, offset * 4, &val);
+	if (ret) {
+		dev_err(info->pctl_dev->dev,
+			"failed to read offset 0x%x\n", offset * 4);
+		return;
+	}
+
+	mode	= (val & K230_PC_SEL) >> K230_SHIFT_SEL;
+	drive	= (val & K230_PC_DS) >> K230_SHIFT_DS;
+	bias	= (val & K230_PC_BIAS) >> K230_SHIFT_BIAS;
+	input	= (val & K230_PC_IE) >> K230_SHIFT_IE;
+	output	= (val & K230_PC_OE) >> K230_SHIFT_OE;
+	slew	= (val & K230_PC_SL) >> K230_SHIFT_SL;
+	schmitt	= (val & K230_PC_ST) >> K230_SHIFT_ST;
+	power	= (val & K230_PC_MSC) >> K230_SHIFT_MSC;
+
+	seq_printf(s, "%s - strength %d - %s - %s - slewrate %s - schmitt %s - %s",
+		   grp ? grp->name : "unknown",
+		   drive,
+		   biasing[bias],
+		   input ? "input" : "output",
+		   enable[slew],
+		   enable[schmitt],
+		   power_source[power]);
+}
+
+static int k230_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np_config,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct device *dev = info->pctl_dev->dev;
+	const struct k230_pmx_func *func;
+	const struct k230_pin_group *grp;
+	struct pinctrl_map *new_map;
+	int map_num, i, j, idx;
+	unsigned int grp_id;
+
+	func = k230_name_to_funtion(info, np_config->name);
+	if (!func) {
+		dev_err(dev, "function %s not found\n", np_config->name);
+		return -EINVAL;
+	}
+
+	map_num = 0;
+	for (i = 0; i < func->ngroups; ++i) {
+		grp_id = func->group_idx[i];
+		/* npins of config map plus a mux map */
+		map_num += info->groups[grp_id].num_pins + 1;
+	}
+
+	new_map = kcalloc(map_num, sizeof(*new_map), GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+	*map = new_map;
+	*num_maps = map_num;
+
+	idx = 0;
+	for (i = 0; i < func->ngroups; ++i) {
+		grp_id = func->group_idx[i];
+		grp = &info->groups[grp_id];
+		new_map[idx].type = PIN_MAP_TYPE_MUX_GROUP;
+		new_map[idx].data.mux.group = grp->name;
+		new_map[idx].data.mux.function = np_config->name;
+		idx++;
+
+		for (j = 0; j < grp->num_pins; ++j) {
+			new_map[idx].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[idx].data.configs.group_or_pin =
+				pin_get_name(pctldev, grp->pins[j]);
+			new_map[idx].data.configs.configs =
+				grp->data[j].configs;
+			new_map[idx].data.configs.num_configs =
+				grp->data[j].nconfigs;
+			idx++;
+		}
+	}
+
+	return 0;
+}
+
+static void k230_dt_free_map(struct pinctrl_dev *pctldev,
+			     struct pinctrl_map *map, unsigned int num_maps)
+{
+	kfree(map);
+}
+
+static const struct pinctrl_ops k230_pctrl_ops = {
+	.get_groups_count	= k230_get_groups_count,
+	.get_group_name		= k230_get_group_name,
+	.get_group_pins		= k230_get_group_pins,
+	.pin_dbg_show		= k230_pinctrl_pin_dbg_show,
+	.dt_node_to_map		= k230_dt_node_to_map,
+	.dt_free_map		= k230_dt_free_map,
+};
+
+static int k230_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned int val, arg;
+	int ret;
+
+	ret = regmap_read(info->regmap_base, pin * 4, &val);
+	if (ret) {
+		dev_err(info->pctl_dev->dev,
+			"failed to read offset 0x%x\n", pin * 4);
+		return ret;
+	}
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		arg = (val & K230_PC_ST) ? 1 : 0;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		arg = (val & K230_PC_DS) >> K230_SHIFT_DS;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		arg = (val & K230_PC_BIAS) ? 0 : 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = (val & K230_PC_PD) ? 1 : 0;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = (val & K230_PC_PU) ? 1 : 0;
+		break;
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		arg = (val & K230_PC_OE) ? 1 : 0;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		arg = (val & K230_PC_IE) ? 1 : 0;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		arg = (val & K230_PC_MSC) ? 1 : 0;
+		break;
+	case PIN_CONFIG_SLEW_RATE:
+		arg = (val & K230_PC_SL) ? 1 : 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int k230_pinconf_set_param(struct pinctrl_dev *pctldev, unsigned int pin,
+				  enum pin_config_param param, unsigned int arg)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct device *dev = info->pctl_dev->dev;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(info->regmap_base, pin * 4, &val);
+	if (ret) {
+		dev_err(dev, "failed to read offset 0x%x\n", pin * 4);
+		return ret;
+	}
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		if (arg)
+			val |= K230_PC_ST;
+		else
+			val &= ~K230_PC_ST;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		val &= ~K230_PC_DS;
+		val |= (arg << K230_SHIFT_DS) & K230_PC_DS;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		val &= ~K230_PC_BIAS;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (!arg)
+			return -EINVAL;
+		val |= K230_PC_PD;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (!arg)
+			return -EINVAL;
+		val |= K230_PC_PU;
+		break;
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		if (!arg)
+			return -EINVAL;
+		val |= K230_PC_OE;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		if (!arg)
+			return -EINVAL;
+		val |= K230_PC_IE;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		if (arg)
+			val |= K230_PC_MSC;
+		else
+			val &= ~K230_PC_MSC;
+		break;
+	case PIN_CONFIG_SLEW_RATE:
+		if (arg)
+			val |= K230_PC_SL;
+		else
+			val &= ~K230_PC_SL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write(info->regmap_base, pin * 4, val);
+	if (ret) {
+		dev_err(dev, "failed to write offset 0x%x\n", pin * 4);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int k230_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	enum pin_config_param param;
+	unsigned int arg, i;
+	int ret;
+
+	if (WARN_ON(pin >= K230_NPINS))
+		return -EINVAL;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+		ret = k230_pinconf_set_param(pctldev, pin, param, arg);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void k230_pconf_dbg_show(struct pinctrl_dev *pctldev,
+				struct seq_file *s, unsigned int pin)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(info->regmap_base, pin * 4, &val);
+	if (ret) {
+		dev_err(info->pctl_dev->dev, "failed to read offset 0x%x\n", pin * 4);
+		return;
+	}
+
+	seq_printf(s, " 0x%08x", val);
+}
+
+static const struct pinconf_ops k230_pinconf_ops = {
+	.is_generic		= true,
+	.pin_config_get		= k230_pinconf_get,
+	.pin_config_set		= k230_pinconf_set,
+	.pin_config_dbg_show	= k230_pconf_dbg_show,
+};
+
+static int k230_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->nfunctions;
+}
+
+static const char *k230_get_fname(struct pinctrl_dev *pctldev,
+				  unsigned int selector)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	return info->functions[selector].name;
+}
+
+static int k230_get_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+			   const char * const **groups, unsigned int *num_groups)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = info->functions[selector].groups;
+	*num_groups = info->functions[selector].ngroups;
+
+	return 0;
+}
+
+static int k230_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			unsigned int group)
+{
+	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	const struct k230_pin_conf *data = info->groups[group].data;
+	struct k230_pin_group *grp = &info->groups[group];
+	const unsigned int *pins = grp->pins;
+	struct regmap *regmap;
+	unsigned int value, mask;
+	int cnt, reg;
+
+	regmap = info->regmap_base;
+
+	for (cnt = 0; cnt < grp->num_pins; cnt++) {
+		reg = pins[cnt] * 4;
+		value = data[cnt].func << K230_SHIFT_SEL;
+		mask = K230_PC_SEL;
+		regmap_update_bits(regmap, reg, mask, value);
+		k230_pins[pins[cnt]].drv_data = grp;
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops k230_pmxops = {
+	.get_functions_count	= k230_get_functions_count,
+	.get_function_name	= k230_get_fname,
+	.get_function_groups	= k230_get_groups,
+	.set_mux		= k230_set_mux,
+	.strict			= true,
+};
+
+static int k230_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct k230_pinctrl *info;
+	struct pinctrl_desc *pctl;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	pctl = &info->pctl;
+
+	pctl->name	= "k230-pinctrl";
+	pctl->owner	= THIS_MODULE;
+	pctl->pins	= k230_pins;
+	pctl->npins	= ARRAY_SIZE(k230_pins);
+	pctl->pctlops	= &k230_pctrl_ops;
+	pctl->pmxops	= &k230_pmxops;
+	pctl->confops	= &k230_pinconf_ops;
+
+	info->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
+
+	k230_regmap_config.name = "canaan,pinctrl";
+	info->regmap_base = devm_regmap_init_mmio(dev, info->base,
+						  &k230_regmap_config);
+	if (IS_ERR(info->regmap_base))
+		return dev_err_probe(dev, PTR_ERR(info->regmap_base),
+				     "failed to init regmap\n");
+
+	info->pctl_dev = devm_pinctrl_register(dev, pctl, info);
+	if (IS_ERR(info->pctl_dev))
+		return dev_err_probe(dev, PTR_ERR(info->pctl_dev),
+				     "devm_pinctrl_register failed\n");
+
+	k230_pinctrl_parse_dt(pdev, info);
+
+	return 0;
+}
+
+static const struct of_device_id k230_dt_ids[] = {
+	{ .compatible = "canaan,k230-pinctrl", },
+	{ /* sintenel */ }
+};
+MODULE_DEVICE_TABLE(of, k230_dt_ids);
+
+static struct platform_driver k230_pinctrl_driver = {
+	.probe = k230_pinctrl_probe,
+	.driver = {
+		.name = "k230-pinctrl",
+		.of_match_table = k230_dt_ids,
+	},
+};
+module_platform_driver(k230_pinctrl_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ze Huang <18771902331@163.com>");
+MODULE_DESCRIPTION("Canaan K230 pinctrl driver");
-- 
2.46.1

[RESEND PATCH 3/3] riscv: dts: canaan: Add k230's pinctrl node

[Ze Huang]

Add pinctrl device, containing default config for uart, pwm, iis, iic and
mmc.

Signed-off-by: Ze Huang <18771902331@163.com>
---
 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi | 316 +++++++++++++++++++
 arch/riscv/boot/dts/canaan/k230-pinctrl.h    |  18 ++
 arch/riscv/boot/dts/canaan/k230.dtsi         |   2 +
 3 files changed, 336 insertions(+)
 create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
 create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h

diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
new file mode 100644
index 000000000000..0737f50d2868
--- /dev/null
+++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2024 Ze Huang <18771902331@163.com>
+ */
+#include "k230-pinctrl.h"
+
+/ {
+	soc {
+		pinctrl: pinctrl@91105000 {
+			compatible = "canaan,k230-pinctrl";
+			reg = <0x0 0x91105000 0x0 0x100>;
+
+			jtag_pins: jtag-pins {
+				jtag-tck-cfg {
+					pinmux = <K230_PINMUX(2, 1)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-pull-down;
+					input-schmitt-enable;
+				};
+
+				jtag-tdi-cfg {
+					pinmux = <K230_PINMUX(3, 1)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-disable;
+				};
+
+				jtag-tdo-cfg {
+					pinmux = <K230_PINMUX(4, 1)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+				};
+
+				jtag-tms-cfg {
+					pinmux = <K230_PINMUX(5, 1)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-pull-up;
+				};
+			};
+
+			uart2_pins: uart2-pins {
+				uart2-pins-cfg {
+					pinmux = <K230_PINMUX(5, 3)>, /* uart2 txd */
+						 <K230_PINMUX(6, 3)>; /* uart2 rxd */
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-disable;
+				};
+			};
+
+			pwm2_pins: pwm2-pins {
+				pwm2-pin-cfg {
+					pinmux = <K230_PINMUX(7, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			pwm3_pins: pwm3-pins {
+				pwm3-pin-cfg {
+					pinmux = <K230_PINMUX(8, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			pwm4_pins: pwm4-pins {
+				pwm4-pin-cfg {
+					pinmux = <K230_PINMUX(9, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			iis_pins: iis-pins {
+				iis-clk-cfg {
+					pinmux = <K230_PINMUX(32, 2)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+				};
+
+				iis-ws-cfg {
+					pinmux = <K230_PINMUX(33, 2)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+				};
+
+				iis-din0-cfg {
+					pinmux = <K230_PINMUX(34, 2)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-disable;
+				};
+
+				iis-dout0-cfg {
+					pinmux = <K230_PINMUX(35, 2)>;
+					slew-rate = <0>;
+					drive-strength = <4>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+				};
+			};
+
+			uart4_pins: uart4-pins {
+				uart4-txd-cfg {
+					pinmux = <K230_PINMUX(36, 4)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+
+				uart4-rxd-cfg {
+					pinmux = <K230_PINMUX(37, 4)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			uart0_pins: uart0-pins {
+				uart0-txd-cfg {
+					pinmux = <K230_PINMUX(38, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+
+				uart0-rxd-cfg {
+					pinmux = <K230_PINMUX(39, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			iic1_pins: iic1-pins {
+				iic1-pins-cfg {
+					pinmux = <K230_PINMUX(40, 2)>, /* iic1 scl */
+						 <K230_PINMUX(41, 2)>; /* iic1 sda */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+			};
+
+			iic3_pins: iic3-pins {
+				iic3-pins-cfg {
+					pinmux = <K230_PINMUX(44, 2)>, /* iic3 scl */
+						 <K230_PINMUX(45, 2)>; /* iic3 sda */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+			};
+
+			iic4_pins: iic4-pins {
+				iic4-pins-cfg {
+					pinmux = <K230_PINMUX(46, 3)>, /* iic4 scl */
+						 <K230_PINMUX(47, 3)>; /* iic4 sda */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+			};
+
+			iic0_pins: iic0-pins {
+				iic0-pins-cfg {
+					pinmux = <K230_PINMUX(48, 3)>, /* iic0 scl */
+						 <K230_PINMUX(49, 3)>; /* iic0 sda */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_1V8>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+			};
+
+			uart3_pins: uart3-pins {
+				uart3-txd-cfg {
+					pinmux = <K230_PINMUX(50, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+
+				uart3-rxd-cfg {
+					pinmux = <K230_PINMUX(51, 1)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					input-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			key_pins: key-pins {
+				key-pins-cfg {
+					pinmux = <K230_PINMUX(52, 0)>, /* key0 */
+						 <K230_PINMUX(53, 0)>; /* key1 */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					input-enable;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+			};
+
+			mmc1_pins: mmc1-pins {
+				mmc1-cmd-cfg {
+					pinmux = <K230_PINMUX(54, 2)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+
+				mmc1-clk-cfg {
+					pinmux = <K230_PINMUX(55, 2)>;
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					output-enable;
+					bias-disable;
+					input-schmitt-enable;
+				};
+
+				mmc1-data-cfg {
+					pinmux = <K230_PINMUX(56, 2)>, /* mmc1 data0 */
+						 <K230_PINMUX(57, 2)>, /* mmc1 data1 */
+						 <K230_PINMUX(58, 2)>, /* mmc1 data2 */
+						 <K230_PINMUX(59, 2)>; /* mmc1 data3 */
+					slew-rate = <0>;
+					drive-strength = <7>;
+					power-source = <K230_MSC_3V3>;
+					input-enable;
+					output-enable;
+					bias-pull-up;
+					input-schmitt-enable;
+				};
+			};
+		};
+	};
+};
diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.h b/arch/riscv/boot/dts/canaan/k230-pinctrl.h
new file mode 100644
index 000000000000..f240a980f37a
--- /dev/null
+++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.h
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+/*
+ * Copyright (C) 2024 Canaan Bright Sight Co. Ltd
+ * Copyright (C) 2024 Ze Huang <18771902331@163.com>
+ */
+
+#ifndef _K230_PINCTRL_H
+#define _K230_PINCTRL_H
+
+#define K230_MSC_3V3 0
+#define K230_MSC_1V8 1
+
+#define BANK_VOLTAGE_DEFAULT       K230_MSC_1V8
+#define BANK_VOLTAGE_IO50_IO61     K230_MSC_3V3
+
+#define K230_PINMUX(pin, mode) (((pin) << 8) | (mode))
+
+#endif /* _K230_PINCTRL_H */
diff --git a/arch/riscv/boot/dts/canaan/k230.dtsi b/arch/riscv/boot/dts/canaan/k230.dtsi
index 95c1a3d8fb11..a9354e538642 100644
--- a/arch/riscv/boot/dts/canaan/k230.dtsi
+++ b/arch/riscv/boot/dts/canaan/k230.dtsi
@@ -140,3 +140,5 @@ uart4: serial@91404000 {
 		};
 	};
 };
+
+#include "k230-pinctrl.dtsi"
-- 
2.46.1

Re: [RESEND PATCH 3/3] riscv: dts: canaan: Add k230's pinctrl node

[Krzysztof Kozlowski]

On 16/09/2024 08:47, Ze Huang wrote:
> Add pinctrl device, containing default config for uart, pwm, iis, iic and
> mmc.
> 
> Signed-off-by: Ze Huang <18771902331@163.com>
> ---
>  arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi | 316 +++++++++++++++++++
>  arch/riscv/boot/dts/canaan/k230-pinctrl.h    |  18 ++
>  arch/riscv/boot/dts/canaan/k230.dtsi         |   2 +
>  3 files changed, 336 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>  create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h
> 
> diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
> new file mode 100644
> index 000000000000..0737f50d2868
> --- /dev/null
> +++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2024 Ze Huang <18771902331@163.com>
> + */
> +#include "k230-pinctrl.h"
> +
> +/ {
> +	soc {
> +		pinctrl: pinctrl@91105000 {

That's odd style - defining SoC nodes outside of SoC DTSI. Are you sure
that's preferred coding style in RISC-V or Canaan?

> +			compatible = "canaan,k230-pinctrl";
> +			reg = <0x0 0x91105000 0x0 0x100>;
> +

Best regards,
Krzysztof

Re: [RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC

[Krzysztof Kozlowski]

On 16/09/2024 08:47, Ze Huang wrote:
> Configuration of the K230 is similar to that of the K210. However, in K210,
> the 256 functions for each pin are shared, whereas in K230, multiplex
> functions are different for every pin.
> 
> Signed-off-by: Ze Huang <18771902331@163.com>
> ---
>  drivers/pinctrl/Kconfig        |  10 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-k230.c | 674 +++++++++++++++++++++++++++++++++
>  3 files changed, 685 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-k230.c

...

> +
> +struct k230_pinctrl {
> +	struct pinctrl_desc	pctl;
> +	struct pinctrl_dev	*pctl_dev;
> +	struct regmap		*regmap_base;
> +	void __iomem		*base;
> +	struct k230_pin_group	*groups;
> +	unsigned int		ngroups;
> +	struct k230_pmx_func	*functions;
> +	unsigned int		nfunctions;
> +};
> +
> +static struct regmap_config k230_regmap_config = {

Why is this not a const?

> +	.reg_bits	= 32,
> +	.val_bits	= 32,
> +	.max_register	= 0x100,
> +	.reg_stride	= 4,
> +};
> +
> +static int k230_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return info->ngroups;
> +}
> +
> +static const char *k230_get_group_name(struct pinctrl_dev *pctldev,
> +				       unsigned int selector)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return info->groups[selector].name;
> +}
> +
> +static int k230_get_group_pins(struct pinctrl_dev *pctldev,
> +			       unsigned int selector,
> +			       const unsigned int **pins,
> +			       unsigned int *num_pins)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (selector >= info->ngroups)
> +		return -EINVAL;
> +
> +	*pins = info->groups[selector].pins;
> +	*num_pins = info->groups[selector].num_pins;
> +
> +	return 0;
> +}
> +
> +static inline const struct k230_pmx_func *k230_name_to_funtion(
> +		const struct k230_pinctrl *info, const char *name)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < info->nfunctions; i++) {
> +		if (!strcmp(info->functions[i].name, name))
> +			return &info->functions[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static int k230_pinctrl_parse_groups(struct device_node *np,
> +				     struct k230_pin_group *grp,
> +				     struct k230_pinctrl *info,
> +				     unsigned int index)
> +{
> +	struct device *dev = info->pctl_dev->dev;
> +	const __be32 *list;
> +	int size, i, ret;
> +
> +	grp->name = np->name;
> +
> +	list = of_get_property(np, "pinmux", &size);
> +	size /= sizeof(*list);
> +
> +	grp->num_pins = size;
> +	grp->pins = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->pins),
> +				 GFP_KERNEL);
> +	grp->data = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->data),
> +				 GFP_KERNEL);
> +	if (!grp->pins || !grp->data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < size; i++) {
> +		unsigned int mux_data = be32_to_cpu(*list++);
> +
> +		grp->pins[i] = (mux_data >> 8);
> +		grp->data[i].func = (mux_data & 0xff);
> +
> +		ret = pinconf_generic_parse_dt_config(np, NULL,
> +						      &grp->data[i].configs,
> +						      &grp->data[i].nconfigs);
> +		if (ret)
> +			return ret;
> +	}
> +	of_node_put(np);
> +

This looks like double free. There is no get in this scope.

> +	return 0;
> +}
> +
> +static void k230_pinctrl_child_count(struct k230_pinctrl *info,
> +				     struct device_node *np)
> +{
> +	struct device_node *child;
> +
> +	for_each_child_of_node(np, child) {
> +		info->nfunctions++;
> +		info->ngroups += of_get_child_count(child);
> +	}
> +}
> +
> +static int k230_pinctrl_parse_functions(struct device_node *np,
> +					struct k230_pinctrl *info,
> +					unsigned int index)
> +{
> +	struct device *dev = info->pctl_dev->dev;
> +	struct k230_pmx_func *func;
> +	struct k230_pin_group *grp;
> +	struct device_node *child;
> +	static unsigned int idx, i;
> +	int ret;
> +
> +	func = &info->functions[index];
> +
> +	func->name = np->name;
> +	func->ngroups = of_get_child_count(np);
> +	if (func->ngroups <= 0)
> +		return 0;
> +
> +	func->groups = devm_kcalloc(dev, func->ngroups,
> +				    sizeof(*func->groups), GFP_KERNEL);
> +	func->group_idx = devm_kcalloc(dev, func->ngroups,
> +				       sizeof(*func->group_idx), GFP_KERNEL);
> +	if (!func->groups || !func->group_idx)
> +		return -ENOMEM;
> +
> +	i = 0;
> +
> +	for_each_child_of_node(np, child) {
> +		func->groups[i] = child->name;
> +		func->group_idx[i] = idx;
> +		grp = &info->groups[idx];
> +		idx++;
> +		ret = k230_pinctrl_parse_groups(child, grp, info, i++);
> +		if (ret) {
> +			of_node_put(child);

Use scoped loop instead.

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int k230_pinctrl_parse_dt(struct platform_device *pdev,
> +				 struct k230_pinctrl *info)

Please keep all probe related code next to each other. That's quite
confusing to find probe code far away from the probe().

> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	unsigned int i;
> +	int ret;
> +
> +	k230_pinctrl_child_count(info, np);
> +
> +	info->functions = devm_kcalloc(dev, info->nfunctions,
> +				       sizeof(*info->functions), GFP_KERNEL);
> +	info->groups = devm_kcalloc(dev, info->ngroups,
> +				    sizeof(*info->groups), GFP_KERNEL);
> +	if (!info->functions || !info->groups)
> +		return -ENOMEM;
> +
> +	i = 0;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = k230_pinctrl_parse_functions(child, info, i++);
> +		if (ret) {
> +			dev_err(dev, "failed to parse function\n");
> +			of_node_put(child);

Use scoped loop instead.

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct pinctrl_pin_desc k230_pins[] = {

Why is this not a const?

> +	PINCTRL_PIN(0,  "IO0"),  PINCTRL_PIN(1,  "IO1"),  PINCTRL_PIN(2,  "IO2"),
> +	PINCTRL_PIN(3,  "IO3"),  PINCTRL_PIN(4,  "IO4"),  PINCTRL_PIN(5,  "IO5"),
> +	PINCTRL_PIN(6,  "IO6"),  PINCTRL_PIN(7,  "IO7"),  PINCTRL_PIN(8,  "IO8"),
> +	PINCTRL_PIN(9,  "IO9"),  PINCTRL_PIN(10, "IO10"), PINCTRL_PIN(11, "IO11"),
> +	PINCTRL_PIN(12, "IO12"), PINCTRL_PIN(13, "IO13"), PINCTRL_PIN(14, "IO14"),
> +	PINCTRL_PIN(15, "IO15"), PINCTRL_PIN(16, "IO16"), PINCTRL_PIN(17, "IO17"),
> +	PINCTRL_PIN(18, "IO18"), PINCTRL_PIN(19, "IO19"), PINCTRL_PIN(20, "IO20"),
> +	PINCTRL_PIN(21, "IO21"), PINCTRL_PIN(22, "IO22"), PINCTRL_PIN(23, "IO23"),
> +	PINCTRL_PIN(24, "IO24"), PINCTRL_PIN(25, "IO25"), PINCTRL_PIN(26, "IO26"),
> +	PINCTRL_PIN(27, "IO27"), PINCTRL_PIN(28, "IO28"), PINCTRL_PIN(29, "IO29"),
> +	PINCTRL_PIN(30, "IO30"), PINCTRL_PIN(31, "IO31"), PINCTRL_PIN(32, "IO32"),
> +	PINCTRL_PIN(33, "IO33"), PINCTRL_PIN(34, "IO34"), PINCTRL_PIN(35, "IO35"),
> +	PINCTRL_PIN(36, "IO36"), PINCTRL_PIN(37, "IO37"), PINCTRL_PIN(38, "IO38"),
> +	PINCTRL_PIN(39, "IO39"), PINCTRL_PIN(40, "IO40"), PINCTRL_PIN(41, "IO41"),
> +	PINCTRL_PIN(42, "IO42"), PINCTRL_PIN(43, "IO43"), PINCTRL_PIN(44, "IO44"),
> +	PINCTRL_PIN(45, "IO45"), PINCTRL_PIN(46, "IO46"), PINCTRL_PIN(47, "IO47"),
> +	PINCTRL_PIN(48, "IO48"), PINCTRL_PIN(49, "IO49"), PINCTRL_PIN(50, "IO50"),
> +	PINCTRL_PIN(51, "IO51"), PINCTRL_PIN(52, "IO52"), PINCTRL_PIN(53, "IO53"),
> +	PINCTRL_PIN(54, "IO54"), PINCTRL_PIN(55, "IO55"), PINCTRL_PIN(56, "IO56"),
> +	PINCTRL_PIN(57, "IO57"), PINCTRL_PIN(58, "IO58"), PINCTRL_PIN(59, "IO59"),
> +	PINCTRL_PIN(60, "IO60"), PINCTRL_PIN(61, "IO61"), PINCTRL_PIN(62, "IO62"),
> +	PINCTRL_PIN(63, "IO63")
> +};
> +
> +static void k230_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
> +				      struct seq_file *s, unsigned int offset)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +	u32 val, mode, bias, drive, input, output, slew, schmitt, power;
> +	struct k230_pin_group *grp = k230_pins[offset].drv_data;
> +	static const char * const biasing[] = {
> +			"pull none", "pull down", "pull up", "" };
> +	static const char * const enable[] = {
> +			"disable", "enable" };
> +	static const char * const power_source[] = {
> +			"3V3", "1V8" };
> +	int ret;
> +
> +	ret = regmap_read(info->regmap_base, offset * 4, &val);
> +	if (ret) {
> +		dev_err(info->pctl_dev->dev,
> +			"failed to read offset 0x%x\n", offset * 4);
> +		return;
> +	}
> +
> +	mode	= (val & K230_PC_SEL) >> K230_SHIFT_SEL;
> +	drive	= (val & K230_PC_DS) >> K230_SHIFT_DS;
> +	bias	= (val & K230_PC_BIAS) >> K230_SHIFT_BIAS;
> +	input	= (val & K230_PC_IE) >> K230_SHIFT_IE;
> +	output	= (val & K230_PC_OE) >> K230_SHIFT_OE;
> +	slew	= (val & K230_PC_SL) >> K230_SHIFT_SL;
> +	schmitt	= (val & K230_PC_ST) >> K230_SHIFT_ST;
> +	power	= (val & K230_PC_MSC) >> K230_SHIFT_MSC;
> +
> +	seq_printf(s, "%s - strength %d - %s - %s - slewrate %s - schmitt %s - %s",
> +		   grp ? grp->name : "unknown",
> +		   drive,
> +		   biasing[bias],
> +		   input ? "input" : "output",
> +		   enable[slew],
> +		   enable[schmitt],
> +		   power_source[power]);
> +}
> +
> +static int k230_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			       struct device_node *np_config,
> +			       struct pinctrl_map **map,
> +			       unsigned int *num_maps)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +	struct device *dev = info->pctl_dev->dev;
> +	const struct k230_pmx_func *func;
> +	const struct k230_pin_group *grp;
> +	struct pinctrl_map *new_map;
> +	int map_num, i, j, idx;
> +	unsigned int grp_id;
> +
> +	func = k230_name_to_funtion(info, np_config->name);
> +	if (!func) {
> +		dev_err(dev, "function %s not found\n", np_config->name);
> +		return -EINVAL;
> +	}
> +
> +	map_num = 0;
> +	for (i = 0; i < func->ngroups; ++i) {
> +		grp_id = func->group_idx[i];
> +		/* npins of config map plus a mux map */
> +		map_num += info->groups[grp_id].num_pins + 1;
> +	}
> +
> +	new_map = kcalloc(map_num, sizeof(*new_map), GFP_KERNEL);
> +	if (!new_map)
> +		return -ENOMEM;
> +	*map = new_map;
> +	*num_maps = map_num;
> +
> +	idx = 0;
> +	for (i = 0; i < func->ngroups; ++i) {
> +		grp_id = func->group_idx[i];
> +		grp = &info->groups[grp_id];
> +		new_map[idx].type = PIN_MAP_TYPE_MUX_GROUP;
> +		new_map[idx].data.mux.group = grp->name;
> +		new_map[idx].data.mux.function = np_config->name;
> +		idx++;
> +
> +		for (j = 0; j < grp->num_pins; ++j) {
> +			new_map[idx].type = PIN_MAP_TYPE_CONFIGS_PIN;
> +			new_map[idx].data.configs.group_or_pin =
> +				pin_get_name(pctldev, grp->pins[j]);
> +			new_map[idx].data.configs.configs =
> +				grp->data[j].configs;
> +			new_map[idx].data.configs.num_configs =
> +				grp->data[j].nconfigs;
> +			idx++;
> +		}
> +	}
> +
> +	return 0;
> +}

...

> +
> +	ret = regmap_write(info->regmap_base, pin * 4, val);
> +	if (ret) {
> +		dev_err(dev, "failed to write offset 0x%x\n", pin * 4);

Isn't regmap an MMIO? If so, drop all of such messages. This just makes
unlikely error paths too big.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k230_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *configs, unsigned int num_configs)
> +{
> +	enum pin_config_param param;
> +	unsigned int arg, i;
> +	int ret;
> +
> +	if (WARN_ON(pin >= K230_NPINS))

Drop WARN_ON. No need to panic kernel. Instead, handle correctly the error.

> +		return -EINVAL;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +		ret = k230_pinconf_set_param(pctldev, pin, param, arg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void k230_pconf_dbg_show(struct pinctrl_dev *pctldev,
> +				struct seq_file *s, unsigned int pin)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap_base, pin * 4, &val);
> +	if (ret) {
> +		dev_err(info->pctl_dev->dev, "failed to read offset 0x%x\n", pin * 4);
> +		return;
> +	}
> +
> +	seq_printf(s, " 0x%08x", val);
> +}
> +
> +static const struct pinconf_ops k230_pinconf_ops = {
> +	.is_generic		= true,
> +	.pin_config_get		= k230_pinconf_get,
> +	.pin_config_set		= k230_pinconf_set,
> +	.pin_config_dbg_show	= k230_pconf_dbg_show,
> +};
> +
> +static int k230_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return info->nfunctions;
> +}
> +
> +static const char *k230_get_fname(struct pinctrl_dev *pctldev,
> +				  unsigned int selector)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return info->functions[selector].name;
> +}
> +
> +static int k230_get_groups(struct pinctrl_dev *pctldev, unsigned int selector,
> +			   const char * const **groups, unsigned int *num_groups)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = info->functions[selector].groups;
> +	*num_groups = info->functions[selector].ngroups;
> +
> +	return 0;
> +}
> +
> +static int k230_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			unsigned int group)
> +{
> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +	const struct k230_pin_conf *data = info->groups[group].data;
> +	struct k230_pin_group *grp = &info->groups[group];
> +	const unsigned int *pins = grp->pins;
> +	struct regmap *regmap;
> +	unsigned int value, mask;
> +	int cnt, reg;
> +
> +	regmap = info->regmap_base;
> +
> +	for (cnt = 0; cnt < grp->num_pins; cnt++) {
> +		reg = pins[cnt] * 4;
> +		value = data[cnt].func << K230_SHIFT_SEL;
> +		mask = K230_PC_SEL;
> +		regmap_update_bits(regmap, reg, mask, value);
> +		k230_pins[pins[cnt]].drv_data = grp;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinmux_ops k230_pmxops = {
> +	.get_functions_count	= k230_get_functions_count,
> +	.get_function_name	= k230_get_fname,
> +	.get_function_groups	= k230_get_groups,
> +	.set_mux		= k230_set_mux,
> +	.strict			= true,
> +};
> +
> +static int k230_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct k230_pinctrl *info;
> +	struct pinctrl_desc *pctl;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	pctl = &info->pctl;
> +
> +	pctl->name	= "k230-pinctrl";
> +	pctl->owner	= THIS_MODULE;
> +	pctl->pins	= k230_pins;
> +	pctl->npins	= ARRAY_SIZE(k230_pins);
> +	pctl->pctlops	= &k230_pctrl_ops;
> +	pctl->pmxops	= &k230_pmxops;
> +	pctl->confops	= &k230_pinconf_ops;
> +
> +	info->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(info->base))
> +		return PTR_ERR(info->base);
> +
> +	k230_regmap_config.name = "canaan,pinctrl";

Why this is not part of definition?

> +	info->regmap_base = devm_regmap_init_mmio(dev, info->base,
> +						  &k230_regmap_config);
> +	if (IS_ERR(info->regmap_base))
> +		return dev_err_probe(dev, PTR_ERR(info->regmap_base),
> +				     "failed to init regmap\n");
> +
> +	info->pctl_dev = devm_pinctrl_register(dev, pctl, info);
> +	if (IS_ERR(info->pctl_dev))
> +		return dev_err_probe(dev, PTR_ERR(info->pctl_dev),
> +				     "devm_pinctrl_register failed\n");
> +
> +	k230_pinctrl_parse_dt(pdev, info);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id k230_dt_ids[] = {
> +	{ .compatible = "canaan,k230-pinctrl", },
> +	{ /* sintenel */ }
> +};
> +MODULE_DEVICE_TABLE(of, k230_dt_ids);
> +
> +static struct platform_driver k230_pinctrl_driver = {
> +	.probe = k230_pinctrl_probe,
> +	.driver = {
> +		.name = "k230-pinctrl",
> +		.of_match_table = k230_dt_ids,
> +	},
> +};
> +module_platform_driver(k230_pinctrl_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ze Huang <18771902331@163.com>");
> +MODULE_DESCRIPTION("Canaan K230 pinctrl driver");

Best regards,
Krzysztof

Re: [RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC

[Ze Huang]

On 9/16/24 11:58 PM, Krzysztof Kozlowski wrote:
> On 16/09/2024 08:47, Ze Huang wrote:
>> Configuration of the K230 is similar to that of the K210. However, in K210,
>> the 256 functions for each pin are shared, whereas in K230, multiplex
>> functions are different for every pin.
>>
>> Signed-off-by: Ze Huang <18771902331@163.com>
>> ---
>>   drivers/pinctrl/Kconfig        |  10 +
>>   drivers/pinctrl/Makefile       |   1 +
>>   drivers/pinctrl/pinctrl-k230.c | 674 +++++++++++++++++++++++++++++++++
>>   3 files changed, 685 insertions(+)
>>   create mode 100644 drivers/pinctrl/pinctrl-k230.c
> ...
>
>> +
>> +struct k230_pinctrl {
>> +	struct pinctrl_desc	pctl;
>> +	struct pinctrl_dev	*pctl_dev;
>> +	struct regmap		*regmap_base;
>> +	void __iomem		*base;
>> +	struct k230_pin_group	*groups;
>> +	unsigned int		ngroups;
>> +	struct k230_pmx_func	*functions;
>> +	unsigned int		nfunctions;
>> +};
>> +
>> +static struct regmap_config k230_regmap_config = {
> Why is this not a const?

Will move definition of 'name' here and set to const.

>
>> +	.reg_bits	= 32,
>> +	.val_bits	= 32,
>> +	.max_register	= 0x100,
>> +	.reg_stride	= 4,
>> +};
>> +
>> +static int k230_get_groups_count(struct pinctrl_dev *pctldev)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	return info->ngroups;
>> +}
>> +
>> +static const char *k230_get_group_name(struct pinctrl_dev *pctldev,
>> +				       unsigned int selector)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	return info->groups[selector].name;
>> +}
>> +
>> +static int k230_get_group_pins(struct pinctrl_dev *pctldev,
>> +			       unsigned int selector,
>> +			       const unsigned int **pins,
>> +			       unsigned int *num_pins)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	if (selector >= info->ngroups)
>> +		return -EINVAL;
>> +
>> +	*pins = info->groups[selector].pins;
>> +	*num_pins = info->groups[selector].num_pins;
>> +
>> +	return 0;
>> +}
>> +
>> +static inline const struct k230_pmx_func *k230_name_to_funtion(
>> +		const struct k230_pinctrl *info, const char *name)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < info->nfunctions; i++) {
>> +		if (!strcmp(info->functions[i].name, name))
>> +			return &info->functions[i];
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int k230_pinctrl_parse_groups(struct device_node *np,
>> +				     struct k230_pin_group *grp,
>> +				     struct k230_pinctrl *info,
>> +				     unsigned int index)
>> +{
>> +	struct device *dev = info->pctl_dev->dev;
>> +	const __be32 *list;
>> +	int size, i, ret;
>> +
>> +	grp->name = np->name;
>> +
>> +	list = of_get_property(np, "pinmux", &size);
>> +	size /= sizeof(*list);
>> +
>> +	grp->num_pins = size;
>> +	grp->pins = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->pins),
>> +				 GFP_KERNEL);
>> +	grp->data = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->data),
>> +				 GFP_KERNEL);
>> +	if (!grp->pins || !grp->data)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < size; i++) {
>> +		unsigned int mux_data = be32_to_cpu(*list++);
>> +
>> +		grp->pins[i] = (mux_data >> 8);
>> +		grp->data[i].func = (mux_data & 0xff);
>> +
>> +		ret = pinconf_generic_parse_dt_config(np, NULL,
>> +						      &grp->data[i].configs,
>> +						      &grp->data[i].nconfigs);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	of_node_put(np);
>> +
> This looks like double free. There is no get in this scope.

Thanks for pointing that out. 'np' would be released by the caller. Will 
remove
`of_node_put()` here.

>> +	return 0;
>> +}
>> +
>> +static void k230_pinctrl_child_count(struct k230_pinctrl *info,
>> +				     struct device_node *np)
>> +{
>> +	struct device_node *child;
>> +
>> +	for_each_child_of_node(np, child) {
>> +		info->nfunctions++;
>> +		info->ngroups += of_get_child_count(child);
>> +	}
>> +}
>> +
>> +static int k230_pinctrl_parse_functions(struct device_node *np,
>> +					struct k230_pinctrl *info,
>> +					unsigned int index)
>> +{
>> +	struct device *dev = info->pctl_dev->dev;
>> +	struct k230_pmx_func *func;
>> +	struct k230_pin_group *grp;
>> +	struct device_node *child;
>> +	static unsigned int idx, i;
>> +	int ret;
>> +
>> +	func = &info->functions[index];
>> +
>> +	func->name = np->name;
>> +	func->ngroups = of_get_child_count(np);
>> +	if (func->ngroups <= 0)
>> +		return 0;
>> +
>> +	func->groups = devm_kcalloc(dev, func->ngroups,
>> +				    sizeof(*func->groups), GFP_KERNEL);
>> +	func->group_idx = devm_kcalloc(dev, func->ngroups,
>> +				       sizeof(*func->group_idx), GFP_KERNEL);
>> +	if (!func->groups || !func->group_idx)
>> +		return -ENOMEM;
>> +
>> +	i = 0;
>> +
>> +	for_each_child_of_node(np, child) {
>> +		func->groups[i] = child->name;
>> +		func->group_idx[i] = idx;
>> +		grp = &info->groups[idx];
>> +		idx++;
>> +		ret = k230_pinctrl_parse_groups(child, grp, info, i++);
>> +		if (ret) {
>> +			of_node_put(child);
> Use scoped loop instead.

OK, will use `for_each_child_of_node_scoped` instead.

>
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int k230_pinctrl_parse_dt(struct platform_device *pdev,
>> +				 struct k230_pinctrl *info)
> Please keep all probe related code next to each other. That's quite
> confusing to find probe code far away from the probe().

Noted, will do.

>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *child;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	k230_pinctrl_child_count(info, np);
>> +
>> +	info->functions = devm_kcalloc(dev, info->nfunctions,
>> +				       sizeof(*info->functions), GFP_KERNEL);
>> +	info->groups = devm_kcalloc(dev, info->ngroups,
>> +				    sizeof(*info->groups), GFP_KERNEL);
>> +	if (!info->functions || !info->groups)
>> +		return -ENOMEM;
>> +
>> +	i = 0;
>> +
>> +	for_each_child_of_node(np, child) {
>> +		ret = k230_pinctrl_parse_functions(child, info, i++);
>> +		if (ret) {
>> +			dev_err(dev, "failed to parse function\n");
>> +			of_node_put(child);
> Use scoped loop instead.

Will use `for_each_child_of_node_scoped` instead.

>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct pinctrl_pin_desc k230_pins[] = {
> Why is this not a const?

Field `drv_data` was used to point to currently activated group, which would
be updated in `set_mux()`. It was used to print the name of the current
function of pin in `k230_pinctrl_pin_dbg_show()`.

>> +	PINCTRL_PIN(0,  "IO0"),  PINCTRL_PIN(1,  "IO1"),  PINCTRL_PIN(2,  "IO2"),
>> +	PINCTRL_PIN(3,  "IO3"),  PINCTRL_PIN(4,  "IO4"),  PINCTRL_PIN(5,  "IO5"),
>> +	PINCTRL_PIN(6,  "IO6"),  PINCTRL_PIN(7,  "IO7"),  PINCTRL_PIN(8,  "IO8"),
>> +	PINCTRL_PIN(9,  "IO9"),  PINCTRL_PIN(10, "IO10"), PINCTRL_PIN(11, "IO11"),
>> +	PINCTRL_PIN(12, "IO12"), PINCTRL_PIN(13, "IO13"), PINCTRL_PIN(14, "IO14"),
>> +	PINCTRL_PIN(15, "IO15"), PINCTRL_PIN(16, "IO16"), PINCTRL_PIN(17, "IO17"),
>> +	PINCTRL_PIN(18, "IO18"), PINCTRL_PIN(19, "IO19"), PINCTRL_PIN(20, "IO20"),
>> +	PINCTRL_PIN(21, "IO21"), PINCTRL_PIN(22, "IO22"), PINCTRL_PIN(23, "IO23"),
>> +	PINCTRL_PIN(24, "IO24"), PINCTRL_PIN(25, "IO25"), PINCTRL_PIN(26, "IO26"),
>> +	PINCTRL_PIN(27, "IO27"), PINCTRL_PIN(28, "IO28"), PINCTRL_PIN(29, "IO29"),
>> +	PINCTRL_PIN(30, "IO30"), PINCTRL_PIN(31, "IO31"), PINCTRL_PIN(32, "IO32"),
>> +	PINCTRL_PIN(33, "IO33"), PINCTRL_PIN(34, "IO34"), PINCTRL_PIN(35, "IO35"),
>> +	PINCTRL_PIN(36, "IO36"), PINCTRL_PIN(37, "IO37"), PINCTRL_PIN(38, "IO38"),
>> +	PINCTRL_PIN(39, "IO39"), PINCTRL_PIN(40, "IO40"), PINCTRL_PIN(41, "IO41"),
>> +	PINCTRL_PIN(42, "IO42"), PINCTRL_PIN(43, "IO43"), PINCTRL_PIN(44, "IO44"),
>> +	PINCTRL_PIN(45, "IO45"), PINCTRL_PIN(46, "IO46"), PINCTRL_PIN(47, "IO47"),
>> +	PINCTRL_PIN(48, "IO48"), PINCTRL_PIN(49, "IO49"), PINCTRL_PIN(50, "IO50"),
>> +	PINCTRL_PIN(51, "IO51"), PINCTRL_PIN(52, "IO52"), PINCTRL_PIN(53, "IO53"),
>> +	PINCTRL_PIN(54, "IO54"), PINCTRL_PIN(55, "IO55"), PINCTRL_PIN(56, "IO56"),
>> +	PINCTRL_PIN(57, "IO57"), PINCTRL_PIN(58, "IO58"), PINCTRL_PIN(59, "IO59"),
>> +	PINCTRL_PIN(60, "IO60"), PINCTRL_PIN(61, "IO61"), PINCTRL_PIN(62, "IO62"),
>> +	PINCTRL_PIN(63, "IO63")
>> +};
>> +
>> +static void k230_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
>> +				      struct seq_file *s, unsigned int offset)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +	u32 val, mode, bias, drive, input, output, slew, schmitt, power;
>> +	struct k230_pin_group *grp = k230_pins[offset].drv_data;
>> +	static const char * const biasing[] = {
>> +			"pull none", "pull down", "pull up", "" };
>> +	static const char * const enable[] = {
>> +			"disable", "enable" };
>> +	static const char * const power_source[] = {
>> +			"3V3", "1V8" };
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap_base, offset * 4, &val);
>> +	if (ret) {
>> +		dev_err(info->pctl_dev->dev,
>> +			"failed to read offset 0x%x\n", offset * 4);
>> +		return;
>> +	}
>> +
>> +	mode	= (val & K230_PC_SEL) >> K230_SHIFT_SEL;
>> +	drive	= (val & K230_PC_DS) >> K230_SHIFT_DS;
>> +	bias	= (val & K230_PC_BIAS) >> K230_SHIFT_BIAS;
>> +	input	= (val & K230_PC_IE) >> K230_SHIFT_IE;
>> +	output	= (val & K230_PC_OE) >> K230_SHIFT_OE;
>> +	slew	= (val & K230_PC_SL) >> K230_SHIFT_SL;
>> +	schmitt	= (val & K230_PC_ST) >> K230_SHIFT_ST;
>> +	power	= (val & K230_PC_MSC) >> K230_SHIFT_MSC;
>> +
>> +	seq_printf(s, "%s - strength %d - %s - %s - slewrate %s - schmitt %s - %s",
>> +		   grp ? grp->name : "unknown",
>> +		   drive,
>> +		   biasing[bias],
>> +		   input ? "input" : "output",
>> +		   enable[slew],
>> +		   enable[schmitt],
>> +		   power_source[power]);
>> +}
>> +
>> +static int k230_dt_node_to_map(struct pinctrl_dev *pctldev,
>> +			       struct device_node *np_config,
>> +			       struct pinctrl_map **map,
>> +			       unsigned int *num_maps)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +	struct device *dev = info->pctl_dev->dev;
>> +	const struct k230_pmx_func *func;
>> +	const struct k230_pin_group *grp;
>> +	struct pinctrl_map *new_map;
>> +	int map_num, i, j, idx;
>> +	unsigned int grp_id;
>> +
>> +	func = k230_name_to_funtion(info, np_config->name);
>> +	if (!func) {
>> +		dev_err(dev, "function %s not found\n", np_config->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	map_num = 0;
>> +	for (i = 0; i < func->ngroups; ++i) {
>> +		grp_id = func->group_idx[i];
>> +		/* npins of config map plus a mux map */
>> +		map_num += info->groups[grp_id].num_pins + 1;
>> +	}
>> +
>> +	new_map = kcalloc(map_num, sizeof(*new_map), GFP_KERNEL);
>> +	if (!new_map)
>> +		return -ENOMEM;
>> +	*map = new_map;
>> +	*num_maps = map_num;
>> +
>> +	idx = 0;
>> +	for (i = 0; i < func->ngroups; ++i) {
>> +		grp_id = func->group_idx[i];
>> +		grp = &info->groups[grp_id];
>> +		new_map[idx].type = PIN_MAP_TYPE_MUX_GROUP;
>> +		new_map[idx].data.mux.group = grp->name;
>> +		new_map[idx].data.mux.function = np_config->name;
>> +		idx++;
>> +
>> +		for (j = 0; j < grp->num_pins; ++j) {
>> +			new_map[idx].type = PIN_MAP_TYPE_CONFIGS_PIN;
>> +			new_map[idx].data.configs.group_or_pin =
>> +				pin_get_name(pctldev, grp->pins[j]);
>> +			new_map[idx].data.configs.configs =
>> +				grp->data[j].configs;
>> +			new_map[idx].data.configs.num_configs =
>> +				grp->data[j].nconfigs;
>> +			idx++;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> ...
>
>> +
>> +	ret = regmap_write(info->regmap_base, pin * 4, val);
>> +	if (ret) {
>> +		dev_err(dev, "failed to write offset 0x%x\n", pin * 4);
> Isn't regmap an MMIO? If so, drop all of such messages. This just makes
> unlikely error paths too big.

Acknowledged. Will drop them.

>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int k230_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>> +			    unsigned long *configs, unsigned int num_configs)
>> +{
>> +	enum pin_config_param param;
>> +	unsigned int arg, i;
>> +	int ret;
>> +
>> +	if (WARN_ON(pin >= K230_NPINS))
> Drop WARN_ON. No need to panic kernel. Instead, handle correctly the error.

Acknowledged. Will use dev_err instead.

>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < num_configs; i++) {
>> +		param = pinconf_to_config_param(configs[i]);
>> +		arg = pinconf_to_config_argument(configs[i]);
>> +		ret = k230_pinconf_set_param(pctldev, pin, param, arg);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void k230_pconf_dbg_show(struct pinctrl_dev *pctldev,
>> +				struct seq_file *s, unsigned int pin)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap_base, pin * 4, &val);
>> +	if (ret) {
>> +		dev_err(info->pctl_dev->dev, "failed to read offset 0x%x\n", pin * 4);
>> +		return;
>> +	}
>> +
>> +	seq_printf(s, " 0x%08x", val);
>> +}
>> +
>> +static const struct pinconf_ops k230_pinconf_ops = {
>> +	.is_generic		= true,
>> +	.pin_config_get		= k230_pinconf_get,
>> +	.pin_config_set		= k230_pinconf_set,
>> +	.pin_config_dbg_show	= k230_pconf_dbg_show,
>> +};
>> +
>> +static int k230_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	return info->nfunctions;
>> +}
>> +
>> +static const char *k230_get_fname(struct pinctrl_dev *pctldev,
>> +				  unsigned int selector)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	return info->functions[selector].name;
>> +}
>> +
>> +static int k230_get_groups(struct pinctrl_dev *pctldev, unsigned int selector,
>> +			   const char * const **groups, unsigned int *num_groups)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	*groups = info->functions[selector].groups;
>> +	*num_groups = info->functions[selector].ngroups;
>> +
>> +	return 0;
>> +}
>> +
>> +static int k230_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
>> +			unsigned int group)
>> +{
>> +	struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>> +	const struct k230_pin_conf *data = info->groups[group].data;
>> +	struct k230_pin_group *grp = &info->groups[group];
>> +	const unsigned int *pins = grp->pins;
>> +	struct regmap *regmap;
>> +	unsigned int value, mask;
>> +	int cnt, reg;
>> +
>> +	regmap = info->regmap_base;
>> +
>> +	for (cnt = 0; cnt < grp->num_pins; cnt++) {
>> +		reg = pins[cnt] * 4;
>> +		value = data[cnt].func << K230_SHIFT_SEL;
>> +		mask = K230_PC_SEL;
>> +		regmap_update_bits(regmap, reg, mask, value);
>> +		k230_pins[pins[cnt]].drv_data = grp;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pinmux_ops k230_pmxops = {
>> +	.get_functions_count	= k230_get_functions_count,
>> +	.get_function_name	= k230_get_fname,
>> +	.get_function_groups	= k230_get_groups,
>> +	.set_mux		= k230_set_mux,
>> +	.strict			= true,
>> +};
>> +
>> +static int k230_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct k230_pinctrl *info;
>> +	struct pinctrl_desc *pctl;
>> +
>> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	pctl = &info->pctl;
>> +
>> +	pctl->name	= "k230-pinctrl";
>> +	pctl->owner	= THIS_MODULE;
>> +	pctl->pins	= k230_pins;
>> +	pctl->npins	= ARRAY_SIZE(k230_pins);
>> +	pctl->pctlops	= &k230_pctrl_ops;
>> +	pctl->pmxops	= &k230_pmxops;
>> +	pctl->confops	= &k230_pinconf_ops;
>> +
>> +	info->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(info->base))
>> +		return PTR_ERR(info->base);
>> +
>> +	k230_regmap_config.name = "canaan,pinctrl";
> Why this is not part of definition?

Will move to definition and set regmap to const.

>> +	info->regmap_base = devm_regmap_init_mmio(dev, info->base,
>> +						  &k230_regmap_config);
>> +	if (IS_ERR(info->regmap_base))
>> +		return dev_err_probe(dev, PTR_ERR(info->regmap_base),
>> +				     "failed to init regmap\n");
>> +
>> +	info->pctl_dev = devm_pinctrl_register(dev, pctl, info);
>> +	if (IS_ERR(info->pctl_dev))
>> +		return dev_err_probe(dev, PTR_ERR(info->pctl_dev),
>> +				     "devm_pinctrl_register failed\n");
>> +
>> +	k230_pinctrl_parse_dt(pdev, info);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id k230_dt_ids[] = {
>> +	{ .compatible = "canaan,k230-pinctrl", },
>> +	{ /* sintenel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, k230_dt_ids);
>> +
>> +static struct platform_driver k230_pinctrl_driver = {
>> +	.probe = k230_pinctrl_probe,
>> +	.driver = {
>> +		.name = "k230-pinctrl",
>> +		.of_match_table = k230_dt_ids,
>> +	},
>> +};
>> +module_platform_driver(k230_pinctrl_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Ze Huang <18771902331@163.com>");
>> +MODULE_DESCRIPTION("Canaan K230 pinctrl driver");
> Best regards,
> Krzysztof

Re: [RESEND PATCH 3/3] riscv: dts: canaan: Add k230's pinctrl node

[Ze Huang]

On 9/16/24 11:52 PM, Krzysztof Kozlowski wrote:
> On 16/09/2024 08:47, Ze Huang wrote:
>> Add pinctrl device, containing default config for uart, pwm, iis, iic and
>> mmc.
>>
>> Signed-off-by: Ze Huang <18771902331@163.com>
>> ---
>>   arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi | 316 +++++++++++++++++++
>>   arch/riscv/boot/dts/canaan/k230-pinctrl.h    |  18 ++
>>   arch/riscv/boot/dts/canaan/k230.dtsi         |   2 +
>>   3 files changed, 336 insertions(+)
>>   create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>>   create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h
>>
>> diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>> new file mode 100644
>> index 000000000000..0737f50d2868
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>> @@ -0,0 +1,316 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright (C) 2024 Ze Huang <18771902331@163.com>
>> + */
>> +#include "k230-pinctrl.h"
>> +
>> +/ {
>> +	soc {
>> +		pinctrl: pinctrl@91105000 {
> That's odd style - defining SoC nodes outside of SoC DTSI. Are you sure
> that's preferred coding style in RISC-V or Canaan?

Pinctrl-related nodes were separated the for ease of maintenance, but the
convention in Canaan is to place them in the board-level DTS file. Would it
be better to stay consistent with their approach?

>> +			compatible = "canaan,k230-pinctrl";
>> +			reg = <0x0 0x91105000 0x0 0x100>;
>> +
> Best regards,
> Krzysztof

Re: [RESEND PATCH 1/3] dt-bindings: pinctrl: Add support for canaan,k230 SoC

[Rob Herring]

On Mon, Sep 16, 2024 at 02:42:23PM +0800, Ze Huang wrote:
> Add device tree binding details for Canaan K230 pinctrl device.
> 
> Signed-off-by: Ze Huang <18771902331@163.com>
> ---
>  .../bindings/pinctrl/canaan,k230-pinctrl.yaml | 128 ++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
> new file mode 100644
> index 000000000000..979c5bd71e3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/canaan,k230-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Canaan Kendryte K230 Pin Controller
> +
> +maintainers:
> +  - Ze Huang <18771902331@163.com>
> +
> +description:
> +  The Canaan Kendryte K230 platform includes 64 IO pins, each capable of
> +  multiplexing up to 5 different functions. Pin function configuration is
> +  performed on a per-pin basis.
> +
> +properties:
> +  compatible:
> +    const: canaan,k230-pinctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +    description:
> +      A pinctrl node should contain at least one subnode representing the
> +      pinctrl groups available on the machine.
> +
> +    patternProperties:
> +      '-cfg$':
> +        type: object
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +        additionalProperties: false
> +        description:
> +          Each subnode will list the pins it needs, and how they should
> +          be configured, with regard to muxer configuration, bias, input
> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
> +          slew-rate, drive strength.
> +
> +        properties:
> +          pinmux:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array

Drop. You need to add a $ref to pinmux-node.yaml (alongside 
pincfg-node.yaml).

> +            description:
> +              The list of GPIOs and their mux settings that properties in
> +              the node apply to. This should be set with the macro
> +              'K230_PINMUX(pin, mode)'
> +
> +          bias-disable: true
> +
> +          bias-pull-up: true
> +
> +          bias-pull-down: true
> +
> +          drive-strength:
> +            minimum: 0
> +            maximum: 15
> +
> +          input-enable: true
> +
> +          output-enable: true
> +
> +          input-schmitt-enable: true
> +
> +          slew-rate:
> +            $ref: /schemas/types.yaml#/definitions/uint32

Drop. Already has a defined type.

> +            description: |
> +              slew rate control enable
> +              0: disable
> +              1: enable
> +
> +            enum: [0, 1]
> +
> +          power-source:
> +            $ref: /schemas/types.yaml#/definitions/uint32

Drop. Already has a defined type.

> +            description: |
> +              Specifies the power source voltage for the IO bank that the
> +              pin belongs to. Each bank of IO pins operate at a specific,
> +              fixed voltage levels. Incorrect voltage configuration can
> +              damage the chip. The defined constants represent the
> +              possible voltage configurations:
> +
> +              - K230_MSC_3V3 (value 0): 3.3V power supply
> +              - K230_MSC_1V8 (value 1): 1.8V power supply
> +
> +              The following banks have the corresponding voltage
> +              configurations:
> +
> +              - bank IO0 to IO1: Fixed at 1.8V
> +              - bank IO2 to IO13: Fixed at 1.8V
> +              - bank IO14 to IO25: Fixed at 1.8V
> +              - bank IO26 to IO37: Fixed at 1.8V
> +              - bank IO38 to IO49: Fixed at 1.8V
> +              - bank IO50 to IO61: Fixed at 3.3V
> +              - bank IO62 to IO63: Fixed at 1.8V
> +
> +            enum: [0, 1]
> +
> +        required:
> +          - pinmux
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pinctrl: pinctrl@91105000 {

Drop unused labels.

> +        compatible = "canaan,k230-pinctrl";
> +        reg = <0x91105000 0x100>;
> +
> +        uart2_pins: uart2-pins {
> +            uart2-pins-cfg {
> +                pinmux = <0x503>, /* uart2 txd */
> +                         <0x603>; /* uart2 rxd */
> +                slew-rate = <0>;
> +                drive-strength = <4>;
> +                power-source = <1>;
> +                input-enable;
> +                output-enable;
> +                bias-disable;
> +            };
> +        };
> +    };
> -- 
> 2.46.1
> 

Re: [RESEND PATCH 3/3] riscv: dts: canaan: Add k230's pinctrl node

[Conor Dooley]

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

On Wed, Sep 18, 2024 at 04:39:29PM +0800, Ze Huang wrote:
> On 9/16/24 11:52 PM, Krzysztof Kozlowski wrote:
> > On 16/09/2024 08:47, Ze Huang wrote:
> > > Add pinctrl device, containing default config for uart, pwm, iis, iic and
> > > mmc.
> > > 
> > > Signed-off-by: Ze Huang <18771902331@163.com>
> > > ---
> > >   arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi | 316 +++++++++++++++++++
> > >   arch/riscv/boot/dts/canaan/k230-pinctrl.h    |  18 ++
> > >   arch/riscv/boot/dts/canaan/k230.dtsi         |   2 +
> > >   3 files changed, 336 insertions(+)
> > >   create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
> > >   create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h
> > > 
> > > diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
> > > new file mode 100644
> > > index 000000000000..0737f50d2868
> > > --- /dev/null
> > > +++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
> > > @@ -0,0 +1,316 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > > +/*
> > > + * Copyright (C) 2024 Ze Huang <18771902331@163.com>
> > > + */
> > > +#include "k230-pinctrl.h"
> > > +
> > > +/ {
> > > +	soc {
> > > +		pinctrl: pinctrl@91105000 {
> > That's odd style - defining SoC nodes outside of SoC DTSI. Are you sure
> > that's preferred coding style in RISC-V or Canaan?
> 
> Pinctrl-related nodes were separated the for ease of maintenance, but the
> convention in Canaan is to place them in the board-level DTS file. Would it
> be better to stay consistent with their approach?

Yeah, please put them in the board-level file.

Thanks,
Conor.

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

Re: [RESEND PATCH 3/3] riscv: dts: canaan: Add k230's pinctrl node

[Ze Huang]

On 9/23/24 5:50 PM, Conor Dooley wrote:
> On Wed, Sep 18, 2024 at 04:39:29PM +0800, Ze Huang wrote:
>> On 9/16/24 11:52 PM, Krzysztof Kozlowski wrote:
>>> On 16/09/2024 08:47, Ze Huang wrote:
>>>> Add pinctrl device, containing default config for uart, pwm, iis, iic and
>>>> mmc.
>>>>
>>>> Signed-off-by: Ze Huang <18771902331@163.com>
>>>> ---
>>>>    arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi | 316 +++++++++++++++++++
>>>>    arch/riscv/boot/dts/canaan/k230-pinctrl.h    |  18 ++
>>>>    arch/riscv/boot/dts/canaan/k230.dtsi         |   2 +
>>>>    3 files changed, 336 insertions(+)
>>>>    create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>>>>    create mode 100644 arch/riscv/boot/dts/canaan/k230-pinctrl.h
>>>>
>>>> diff --git a/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>>>> new file mode 100644
>>>> index 000000000000..0737f50d2868
>>>> --- /dev/null
>>>> +++ b/arch/riscv/boot/dts/canaan/k230-pinctrl.dtsi
>>>> @@ -0,0 +1,316 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>> +/*
>>>> + * Copyright (C) 2024 Ze Huang <18771902331@163.com>
>>>> + */
>>>> +#include "k230-pinctrl.h"
>>>> +
>>>> +/ {
>>>> +	soc {
>>>> +		pinctrl: pinctrl@91105000 {
>>> That's odd style - defining SoC nodes outside of SoC DTSI. Are you sure
>>> that's preferred coding style in RISC-V or Canaan?
>> Pinctrl-related nodes were separated the for ease of maintenance, but the
>> convention in Canaan is to place them in the board-level DTS file. Would it
>> be better to stay consistent with their approach?
> Yeah, please put them in the board-level file.

OK

>
> Thanks,
> Conor.

Re: [RESEND PATCH 1/3] dt-bindings: pinctrl: Add support for canaan,k230 SoC

[Ze Huang]

On 9/19/24 1:13 AM, Rob Herring wrote:
> On Mon, Sep 16, 2024 at 02:42:23PM +0800, Ze Huang wrote:
>> Add device tree binding details for Canaan K230 pinctrl device.
>>
>> Signed-off-by: Ze Huang <18771902331@163.com>
>> ---
>>   .../bindings/pinctrl/canaan,k230-pinctrl.yaml | 128 ++++++++++++++++++
>>   1 file changed, 128 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..979c5bd71e3d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/canaan,k230-pinctrl.yaml
>> @@ -0,0 +1,128 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/canaan,k230-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Canaan Kendryte K230 Pin Controller
>> +
>> +maintainers:
>> +  - Ze Huang <18771902331@163.com>
>> +
>> +description:
>> +  The Canaan Kendryte K230 platform includes 64 IO pins, each capable of
>> +  multiplexing up to 5 different functions. Pin function configuration is
>> +  performed on a per-pin basis.
>> +
>> +properties:
>> +  compatible:
>> +    const: canaan,k230-pinctrl
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    additionalProperties: false
>> +    description:
>> +      A pinctrl node should contain at least one subnode representing the
>> +      pinctrl groups available on the machine.
>> +
>> +    patternProperties:
>> +      '-cfg$':
>> +        type: object
>> +        $ref: /schemas/pinctrl/pincfg-node.yaml
>> +        additionalProperties: false
>> +        description:
>> +          Each subnode will list the pins it needs, and how they should
>> +          be configured, with regard to muxer configuration, bias, input
>> +          enable/disable, input schmitt trigger, slew-rate enable/disable,
>> +          slew-rate, drive strength.
>> +
>> +        properties:
>> +          pinmux:
>> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> Drop. You need to add a $ref to pinmux-node.yaml (alongside
> pincfg-node.yaml).

OK. will use a allOf like this:

patternProperties:
   '-cfg$':
   type: object
   allOf:
     - $ref: /schemas/pinctrl/pincfg-node.yaml#
     - $ref: /schemas/pinctrl/pinmux-node.yaml#

>
>> +            description:
>> +              The list of GPIOs and their mux settings that properties in
>> +              the node apply to. This should be set with the macro
>> +              'K230_PINMUX(pin, mode)'
>> +
>> +          bias-disable: true
>> +
>> +          bias-pull-up: true
>> +
>> +          bias-pull-down: true
>> +
>> +          drive-strength:
>> +            minimum: 0
>> +            maximum: 15
>> +
>> +          input-enable: true
>> +
>> +          output-enable: true
>> +
>> +          input-schmitt-enable: true
>> +
>> +          slew-rate:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
> Drop. Already has a defined type.

Acknowledged.

>
>> +            description: |
>> +              slew rate control enable
>> +              0: disable
>> +              1: enable
>> +
>> +            enum: [0, 1]
>> +
>> +          power-source:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
> Drop. Already has a defined type.

Acknowledged.

>
>> +            description: |
>> +              Specifies the power source voltage for the IO bank that the
>> +              pin belongs to. Each bank of IO pins operate at a specific,
>> +              fixed voltage levels. Incorrect voltage configuration can
>> +              damage the chip. The defined constants represent the
>> +              possible voltage configurations:
>> +
>> +              - K230_MSC_3V3 (value 0): 3.3V power supply
>> +              - K230_MSC_1V8 (value 1): 1.8V power supply
>> +
>> +              The following banks have the corresponding voltage
>> +              configurations:
>> +
>> +              - bank IO0 to IO1: Fixed at 1.8V
>> +              - bank IO2 to IO13: Fixed at 1.8V
>> +              - bank IO14 to IO25: Fixed at 1.8V
>> +              - bank IO26 to IO37: Fixed at 1.8V
>> +              - bank IO38 to IO49: Fixed at 1.8V
>> +              - bank IO50 to IO61: Fixed at 3.3V
>> +              - bank IO62 to IO63: Fixed at 1.8V
>> +
>> +            enum: [0, 1]
>> +
>> +        required:
>> +          - pinmux
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    pinctrl: pinctrl@91105000 {
> Drop unused labels.

Will do.

>
>> +        compatible = "canaan,k230-pinctrl";
>> +        reg = <0x91105000 0x100>;
>> +
>> +        uart2_pins: uart2-pins {
>> +            uart2-pins-cfg {
>> +                pinmux = <0x503>, /* uart2 txd */
>> +                         <0x603>; /* uart2 rxd */
>> +                slew-rate = <0>;
>> +                drive-strength = <4>;
>> +                power-source = <1>;
>> +                input-enable;
>> +                output-enable;
>> +                bias-disable;
>> +            };
>> +        };
>> +    };
>> -- 
>> 2.46.1
>>