[RFC v2 0/5] gpio: add pinctrl based generic gpio driver

[RFC v2 0/5] gpio: add pinctrl based generic gpio driver

[AKASHI Takahiro]

This is a revised version of my previous RFC[1]. Although I modified
the commits to make them look SCMI-independent, they are still posted
as RFC because I have never tested them on real hardware.

(background)
I'm currently working on implementing SCMI pinctrl/gpio drivers
on U-Boot[2]. Although the pinctrl driver for the kernel[3] 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.

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[4], table-23.)

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

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

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

3) generic GPIO driver
   Based on (2), I tried to prototype a generic driver in patch#4.
   Thanks to a set of existing pinctrl_gpio helper functions, except (1),
   It seems that the driver can be implemented not relying on pin controller
   specific code, at least for SCMI pinctrl.

I will appreciate any comments.

-Takahiro Akashi

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

AKASHI Takahiro (5):
  pinctrl: define PIN_CONFIG_INPUT
  pinctrl: always export pin_config_get_for_pin()
  pinctrl: add pinctrl_gpio_get_config()
  gpio: add pinctrl based generic gpio driver
  dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

 .../bindings/gpio/pin-control-gpio.yaml       |  55 ++++++
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-by-pinctrl.c                | 165 ++++++++++++++++++
 drivers/pinctrl/core.c                        |  19 ++
 drivers/pinctrl/pinconf.h                     |  10 +-
 include/linux/pinctrl/consumer.h              |   8 +
 include/linux/pinctrl/pinconf-generic.h       |   5 +
 8 files changed, 268 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
 create mode 100644 drivers/gpio/gpio-by-pinctrl.c

-- 
2.34.1

[RFC v2 1/5] pinctrl: define PIN_CONFIG_INPUT

[AKASHI Takahiro]

This configuration is intended to be used to allow a pin controller based
GPIO driver to obtain a value at a gpio input pin.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
RFC v2 (Oct 5, 2023)
* improve a comment against @PIN_CONFIG_INPUT as per Linus
RFC(Oct 2, 2023)
---
 include/linux/pinctrl/pinconf-generic.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d74b7a4ea154..da0d80aa532d 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -67,6 +67,10 @@ 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. To put a line
+ *	into input mode, @PIN_CONFIG_INPUT_ENABLE must be used. Otherwise,
+ *	an error will be returned. The returned argument is 1 for logic high
+ *	and 0 for logic low.
  * @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 +132,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

[RFC v2 2/5] pinctrl: always export pin_config_get_for_pin()

[AKASHI Takahiro]

This function will be used to implement a new pinctrl_gpio_get_config()
outside pinconf.c in a succeeding commit.
So make it always visible to avoid a kernel test bot error.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310021320.gYfm1nLQ-lkp@intel.com/
---
RFC v2 (Oct 5, 2023)
* new
---
 drivers/pinctrl/pinconf.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 694bfc9961fa..068089b199e4 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -31,13 +31,13 @@ int pinconf_apply_setting(const struct pinctrl_setting *setting);
 
 int pinconf_set_config(struct pinctrl_dev *pctldev, unsigned pin,
 		       unsigned long *configs, size_t nconfigs);
+int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
+			   unsigned long *config);
 
 /*
  * You will only be interested in these if you're using PINCONF
  * so don't supply any stubs for these.
  */
-int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
-			   unsigned long *config);
 int pin_config_group_get(const char *dev_name, const char *pin_group,
 			 unsigned long *config);
 
@@ -74,6 +74,12 @@ static inline int pinconf_set_config(struct pinctrl_dev *pctldev, unsigned pin,
 	return -ENOTSUPP;
 }
 
+static inline int pin_config_get_for_pin(struct pinctrl_dev *pctldev,
+					 unsigned pin, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
 #endif
 
 #if defined(CONFIG_PINCONF) && defined(CONFIG_DEBUG_FS)
-- 
2.34.1

[RFC v2 3/5] pinctrl: add pinctrl_gpio_get_config()

[AKASHI Takahiro]

This is a counterpart of pinctrl_gpio_set_config() which will be used,
at least initially, to implement gpio_get interface in pin controller
based generic gpio driver.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
RFC (Oct 2, 2023)
---
 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

[RFC v2 4/5] gpio: add pinctrl based generic gpio driver

[AKASHI Takahiro]

Some pin controllers provide not only a method to set up lines but
also gpio function. With this commit, a new generic gpio driver will
be provided. It is implemented purely by using pinctrl interfaces.
One of such pin controllers is Arm's SCMI.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
RFC v2 (Oct 5, 2023)
* rename the driver to pin-control-gpio (CONFIG_GPIO_BY_PINCTRL)
* return meaningful error codes instead of -1
* remove the masking at PIN_CONFIG_PACKED
* handle emulated OPEN_DRAIN configuration at get_direction()
* define config_set in gpio_chip
* drop remove hook
RFC (Oct 2, 2023)
---
 drivers/gpio/Kconfig           |   7 ++
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-by-pinctrl.c | 165 +++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/gpio/gpio-by-pinctrl.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 673bafb8be58..a60972be114c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -216,6 +216,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, specifically pin configuration, such as SCMI.
+
 config GPIO_CADENCE
 	tristate "Cadence GPIO support"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..71458d81e16a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_GPIO_BD71828)		+= gpio-bd71828.o
 obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.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_CLPS711X)		+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_SNPS_CREG)		+= gpio-creg-snps.o
diff --git a/drivers/gpio/gpio-by-pinctrl.c b/drivers/gpio/gpio-by-pinctrl.c
new file mode 100644
index 000000000000..c297a9633e03
--- /dev/null
+++ b/drivers/gpio/gpio-by-pinctrl.c
@@ -0,0 +1,165 @@
+// 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/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include "gpiolib.h"
+
+struct pin_control_gpio_priv {
+	struct gpio_chip chip;
+};
+
+static int pin_control_gpio_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	unsigned long config;
+	bool out_en, in_en;
+	int ret;
+
+	config = PIN_CONFIG_OUTPUT_ENABLE;
+	ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+	if (!ret)
+		out_en = !!config;
+	else if (ret == -EINVAL)
+		out_en = false;
+	else
+		return ret;
+
+	config = PIN_CONFIG_INPUT_ENABLE;
+	ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+	if (!ret)
+		in_en = !!config;
+	else if (ret == -EINVAL)
+		in_en = false;
+	else
+		return ret;
+
+	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 may be an emulation for output with open drain */
+		config = PIN_CONFIG_DRIVE_OPEN_DRAIN;
+		ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset,
+					      &config);
+		if (!ret && config)
+			return GPIO_LINE_DIRECTION_OUT;
+	}
+
+	return -EINVAL;
+}
+
+static int pin_control_gpio_direction_input(struct gpio_chip *chip,
+					    unsigned int offset)
+{
+	return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
+}
+
+static int pin_control_gpio_direction_output(struct gpio_chip *chip,
+					     unsigned int offset, int val)
+{
+	return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
+}
+
+static int pin_control_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	unsigned long config;
+	int ret;
+
+	config = PIN_CONFIG_INPUT;
+	ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+	if (ret)
+		return ret;
+
+	if (config >> 8)
+		return 1;
+
+	return 0;
+}
+
+static void pin_control_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				 int val)
+{
+	unsigned long config;
+
+	config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val);
+
+	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 pin_control_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pin_control_gpio_priv *priv;
+	struct gpio_chip *chip;
+	int ret;
+
+	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 = pin_control_gpio_get_direction;
+	chip->direction_input = pin_control_gpio_direction_input;
+	chip->direction_output = pin_control_gpio_direction_output;
+	chip->get = pin_control_gpio_get;
+	chip->set = pin_control_gpio_set;
+	chip->set_config = gpiochip_generic_config;
+
+	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 const struct of_device_id pin_control_gpio_match[] = {
+	{ .compatible = "pin-control-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pin_control_gpio_match);
+
+static struct platform_driver pin_control_gpio_driver = {
+	.probe = pin_control_gpio_probe,
+	.driver = {
+		.name = "pin-control-gpio",
+		.of_match_table = pin_control_gpio_match,
+	},
+};
+module_platform_driver(pin_control_gpio_driver);
+
+MODULE_AUTHOR("AKASHI Takahiro <takahiro.akashi@linaro.org>");
+MODULE_DESCRIPTION("Pinctrl based GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

A dt binding for pin controller based generic gpio driver is defined in
this commit. One usable device is Arm's SCMI.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
RFC v2 (Oct 5, 2023)
* rename the binding to pin-control-gpio
* add the "description"
* remove nodename, hog properties, and a consumer example
RFC (Oct 2, 2023)
---
 .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
new file mode 100644
index 000000000000..bc935dbd7edb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
@@ -0,0 +1,55 @@
+# 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:
+  - AKASHI Takahiro <akashi.takahiro@linaro.org>
+
+properties:
+  compatible:
+    const: pin-control-gpio
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+
+    required:
+      - gpio-hog
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio@0 {
+        compatible = "pin-control-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&scmi_pinctrl 0 10 5>,
+                      <&scmi_pinctrl 5 0 0>;
+        gpio-ranges-group-names = "",
+                                  "pinmux_gpio";
+    };
-- 
2.34.1

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Krzysztof Kozlowski]

On 05/10/2023 04:58, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio@0 {

No reg, so no unit address.

Drop also unused label.


> +        compatible = "pin-control-gpio";
> +        gpio-controller;

Best regards,
Krzysztof

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Rob Herring]

On Thu, 05 Oct 2023 11:58:43 +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-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/pin-control-gpio.example.dts:18.23-26.11: Warning (unit_address_vs_reg): /example-0/gpio@0: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231005025843.508689-6-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.

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Rob Herring]

On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.

You don't need a "generic" binding to have a generic driver. Keep the 
binding specific and then decide in the OS to whether to use a generic 
or specific driver. That decision could change over time, but the 
binding can't. For example, see simple-panel.


> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> new file mode 100644
> index 000000000000..bc935dbd7edb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> @@ -0,0 +1,55 @@
> +# 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:
> +  - AKASHI Takahiro <akashi.takahiro@linaro.org>
> +
> +properties:
> +  compatible:
> +    const: pin-control-gpio
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges: true
> +
> +  gpio-ranges-group-names: true
> +
> +patternProperties:
> +  "^.+-hog(-[0-9]+)?$":
> +    type: object
> +
> +    required:
> +      - gpio-hog
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };
> -- 
> 2.34.1
> 

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:

> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
>
> You don't need a "generic" binding to have a generic driver. Keep the
> binding specific and then decide in the OS to whether to use a generic
> or specific driver. That decision could change over time, but the
> binding can't. For example, see simple-panel.

What you say is true for simple-panel (a word like "simple" should
always cause red flags).

This case is more like mfd/syscon.yaml, where the singular
compatible = "syscon"; is in widespread use:

$ git grep 'compatible = \"syscon\";' |wc -l
50

I would accept adding a tuple compatible if you insist, so:

compatible = "foo-silicon", "pin-contro-gpio";

One case will be something like:

compatible = "optee-scmi-pin-control", "pin-control-gpio";

In this case I happen to know that we have the problem of
this being standardization work ahead of implementation on
actual hardware, and that is driven by the will known firmware
ambition to be completely abstract. It is supposed to sit on
top of pin control, or as part of pin control. Which leads me to
this thing (which I didn't think of before...)

> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };

Maybe we should require that the pin-control-gpio node actually
be *inside* the pin control node, in this case whatever the label
&scmi_pinctrl is pointing to?

We can probably mandate that this has to be inside a pin controller
since it is a first.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Cristian Marussi]

On Mon, Oct 09, 2023 at 09:49:33AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> 

Hi Linus and all,

> > > A dt binding for pin controller based generic gpio driver is defined in
> > > this commit. One usable device is Arm's SCMI.
> >
> > You don't need a "generic" binding to have a generic driver. Keep the
> > binding specific and then decide in the OS to whether to use a generic
> > or specific driver. That decision could change over time, but the
> > binding can't. For example, see simple-panel.
> 
> What you say is true for simple-panel (a word like "simple" should
> always cause red flags).
> 
> This case is more like mfd/syscon.yaml, where the singular
> compatible = "syscon"; is in widespread use:
> 
> $ git grep 'compatible = \"syscon\";' |wc -l
> 50
> 
> I would accept adding a tuple compatible if you insist, so:
> 
> compatible = "foo-silicon", "pin-contro-gpio";
> 
> One case will be something like:
> 
> compatible = "optee-scmi-pin-control", "pin-control-gpio";
> 
> In this case I happen to know that we have the problem of
> this being standardization work ahead of implementation on
> actual hardware, and that is driven by the will known firmware
> ambition to be completely abstract. It is supposed to sit on
> top of pin control, or as part of pin control. Which leads me to
> this thing (which I didn't think of before...)
> 
> > +    gpio0: gpio@0 {
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > +                      <&scmi_pinctrl 5 0 0>;
> > +        gpio-ranges-group-names = "",
> > +                                  "pinmux_gpio";
> > +    };
> 

Assuming the above &scmi_pinctrl refers to the protocol node as we
usually do,  I am a bit puzzled by this example in this RFC series, because
usually in SCMI we DO refer to some resources using the phandle and the
domain IDs as in:

	scmi_sensor: protocol@15 {
		reg = <15>;
		#thermal-sensors-cells = <1>;
	};

	...

	thermal_zones {
		pmic {
			thermal-sensor = <&scmi_sensor 0>;
		};
	};
 
BUT in the SCMI Pinctrl case the current v4 Oleksii series takes advantage
of the existing Pinctrl bindings and naming to describe and refer to
pin/groups/functions, indeed #pinctrl-cells is defined as '0' in the
upcoming SCMI DT protocol node @19 in Oleksii v4, since indeed all the
parsing/matching is done by resource-names following the Picntrl
framework conventions. (AFAIU)

Moreover, due to how the SCMI Pinctrl protocol defines and describes the
pins/groups/functions using a tuple like (<TYPE>, <ID>) , with TYPE
being pin/group/function, a generic binding like the above would have to
be defined by at least 2 cells to be able to identify an SCMI PinCtrl
resource by index. (if that is the aim here...)

Am I right to think that such a generic SCMI PinCtrl binding is still to
be defined somewhere on the SCMI side, if needed as such by this GPIO driver ?

... or I am missing something ?

Thanks,
Cristian

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

On Mon, Oct 9, 2023 at 11:08 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:

> > > +    gpio0: gpio@0 {
> > > +        compatible = "pin-control-gpio";
> > > +        gpio-controller;
> > > +        #gpio-cells = <2>;
> > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > +                      <&scmi_pinctrl 5 0 0>;
> > > +        gpio-ranges-group-names = "",
> > > +                                  "pinmux_gpio";
> > > +    };
> >
>
> Assuming the above &scmi_pinctrl refers to the protocol node as we
> usually do,

No it does not, it is a three-layer cake.

scmi <-> scmi_pinctrl <-> scmi_gpio

it refers to the scmi_pinctrl node.

There is no SCMI GPIO protocol, instead SCMI is using the
operations already available in the pin controller to exercise
GPIO. Generic pin control has operations to drive lines for
example, and Takahiro is adding the ability for a generic pin
controller to also read a line.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Cristian Marussi]

