[RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey Linus,

Got the other driver that I was talking about here for you...
It's in RFC state because I'd like to get some feedback on the approach
while I try to figure out a) what ibufmd is and b) how the bank voltage
interacts with the schmitt trigger setting. Although, I am pretty sure
for the latter that it is not forced on for low voltages and that the
commented code should be deleted.

There's some specific @Linus questions in the driver, mostly where I was
unsure about how config bits should be handled and looking around at
other drivers wasn't sufficient because they did different things.

Finally, on the dt side, this was using the pinmux property before the
other drivers were submitted, but I didn't really like it to begin with
(shoving two things into entries of the same property gives me the ick).
I moved to using pins + function which at least looks prettier in the
devicetree. I had been hoping that I could move to some sort of generic
dt_node_to_map function, but I couldn't figure out one that'd work
without creating groups in the driver. If there is, I'd love to get rid
of the custom dt_node_to_map stuff.
I want to avoid doing having set groups, because of the number of
possible configurations in the MSS Configurator FPGA tool is fairly
large, and I believe that MSS Configurator actually undersells the
number of possible combinations for ease of use. I haven't tested that
and the driver technically doesn't support it, but I feel like not trying
to define groups statically and using pins instead would permit those
combos in the future should that use case ever materialise.

Cheers,
Conor.

CC: Linus Walleij <linus.walleij@linaro.org>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: linux-gpio@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: Valentina.FernandezAlanis@microchip.com

Conor Dooley (4):
  dt-bindings: pinctrl: document polarfire soc mssio pin controller
  pinctrl: add polarfire soc mssio pinctrl driver
  MAINTAINERS: add Microchip mpfs mssio driver/bindings to entry
  riscv: dts: microchip: add pinctrl nodes for mpfs/icicle kit

 .../pinctrl/microchip,mpfs-pinctrl-mssio.yaml | 108 +++
 .../microchip,mpfs-mss-top-sysreg.yaml        |   4 +
 MAINTAINERS                                   |   2 +
 .../dts/microchip/mpfs-icicle-kit-common.dtsi |   1 -
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  63 ++
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 165 ++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  16 +
 drivers/pinctrl/Kconfig                       |   5 +-
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-mpfs-mssio.c          | 798 ++++++++++++++++++
 10 files changed, 1161 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-mssio.c

-- 
2.51.0

[RFC v1 1/4] dt-bindings: pinctrl: document polarfire soc mssio pin controller

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On Polarfire SoC, the Bank 2 and Bank 4 IOs connected to the
Multiprocessor Subsystem (MSS) are controlled by IOMUX_CRs 1 through 6,
which determine what function in routed to them, and
MSSIO_BANK#_IO_CFG_CRs, which determine the configuration of each pin.

Document it, including several custom configuration options that stem
from MSS Configurator options (the MSS Configurator is part of the FPGA
tooling for this device). "ibufmd" unfortunately is not a 1:1 mapping
with an MSS Configurator option, unlike clamp-diode or lockdown, and I
do not know the effect of any bits in the field. I have no been able to
find an explanation for these bits in documentation.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../pinctrl/microchip,mpfs-pinctrl-mssio.yaml | 108 ++++++++++++++++++
 .../microchip,mpfs-mss-top-sysreg.yaml        |   4 +
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
new file mode 100644
index 000000000000..32d7a31d669f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,mpfs-pinctrl-mssio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Polarfire SoC MSSIO pinctrl
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+properties:
+  compatible:
+    const: microchip,mpfs-pinctrl-mssio
+
+  reg:
+    maxItems: 1
+
+  pinctrl-use-default: true
+
+patternProperties:
+  '-cfg$':
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '-pins$':
+        type: object
+        additionalProperties: false
+
+        allOf:
+          - $ref: pincfg-node.yaml#
+          - $ref: pinmux-node.yaml#
+
+        properties:
+          pins:
+            description: |
+              The list of IOs that properties in the pincfg node apply to.
+
+          function:
+            description:
+              A string containing the name of the function to mux for these
+              pins. The "reserved" function tristates a pin.
+            enum: [ sd, emmc, qspi, spi, usb, uart, i2c, can, mdio, misc
+                    reserved, gpio, fabric-test, tied-low, tied-high, tristate ]
+
+          bias-bus-hold: true
+          bias-disable: true
+          bias-pull-down: true
+          bias-pull-up: true
+          input-schmitt-enable: true
+          low-power-enable: true
+
+          drive-strength:
+            enum: [ 2, 4, 6, 8, 10, 12, 16, 20 ]
+
+          microchip,clamp-diode:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description: |
+              Reflects the "Clamp Diode" setting in the MSS Configurator for
+              this pin.
+
+          microchip,bank-lockdown:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description: |
+              Reflects the "Lock Down Bank{2,4} I/Os" setting in the MSS
+              Configurator for this pin.
+
+          microchip,ibufmd:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            default: 0
+            description: |
+              Reflects the "IBUFMD" bits in the MSS Configurator output files
+              for this pin.
+
+        required:
+          - pins
+          - function
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pinctrl@204 {
+      compatible = "microchip,mpfs-pinctrl-mssio";
+      reg = <0x204 0x7c>;
+
+      irkd_sd_cfg: irkd-sd-cfg {
+        sd-10ma-pins {
+          pins = <0>, <1>, <2>, <3>, <4>, <5>, <8>, <9>, <10>, <11>, <12>, <13>;
+          function = "sd";
+          bias-pull-up;
+          drive-strength = <10>;
+        };
+
+        sd-8ma-pins {
+          pins = <6>, <7>;
+          function = "sd";
+          bias-pull-up;
+          drive-strength = <8>;
+        };
+      };
+    };
+...
diff --git a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
index 39987f722411..44e4a50c3155 100644
--- a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
+++ b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
@@ -42,6 +42,10 @@ properties:
     type: object
     $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
 
+  pinctrl@204:
+    type: object
+    $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
+
 required:
   - compatible
   - reg
-- 
2.51.0

[RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On Polarfire SoC, the Bank 2 and Bank 4 IOs connected to the
Multiprocessor Subsystem (MSS) are controlled by IOMUX_CRs 1 through 6,
which determine what function in routed to them, and
MSSIO_BANK#_IO_CFG_CRs, which determine the configuration of each pin.

Add a driver for this pin controller, including several custom
properties that reflect aspects of the MSS's configuration.

Reuse the Kconfig option for iomux0, since controlling MSSIOs without
iomux0 routing a function to the MSSIOs in question is pointless, and
routing a function to the MSSIOs is equally unhelpful if none of them
are configured to make use of that function.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/Kconfig              |   5 +-
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-mpfs-mssio.c | 798 +++++++++++++++++++++++++++
 3 files changed, 803 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-mssio.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 4ec2bb7f67cf..437616e5a6d5 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -506,9 +506,12 @@ config PINCTRL_PISTACHIO
 	  This support pinctrl and GPIO driver for IMG Pistachio SoC.
 
 config PINCTRL_POLARFIRE_SOC
-	bool "Polarfire SoC pinctrl driver"
+	bool "Polarfire SoC pinctrl drivers"
 	depends on ARCH_MICROCHIP || COMPILE_TEST
+	select PINMUX
 	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
 	default y
 	help
 	  This selects the pinctrl driver for Microchip Polarfire SoC.
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ea4e890766e1..bf181654fe7f 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_PINCTRL_PEF2256)	+= pinctrl-pef2256.o
 obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PIC64GX)	+= pinctrl-pic64gx-gpio2.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
+obj-$(CONFIG_PINCTRL_POLARFIRE_SOC)	+= pinctrl-mpfs-mssio.o
 obj-$(CONFIG_PINCTRL_POLARFIRE_SOC)	+= pinctrl-mpfs-iomux0.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
