From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
The name of the pin function has no real meaning to pinctrl core and is
there only for human readability of device properties. Some pins are
muxed as GPIOs but for "strict" pinmuxers it's impossible to request
them as GPIOs if they're bound to a devide - even if their function name
explicitly says "gpio". Add a new field to struct pinfunction that
allows to pass additional flags to pinctrl core. While we could go with
a boolean "is_gpio" field, a flags field is more future-proof.
If the PINFUNCTION_FLAG_GPIO is set for a given function, the pin muxed
to it can be requested as GPIO even on strict pin controllers. Add a new
callback to struct pinmux_ops - function_is_gpio() - that allows pinmux
core to inspect a function and see if it's a GPIO one. Provide a generic
implementation of this callback.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/pinctrl/pinmux.c | 36 ++++++++++++++++++++++++++++++++++--
drivers/pinctrl/pinmux.h | 3 +++
include/linux/pinctrl/pinctrl.h | 14 ++++++++++++++
include/linux/pinctrl/pinmux.h | 2 ++
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 504dbb3e97cf334e39b49121137c6768081fcd40..52623b47cc87b49b649610eabfa547d7543292dd 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -89,13 +89,19 @@ bool pinmux_can_be_used_for_gpio(struct pinctrl_dev *pctldev, unsigned int pin)
{
struct pin_desc *desc = pin_desc_get(pctldev, pin);
const struct pinmux_ops *ops = pctldev->desc->pmxops;
+ const struct pinctrl_setting_mux *mux_setting = desc->mux_setting;
+ bool func_is_gpio = false;
/* Can't inspect pin, assume it can be used */
if (!desc || !ops)
return true;
guard(mutex)(&desc->mux_lock);
- if (ops->strict && desc->mux_usecount)
+ if (ops->function_is_gpio && mux_setting)
+ func_is_gpio = ops->function_is_gpio(pctldev,
+ mux_setting->func);
+
+ if (ops->strict && desc->mux_usecount && !func_is_gpio)
return false;
return !(ops->strict && !!desc->gpio_owner);
@@ -116,7 +122,9 @@ static int pin_request(struct pinctrl_dev *pctldev,
{
struct pin_desc *desc;
const struct pinmux_ops *ops = pctldev->desc->pmxops;
+ const struct pinctrl_setting_mux *mux_setting;
int status = -EINVAL;
+ bool func_is_gpio = false;
desc = pin_desc_get(pctldev, pin);
if (desc == NULL) {
@@ -126,11 +134,16 @@ static int pin_request(struct pinctrl_dev *pctldev,
goto out;
}
+ mux_setting = desc->mux_setting;
+
dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
pin, desc->name, owner);
scoped_guard(mutex, &desc->mux_lock) {
- if ((!gpio_range || ops->strict) &&
+ if (ops->function_is_gpio && mux_setting)
+ func_is_gpio = ops->function_is_gpio(pctldev,
+ mux_setting->func);
+ if ((!gpio_range || ops->strict) && !func_is_gpio &&
desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
dev_err(pctldev->dev,
"pin %s already requested by %s; cannot claim for %s\n",
@@ -861,6 +874,25 @@ pinmux_generic_get_function(struct pinctrl_dev *pctldev, unsigned int selector)
}
EXPORT_SYMBOL_GPL(pinmux_generic_get_function);
+/**
+ * pinmux_generic_function_is_gpio() - returns true if given function is a GPIO
+ * @pctldev: pin controller device
+ * @selector: function number
+ */
+bool pinmux_generic_function_is_gpio(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ struct function_desc *function;
+
+ function = radix_tree_lookup(&pctldev->pin_function_tree,
+ selector);
+ if (!function)
+ return false;
+
+ return function->func->flags & PINFUNCTION_FLAG_GPIO;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_function_is_gpio);
+
/**
* pinmux_generic_add_function() - adds a function group
* @pctldev: pin controller device
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index 653684290666d78fd725febb5f8bc987b66a1afb..4e826c1a5246cf8b1ac814c8c0df24c4e036edd2 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -169,6 +169,9 @@ int pinmux_generic_remove_function(struct pinctrl_dev *pctldev,
void pinmux_generic_free_functions(struct pinctrl_dev *pctldev);
+bool pinmux_generic_function_is_gpio(struct pinctrl_dev *pctldev,
+ unsigned int selector);
+
#else
static inline void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index d138e18156452e008f24ca06358fcab45135632f..1a8084e2940537f8f0862761d3e47c56c8783193 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -11,6 +11,7 @@
#ifndef __LINUX_PINCTRL_PINCTRL_H
#define __LINUX_PINCTRL_PINCTRL_H
+#include <linux/bits.h>
#include <linux/types.h>
struct device;
@@ -206,16 +207,20 @@ extern int pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
const char *pin_group, const unsigned int **pins,
unsigned int *num_pins);
+#define PINFUNCTION_FLAG_GPIO BIT(0)
+
/**
* struct pinfunction - Description about a function
* @name: Name of the function
* @groups: An array of groups for this function
* @ngroups: Number of groups in @groups
+ * @flags: Additional pin function flags
*/
struct pinfunction {
const char *name;
const char * const *groups;
size_t ngroups;
+ unsigned long flags;
};
/* Convenience macro to define a single named pinfunction */
@@ -226,6 +231,15 @@ struct pinfunction {
.ngroups = (_ngroups), \
}
+/* Same as PINCTRL_PINFUNCTION() but for the GPIO category of functions */
+#define PINCTRL_GPIO_PINFUNCTION(_name, _groups, _ngroups) \
+(struct pinfunction) { \
+ .name = (_name), \
+ .groups = (_groups), \
+ .ngroups = (_ngroups), \
+ .flags = PINFUNCTION_FLAG_GPIO, \
+ }
+
#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_PINCTRL)
extern struct pinctrl_dev *of_pinctrl_get(struct device_node *np);
#else
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index d6f7b58d6ad0cce421aad80463529c9ccc65d68e..6db6c3e1ccc2249d4b4204e6fc19bf7b4397cc81 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -66,6 +66,8 @@ struct pinmux_ops {
unsigned int selector,
const char * const **groups,
unsigned int *num_groups);
+ bool (*function_is_gpio) (struct pinctrl_dev *pctldev,
+ unsigned int selector);
int (*set_mux) (struct pinctrl_dev *pctldev, unsigned int func_selector,
unsigned int group_selector);
int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
--
2.48.1On Thu, Jul 24, 2025 at 11:25 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> The name of the pin function has no real meaning to pinctrl core and is
> there only for human readability of device properties. Some pins are
> muxed as GPIOs but for "strict" pinmuxers it's impossible to request
> them as GPIOs if they're bound to a devide - even if their function name
> explicitly says "gpio". Add a new field to struct pinfunction that
> allows to pass additional flags to pinctrl core. While we could go with
passing
to the pinctrl
> a boolean "is_gpio" field, a flags field is more future-proof.
>
> If the PINFUNCTION_FLAG_GPIO is set for a given function, the pin muxed
> to it can be requested as GPIO even on strict pin controllers. Add a new
"...the pin, which is muxed to it, ..."
> callback to struct pinmux_ops - function_is_gpio() - that allows pinmux
> core to inspect a function and see if it's a GPIO one. Provide a generic
> implementation of this callback.
...
> - if (ops->strict && desc->mux_usecount)
> + if (ops->function_is_gpio && mux_setting)
Seems mux_setting presence is prior to the GPIO checks, I would swap
the parameters of &&.
> + func_is_gpio = ops->function_is_gpio(pctldev,
> + mux_setting->func);
One line is okay.
> + if (ops->strict && desc->mux_usecount && !func_is_gpio)
> return false;
>
> return !(ops->strict && !!desc->gpio_owner);
I think this whole if/return chain can be made slightly more readable,
but I haven't had something to provide right now. Lemme think about
it,
...
> + if (ops->function_is_gpio && mux_setting)
> + func_is_gpio = ops->function_is_gpio(pctldev,
> + mux_setting->func);
> + if ((!gpio_range || ops->strict) && !func_is_gpio &&
> desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
This is very similar to the above check, I think at bare minimum here
can be a helper for both cases.
...
> +/**
> + * pinmux_generic_function_is_gpio() - returns true if given function is a GPIO
> + * @pctldev: pin controller device
> + * @selector: function number
Missing Return section. Please run kernel-doc validator against new kernel-docs.
> + */
> +bool pinmux_generic_function_is_gpio(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + struct function_desc *function;
> +
> + function = radix_tree_lookup(&pctldev->pin_function_tree,
> + selector);
One line is okay.
> + if (!function)
> + return false;
> +
> + return function->func->flags & PINFUNCTION_FLAG_GPIO;
> +}
...
> struct pinfunction {
> const char *name;
> const char * const *groups;
> size_t ngroups;
> + unsigned long flags;
Not sure we need this. If the function is GPIO, pin control already
knows about this. The pin muxing has gpio request / release callbacks
that change the state. Why do we need an additional flag(s)?
> };
--
With Best Regards,
Andy Shevchenko
On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> > struct pinfunction {
> > const char *name;
> > const char * const *groups;
> > size_t ngroups;
> > + unsigned long flags;
>
> Not sure we need this. If the function is GPIO, pin control already
> knows about this. The pin muxing has gpio request / release callbacks
> that change the state. Why do we need an additional flag(s)?
>
I'm not following, how does the pin controller know that the function
is GPIO exactly, other than by the bit set in this field?
Bartosz
On Wed, Jul 30, 2025 at 11:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > > struct pinfunction {
> > > const char *name;
> > > const char * const *groups;
> > > size_t ngroups;
> > > + unsigned long flags;
> >
> > Not sure we need this. If the function is GPIO, pin control already
> > knows about this. The pin muxing has gpio request / release callbacks
> > that change the state. Why do we need an additional flag(s)?
> >
>
> I'm not following, how does the pin controller know that the function
> is GPIO exactly, other than by the bit set in this field?
AFAICS the gpio_owner != NULL means that. No need to have a duplicate
of this information.
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 30, 2025 at 2:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jul 30, 2025 at 11:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > > struct pinfunction {
> > > > const char *name;
> > > > const char * const *groups;
> > > > size_t ngroups;
> > > > + unsigned long flags;
> > >
> > > Not sure we need this. If the function is GPIO, pin control already
> > > knows about this. The pin muxing has gpio request / release callbacks
> > > that change the state. Why do we need an additional flag(s)?
> > >
> >
> > I'm not following, how does the pin controller know that the function
> > is GPIO exactly, other than by the bit set in this field?
>
> AFAICS the gpio_owner != NULL means that. No need to have a duplicate
> of this information.
>
No, that's not at all what this series does... gpio_owner is the
consumer label of a pin used by the GPIOLIB framework. The flag I'm
introducing it telling the pinctrl core - before GPIOLIB is ever
involved - that *this pin can be requested as a GPIO by GPIOLIB*. It's
the other way around - without knowing this, for strict pinmuxers,
GPIOLIB would never be able to request this pin if it was muxed to a
function (even if the function is called "GPIO").
Bart
On Wed, Jul 30, 2025 at 2:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Jul 30, 2025 at 2:50 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jul 30, 2025 at 11:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > > struct pinfunction {
> > > > > const char *name;
> > > > > const char * const *groups;
> > > > > size_t ngroups;
> > > > > + unsigned long flags;
> > > >
> > > > Not sure we need this. If the function is GPIO, pin control already
> > > > knows about this. The pin muxing has gpio request / release callbacks
> > > > that change the state. Why do we need an additional flag(s)?
> > >
> > > I'm not following, how does the pin controller know that the function
> > > is GPIO exactly, other than by the bit set in this field?
> >
> > AFAICS the gpio_owner != NULL means that. No need to have a duplicate
> > of this information.
>
> No, that's not at all what this series does... gpio_owner is the
> consumer label of a pin used by the GPIOLIB framework. The flag I'm
> introducing it telling the pinctrl core - before GPIOLIB is ever
> involved - that *this pin can be requested as a GPIO by GPIOLIB*.
The certain pin control driver may even not know about this. But even
though the proposed change is an overkill. If it indeed needs to be
done, the solution of valid_mask approach sounds to me much better. It
will be a single bitmask per pin control to tell this.
> It's
> the other way around - without knowing this, for strict pinmuxers,
> GPIOLIB would never be able to request this pin if it was muxed to a
> function (even if the function is called "GPIO").
I need to read the series again, but I truly believe we don't need
this new field in the struct pinfunction.
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 30, 2025 at 3:30 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jul 30, 2025 at 2:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Wed, Jul 30, 2025 at 2:50 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Wed, Jul 30, 2025 at 11:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > > struct pinfunction {
> > > > > > const char *name;
> > > > > > const char * const *groups;
> > > > > > size_t ngroups;
> > > > > > + unsigned long flags;
> > > > >
> > > > > Not sure we need this. If the function is GPIO, pin control already
> > > > > knows about this. The pin muxing has gpio request / release callbacks
> > > > > that change the state. Why do we need an additional flag(s)?
> > > >
> > > > I'm not following, how does the pin controller know that the function
> > > > is GPIO exactly, other than by the bit set in this field?
> > >
> > > AFAICS the gpio_owner != NULL means that. No need to have a duplicate
> > > of this information.
> >
> > No, that's not at all what this series does... gpio_owner is the
> > consumer label of a pin used by the GPIOLIB framework. The flag I'm
> > introducing it telling the pinctrl core - before GPIOLIB is ever
> > involved - that *this pin can be requested as a GPIO by GPIOLIB*.
>
> The certain pin control driver may even not know about this. But even
> though the proposed change is an overkill. If it indeed needs to be
> done, the solution of valid_mask approach sounds to me much better. It
> will be a single bitmask per pin control to tell this.
>
> > It's
> > the other way around - without knowing this, for strict pinmuxers,
> > GPIOLIB would never be able to request this pin if it was muxed to a
> > function (even if the function is called "GPIO").
>
> I need to read the series again, but I truly believe we don't need
> this new field in the struct pinfunction.
>
Without a code example, I can't tell what you're imagining but let me
give some more context: the flags field could only exist in the
qualcomm drivers but the problem will be the same on all existing
platforms so IMO it's better to centralize it right away. And if we're
already centralizing it, let's make it future proof by making it
possible to define more such flags if we need it. Since the GPIO
category is a function property, it only makes sense to put it in the
structure defining the function.
Bartosz
On Wed, Jul 30, 2025 at 2:49 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jul 30, 2025 at 11:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Thu, Jul 24, 2025 at 2:22 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > > struct pinfunction {
> > > > const char *name;
> > > > const char * const *groups;
> > > > size_t ngroups;
> > > > + unsigned long flags;
> > >
> > > Not sure we need this. If the function is GPIO, pin control already
> > > knows about this. The pin muxing has gpio request / release callbacks
> > > that change the state. Why do we need an additional flag(s)?
> >
> > I'm not following, how does the pin controller know that the function
> > is GPIO exactly, other than by the bit set in this field?
>
> AFAICS the gpio_owner != NULL means that. No need to have a duplicate
> of this information.
To be clear, the pin control and muxing core knows about this, if the
certain pin control driver needs that information it can request this
from the core or do some other shortcuts (as it knows the state as
well in the HW). So, I do not see any need for this flag. But again,
maybe I'm missing the subtle corner case?
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.