On Mon, Oct 09, 2023 at 03:13:24PM +0200, Linus Walleij wrote:
> On Mon, Oct 9, 2023 at 11:08 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> 
> > > > +    gpio0: gpio@0 {
> > > > +        compatible = "pin-control-gpio";
> > > > +        gpio-controller;
> > > > +        #gpio-cells = <2>;
> > > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > > +                      <&scmi_pinctrl 5 0 0>;
> > > > +        gpio-ranges-group-names = "",
> > > > +                                  "pinmux_gpio";
> > > > +    };
> > >
> >
> > Assuming the above &scmi_pinctrl refers to the protocol node as we
> > usually do,
> 
> No it does not, it is a three-layer cake.
> 
> scmi <-> scmi_pinctrl <-> scmi_gpio
> 
> it refers to the scmi_pinctrl node.
> 

Thanks, this explains a lot.
Cristian

> There is no SCMI GPIO protocol, instead SCMI is using the
> operations already available in the pin controller to exercise
> GPIO. Generic pin control has operations to drive lines for
> example, and Takahiro is adding the ability for a generic pin
> controller to also read a line.


> 
> Yours,
> Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

On Mon, Oct 09, 2023 at 04:08:13PM +0100, Cristian Marussi wrote:
> On Mon, Oct 09, 2023 at 03:13:24PM +0200, Linus Walleij wrote:
> > On Mon, Oct 9, 2023 at 11:08???AM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > 
> > > > > +    gpio0: gpio@0 {
> > > > > +        compatible = "pin-control-gpio";
> > > > > +        gpio-controller;
> > > > > +        #gpio-cells = <2>;
> > > > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > > > +                      <&scmi_pinctrl 5 0 0>;
> > > > > +        gpio-ranges-group-names = "",
> > > > > +                                  "pinmux_gpio";
> > > > > +    };
> > > >
> > >
> > > Assuming the above &scmi_pinctrl refers to the protocol node as we
> > > usually do,
> > 
> > No it does not, it is a three-layer cake.
> > 
> > scmi <-> scmi_pinctrl <-> scmi_gpio
> > 
> > it refers to the scmi_pinctrl node.
> > 
> 
> Thanks, this explains a lot.
> Cristian

Just in case, 

    gpio-ranges = <&scmi_pinctrl 0 10 5>;

means that SCMI *pin* range [10..(10+5-1)] are mapped to this driver's
gpio range [0..(5-1)]. So any consumer driver can access a gpio pin
as:
    foo-gpios = <&gpio0 3>;

will refer to gpio pin#3 that is actually SCMI's 13.

    gpio-ranges = <&scmi_pinctrl 5 0 0>;
    gpio-ranges-group-names = "pinmux_gpio";

means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
range [20..24],

    baa-gpios = <&gpio0 7>;
will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

This way, we (consumer drivers) don't care what is the underlying pin
controller.

-Takahiro Akashi

> 
> > There is no SCMI GPIO protocol, instead SCMI is using the
> > operations already available in the pin controller to exercise
> > GPIO. Generic pin control has operations to drive lines for
> > example, and Takahiro is adding the ability for a generic pin
> > controller to also read a line.
> 
> 
> > 
> > Yours,
> > Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

On Mon, Oct 09, 2023 at 09:49:33AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 3:23???PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> 
> > > A dt binding for pin controller based generic gpio driver is defined in
> > > this commit. One usable device is Arm's SCMI.
> >
> > You don't need a "generic" binding to have a generic driver. Keep the
> > binding specific and then decide in the OS to whether to use a generic
> > or specific driver. That decision could change over time, but the
> > binding can't. For example, see simple-panel.
> 
> What you say is true for simple-panel (a word like "simple" should
> always cause red flags).
> 
> This case is more like mfd/syscon.yaml, where the singular
> compatible = "syscon"; is in widespread use:
> 
> $ git grep 'compatible = \"syscon\";' |wc -l
> 50
> 
> I would accept adding a tuple compatible if you insist, so:
> 
> compatible = "foo-silicon", "pin-contro-gpio";
> 
> One case will be something like:
> 
> compatible = "optee-scmi-pin-control", "pin-control-gpio";
> 
> In this case I happen to know that we have the problem of
> this being standardization work ahead of implementation on
> actual hardware, and that is driven by the will known firmware
> ambition to be completely abstract. It is supposed to sit on
> top of pin control, or as part of pin control. Which leads me to
> this thing (which I didn't think of before...)
> 
> > +    gpio0: gpio@0 {
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > +                      <&scmi_pinctrl 5 0 0>;
> > +        gpio-ranges-group-names = "",
> > +                                  "pinmux_gpio";
> > +    };
> 
> Maybe we should require that the pin-control-gpio node actually
> be *inside* the pin control node, in this case whatever the label
> &scmi_pinctrl is pointing to?

null (or '_' as dummy) if the dt schema allows such a value as
a trivial case?

> We can probably mandate that this has to be inside a pin controller
> since it is a first.

Yeah, my U-Boot implementation tentatively supports both (inside and
outside pin controller). But it is not a user's choice, but we should
decide which way to go.

Thanks,
-Takahiro Akashi

> Yours,
> Linus Walleij

Re: [RFC v2 1/5] pinctrl: define PIN_CONFIG_INPUT

[Linus Walleij]

On Thu, Oct 5, 2023 at 4:59 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> This configuration is intended to be used to allow a pin controller based
> GPIO driver to obtain a value at a gpio input pin.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC v2 2/5] pinctrl: always export pin_config_get_for_pin()

[Linus Walleij]

On Thu, Oct 5, 2023 at 4:59 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:


