Skip to contentSkip to conversation

pinctrl-scmi: Add GPIO support

RFC PATCH In review Dan Carpenter 4 participants 20 messages Jul 20, 2025 – Sep 5, 2025

Subsystems (3)
GPIO Pin control System control & power/management interface (SCPI/SCMI) Message Protocol drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

v2 cover pinctrl-scmi: Add GPIO support

This is version 2 of the RFC. The main reason I'm sending this is because there was a bug in the first version where it didn't calculate the offset correctly so pins and groups weren't linked correctly.

I've also fixed a few style issues that people pointed out.

These patches are basically as ready as they can be. The remaining thing is to figure out the format to describe it in the device tree. Actually that's probably already done, somewhat because people are already using SCMI pinctrl?

AKASHI Takahiro (1):
pinctrl: introduce pinctrl_gpio_get_config()

Dan Carpenter (6):
firmware: arm_scmi: move boiler plate code into the get info functions
firmware: arm_scmi: add is_gpio() function
pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE
pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support
pinctrl-scmi: Add GPIO support
pinctrl-scmi: remove unused struct member

 drivers/firmware/arm_scmi/pinctrl.c     | 142 +++++++++-------
 drivers/pinctrl/core.c                  |  31 ++++
 drivers/pinctrl/pinctrl-scmi.c          | 213 +++++++++++++++++++++++-
 include/linux/pinctrl/consumer.h        |   9 +
 include/linux/pinctrl/pinconf-generic.h |   3 +
 include/linux/scmi_protocol.h           |   2 +
 6 files changed, 331 insertions(+), 69 deletions(-)
Signature · 1 line
2.47.2
author
#1 lore raw

RFC PATCH v2 1/7 firmware: arm_scmi: move boiler plate code into the get info functions

This code to check whether the selector is valid and if the item has already been recorded in the array can be moved to the scmi_pinctrl_get_function_info() type functions. That way it's in one place instead of duplicated in each of the callers.

I removed the check for if "pi->nr_groups == 0" because if that were the case then "selector >= pi->nr_groups" would already be true. It already was not checked for pins so this makes things a bit more uniform.

I also removed the check for if (!pin) since pin is an offset into the middle of an array and can't be NULL.

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/firmware/arm_scmi/pinctrl.c | 108 +++++++++++++---------------
 1 file changed, 48 insertions(+), 60 deletions(-)
drivers/firmware/arm_scmi/pinctrl.c +48 -60
index 3855c98caf06..d18c2d248f04 100644
@@ -596,11 +596,19 @@ static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
 }
 
 static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
-				       u32 selector,
-				       struct scmi_group_info *group)
+				       u32 selector)
 {
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	struct scmi_group_info *group;
 	int ret;
 
+	if (selector >= pi->nr_groups)
+		return -EINVAL;
+
+	group = &pi->groups[selector];
+	if (group->present)
+		return 0;
+
 	ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, group->name,
 				      &group->nr_pins);
 	if (ret)

@@ -632,21 +640,14 @@ static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
 				       u32 selector, const char **name)
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
 
 	if (!name)
 		return -EINVAL;
 
-	if (selector >= pi->nr_groups || pi->nr_groups == 0)
-		return -EINVAL;
-
-	if (!pi->groups[selector].present) {
-		int ret;
-
-		ret = scmi_pinctrl_get_group_info(ph, selector,
-						  &pi->groups[selector]);
-		if (ret)
-			return ret;
-	}
+	ret = scmi_pinctrl_get_group_info(ph, selector);
+	if (ret)
+		return ret;
 
 	*name = pi->groups[selector].name;
 

@@ -658,21 +659,14 @@ static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
 				       u32 *nr_pins)
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
 
 	if (!pins || !nr_pins)
 		return -EINVAL;
 
-	if (selector >= pi->nr_groups || pi->nr_groups == 0)
-		return -EINVAL;
-
-	if (!pi->groups[selector].present) {
-		int ret;
-
-		ret = scmi_pinctrl_get_group_info(ph, selector,
-						  &pi->groups[selector]);
-		if (ret)
-			return ret;
-	}
+	ret = scmi_pinctrl_get_group_info(ph, selector);
+	if (ret)
+		return ret;
 
 	*pins = pi->groups[selector].group_pins;
 	*nr_pins = pi->groups[selector].nr_pins;

@@ -681,11 +675,19 @@ static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
-					  u32 selector,
-					  struct scmi_function_info *func)
+					  u32 selector)
 {
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	struct scmi_function_info *func;
 	int ret;
 
+	if (selector >= pi->nr_functions)
+		return -EINVAL;
+
+	func = &pi->functions[selector];
+	if (func->present)
+		return 0;
+
 	ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, func->name,
 				      &func->nr_groups);
 	if (ret)

