[RFC 0/4] gpio: add SCMI pinctrl based driver

I'm currently working on implementing SCMI pinctrl/gpio drivers
on U-Boot[1]. Although the pinctrl driver for the kernel[2] was submitted
by EPAM, it doesn't contain the gpio driver and I believe that we should
discuss a couple of points on the kernel side to finalize my design for
U-Boot. 

So this RFC is intended for reviews, especially to raise some issues.
(Please note that I have *never* tested the code because I don't have
any real hardware to test SCMI on it.)

1) how to obtain a value on an input pin
   All the existing gpio drivers are set to obtain a value on an input
   pin by accessing the hardware directly. In SCMI case, however, this is
   just impossible in its nature and must be supported via a protocol
   using "Input-value" configuration type. (See the spec[3], table-23.)

   The current pinconf framework is missing the feature (the pinconf
   parameter and a helper function). See patch#1 and #2.

   Please note that there is an issue around the pin configuration in
   EPAM's current pinctrl driver as I commented[4].

2) DT bindings
   I would like to propose a generic binding for SCMI pinctrl based
   gpio driver. This allows a "consumer" driver to handle gpio input pins
   like as other normal gpio controllers provide. (patch#4)

3) generic GPIO driver
   Based on (2), I tried to prototype a generic driver in patch#3.
   As you can see, there is no SCMI-specific line of code as a set of
   existing helper functions, except (1), seem to be enough to implement
   required interfaces.
   So I'm not sure whether the driver should has a "compatibles" property
   of "arm,scmi-gpio-generic".

I will appreciate any comments.

-Takahiro Akashi

[1] https://lists.denx.de/pipermail/u-boot/2023-September/529765.html
[2] https://lkml.iu.edu/hypermail/linux/kernel/2308.1/01082.html
[3] https://developer.arm.com/documentation/den0056/
[4] https://lkml.iu.edu/hypermail/linux/kernel/2308.2/07483.html

AKASHI Takahiro (4):
  pinctrl: define PIN_CONFIG_INPUT
  pinctrl: add pinctrl_gpio_get_config()
  gpio: scmi: add SCMI pinctrl based gpio driver
  dt-bindings: gpio: Add bindings for SCMI pinctrl based gpio

 .../bindings/gpio/arm,scmi-gpio.yaml          |  71 ++++++++
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-scmi.c                      | 154 ++++++++++++++++++
 drivers/pinctrl/core.c                        |  19 +++
 include/linux/pinctrl/consumer.h              |   8 +
 include/linux/pinctrl/pinconf-generic.h       |   3 +
 7 files changed, 264 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
 create mode 100644 drivers/gpio/gpio-scmi.c

-- 
2.34.1

This allows for enabling SCMI pinctrl based GPIO driver to obtain
an input gpio pin.

Signed-off-by: AKASHI Takahiro <redacted>
---
 include/linux/pinctrl/pinconf-generic.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d74b7a4ea154..842b328ea504 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h

@@ -67,6 +67,8 @@ struct pinctrl_map;
  *	passed as argument. The argument is in mA.
  * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
  *	passed as argument. The argument is in uA.
+ * @PIN_CONFIG_INPUT: This will obtain a value on an input pin. The returned
+ *	argument indicates the value.
  * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
  *	which means it will wait for signals to settle when reading inputs. The
  *	argument gives the debounce time in usecs. Setting the

@@ -128,6 +130,7 @@ enum pin_config_param {
 	PIN_CONFIG_DRIVE_PUSH_PULL,
 	PIN_CONFIG_DRIVE_STRENGTH,
 	PIN_CONFIG_DRIVE_STRENGTH_UA,
+	PIN_CONFIG_INPUT,
 	PIN_CONFIG_INPUT_DEBOUNCE,
 	PIN_CONFIG_INPUT_ENABLE,
 	PIN_CONFIG_INPUT_SCHMITT,

-- 
2.34.1

This is a counterpart of pinctrl_gpio_set_config(), which will initially
be used to implement gpio_get interface in SCMI pinctrl based GPIO driver.

Signed-off-by: AKASHI Takahiro <redacted>
---
 drivers/pinctrl/core.c           | 19 +++++++++++++++++++
 include/linux/pinctrl/consumer.h |  8 ++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e9dc9638120a..2f9c2efdfe0e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c

@@ -926,6 +926,25 @@ int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
 }
 EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
 
+int pinctrl_gpio_get_config(unsigned int gpio, unsigned long *config)
+{
+	struct pinctrl_gpio_range *range;
+	struct pinctrl_dev *pctldev;
+	int ret, pin;
+
+	ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
+	if (ret)
+		return ret;
+
+	mutex_lock(&pctldev->mutex);
+	pin = gpio_to_pin(range, gpio);
+	ret = pin_config_get_for_pin(pctldev, pin, config);
+	mutex_unlock(&pctldev->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
+
 static struct pinctrl_state *find_state(struct pinctrl *p,
 					const char *name)
 {

diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4729d54e8995..852fac97a79b 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h

@@ -31,6 +31,8 @@ extern void pinctrl_gpio_free(unsigned gpio);
 extern int pinctrl_gpio_direction_input(unsigned gpio);
 extern int pinctrl_gpio_direction_output(unsigned gpio);
 extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);
+extern int pinctrl_gpio_get_config(unsigned int gpio,
+				   unsigned long *config);
 
 extern struct pinctrl * __must_check pinctrl_get(struct device *dev);
 extern void pinctrl_put(struct pinctrl *p);

@@ -92,6 +94,12 @@ static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
 	return 0;
 }
 
+static inline int pinctrl_gpio_get_config(unsigned int gpio,
+					  unsigned long *config)
+{
+	return 0;
+}
+
 static inline struct pinctrl * __must_check pinctrl_get(struct device *dev)
 {
 	return NULL;

-- 
2.34.1

SCMI pin control protocol supports not only pin controllers, but also
gpio controllers by design. This patch includes a generic gpio driver
which allows consumer drivers to access gpio pins that are handled
through SCMI interfaces.

Signed-off-by: AKASHI Takahiro <redacted>
---
 drivers/gpio/Kconfig     |   8 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-scmi.c | 154 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+)
 create mode 100644 drivers/gpio/gpio-scmi.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 673bafb8be58..1a968b950f3a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig

@@ -566,6 +566,14 @@ config GPIO_SAMA5D2_PIOBU
 	  The difference from regular GPIOs is that they
 	  maintain their value during backup/self-refresh.
 
+config GPIO_SCMI
+	tristate "GPIO support based on SCMI pinctrl"
+	depends on OF_GPIO
+	depends on PINCTRL_SCMI
+	help
+	  Select this option to support GPIO devices based on SCMI pin
+	  control protocol.
+
 config GPIO_SIFIVE
 	tristate "SiFive GPIO support"
 	depends on OF_GPIO

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..2abe1e9d5e77 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile

@@ -141,6 +141,7 @@ obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
+obj-$(CONFIG_GPIO_SCMI)			+= gpio-scmi.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o

diff --git a/drivers/gpio/gpio-scmi.c b/drivers/gpio/gpio-scmi.c
new file mode 100644
index 000000000000..ece63ea62b70
--- /dev/null
+++ b/drivers/gpio/gpio-scmi.c

@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2023 Linaro Inc.
+//   Author: AKASHI takahiro <takahiro.akashi@linaro.org>
+
+#include <linux/gpio/driver.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include "gpiolib.h"
+
+struct scmi_gpio_priv {
+	struct gpio_chip chip;
+};
+
+static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	unsigned long config;
+
+	config = PIN_CONFIG_OUTPUT_ENABLE;
+	if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+		return -1;
+	if (config)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	config = PIN_CONFIG_INPUT_ENABLE;
+	if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+		return -1;
+	if (config)
+		return GPIO_LINE_DIRECTION_IN;
+
+	return -1;
+}
+
+static int scmi_gpio_direction_input(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
+}
+
+static int scmi_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned int offset, int val)
+{
+	return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
+}
+
+static int scmi_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	unsigned long config;
+
+	/* FIXME: currently, PIN_CONFIG_INPUT not defined */
+	config = PIN_CONFIG_INPUT;
+	if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+		return -1;
+
+	/* FIXME: the packed format not defined */
+	if (config >> 8)
+		return 1;
+
+	return 0;
+}
+
+static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
+{
+	unsigned long config;
+
+	config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val & 0x1);
+;
+	pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);
+}
+
+static u16 sum_up_ngpios(struct gpio_chip *chip)
+{
+	struct gpio_pin_range *range;
+	struct gpio_device *gdev = chip->gpiodev;
+	u16 ngpios = 0;
+
+	list_for_each_entry(range, &gdev->pin_ranges, node) {
+		ngpios += range->range.npins;
+	}
+
+	return ngpios;
+}
+
+static int scmi_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent_np;
+	struct scmi_gpio_priv *priv;
+	struct gpio_chip *chip;
+	int ret;
+
+	/* FIXME: who should be the parent */
+	parent_np = NULL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	chip = &priv->chip;
+	chip->label = dev_name(dev);
+	chip->parent = dev;
+	chip->base = -1;
+
+	chip->request = gpiochip_generic_request;
+	chip->free = gpiochip_generic_free;
+	chip->get_direction = scmi_gpio_get_direction;
+	chip->direction_input = scmi_gpio_direction_input;
+	chip->direction_output = scmi_gpio_direction_output;
+	chip->get = scmi_gpio_get;
+	chip->set = scmi_gpio_set;
+
+	ret = devm_gpiochip_add_data(dev, chip, priv);
+	if (ret)
+		return ret;
+
+	chip->ngpio = sum_up_ngpios(chip);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int scmi_gpio_remove(struct platform_device *pdev)
+{
+	struct scmi_gpio_priv *priv = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&priv->chip);
+
+	return 0;
+}
+
+static const struct of_device_id scmi_gpio_match[] = {
+	{ .compatible = "arm,scmi-gpio-generic" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, scmi_gpio_match);
+
+static struct platform_driver scmi_gpio_driver = {
+	.probe = scmi_gpio_probe,
+	.remove = scmi_gpio_remove,
+	.driver = {
+		.name = "scmi-gpio",
+		.of_match_table = scmi_gpio_match,
+	},
+};
+module_platform_driver(scmi_gpio_driver);
+
+MODULE_AUTHOR("AKASHI Takahiro <takahiro.akashi@linaro.org>");
+MODULE_DESCRIPTION("SCMI Pinctrl based GPIO driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

Signed-off-by: AKASHI Takahiro <redacted>
---
 .../bindings/gpio/arm,scmi-gpio.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
new file mode 100644
index 000000000000..2601c5594567
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SCMI pinctrl based generic GPIO controller
+
+maintainers:
+  - AKASHI Takahiro <akashi.takahiro@linaro.org>
+
+properties:
+  $nodename:
+    pattern: "^scmi_gpio(@[0-9a-f]+)$"
+
+  compatible:
+    const: arm,scmi-gpio-generic
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      input: true
+      output-high: true
+      output-low: true
+      line-name: true
+
+    required:
+      - gpio-hog
+      - gpios
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    scmi_gpio_0: scmi_gpio@0 {
+        compatible = "arm,scmi-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&scmi_pinctrl 0 10 5>,
+                      <&scmi_pinctrl 5 0 0>;
+        gpio-ranges-group-names = "",
+                                  "pinmux_gpio";
+    };
+
+    // Consumer:
+    sdhci0_pwrseq {
+        compatible = "mmc-pwrseq-emmc";
+        reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>;
+    };

-- 
2.34.1

On Mon, 02 Oct 2023 11:16:02 +0900, AKASHI Takahiro wrote:

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

Signed-off-by: AKASHI Takahiro <redacted>
---
 .../bindings/gpio/arm,scmi-gpio.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/gpio/arm,scmi-gpio.example.dts:20.34-28.11: Warning (unit_address_vs_reg): /example-0/scmi_gpio@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/gpio/arm,scmi-gpio.example.dtb: /example-0/scmi_gpio@0: failed to match any schema with compatible: ['arm,scmi-gpio']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231002021602.260100-5-takahiro.akashi@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

What is "generic pinctrl-gpio mapping framework"? DT doesn't have 
frameworks.

Not the correct name.

What makes it generic? No such thing. Just drop '-generic'.

You don't need all this just 'required: [ gpio-hog ]'. Then the hog 
schema will check the rest.

gpio {

But doesn't SCMI have protocol numbers?

Outside the scope of this binding. Drop this node.

+    sdhci0_pwrseq {
+        compatible = "mmc-pwrseq-emmc";
+        reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>;
+    };
-- 
2.34.1

On Mon, Oct 02, 2023 at 09:41:55AM -0500, Rob Herring wrote:

On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:

quoted

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

[ snip]

quoted

+    additionalProperties: false
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    scmi_gpio_0: scmi_gpio@0 {

gpio {

But doesn't SCMI have protocol numbers?

My understanding is that this RFC GPIO driver from Akashi is built
completely on Pinctrl facilities (as he says in the cover), it is not
indeed a typical pure SCMI driver, it just happen to trigger the use
of SCMI if the underlying backend pinctrl driver is pinctrl-scmi;
but this driver does not really call directly into any SCMI API by
itself, i.e. it does not get and call any SCMI protocol ops.
(but it could indeed trigger the backend Pinctrl SCMI driver to issue
 such call on its behalf AFAIU...)

I wonder why it has even a dependency on PINCTRL_SCMI at this point;
is not that it could work (generically) even if the backend Pinctrl
driver is NOT SCMI ?
What makes it usable only against an SCMI Pinctrl backend ?
Cannot be a generic GPIO driver based on top of Pinctrl, no matter which
Pinctrl backend driver has been configured ?

...I maybe missing something here about Pinctrl AND GPIO frameworks :P

Thanks,
Cristian

Hi Rob,

On Mon, Oct 02, 2023 at 09:41:55AM -0500, Rob Herring wrote:

On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:

quoted

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

What is "generic pinctrl-gpio mapping framework"? DT doesn't have 
frameworks.

I meant to refer to section 2.1-2.3 in "Documentation/devicetree/bindings/gpio/gpio.txt". The semantics is implemented in drivers/gpio/gpiolib(-of).c.

quoted

Signed-off-by: AKASHI Takahiro <redacted>
---
 .../bindings/gpio/arm,scmi-gpio.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
new file mode 100644
index 000000000000..2601c5594567
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SCMI pinctrl based generic GPIO controller
+
+maintainers:
+  - AKASHI Takahiro <akashi.takahiro@linaro.org>
+
+properties:
+  $nodename:
+    pattern: "^scmi_gpio(@[0-9a-f]+)$"

Not the correct name.

How not?

quoted

+
+  compatible:
+    const: arm,scmi-gpio-generic

What makes it generic? No such thing. Just drop '-generic'.

I will discuss this issue in following Cristian's comment.

quoted

+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      input: true
+      output-high: true
+      output-low: true
+      line-name: true
+
+    required:
+      - gpio-hog
+      - gpios

You don't need all this just 'required: [ gpio-hog ]'. Then the hog 
schema will check the rest.

Okay.

quoted

+
+    additionalProperties: false
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    scmi_gpio_0: scmi_gpio@0 {

gpio {

But doesn't SCMI have protocol numbers?

quoted

+        compatible = "arm,scmi-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&scmi_pinctrl 0 10 5>,
+                      <&scmi_pinctrl 5 0 0>;
+        gpio-ranges-group-names = "",
+                                  "pinmux_gpio";
+    };
+
+    // Consumer:

Outside the scope of this binding. Drop this node.

Even though it's in an example?
"#gpio-cells" has a meaning in consumer side.

-Takahiro Akashi

quoted

+    sdhci0_pwrseq {
+        compatible = "mmc-pwrseq-emmc";
+        reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>;
+    };
-- 
2.34.1

Hi Rob, Cristian,

On Mon, Oct 02, 2023 at 03:58:27PM +0100, Cristian Marussi wrote:

On Mon, Oct 02, 2023 at 09:41:55AM -0500, Rob Herring wrote:

quoted

On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:

quoted

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

[ snip]

quoted

quoted

+    additionalProperties: false
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    scmi_gpio_0: scmi_gpio@0 {

gpio {

But doesn't SCMI have protocol numbers?

My understanding is that this RFC GPIO driver from Akashi is built
completely on Pinctrl facilities (as he says in the cover), it is not
indeed a typical pure SCMI driver, it just happen to trigger the use
of SCMI if the underlying backend pinctrl driver is pinctrl-scmi;
but this driver does not really call directly into any SCMI API by
itself, i.e. it does not get and call any SCMI protocol ops.
(but it could indeed trigger the backend Pinctrl SCMI driver to issue
 such call on its behalf AFAIU...)

It would be possible to implement this driver by directly using SCMI
pinctrl interfaces (I mean drivers/firmware/arm,scmi/pinctrl.c)
if the system wants to utilize SCMI solely for GPIO accesses and doesn't
need pinctrl support.
(Even so, "protocol@19" will be required due to the current SCMI binding.)

But I didn't take this approach because the kernel's pinctrl framework
(and many existing pinctrl drivers) instead adopts standard pinctrl-
gpio mapping (I mean gpiolib(-of).c) and it just seems to work well.

I wonder why it has even a dependency on PINCTRL_SCMI at this point;
is not that it could work (generically) even if the backend Pinctrl
driver is NOT SCMI ?
What makes it usable only against an SCMI Pinctrl backend ?
Cannot be a generic GPIO driver based on top of Pinctrl, no matter which
Pinctrl backend driver has been configured ?

That is one of my questions (See the issue (3) in my cover letter.)
Why doesn't there exist a generic GPIO driver of this kind (based on gpiolib
framework) even though it could apparently be possible?

I guess that there a couple of reasons:
1) As I mentioned in the issue (1) in my cover letter, the current
  framework doesn't present an interface, especially for obtaining
  a value on a gpio input pin. Then it enforces each pinctrl-based gpio
  driver needs to have its own driver.
2) Furthermore, there may be driver-specific semantics required,
  say, for pinconf-related configurations? (I don't come up with any
  example, though)

If my driver is good enough for applying to other gpio controllers as well,
I would not hesitate to name it a genuine generic driver whether the backend
may be SCMI or not.
        -> Linus, comment here please.

Due to possible cases of (2), I still added "-generic" postfix to the
"compatibles" property so that other variant drivers may be tagged as
"arm,scmi-gpio-some-system" or "some-vendor,scmi-gpio".

Thanks,
-Takahiro Akashi

...I maybe missing something here about Pinctrl AND GPIO frameworks :P

Thanks,
Cristian

On 03/10/2023 02:41, AKASHI Takahiro wrote:

Hi Rob,

On Mon, Oct 02, 2023 at 09:41:55AM -0500, Rob Herring wrote:

quoted

On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:

quoted

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

What is "generic pinctrl-gpio mapping framework"? DT doesn't have 
frameworks.

I meant to refer to section 2.1-2.3 in "Documentation/devicetree/bindings/gpio/gpio.txt". The semantics is implemented in drivers/gpio/gpiolib(-of).c.

Linux specific GPIO library is as well outside of DT scope. Please focus
here on hardware, not Linux specifics.

quoted

quoted

Signed-off-by: AKASHI Takahiro <redacted>
---
 .../bindings/gpio/arm,scmi-gpio.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
new file mode 100644
index 000000000000..2601c5594567
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SCMI pinctrl based generic GPIO controller
+
+maintainers:
+  - AKASHI Takahiro <akashi.takahiro@linaro.org>
+
+properties:
+  $nodename:
+    pattern: "^scmi_gpio(@[0-9a-f]+)$"

Not the correct name.

How not?

Underscores are no allowed and are pointed by dtc (W=2). scmi is
redundant here, because names should be generic. Anyway, we do not add
node name requirements to device schema.

quoted

quoted

+
+  compatible:
+    const: arm,scmi-gpio-generic

What makes it generic? No such thing. Just drop '-generic'.

I will discuss this issue in following Cristian's comment.

quoted

quoted

+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      input: true
+      output-high: true
+      output-low: true
+      line-name: true
+
+    required:
+      - gpio-hog
+      - gpios

You don't need all this just 'required: [ gpio-hog ]'. Then the hog 
schema will check the rest.

Okay.

quoted

quoted

+
+    additionalProperties: false
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    scmi_gpio_0: scmi_gpio@0 {

gpio {

But doesn't SCMI have protocol numbers?

quoted

+        compatible = "arm,scmi-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&scmi_pinctrl 0 10 5>,
+                      <&scmi_pinctrl 5 0 0>;
+        gpio-ranges-group-names = "",
+                                  "pinmux_gpio";
+    };
+
+    // Consumer:

Outside the scope of this binding. Drop this node.

Even though it's in an example?
"#gpio-cells" has a meaning in consumer side.

Just look at any other bindings.

Best regards,
Krzysztof

Hi Takahiro,

first, thanks for working on this important and crucial driver!

I'll try to clarify and also explain something of what the others
are saying (unless I misunderstand them...)

On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
[off-list ref] wrote:

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

Signed-off-by: AKASHI Takahiro <redacted>

I think like Christian says that SCMI maybe has nothing to do
with this binding? It is just one possible use case (though we don't know
of any others.) The resource it is using is generic functionality that exist
in any pin controller that provides ways to drive lines high and low
etc.

Would it be named a generic pin control-based GPIO?

(...)

(...)

+$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#

So no ARM, no scmi, just pin-control-gpio.yaml, be bold!

(I like this long unabbreviated name)

+title: SCMI pinctrl based generic GPIO controller

Pin control-based generic GPIO controller

Add

description:
  The pin control-based GPIO will facilitate a pin controllers ability
  to drive electric lines high/low and other generic properties of a
  pin controller to perform general-purpose one-bit binary I/O.

(At least I think this is the idea, I hope I understand correctly.)

These nodes are always just named gpio@...
the resource marker is "this is a GPIO" that's all it means.

+  compatible:
+    const: arm,scmi-gpio-generic

const: pin-control-gpio

Other than that I am aboard with the solution!

Yours,
Linus Walleij

Hi Takahiro,

On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
[off-list ref] wrote:

This allows for enabling SCMI pinctrl based GPIO driver to obtain
an input gpio pin.

Signed-off-by: AKASHI Takahiro <redacted>

(...)

+ * @PIN_CONFIG_INPUT: This will obtain a value on an input pin. The returned
+ *     argument indicates the value.

We need to specify that this is the inverse of @PIN_CONFIG_OUTPUT,
that setting a line into *input mode* requires the use of
@PIN_CONFIG_INPUT_ENABLE, so the config can never be set
but should return an error on set, and that the argument returned is 1 for
logic high and 0 for logic low.

Otherwise I think this is fine!

Yours,
Linus Walleij

On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
[off-list ref] wrote:

This is a counterpart of pinctrl_gpio_set_config(), which will initially
be used to implement gpio_get interface in SCMI pinctrl based GPIO driver.

Signed-off-by: AKASHI Takahiro <redacted>

Makes perfect sense for what you are trying to do.
Reviewed-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
[off-list ref] wrote:

SCMI pin control protocol supports not only pin controllers, but also
gpio controllers by design. This patch includes a generic gpio driver
which allows consumer drivers to access gpio pins that are handled
through SCMI interfaces.

Signed-off-by: AKASHI Takahiro <redacted>

I would write a bit that this is intended for SCMI but it actually
is a GPIO front-end to any pin controller that supports the
necessary pin config operations.

 drivers/gpio/gpio-scmi.c | 154 +++++++++++++++++++++++++++++++++++++++

So I would name it gpio-by-pinctrl.c
(clear and hard to misunderstand)

+config GPIO_SCMI

GPIO_BY_PINCTRL

+       tristate "GPIO support based on SCMI pinctrl"

"GPIO support based on a pure pin control back-end"

+       depends on OF_GPIO

Skip this, let's use device properties instead. They will anyways just translate
to OF properties in the OF case.

"GPIO devices based solely on pin control, specifically pin configuration, such
as SCMI."

+#include <linux/of.h>

Use #include <linux/property.h> so we remove reliance on OF.

+#include "gpiolib.h"

Why?

+static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)

Rename all functions pinctrl_gpio_*

Probably you want to return the error code from pinctrl_gpio_get_config()
rather than -1? (same below).

I would actually not return after checking PIN_CONFIG_OUTPUT_ENABLE.
I would call *both* something like:

int ret;
bool  out_en, in_en;

config = PIN_CONFIG_OUTPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
    return ret;
/* Maybe check for "not implemented" error code here and let that pass
 * setting out_en = false; not sure. Maybe we should mandate support
 * for this.
 */
out_en = !!config;
config = PIN_CONFIG_INPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
    return ret;
in_en = !!config;

/* Consistency check - in theory both can be enabled! */
if (in_en && !out_en)
    return GPIO_LINE_DIRECTION_IN;
if (!in_en && out_en)
    return GPIO_LINE_DIRECTION_OUT;
if (in_en && out_en) {
    /*
     * This is e.g. open drain emulation!
     * In this case check @PIN_CONFIG_DRIVE_OPEN_DRAIN
     * if this is enabled, return GPIO_LINE_DIRECTION_OUT,
     * else return an error. (I think.)
     */
}

/* We get here for (!in_en && !out_en) */
return -EINVAL;

Proper error code instead of -1 otherwise looks good!

+static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)

static int?

No need to add & 0x01, the gpiolib core already does this.

+       pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);

return pinctrl_gpio_set_config(); so error is propagated.

This works but isn't really the intended use case of the ranges.
Feel a bit uncertain about it, but I can't think of anything better.
And I guess these come directly out of SCMI so it's first hand
information about all GPIOs.

Skip (not used)

+       /* FIXME: who should be the parent */
+       parent_np = NULL;

Skip (not used)

This is the actual parent, which is good enough?

Add:
chip->set_config = gpiochip_generic_config;

which in turn becomes just pinctrl_gpio_set_config(), which
is what we want.

The second cell in two-cell GPIOs already supports passing
GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE,
GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE,
which you can this way trivially pass down to the pin control driver.

NB: make sure the scmi pin control driver returns error for
unknown configs.

You are using devm_* to add it so this is not needed!

Just drop the remove function.

+static const struct of_device_id scmi_gpio_match[] = {
+       { .compatible = "arm,scmi-gpio-generic" },

"pin-control-gpio" is my suggestion for this!

I hope this helps.

Yours,
Linus Walleij

Hi Linus,

On Tue, Oct 03, 2023 at 11:35:31PM +0200, Linus Walleij wrote:

On Mon, Oct 2, 2023 at 4:17???AM AKASHI Takahiro
[off-list ref] wrote:

quoted

SCMI pin control protocol supports not only pin controllers, but also
gpio controllers by design. This patch includes a generic gpio driver
which allows consumer drivers to access gpio pins that are handled
through SCMI interfaces.

Signed-off-by: AKASHI Takahiro <redacted>

I would write a bit that this is intended for SCMI but it actually
is a GPIO front-end to any pin controller that supports the
necessary pin config operations.

I'm still not sure whether my approach can be applied to any other
pinctrl-based gpio drivers, in which extra (driver-specific) operations
might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
For instance, look at gpio-tegra.c:

! static int tegra_gpio_direction_input(struct gpio_chip *chip,
!                                       unsigned int offset)
! {
!         struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
! 
!         tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
!         tegra_gpio_enable(tgi, offset);
! 
!         ret = pinctrl_gpio_direction_input(chip->base + offset);
!	  ...
! }

That said, I will send a next version incorporating the changes you
suggest here.

quoted

 drivers/gpio/gpio-scmi.c | 154 +++++++++++++++++++++++++++++++++++++++

So I would name it gpio-by-pinctrl.c
(clear and hard to misunderstand)

quoted

+config GPIO_SCMI

GPIO_BY_PINCTRL

Okay.

quoted

+       tristate "GPIO support based on SCMI pinctrl"

"GPIO support based on a pure pin control back-end"

Okay.

quoted

+       depends on OF_GPIO

Skip this, let's use device properties instead. They will anyways just translate
to OF properties in the OF case.

Okay, I don't know how device properties work, though.

quoted

+       depends on PINCTRL_SCMI
+       help
+         Select this option to support GPIO devices based on SCMI pin
+         control protocol.

"GPIO devices based solely on pin control, specifically pin configuration, such
as SCMI."

Okay.

quoted

+#include <linux/of.h>

Use #include <linux/property.h> so we remove reliance on OF.

Actually we need neither to compile the code.

quoted

+#include "gpiolib.h"

Why?

Because we need to access members of struct gpio_device.

quoted

+static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)

Rename all functions pinctrl_gpio_*

Well, this change will result in name conflicts against existing
pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.

quoted

+{
+       unsigned long config;
+
+       config = PIN_CONFIG_OUTPUT_ENABLE;
+       if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+               return -1;

Probably you want to return the error code from pinctrl_gpio_get_config()
rather than -1? (same below).

Yes.

quoted

+       if (config)
+               return GPIO_LINE_DIRECTION_OUT;
+
+       config = PIN_CONFIG_INPUT_ENABLE;
+       if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+               return -1;
+       if (config)
+               return GPIO_LINE_DIRECTION_IN;

I would actually not return after checking PIN_CONFIG_OUTPUT_ENABLE.
I would call *both* something like:

int ret;
bool  out_en, in_en;

config = PIN_CONFIG_OUTPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
    return ret;
/* Maybe check for "not implemented" error code here and let that pass
 * setting out_en = false; not sure. Maybe we should mandate support
 * for this.
 */
out_en = !!config;
config = PIN_CONFIG_INPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
    return ret;
in_en = !!config;

/* Consistency check - in theory both can be enabled! */
if (in_en && !out_en)
    return GPIO_LINE_DIRECTION_IN;
if (!in_en && out_en)
    return GPIO_LINE_DIRECTION_OUT;
if (in_en && out_en) {
    /*
     * This is e.g. open drain emulation!
     * In this case check @PIN_CONFIG_DRIVE_OPEN_DRAIN
     * if this is enabled, return GPIO_LINE_DIRECTION_OUT,
     * else return an error. (I think.)
     */
}

Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.

In order to be able to read a value as an input pin, I think, we need
to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
In this case, however, we cannot distinguish the other case where we want
to use the pin as OUTPUT and drive it to (active) high.

/* We get here for (!in_en && !out_en) */
return -EINVAL;

quoted

+static int scmi_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+       unsigned long config;
+
+       /* FIXME: currently, PIN_CONFIG_INPUT not defined */
+       config = PIN_CONFIG_INPUT;
+       if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
+               return -1;
+
+       /* FIXME: the packed format not defined */
+       if (config >> 8)
+               return 1;
+
+       return 0;
+}

Proper error code instead of -1 otherwise looks good!

Yes.

quoted

+static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)

static int?

Unfortunately, the function prototype of "set" in struct gpio_device is
        void (*set)(struct gpio_chip *gc, unsigned int offset, int value);

So we cannot propagate an error to the caller.

quoted

+{
+       unsigned long config;
+
+       config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val & 0x1);

No need to add & 0x01, the gpiolib core already does this.

Which part of gpiolib core?
The argument is shifted by 8 in PIN_CONF_PACKED(), but never normalized.
Since the driver code, however, should verify the value in some way, I will
drop the masking here.

quoted

+       pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);

return pinctrl_gpio_set_config(); so error is propagated.

See above.

quoted

+static u16 sum_up_ngpios(struct gpio_chip *chip)
+{
+       struct gpio_pin_range *range;
+       struct gpio_device *gdev = chip->gpiodev;
+       u16 ngpios = 0;
+
+       list_for_each_entry(range, &gdev->pin_ranges, node) {
+               ngpios += range->range.npins;
+       }

This works but isn't really the intended use case of the ranges.
Feel a bit uncertain about it, but I can't think of anything better.
And I guess these come directly out of SCMI so it's first hand
information about all GPIOs.

I don't get your point.
However many pins SCMI firmware (or other normal pin controllers) might
expose, the total number of pins available by this driver is limited by
"gpio-ranges" property.
So the sum as "ngpios" should make sense unless a user accidentally
specifies a wrong range of pins.

Do I misunderstand anything?

quoted

+static int scmi_gpio_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct device_node *parent_np;

Skip (not used)

Okay. This code is a remnant from the original driver that I referred to
as a base.

quoted

+       /* FIXME: who should be the parent */
+       parent_np = NULL;

Skip (not used)

quoted

+       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+
+       chip = &priv->chip;
+       chip->label = dev_name(dev);
+       chip->parent = dev;

This is the actual parent, which is good enough?

quoted

+       chip->base = -1;
+
+       chip->request = gpiochip_generic_request;
+       chip->free = gpiochip_generic_free;
+       chip->get_direction = scmi_gpio_get_direction;
+       chip->direction_input = scmi_gpio_direction_input;
+       chip->direction_output = scmi_gpio_direction_output;

Add:
chip->set_config = gpiochip_generic_config;

Yes.

which in turn becomes just pinctrl_gpio_set_config(), which
is what we want.

The second cell in two-cell GPIOs already supports passing
GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE,
GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE,
which you can this way trivially pass down to the pin control driver.

NB: make sure the scmi pin control driver returns error for
unknown configs.

Well, the error will be determined by SCMI firmware(server)
not the driver itself :)

quoted

+static int scmi_gpio_remove(struct platform_device *pdev)
+{
+       struct scmi_gpio_priv *priv = platform_get_drvdata(pdev);
+
+       gpiochip_remove(&priv->chip);

You are using devm_* to add it so this is not needed!

Just drop the remove function.

Okay.

quoted

+static const struct of_device_id scmi_gpio_match[] = {
+       { .compatible = "arm,scmi-gpio-generic" },

"pin-control-gpio" is my suggestion for this!

I hope this helps.

Thank you for your kind suggestions.

-Takahiro Akashi

Yours,
Linus Walleij

On Tue, Oct 03, 2023 at 10:49:10PM +0200, Linus Walleij wrote:

Hi Takahiro,

On Mon, Oct 2, 2023 at 4:17???AM AKASHI Takahiro
[off-list ref] wrote:

quoted

This allows for enabling SCMI pinctrl based GPIO driver to obtain
an input gpio pin.

Signed-off-by: AKASHI Takahiro <redacted>

(...)

quoted

+ * @PIN_CONFIG_INPUT: This will obtain a value on an input pin. The returned
+ *     argument indicates the value.

We need to specify that this is the inverse of @PIN_CONFIG_OUTPUT,
that setting a line into *input mode* requires the use of
@PIN_CONFIG_INPUT_ENABLE, so the config can never be set
but should return an error on set, and that the argument returned is 1 for
logic high and 0 for logic low.

I will add more as you suggest.

-Takahiro Akashi

Otherwise I think this is fine!

Yours,
Linus Walleij

On Tue, Oct 03, 2023 at 03:16:49PM +0200, Linus Walleij wrote:

Hi Takahiro,

first, thanks for working on this important and crucial driver!

I'll try to clarify and also explain something of what the others
are saying (unless I misunderstand them...)

Ah, thank you.

On Mon, Oct 2, 2023 at 4:17???AM AKASHI Takahiro
[off-list ref] wrote:

quoted

A dt binding for SCMI pinctrl based gpio driver is defined in this
commit. It basically conforms to generic pinctrl-gpio mapping framework.

Signed-off-by: AKASHI Takahiro <redacted>

I think like Christian says that SCMI maybe has nothing to do
with this binding? It is just one possible use case (though we don't know
of any others.) The resource it is using is generic functionality that exist
in any pin controller that provides ways to drive lines high and low
etc.

Would it be named a generic pin control-based GPIO?

If you like :)
As I said, I was not confident that the driver be applicable
to other pinctrl-gpio cases.

(...)

quoted

+++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml

(...)

quoted

+$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#

So no ARM, no scmi, just pin-control-gpio.yaml, be bold!

I'm not so ambitious.

(I like this long unabbreviated name)

quoted

+title: SCMI pinctrl based generic GPIO controller

Pin control-based generic GPIO controller

Add

description:
  The pin control-based GPIO will facilitate a pin controllers ability
  to drive electric lines high/low and other generic properties of a
  pin controller to perform general-purpose one-bit binary I/O.

(At least I think this is the idea, I hope I understand correctly.)

Okay.

quoted

+properties:
+  $nodename:
+    pattern: "^scmi_gpio(@[0-9a-f]+)$"

These nodes are always just named gpio@...
the resource marker is "this is a GPIO" that's all it means.

By following other gpio drivers' bindings, I will drop this rule.

quoted

+  compatible:
+    const: arm,scmi-gpio-generic

const: pin-control-gpio

Other than that I am aboard with the solution!

Hope that the driver works on real hardware :)

-Takahiro Akashi

Yours,
Linus Walleij

Hi Takahiro,

I see you are on track with this!

Some clarifications:

On Wed, Oct 4, 2023 at 8:53 AM AKASHI Takahiro
[off-list ref] wrote:

I'm still not sure whether my approach can be applied to any other
pinctrl-based gpio drivers, in which extra (driver-specific) operations
might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
For instance, look at gpio-tegra.c:

Yeah, it kind of requires a "pure" pin controller underneath that don't
want to do anything else on any operations, otherwise we are back
to a per-soc pin control driver.

But I think it is appropriate for abstractions that strive to provide
"total abstraction behind a firmware", so such as SCMI or ACPI (heh).

quoted

Skip this, let's use device properties instead. They will anyways just translate
to OF properties in the OF case.

Okay, I don't know how device properties work, though.

They are pretty much 1-to-1 slot-ins for the corresponding of_*
functions, passing struct device * instead of struct device_node *,
if you look in include/linux/property.h you will feel at home very
quickly.

quoted

quoted

+static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)