> This function will be used to implement a new pinctrl_gpio_get_config()
> outside pinconf.c in a succeeding commit.
> So make it always visible to avoid a kernel test bot error.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202310021320.gYfm1nLQ-lkp@intel.com/

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC v2 4/5] gpio: add pinctrl based generic gpio driver

[Linus Walleij]

On Thu, Oct 5, 2023 at 4:59 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:


> Some pin controllers provide not only a method to set up lines but
> also gpio function. With this commit, a new generic gpio driver will
> be provided. It is implemented purely by using pinctrl interfaces.
> One of such pin controllers is Arm's SCMI.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)

RFC v2 looks very good to me, definitely something that can be merged
as a starting point once the hardware has been tested.

> +static int pin_control_gpio_direction_input(struct gpio_chip *chip,
> +                                           unsigned int offset)
> +{
> +       return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
> +}
> +
> +static int pin_control_gpio_direction_output(struct gpio_chip *chip,
> +                                            unsigned int offset, int val)
> +{
> +       return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
> +}

IIRC Bartosz is working on a patch set getting rid of this kludge having to
call with base + offset in every driver, replacing it with generic calls that
you can just assign in the gpio_chip.

When this gets applied these changes will likely be in place so you will
get rid of this too.

Yours,
Linus Walleij

Re: [RFC v2 4/5] gpio: add pinctrl based generic gpio driver

[AKASHI Takahiro]

Hi Linus and Oleksii,

On Tue, Oct 10, 2023 at 02:00:40PM +0200, Linus Walleij wrote:
> On Thu, Oct 5, 2023 at 4:59???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> 
> > Some pin controllers provide not only a method to set up lines but
> > also gpio function. With this commit, a new generic gpio driver will
> > be provided. It is implemented purely by using pinctrl interfaces.
> > One of such pin controllers is Arm's SCMI.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > RFC v2 (Oct 5, 2023)
> 
> RFC v2 looks very good to me, definitely something that can be merged
> as a starting point once the hardware has been tested.

Thank you for your support.
I think the easiest and best way to test the code is that Oleskii will try
my patch on his platform, r-car, on which I believe that SCMI FW for pin
controller is already available since he tested his pinctrl driver.

@Oleskii, can you please take a time for the test?
(I will assist you in case of any error.)

> > +static int pin_control_gpio_direction_input(struct gpio_chip *chip,
> > +                                           unsigned int offset)
> > +{
> > +       return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
> > +}
> > +
> > +static int pin_control_gpio_direction_output(struct gpio_chip *chip,
> > +                                            unsigned int offset, int val)
> > +{
> > +       return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
> > +}
> 
> IIRC Bartosz is working on a patch set getting rid of this kludge having to
> call with base + offset in every driver, replacing it with generic calls that
> you can just assign in the gpio_chip.
> 
> When this gets applied these changes will likely be in place so you will
> get rid of this too.

I will try to keep eyes on Bartosz's patch.

Thanks,
-Takahiro Akashi


> Yours,
> Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

On Thu, Oct 05, 2023 at 09:48:09PM +0200, Krzysztof Kozlowski wrote:
> On 05/10/2023 04:58, AKASHI Takahiro wrote:
> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> > +
> > +required:
> > +  - compatible
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +  - gpio-ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio0: gpio@0 {
> 
> No reg, so no unit address.

My intention was to allow for multiple nodes (instances) of
pinctrl based gpio devices. But I don't care the naming.

> Drop also unused label.

Okay, I already dropped an example consumer device and have no need
for the label any longer.

-Takahiro Akashi

> 
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> 
> Best regards,
> Krzysztof
> 

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

On Tue, Oct 10, 2023 at 7:25 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > We can probably mandate that this has to be inside a pin controller
> > since it is a first.
>
> Yeah, my U-Boot implementation tentatively supports both (inside and
> outside pin controller). But it is not a user's choice, but we should
> decide which way to go.

OK I have decided we are going to put it inside the pin control node,
as a subnode. (I don't expect anyone to object.)

It makes everything easier and clearer for users I think.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Krzysztof Kozlowski]

On 12/10/2023 03:15, AKASHI Takahiro wrote:

>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    gpio0: gpio@0 {
>>
>> No reg, so no unit address.
> 
> My intention was to allow for multiple nodes (instances) of
> pinctrl based gpio devices. But I don't care the naming.

How can you have unit address without reg? This causes warnings.

Best regards,
Krzysztof

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

Hi Linus,

On Thu, Oct 12, 2023 at 09:25:20AM +0200, Linus Walleij wrote:
> On Tue, Oct 10, 2023 at 7:25???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > We can probably mandate that this has to be inside a pin controller
> > > since it is a first.
> >
> > Yeah, my U-Boot implementation tentatively supports both (inside and
> > outside pin controller). But it is not a user's choice, but we should
> > decide which way to go.
> 
> OK I have decided we are going to put it inside the pin control node,
> as a subnode. (I don't expect anyone to object.)

While I'm still thinking of how I can modify my current implementation
to fit into 'inside' syntax, there are a couple of concerns:

1) invoke gpiochip_add_data() at probe function
Probably we no longer need "compatible" property, but instead we need to
call gpiochip_add_data() explicitly in SCMI pin controller's probe
as follows:

scmi_pinctrl_probe()
    ...
    devm_pinctrl_register_and_init(dev, ..., pctrldev);
    pinctrl_enable(pctrldev);

    device_for_each_child_node(dev, fwnode)
        if (fwnode contains "gpio-controller") {
            /* what pin_control_gpio_probe() does */
            gc->get_direction = ...;
            ...
            devm_gpiochip_data_add(dev, gc, ...);
        }

2) gpio-by-pinctrl.c
While this file is SCMI-independent now, due to a change at (1),
it would be better to move the whole content inside SCMI pin controller
driver (because there is no other user for now).

3) Then, pin-control-gpio.yaml may also be put into SCMI binding
(i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?

4) phandle in "gpio-ranges" property
(As you mentioned)
The first element in a tuple of "gpio-ranges" is a phandle to a pin
controller node. Now that the gpio node is a sub node of pin controller,
the phandle is trivial. But there is no easier way to represent it
than using an explicit label:
(My U-Boot implementation does this.)

scmi {
    ...
    scmi_pinctrl: protocol@19 {
        ...
        gpio {
            gpio-controller;
            ...
            gpio-ranges = <&scmi_pinctrl ... >;
        }
    }
}

I tried:
    gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
    gpio-ranges = <(-1) ...>; // dtc passed, but ...
    gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.

Do you have any other idea? Otherwise, I will modify my RFC
with the changes above.

-Takahiro Akashi


> It makes everything easier and clearer for users I think.
> 
> Yours,
> Linus Walleij

Re: [RFC v2 0/5] gpio: add pinctrl based generic gpio driver