@@ -716,21 +718,14 @@ static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
 					  u32 selector, const char **name)
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
 
 	if (!name)
 		return -EINVAL;
 
-	if (selector >= pi->nr_functions || pi->nr_functions == 0)
-		return -EINVAL;
-
-	if (!pi->functions[selector].present) {
-		int ret;
-
-		ret = scmi_pinctrl_get_function_info(ph, selector,
-						     &pi->functions[selector]);
-		if (ret)
-			return ret;
-	}
+	ret = scmi_pinctrl_get_function_info(ph, selector);
+	if (ret)
+		return ret;
 
 	*name = pi->functions[selector].name;
 	return 0;

@@ -742,21 +737,14 @@ scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
 				 const u32 **groups)
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
 
 	if (!groups || !nr_groups)
 		return -EINVAL;
 
-	if (selector >= pi->nr_functions || pi->nr_functions == 0)
-		return -EINVAL;
-
-	if (!pi->functions[selector].present) {
-		int ret;
-
-		ret = scmi_pinctrl_get_function_info(ph, selector,
-						     &pi->functions[selector]);
-		if (ret)
-			return ret;
-	}
+	ret = scmi_pinctrl_get_function_info(ph, selector);
+	if (ret)
+		return ret;
 
 	*groups = pi->functions[selector].groups;
 	*nr_groups = pi->functions[selector].nr_groups;

@@ -771,13 +759,19 @@ static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
-				     u32 selector, struct scmi_pin_info *pin)
+				     u32 selector)
 {
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	struct scmi_pin_info *pin;
 	int ret;
 
-	if (!pin)
+	if (selector >= pi->nr_pins)
 		return -EINVAL;
 
+	pin = &pi->pins[selector];
+	if (pin->present)
+		return 0;
+
 	ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, NULL);
 	if (ret)
 		return ret;

@@ -790,20 +784,14 @@ static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
 				     u32 selector, const char **name)
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
 
 	if (!name)
 		return -EINVAL;
 
-	if (selector >= pi->nr_pins)
-		return -EINVAL;
-
-	if (!pi->pins[selector].present) {
-		int ret;
-
-		ret = scmi_pinctrl_get_pin_info(ph, selector, &pi->pins[selector]);
-		if (ret)
-			return ret;
-	}
+	ret = scmi_pinctrl_get_pin_info(ph, selector);
+	if (ret)
+		return ret;
 
 	*name = pi->pins[selector].name;
Signature · 1 line
2.47.2
reviewer
#2 lore raw

Re: [PATCH RFC v2 1/7] firmware: arm_scmi: move boiler plate code into the get info functions

Quoted 5 lines · Dan Carpenter

This code to check whether the selector is valid and if the item has already been recorded in the array can be moved to the scmi_pinctrl_get_function_info() type functions. That way it's in one place instead of duplicated in each of the callers.

Hi,

Quoted 9 lines

I removed the check for if "pi->nr_groups == 0" because if that were the case then "selector >= pi->nr_groups" would already be true. It already was not checked for pins so this makes things a bit more uniform.

I also removed the check for if (!pin) since pin is an offset into the middle of an array and can't be NULL.

Signed-off-by Dan Carpenter <[email protected]>

---

...has anything change in this commit from V1 ?

Asking because there is no version-diff and you did not pick up my Reviewed-by tag from V1, so I am wondering if there is anything to review or not..

Thanks,
Cristian

author
#3 lore raw

RFC PATCH v2 2/7 firmware: arm_scmi: add is_gpio() function

In reply to Dan Carpenter

Parse the GPIO response in scmi_pinctrl_attributes(), set the gpio flag, and create an is_gpio() function pointer so that it can be queried.

In SCMI only functions and pins have a GPIO flag so that's why groups are not handled here.

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/firmware/arm_scmi/pinctrl.c | 34 +++++++++++++++++++++++++----
 include/linux/scmi_protocol.h       |  2 ++
 2 files changed, 32 insertions(+), 4 deletions(-)
drivers/firmware/arm_scmi/pinctrl.c +30 -4
index d18c2d248f04..10c92007bc1b 100644
@@ -28,6 +28,7 @@
 
 #define EXT_NAME_FLAG(x)	le32_get_bits((x), BIT(31))
 #define NUM_ELEMS(x)		le32_get_bits((x), GENMASK(15, 0))
+#define GPIO_FUNC(x)		le32_get_bits((x), BIT(17))
 
 #define REMAINING(x)		le32_get_bits((x), GENMASK(31, 16))
 #define RETURNED(x)		le32_get_bits((x), GENMASK(11, 0))

@@ -107,6 +108,7 @@ struct scmi_group_info {
 struct scmi_function_info {
 	char name[SCMI_MAX_STR_SIZE];
 	bool present;
+	bool gpio;
 	u32 *groups;
 	u32 nr_groups;
 };

@@ -114,6 +116,7 @@ struct scmi_function_info {
 struct scmi_pin_info {
 	char name[SCMI_MAX_STR_SIZE];
 	bool present;
+	bool gpio;
 };
 
 struct scmi_pinctrl_info {

@@ -189,7 +192,7 @@ static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
 
 static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
 				   enum scmi_pinctrl_selector_type type,
-				   u32 selector, char *name,
+				   u32 selector, char *name, bool *gpio,
 				   u32 *n_elems)
 {
 	int ret;

@@ -217,6 +220,8 @@ static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
 
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
+		if (gpio)
+			*gpio = GPIO_FUNC(rx->attributes);
 		if (n_elems)
 			*n_elems = NUM_ELEMS(rx->attributes);
 

@@ -610,7 +615,7 @@ static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
 		return 0;
 
 	ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, group->name,
-				      &group->nr_pins);
+				      NULL, &group->nr_pins);
 	if (ret)
 		return ret;
 

@@ -689,7 +694,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
 		return 0;
 
 	ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, func->name,
-				      &func->nr_groups);
+				      &func->gpio, &func->nr_groups);
 	if (ret)
 		return ret;
 

@@ -772,7 +777,8 @@ static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
 	if (pin->present)
 		return 0;
 
-	ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, NULL);
+	ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name,
+				      &pin->gpio, NULL);
 	if (ret)
 		return ret;
 

@@ -815,9 +821,29 @@ static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
 	}
 }
 
+static int scmi_pinctrl_is_gpio(const struct scmi_protocol_handle *ph,
+				u32 selector,
+				enum scmi_pinctrl_selector_type type)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+	int ret;
+
+	ret = scmi_pinctrl_get_pin_info(ph, selector);
+	if (ret)
+		return ret;
+
+	if (type == PIN_TYPE)
+		return pi->pins[selector].gpio;
+	if (type == FUNCTION_TYPE)
+		return pi->functions[selector].gpio;
+
+	return -EINVAL;
+}
+
 static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
 	.count_get = scmi_pinctrl_count_get,
 	.name_get = scmi_pinctrl_name_get,
+	.is_gpio = scmi_pinctrl_is_gpio,
 	.group_pins_get = scmi_pinctrl_group_pins_get,
 	.function_groups_get = scmi_pinctrl_function_groups_get,
 	.mux_set = scmi_pinctrl_mux_set,