diff --git a/drivers/pinctrl/pinctrl-mpfs-mssio.c b/drivers/pinctrl/pinctrl-mpfs-mssio.c
new file mode 100644
index 000000000000..e1b963350016
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mpfs-mssio.c
@@ -0,0 +1,798 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "linux/dev_printk.h"
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define MPFS_PINCTRL_PAD_MUX_MASK GENMASK(3, 0)
+
+#define MPFS_PINCTRL_IOCFG_MASK GENMASK(14, 0)
+#define MPFS_PINCTRL_IBUFMD_MASK GENMASK(2, 0)
+#define MPFS_PINCTRL_DRV_MASK GENMASK(6, 3)
+#define MPFS_PINCTRL_CLAMP BIT(7)
+#define MPFS_PINCTRL_ENHYST BIT(8)
+#define MPFS_PINCTRL_LOCKDN BIT(9)
+#define MPFS_PINCTRL_WPD BIT(10)
+#define MPFS_PINCTRL_WPU BIT(11)
+#define MPFS_PINCTRL_PULL_MASK GENMASK(11, 10)
+#define MPFS_PINCTRL_LP_PERSIST_EN BIT(12)
+#define MPFS_PINCTRL_LP_BYPASS_EN BIT(13)
+
+#define MPFS_PINCTRL_LVCMOS25 0x6
+#define MPFS_PINCTRL_BANK_VOLTAGE_MASK GENMASK(19, 16)
+
+#define MPFS_PINCTRL_BANK2_CFG_CR 0x1c4
+#define MPFS_PINCTRL_BANK4_CFG_CR 0x1c8
+#define MPFS_PINCTRL_IOCFG01_REG 0x234
+
+#define MPFS_PINCTRL_INTER_BANK_GAP 0x4
+
+#define MPFS_PINCTRL_BANK2_START 14
+
+#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
+#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
+#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)
+
+struct mpfs_pinctrl_mux_config {
+	u8 pin;
+	u8 function;
+};
+
+struct mpfs_pinctrl {
+	struct pinctrl_dev *pctrl;
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct pinctrl_desc desc;
+	u32 bank2_voltage;
+	u32 bank4_voltage;
+};
+
+struct mpfs_pinctrl_drive_strength {
+	u8 ma;
+	u8 val;
+};
+
+static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
+	{ 2,   2 },
+	{ 4,   3 },
+	{ 6,   4 },
+	{ 8,   5 },
+	{ 10,  6 },
+	{ 12,  7 },
+	{ 16, 10 },
+	{ 20, 12 },
+};
+
+static char *mpfs_pinctrl_function_names[] = {
+	"sd",
+	"emmc",
+	"qspi",
+	"spi",
+	"usb",
+	"uart",
+	"i2c",
+	"can",
+	"mdio",
+	"misc",
+	"reserved",
+	"gpio",
+	"fabric test",
+	"tied-low",
+	"tied-high",
+	"tristate"
+};
+
+static int mpfs_pinctrl_function_map(const char *function)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_function_names);
+
+	for (int i = 0; i < num; i++)
+		if (!strcmp(function, mpfs_pinctrl_function_names[i]))
+			return i;
+
+	return -EINVAL;
+}
+
+static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
+	{ "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
+	{ "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
+	{ "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
+};
+
+static int mpfs_pinctrl_pin_to_iomux_offset(unsigned int pin)
+{
+	int offset;
+
+	switch (pin) {
+	case 0 ... 7:
+		offset = pin * 4;
+		break;
+	case 8 ... 13:
+		offset = (pin - 8) * 4;
+		break;
+	case 14 ... 21:
+		offset = (pin - 14) * 4;
+		break;
+	case 22 ... 29:
+		offset = (pin - 22) * 4;
+		break;
+	case 30 ... 37:
+		offset = (pin - 30) * 4;
+		break;
+	default:
+		offset = -EINVAL;
+	}
+
+	return offset;
+}
+
+static int mpfs_pinctrl_pin_to_iomux_reg(unsigned int pin)
+{
+	int reg;
+
+	switch (pin) {
+	case 0 ... 7:
+		reg = 0x204;
+		break;
+	case 8 ... 13:
+		reg = 0x208;
+		break;
+	case 14 ... 21:
+		reg = 0x20c;
+		break;
+	case 22 ... 29:
+		reg = 0x210;
+		break;
+	case 30 ... 37:
+		reg = 0x214;
+		break;
+	default:
+		reg = -EINVAL;
+	}
+
+	return reg;
+}
+
+static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
+{
+	u32 reg = MPFS_PINCTRL_IOCFG01_REG;
+
+	if (pin >= MPFS_PINCTRL_BANK2_START)
+		reg += MPFS_PINCTRL_INTER_BANK_GAP;
+
+	// 2 pins per 32-bit register
+	reg += (pin / 2) * 0x4;
+
+	return reg;
+}
+
+static int mpfs_pinctrl_pin_to_iocfg_offset(unsigned int pin)
+{
+	return 16 * (pin % 2);
+}
+
+static u32 mpfs_pinctrl_pin_to_bank_voltage(struct mpfs_pinctrl *pctrl, unsigned int pin)
+{
+	u32 bank_voltage;
+
+	if (pin < MPFS_PINCTRL_BANK2_START)
+		bank_voltage = pctrl->bank4_voltage;
+	else
+		bank_voltage = pctrl->bank2_voltage;
+
+	return FIELD_GET(MPFS_PINCTRL_BANK_VOLTAGE_MASK, bank_voltage);
+}
+
+static void mpfs_pinctrl_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+				  unsigned int pin)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 func;
+	int reg, offset;
+
+	reg = mpfs_pinctrl_pin_to_iomux_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iomux_offset(pin);
+
+	seq_printf(seq, "reg: %x, offset: %u ", reg, offset);
+	seq_printf(seq, "pin: %u ", pin);
+
+	if (reg < 0 || offset < 0)
+		return;
+
+	regmap_read(pctrl->regmap, reg, &func);
+	func = (func >> offset) & MPFS_PINCTRL_PAD_MUX_MASK;
+	seq_printf(seq, "func: %s (%x)\n", mpfs_pinctrl_function_names[func], func);
+}
+
+static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
+				       struct pinctrl_map **maps, unsigned int *num_maps)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	struct device *dev = pctrl->dev;
+	struct device_node *child;
+	struct pinctrl_map *map;
+	const char **group_names;
+	const char *group_name;
+	int ngroups = 0;
+	int nmaps = 0;
+	int ret;
+
+	for_each_available_child_of_node(np, child)
+		ngroups += 1;
+
+	group_names = devm_kcalloc(dev, ngroups, sizeof(*group_names), GFP_KERNEL);
+	if (!group_names)
+		return -ENOMEM;
+
+	map = kcalloc(ngroups * 2, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	ngroups = 0;
+	guard(mutex)(&pctrl->mutex);
+	for_each_available_child_of_node_scoped(np, child) {
+		struct mpfs_pinctrl_mux_config *pinmuxs;
+		const char *function_name;
+		unsigned int pin, *pins;
+		int function, npins;
+
+		npins = of_property_count_u32_elems(child, "pins");
+
+		if (npins < 1) {
+			dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
+				np, child, npins);
+			return npins;
+		}
+
+		group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn",
+					 np, child);
+		if (!group_name)
+			return -ENOMEM;
+
+		group_names[ngroups++] = group_name;
+
+		pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+		if (!pins)
+			return -ENOMEM;
+
+		pinmuxs = devm_kcalloc(dev, npins, sizeof(*pinmuxs), GFP_KERNEL);
+		if (!pinmuxs)
+			return -ENOMEM;
+
+		for (int i = 0; i < npins; i++) {
+			ret = of_property_read_u32_index(child, "pins", i, &pin);
+			if (ret)
+				return ret;
+
+			pins[i] = pin;
+			pinmuxs[i].pin = pin;
+
+			ret = of_property_read_string(child, "function", &function_name);
+			function = mpfs_pinctrl_function_map(function_name);
+			if (function < 0)
+				return dev_err_probe(dev, function, "invalid function %s\n",
+						     function_name);
+			pinmuxs[i].function = function;
+		}
+
+		map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
+		map[nmaps].data.mux.function = np->name;
+		map[nmaps].data.mux.group = group_name;
+		nmaps += 1;
+
+		ret = pinctrl_generic_add_group(pctrl_dev, group_name, pins, npins, pinmuxs);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "failed to add group %s: %d\n",
+					     group_name, ret);
+
+		ret = pinconf_generic_parse_dt_config(child, pctrl_dev,
+						      &map[nmaps].data.configs.configs,
+						      &map[nmaps].data.configs.num_configs);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to parse pin config of group %s\n",
+				group_name);
+
+		if (map[nmaps].data.configs.num_configs == 0)
+			continue;
+
+		map[nmaps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+		map[nmaps].data.configs.group_or_pin = group_name;
+		nmaps += 1;
+	}
+
+	ret = pinmux_generic_add_function(pctrl_dev, np->name, group_names, ngroups, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctrl_dev, map, nmaps);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	*maps = map;
+	*num_maps = nmaps;
+
+	return 0;
+};
+
+static const struct pinctrl_ops mpfs_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name	= pinctrl_generic_get_group_name,
+	.get_group_pins	= pinctrl_generic_get_group_pins,
+	.pin_dbg_show = mpfs_pinctrl_dbg_show,
+	.dt_node_to_map = mpfs_pinctrl_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int mpfs_pinctrl_set_pin_func(struct mpfs_pinctrl *pctrl, u8 pin, u8 function)
+{
+	struct device *dev = pctrl->dev;
+	u32 reg, func, mask, offset;
+
+	reg = mpfs_pinctrl_pin_to_iomux_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iomux_offset(pin);
+
+	func = function << offset;
+	mask = MPFS_PINCTRL_PAD_MUX_MASK << offset;
+
+	dev_dbg(dev, "Setting pin %u. reg: %x offset %u func %x\n", pin, reg, offset, func);
+
+	if (reg < 0 || offset < 0)
+		return -EINVAL;
+
+	regmap_update_bits(pctrl->regmap, reg, mask, func);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev, unsigned int fsel,
+				unsigned int gsel)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	struct mpfs_pinctrl_mux_config *configs;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	configs = group->data;
+
+	for (int i = 0; i < group->grp.npins; i++)
+		mpfs_pinctrl_set_pin_func(pctrl, configs[i].pin, configs[i].function);
+
+	return 0;
+}
+
+static const struct pinmux_ops mpfs_pinctrl_pinmux_ops = {
+	.get_functions_count	= pinmux_generic_get_function_count,
+	.get_function_name	= pinmux_generic_get_function_name,
+	.get_function_groups	= pinmux_generic_get_function_groups,
+	.set_mux		= mpfs_pinctrl_set_mux,
+};
+
+static int mpfs_pinctrl_get_drive_strength_ma(u32 drive_strength)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_drive_strengths);
+
+	for (int i = 0; i < num; i++)
+		if (drive_strength == mpfs_pinctrl_drive_strengths[i].val)
+			return mpfs_pinctrl_drive_strengths[i].ma;
+
+	return -EINVAL;
+}
+
+static int mpfs_pinctrl_get_drive_strength_val(u32 drive_strength_ma)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_drive_strengths);
+
+	if (!drive_strength_ma)
+		return -EINVAL;
+
+	for (int i = 0; i < num; i++)
+		if (drive_strength_ma <= mpfs_pinctrl_drive_strengths[i].ma)
+			return mpfs_pinctrl_drive_strengths[i].val;
+
+	return mpfs_pinctrl_drive_strengths[num - 1].val;
+}
+
+static int mpfs_pinctrl_pinconf_get(struct pinctrl_dev *pctrl_dev, unsigned int pin,
+				    unsigned long *config)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	int param = pinconf_to_config_param(*config);
+	int reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	int val;
+	u32 arg;
+	//TODO bank_voltage;
+	u8 str;
+
+	regmap_read(pctrl->regmap, reg, &val);
+
+	val = val >> mpfs_pinctrl_pin_to_iocfg_offset(pin);
+	val = val & MPFS_PINCTRL_IOCFG_MASK;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_BUS_HOLD:
+		if (!(val & MPFS_PINCTRL_WPD))
+			return -EINVAL;
+
+		if (!(val & MPFS_PINCTRL_WPU))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (!(val & MPFS_PINCTRL_WPD))
+			return -EINVAL;
+
+		if (val & MPFS_PINCTRL_WPU)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (!(val & MPFS_PINCTRL_WPU))
+			return -EINVAL;
+
+		if (val & MPFS_PINCTRL_WPD)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (val & MPFS_PINCTRL_PULL_MASK)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		str = FIELD_GET(MPFS_PINCTRL_DRV_MASK, val);
+		if (!str)
+			return -EINVAL;
+
+		arg = mpfs_pinctrl_get_drive_strength_ma(str);
+		break;
+	//TODO @Linus, it correct to group these 3? There's no control over voltage.
+	case PIN_CONFIG_INPUT_SCHMITT:
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+	case PIN_CONFIG_INPUT_SCHMITT_UV:
+		//TODO Is it enabled regardless of register setting, or must it
+		// be set for lower voltage IO? Docs are missing, MSS Configurator
+		// is not clear. Leaning towards the latter.
+		//bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
+		//if (bank_voltage < MPFS_PINCTRL_LVCMOS25) {
+		//	arg = 1;
+		//	break;
+		//}
+
+		if (!FIELD_GET(MPFS_PINCTRL_ENHYST, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_PERSIST_STATE:
+		if (!FIELD_GET(MPFS_PINCTRL_LP_PERSIST_EN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_MODE_LOW_POWER:
+		if (!FIELD_GET(MPFS_PINCTRL_LP_BYPASS_EN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_CLAMP_DIODE:
+		if (!FIELD_GET(MPFS_PINCTRL_CLAMP, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_LOCKDOWN:
+		if (!FIELD_GET(MPFS_PINCTRL_LOCKDN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_IBUFMD:
+		arg = FIELD_GET(MPFS_PINCTRL_IBUFMD_MASK, val);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
+						unsigned long *configs, unsigned int num_configs,
+						u32 *value)
+{
+	u32 val = 0;
+
+	for (int i = 0; i < num_configs; i++) {
+		int param, tmp;
+		u32 arg;
+		//TODO bank_voltage;
+
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_BUS_HOLD:
+			val |= MPFS_PINCTRL_PULL_MASK;
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			//TODO always start from val == 0, there's no reason to ever actually
+			// clear anything AFAICT. @Linus, does the driver need to check mutual
+			// exclusion on these, or can I drop the clearing?
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			val |= MPFS_PINCTRL_WPD;
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			val |= MPFS_PINCTRL_WPU;
+			break;
+		case PIN_CONFIG_BIAS_DISABLE:
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			tmp = mpfs_pinctrl_get_drive_strength_val(arg);
+			if (tmp < 0)
+				return tmp;
+
+			val |= FIELD_PREP(MPFS_PINCTRL_DRV_MASK, tmp);
+			break;
+		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+			if (!arg)
+				break;
+			fallthrough;
+		case PIN_CONFIG_INPUT_SCHMITT:
+		case PIN_CONFIG_INPUT_SCHMITT_UV:
+			//TODO Is it enabled regardless of register setting, or must it
+			// be set for lower voltage IO? Docs are missing, MSS Configurator
+			// is not clear. Leaning towards the latter.
+			//bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
+			//if (bank_voltage < MPFS_PINCTRL_LVCMOS25 && !arg) {
+			//	dev_err(pctrl->dev,
+			//		"schmitt always enabled for 1.2, 1.5 and 1.8 volt io\n");
+			//	return -EINVAL;
+			//}
+			val |= MPFS_PINCTRL_ENHYST;
+			break;
+		case PIN_CONFIG_PERSIST_STATE:
+			val |= MPFS_PINCTRL_LP_PERSIST_EN;
+			break;
+		case PIN_CONFIG_MODE_LOW_POWER:
+			if (arg)
+				val |= MPFS_PINCTRL_LP_BYPASS_EN;
+			break;
+		//TODO @Linus, do I have to document these custom controls other than in the binding?
+		case MPFS_PINCTRL_CLAMP_DIODE:
+			val |= MPFS_PINCTRL_CLAMP;
+			break;
+		case MPFS_PINCTRL_LOCKDOWN:
+			val &= MPFS_PINCTRL_LOCKDN;
+			break;
+		case MPFS_PINCTRL_IBUFMD:
+			val |= FIELD_PREP(MPFS_PINCTRL_IBUFMD_MASK, arg);
+			break;
+		default:
+			dev_err(pctrl->dev, "config %u not supported\n", param);
+			return -ENOTSUPP;
+		}
+	}
+
+	*value = val;
+	return 0;
+}
+
+static int mpfs_pinctrl_pin_set_config(struct mpfs_pinctrl *pctrl, unsigned int pin, u32 config)
+{
+	int reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	int offset = mpfs_pinctrl_pin_to_iocfg_offset(pin);
+	u32 val, mask;
+
+	mask = MPFS_PINCTRL_IOCFG_MASK << offset;
+	val = config << offset;
+
+	regmap_update_bits(pctrl->regmap, reg, mask, val);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_pinconf_set(struct pinctrl_dev *pctrl_dev, unsigned int pin,
+				    unsigned long *configs, unsigned int num_configs)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+	int ret;
+
+	ret = mpfs_pinctrl_pinconf_generate_config(pctrl, pin, configs, num_configs, &val);
+	if (ret)
+		return ret;
+
+	return mpfs_pinctrl_pin_set_config(pctrl, pin, val);
+}
+
+static int mpfs_pinctrl_pinconf_group_set(struct pinctrl_dev *pctrl_dev, unsigned int gsel,
+					  unsigned long *configs, unsigned int num_configs)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	unsigned int pin;
+	u32 val;
+	int ret;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	/*
+	 * Assume that the first pin in a group is representative, as the mss
+	 * configurator doesn't allow splitting a function between two banks.
+	 */
+	pin = group->grp.pins[0];
+
+	ret = mpfs_pinctrl_pinconf_generate_config(pctrl, pin, configs, num_configs, &val);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < group->grp.npins; i++)
+		mpfs_pinctrl_pin_set_config(pctrl, group->grp.pins[i], val);
+
+	return 0;
+}
+
+static void mpfs_pinctrl_pinconf_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+					  unsigned int pin)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+	int reg, offset;
+
+	seq_printf(seq, ", bank voltage: 0x%x, ", mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin));
+
+	reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iocfg_offset(pin);
+
+	seq_printf(seq, "pin: %u ", pin);
+	seq_printf(seq, "reg: %x offset: %u ", reg, offset);
+
+	if (reg < 0 || offset < 0)
+		return;
+
+	regmap_read(pctrl->regmap, reg, &val);
+	val = (val & (MPFS_PINCTRL_IOCFG_MASK << offset)) >> offset;
+	seq_printf(seq, "val: %x\n", val);
+}
+
+static const struct pinconf_ops mpfs_pinctrl_pinconf_ops = {
+	.pin_config_get			= mpfs_pinctrl_pinconf_get,
+	.pin_config_set			= mpfs_pinctrl_pinconf_set,
+	.pin_config_group_set		= mpfs_pinctrl_pinconf_group_set,
+	.pin_config_dbg_show		= mpfs_pinctrl_pinconf_dbg_show,
+	.is_generic			= true,
+};
+
+static const struct pinctrl_pin_desc mpfs_pinctrl_pins[] = {
+	PINCTRL_PIN(0,  "bank 4 0"),
+	PINCTRL_PIN(1,  "bank 4 1"),
+	PINCTRL_PIN(2,  "bank 4 2"),
+	PINCTRL_PIN(3,  "bank 4 3"),
+	PINCTRL_PIN(4,  "bank 4 4"),
+	PINCTRL_PIN(5,  "bank 4 5"),
+	PINCTRL_PIN(6,  "bank 4 6"),
+	PINCTRL_PIN(7,  "bank 4 7"),
+	PINCTRL_PIN(8,  "bank 4 8"),
+	PINCTRL_PIN(9,  "bank 4 9"),
+	PINCTRL_PIN(10, "bank 4 10"),
+	PINCTRL_PIN(11, "bank 4 11"),
+	PINCTRL_PIN(12, "bank 4 12"),
+	PINCTRL_PIN(13, "bank 4 13"),
+
+	PINCTRL_PIN(14, "bank 2 0"),
+	PINCTRL_PIN(15, "bank 2 1"),
+	PINCTRL_PIN(16, "bank 2 2"),
+	PINCTRL_PIN(17, "bank 2 3"),
+	PINCTRL_PIN(18, "bank 2 4"),
+	PINCTRL_PIN(19, "bank 2 5"),
+	PINCTRL_PIN(20, "bank 2 6"),
+	PINCTRL_PIN(21, "bank 2 7"),
+	PINCTRL_PIN(22, "bank 2 8"),
+	PINCTRL_PIN(23, "bank 2 9"),
+	PINCTRL_PIN(24, "bank 2 10"),
+	PINCTRL_PIN(25, "bank 2 11"),
+	PINCTRL_PIN(26, "bank 2 12"),
+	PINCTRL_PIN(27, "bank 2 13"),
+	PINCTRL_PIN(28, "bank 2 14"),
+	PINCTRL_PIN(29, "bank 2 15"),
+	PINCTRL_PIN(30, "bank 2 16"),
+	PINCTRL_PIN(31, "bank 2 17"),
+	PINCTRL_PIN(32, "bank 2 18"),
+	PINCTRL_PIN(33, "bank 2 19"),
+	PINCTRL_PIN(34, "bank 2 20"),
+	PINCTRL_PIN(35, "bank 2 21"),
+	PINCTRL_PIN(36, "bank 2 22"),
+	PINCTRL_PIN(37, "bank 2 23"),
+};
+
+static int mpfs_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mpfs_pinctrl *pctrl;
+	struct regmap *sysreg_scb;
+	int ret;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->regmap = device_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(pctrl->regmap))
+		dev_err_probe(dev, PTR_ERR(pctrl->regmap), "Failed to find syscon regmap\n");
+
+	sysreg_scb = syscon_regmap_lookup_by_compatible("microchip,mpfs-sysreg-scb");
+	if (IS_ERR(sysreg_scb))
+		return PTR_ERR(sysreg_scb);
+
+	pctrl->desc.name = dev_name(dev);
+	pctrl->desc.pins = mpfs_pinctrl_pins;
+	pctrl->desc.npins = ARRAY_SIZE(mpfs_pinctrl_pins);
+	pctrl->desc.pctlops = &mpfs_pinctrl_ops;
+	pctrl->desc.pmxops = &mpfs_pinctrl_pinmux_ops;
+	pctrl->desc.confops = &mpfs_pinctrl_pinconf_ops;
+	pctrl->desc.owner = THIS_MODULE;
+	pctrl->desc.num_custom_params = ARRAY_SIZE(mpfs_pinctrl_custom_bindings);
+	pctrl->desc.custom_params = mpfs_pinctrl_custom_bindings;
+
+	pctrl->dev = dev;
+
+	ret = devm_mutex_init(dev, &pctrl->mutex);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pctrl);
+
+	pctrl->pctrl = devm_pinctrl_register(&pdev->dev, &pctrl->desc, pctrl);
+	if (IS_ERR(pctrl->pctrl))
+		return PTR_ERR(pctrl->pctrl);
+
+	return 0;
+}
+
+static const struct of_device_id mpfs_pinctrl_of_match[] = {
+	{ .compatible = "microchip,mpfs-pinctrl-mssio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mpfs_pinctrl_of_match);
+
+static struct platform_driver mpfs_pinctrl_driver = {
+	.driver = {
+		.name = "mpfs-pinctrl",
+		.of_match_table = mpfs_pinctrl_of_match,
+	},
+	.probe = mpfs_pinctrl_probe,
+};
+module_platform_driver(mpfs_pinctrl_driver);
+
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("Polarfire SoC mssio pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.51.0

[RFC v1 3/4] MAINTAINERS: add Microchip mpfs mssio driver/bindings to entry

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Add the new mssio driver and bindings to the existing entry
for Microchip RISC-V devices.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d4825073fcd..380970935407 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22090,6 +22090,7 @@ F:	Documentation/devicetree/bindings/i2c/microchip,corei2c.yaml
 F:	Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
 F:	Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
 F:	Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
+F:	Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
 F:	Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
 F:	Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
 F:	Documentation/devicetree/bindings/riscv/microchip.yaml
@@ -22105,6 +22106,7 @@ F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
 F:	drivers/pci/controller/plda/pcie-microchip-host.c
 F:	drivers/pinctrl/pinctrl-mpfs-iomux0.c
+F:	drivers/pinctrl/pinctrl-mpfs-mssio.c
 F:	drivers/pinctrl/pinctrl-pic64gx-gpio2.c
 F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/reset/reset-mpfs.c
-- 
2.51.0

[RFC v1 4/4] riscv: dts: microchip: add pinctrl nodes for mpfs/icicle kit

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Add pinctrl nodes to PolarFire to demonstrate their use, matching the
default configuration set by the HSS firmware for the Icicle kit's
reference design, as a demonstration of use.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../dts/microchip/mpfs-icicle-kit-common.dtsi |   1 -
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  63 +++++++
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 165 ++++++++++++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  16 ++
 4 files changed, 244 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
index b3f61c58e57c..5667805b4b14 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
@@ -3,7 +3,6 @@
 
 /dts-v1/;
 
-#include "mpfs.dtsi"
 #include "mpfs-icicle-kit-fabric.dtsi"
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/leds/common.h>
diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index 71f724325578..785176dabcf1 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0 OR MIT)
 /* Copyright (c) 2020-2021 Microchip Technology Inc */
 
+#include "mpfs.dtsi"
+#include "mpfs-pinctrl.dtsi"
+
 / {
 	core_pwm0: pwm@40000000 {
 		compatible = "microchip,corepwm-rtl-v4";
@@ -80,6 +83,16 @@ refclk_ccc: clock-cccref {
 	};
 };
 
+&can0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&can0_fabric>;
+};
+
+&can1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ikrd_can1_cfg>;
+};
+
 &ccc_nw {
 	clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>,
 		 <&refclk_ccc>, <&refclk_ccc>;
@@ -87,3 +100,53 @@ &ccc_nw {
 		      "dll0_ref", "dll1_ref";
 	status = "okay";
 };
+
+&i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c0_fabric>;
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_fabric>;
+};
+
+&mmuart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_fabric>;
+};
+
+&mmuart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart2_fabric>;
+};
+
+&mmuart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart3_fabric>;
+};
+
+&mmuart4 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart4_fabric>;
+};
+
+&mssio {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi1_mssio>, <&can1_mssio>, <&mdio0_mssio>, <&mdio1_mssio>;
+};
+
+&qspi {
+	pinctrl-names = "default";
+	pinctrl-0 = <&qspi_fabric>;
+};
+
+&spi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_fabric>;
+};
+
+&spi1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ikrd_spi1_cfg>;
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi b/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
new file mode 100644
index 000000000000..47fc4a523c33
--- /dev/null
+++ b/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+
+&iomux0 {
+	spi0_fabric: mux-spi0-fabric {
+		function = "spi0";
+		groups = "spi0_fabric";
+	};
+
+	spi0_mssio: mux-spi0-mssio {
+		function = "spi0";
+		groups = "spi0_mssio";
+	};
+
+	spi1_fabric: mux-spi1-fabric {
+		function = "spi1";
+		groups = "spi1_fabric";
+	};
+
+	spi1_mssio: mux-spi1-mssio {
+		function = "spi1";
+		groups = "spi1_mssio";
+	};
+
+	i2c0_fabric: mux-i2c0-fabric {
+		function = "i2c0";
+		groups = "i2c0_fabric";
+	};
+
+	i2c0_mssio: mux-i2c0-mssio {
+		function = "i2c0";
+		groups = "i2c0_mssio";
+	};
+
+	i2c1_fabric: mux-i2c1-fabric {
+		function = "i2c1";
+		groups = "i2c1_fabric";
+	};
+
+	i2c1_mssio: mux-i2c1-mssio {
+		function = "i2c1";
+		groups = "i2c1_mssio";
+	};
+
+	can0_fabric: mux-can0-fabric {
+		function = "can0";
+		groups = "can0_fabric";
+	};
+
+	can0_mssio: mux-can0-mssio {
+		function = "can0";
+		groups = "can0_mssio";
+	};
+
+	can1_fabric: mux-can1-fabric {
+		function = "can1";
+		groups = "can1_fabric";
+	};
+
+	can1_mssio: mux-can1-mssio {
+		function = "can1";
+		groups = "can1_mssio";
+	};
+
+	qspi_fabric: mux-qspi-fabric {
+		function = "qspi";
+		groups = "qspi_fabric";
+	};
+
+	qspi_mssio: mux-qspi-mssio {
+		function = "qspi";
+		groups = "qspi_mssio";
+	};
+
+	uart0_fabric: mux-uart0-fabric {
+		function = "uart0";
+		groups = "uart0_fabric";
+	};
+
+	uart0_mssio: mux-uart0-mssio {
+		function = "uart0";
+		groups = "uart0_mssio";
+	};
+
+	uart1_fabric: mux-uart1-fabric {
+		function = "uart1";
+		groups = "uart1_fabric";
+	};
+
+	uart1_mssio: mux-uart1-mssio {
+		function = "uart1";
+		groups = "uart1_mssio";
+	};
+
+	uart2_fabric: mux-uart2-fabric {
+		function = "uart2";
+		groups = "uart2_fabric";
+	};
+
+	uart2_mssio: mux-uart2-mssio {
+		function = "uart2";
+		groups = "uart2_mssio";
+	};
+
+	uart3_fabric: mux-uart3-fabric {
+		function = "uart3";
+		groups = "uart3_fabric";
+	};
+
+	uart3_mssio: mux-uart3-mssio {
+		function = "uart3";
+		groups = "uart3_mssio";
+	};
+
+	uart4_fabric: mux-uart4-fabric {
+		function = "uart4";
+		groups = "uart4_fabric";
+	};
+
+	uart4_mssio: mux-uart4-mssio {
+		function = "uart4";
+		groups = "uart4_mssio";
+	};
+
+	mdio0_fabric: mux-mdio0-fabric {
+		function = "mdio0";
+		groups = "mdio0_fabric";
+	};
+
+	mdio0_mssio: mux-mdio0-mssio {
+		function = "mdio0";
+		groups = "mdio0_mssio";
+	};
+
+	mdio1_fabric: mux-mdio1-fabric {
+		function = "mdio1";
+		groups = "mdio1_fabric";
+	};
+
+	mdio1_mssio: mux-mdio1-mssio {
+		function = "mdio1";
+		groups = "mdio1_mssio";
+	};
+};
+
+&mssio {
+	ikrd_can1_cfg: ikrd-can1-cfg {
+		can1-pins {
+			pins = <34>, <35>, <36>;
+			function = "spi";
+			bias-pull-up;
+			drive-strength = <8>;
+			microchip,ibufmd = <0x1>;
+		};
+	};
+
+	ikrd_spi1_cfg: ikrd-spi1-cfg {
+		spi1-pins {
+			pins = <30>, <31>, <32>, <33>;
+			function = "spi";
+			bias-pull-up;
+			drive-strength = <8>;
+			microchip,ibufmd = <0x1>;
+		};
+	};
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 5c2963e269b8..0fb94581b6cb 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -254,7 +254,23 @@ pdma: dma-controller@3000000 {
 		mss_top_sysreg: syscon@20002000 {
 			compatible = "microchip,mpfs-mss-top-sysreg", "syscon", "simple-mfd";
 			reg = <0x0 0x20002000 0x0 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
 			#reset-cells = <1>;
+
+			iomux0: pinctrl@200 {
+				compatible = "microchip,mpfs-pinctrl-iomux0";
+				reg = <0x200 0x4>;
+				pinctrl-use-default;
+
+			};
+
+			mssio: pinctrl@204 {
+				compatible = "microchip,mpfs-pinctrl-mssio";
+				reg = <0x204 0x7c>;
+				/* on icicle ref design at least */
+				pinctrl-use-default;
+			};
 		};
 
 		sysreg_scb: syscon@20003000 {
-- 
2.51.0

Re: [RFC v1 1/4] dt-bindings: pinctrl: document polarfire soc mssio pin controller

[Linus Walleij]

Hi Conor,

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> +examples:
> +  - |
> +    pinctrl@204 {
> +      compatible = "microchip,mpfs-pinctrl-mssio";
> +      reg = <0x204 0x7c>;
> +
> +      irkd_sd_cfg: irkd-sd-cfg {
> +        sd-10ma-pins {
> +          pins = <0>, <1>, <2>, <3>, <4>, <5>, <8>, <9>, <10>, <11>, <12>, <13>;
> +          function = "sd";
> +          bias-pull-up;
> +          drive-strength = <10>;
> +        };
> +
> +        sd-8ma-pins {
> +          pins = <6>, <7>;
> +          function = "sd";
> +          bias-pull-up;
> +          drive-strength = <8>;
> +        };
> +      };
> +    };

This looks good to me and you sure know how to do syntax
so I don't even need to check that part.

These bindings are fine.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

Hi Conor,

took a quick look at this!

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> +#include "linux/dev_printk.h"

Weird but it's RFC so OK :)

> +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)

Yeah this should work for custom props.

> +struct mpfs_pinctrl_drive_strength {
> +       u8 ma;
> +       u8 val;
> +};
> +
> +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> +       { 2,   2 },
> +       { 4,   3 },
> +       { 6,   4 },
> +       { 8,   5 },
> +       { 10,  6 },
> +       { 12,  7 },
> +       { 16, 10 },
> +       { 20, 12 },
> +};

I would probably assign field explicitly with C99 syntax, but no
hard requirement.

{ .ma = 2, .val = 2 }

BTW you can see clearly that each setting activates
another driver stage in the silicon, each totempole giving
2 mA.

> +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> +};

