gpio: introduce a gpio driver for SCMI

This basically abandons my earlier attempts and goes back to Takahiro Akashi's driver. Here is the link to Takahiro's patchset:

https://lore.kernel.org/all/[email protected]/

In the review comments, to that patchset we had discussed putting the gpio section inside the pinctrl section. The ordering problems are a bit tricky because you want the pinctrl driver to finish probing before you start probing the gpio driver. To me it seems nicer to put the pinctrl things such as pinmuxing in the pinctrl block and the gpio things in the gpio block.

I updated Takahiro's patch to work on current kernels. I've had added a few other patches to make things work on current kernels. The most noteworthy change is that instead of calculating the ngpios, I changed it so you have to specify the ngpios in the device tree.

I updated the device tree spec file to address review comments. I changed the compatible to scmi-pinctrl-gpio. I also updated the examples to show how pinmuxing works. I didn't know how to include all the potential GPIO configuration options so I set "additionalProperties: true". Hopefully, that's okay.

AKASHI Takahiro (3):
pinctrl: introduce pinctrl_gpio_get_config()
dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver
gpio: add pinctrl based generic gpio driver

Dan Carpenter (4):
pinctrl: scmi: Add SCMI_PIN_INPUT_VALUE
pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support
pinctrl-scmi: ignore PIN_CONFIG_PERSIST_STATE
arm_scmi: pinctrl: allow PINCTRL_REQUEST to return EOPNOTSUPP

 .../bindings/gpio/pin-control-gpio.yaml       |  70 ++++++++++
 drivers/firmware/arm_scmi/pinctrl.c           |   2 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-by-pinctrl.c                | 124 ++++++++++++++++++
 drivers/pinctrl/core.c                        |  30 +++++
 drivers/pinctrl/pinctrl-scmi.c                |  46 +++++--
 include/linux/pinctrl/consumer.h              |   9 ++
 8 files changed, 276 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
 create mode 100644 drivers/gpio/gpio-by-pinctrl.c

2.51.0

From: AKASHI Takahiro <[email protected]>

This is a counterpart of pinctrl_gpio_set_config(), which will be used to implement the ->get() interface in a GPIO driver for SCMI.

---
 drivers/pinctrl/core.c           | 30 ++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h |  9 +++++++++
 2 files changed, 39 insertions(+)

index b5e97689589f..732ca6785c67 100644
@@ -30,6 +30,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/pinctrl/devinfo.h>
 #include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 
 #include "core.h"

@@ -938,6 +939,35 @@ int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 }
 EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
 
+/**
+ * pinctrl_gpio_get_config() - Get the config for a given GPIO pin
+ * @gc: GPIO chip structure from the GPIO subsystem
+ * @offset: hardware offset of the GPIO relative to the controller
+ * @config: the configuration to query.  On success it holds the result
+ */
+int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
+{
+	struct pinctrl_gpio_range *range;
+	struct pinctrl_dev *pctldev;
+	int ret, pin;
+
+	ret = pinctrl_get_device_gpio_range(gc, offset, &pctldev, &range);
+	if (ret)
+		return ret;
+
+	mutex_lock(&pctldev->mutex);
+	pin = gpio_to_pin(range, gc, offset);
+	ret = pin_config_get_for_pin(pctldev, pin, config);
+	mutex_unlock(&pctldev->mutex);
+
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_argument(*config);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
+
 static struct pinctrl_state *find_state(struct pinctrl *p,
 					const char *name)
 {

index 63ce16191eb9..11b8f0b8da0c 100644
@@ -35,6 +35,8 @@ int pinctrl_gpio_direction_output(struct gpio_chip *gc,
 				  unsigned int offset);
 int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 				unsigned long config);
+int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset,
+			    unsigned long *config);
 
 struct pinctrl * __must_check pinctrl_get(struct device *dev);
 void pinctrl_put(struct pinctrl *p);

@@ -101,6 +103,13 @@ pinctrl_gpio_direction_output(struct gpio_chip *gc, unsigned int offset)
 	return 0;
 }
 
