[PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output

[PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output

[Stefan Wahren]

Based on a recent discussion [1],[2] about the unexpected output of gpioinfo
this patch series tries to improve the situation. Patches 1-3 are specific
changes for bcm2835 while the last one is the intended improvement for gpiolib.

Changes in V2:
- take strict mode into account as suggested by Linus
- add bcm2835 specific changes

[1] - https://marc.info/?l=linux-gpio&m=154716249606544
[2] - https://marc.info/?l=linux-gpio&m=154747265118765

Stefan Wahren (4):
  pinctrl: bcm2835: declare pin config as generic
  pinctrl: bcm2835: Direct GPIO config changes to generic pinctrl
  pinctrl: bcm2835: activate strict mux mode
  gpiolib: Take MUX usage into account

 drivers/gpio/gpiolib.c                |  3 ++-
 drivers/pinctrl/bcm/pinctrl-bcm2835.c |  5 ++++-
 drivers/pinctrl/core.c                | 23 +++++++++++++++++++++++
 drivers/pinctrl/pinmux.c              | 18 ++++++++++++++++++
 drivers/pinctrl/pinmux.h              |  7 +++++++
 include/linux/pinctrl/consumer.h      |  6 ++++++
 6 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH RFC V2 1/4] pinctrl: bcm2835: declare pin config as generic

[Stefan Wahren]

Since commit 0de704955ee44 ("pinctrl: bcm2835: Add support for
generic pinctrl binding") this driver is capable to use the generic
interface. So declare this accordingly.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index f180aa4..19eb4fd 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -969,6 +969,7 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops bcm2835_pinconf_ops = {
+	.is_generic = true,
 	.pin_config_get = bcm2835_pinconf_get,
 	.pin_config_set = bcm2835_pinconf_set,
 };
-- 
2.7.4

Re: [PATCH RFC V2 1/4] pinctrl: bcm2835: declare pin config as generic

[Linus Walleij]

On Mon, Jan 21, 2019 at 8:12 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:

> Since commit 0de704955ee44 ("pinctrl: bcm2835: Add support for
> generic pinctrl binding") this driver is capable to use the generic
> interface. So declare this accordingly.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>

Patch applied.

Yours,
Linus Walleij

[PATCH RFC V2 2/4] pinctrl: bcm2835: Direct GPIO config changes to generic pinctrl

[Stefan Wahren]

In order to support GPIO config changes direct these to the generic pinctrl.
This also requires an adjust of the return code for unsupported parameter
otherwise gpiod_configure_flags wont work as expected.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 19eb4fd..183d1ff 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -341,6 +341,7 @@ static const struct gpio_chip bcm2835_gpio_chip = {
 	.get_direction = bcm2835_gpio_get_direction,
 	.get = bcm2835_gpio_get,
 	.set = bcm2835_gpio_set,
+	.set_config = gpiochip_generic_config,
 	.base = -1,
 	.ngpio = BCM2835_NUM_GPIOS,
 	.can_sleep = false,
@@ -960,7 +961,7 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			break;
 
 		default:
-			return -EINVAL;
+			return -ENOTSUPP;
 
 		} /* switch param type */
 	} /* for each config */
-- 
2.7.4

[PATCH RFC V2 3/4] pinctrl: bcm2835: activate strict mux mode

[Stefan Wahren]

This activates strict mode muxing for the bcm2835 pin controller,
as the GPIO Function Select Registers do not allow GPIO and functions
at the same time.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 183d1ff..452e734 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -894,6 +894,7 @@ static const struct pinmux_ops bcm2835_pmx_ops = {
 	.set_mux = bcm2835_pmx_set,
 	.gpio_disable_free = bcm2835_pmx_gpio_disable_free,
 	.gpio_set_direction = bcm2835_pmx_gpio_set_direction,
+	.strict = true,
 };
 
 static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
-- 
2.7.4

Re: [PATCH RFC V2 3/4] pinctrl: bcm2835: activate strict mux mode

[Stefan Wahren]

Am 21.01.19 um 08:34 schrieb Lukas Wunner:
> On Mon, Jan 21, 2019 at 08:11:24AM +0100, Stefan Wahren wrote:
>> This activates strict mode muxing for the bcm2835 pin controller,
>> as the GPIO Function Select Registers do not allow GPIO and functions
>> at the same time.
> bcm2835_spi_setup() in spi-bcm2835.c converts a native chip select
> to a gpio chip select.  So pins 7+8 and/or 35+36 may be set to
> function alt0 but in reality the pins are requested and driven as
> generic output gpio pins.
>
> I'm not sure but doesn't this break in strict mode?

That's a good point. I hope that isn't a problem because owner should be
the same.

But i didn't test this scenario. Do you mind to give this series a try?

The only issue i found so far was a unnecessary pinctrl on the Raspberry
Pi Zero W, which should be fixed with this [1].

At the end we need to switch to strict mode, because currently a simple
gpioset can "destroy" the mux setting for I2C or SPI.

[1] - https://marc.info/?l=linux-arm-kernel&m=154800402627578

>
> Thanks,
>
> Lukas
>

[PATCH RFC V2 4/4] gpiolib: Take MUX usage into account

[Stefan Wahren]

The user space like gpioinfo only see the GPIO usage but not the
MUX usage (e.g. I2C or SPI usage) of a pin. As a user we want to know which
pin is free/safe to use. So take the MUX usage of strict pinmux controllers
into account to get a more realistic view for ioctl GPIO_GET_LINEINFO_IOCTL.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/gpio/gpiolib.c           |  3 ++-
 drivers/pinctrl/core.c           | 23 +++++++++++++++++++++++
 drivers/pinctrl/pinmux.c         | 18 ++++++++++++++++++
 drivers/pinctrl/pinmux.h         |  7 +++++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 5 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1651d7f..da3b008 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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))
 			lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
 		if (test_bit(FLAG_IS_OUT, &desc->flags))
 			lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c6ff4d5..6e097e4 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -760,6 +760,29 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 	return -EINVAL;
 }
 