Rename all functions pinctrl_gpio_*

Well, this change will result in name conflicts against existing
pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.

Yeah that works, or pincontro_by_gpio_ or such.

Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.

I wrote some documentation! But it is hidden deep in the docs:
https://docs.kernel.org/driver-api/gpio/driver.html#gpio-lines-with-open-drain-source-support

In order to be able to read a value as an input pin, I think, we need
to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
In this case, however, we cannot distinguish the other case where we want
to use the pin as OUTPUT and drive it to (active) high.

With open drain, on GPIO controllers that do not support a native
open drain mode, we emulate open drain output high by switching
the line into input mode. The line in this case has a pull-up resistor
(internal or external) and as input mode is high-Z the pull up resistor
will pull the signal high, to any level - could be e.g 48V which is
helpful for some serial links.

But this case is really tricky so it can be hard to get things right,
I get a bit confused and so we need to think about it a few times.

quoted

quoted

+static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)

static int?

Unfortunately, the function prototype of "set" in struct gpio_device is
        void (*set)(struct gpio_chip *gc, unsigned int offset, int value);

So we cannot propagate an error to the caller.

Grrr that must be my fault. Sorry about not fixing this :(

quoted

No need to add & 0x01, the gpiolib core already does this.

Which part of gpiolib core?

chip->set = scmi_gpio_set; gets called like this in gpiolib:

 gpiod_direction_output_raw_commit(..., int value)
{
    int val = !!value;
(...)
    gc->set(gc, gpio_chip_hwgpio(desc), val);

Notice clamping int val = !!value; will make the passed val 0 or 1.

quoted

quoted

+static u16 sum_up_ngpios(struct gpio_chip *chip)
+{
+       struct gpio_pin_range *range;
+       struct gpio_device *gdev = chip->gpiodev;
+       u16 ngpios = 0;
+
+       list_for_each_entry(range, &gdev->pin_ranges, node) {
+               ngpios += range->range.npins;
+       }

This works but isn't really the intended use case of the ranges.
Feel a bit uncertain about it, but I can't think of anything better.
And I guess these come directly out of SCMI so it's first hand
information about all GPIOs.

I don't get your point.
However many pins SCMI firmware (or other normal pin controllers) might
expose, the total number of pins available by this driver is limited by
"gpio-ranges" property.
So the sum as "ngpios" should make sense unless a user accidentally
specifies a wrong range of pins.

Yes.

And it is this fact that the same number need to appear in two places
and double-specification will sooner or later bring us to the situation
where the two do not agree, and what do we do then?

If the ranges come from firmware, which is subject to change such
as "oops we forgot this pin", the GPIO number will just insert itself
among the already existing ones: say we have two ranges:

1: 0..5
2: 6..9

Ooops forgot a GPIO in the first range, it has to be bumped to
0..6.

But somewhere in the device tree there is:

foo-gpios = <&scmi_gpio 7 GPIO_OUT_LOW>;

So now this is wrong (need to be changed to 8) and we have zero tooling
to detect this, the author just has to be very careful all the time.

But I honestly do not know any better way.

quoted

which in turn becomes just pinctrl_gpio_set_config(), which
is what we want.

The second cell in two-cell GPIOs already supports passing
GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE,
GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE,
which you can this way trivially pass down to the pin control driver.

NB: make sure the scmi pin control driver returns error for
unknown configs.

Well, the error will be determined by SCMI firmware(server)
not the driver itself :)

Hehe, I think it is good that the SCMI firmware gets some exercise
from day 1!

Yours,
Linus Walleij

Hi Linus,

On Wed, Oct 04, 2023 at 10:35:05AM +0200, Linus Walleij wrote:

Hi Takahiro,

I see you are on track with this!

Some clarifications:

On Wed, Oct 4, 2023 at 8:53???AM AKASHI Takahiro
[off-list ref] wrote:

quoted

I'm still not sure whether my approach can be applied to any other
pinctrl-based gpio drivers, in which extra (driver-specific) operations
might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
For instance, look at gpio-tegra.c:

Yeah, it kind of requires a "pure" pin controller underneath that don't
want to do anything else on any operations, otherwise we are back
to a per-soc pin control driver.

But I think it is appropriate for abstractions that strive to provide
"total abstraction behind a firmware", so such as SCMI or ACPI (heh).

Right. So we are on the same page now.

quoted

quoted

Skip this, let's use device properties instead. They will anyways just translate
to OF properties in the OF case.

Okay, I don't know how device properties work, though.

They are pretty much 1-to-1 slot-ins for the corresponding of_*
functions, passing struct device * instead of struct device_node *,
if you look in include/linux/property.h you will feel at home very
quickly.

quoted

quoted

quoted

+static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)

Rename all functions pinctrl_gpio_*

Well, this change will result in name conflicts against existing
pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.

Yeah that works, or pincontro_by_gpio_ or such.

I will use "pin_control_gpio_", which still sounds confusing though.
Please modify it if you don't like.

quoted

Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.

I wrote some documentation! But it is hidden deep in the docs:
https://docs.kernel.org/driver-api/gpio/driver.html#gpio-lines-with-open-drain-source-support

quoted

In order to be able to read a value as an input pin, I think, we need
to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
In this case, however, we cannot distinguish the other case where we want
to use the pin as OUTPUT and drive it to (active) high.

With open drain, on GPIO controllers that do not support a native
open drain mode, we emulate open drain output high by switching
the line into input mode. The line in this case has a pull-up resistor
(internal or external) and as input mode is high-Z the pull up resistor
will pull the signal high, to any level - could be e.g 48V which is
helpful for some serial links.

I now think I see what you meant here, but still not sure why we need to
assert CONFIG_INPUT and CONFIG_OUT at the same time from API viewpoint.

Anyhow, I will follow the logic that you suggested.

But this case is really tricky so it can be hard to get things right,
I get a bit confused and so we need to think about it a few times.

quoted

quoted

quoted

+static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)