[andy.shevchenko]

Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> This is a revised version of my previous RFC[1]. Although I modified
> the commits to make them look SCMI-independent, they are still posted
> as RFC because I have never tested them on real hardware.
> 
> (background)
> I'm currently working on implementing SCMI pinctrl/gpio drivers
> on U-Boot[2]. Although the pinctrl driver for the kernel[3] 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.
> 
> 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[4], table-23.)
> 
>    The current pinconf framework is missing the feature (the pinconf
>    parameter and a helper function). See patch#1, #2 and #3.
> 
>    Please note that there is an issue around the pin configuration in
>    EPAM's current pinctrl driver as I commented[5].
> 
> 2) DT bindings
>    I would like to propose a generic binding for pinctrl based gpio driver.
>    This allows a "consumer" driver to handle gpio pins like as other
>    normal gpio controllers support. (patch#5)
> 
> 3) generic GPIO driver
>    Based on (2), I tried to prototype a generic driver in patch#4.
>    Thanks to a set of existing pinctrl_gpio helper functions, except (1),
>    It seems that the driver can be implemented not relying on pin controller
>    specific code, at least for SCMI pinctrl.
> 
> I will appreciate any comments.

Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
not Cc'ed on this? I definitely have some comments against the code (no DT,
though). Please, use (up-to-date) MAINTAINERS in your v3.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC v2 0/5] gpio: add pinctrl based generic gpio driver

[AKASHI Takahiro]

Hi Andy,

On Fri, Oct 20, 2023 at 12:27:58AM +0300, andy.shevchenko@gmail.com wrote:
> Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> > This is a revised version of my previous RFC[1]. Although I modified
> > the commits to make them look SCMI-independent, they are still posted
> > as RFC because I have never tested them on real hardware.
> > 
> > (background)
> > I'm currently working on implementing SCMI pinctrl/gpio drivers
> > on U-Boot[2]. Although the pinctrl driver for the kernel[3] 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.
> > 
> > 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[4], table-23.)
> > 
> >    The current pinconf framework is missing the feature (the pinconf
> >    parameter and a helper function). See patch#1, #2 and #3.
> > 
> >    Please note that there is an issue around the pin configuration in
> >    EPAM's current pinctrl driver as I commented[5].
> > 
> > 2) DT bindings
> >    I would like to propose a generic binding for pinctrl based gpio driver.
> >    This allows a "consumer" driver to handle gpio pins like as other
> >    normal gpio controllers support. (patch#5)
> > 
> > 3) generic GPIO driver
> >    Based on (2), I tried to prototype a generic driver in patch#4.
> >    Thanks to a set of existing pinctrl_gpio helper functions, except (1),
> >    It seems that the driver can be implemented not relying on pin controller
> >    specific code, at least for SCMI pinctrl.
> > 
> > I will appreciate any comments.
> 
> Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
> not Cc'ed on this?

My apologies. I will add you in Cc.

> I definitely have some comments against the code (no DT,
> though). Please, use (up-to-date) MAINTAINERS in your v3.

Please don't hesitate to make comments here on v2 so that I can
include your reviews in v3.

Thanks,
-Takahiro Akashi


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

Hi Takashi,

sorry for slow response :(

On Tue, Oct 17, 2023 at 4:32 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > > > We can probably mandate that this has to be inside a pin controller
> > > > since it is a first.
> > >
> > > Yeah, my U-Boot implementation tentatively supports both (inside and
> > > outside pin controller). But it is not a user's choice, but we should
> > > decide which way to go.
> >
> > OK I have decided we are going to put it inside the pin control node,
> > as a subnode. (I don't expect anyone to object.)
>
> While I'm still thinking of how I can modify my current implementation
> to fit into 'inside' syntax, there are a couple of concerns:
>
> 1) invoke gpiochip_add_data() at probe function
> Probably we no longer need "compatible" property,

The DT binding people made it clear to me that they really
like compatibles for this kind of stuff so we should probably
keep it.

> but instead we need to
> call gpiochip_add_data() explicitly in SCMI pin controller's probe
> as follows:
>
> scmi_pinctrl_probe()
>     ...
>     devm_pinctrl_register_and_init(dev, ..., pctrldev);
>     pinctrl_enable(pctrldev);
>
>     device_for_each_child_node(dev, fwnode)
>         if (fwnode contains "gpio-controller") {
>             /* what pin_control_gpio_probe() does */
>             gc->get_direction = ...;
>             ...
>             devm_gpiochip_data_add(dev, gc, ...);
>         }

I think it is better of the pin controller just parse and add any
subdevices (GPIO or other) using of_platform_default_populate()
(just grep for this function and you will see how many device
drivers use that).

What is good with this approach is that if you place this call
last in the probe() we know the GPIO driver has all resources
it needs when it probes so it won't defer.

> 2) gpio-by-pinctrl.c
> While this file is SCMI-independent now, due to a change at (1),
> it would be better to move the whole content inside SCMI pin controller
> driver (because there is no other user for now).

That works, too. I have no strong opinion on this subject.

> 3) Then, pin-control-gpio.yaml may also be put into SCMI binding
> (i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?

There is no clear pattern whether to put subdevice bindings into
the parent device binding or not. Maybe? A lot of MFD devices does
this for sure.

> 4) phandle in "gpio-ranges" property
> (As you mentioned)
> The first element in a tuple of "gpio-ranges" is a phandle to a pin
> controller node. Now that the gpio node is a sub node of pin controller,
> the phandle is trivial. But there is no easier way to represent it
> than using an explicit label:
> (My U-Boot implementation does this.)
>
> scmi {
>     ...
>     scmi_pinctrl: protocol@19 {
>         ...
>         gpio {
>             gpio-controller;
>             ...
>             gpio-ranges = <&scmi_pinctrl ... >;
>         }
>     }
> }
>
> I tried:
>     gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
>     gpio-ranges = <(-1) ...>; // dtc passed, but ...
>     gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.
>
> Do you have any other idea? Otherwise, I will modify my RFC
> with the changes above.

If you have the GPIO node inside the pin controller node
and have all the details of the existing ranges available, there
is no need to put that into the device tree at all, just omit it?

Instead just call gpiochip_add_pin_range() directly in Linux
after adding the pin controller and gpio_chip.
C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
doing this. In this case the SX150X is hot-plugged (on a slow
bus) so it needs to figure out all ranges at runtime anyway.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

Hi Linus,