+bool pinctrl_gpio_is_in_use(unsigned gpio)
+{
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_gpio_range *range;
+	bool result;
+	int pin;
+
+	if (pinctrl_get_device_gpio_range(gpio, &pctldev, &range))
+		return false;
+
+	mutex_lock(&pctldev->mutex);
+
+	/* Convert to the pin controllers number space */
+	pin = gpio_to_pin(range, gpio);
+
+	result = pinmux_is_in_use(pctldev, pin);
+
+	mutex_unlock(&pctldev->mutex);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_is_in_use);
+
 /**
  * pinctrl_gpio_request() - request a single pin to be used as GPIO
  * @gpio: the GPIO pin number from the GPIO subsystem number space
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 4d0cc18..c1f6d46 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -71,6 +71,24 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i)
 	return 0;
 }
 
+bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin)
+{
+	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);
+		return false;
+	}
+
+	if (ops->strict && desc->mux_usecount)
+		return true;
+
+	return ops->strict && !!desc->gpio_owner;
+}
+
 /**
  * pin_request() - request a single pin to be muxed in, typically for GPIO
  * @pin: the pin number in the global pin space
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index 3319535..1da2a75 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -16,6 +16,8 @@ int pinmux_check_ops(struct pinctrl_dev *pctldev);
 
 int pinmux_validate_map(const struct pinctrl_map *map, int i);
 
+bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin);
+
 int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
 			unsigned pin, unsigned gpio);
@@ -43,6 +45,11 @@ static inline int pinmux_validate_map(const struct pinctrl_map *map, int i)
 	return 0;
 }
 
+static inline bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin)
+{
+	return false;
+}
+
 static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range,
 			unsigned pin, unsigned gpio)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9..0093a2b 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -25,6 +25,7 @@ struct device;
 #ifdef CONFIG_PINCTRL
 
 /* External interface to pin control */
+extern bool pinctrl_gpio_is_in_use(unsigned gpio);
 extern int pinctrl_gpio_request(unsigned gpio);
 extern void pinctrl_gpio_free(unsigned gpio);
 extern int pinctrl_gpio_direction_input(unsigned gpio);
@@ -62,6 +63,11 @@ static inline int pinctrl_pm_select_idle_state(struct device *dev)
 
 #else /* !CONFIG_PINCTRL */
 
+static inline bool pinctrl_gpio_is_in_use(unsigned gpio)
+{
+	return 0;
+}
+
 static inline int pinctrl_gpio_request(unsigned gpio)
 {
 	return 0;
-- 
2.7.4

Re: [PATCH RFC V2 4/4] gpiolib: Take MUX usage into account

[Linus Walleij]

Hi Stefan!

I like the idea with these patches.

Some thinking aloud below:

On Mon, Jan 21, 2019 at 8:11 AM Stefan Wahren <stefan.wahren@i2se.com> 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";
                                        bias-pull-up;
                                };
                        };
                        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.

Yours,
Linus Walleij

Re: [PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output

[Linus Walleij]

On Mon, Jan 21, 2019 at 8:11 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:

> Based on a recent discussion [1],[2] about the unexpected output of gpioinfo
> this patch series tries to improve the situation. Patches 1-3 are specific
> changes for bcm2835 while the last one is the intended improvement for gpiolib.
>
> Changes in V2:
> - take strict mode into account as suggested by Linus
> - add bcm2835 specific changes

Ooops started to apply this but now I noticed it's in RFC state so I
backed them out and wait for a non-RFC version first.

Yours,
Linus Walleij

Re: [PATCH RFC V2 0/4] pinctrl: bcm2835: improve libgpiod output

[Stefan Wahren]

Hi Linus,

Am 28.01.19 um 09:06 schrieb Linus Walleij:
> On Mon, Jan 21, 2019 at 8:11 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
>> Based on a recent discussion [1],[2] about the unexpected output of gpioinfo
>> this patch series tries to improve the situation. Patches 1-3 are specific
>> changes for bcm2835 while the last one is the intended improvement for gpiolib.
>>
>> Changes in V2:
>> - take strict mode into account as suggested by Linus
>> - add bcm2835 specific changes
> Ooops started to apply this but now I noticed it's in RFC state so I
> backed them out and wait for a non-RFC version first.

i will split this series and resend the non-controversial part (Patch 1+2).

Stefan

>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel