[v1] pinctrl-scmi: Add GPIO support

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

Posted by Dan Carpenter 9 months, 1 week ago

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 <dan.carpenter@linaro.org>
---
 drivers/firmware/arm_scmi/pinctrl.c | 38 ++++++++++++++++++++++++++---
 include/linux/scmi_protocol.h       |  2 ++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index d18c2d248f04..f842bf7fe628 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -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;
 
@@ -679,6 +684,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
 	struct scmi_function_info *func;
+	bool gpio;
 	int ret;
 
 	if (selector >= pi->nr_functions)
@@ -689,7 +695,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);
+				      &gpio, &func->nr_groups);
 	if (ret)
 		return ret;
 
@@ -698,6 +704,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
 		return -ENODATA;
 	}
 
+	func->gpio = gpio;
 	func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
 				     GFP_KERNEL);
 	if (!func->groups)
@@ -763,6 +770,7 @@ static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
 {
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
 	struct scmi_pin_info *pin;
+	bool gpio;
 	int ret;
 
 	if (selector >= pi->nr_pins)
@@ -772,10 +780,12 @@ 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, &gpio,
+				      NULL);
 	if (ret)
 		return ret;
 
+	pin->gpio = gpio;
 	pin->present = true;
 	return 0;
 }
@@ -815,9 +825,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,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816..b4ad32067fc4 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -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);
-- 
2.47.2

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

Posted by Cristian Marussi 8 months, 2 weeks ago

On Mon, May 05, 2025 at 02:37:36PM +0300, Dan Carpenter wrote:
> 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,

> In SCMI only functions and pins have a GPIO flag so that's why groups are
> not handled here.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/firmware/arm_scmi/pinctrl.c | 38 ++++++++++++++++++++++++++---
>  include/linux/scmi_protocol.h       |  2 ++
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> index d18c2d248f04..f842bf7fe628 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -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;
>  
> @@ -679,6 +684,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
>  {
>  	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
>  	struct scmi_function_info *func;
> +	bool gpio;

...can we just avoid this local var and just...

>  	int ret;
>  
>  	if (selector >= pi->nr_functions)
> @@ -689,7 +695,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);
> +				      &gpio, &func->nr_groups);

				      &func->gpio, &func->nr_groups);

>  	if (ret)
>  		return ret;
>  
> @@ -698,6 +704,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
>  		return -ENODATA;
>  	}
>  
> +	func->gpio = gpio;

...and dropping this...

>  	func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
>  				     GFP_KERNEL);
>  	if (!func->groups)
> @@ -763,6 +770,7 @@ static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
>  {
>  	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
>  	struct scmi_pin_info *pin;
> +	bool gpio;

... same here...

>  	int ret;
>  
>  	if (selector >= pi->nr_pins)
> @@ -772,10 +780,12 @@ 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, &gpio,
> +				      NULL);

Ditto.

>  	if (ret)
>  		return ret;
>  
> +	pin->gpio = gpio;

Ditto.

>  	pin->present = true;
>  	return 0;
>  }
> @@ -815,9 +825,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)

being an is_something function could not make it return a bool ?

...but of course loosing the return error code in case of query is_gpio
on a group, and just maybe returning false in that case and a
dev_warn()... (but I have still not seen how this is used by your driver
as of this review...)

> +{
> +	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,
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 688466a0e816..b4ad32067fc4 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -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);o

Doxygen comment above too please...

>  	int (*group_pins_get)(const struct scmi_protocol_handle *ph,
>  			      u32 selector, const unsigned int **pins,
>  			      unsigned int *nr_pins);
> -- 
> 2.47.2
> 

Thanks,
Cristian

[RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions
[RFC 2/7] firmware: arm_scmi: add is_gpio() function
[RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config()
[RFC 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE
[RFC 5/7] pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support
[RFC 6/7] pinctrl-scmi: Add GPIO support
[RFC 7/7] pinctrl-scmi: remove unused struct member