include/linux/scmi_protocol.h +2 -0
index 688466a0e816..b4ad32067fc4 100644
@@ -792,6 +792,8 @@ struct scmi_pinctrl_proto_ops {
 	int (*name_get)(const struct scmi_protocol_handle *ph, u32 selector,
 			enum scmi_pinctrl_selector_type type,
 			const char **name);
+	int (*is_gpio)(const struct scmi_protocol_handle *ph, u32 selector,
+		       enum scmi_pinctrl_selector_type type);
 	int (*group_pins_get)(const struct scmi_protocol_handle *ph,
 			      u32 selector, const unsigned int **pins,
 			      unsigned int *nr_pins);
Signature · 1 line
2.47.2
reviewer Reviewed-by on v2 2/7
#4 lore raw

Re: [PATCH RFC v2 2/7] firmware: arm_scmi: add is_gpio() function

Quoted 3 lines · Dan Carpenter

Parse the GPIO response in scmi_pinctrl_attributes(), set the gpio flag, and create an is_gpio() function pointer so that it can be queried.

Hi,

Quoted 4 lines

In SCMI only functions and pins have a GPIO flag so that's why groups are not handled here.

Signed-off-by Dan Carpenter <[email protected]>

[snip]

Quoted 12 lines

static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {

drivers/firmware/arm_scmi/pinctrl.c +1 -0
 	.count_get = scmi_pinctrl_count_get,
 	.name_get = scmi_pinctrl_name_get,
+	.is_gpio = scmi_pinctrl_is_gpio,
 	.group_pins_get = scmi_pinctrl_group_pins_get,
 	.function_groups_get = scmi_pinctrl_function_groups_get,
 	.mux_set = scmi_pinctrl_mux_set,
include/linux/scmi_protocol.h +0 -0
index 688466a0e816..b4ad32067fc4 100644

include/linux/scmi_protocol.h struct scmi_pinctrl_proto_ops { · line 796

Quoted 5 lines
include/linux/scmi_protocol.h +2 -0
@@ -792,6 +792,8 @@ struct scmi_pinctrl_proto_ops {
 	int (*name_get)(const struct scmi_protocol_handle *ph, u32 selector,
 			enum scmi_pinctrl_selector_type type,
 			const char **name);
+	int (*is_gpio)(const struct scmi_protocol_handle *ph, u32 selector,
+		       enum scmi_pinctrl_selector_type type);

Doxygen comment above too please...

With that fixed, LGTM.

Reviewed-by Cristian Marussi <[email protected]>

Thanks,
Cristian

author
#5 lore raw

RFC PATCH v2 3/7 pinctrl: introduce pinctrl_gpio_get_config()

In reply to Dan Carpenter

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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/pinctrl/core.c           | 35 ++++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h |  9 ++++++++
 2 files changed, 44 insertions(+)
drivers/pinctrl/core.c +35 -0
index 73b78d6eac67..bbcc6881b119 100644
@@ -30,6 +30,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/pinctrl/devinfo.h>
 #include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 
 #include "core.h"

@@ -937,6 +938,40 @@ int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 }
 EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
 
+/**
+ * pinctrl_gpio_get_config() - Get the config for a given GPIO pin
+ * @gc: GPIO chip structure from the GPIO subsystem
+ * @offset: hardware offset of the GPIO relative to the controller
+ * @config: the configuration to query.  On success it holds the result
+ */
+int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
+{
+	struct pinctrl_gpio_range *range;
+	const struct pinconf_ops *ops;
+	struct pinctrl_dev *pctldev;
+	int ret, pin;
+
+	ret = pinctrl_get_device_gpio_range(gc, offset, &pctldev, &range);
+	if (ret)
+		return ret;
+
+	ops = pctldev->desc->confops;
+	if (!ops || !ops->pin_config_get)
+		return -EINVAL;
+
+	mutex_lock(&pctldev->mutex);
+	pin = gpio_to_pin(range, gc, offset);
+	ret = ops->pin_config_get(pctldev, pin, config);
+	mutex_unlock(&pctldev->mutex);
+
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_argument(*config);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
+
 static struct pinctrl_state *find_state(struct pinctrl *p,
 					const char *name)
 {
include/linux/pinctrl/consumer.h +9 -0
index 73de70362b98..e5815b3382dc 100644
@@ -35,6 +35,8 @@ int pinctrl_gpio_direction_output(struct gpio_chip *gc,
 				  unsigned int offset);
 int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 				unsigned long config);
+int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset,
+			    unsigned long *config);
 
 struct pinctrl * __must_check pinctrl_get(struct device *dev);
 void pinctrl_put(struct pinctrl *p);

@@ -96,6 +98,13 @@ pinctrl_gpio_direction_output(struct gpio_chip *gc, unsigned int offset)
 	return 0;
 }
 
+static inline int
+pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset,
+			unsigned long *config)
+{
+	return 0;
+}
+
 static inline int
 pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config)
Signature · 1 line
2.47.2
author
#6 lore raw

RFC PATCH v2 4/7 pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

In reply to Dan Carpenter

In SCMI the value of the pin is just another configuration option. Add this as an option in the pin_config_param enum and creating a mapping to SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()

Since this is an RFC patch, I'm going to comment that I think the SCMI pinctrl driver misuses the PIN_CONFIG_OUTPUT enum. It should be for enabling and disabling output on pins which can serve as both input and output. Enabling it is supposed to write a 1 and disabling it is supposed to write a 0 but we use that side effect to write 1s and 0s. I did't change this because it would break userspace but I'd like to add a PIN_CONFIG_OUTPUT_VALUE enum as well and use that in the GPIO driver. But in this patchset I just use PIN_CONFIG_OUTPUT.

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/pinctrl/pinctrl-scmi.c          | 3 +++
 include/linux/pinctrl/pinconf-generic.h | 3 +++
 2 files changed, 6 insertions(+)
drivers/pinctrl/pinctrl-scmi.c +3 -0
index 383681041e4c..d1f2f971cd96 100644
@@ -250,6 +250,9 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
 		*type = SCMI_PIN_INPUT_MODE;
 		break;