On Mon, Oct 23, 2023 at 10:12:21AM +0200, Linus Walleij wrote:
> Hi Takashi,
> 
> sorry for slow response :(

Thank you for your feedback.

> On Tue, Oct 17, 2023 at 4:32???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > > > We can probably mandate that this has to be inside a pin controller
> > > > > since it is a first.
> > > >
> > > > Yeah, my U-Boot implementation tentatively supports both (inside and
> > > > outside pin controller). But it is not a user's choice, but we should
> > > > decide which way to go.
> > >
> > > OK I have decided we are going to put it inside the pin control node,
> > > as a subnode. (I don't expect anyone to object.)
> >
> > While I'm still thinking of how I can modify my current implementation
> > to fit into 'inside' syntax, there are a couple of concerns:
> >
> > 1) invoke gpiochip_add_data() at probe function
> > Probably we no longer need "compatible" property,
> 
> The DT binding people made it clear to me that they really
> like compatibles for this kind of stuff so we should probably
> keep it.

Okay, I will assume this in the following discussion.

> > but instead we need to
> > call gpiochip_add_data() explicitly in SCMI pin controller's probe
> > as follows:
> >
> > scmi_pinctrl_probe()
> >     ...
> >     devm_pinctrl_register_and_init(dev, ..., pctrldev);
> >     pinctrl_enable(pctrldev);
> >
> >     device_for_each_child_node(dev, fwnode)
> >         if (fwnode contains "gpio-controller") {
> >             /* what pin_control_gpio_probe() does */
> >             gc->get_direction = ...;
> >             ...
> >             devm_gpiochip_data_add(dev, gc, ...);
> >         }
> 
> I think it is better of the pin controller just parse and add any
> subdevices (GPIO or other) using of_platform_default_populate()
> (just grep for this function and you will see how many device
> drivers use that).

IICU, then, we will have to add a "compatible" to pinctrl node
to make of_platform_default_populate() work as expected. That is:

scmi {
    ...
    protocol@19 {
        compatible = "simple-bus"; // <- added
        reg = <0x19>;

        ... // a couple of pinconf nodes

        scmi_gpio {
            compatible = "pin-control-gpio";
            ...
        }
    }
}

Is this what you meant?
In this case, however, "protocol@19" has a mixture of sub-nodes,
most are pinconf definitions which are the properties of the pin
controller, while "scmi_gpio" is a separate device.

The code will work, but is it sane from DT binding pov?

> What is good with this approach is that if you place this call
> last in the probe() we know the GPIO driver has all resources
> it needs when it probes so it won't defer.
> 
> > 2) gpio-by-pinctrl.c
> > While this file is SCMI-independent now, due to a change at (1),
> > it would be better to move the whole content inside SCMI pin controller
> > driver (because there is no other user for now).
> 
> That works, too. I have no strong opinion on this subject.

I assumed that "compatible" had been removed here.
If we retain "compatible" property, I don't care either way.

> > 3) Then, pin-control-gpio.yaml may also be put into SCMI binding
> > (i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?
> 
> There is no clear pattern whether to put subdevice bindings into
> the parent device binding or not. Maybe? A lot of MFD devices does
> this for sure.

The same as above.

> > 4) phandle in "gpio-ranges" property
> > (As you mentioned)
> > The first element in a tuple of "gpio-ranges" is a phandle to a pin
> > controller node. Now that the gpio node is a sub node of pin controller,
> > the phandle is trivial. But there is no easier way to represent it
> > than using an explicit label:
> > (My U-Boot implementation does this.)
> >
> > scmi {
> >     ...
> >     scmi_pinctrl: protocol@19 {
> >         ...
> >         gpio {
> >             gpio-controller;
> >             ...
> >             gpio-ranges = <&scmi_pinctrl ... >;
> >         }
> >     }
> > }
> >
> > I tried:
> >     gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
> >     gpio-ranges = <(-1) ...>; // dtc passed, but ...
> >     gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.
> >
> > Do you have any other idea? Otherwise, I will modify my RFC
> > with the changes above.
> 
> If you have the GPIO node inside the pin controller node
> and have all the details of the existing ranges available, there
> is no need to put that into the device tree at all, just omit it?

Then, of_gpiochip_add_data() (hence, of_gpiochip_add()/gpiochip_add_data())
won't work because it always assume phandle + 3 arguments. Right?

In this case, "gpio-ranges" here may have different semantics from
the other pinctrl-based gpio drivers. Doesn't matter from DT pov?

> Instead just call gpiochip_add_pin_range() directly in Linux
> after adding the pin controller and gpio_chip.
> C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> doing this. In this case the SX150X is hot-plugged (on a slow
> bus) so it needs to figure out all ranges at runtime anyway.

Are you suggesting implementing a custom function for parsing "gpio-ranges"
and calling it in pin_control_gpio_probe() instead of a generic helper?

Or do you want to always map all the pin controller's pins to
gpio pins as sx150x does?

-Takahiro Akashi

> Yours,
> Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

Hi Takahiro,

On Tue, Oct 24, 2023 at 9:12 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > I think it is better of the pin controller just parse and add any
> > subdevices (GPIO or other) using of_platform_default_populate()
> > (just grep for this function and you will see how many device
> > drivers use that).
>
> IICU, then, we will have to add a "compatible" to pinctrl node
> to make of_platform_default_populate() work as expected. That is:
>
> scmi {
>     ...
>     protocol@19 {
>         compatible = "simple-bus"; // <- added

Hm right, but you could also use
of_platform_populate(np, NULL, NULL, dev);

Then the compatible match is of no concern.

Sorry for my lack of attention to details :/

If you want to restrict the population to a few select compatibles
(maybe only "pin-control-gpio") then you can do
that with

const struct of_device_id of_scmi_protocol_19_match_table[] = {
        { .compatible = "pin-control-gpio", },
        {}
};
of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);

> Is this what you meant?
> In this case, however, "protocol@19" has a mixture of sub-nodes,
> most are pinconf definitions which are the properties of the pin
> controller, while "scmi_gpio" is a separate device.

That looks good to me, it makes sense to have the GPIO as a subnode
here and mandate it with a compatible to match.

> The code will work, but is it sane from DT binding pov?

Let's let the DT people jump in on that.

> > Instead just call gpiochip_add_pin_range() directly in Linux
> > after adding the pin controller and gpio_chip.
> > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > doing this. In this case the SX150X is hot-plugged (on a slow
> > bus) so it needs to figure out all ranges at runtime anyway.
>
> Are you suggesting implementing a custom function for parsing "gpio-ranges"
> and calling it in pin_control_gpio_probe() instead of a generic helper?

The generic helper will always be attempted but if there are
no ranges in the device tree, it will just continue without adding
any ranges. I suggest putting *no* ranges into the device tree.

> Or do you want to always map all the pin controller's pins to
> gpio pins as sx150x does?

I think since the SCMI firmware knows about the available line
and pins etc, it makes sense that the driver comes up with the
applicable ranges on its own (derived from the information from
the SCMI firmware) and add them, instead of trying to put that
information into the device tree at all. Just omit it, and make your
own ranges, and add them in the Linux driver with
gpiochip_add_pin_range() without involving DT at all when defining
the ranges.

I'm sorry if I'm unclear sometimes.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Cristian Marussi]

On Tue, Oct 24, 2023 at 11:40:00AM +0200, Linus Walleij wrote:
> Hi Takahiro,
> 

Hi,