I take it these have proper documentation in the DT bindings, so users know
exactly what they do.

> +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> +{
> +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> +
> +       if (pin >= MPFS_PINCTRL_BANK2_START)
> +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> +
> +       // 2 pins per 32-bit register
> +       reg += (pin / 2) * 0x4;

This is a nice comment, easy to follow the code with small helpful
things like this.

> +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> +                                      struct pinctrl_map **maps, unsigned int *num_maps)

I saw in the cover letter that you wanted this to use more generic helpers.

If you see room for improvement of the generic code, do not hesitate.
Doing a new driver is the only time when you actually have all these
details in your head and can create good helpers.

> +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> +       case PIN_CONFIG_INPUT_SCHMITT:
> +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +       case PIN_CONFIG_INPUT_SCHMITT_UV:

Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
in the bindings if they don't make any sense, and let it just return error
if someone tries to do that.

Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
makes sense for this hardware?

> +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> +                                               unsigned long *configs, unsigned int num_configs,
> +                                               u32 *value)
(...)
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       //TODO always start from val == 0, there's no reason to ever actually
> +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> +                       // exclusion on these, or can I drop the clearing?
> +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> +                       val |= MPFS_PINCTRL_WPD;
> +                       break;

I was about to say that the core checks that you don't enable pull up
and pull down
at the same time, but apparently that was just a dream I had.