+	case PIN_CONFIG_INPUT_VALUE:
+		*type = SCMI_PIN_INPUT_VALUE;
+		break;
 	case PIN_CONFIG_MODE_LOW_POWER:
 		*type = SCMI_PIN_LOW_POWER_MODE;
 		break;
include/linux/pinctrl/pinconf-generic.h +3 -0
index 1bcf071b860e..b37838171581 100644
@@ -83,6 +83,8 @@ struct pinctrl_map;
  *      schmitt-trigger mode is disabled.
  * @PIN_CONFIG_INPUT_SCHMITT_UV: this will configure an input pin to run in
  *	schmitt-trigger mode. The argument is in uV.
+ * @PIN_CONFIG_INPUT_VALUE: This is used in SCMI to read the value from the
+ *	pin.
  * @PIN_CONFIG_MODE_LOW_POWER: this will configure the pin for low power
  *	operation, if several modes of operation are supported these can be
  *	passed in the argument on a custom form, else just use argument 1

@@ -135,6 +137,7 @@ enum pin_config_param {
 	PIN_CONFIG_INPUT_SCHMITT,
 	PIN_CONFIG_INPUT_SCHMITT_ENABLE,
 	PIN_CONFIG_INPUT_SCHMITT_UV,
+	PIN_CONFIG_INPUT_VALUE,
 	PIN_CONFIG_MODE_LOW_POWER,
 	PIN_CONFIG_MODE_PWM,
 	PIN_CONFIG_OUTPUT,
Signature · 1 line
2.47.2
reviewer
#7 lore raw

Re: [PATCH RFC v2 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

Quoted 3 lines · Dan Carpenter

In SCMI the value of the pin is just another configuration option. Add this as an option in the pin_config_param enum and creating a mapping to SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()

If this is the case, should not this commit have also a Fixes tag ?

Cheers,
Cristian

#8 lore raw

Re: [PATCH RFC v2 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

In reply to Dan Carpenter
Quoted 13 lines · Dan Carpenter

In SCMI the value of the pin is just another configuration option. Add this as an option in the pin_config_param enum and creating a mapping to SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()

Since this is an RFC patch, I'm going to comment that I think the SCMI pinctrl driver misuses the PIN_CONFIG_OUTPUT enum. It should be for enabling and disabling output on pins which can serve as both input and output. Enabling it is supposed to write a 1 and disabling it is supposed to write a 0 but we use that side effect to write 1s and 0s. I did't change this because it would break userspace but I'd like to add a PIN_CONFIG_OUTPUT_VALUE enum as well and use that in the GPIO driver. But in this patchset I just use PIN_CONFIG_OUTPUT.

Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

I tweaked this patch around a bit and applied: removed the second comment in the commit message and wrote the docs to be more generic since in the future other things than SCMI might want to use this config option.

Yours,
Linus Walleij

#9 lore raw

Re: [PATCH RFC v2 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

Quoted 15 lines · Linus Walleij
Quoted 13 lines · Dan Carpenter

In SCMI the value of the pin is just another configuration option. Add this as an option in the pin_config_param enum and creating a mapping to SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()

Since this is an RFC patch, I'm going to comment that I think the SCMI pinctrl driver misuses the PIN_CONFIG_OUTPUT enum. It should be for enabling and disabling output on pins which can serve as both input and output. Enabling it is supposed to write a 1 and disabling it is supposed to write a 0 but we use that side effect to write 1s and 0s. I did't change this because it would break userspace but I'd like to add a PIN_CONFIG_OUTPUT_VALUE enum as well and use that in the GPIO driver. But in this patchset I just use PIN_CONFIG_OUTPUT.

Quoted 6 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

I tweaked this patch around a bit and applied: removed the second comment in the commit message and wrote the docs to be more generic since in the future other things than SCMI might want to use this config option.

Then I thought about it some more. ...

Isn't it more intuitive that we rewrite the curren PIN_CONFIG_OUTPUT_VALUE to just PIN_CONFIG_VALUE that can be used for both reading and writing binary low/high instead of having two different things like this?

I will look over current users and maybe propose a patch.

Yours,
Linus Walleij

#10 lore raw

Re: [PATCH RFC v2 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

Quoted 23 lines · Linus Walleij
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Dan Carpenter

In SCMI the value of the pin is just another configuration option. Add this as an option in the pin_config_param enum and creating a mapping to SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()

Since this is an RFC patch, I'm going to comment that I think the SCMI pinctrl driver misuses the PIN_CONFIG_OUTPUT enum. It should be for enabling and disabling output on pins which can serve as both input and output. Enabling it is supposed to write a 1 and disabling it is supposed to write a 0 but we use that side effect to write 1s and 0s. I did't change this because it would break userspace but I'd like to add a PIN_CONFIG_OUTPUT_VALUE enum as well and use that in the GPIO driver. But in this patchset I just use PIN_CONFIG_OUTPUT.

Quoted 6 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

I tweaked this patch around a bit and applied: removed the second comment in the commit message and wrote the docs to be more generic since in the future other things than SCMI might want to use this config option.

Quoted 7 lines

Then I thought about it some more. ...

Isn't it more intuitive that we rewrite the curren PIN_CONFIG_OUTPUT_VALUE to just PIN_CONFIG_VALUE that can be used for both reading and writing binary low/high instead of having two different things like this?

I will look over current users and maybe propose a patch.

I discovered that several in-tree drivers are already *reading* the property PIN_CONFIG_OUTPUT_VALUE to get the logic level of the line.

I sent a patch renaming this property to PIN_CONFIG_LEVEL so it is clear that this can also be read, and you can drop this patch and just read/write PIN_CONFIG_LEVEL instead for the GPIO driver.

Yours,
Linus Walleij

author
#11 lore raw

RFC PATCH v2 5/7 pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support

In reply to Dan Carpenter

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

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

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/pinctrl/pinctrl-scmi.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/pinctrl/pinctrl-scmi.c +0 -3
index d1f2f971cd96..fba0a3a2fc10 100644
@@ -262,9 +262,6 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	case PIN_CONFIG_OUTPUT_ENABLE:
 		*type = SCMI_PIN_OUTPUT_MODE;
 		break;
-	case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
-		*type = SCMI_PIN_OUTPUT_VALUE;
-		break;
 	case PIN_CONFIG_POWER_SOURCE:
 		*type = SCMI_PIN_POWER_SOURCE;
 		break;
Signature · 1 line
2.47.2
reviewer
#12 lore raw

Re: [PATCH RFC v2 5/7] pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support

Quoted 7 lines · Dan Carpenter

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

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

Wouldn't this risk to break any deployed system which already happen to misuse this ? I am thinking especially of the guys who have actively developed this driver and the related SCMI server, like EPAM and NXP ?

+CC Peng@NXP

Thanks,
Cristian

author
#13 lore raw

RFC PATCH v2 6/7 pinctrl-scmi: Add GPIO support

In reply to Dan Carpenter

This adds GPIO support to the SCMI pin controller driver. It's an RFC patch because I'm not really sure how these are used and so I don't know how they should be configured via devicetree. I've labeled the places where I think devicetree configuration would go with a FIXME.

This driver was based on work from Takahiro AKASHI.

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/pinctrl/core.c         |   8 +-
 drivers/pinctrl/pinctrl-scmi.c | 206 ++++++++++++++++++++++++++++++++-
 2 files changed, 207 insertions(+), 7 deletions(-)
drivers/pinctrl/core.c +2 -6
index bbcc6881b119..91882c68bcd5 100644
@@ -947,7 +947,6 @@ EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
 int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
 {
 	struct pinctrl_gpio_range *range;
-	const struct pinconf_ops *ops;
 	struct pinctrl_dev *pctldev;
 	int ret, pin;
 

@@ -955,19 +954,16 @@ int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned
 	if (ret)
 		return ret;
 
-	ops = pctldev->desc->confops;
-	if (!ops || !ops->pin_config_get)
-		return -EINVAL;
-
 	mutex_lock(&pctldev->mutex);
 	pin = gpio_to_pin(range, gc, offset);
-	ret = ops->pin_config_get(pctldev, pin, config);
+	ret = pin_config_get_for_pin(pctldev, pin, config);
 	mutex_unlock(&pctldev->mutex);
 
 	if (ret)
 		return ret;
 
 	*config = pinconf_to_config_argument(*config);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
drivers/pinctrl/pinctrl-scmi.c +205 -1

236 lines not on this page · raw message

Signature · 1 line
2.47.2
build bot
#14 lore raw
kernel test robot noticed the following build errors:

Hi Dan,

[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:

[auto build test ERROR on linusw-pinctrl/devel]
[also build test ERROR on linusw-pinctrl/for-next linus/master v6.16-rc7 next-20250718] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/firmware-arm_scmi-move-boiler-plate-code-into-the-get-info-functions/20250721-034113
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/bd79aa90-73f7-4d7b-a340-539b52a73bcf%40sabinyo.mountain
patch subject: [PATCH RFC v2 6/7] pinctrl-scmi: Add GPIO support
config: s390-randconfig-002-20250721 (https://download.01.org/0day-ci/archive/20250721/[email protected]/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250721/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

>> drivers/pinctrl/core.c:959:8: error: call to undeclared function 'pin_config_get_for_pin'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     959 |         ret = pin_config_get_for_pin(pctldev, pin, config);
         |               ^

1 error generated.

vim +/pin_config_get_for_pin +959 drivers/pinctrl/core.c

940
941 /**
942 * pinctrl_gpio_get_config() - Get the config for a given GPIO pin
943 * @gc: GPIO chip structure from the GPIO subsystem
944 * @offset: hardware offset of the GPIO relative to the controller
945 * @config: the configuration to query. On success it holds the result
946 */
947 int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
948 {
949 struct pinctrl_gpio_range *range;
950 struct pinctrl_dev *pctldev;
951 int ret, pin;
952
953 ret = pinctrl_get_device_gpio_range(gc, offset, &pctldev, &range);
954 if (ret)
955 return ret;
956
957 mutex_lock(&pctldev->mutex);
958 pin = gpio_to_pin(range, gc, offset);

 > 959		ret = pin_config_get_for_pin(pctldev, pin, config);

960 mutex_unlock(&pctldev->mutex);
961
962 if (ret)
963 return ret;
964
965 *config = pinconf_to_config_argument(*config);
966
967 return 0;
968 }
969 EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
970

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#15 lore raw
In reply to Dan Carpenter
Quoted 8 lines · Dan Carpenter

This adds GPIO support to the SCMI pin controller driver. It's an RFC patch because I'm not really sure how these are used and so I don't know how they should be configured via devicetree. I've labeled the places where I think devicetree configuration would go with a FIXME.

This driver was based on work from Takahiro AKASHI.

Signed-off-by Dan Carpenter <[email protected]>

(...)

Quoted 11 lines
drivers/pinctrl/core.c +0 -5
@@ -955,19 +954,16 @@ int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned
        if (ret)
                return ret;
 
-       ops = pctldev->desc->confops;
-       if (!ops || !ops->pin_config_get)
-               return -EINVAL;
-
        mutex_lock(&pctldev->mutex);
        pin = gpio_to_pin(range, gc, offset);
-       ret = ops->pin_config_get(pctldev, pin, config);

drivers/pinctrl/core.c int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned · line 968

Quoted 10 lines
drivers/pinctrl/core.c +2 -0
+       ret = pin_config_get_for_pin(pctldev, pin, config);
        mutex_unlock(&pctldev->mutex);
 
        if (ret)
                return ret;
 
        *config = pinconf_to_config_argument(*config);
+
        return 0;
 }

Looks reasonable.

drivers/pinctrl/pinctrl-scmi.c struct scmi_pinctrl { · line 49

Quoted 5 lines
drivers/pinctrl/pinctrl-scmi.c +1 -0
@@ -42,6 +46,7 @@ struct scmi_pinctrl {
        unsigned int nr_functions;
        struct pinctrl_pin_desc *pins;
        unsigned int nr_pins;
+       struct gpio_chip *gc;

This being a pointer is slightly confusing. And looking below I see why:

drivers/pinctrl/pinctrl-scmi.c static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, · line 642

Quoted 7 lines
drivers/pinctrl/pinctrl-scmi.c +7 -0
+       gpio = fwnode_get_named_child_node(dev->fwnode, "gpio");
+       if (!gpio)
+               return 0;
+
+       pmx->gc = devm_kcalloc(dev, pmx->nr_functions, sizeof(*pmx->gc), GFP_KERNEL);
+       if (!pmx->gc)
+               return -ENOMEM;

So this needs a comment to what is actually going on here.

To me it looks like the code is instantiating a struct gpio_chip for each function on the pin mux.

That feels wrong: instead we should probably instantiate *one* gpio_chip for the whole pinmux and then create individual gpio lines for each pin that can work as a GPIO.

drivers/pinctrl/pinctrl-scmi.c static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, · line 649

Quoted 6 lines
drivers/pinctrl/pinctrl-scmi.c +6 -0
+       for (i = 0; i < pmx->nr_functions; i++) {
+               const char *fn_name;
+
+               ret = pinctrl_ops->is_gpio(pmx->ph, i, FUNCTION_TYPE);
+               if (ret < 0)
+                       return ret;

So we are only looking for functions that are of GPIO type.

Quoted 16 lines
drivers/pinctrl/pinctrl-scmi.c +16 -0
+               if (ret == false)
+                       continue;
+
+               ret = pinctrl_ops->name_get(pmx->ph, i, FUNCTION_TYPE, &fn_name);
+               if (ret)
+                       return ret;
+
+               pmx->gc[i].label = devm_kasprintf(dev, GFP_KERNEL, "%s", fn_name);
+               if (!pmx->gc[i].label)
+                       return -ENOMEM;
+
+               pmx->gc[i].owner = THIS_MODULE;
+               pmx->gc[i].get = pinctrl_gpio_get;
+               pmx->gc[i].set = pinctrl_gpio_set;
+               pmx->gc[i].get_direction = pinctrl_gpio_get_direction;
+               pmx->gc[i].direction_input = pinctrl_gpio_direction_input;

drivers/pinctrl/pinctrl-scmi.c static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, · line 675

Quoted 10 lines
drivers/pinctrl/pinctrl-scmi.c +10 -0
+               pmx->gc[i].direction_output = pinctrl_gpio_direction_output_wrapper;
+               pmx->gc[i].add_pin_ranges = gpio_add_pin_ranges;
+
+               // FIXME: verify that this is correct
+               pmx->gc[i].can_sleep = true;
+
+               ret = get_nr_pins_in_function(pmx, i);
+               if (ret < 0)
+                       return ret;
+               pmx->gc[i].ngpio = ret;

Please put a print here and see how many pins there are usually in the function here. In my mind it should always be 1 and if it is not 1 then something is wrong.

We cannot instantiate n gpio_chips for a pin controller where n is the number of functions for GPIO, intead we need to instantiate one gpio_chip for the whole pin controller with ngpio set to the number of pins that exist on the pin controller and mask of any lines that cannot be used as GPIO from the chip using the gc->init_valid_mask() callback.

I think you want to look at Bartosz series that introduce a GPIO function category and use this as a basis for your work, because I plan to merge this for v6.18:
https://lore.kernel.org/linux-gpio/[email protected]/T/#t

If you can first patch the SCMI pin control driver to use pinmux_generic_add_pinfunction() based on Bartosz patch set and get to this:

+ .get_functions_count = pinmux_generic_get_function_count,
+ .get_function_name = pinmux_generic_get_function_name,
+ .get_function_groups = pinmux_generic_get_function_groups,

Then it is also easier to figure out if a line is GPIO or not (the core will help you).

I hope the conversion to Bartosz generics could be straightforward... It looks straightforward but there may be devil in the details :)

After this you can use pinmux_generic_function_is_gpio() to check if a certain function is used for GPIO or not, I think in the SCMI case this is done with a simple strcmp().

Then you can use this to instantiate a gpio_chip for SCMI that will dynamically allow pins to be remuxed into GPIO and made available on the chip as we go.

I'm sure Bartosz can help out a bit with some details!

Yours,
Linus Walleij

author
#16 lore raw

RFC PATCH v2 7/7 pinctrl-scmi: remove unused struct member

In reply to Dan Carpenter

The ->nr_pins is not used so delete that.

Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/pinctrl/pinctrl-scmi.c | 1 -
 1 file changed, 1 deletion(-)
drivers/pinctrl/pinctrl-scmi.c +0 -1
index 9a947ced0df7..372fa67589fb 100644
@@ -45,7 +45,6 @@ struct scmi_pinctrl {
 	struct pinfunction *functions;
 	unsigned int nr_functions;
 	struct pinctrl_pin_desc *pins;
-	unsigned int nr_pins;
 	struct gpio_chip *gc;
 };
Signature · 1 line
2.47.2
#18 lore raw
In reply to Dan Carpenter
Quoted 3 lines · Dan Carpenter

This is version 2 of the RFC. The main reason I'm sending this is because there was a bug in the first version where it didn't calculate the offset correctly so pins and groups weren't linked correctly.

I'm thinking of applying patches 4, 5 and 7 of this patch set to get some movement in the code upstream and make less work for you to rebase the thing, would this be OK?

Yours,
Linus Walleij

author
#19 lore raw
Quoted 9 lines · Linus Walleij
Quoted 3 lines · Dan Carpenter

This is version 2 of the RFC. The main reason I'm sending this is because there was a bug in the first version where it didn't calculate the offset correctly so pins and groups weren't linked correctly.

I'm thinking of applying patches 4, 5 and 7 of this patch set to get some movement in the code upstream and make less work for you to rebase the thing, would this be OK?

I think we need to hold of on 5. I think patch 5 is the right thing, but I need to make sure that it doesn't break anything. I was hoping people who care about it would let me know.

Applying patch 1 is pretty easy as well. We could do that too.

Sorry for the delayed response.

regards,
dan carpenter