> On Tue, Oct 24, 2023 at 9:12 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > I think it is better of the pin controller just parse and add any
> > > subdevices (GPIO or other) using of_platform_default_populate()
> > > (just grep for this function and you will see how many device
> > > drivers use that).
> >
> > IICU, then, we will have to add a "compatible" to pinctrl node
> > to make of_platform_default_populate() work as expected. That is:
> >
> > scmi {
> >     ...
> >     protocol@19 {
> >         compatible = "simple-bus"; // <- added
> 
> Hm right, but you could also use
> of_platform_populate(np, NULL, NULL, dev);
> 
> Then the compatible match is of no concern.
> 
> Sorry for my lack of attention to details :/
> 
> If you want to restrict the population to a few select compatibles
> (maybe only "pin-control-gpio") then you can do
> that with
> 
> const struct of_device_id of_scmi_protocol_19_match_table[] = {
>         { .compatible = "pin-control-gpio", },
>         {}
> };
> of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);
> 
> > Is this what you meant?
> > In this case, however, "protocol@19" has a mixture of sub-nodes,
> > most are pinconf definitions which are the properties of the pin
> > controller, while "scmi_gpio" is a separate device.
> 
> That looks good to me, it makes sense to have the GPIO as a subnode
> here and mandate it with a compatible to match.
> 
> > The code will work, but is it sane from DT binding pov?
> 
> Let's let the DT people jump in on that.
> 
> > > Instead just call gpiochip_add_pin_range() directly in Linux
> > > after adding the pin controller and gpio_chip.
> > > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > > doing this. In this case the SX150X is hot-plugged (on a slow
> > > bus) so it needs to figure out all ranges at runtime anyway.
> >
> > Are you suggesting implementing a custom function for parsing "gpio-ranges"
> > and calling it in pin_control_gpio_probe() instead of a generic helper?
> 
> The generic helper will always be attempted but if there are
> no ranges in the device tree, it will just continue without adding
> any ranges. I suggest putting *no* ranges into the device tree.
> 
> > Or do you want to always map all the pin controller's pins to
> > gpio pins as sx150x does?
> 
> I think since the SCMI firmware knows about the available line
> and pins etc, it makes sense that the driver comes up with the
> applicable ranges on its own (derived from the information froms
> the SCMI firmware) and add them, instead of trying to put that
> information into the device tree at all. Just omit it, and make your
> own ranges, and add them in the Linux driver with
> gpiochip_add_pin_range() without involving DT at all when defining
> the ranges.
> 
> I'm sorry if I'm unclear sometimes.

...a maybe dumb question from my side, BUT does the SCMI Pinctrl carry
enough information as it stands for the driver to derive autonomously
and efficently the possible/applicable gpio ranges ?

Are they (GPIOs) all the remaining unassociated pins ?
If this is the case note that the SCMI Pinctrl lets you query the
associations in groups or functions and this is generally now done
only lazily on-demand when specific pins/groups/funcs are requested
by the parsed DT confs: IOW, in order to derive GPIOs from the set
of unassociated ones, you will have to at first add some new full-lookup
pinctrl_ops to query upfront all existing associations (avoiding, at will,
the lazy querying adopted now) and then singling-out the non-associated
ones from the lists of all possible group/funcs associations.

Moreover, should we allow anyway the optional possibility to forcibly
restrict the available gpios from the DT, or we can just assume that
those un-available (map out as above) wont just be exposed by the
SCMI server ?

..again sorry if I am missing something crucial here and just talking
non-sense but I have limited familiarity with Pinctrl/GPIOs usage.

Thanks
Cristian

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

Hi Linus,

On Tue, Oct 24, 2023 at 11:40:00AM +0200, Linus Walleij wrote:
> Hi Takahiro,
> 
> On Tue, Oct 24, 2023 at 9:12???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > I think it is better of the pin controller just parse and add any
> > > subdevices (GPIO or other) using of_platform_default_populate()
> > > (just grep for this function and you will see how many device
> > > drivers use that).
> >
> > IICU, then, we will have to add a "compatible" to pinctrl node
> > to make of_platform_default_populate() work as expected. That is:
> >
> > scmi {
> >     ...
> >     protocol@19 {
> >         compatible = "simple-bus"; // <- added
> 
> Hm right, but you could also use
> of_platform_populate(np, NULL, NULL, dev);
> 
> Then the compatible match is of no concern.
> 
> Sorry for my lack of attention to details :/
> 
> If you want to restrict the population to a few select compatibles
> (maybe only "pin-control-gpio") then you can do
> that with
> 
> const struct of_device_id of_scmi_protocol_19_match_table[] = {
>         { .compatible = "pin-control-gpio", },
>         {}
> };
> of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);
> 
> > Is this what you meant?
> > In this case, however, "protocol@19" has a mixture of sub-nodes,
> > most are pinconf definitions which are the properties of the pin
> > controller, while "scmi_gpio" is a separate device.
> 
> That looks good to me, it makes sense to have the GPIO as a subnode
> here and mandate it with a compatible to match.
> 
> > The code will work, but is it sane from DT binding pov?
> 
> Let's let the DT people jump in on that.
> 
> > > Instead just call gpiochip_add_pin_range() directly in Linux
> > > after adding the pin controller and gpio_chip.
> > > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > > doing this. In this case the SX150X is hot-plugged (on a slow
> > > bus) so it needs to figure out all ranges at runtime anyway.
> >
> > Are you suggesting implementing a custom function for parsing "gpio-ranges"
> > and calling it in pin_control_gpio_probe() instead of a generic helper?
> 
> The generic helper will always be attempted but if there are
> no ranges in the device tree, it will just continue without adding
> any ranges. I suggest putting *no* ranges into the device tree.
> 
> > Or do you want to always map all the pin controller's pins to
> > gpio pins as sx150x does?
> 
> I think since the SCMI firmware knows about the available line
> and pins etc, it makes sense that the driver comes up with the
> applicable ranges on its own (derived from the information from
> the SCMI firmware) and add them, instead of trying to put that
> information into the device tree at all. Just omit it, and make your
> own ranges, and add them in the Linux driver with
> gpiochip_add_pin_range() without involving DT at all when defining
> the ranges.

As far as I understand, there is only one way for SCMI gpio driver
to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
command with "Input-mode" or "Output-mode" configuration type
against *every* pin-controller's pins.
(Here I assume that the command would fail with INVALID_PARAMETERS or
NOT_SUPPORTED if configuring the given pin as a GPIO input or output
is not possible. But the specification seems to be a bit ambiguous.)

It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
to call the command 200 times in order to get the answer.

It is possible but I believe that it is clunky and painful for the driver
initialization.
I'd like to see explicit "gpio-ranges" property in a device tree.

Thanks,
-Takahiro Akashi



> I'm sorry if I'm unclear sometimes.
> 
> Yours,
> Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

On Tue, Oct 24, 2023 at 12:55 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:

> ...a maybe dumb question from my side, BUT does the SCMI Pinctrl carry
> enough information as it stands for the driver to derive autonomously
> and efficently the possible/applicable gpio ranges ?

I don't know, that's part of the problem I suppose. But if the
pin controller can report functions supported by certain pins
or groups of pins, then certainly "gpio" should be one of those
functions or else the pin cannot be used for GPIO at all?

Then maybe that function is just a name convention, such
as "all pins are members of a 1-pin group named 'gpioN'
where N is the pin number" then you need to switch the pin
into this function in order to use the pin as a GPIO line.
Pins that do not have this group associated with them
cannot be used for GPIO.

This is incidentally exactly the method used by the Qualcomm
pin control driver (IIRC).

If the SCMI protocol has not though about GPIO as a special
function, or mentioned anything about group name
conventions for the GPIO function, then there is a hole
in the specification, and this is likely best filled by creating
one-pin groups as per above and feed this back to the
spec.

If the GPIO usecase isn't even considered a function by SCMI,
or (more likely) "nobody thought about that" then this is
a good time to send it back to the drawing board for
specification, right? It's normal for specs to run into a bit
of friction when confronted with the real world.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

On Tue, Oct 24, 2023 at 1:10 PM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> As far as I understand, there is only one way for SCMI gpio driver
> to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
> command with "Input-mode" or "Output-mode" configuration type
> against *every* pin-controller's pins.
> (Here I assume that the command would fail with INVALID_PARAMETERS or
> NOT_SUPPORTED if configuring the given pin as a GPIO input or output
> is not possible. But the specification seems to be a bit ambiguous.)

As I also wrote in the reply to Christian, I expect the SCMI firmware
to consider GPIO a function on the pins, and either individual pins
(groups with just one pin in it) or entire groups of pins can be switched
to perform the "gpio function". ("gpio function" is akin to "i2c function"
or "HDMI function" etc.)

If the SCMI protocol considers GPIO usage to be something else
than a function of a pin, that is going to be a problem. Then the SCMI
protocol need some other way of determining that the pin is in
GPIO mode, and perhaps something would need to be added to
the protocol for that.

The reason is that in practice a lot of hardware has to decouple
the pin from any internal function in order to use it as GPIO, and
connect it to the GPIO block that can drive the line high and low.
And we don't select that as a function, how is the firmware going
to know that it needs to do this? Implicitly from the call requesting
the line to be output perhaps. But if the firmware can be altered
to do that, the firmware can just as well be altered to present
GPIO as a proper function.

Using a function makes most sense, because the board firmware
knows which pins are GPIO lines and what they are used for
(such as a LED or camera flash) and at boot certainly put them
into GPIO function and drive them high/low or set them as
input (high impedance).

> It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
> to call the command 200 times in order to get the answer.

I think we should only need to check which function each pin
has and those that are in the GPIO function we put into the ranges.

Yours,
Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro]

On Tue, Oct 24, 2023 at 03:12:51PM +0200, Linus Walleij wrote:
> On Tue, Oct 24, 2023 at 1:10???PM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > As far as I understand, there is only one way for SCMI gpio driver
> > to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
> > command with "Input-mode" or "Output-mode" configuration type
> > against *every* pin-controller's pins.
> > (Here I assume that the command would fail with INVALID_PARAMETERS or
> > NOT_SUPPORTED if configuring the given pin as a GPIO input or output
> > is not possible. But the specification seems to be a bit ambiguous.)
> 
> As I also wrote in the reply to Christian, I expect the SCMI firmware
> to consider GPIO a function on the pins, and either individual pins
> (groups with just one pin in it) or entire groups of pins can be switched
> to perform the "gpio function". ("gpio function" is akin to "i2c function"
> or "HDMI function" etc.)

First of all, there is no pre-defined naming convention either for
pins, groups or functions. SCMI firmware can give them any names.

Secondly, What you said in the above is already implemented in
my RFC patch. Please remember the example that I gave:

>     gpio-ranges = <&scmi_pinctrl 6 0 0>;
>     gpio-ranges-group-names = "pinmux_gpio";
> 
> means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
> gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
> range [20..24],
> 
>     baa-gpios = <&gpio0 7>;
> will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

Given the fact there is no naming convention, we need to explicitly
specify an associated group name in "gpio-ranges-group-names" in any way.

What my driver doesn't care for now is the case where a group of GPIO pins
are multiplexed with other functions (UART, I2C or whatever else).
In this case, we need to configure "pinconf" setup prior to using those
pins as GPIO anyway. Simply, it is out of scope of my driver.
(We can still use existing generic GPIO interfaces to operate them once
set up, though.)

After all, I still believe we need "gpio-ranges" property in most of
all use cases (The only exception is, as I mentioned, to unconditionally
map all pinctrl's pins to GPIO (if possible) when SCMI firmware provides
only GPIO function for all pins. I think it is a simple and yet likely
use case.

Thanks,
-Takahiro Akashi


> 
> If the SCMI protocol considers GPIO usage to be something else
> than a function of a pin, that is going to be a problem. Then the SCMI
> protocol need some other way of determining that the pin is in
> GPIO mode, and perhaps something would need to be added to
> the protocol for that.
> 
> The reason is that in practice a lot of hardware has to decouple
> the pin from any internal function in order to use it as GPIO, and
> connect it to the GPIO block that can drive the line high and low.
> And we don't select that as a function, how is the firmware going
> to know that it needs to do this? Implicitly from the call requesting
> the line to be output perhaps. But if the firmware can be altered
> to do that, the firmware can just as well be altered to present
> GPIO as a proper function.
> 
> Using a function makes most sense, because the board firmware
> knows which pins are GPIO lines and what they are used for
> (such as a LED or camera flash) and at boot certainly put them
> into GPIO function and drive them high/low or set them as
> input (high impedance).
> 
> > It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
> > to call the command 200 times in order to get the answer.
> 
> I think we should only need to check which function each pin
> has and those that are in the GPIO function we put into the ranges.
> 
> Yours,
> Linus Walleij

Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[Linus Walleij]

Hi Takahiro,

On Tue, Oct 24, 2023 at 3:43 PM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> First of all, there is no pre-defined naming convention either for
> pins, groups or functions. SCMI firmware can give them any names.

OK maybe that should be added to the spec?

[NB: I poked the pinctrl implementers in a separate mail, you
are on CC.]

Otherwise I think this is one of those cases where firmware
authors will simply start to use a certain naming convention if
the Linux driver requires it.

> Secondly, What you said in the above is already implemented in
> my RFC patch. Please remember the example that I gave:
>
> >     gpio-ranges = <&scmi_pinctrl 6 0 0>;
> >     gpio-ranges-group-names = "pinmux_gpio";
> >
> > means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
> > gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
> > range [20..24],
> >
> >     baa-gpios = <&gpio0 7>;
> > will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

Right! I am so unused to the gpio-ranges-group-names that
I didn't parse that properly :(

> After all, I still believe we need "gpio-ranges" property in most of
> all use cases (The only exception is, as I mentioned, to unconditionally
> map all pinctrl's pins to GPIO (if possible) when SCMI firmware provides
> only GPIO function for all pins. I think it is a simple and yet likely
> use case.

I suppose it is a bit of placement question.

The device tree GPIO ranges will have to duplicate more information
that the SCMI firmware already knows (what ranges are GPIOs, the
name of the GPIO mux function), that is my main concern.
And when we have information in two places that need to be matched,
invariably we get mismatches.

I'm trying to figure out what is the best way forward here but I think
we need some feedback from the pinctrl driver authors.

Yours,
Linus Walleij