archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <>
To: Stefan Wahren <>
Cc: "open list:GPIO SUBSYSTEM" <>,
	Eric Anholt <>,
	Bartosz Golaszewski <>,
	Lukas Wunner <>,
	Peter Robinson <>,
	Linux ARM <>
Subject: Re: [PATCH RFC V2 4/4] gpiolib: Take MUX usage into account
Date: Mon, 28 Jan 2019 10:06:46 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Hi Stefan!

I like the idea with these patches.

Some thinking aloud below:

On Mon, Jan 21, 2019 at 8:11 AM Stefan Wahren <> wrote:

> @@ -1075,7 +1075,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                     test_bit(FLAG_IS_HOGGED, &desc->flags) ||
>                     test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
>                     test_bit(FLAG_EXPORT, &desc->flags) ||
> -                   test_bit(FLAG_SYSFS, &desc->flags))
> +                   test_bit(FLAG_SYSFS, &desc->flags) ||
> +                   pinctrl_gpio_is_in_use(chip->base + lineinfo.line_offset))

So the idea is that if a line is already in use somehow, we do not
present it to userspace. Fair enough.

> +bool pinctrl_gpio_is_in_use(unsigned gpio)

Please rename it pinctrl_gpio_can_use_line(), as it is more
specific. (Also see below as to why.)

> +{
> +       struct pinctrl_dev *pctldev;
> +       struct pinctrl_gpio_range *range;
> +       bool result;
> +       int pin;
> +
> +       if (pinctrl_get_device_gpio_range(gpio, &pctldev, &range))
> +               return false;

So if this tries to obtain the GPIO range, if the gpio line
is not found in any range, we decide that it is not in use.

Add a comment saying that if we don't find a range, it probably
means we are using a GPIO driver without a backing
pin controller, so we bail out. (Just like the stub does, maybe
add that kind of comment to the stub as well.)

> +       mutex_lock(&pctldev->mutex);
> +
> +       /* Convert to the pin controllers number space */
> +       pin = gpio_to_pin(range, gpio);
> +
> +       result = pinmux_is_in_use(pctldev, pin);

It gets hard to understand the code here, we need to either add
a comment on what's going on or rename that function
(as described below) or both.

> +
> +       mutex_unlock(&pctldev->mutex);
> +
> +       return result;
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_gpio_is_in_use);


> +bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin)

Please name is pinmux_can_be_used_for_gpio() as non-strict
pin controllers allow non-exclusive use and return false,
and that is something GPIO-specific.

I see the idea with the naming, but the fact that this will
return false in some cases where the pin is actually is in
use makes the function name misleading.

Add some kerneldoc above it explaining that this returns true
if the pin is in any kind of use whether by a function or some
GPIO and the controller is strict.

> +{
> +       struct pin_desc *desc = pin_desc_get(pctldev, pin);
> +       const struct pinmux_ops *ops = pctldev->desc->pmxops;
> +
> +       if (!desc) {
> +               dev_err(pctldev->dev,
> +                       "pin %u is not registered so it cannot be requested\n",
> +                       pin);

Is this error message really relevant?

Should be something like "cannot be inspected" but I suspect it
should just silently return false.

> +               return false;
> +       }
> +
> +       if (ops->strict && desc->mux_usecount)
> +               return true;

We return here if the pin is used for some function.

This *could* create a problem for some systems.

In those systems, a pin controller may define a hog in the
device tree like this:

pinctrl@800000 {
                        foo_gpios: foo {
                                bar {
                                        pins = "gpio57";
                        pinctrl-names = "default";
                        pinctrl-0 = <&foo_gpios>;

This means this controller is not using the fast path with
gpio_owner etc, but instead has created one group per
GPIO pin, which is a common solution.

What the device tree want to do is set this pin up as GPIO,
and apply a pull up, but still expects that pin to be used
from userspace later on. This patch however, would
hide the pin from userspace if the pin controller is strict.

I don't know for sure if this is a real problem, probably not
because such a pin controller should be careful not to
set itself as strict. (That would strictly speaking be a bug IMO, they
are already hogging the line mux and thus the pin controller
itself IS using it, so....) But if someone is using this approach
today it would cause a regression.

But I think we are safe.

> +       return ops->strict && !!desc->gpio_owner;

This catches the case for gpio controllers using the
fast path to access GPIOs very nicely, i.e. controllers
implementing .gpio_request_enable() etc in their
struct pinmux_ops.

Add a comment saying that all non-strict controllers
will return false here thus making all pins in a GPIO range
available for use.

Linus Walleij

  reply	other threads:[~2019-01-28  9:06 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-21  7:11 [PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output Stefan Wahren
2019-01-21  7:11 ` [PATCH RFC V2 1/4] pinctrl: bcm2835: declare pin config as generic Stefan Wahren
2019-01-28  8:02   ` Linus Walleij
2019-01-21  7:11 ` [PATCH RFC V2 2/4] pinctrl: bcm2835: Direct GPIO config changes to generic pinctrl Stefan Wahren
2019-01-21  7:11 ` [PATCH RFC V2 3/4] pinctrl: bcm2835: activate strict mux mode Stefan Wahren
     [not found]   ` <>
2019-01-21  9:57     ` Stefan Wahren
2019-01-21  7:11 ` [PATCH RFC V2 4/4] gpiolib: Take MUX usage into account Stefan Wahren
2019-01-28  9:06   ` Linus Walleij [this message]
2019-01-28  8:06 ` [PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output Linus Walleij
2019-01-28  9:23   ` Stefan Wahren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).