+static inline int
+pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset,
+			unsigned long *config)
+{
+	return 0;
+}
+
 static inline int
 pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config)

2.51.0

...

kernel-doc is not happy: return section is missing. Always run it with -Wall to see that.

...

I'm also wondering if this needs to be used in / already part of UP board pin control and GPIO driver. There they used a proxy based on gpio-aggregator code. With this it might be (I haven't checked at all) that the code can be simplified.

With Best Regards,
Andy Shevchenko

If you fix the kerneldoc as pointed out by Andy:

Yours,
Linus Walleij

The PIN_CONFIG_LEVEL parameter represents the value of the pin, whether reading or writing to the pin. In SCMI, the parameter is represented by two different values SCMI_PIN_OUTPUT_VALUE for writing to a pin and SCMI_PIN_INPUT_VALUE for reading. The current code translates PIN_CONFIG_LEVEL as SCMI_PIN_OUTPUT_VALUE (writing).

Add a function to translate it to either INPUT or OUTPUT depending on whether it is called from a _get or _set() operation.

---
 drivers/pinctrl/pinctrl-scmi.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

index f4f296e07be5..5d347e6b2e4c 100644
@@ -251,9 +251,6 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	case PIN_CONFIG_MODE_LOW_POWER:
 		*type = SCMI_PIN_LOW_POWER_MODE;
 		break;
-	case PIN_CONFIG_LEVEL:
-		*type = SCMI_PIN_OUTPUT_VALUE;
-		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
 		*type = SCMI_PIN_OUTPUT_MODE;
 		break;

@@ -276,6 +273,28 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	return 0;
 }
 
+static int pinctrl_scmi_map_pinconf_type_get(enum pin_config_param param,
+					     enum scmi_pinctrl_conf_type *type)
+{
+	if (param == PIN_CONFIG_LEVEL) {
+		*type = SCMI_PIN_INPUT_VALUE;
+		return 0;
+	}
+
+	return pinctrl_scmi_map_pinconf_type(param, type);
+}
+
+static int pinctrl_scmi_map_pinconf_type_set(enum pin_config_param param,
+					     enum scmi_pinctrl_conf_type *type)
+{
+	if (param == PIN_CONFIG_LEVEL) {
+		*type = SCMI_PIN_OUTPUT_VALUE;
+		return 0;
+	}
+
+	return pinctrl_scmi_map_pinconf_type(param, type);
+}
+
 static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
 				    unsigned int pin, unsigned long *config)
 {

@@ -290,7 +309,7 @@ static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
 
 	config_type = pinconf_to_config_param(*config);
 
-	ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+	ret = pinctrl_scmi_map_pinconf_type_get(config_type, &type);
 	if (ret)
 		return ret;
 

@@ -363,7 +382,7 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
-		ret = pinctrl_scmi_map_pinconf_type(param, &p_config_type[i]);
+		ret = pinctrl_scmi_map_pinconf_type_set(param, &p_config_type[i]);
 		if (ret) {
 			dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
 			goto free_config;

@@ -405,7 +424,7 @@ static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
-		ret = pinctrl_scmi_map_pinconf_type(param, &p_config_type[i]);
+		ret = pinctrl_scmi_map_pinconf_type_set(param, &p_config_type[i]);
 		if (ret) {
 			dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
 			goto free_config;

@@ -440,7 +459,7 @@ static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 
 	config_type = pinconf_to_config_param(*config);
-	ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+	ret = pinctrl_scmi_map_pinconf_type_get(config_type, &type);
 	if (ret) {
 		dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
 		return ret;

2.51.0

I think you nailed it :)

Yours,
Linus Walleij

The argument for PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS is supposed to be expressed in terms of ohms. But the pinctrl-scmi driver was implementing it the same as PIN_CONFIG_OUTPUT and writing either a zero or one to the pin.

The SCMI protocol doesn't have an support configuration type so just delete this code instead of replacing it.

---
Hi Peng, could you Ack this patch? I could drop it, since obviously it doesn't affect my stuff at all but it's the right thing to do.

 drivers/pinctrl/pinctrl-scmi.c | 3 ---
 1 file changed, 3 deletions(-)

index 5d347e6b2e4c..de8c113bc61d 100644
@@ -254,9 +254,6 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	case PIN_CONFIG_OUTPUT_ENABLE:
 		*type = SCMI_PIN_OUTPUT_MODE;
 		break;
-	case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
-		*type = SCMI_PIN_OUTPUT_VALUE;
-		break;
 	case PIN_CONFIG_POWER_SOURCE:
 		*type = SCMI_PIN_POWER_SOURCE;
 		break;

2.51.0

Yours,
Linus Walleij

The PIN_CONFIG_PERSIST_STATE setting ensures that the pin state persists across a sleep or controller reset. The SCMI spec does not have an equivalent command to this so just ignore it.

---
 drivers/pinctrl/pinctrl-scmi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index de8c113bc61d..f22be6b7b82a 100644
@@ -361,7 +361,7 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
 				    unsigned long *configs,
 				    unsigned int num_configs)
 {
-	int i, ret;
+	int i, cnt, ret;
 	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
 	enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
 	u32 config_value[SCMI_NUM_CONFIGS];

@@ -377,17 +377,21 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
 	if (ret)
 		return ret;
 
+	cnt = 0;
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
-		ret = pinctrl_scmi_map_pinconf_type_set(param, &p_config_type[i]);
+		if (param == PIN_CONFIG_PERSIST_STATE)
+			continue;
+		ret = pinctrl_scmi_map_pinconf_type_set(param, &p_config_type[cnt]);
 		if (ret) {
 			dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
 			goto free_config;
 		}
-		p_config_value[i] = pinconf_to_config_argument(configs[i]);
+		p_config_value[cnt] = pinconf_to_config_argument(configs[i]);
+		cnt++;
 	}
 
-	ret = pinctrl_ops->settings_conf(pmx->ph, pin, PIN_TYPE, num_configs,
+	ret = pinctrl_ops->settings_conf(pmx->ph, pin, PIN_TYPE, cnt,
 					 p_config_type,  p_config_value);
 	if (ret)
 		dev_err(pmx->dev, "Error parsing config %d\n", ret);

2.51.0

The SCMI implementation by definition lives in some always-on universe I think? It never sleeps or resets. Nevertheless:

Yours,
Linus Walleij

The SCMI protocol specification says that the PINCTRL_REQUEST and PINCTRL_RELEASE commands are optional. So if the SCMI server returns -EOPNOTSUPP, then treat that as success and continue.

---
 drivers/firmware/arm_scmi/pinctrl.c | 2 ++
 1 file changed, 2 insertions(+)

index a020e23d7c49..42cb1aef1fe1 100644
@@ -578,6 +578,8 @@ static int scmi_pinctrl_request_free(const struct scmi_protocol_handle *ph,
 	tx->flags = cpu_to_le32(type);
 
 	ret = ph->xops->do_xfer(ph, t);
+	if (ret == -EOPNOTSUPP)
+		ret = 0;
 	ph->xops->xfer_put(ph, t);
 
 	return ret;

2.51.0

Yours,
Linus Walleij

I see Linus W has provided review tags too. Though I am assuming it will go via some other tree as SCMI core change is tiny, just wanted to check it explicitly so that there is no wrong assumption on my side.

Regards,
Sudeep

From: AKASHI Takahiro <[email protected]>

Add a dt binding for the gpio-by-pinctrl driver. The driver is used for doing GPIO over the SCMI pinctrl protocol. There are a few mandatory properties such as gpio-ranges and ngpios, but it's not mandatory to specify the pin-mux.

---
 .../bindings/gpio/pin-control-gpio.yaml       | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml

new file mode 100644
index 000000000000..81c68579df6e
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/pin-control-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pin control based generic GPIO controller
+
+description:
+  The pin control-based GPIO will facilitate a pin controller's ability
+  to drive electric lines high/low and other generic properties of a
+  pin controller to perform general-purpose one-bit binary I/O.
+
+maintainers:
+  - Dan Carpenter <[email protected]>
+
+properties:
+  compatible:
+    const: scmi-pinctrl-gpio
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  ngpios: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+
+    required:
+      - gpio-hog
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+  - ngpios
+
+additionalProperties: true
+
+examples:
+  - |
+    gpio1 {
+        compatible = "scmi-pinctrl-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <10>;
+        gpio-ranges = <&scmi_pinctrl 0 8 4>,
+                      <&scmi_pinctrl 4 12 1>,
+                      <&scmi_pinctrl 5 15 1>,
+                      <&scmi_pinctrl 6 17 4>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&i2c2_pins>;
+    };
+
+    gpio2 {
+        compatible = "scmi-pinctrl-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        ngpios = <3>;
+        gpio-line-names = "gpio_5_17", "gpio_5_20", "gpio_5_22", "gpio_2_1";
+        gpio-ranges = <&scmi_pinctrl 0 30 4>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&keys_pins>;
+    };

2.51.0

I think Takashi's original idea is sound now that we clarified the level read/set semantics!

Yours,
Linus Walleij

From: AKASHI Takahiro <[email protected]>

The ARM SCMI pinctrl protocol allows GPIO access. Instead of creating a new SCMI gpio driver, this driver is a generic GPIO driver that uses standard pinctrl interfaces.

---
 drivers/gpio/Kconfig           |   7 ++
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-by-pinctrl.c | 124 +++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
 create mode 100644 drivers/gpio/gpio-by-pinctrl.c

index b45fb799e36c..4c8d2589c412 100644
@@ -246,6 +246,13 @@ config GPIO_BRCMSTB
 	help
 	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
+config GPIO_BY_PINCTRL
+	tristate "GPIO support based on a pure pin control backend"
+	depends on GPIOLIB
+	help
+	  Select this option to support GPIO devices based solely on pin
+	  control.  This is used to do GPIO over the ARM SCMI protocol.
+
 config GPIO_CADENCE
 	tristate "Cadence GPIO support"
 	depends on OF_GPIO

index c05f7d795c43..20d4a57afdaa 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BLZP1600)		+= gpio-blzp1600.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)		+= gpio-bt8xx.o
+obj-$(CONFIG_GPIO_BY_PINCTRL)		+= gpio-by-pinctrl.o
 obj-$(CONFIG_GPIO_CADENCE)		+= gpio-cadence.o
 obj-$(CONFIG_GPIO_CGBC)			+= gpio-cgbc.o
 obj-$(CONFIG_GPIO_CLPS711X)		+= gpio-clps711x.o

2.51.0

Similar wondering here... Can't this code be integrated with one of the existing generic things, like gpio-aggregator?

I really don't want to see yet another generic code in this folder.

With Best Regards,
Andy Shevchenko

The aggregator is very different, it takes existing GPIOs and creates a new GPIOchip from them.

What this does is essentially take the gpio-ranges, find the backing pins on the pin controller, and creates a GPIOchip from them.

I don't see anything that can be shared by the other generic business sadly, but I think maybe another back-end using just pin control can re-use this. In this case it is SCMI but any other firmware API just exposing pin control and no explicit GPIO could use this. (Not that I can think of any.)

Yours,
Linus Walleij

Thanks for elaboration! But why is it done this way? It doesn't sound like a usual (generic) problem to solve.

With Best Regards,
Andy Shevchenko

The experts can confirm, but I think this was done to simplify the SCMI firmware, both the specification and the implementation.

It was identified that it was necessary to have a pin control SCMI specification and implementation, but no separate GPIO specification and implementation was defined, leading to just one spec, and one single firmware service to implement that will support both pin control and GPIO.

These days, if we were re-specifying ACPI we could do the same there, but alas it already has legacy GPIO-only interfaces. SCMI has the upside of being able to start from scratch and upend a few traditions.

Yours,
Linus Walleij

To me this is as pure as a GPIO-on-pinctrl can get.

Yours,
Linus Walleij

That is actually pretty neat.