The gpiolib however contains this in gpiod_configure_flags():

        if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
            ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
            ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
                gpiod_err(desc,
                          "multiple pull-up, pull-down or pull-disable
enabled, invalid configuration\n");
                return -EINVAL;
        }

So there is a precedent for checking this.

So if you patch pinconf-generic.c to disallow this that'd be great, I think
it makes most sense to do this in the core.

> +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +                       if (!arg)
> +                               break;
> +                       fallthrough;
> +               case PIN_CONFIG_INPUT_SCHMITT:
> +               case PIN_CONFIG_INPUT_SCHMITT_UV:
> +                       //TODO Is it enabled regardless of register setting, or must it
> +                       // be set for lower voltage IO? Docs are missing, MSS Configurator
> +                       // is not clear. Leaning towards the latter.
> +                       //bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
> +                       //if (bank_voltage < MPFS_PINCTRL_LVCMOS25 && !arg) {
> +                       //      dev_err(pctrl->dev,
> +                       //              "schmitt always enabled for 1.2, 1.5 and 1.8 volt io\n");
> +                       //      return -EINVAL;
> +                       //}
> +                       val |= MPFS_PINCTRL_ENHYST;
> +                       break;

See above.

I hope this helps!

Yours,
Linus Walleij

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Linus Walleij]

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> Got the other driver that I was talking about here for you...
> It's in RFC state because I'd like to get some feedback on the approach
> while I try to figure out a) what ibufmd is

I was going to ask about that :D

> and b) how the bank voltage
> interacts with the schmitt trigger setting.

Please check if "bank voltage" is somewhat analogous to
this generic config:

* @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
 *      supplies, the argument to this parameter (on a custom format) tells
 *      the driver which alternative power source to use.

> There's some specific @Linus questions in the driver, mostly where I was
> unsure about how config bits should be handled and looking around at
> other drivers wasn't sufficient because they did different things.

I tried to answer as best I could.

> Finally, on the dt side, this was using the pinmux property before the
> other drivers were submitted, but I didn't really like it to begin with
> (shoving two things into entries of the same property gives me the ick).
> I moved to using pins + function which at least looks prettier in the
> devicetree.

I think this looks way better than any pinmux properties.

> I had been hoping that I could move to some sort of generic
> dt_node_to_map function, but I couldn't figure out one that'd work
> without creating groups in the driver. If there is, I'd love to get rid
> of the custom dt_node_to_map stuff.

It seems like something that could be added to the core
(drivers/pinctrl/devicetree.c), if you feel like and have time for going
the extra mile. Maybe it would be simple to move some drivers
over to using it if done right.

> I want to avoid doing having set groups, because of the number of
> possible configurations in the MSS Configurator FPGA tool is fairly
> large, and I believe that MSS Configurator actually undersells the
> number of possible combinations for ease of use.

FPGA:s often have this "phone exchange" property.

> I haven't tested that
> and the driver technically doesn't support it, but I feel like not trying
> to define groups statically and using pins instead would permit those
> combos in the future should that use case ever materialise.

It makes sense for a driver for this type of very flexible hardware.

Yours,
Linus Walleij

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > Got the other driver that I was talking about here for you...
> > It's in RFC state because I'd like to get some feedback on the approach
> > while I try to figure out a) what ibufmd is
> 
> I was going to ask about that :D
> 
> > and b) how the bank voltage
> > interacts with the schmitt trigger setting.
> 
> Please check if "bank voltage" is somewhat analogous to
> this generic config:
> 
> * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
>  *      supplies, the argument to this parameter (on a custom format) tells
>  *      the driver which alternative power source to use.

It's not pin based, the whole bank it is connected to has to be changed.
I'm not entirely sure if I want the driver to be able to control this at
all (at least not right now) because I don't know exactly what the
ramifications of changing it away from what the configuration tool
decided that it was are. Probably it could be done, but I don't know how
safe it is to change, as I think most of these kinds of settings were
never really seen as something "application" software would change
(linux is an application from the perspective of the design folks),
since it was all modifiable from the MSS configuration tool.

> > There's some specific @Linus questions in the driver, mostly where I was
> > unsure about how config bits should be handled and looking around at
> > other drivers wasn't sufficient because they did different things.
> 
> I tried to answer as best I could.

Cool, thanks.