static int?

Unfortunately, the function prototype of "set" in struct gpio_device is
        void (*set)(struct gpio_chip *gc, unsigned int offset, int value);

So we cannot propagate an error to the caller.

Grrr that must be my fault. Sorry about not fixing this :(

quoted

quoted

No need to add & 0x01, the gpiolib core already does this.

Which part of gpiolib core?

chip->set = scmi_gpio_set; gets called like this in gpiolib:

 gpiod_direction_output_raw_commit(..., int value)
{
    int val = !!value;
(...)
    gc->set(gc, gpio_chip_hwgpio(desc), val);

Notice clamping int val = !!value; will make the passed val 0 or 1.

Yeah.

quoted

quoted

quoted

+static u16 sum_up_ngpios(struct gpio_chip *chip)
+{
+       struct gpio_pin_range *range;
+       struct gpio_device *gdev = chip->gpiodev;
+       u16 ngpios = 0;
+
+       list_for_each_entry(range, &gdev->pin_ranges, node) {
+               ngpios += range->range.npins;
+       }

This works but isn't really the intended use case of the ranges.
Feel a bit uncertain about it, but I can't think of anything better.
And I guess these come directly out of SCMI so it's first hand
information about all GPIOs.

I don't get your point.
However many pins SCMI firmware (or other normal pin controllers) might
expose, the total number of pins available by this driver is limited by
"gpio-ranges" property.
So the sum as "ngpios" should make sense unless a user accidentally
specifies a wrong range of pins.

Yes.

And it is this fact that the same number need to appear in two places
and double-specification will sooner or later bring us to the situation
where the two do not agree, and what do we do then?

If the ranges come from firmware, which is subject to change such
as "oops we forgot this pin", the GPIO number will just insert itself
among the already existing ones: say we have two ranges:

1: 0..5
2: 6..9

Ooops forgot a GPIO in the first range, it has to be bumped to
0..6.

But somewhere in the device tree there is:

foo-gpios = <&scmi_gpio 7 GPIO_OUT_LOW>;

So now this is wrong (need to be changed to 8) and we have zero tooling
to detect this, the author just has to be very careful all the time.

Well, even without a change by an user, this kind of human error
may happen. There is no way to verify the correct *pin number*,
say, if I specify 100 instead of 7 in an above example.

But I honestly do not know any better way.

One good practice to mitigate those cases might be to use a (gpio or
gpio-group) name instead of a pin number, or a "virtual" gpio device.

        foo_gpio: gpio@0 {
            compatibles = "pin-control-gpio";

            gpio-range = <&scmi_pinctrl 0 0 0>;
            gpio-range-group-name = "pins_for_foo";
        }
        baa_gpio: gpio@1 {
            compatibles = "pin-control-gpio";

            gpio-range = <&scmi_pinctrl 0 0 0>;
            gpio-range-group-name = "pins_for_baa";
        }

# Not sure multiple "pin-control-gpio" devices are possible.

-Takahiro Akashi

quoted

quoted

which in turn becomes just pinctrl_gpio_set_config(), which
is what we want.

The second cell in two-cell GPIOs already supports passing
GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE,
GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE,
which you can this way trivially pass down to the pin control driver.

NB: make sure the scmi pin control driver returns error for
unknown configs.

Well, the error will be determined by SCMI firmware(server)
not the driver itself :)

Hehe, I think it is good that the SCMI firmware gets some exercise
from day 1!

Yours,
Linus Walleij