* [RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
@ 2025-05-05 11:37 ` Dan Carpenter
2025-05-30 12:48 ` Cristian Marussi
2025-05-05 11:37 ` [RFC 2/7] firmware: arm_scmi: add is_gpio() function Dan Carpenter
` (6 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:37 UTC (permalink / raw)
To: Sudeep Holla; +Cc: Cristian Marussi, arm-scmi, linux-arm-kernel, linux-kernel
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 <dan.carpenter@linaro.org>
---
drivers/firmware/arm_scmi/pinctrl.c | 108 +++++++++++++---------------
1 file changed, 48 insertions(+), 60 deletions(-)
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 3855c98caf06..d18c2d248f04 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -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;
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions
2025-05-05 11:37 ` [RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions Dan Carpenter
@ 2025-05-30 12:48 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2025-05-30 12:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel,
linux-kernel
On Mon, May 05, 2025 at 02:37:22PM +0300, Dan Carpenter wrote:
> 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 Dan,
thanks for this.
>
> 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.
>
A much needed and appreciated cleanup. LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Thanks,
Cristian
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC 2/7] firmware: arm_scmi: add is_gpio() function
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
2025-05-05 11:37 ` [RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions Dan Carpenter
@ 2025-05-05 11:37 ` Dan Carpenter
2025-05-30 13:09 ` Cristian Marussi
2025-05-05 11:37 ` [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config() Dan Carpenter
` (5 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:37 UTC (permalink / raw)
To: Sudeep Holla; +Cc: Cristian Marussi, arm-scmi, linux-arm-kernel, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC 2/7] firmware: arm_scmi: add is_gpio() function
2025-05-05 11:37 ` [RFC 2/7] firmware: arm_scmi: add is_gpio() function Dan Carpenter
@ 2025-05-30 13:09 ` Cristian Marussi
0 siblings, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2025-05-30 13:09 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config()
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
2025-05-05 11:37 ` [RFC 1/7] firmware: arm_scmi: move boiler plate code into the get info functions Dan Carpenter
2025-05-05 11:37 ` [RFC 2/7] firmware: arm_scmi: add is_gpio() function Dan Carpenter
@ 2025-05-05 11:37 ` Dan Carpenter
2025-05-05 16:30 ` ALOK TIWARI
2025-05-05 11:38 ` [RFC 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE Dan Carpenter
` (4 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:37 UTC (permalink / raw)
To: Linus Walleij
Cc: Bartosz Golaszewski, linux-gpio, linux-kernel, Takahiro AKASHI
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
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 <takahiro.akashi@linaro.org>
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/pinctrl/core.c | 35 ++++++++++++++++++++++++++++++++
include/linux/pinctrl/consumer.h | 9 ++++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4bdbf6bb26e2..4310f9e2118b 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -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)
{
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 73de70362b98..e5815b3382dc 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -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)
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config()
2025-05-05 11:37 ` [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config() Dan Carpenter
@ 2025-05-05 16:30 ` ALOK TIWARI
2025-05-07 11:32 ` Dan Carpenter
0 siblings, 1 reply; 13+ messages in thread
From: ALOK TIWARI @ 2025-05-05 16:30 UTC (permalink / raw)
To: Dan Carpenter, Linus Walleij
Cc: Bartosz Golaszewski, linux-gpio, linux-kernel, Takahiro AKASHI
Hi Dan,
Thanks for your patch.
On 05-05-2025 17:07, Dan Carpenter wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> 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 <takahiro.akashi@linaro.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> drivers/pinctrl/core.c | 35 ++++++++++++++++++++++++++++++++
> include/linux/pinctrl/consumer.h | 9 ++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 4bdbf6bb26e2..4310f9e2118b 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -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
remove extra ' ' before * @config and On.
> + */
> +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);
can we add reason here, as now we are not calling pin_config_get_for_pin()
https://lore.kernel.org/all/20231002021602.260100-3-takahiro.akashi@linaro.org/
> + mutex_unlock(&pctldev->mutex);
> +
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_argument(*config);
a '\n' before return.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
> +
> static struct pinctrl_state *find_state(struct pinctrl *p,
> const char *name)
> {
Thanks,
Alok
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config()
2025-05-05 16:30 ` ALOK TIWARI
@ 2025-05-07 11:32 ` Dan Carpenter
0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2025-05-07 11:32 UTC (permalink / raw)
To: ALOK TIWARI
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel,
Takahiro AKASHI
On Mon, May 05, 2025 at 10:00:35PM +0530, ALOK TIWARI wrote:
> > +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);
>
> can we add reason here, as now we are not calling pin_config_get_for_pin()
>
> https://lore.kernel.org/all/20231002021602.260100-3-takahiro.akashi@linaro.org/
>
I don't even know why I changed that. Using pin_config_get_for_pin()
works and it's cleaner.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (2 preceding siblings ...)
2025-05-05 11:37 ` [RFC 3/7] pinctrl: introduce pinctrl_gpio_get_config() Dan Carpenter
@ 2025-05-05 11:38 ` Dan Carpenter
2025-05-05 11:39 ` [RFC 5/7] pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support Dan Carpenter
` (3 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:38 UTC (permalink / raw)
To: Sudeep Holla
Cc: Cristian Marussi, Linus Walleij, arm-scmi, linux-arm-kernel,
linux-gpio, linux-kernel, Takahiro AKASHI, Peng Fan
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 <dan.carpenter@linaro.org>
---
drivers/pinctrl/pinctrl-scmi.c | 3 +++
include/linux/pinctrl/pinconf-generic.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index df4bbcd7d1d5..362a6d2c3c68 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -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;
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 1bcf071b860e..b37838171581 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -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,
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC 5/7] pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (3 preceding siblings ...)
2025-05-05 11:38 ` [RFC 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE Dan Carpenter
@ 2025-05-05 11:39 ` Dan Carpenter
2025-05-05 11:39 ` [RFC 6/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:39 UTC (permalink / raw)
To: Sudeep Holla
Cc: Cristian Marussi, Linus Walleij, arm-scmi, linux-arm-kernel,
linux-gpio, linux-kernel, Peng Fan
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 <dan.carpenter@linaro.org>
---
drivers/pinctrl/pinctrl-scmi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 362a6d2c3c68..f369f0354e43 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -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;
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC 6/7] pinctrl-scmi: Add GPIO support
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (4 preceding siblings ...)
2025-05-05 11:39 ` [RFC 5/7] pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support Dan Carpenter
@ 2025-05-05 11:39 ` Dan Carpenter
2025-05-05 11:39 ` [RFC 7/7] pinctrl-scmi: remove unused struct member Dan Carpenter
2025-05-07 8:54 ` [RFC 0/7] pinctrl-scmi: Add GPIO support Khaled Ali Ahmed
7 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:39 UTC (permalink / raw)
To: Sudeep Holla
Cc: Cristian Marussi, Linus Walleij, Bartosz Golaszewski, arm-scmi,
linux-arm-kernel, linux-gpio, linux-kernel, Takahiro AKASHI
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 <dan.carpenter@linaro.org>
---
drivers/pinctrl/pinctrl-scmi.c | 206 ++++++++++++++++++++++++++++++++-
1 file changed, 205 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index f369f0354e43..40b432aa4756 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -6,6 +6,7 @@
* Copyright 2024 NXP
*/
+#include <linux/bitmap.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/errno.h>
@@ -16,6 +17,9 @@
#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/gpio/driver.h>
+
+#include <linux/pinctrl/consumer.h>
#include <linux/pinctrl/machine.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinconf-generic.h>
@@ -42,6 +46,7 @@ struct scmi_pinctrl {
unsigned int nr_functions;
struct pinctrl_pin_desc *pins;
unsigned int nr_pins;
+ struct gpio_chip *gc;
};
static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
@@ -505,6 +510,197 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
return 0;
}
+static int pinctrl_gpio_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ bitmap_fill(valid_mask, ngpios);
+ return 0;
+}
+
+static int pinctrl_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ unsigned long config;
+ bool in, out;
+ int ret;
+
+ config = PIN_CONFIG_INPUT_ENABLE;
+ ret = pinctrl_gpio_get_config(gc, offset, &config);
+ if (ret)
+ return ret;
+ in = config;
+
+ config = PIN_CONFIG_OUTPUT_ENABLE;
+ ret = pinctrl_gpio_get_config(gc, offset, &config);
+ if (ret)
+ return ret;
+ out = config;
+
+ /* Consistency check - in theory both can be enabled! */
+ if (in && !out)
+ return GPIO_LINE_DIRECTION_IN;
+ if (!in && out)
+ return GPIO_LINE_DIRECTION_OUT;
+
+ return -EINVAL;
+}
+
+static int pinctrl_gpio_direction_output_wrapper(struct gpio_chip *gc,
+ unsigned int offset, int val)
+{
+ return pinctrl_gpio_direction_output(gc, offset);
+}
+
+static int pinctrl_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ unsigned long config;
+ int ret;
+
+ config = PIN_CONFIG_INPUT_VALUE;
+ ret = pinctrl_gpio_get_config(gc, offset, &config);
+ if (ret)
+ return ret;
+
+ return config;
+}
+
+static void pinctrl_gpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ unsigned long config;
+
+ config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val);
+ pinctrl_gpio_set_config(gc, offset, config);
+}
+
+static int pinctrl_gc_to_func(struct gpio_chip *gc)
+{
+ struct scmi_pinctrl *pmx = gpiochip_get_data(gc);
+
+ return (gc - pmx->gc);
+}
+
+static int gpio_add_pin_ranges(struct gpio_chip *gc)
+{
+ struct scmi_pinctrl *pmx = gpiochip_get_data(gc);
+ const char * const *p_groups;
+ unsigned int n_groups;
+ int func = pinctrl_gc_to_func(gc);
+ int group;
+ int ret;
+
+ ret = pmx->pctl_desc.pmxops->get_function_groups(pmx->pctldev, func, &p_groups, &n_groups);
+ if (ret)
+ return ret;
+
+ // FIXME: fix the correct group from the device tree
+ for (group = 0; group < n_groups; group++) {
+ ret = gpiochip_add_pingroup_range(gc, pmx->pctldev, 0, p_groups[group]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int get_nr_pins_in_function(struct scmi_pinctrl *pmx, int func)
+{
+ const char * const *pin_groups;
+ unsigned int n_groups;
+ const unsigned int *pins;
+ unsigned int n_pins;
+ int total = 0;
+ int i, ret;
+
+ // FIXME: get the correct number of gc.ngpio
+ // Find the right group from the device tree
+ ret = pmx->pctl_desc.pmxops->get_function_groups(pmx->pctldev, func, &pin_groups, &n_groups);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < n_groups; i++) {
+ ret = pinctrl_get_group_pins(pmx->pctldev, pin_groups[i], &pins, &n_pins);
+ if (ret)
+ return ret;
+ total += n_pins;
+ }
+
+ return total;
+}
+
+static int register_scmi_pinctrl_gpio_handler(struct device *dev, struct scmi_pinctrl *pmx)
+{
+ struct fwnode_handle *gpio = NULL;
+ int ret, i;
+
+ 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;
+
+ 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;
+ 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;
+ 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;
+
+ pmx->gc[i].init_valid_mask = pinctrl_gpio_init_valid_mask;
+ pmx->gc[i].parent = dev;
+ pmx->gc[i].base = -1;
+ }
+
+ return 0;
+}
+
+static int scmi_gpiochip_add_data(struct device *dev, struct scmi_pinctrl *pmx)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < pmx->nr_functions; i++) {
+ ret = pinctrl_ops->is_gpio(pmx->ph, i, FUNCTION_TYPE);
+ if (ret < 0)
+ return ret;
+ if (ret == false)
+ continue;
+
+ ret = devm_gpiochip_add_data(dev, &pmx->gc[i], pmx);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static const char * const scmi_pinctrl_blocklist[] = {
"fsl,imx95",
NULL
@@ -558,7 +754,15 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
if (!pmx->functions)
return -ENOMEM;
- return pinctrl_enable(pmx->pctldev);
+ ret = register_scmi_pinctrl_gpio_handler(dev, pmx);
+ if (ret)
+ return ret;
+
+ ret = pinctrl_enable(pmx->pctldev);
+ if (ret)
+ return ret;
+
+ return scmi_gpiochip_add_data(dev, pmx);
}
static const struct scmi_device_id scmi_id_table[] = {
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC 7/7] pinctrl-scmi: remove unused struct member
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (5 preceding siblings ...)
2025-05-05 11:39 ` [RFC 6/7] pinctrl-scmi: Add GPIO support Dan Carpenter
@ 2025-05-05 11:39 ` Dan Carpenter
2025-05-07 8:54 ` [RFC 0/7] pinctrl-scmi: Add GPIO support Khaled Ali Ahmed
7 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2025-05-05 11:39 UTC (permalink / raw)
To: Sudeep Holla
Cc: Cristian Marussi, Linus Walleij, arm-scmi, linux-arm-kernel,
linux-gpio, linux-kernel
The ->nr_pins is not used so delete that.
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/pinctrl/pinctrl-scmi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 40b432aa4756..d1f3c126c3cb 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -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;
};
--
2.47.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC 0/7] pinctrl-scmi: Add GPIO support
2025-05-05 11:36 [RFC 0/7] pinctrl-scmi: Add GPIO support Dan Carpenter
` (6 preceding siblings ...)
2025-05-05 11:39 ` [RFC 7/7] pinctrl-scmi: remove unused struct member Dan Carpenter
@ 2025-05-07 8:54 ` Khaled Ali Ahmed
7 siblings, 0 replies; 13+ messages in thread
From: Khaled Ali Ahmed @ 2025-05-07 8:54 UTC (permalink / raw)
To: Dan Carpenter, Linus Walleij
Cc: arm-scmi@vger.kernel.org, Bartosz Golaszewski, Cristian Marussi,
linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org, Sudeep Holla, Vincent Guittot,
Girish Pathak, Peng Fan, Takahiro AKASHI
Good morning Dan,
Regarding the scmi_pinctrl stack design, what we have made in the SW is that the stack can communicate with multiple drivers with only two constraints:
1- Implement the interfacing APIs. which is declared by the object "struct mod_pinctrl_drv_api".
2- Integrate itself with the scmi_pinctrl HAL or backend as you prefer.
Also, we have an example I can discuss if you like.
thanks in advance
________________________________________
From: Dan Carpenter <dan.carpenter@linaro.org>
Sent: Monday, May 5, 2025 12:36 PM
To: Linus Walleij <linus.walleij@linaro.org>
Cc: arm-scmi@vger.kernel.org <arm-scmi@vger.kernel.org>; Bartosz Golaszewski <brgl@bgdev.pl>; Cristian Marussi <Cristian.Marussi@arm.com>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Sudeep Holla <Sudeep.Holla@arm.com>; Vincent Guittot <vincent.guittot@linaro.org>; Khaled Ali Ahmed <Khaled.AliAhmed@arm.com>; Girish Pathak <Girish.Pathak@arm.com>; Peng Fan <peng.fan@nxp.com>; Takahiro AKASHI <akashi.tkhro@gmail.com>
Subject: [RFC 0/7] pinctrl-scmi: Add GPIO support
This patchset adds GPIO support to the SCMI pin control driver.
AKASHI Takahiro did some of that work earlier, but we decided to make
this a part of the pinctrl driver instead of a separate GPIO driver.
I'm not really sure how the device tree stuff wires it all in. I've
been using a mock driver on SCP with virtio to test it.
Dan Carpenter (7):
firmware: arm_scmi: move boiler plate code into the get info functions
firmware: arm_scmi: add is_gpio() function
pinctrl: introduce pinctrl_gpio_get_config()
pinctrl-scmi: implement 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 | 146 +++++++++-------
drivers/pinctrl/core.c | 35 ++++
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, 339 insertions(+), 69 deletions(-)
--
2.47.2
^ permalink raw reply [flat|nested] 13+ messages in thread