> > Finally, on the dt side, this was using the pinmux property before the
> > other drivers were submitted, but I didn't really like it to begin with
> > (shoving two things into entries of the same property gives me the ick).
> > I moved to using pins + function which at least looks prettier in the
> > devicetree.
> 
> I think this looks way better than any pinmux properties.
> 
> > I had been hoping that I could move to some sort of generic
> > dt_node_to_map function, but I couldn't figure out one that'd work
> > without creating groups in the driver. If there is, I'd love to get rid
> > of the custom dt_node_to_map stuff.
> 
> It seems like something that could be added to the core
> (drivers/pinctrl/devicetree.c), if you feel like and have time for going
> the extra mile. Maybe it would be simple to move some drivers
> over to using it if done right.

Yeah I might, especially if it pushes people away from these pinmux
properties! Might also just submit this as-is and do it on top, depends
on what stuff I have going on.

> 
> > I want to avoid doing having set groups, because of the number of
> > possible configurations in the MSS Configurator FPGA tool is fairly
> > large, and I believe that MSS Configurator actually undersells the
> > number of possible combinations for ease of use.
> 
> FPGA:s often have this "phone exchange" property.
> 
> > I haven't tested that
> > and the driver technically doesn't support it, but I feel like not trying
> > to define groups statically and using pins instead would permit those
> > combos in the future should that use case ever materialise.
> 
> It makes sense for a driver for this type of very flexible hardware.


Thanks for taking a look.

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> Hi Conor,
> 
> took a quick look at this!
> 
> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > +#include "linux/dev_printk.h"
> 
> Weird but it's RFC so OK :)
> 
> > +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> > +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> > +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)
> 
> Yeah this should work for custom props.
> 
> > +struct mpfs_pinctrl_drive_strength {
> > +       u8 ma;
> > +       u8 val;
> > +};
> > +
> > +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> > +       { 2,   2 },
> > +       { 4,   3 },
> > +       { 6,   4 },
> > +       { 8,   5 },
> > +       { 10,  6 },
> > +       { 12,  7 },
> > +       { 16, 10 },
> > +       { 20, 12 },
> > +};
> 
> I would probably assign field explicitly with C99 syntax, but no
> hard requirement.
> 
> { .ma = 2, .val = 2 }
> 
> BTW you can see clearly that each setting activates
> another driver stage in the silicon, each totempole giving
> 2 mA.
> 
> > +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> > +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> > +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> > +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> > +};
> 
> I take it these have proper documentation in the DT bindings, so users know
> exactly what they do.

Honestly, even if users don't know what they do, they should just be
making this stuff 1:1 match some MSS configurator output, where it's
very clear if you select lockdown or turn on the clamp diode. The clamp
diode is almost certainly input voltage clamping (although the
documentation on the mssio stuff doesn't say anything about it) and the
lockdwn thing is some security feature that doesn't actually do anything
on its own, but will disable the bank in question if the tamper
detection hardware gets upset.
I'm not really all that sure why this is a per-pin setting, because the
documentation about this (and the MSS configurator) treat it as being
bank wide. There's some other part of this in another register that is
actually bank wide that I only discovered yesterday, and I dunno how
they interact.
The binding doesn't really explain this stuff, other than saying what
field in the configurator to get it from. I'll add something for !RFC.

ibufmd is a different story, the only source of it I could find was
in the configurator output files. I suspect that it can be just computed
from the other dt properties and the bank voltage, but I need to find
the answer in the configurator source code I think.

> > +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> > +{
> > +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> > +
> > +       if (pin >= MPFS_PINCTRL_BANK2_START)
> > +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> > +
> > +       // 2 pins per 32-bit register
> > +       reg += (pin / 2) * 0x4;
> 
> This is a nice comment, easy to follow the code with small helpful
> things like this.
> 
> > +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> > +                                      struct pinctrl_map **maps, unsigned int *num_maps)
> 
> I saw in the cover letter that you wanted this to use more generic helpers.
> 
> If you see room for improvement of the generic code, do not hesitate.
> Doing a new driver is the only time when you actually have all these
> details in your head and can create good helpers.
> 
> > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > +       case PIN_CONFIG_INPUT_SCHMITT:
> > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> 
> Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> in the bindings if they don't make any sense, and let it just return error
> if someone tries to do that.
> 
> Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> makes sense for this hardware?

Yeah, I just didn't know if having the others was helpful, since they
say things like:
 * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
 *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
 *	the threshold value is given on a custom format as argument when
 *	setting pins to this mode.
which implies they should be implemented for systen not permitting
hysteresis adjustment.

> > +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> > +                                               unsigned long *configs, unsigned int num_configs,
> > +                                               u32 *value)
> (...)
> > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > +                       //TODO always start from val == 0, there's no reason to ever actually
> > +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> > +                       // exclusion on these, or can I drop the clearing?
> > +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> > +                       val |= MPFS_PINCTRL_WPD;
> > +                       break;
> 
> I was about to say that the core checks that you don't enable pull up
> and pull down
> at the same time, but apparently that was just a dream I had.
> 
> The gpiolib however contains this in gpiod_configure_flags():
> 
>         if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
>             ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
>             ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
>                 gpiod_err(desc,
>                           "multiple pull-up, pull-down or pull-disable
> enabled, invalid configuration\n");
>                 return -EINVAL;
>         }
> 
> So there is a precedent for checking this.
> 
> So if you patch pinconf-generic.c to disallow this that'd be great, I think
> it makes most sense to do this in the core.

Yeah, seems better than doing it in my driver..

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

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Linus Walleij]

On Wed, Nov 19, 2025 at 7:06 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > > and b) how the bank voltage
> > > interacts with the schmitt trigger setting.
> >
> > Please check if "bank voltage" is somewhat analogous to
> > this generic config:
> >
> > * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> >  *      supplies, the argument to this parameter (on a custom format) tells
> >  *      the driver which alternative power source to use.
>
> It's not pin based, the whole bank it is connected to has to be changed.

So there *is* such a thing as a group pin config setting for a
whole group of pins. Groups are not just for functions...

And I don't know what is meant by a bank here, but it seems
to be exactly a group of pins.

From arch/arm/boot/dts/gemini/gemini-sq201.dts:

 /* Set up drive strength on GMAC0 and GMAC1 to 16 mA */
conf9 {
    groups = "gmii_gmac0_grp", "gmii_gmac1_grp";
    drive-strength = <16>;
};

If you look in driver/pinctrl/pinctrl-gemini.c you find:
gemini_pinconf_group_set()

static const struct pinconf_ops gemini_pinconf_ops = {
        .pin_config_get = gemini_pinconf_get,
        .pin_config_set = gemini_pinconf_set,
        .pin_config_group_set = gemini_pinconf_group_set,
        .is_generic = true,
};

OTOMH it's actually *fine* to *just* use groups for pin config like this
and *not* use it for muxing, i.e. have this group correspond to
a bank and not use that group for anything else than to set this
or any other per-bank property. But have a look!

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > > +       case PIN_CONFIG_INPUT_SCHMITT:
> > > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> >
> > Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> > in the bindings if they don't make any sense, and let it just return error
> > if someone tries to do that.
> >
> > Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> > makes sense for this hardware?
>
> Yeah, I just didn't know if having the others was helpful, since they
> say things like:
>  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>  *      schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>  *      the threshold value is given on a custom format as argument when
>  *      setting pins to this mode.
> which implies they should be implemented for systen not permitting
> hysteresis adjustment.

I see.

I looked at the bindings that look like this and are not 1:1 to the
in-kernel configs:

  input-schmitt-enable:
    type: boolean
    description: enable schmitt-trigger mode

  input-schmitt-disable:
    type: boolean
    description: disable schmitt-trigger mode

  input-schmitt-microvolt:
    description: threshold strength for schmitt-trigger

1. input-schmitt is missing! But it is right there in
drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
using input-schmitt-enable/disable and -microvolt.

2. input-schmitt-microvolt should probably be used separately
to set the voltage threshold and can be used in conjunction
with input-schmitt-enable in the same node. In your case
you probably don't want to use it at all and disallow it.

They are all treated individually in the parser.

Maybe we could patch the docs in pinconf-generic.h to make it clear that
they are all mutually exclusive.

The DT parser is a bit primitive for these.
For example right now it is fine with the schema
to set input-schmitt-enable and input-schmitt-disable at the same time, and
the result will be enabled because of parse order :/

The real trick would be to also make the
schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
make them at least mutually exclusive and deprecate the
input-schmitt that noone is using, maybe that is simpler than I think?

Yours,
Linus Walleij

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 10:31:47PM +0100, Linus Walleij wrote:
> On Wed, Nov 19, 2025 at 7:06 PM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > > and b) how the bank voltage
> > > > interacts with the schmitt trigger setting.
> > >
> > > Please check if "bank voltage" is somewhat analogous to
> > > this generic config:
> > >
> > > * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> > >  *      supplies, the argument to this parameter (on a custom format) tells
> > >  *      the driver which alternative power source to use.
> >
> > It's not pin based, the whole bank it is connected to has to be changed.
> 
> So there *is* such a thing as a group pin config setting for a
> whole group of pins. Groups are not just for functions...
> 
> And I don't know what is meant by a bank here, but it seems
> to be exactly a group of pins.

Yeah, it's a whole group of pins. There's two banks that the IOs this
driver controls are split between, they don't really neatly correspond
to a particular function (although the configurator doesn't permit a
function belong to two banks, but it is technically possible for them
to).
> 
> From arch/arm/boot/dts/gemini/gemini-sq201.dts:
> 
>  /* Set up drive strength on GMAC0 and GMAC1 to 16 mA */
> conf9 {
>     groups = "gmii_gmac0_grp", "gmii_gmac1_grp";
>     drive-strength = <16>;
> };
> 
> If you look in driver/pinctrl/pinctrl-gemini.c you find:
> gemini_pinconf_group_set()
> 
> static const struct pinconf_ops gemini_pinconf_ops = {
>         .pin_config_get = gemini_pinconf_get,
>         .pin_config_set = gemini_pinconf_set,
>         .pin_config_group_set = gemini_pinconf_group_set,
>         .is_generic = true,
> };
> 
> OTOMH it's actually *fine* to *just* use groups for pin config like this
> and *not* use it for muxing, i.e. have this group correspond to
> a bank and not use that group for anything else than to set this
> or any other per-bank property. But have a look!

I could just put it in as a per-pin thing, even if the control isn't
actually that granular. The bank lockdown has a per-pin bit, but is
actually a bank wide toggle in the configurator, it wouldn't be too
different.

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > > > +       case PIN_CONFIG_INPUT_SCHMITT:
> > > > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> > >
> > > Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> > > in the bindings if they don't make any sense, and let it just return error
> > > if someone tries to do that.
> > >
> > > Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> > > makes sense for this hardware?
> >
> > Yeah, I just didn't know if having the others was helpful, since they
> > say things like:
> >  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> >  *      schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> >  *      the threshold value is given on a custom format as argument when
> >  *      setting pins to this mode.
> > which implies they should be implemented for systen not permitting
> > hysteresis adjustment.
> 
> I see.
> 
> I looked at the bindings that look like this and are not 1:1 to the
> in-kernel configs:
> 
>   input-schmitt-enable:
>     type: boolean
>     description: enable schmitt-trigger mode
> 
>   input-schmitt-disable:
>     type: boolean
>     description: disable schmitt-trigger mode
> 
>   input-schmitt-microvolt:
>     description: threshold strength for schmitt-trigger
> 
> 1. input-schmitt is missing! But it is right there in
> drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> using input-schmitt-enable/disable and -microvolt.
> 
> 2. input-schmitt-microvolt should probably be used separately
> to set the voltage threshold and can be used in conjunction
> with input-schmitt-enable in the same node. In your case
> you probably don't want to use it at all and disallow it.
> 
> They are all treated individually in the parser.
> 
> Maybe we could patch the docs in pinconf-generic.h to make it clear that
> they are all mutually exclusive.
> 
> The DT parser is a bit primitive for these.
> For example right now it is fine with the schema
> to set input-schmitt-enable and input-schmitt-disable at the same time, and
> the result will be enabled because of parse order :/

> The real trick would be to also make the
> schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> make them at least mutually exclusive and deprecate the
> input-schmitt that noone is using, maybe that is simpler than I think?

I think that this is probably what to do. Mutual exclusion isn't
difficult to set up there and if there's no property for "input-schmitt"
then deprecating it sounds pretty reasonable?

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:

> > I looked at the bindings that look like this and are not 1:1 to the
> > in-kernel configs:
> >
> >   input-schmitt-enable:
> >     type: boolean
> >     description: enable schmitt-trigger mode
> >
> >   input-schmitt-disable:
> >     type: boolean
> >     description: disable schmitt-trigger mode
> >
> >   input-schmitt-microvolt:
> >     description: threshold strength for schmitt-trigger
> >
> > 1. input-schmitt is missing! But it is right there in
> > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > using input-schmitt-enable/disable and -microvolt.
> >
> > 2. input-schmitt-microvolt should probably be used separately
> > to set the voltage threshold and can be used in conjunction
> > with input-schmitt-enable in the same node. In your case
> > you probably don't want to use it at all and disallow it.
> >
> > They are all treated individually in the parser.
> >
> > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > they are all mutually exclusive.
> >
> > The DT parser is a bit primitive for these.
> > For example right now it is fine with the schema
> > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > the result will be enabled because of parse order :/
>
> > The real trick would be to also make the
> > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > make them at least mutually exclusive and deprecate the
> > input-schmitt that noone is using, maybe that is simpler than I think?
>
> I think that this is probably what to do. Mutual exclusion isn't
> difficult to set up there and if there's no property for "input-schmitt"
> then deprecating it sounds pretty reasonable?

Yeah I agree.

Do you want to look into it?

Otherwise it becomes my problem now that I've noticed it :D

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > I looked at the bindings that look like this and are not 1:1 to the
> > > in-kernel configs:
> > >
> > >   input-schmitt-enable:
> > >     type: boolean
> > >     description: enable schmitt-trigger mode
> > >
> > >   input-schmitt-disable:
> > >     type: boolean
> > >     description: disable schmitt-trigger mode
> > >
> > >   input-schmitt-microvolt:
> > >     description: threshold strength for schmitt-trigger
> > >
> > > 1. input-schmitt is missing! But it is right there in
> > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > using input-schmitt-enable/disable and -microvolt.
> > >
> > > 2. input-schmitt-microvolt should probably be used separately
> > > to set the voltage threshold and can be used in conjunction
> > > with input-schmitt-enable in the same node. In your case
> > > you probably don't want to use it at all and disallow it.
> > >
> > > They are all treated individually in the parser.
> > >
> > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > they are all mutually exclusive.
> > >
> > > The DT parser is a bit primitive for these.
> > > For example right now it is fine with the schema
> > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > the result will be enabled because of parse order :/
> >
> > > The real trick would be to also make the
> > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > make them at least mutually exclusive and deprecate the
> > > input-schmitt that noone is using, maybe that is simpler than I think?
> >
> > I think that this is probably what to do. Mutual exclusion isn't
> > difficult to set up there and if there's no property for "input-schmitt"
> > then deprecating it sounds pretty reasonable?
> 
> Yeah I agree.
> 
> Do you want to look into it?
> 
> Otherwise it becomes my problem now that I've noticed it :D

Yeah, it's just a binding patch here I think, so yeah I'll do it.

> 
> Yours,
> Linus Walleij

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Fri, Nov 21, 2025 at 10:46:54AM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> > On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > > I looked at the bindings that look like this and are not 1:1 to the
> > > > in-kernel configs:
> > > >
> > > >   input-schmitt-enable:
> > > >     type: boolean
> > > >     description: enable schmitt-trigger mode
> > > >
> > > >   input-schmitt-disable:
> > > >     type: boolean
> > > >     description: disable schmitt-trigger mode
> > > >
> > > >   input-schmitt-microvolt:
> > > >     description: threshold strength for schmitt-trigger
> > > >
> > > > 1. input-schmitt is missing! But it is right there in
> > > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > > using input-schmitt-enable/disable and -microvolt.
> > > >
> > > > 2. input-schmitt-microvolt should probably be used separately
> > > > to set the voltage threshold and can be used in conjunction
> > > > with input-schmitt-enable in the same node. In your case
> > > > you probably don't want to use it at all and disallow it.
> > > >
> > > > They are all treated individually in the parser.
> > > >
> > > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > > they are all mutually exclusive.
> > > >
> > > > The DT parser is a bit primitive for these.
> > > > For example right now it is fine with the schema
> > > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > > the result will be enabled because of parse order :/
> > >
> > > > The real trick would be to also make the
> > > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > > make them at least mutually exclusive and deprecate the
> > > > input-schmitt that noone is using, maybe that is simpler than I think?
> > >
> > > I think that this is probably what to do. Mutual exclusion isn't
> > > difficult to set up there and if there's no property for "input-schmitt"
> > > then deprecating it sounds pretty reasonable?
> > 
> > Yeah I agree.
> > 
> > Do you want to look into it?
> > 
> > Otherwise it becomes my problem now that I've noticed it :D
> 
> Yeah, it's just a binding patch here I think, so yeah I'll do it.

ngl, I forget if there's a shorthand for the bias part, so I just want
to know if is this an accurate summary of what's exclusive?

diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
index cbfcf215e571..6865472ac124 100644
--- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
@@ -153,4 +153,66 @@ properties:
       pin. Typically indicates how many double-inverters are
       used to delay the signal.
 
+allOf:
+  - if:
+      required:
+        - output-disable
+    then:
+      properties:
+        output-enable: false
+        output-impedance-ohms: false
+  - if:
+      required:
+        - output-low
+    then:
+      properties:
+        output-high: false
+  - if:
+      required:
+        - low-power-enable
+    then:
+      properties:
+        low-power-disable: false
+  - if:
+      required:
+        - input-schmitt-disable
+    then:
+      properties:
+        input-schmitt-enable: false
+        input-schmitt-microvolt: false
+  - if:
+      required:
+        - drive-open-source
+    then:
+      properties:
+        drive-open-drain: false
+  - if:
+      anyOf:
+        - required:
+            - bias-disable
+        - required:
+            - bias-high-impedance
+        - required:
+            - bias-hold
+        - required:
+            - bias-up
+        - required:
+            - bias-down
+        - required:
+            - bias-pull-pin-default
+    then:
+      oneOf:
+        - required:
+            - bias-disable
+        - required:
+            - bias-high-impedance
+        - required:
+            - bias-hold
+        - required:
+            - bias-up
+        - required:
+            - bias-down
+        - required:
+            - bias-pull-pin-default
+
 additionalProperties: true



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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Fri, Nov 21, 2025 at 11:21:38AM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 10:46:54AM +0000, Conor Dooley wrote:
> > On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> > > On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > > > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > > 
> > > > > I looked at the bindings that look like this and are not 1:1 to the
> > > > > in-kernel configs:
> > > > >
> > > > >   input-schmitt-enable:
> > > > >     type: boolean
> > > > >     description: enable schmitt-trigger mode
> > > > >
> > > > >   input-schmitt-disable:
> > > > >     type: boolean
> > > > >     description: disable schmitt-trigger mode
> > > > >
> > > > >   input-schmitt-microvolt:
> > > > >     description: threshold strength for schmitt-trigger
> > > > >
> > > > > 1. input-schmitt is missing! But it is right there in
> > > > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > > > using input-schmitt-enable/disable and -microvolt.
> > > > >
> > > > > 2. input-schmitt-microvolt should probably be used separately
> > > > > to set the voltage threshold and can be used in conjunction
> > > > > with input-schmitt-enable in the same node. In your case
> > > > > you probably don't want to use it at all and disallow it.
> > > > >
> > > > > They are all treated individually in the parser.
> > > > >
> > > > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > > > they are all mutually exclusive.
> > > > >
> > > > > The DT parser is a bit primitive for these.
> > > > > For example right now it is fine with the schema
> > > > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > > > the result will be enabled because of parse order :/
> > > >
> > > > > The real trick would be to also make the
> > > > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > > > make them at least mutually exclusive and deprecate the
> > > > > input-schmitt that noone is using, maybe that is simpler than I think?
> > > >
> > > > I think that this is probably what to do. Mutual exclusion isn't
> > > > difficult to set up there and if there's no property for "input-schmitt"
> > > > then deprecating it sounds pretty reasonable?
> > > 
> > > Yeah I agree.
> > > 
> > > Do you want to look into it?
> > > 
> > > Otherwise it becomes my problem now that I've noticed it :D
> > 
> > Yeah, it's just a binding patch here I think, so yeah I'll do it.
> 
> ngl, I forget if there's a shorthand for the bias part, so I just want
> to know if is this an accurate summary of what's exclusive?
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> index cbfcf215e571..6865472ac124 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> @@ -153,4 +153,66 @@ properties:
>        pin. Typically indicates how many double-inverters are
>        used to delay the signal.
>  
> +allOf:
> +  - if:
> +      required:
> +        - output-disable
> +    then:
> +      properties:
> +        output-enable: false
> +        output-impedance-ohms: false
> +  - if:
> +      required:
> +        - output-low
> +    then:
> +      properties:
> +        output-high: false
> +  - if:
> +      required:
> +        - low-power-enable
> +    then:
> +      properties:
> +        low-power-disable: false
> +  - if:
> +      required:
> +        - input-schmitt-disable
> +    then:
> +      properties:
> +        input-schmitt-enable: false
> +        input-schmitt-microvolt: false
> +  - if:
> +      required:
> +        - drive-open-source
> +    then:
> +      properties:
> +        drive-open-drain: false
> +  - if:
> +      anyOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default
> +    then:
> +      oneOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default
> +
>  additionalProperties: true

I was looking at the kernel part of this today, trying to figure out
where it would make sense to actually check this, but I'm not super keen
on what has to be done. I think doing it in parse_dt_cfg() makes the
most sense, setting flags if the property is one we care about during
the loop and then checking mutual exclusion at the end based on the
flags? The gpio example you gave has it easy, since they already appear
to have these things stored in flag properties.
Is there somewhere else, in addition to creating the config from dt that
this would have to be checked?

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:

> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> > +                                      struct pinctrl_map **maps, unsigned int *num_maps)
> 
> I saw in the cover letter that you wanted this to use more generic helpers.
> 
> If you see room for improvement of the generic code, do not hesitate.
> Doing a new driver is the only time when you actually have all these
> details in your head and can create good helpers.

Started looking at this today too, and I found one of my sources of
confusion - the recently added helper which I think is confusingly
named. pinconf_generic_dt_node_to_map_pinmux() works differently to
pinconf_generic_dt_node_to_map(), because it only works if you have
the following setup:

label: group {
	pinmux = <asjhdasjhlajskd>;
	config-item1;
};

It does not work if you have:

label: cfg {
	group1 {
		pinmux = <dsjhlfka>;
		config-item2;
	};
	group2 {
		pinmux = <lsdjhaf>;
		config-item1;
	};
};

Specifically, the label must point to a group.
pinconf_generic_dt_node_to_map() does not work like this, it accepts both!
I think the pinconf_generic_dt_node_to_map_pinmux() function should
actually be called pinconf_generic_dt_subnode_to_map_pinmux(), because
it operates at the same level as pinconf_generic_dt_subnode_to_map().

Probably there should be a "real" pinconf_generic_dt_node_to_map() that
accepts both setups, since AFAICT it is pretty normal to have different
pins in a group that get different pinconf settings. Obviously

label: cfg {
	group1 {
		pinmux = <dsjhlfka>;
		config-item2;
	};
	group2 {
		pinmux = <lsdjhaf>;
		config-item1;
	};
};

peripheral {
	pinctrl-0 = <&label>;
}

isn't the only way to do things, and the amlogic user of the current
setup could just go and do

cfg {
	label1: group1 {
		pinmux = <dsjhlfka>;
		config-item2;
	};
	label2: group2 {
		pinmux = <lsdjhaf>;
		config-item1;
	};
};

peripheral {
	pinctrl-0 = <&label1>, <&label2>;
}

if it never needs more than one set of configs so this isn't a bug in
the amlogic stuff, just something I found misleading while trying to
make my own helper.

Even then though, I'm not really sure that this function does what I
would have expected it to do, because it won't work as a replacement for
the custom dt_node_to_map in the spacemit k1 driver, for example, even
ignoring the requirement about how the labels are done in the dt. That's
because it doesn't actually do anything with the pinmux property, despite
that being in the name. It never actually interacts with the pinmux property
at all AFAICT! It seems to depend on aml_pctl_parse_functions() being called
during probe which creates the groups and functions.
There's a weird warning about expecting a function parent node that seems
very amlogic specific too.

In my eyes, there should be some generic dt_node_to_map helpers that
do it all for you on the "configuration entirely described in dt"
platforms because that's what stuff like spacemit k1 driver that do
this in their dt_node_to_map implementations.

I'm not gonna get in over my head, and just make a helper for doing the
pins + function thing that I need for my driver, but would you be open
to an equivalent for the pinmux scenario? I'm thinking of something
that'd work for both the amlogic platform and for the spacemit k1.

Cheers,
Conor.

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Fri, Nov 21, 2025 at 12:21 PM Conor Dooley <conor@kernel.org> wrote:

> ngl, I forget if there's a shorthand for the bias part, so I just want
> to know if is this an accurate summary of what's exclusive?
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> index cbfcf215e571..6865472ac124 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> @@ -153,4 +153,66 @@ properties:
>        pin. Typically indicates how many double-inverters are
>        used to delay the signal.
>
> +allOf:
> +  - if:
> +      required:
> +        - output-disable
> +    then:
> +      properties:
> +        output-enable: false
> +        output-impedance-ohms: false

Looks right.

> +  - if:
> +      required:
> +        - output-low
> +    then:
> +      properties:
> +        output-high: false

Looks right.

> +  - if:
> +      required:
> +        - low-power-enable
> +    then:
> +      properties:
> +        low-power-disable: false

Looks right.

> +  - if:
> +      required:
> +        - input-schmitt-disable
> +    then:
> +      properties:
> +        input-schmitt-enable: false
> +        input-schmitt-microvolt: false

Looks right.

> +  - if:
> +      required:
> +        - drive-open-source
> +    then:
> +      properties:
> +        drive-open-drain: false

drive-push-pull is mutually exclusive
with each of these as well.

> +  - if:
> +      anyOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default
> +    then:
> +      oneOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default

These is a bunch of "pull" infixes missing from the
above.

After looking at it for a while I concluded this
is right as well, if just the right names are added.
I would add a comment like

# We can only ever allow exactly one of these,
# they are all mutually exclusive.

Additionally:

drive-strength and drive-strength-microamp are mutually
exclusive.

input-enable and input-disable are mutually exclusive.

low-power-enable and low-power-disable are mutually
exclusive.

input-schmitt need to be added as deprecated.

Can you cook a patch? Maybe test it on the existing
device trees first to see that it doesn't wreac havoc.

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Tue, Nov 25, 2025 at 01:10:26AM +0100, Linus Walleij wrote:
> On Fri, Nov 21, 2025 at 12:21 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > ngl, I forget if there's a shorthand for the bias part, so I just want
> > to know if is this an accurate summary of what's exclusive?
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > index cbfcf215e571..6865472ac124 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > @@ -153,4 +153,66 @@ properties:
> >        pin. Typically indicates how many double-inverters are
> >        used to delay the signal.
> >
> > +allOf:
> > +  - if:
> > +      required:
> > +        - output-disable
> > +    then:
> > +      properties:
> > +        output-enable: false
> > +        output-impedance-ohms: false
> 
> Looks right.
> 
> > +  - if:
> > +      required:
> > +        - output-low
> > +    then:
> > +      properties:
> > +        output-high: false
> 
> Looks right.
> 
> > +  - if:
> > +      required:
> > +        - low-power-enable
> > +    then:
> > +      properties:
> > +        low-power-disable: false
> 
> Looks right.
> 
> > +  - if:
> > +      required:
> > +        - input-schmitt-disable
> > +    then:
> > +      properties:
> > +        input-schmitt-enable: false
> > +        input-schmitt-microvolt: false
> 
> Looks right.
> 
> > +  - if:
> > +      required:
> > +        - drive-open-source
> > +    then:
> > +      properties:
> > +        drive-open-drain: false
> 
> drive-push-pull is mutually exclusive
> with each of these as well.
> 
> > +  - if:
> > +      anyOf:
> > +        - required:
> > +            - bias-disable
> > +        - required:
> > +            - bias-high-impedance
> > +        - required:
> > +            - bias-hold
> > +        - required:
> > +            - bias-up
> > +        - required:
> > +            - bias-down
> > +        - required:
> > +            - bias-pull-pin-default
> > +    then:
> > +      oneOf:
> > +        - required:
> > +            - bias-disable
> > +        - required:
> > +            - bias-high-impedance
> > +        - required:
> > +            - bias-hold
> > +        - required:
> > +            - bias-up
> > +        - required:
> > +            - bias-down
> > +        - required:
> > +            - bias-pull-pin-default
> 
> These is a bunch of "pull" infixes missing from the
> above.

lol, that's silly to miss.

> After looking at it for a while I concluded this
> is right as well, if just the right names are added.
> I would add a comment like
> 
> # We can only ever allow exactly one of these,
> # they are all mutually exclusive.
> 
> Additionally:
> 
> drive-strength and drive-strength-microamp are mutually
> exclusive.

Oh right yea, didn't notice that one.

> input-enable and input-disable are mutually exclusive.

And this one I meant to do and forgot.

> low-power-enable and low-power-disable are mutually
> exclusive.

This I did do.

> input-schmitt need to be added as deprecated.

I figured the best way to deprecate it was not add it at all to the
binding? Deprecated properties don't warn, undocumented ones do.

> Can you cook a patch? Maybe test it on the existing
> device trees first to see that it doesn't wreac havoc.

Yeah, I'll send it along with whatever the code equivalent ends up
being. I tested against riscv, which is a tiny sample. I'll probably do
it on the arms before submitting and see.


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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Mon, Nov 24, 2025 at 6:16 PM Conor Dooley <conor@kernel.org> wrote:

> I was looking at the kernel part of this today, trying to figure out
> where it would make sense to actually check this, but I'm not super keen
> on what has to be done. I think doing it in parse_dt_cfg() makes the
> most sense, setting flags if the property is one we care about during
> the loop and then checking mutual exclusion at the end based on the
> flags? The gpio example you gave has it easy, since they already appear
> to have these things stored in flag properties.
> Is there somewhere else, in addition to creating the config from dt that
> this would have to be checked?


We are right now parsing with an array of
struct pinconf_generic_params:

static const struct pinconf_generic_params dt_params[] = {
    { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
    (...)
};

(Sorry for not using C99 .initializers on the above array)

Struct looks like so:

struct pinconf_generic_params {
        const char * const property;
        enum pin_config_param param;
        u32 default_value;
        const char * const *values;
        size_t num_values;
};

Can't we add a
    const enum pin_config_param *conflicts;
    size_t num_conflicts;

And rewrite the parsing table to be more explicit:

static const char * const input_disable_conflicts[] = {
    "input-enable",
};

static const struct pinconf_generic_params dt_params[] = {
    {
        .property = "input-disable",
        .param = PIN_CONFIG_INPUT_ENABLE,
        .default_value = 0,
        .conflicting_properties = input_disable_conflicts,
        .num_conflicting_properties = ARRAY_SIZE(input_disable_conflicts),
     },
    (...)
};

Then in the loop we can use of_property_present(np, ...) to check for
conflicting properties when we encounter something.

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Tue, Nov 25, 2025 at 01:31:49AM +0100, Linus Walleij wrote:
> On Mon, Nov 24, 2025 at 6:16 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > I was looking at the kernel part of this today, trying to figure out
> > where it would make sense to actually check this, but I'm not super keen
> > on what has to be done. I think doing it in parse_dt_cfg() makes the
> > most sense, setting flags if the property is one we care about during
> > the loop and then checking mutual exclusion at the end based on the
> > flags? The gpio example you gave has it easy, since they already appear
> > to have these things stored in flag properties.
> > Is there somewhere else, in addition to creating the config from dt that
> > this would have to be checked?
> 
> 
> We are right now parsing with an array of
> struct pinconf_generic_params:
> 
> static const struct pinconf_generic_params dt_params[] = {
>     { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
>     (...)
> };
> 
> (Sorry for not using C99 .initializers on the above array)
> 
> Struct looks like so:
> 
> struct pinconf_generic_params {
>         const char * const property;
>         enum pin_config_param param;
>         u32 default_value;
>         const char * const *values;
>         size_t num_values;
> };
> 
> Can't we add a
>     const enum pin_config_param *conflicts;
>     size_t num_conflicts;
> 
> And rewrite the parsing table to be more explicit:
> 
> static const char * const input_disable_conflicts[] = {
>     "input-enable",
> };
> 
> static const struct pinconf_generic_params dt_params[] = {
>     {
>         .property = "input-disable",
>         .param = PIN_CONFIG_INPUT_ENABLE,
>         .default_value = 0,
>         .conflicting_properties = input_disable_conflicts,
>         .num_conflicting_properties = ARRAY_SIZE(input_disable_conflicts),
>      },
>     (...)
> };
> 
> Then in the loop we can use of_property_present(np, ...) to check for
> conflicting properties when we encounter something.

ngl, I didn't consider something along these lines cos I was trying not
to disrupt how things work at the moment and slot in some non-invasive.
That's probably a bad mindset.

I'm not sure how keen I would be on this of_property_present() idea
though, feels like wasteful to search the dt for conflicting properties
when we are already going through all possible properties and can do the
check at the end, when we've got all the information? Could probably
keep a temporay bitmap, using pin_config_param as the index, and use
test_bit() using the known-incompatibles to check it somewhat neatly?

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Mon, Nov 24, 2025 at 8:14 PM Conor Dooley <conor@kernel.org> wrote:

> Started looking at this today too, and I found one of my sources of
> confusion - the recently added helper which I think is confusingly
> named. pinconf_generic_dt_node_to_map_pinmux() works differently to
> pinconf_generic_dt_node_to_map(), because it only works if you have
> the following setup:
>
> label: group {
>         pinmux = <asjhdasjhlajskd>;
>         config-item1;
> };
>
> It does not work if you have:
>
> label: cfg {
>         group1 {
>                 pinmux = <dsjhlfka>;
>                 config-item2;
>         };
>         group2 {
>                 pinmux = <lsdjhaf>;
>                 config-item1;
>         };
> };
>
> Specifically, the label must point to a group.
> pinconf_generic_dt_node_to_map() does not work like this, it accepts both!

My feeling is that this is a bug, it should certainly handle configs
with subnodes.

> I think the pinconf_generic_dt_node_to_map_pinmux() function should
> actually be called pinconf_generic_dt_subnode_to_map_pinmux(), because
> it operates at the same level as pinconf_generic_dt_subnode_to_map().

If it should be renamed, yes. But I think it should be fixed to
parse subnodes, if present.

> Probably there should be a "real" pinconf_generic_dt_node_to_map() that
> accepts both setups, since AFAICT it is pretty normal to have different
> pins in a group that get different pinconf settings. Obviously

I think it should be fine to augment the existing function to handle
both cases? (configs inside the current node or in subnodes
alike) I don't see it causing any regressions.

> label: cfg {
>         group1 {
>                 pinmux = <dsjhlfka>;
>                 config-item2;
>         };
>         group2 {
>                 pinmux = <lsdjhaf>;
>                 config-item1;
>         };
> };
>
> peripheral {
>         pinctrl-0 = <&label>;
> }
>
> isn't the only way to do things, and the amlogic user of the current
> setup could just go and do
>
> cfg {
>         label1: group1 {
>                 pinmux = <dsjhlfka>;
>                 config-item2;
>         };
>         label2: group2 {
>                 pinmux = <lsdjhaf>;
>                 config-item1;
>         };
> };
>
> peripheral {
>         pinctrl-0 = <&label1>, <&label2>;
> }

That works too, because sometimes you want to pick a few
different configs and collect them as one.

> Even then though, I'm not really sure that this function does what I
> would have expected it to do, because it won't work as a replacement for
> the custom dt_node_to_map in the spacemit k1 driver, for example, even
> ignoring the requirement about how the labels are done in the dt. That's
> because it doesn't actually do anything with the pinmux property, despite
> that being in the name. It never actually interacts with the pinmux property
> at all AFAICT!

I think it's unfortunate naming, people sometimes use the word
"pinmux" as a DT property, sometimes to describe the subsystem,
sometimes a part of the subsystem, sometimes anything related
to pins.

I know I should perhaps have shepherded this better :/

> It seems to depend on aml_pctl_parse_functions() being called
> during probe which creates the groups and functions.
> There's a weird warning about expecting a function parent node that seems
> very amlogic specific too.
>
> In my eyes, there should be some generic dt_node_to_map helpers that
> do it all for you on the "configuration entirely described in dt"
> platforms because that's what stuff like spacemit k1 driver that do
> this in their dt_node_to_map implementations.

I think you're right!

> I'm not gonna get in over my head, and just make a helper for doing the
> pins + function thing that I need for my driver, but would you be open
> to an equivalent for the pinmux scenario?

Yes!

> I'm thinking of something
> that'd work for both the amlogic platform and for the spacemit k1.

That's a good start!

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Tue, Nov 25, 2025 at 2:03 AM Conor Dooley <conor@kernel.org> wrote:

> I'm not sure how keen I would be on this of_property_present() idea
> though, feels like wasteful to search the dt for conflicting properties
> when we are already going through all possible properties and can do the
> check at the end, when we've got all the information? Could probably
> keep a temporay bitmap, using pin_config_param as the index, and use
> test_bit() using the known-incompatibles to check it somewhat neatly?

I think it could work, as all conflicts (I think?) are boolean (this OR that).

It's probably a bit ambitious, but I'm game :D

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Tue, Nov 25, 2025 at 02:24:55PM +0100, Linus Walleij wrote:
> On Mon, Nov 24, 2025 at 8:14 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > Started looking at this today too, and I found one of my sources of
> > confusion - the recently added helper which I think is confusingly
> > named. pinconf_generic_dt_node_to_map_pinmux() works differently to
> > pinconf_generic_dt_node_to_map(), because it only works if you have
> > the following setup:
> >
> > label: group {
> >         pinmux = <asjhdasjhlajskd>;
> >         config-item1;
> > };
> >
> > It does not work if you have:
> >
> > label: cfg {
> >         group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > Specifically, the label must point to a group.
> > pinconf_generic_dt_node_to_map() does not work like this, it accepts both!
> 
> My feeling is that this is a bug, it should certainly handle configs
> with subnodes.
> 
> > I think the pinconf_generic_dt_node_to_map_pinmux() function should
> > actually be called pinconf_generic_dt_subnode_to_map_pinmux(), because
> > it operates at the same level as pinconf_generic_dt_subnode_to_map().
> 
> If it should be renamed, yes. But I think it should be fixed to
> parse subnodes, if present.
> 
> > Probably there should be a "real" pinconf_generic_dt_node_to_map() that
> > accepts both setups, since AFAICT it is pretty normal to have different
> > pins in a group that get different pinconf settings. Obviously
> 
> I think it should be fine to augment the existing function to handle
> both cases? (configs inside the current node or in subnodes
> alike) I don't see it causing any regressions.

I think the rename to add subnode actually makes sense, as it would
match how the code is broken down for pinconf_generic_dt_node_to_map(),
since the code in the current pinconf_generic_dt_node_to_map_pinmux()
would have to be executed in two different places. That'd be invisible
to the amlogic driver though, if done properly, since a
pinconf_generic_dt_node_to_map_pinmux() would still exist.

> 
> > label: cfg {
> >         group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > peripheral {
> >         pinctrl-0 = <&label>;
> > }
> >
> > isn't the only way to do things, and the amlogic user of the current
> > setup could just go and do
> >
> > cfg {
> >         label1: group1 {
> >                 pinmux = <dsjhlfka>;
> >                 config-item2;
> >         };
> >         label2: group2 {
> >                 pinmux = <lsdjhaf>;
> >                 config-item1;
> >         };
> > };
> >
> > peripheral {
> >         pinctrl-0 = <&label1>, <&label2>;
> > }
> 
> That works too, because sometimes you want to pick a few
> different configs and collect them as one.
> 
> > Even then though, I'm not really sure that this function does what I
> > would have expected it to do, because it won't work as a replacement for
> > the custom dt_node_to_map in the spacemit k1 driver, for example, even
> > ignoring the requirement about how the labels are done in the dt. That's
> > because it doesn't actually do anything with the pinmux property, despite
> > that being in the name. It never actually interacts with the pinmux property
> > at all AFAICT!
> 
> I think it's unfortunate naming, people sometimes use the word
> "pinmux" as a DT property, sometimes to describe the subsystem,
> sometimes a part of the subsystem, sometimes anything related
> to pins.

I think I actually understand the naming now. It's called pinmux because
the existing function pinconf_generic_dt_node_to_map() doesn't support
pinmux, so this is the version you need for platforms that are using
pinmux. But then nothing about it limits it actually to pinmuxes, other
than arbitrary property checks, it could actually be used for my pins +
functions use-case, if I added similar code to amlogics in my probe
function that creates the functions and groups.

I still think the naming is poor though, and that it is not as generic as
it purports to be, since it depends on having the exact dt configuration
that amlogic has, and wouldn't work for spacemit that use the first
multi-group example that I gave above. I'd be inclined to say that it
should be shunted back to the amlogic driver, to avoid baiting people
into trying to use it because of the label position problem, the fact
that it requires parsing the dt twice (the first time in probe to
generate the groups/functions), and because the code uses the parent
of node as the pinmux function. IOW, it needs:

pinctrl@bla {

	func-foo {
		label: group-default {
			pinmuxes = <lskdf>;
		};
	};
};

and couldn't be used somewhere like:

pinctrl@bla {

	label: group-default {
		pinmuxes = <lskdf>;
	};
};

Probably there's very few places where this second example makes sense
(cos it requires all pins in a group to always have the same config
parameters, but I could definitely see that being possible.

So ye, proposing moving it back from whence it came, rather than
actually making it work for both label positions.

> I know I should perhaps have shepherded this better :/

idk, I think this is the usual "creating something generic but with only
one user" problem. Hard to know if it actually is generic at all.

> > It seems to depend on aml_pctl_parse_functions() being called
> > during probe which creates the groups and functions.
> > There's a weird warning about expecting a function parent node that seems
> > very amlogic specific too.
> >
> > In my eyes, there should be some generic dt_node_to_map helpers that
> > do it all for you on the "configuration entirely described in dt"
> > platforms because that's what stuff like spacemit k1 driver that do
> > this in their dt_node_to_map implementations.
> 
> I think you're right!

My dilemma now is what to call them and where to put them.
pinconf_generic_dt_node_to_map<something>() feels weird for something
that is also creating functions and groups, which I noticed because I
was having to include pinmux.h in pinconf.c so that I could call
pinmux_generic_add_function().

> 
> > I'm not gonna get in over my head, and just make a helper for doing the
> > pins + function thing that I need for my driver, but would you be open
> > to an equivalent for the pinmux scenario?
> 
> Yes!
> 
> > I'm thinking of something
> > that'd work for both the amlogic platform and for the spacemit k1.
> 
> That's a good start!
> 
> Yours,
> Linus Walleij

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Tue, Nov 25, 2025 at 6:47 PM Conor Dooley <conor@kernel.org> wrote:

> > I think it's unfortunate naming, people sometimes use the word
> > "pinmux" as a DT property, sometimes to describe the subsystem,
> > sometimes a part of the subsystem, sometimes anything related
> > to pins.
>
> I think I actually understand the naming now. It's called pinmux because
> the existing function pinconf_generic_dt_node_to_map() doesn't support
> pinmux, so this is the version you need for platforms that are using
> pinmux. But then nothing about it limits it actually to pinmuxes, other
> than arbitrary property checks, it could actually be used for my pins +
> functions use-case, if I added similar code to amlogics in my probe
> function that creates the functions and groups.
>
> I still think the naming is poor though, and that it is not as generic as
> it purports to be, since it depends on having the exact dt configuration
> that amlogic has, and wouldn't work for spacemit that use the first
> multi-group example that I gave above. I'd be inclined to say that it
> should be shunted back to the amlogic driver, to avoid baiting people
> into trying to use it because of the label position problem,

You're probably right. I see the problem here.

> > I know I should perhaps have shepherded this better :/
>
> idk, I think this is the usual "creating something generic but with only
> one user" problem. Hard to know if it actually is generic at all.

The problem is mostly too few people working on genericizing
the pinctrl code I think, it makes me be happy about any such
attempts. But I should pay more attention, clearly I just looked
at it superficially.

> > > It seems to depend on aml_pctl_parse_functions() being called
> > > during probe which creates the groups and functions.
> > > There's a weird warning about expecting a function parent node that seems
> > > very amlogic specific too.
> > >
> > > In my eyes, there should be some generic dt_node_to_map helpers that
> > > do it all for you on the "configuration entirely described in dt"
> > > platforms because that's what stuff like spacemit k1 driver that do
> > > this in their dt_node_to_map implementations.
> >
> > I think you're right!
>
> My dilemma now is what to call them and where to put them.
> pinconf_generic_dt_node_to_map<something>() feels weird for something
> that is also creating functions and groups, which I noticed because I
> was having to include pinmux.h in pinconf.c so that I could call
> pinmux_generic_add_function().

pinctrl_generic_dt_node_parse_config() or so? Is it vague enough?

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Tue, Nov 25, 2025 at 08:28:10PM +0100, Linus Walleij wrote:
> On Tue, Nov 25, 2025 at 6:47 PM Conor Dooley <conor@kernel.org> wrote:

> > > > It seems to depend on aml_pctl_parse_functions() being called
> > > > during probe which creates the groups and functions.
> > > > There's a weird warning about expecting a function parent node that seems
> > > > very amlogic specific too.
> > > >
> > > > In my eyes, there should be some generic dt_node_to_map helpers that
> > > > do it all for you on the "configuration entirely described in dt"
> > > > platforms because that's what stuff like spacemit k1 driver that do
> > > > this in their dt_node_to_map implementations.
> > >
> > > I think you're right!
> >
> > My dilemma now is what to call them and where to put them.
> > pinconf_generic_dt_node_to_map<something>() feels weird for something
> > that is also creating functions and groups, which I noticed because I
> > was having to include pinmux.h in pinconf.c so that I could call
> > pinmux_generic_add_function().
> 
> pinctrl_generic_dt_node_parse_config() or so? Is it vague enough?

Probably too vague, since it's gonna be pins + functions specific, but
I'll do something along these lines. Where should I put it? Leave it in
pinconf-generic, but do some gating of it to only be exposed for configs
with GENERIC_PINMUX_FUNCTIONS enabled?

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Tue, Nov 25, 2025 at 8:55 PM Conor Dooley <conor@kernel.org> wrote:

> Probably too vague, since it's gonna be pins + functions specific, but
> I'll do something along these lines. Where should I put it? Leave it in
> pinconf-generic, but do some gating of it to only be exposed for configs
> with GENERIC_PINMUX_FUNCTIONS enabled?

Yeah just add something new and a whole new file like that, we will
probably add more stuff there as time goes.

Yours,
Linus Walleij