[RFC] gpiolib: Take MUX usage into account
diff mbox series

Message ID 1547414494-8556-1-git-send-email-stefan.wahren@i2se.com
State New
Headers show
Series
  • [RFC] gpiolib: Take MUX usage into account
Related show

Commit Message

Stefan Wahren Jan. 13, 2019, 9:21 p.m. UTC
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 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         | 17 +++++++++++++++++
 drivers/pinctrl/pinmux.h         |  7 +++++++
 include/linux/pinctrl/consumer.h |  6 ++++++
 5 files changed, 55 insertions(+), 1 deletion(-)

Hi,

as proposed in a recent discussion [1], this is a hack to improve gpioinfo.
I think this could a start point for a discussion.

Obvious drawback is the resulting overhead for the ioctl call.

Stefan

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

Comments

Linus Walleij Jan. 14, 2019, 1:30 p.m. UTC | #1
On Sun, Jan 13, 2019 at 10:22 PM Stefan Wahren <stefan.wahren@i2se.com> wrote:

> as proposed in a recent discussion [1], this is a hack to improve gpioinfo.
> I think this could a start point for a discussion.

I like the idea.

> +bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin)
> +{
> +       struct pin_desc *desc = pin_desc_get(pctldev, pin);
> +
> +       if (!desc) {
> +               dev_err(pctldev->dev,
> +                       "pin %u is not registered so it cannot be requested\n",
> +                       pin);
> +               return false;
> +       }
> +
> +       if (desc->mux_usecount)
> +               return true;
> +
> +       return !!desc->gpio_owner;
> +}

This needs to be augmented to respect the .strict attribute of the
pin.

Only if the pin controller is strict can you assume that it's
use by something else than GPIO blocks the GPIO
usecase. Further explanations to this are detailed in
Documentation/driver-api/pinctl.rst section
"GPIO mode pitfalls".

Since BCM2835 which IIRC is your target does not set
.strict to true, you might first have to look into and solve that.

Yours,
Linus Walleij
Stefan Wahren Jan. 14, 2019, 3:53 p.m. UTC | #2
Hi Linus,

Am 14.01.19 um 14:30 schrieb Linus Walleij:
> On Sun, Jan 13, 2019 at 10:22 PM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
>> as proposed in a recent discussion [1], this is a hack to improve gpioinfo.
>> I think this could a start point for a discussion.
> I like the idea.
>
>> +bool pinmux_is_in_use(struct pinctrl_dev *pctldev, unsigned pin)
>> +{
>> +       struct pin_desc *desc = pin_desc_get(pctldev, pin);
>> +
>> +       if (!desc) {
>> +               dev_err(pctldev->dev,
>> +                       "pin %u is not registered so it cannot be requested\n",
>> +                       pin);
>> +               return false;
>> +       }
>> +
>> +       if (desc->mux_usecount)
>> +               return true;
>> +
>> +       return !!desc->gpio_owner;
>> +}
> This needs to be augmented to respect the .strict attribute of the
> pin.

sure

>
> Only if the pin controller is strict can you assume that it's
> use by something else than GPIO blocks the GPIO
> usecase. Further explanations to this are detailed in
> Documentation/driver-api/pinctl.rst section
> "GPIO mode pitfalls".
>
> Since BCM2835 which IIRC is your target does not set
> .strict to true, you might first have to look into and solve that.

According to the BCM2835 datasheet (GPIO Function Select Registers
GPFSELn on p.92) a pin can be configured as

0 = input
1 = output
2 = alternative function 5 (UART, PWM, ...)
3 = alternative function 4 (SPI, JTAG)
...

so it's not possible to have GPIO and a alternative function at the same
time. So in case bcm2835_pmx_gpio_set_direction is called the original
function get lost. From this i conclude strict should set to true.

BUT i must check that this doesn't break any BCM283x board.

Does this make sense to you?

Best regards
Stefan

[1] -
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

>
> Yours,
> Linus Walleij
Linus Walleij Jan. 21, 2019, 1:32 p.m. UTC | #3
On Mon, Jan 14, 2019 at 4:53 PM Stefan Wahren <stefan.wahren@i2se.com> wrote:

> > Only if the pin controller is strict can you assume that it's
> > use by something else than GPIO blocks the GPIO
> > usecase. Further explanations to this are detailed in
> > Documentation/driver-api/pinctl.rst section
> > "GPIO mode pitfalls".
> >
> > Since BCM2835 which IIRC is your target does not set
> > .strict to true, you might first have to look into and solve that.
>
> According to the BCM2835 datasheet (GPIO Function Select Registers
> GPFSELn on p.92) a pin can be configured as
>
> 0 = input
> 1 = output
> 2 = alternative function 5 (UART, PWM, ...)
> 3 = alternative function 4 (SPI, JTAG)
> ...
>
> so it's not possible to have GPIO and a alternative function at the same
> time. So in case bcm2835_pmx_gpio_set_direction is called the original
> function get lost. From this i conclude strict should set to true.
>
> BUT i must check that this doesn't break any BCM283x board.
>
> Does this make sense to you?

Yep. However I think (vague feeling) that people sometimes have
specified their pin muxes as non-strict for various reasons, like that
it may only work properly if the pinmux implements
.gpio_request_enable() etc as a shortcut for activating GPIO
lines.

The BCM2835 driver does not do that and instead has explicit
per-pin groups for each GPIO but curiously implements
.gpio_disable_free() so I don't know what the story is here.

This is because the pin control core must be able to distinguish
between GPIO uses and other random muxing to achieve
strictness (and the nicer debugfs that you want).

So if the driver is not using these callbacks, the pinmux
core needs some other way to infer if a pin is used as GPIO
or not.

Yours,
Linus Walleij
Stefan Wahren Jan. 21, 2019, 1:47 p.m. UTC | #4
Hi Linus,

Am 21.01.19 um 14:32 schrieb Linus Walleij:
> On Mon, Jan 14, 2019 at 4:53 PM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
>>> Only if the pin controller is strict can you assume that it's
>>> use by something else than GPIO blocks the GPIO
>>> usecase. Further explanations to this are detailed in
>>> Documentation/driver-api/pinctl.rst section
>>> "GPIO mode pitfalls".
>>>
>>> Since BCM2835 which IIRC is your target does not set
>>> .strict to true, you might first have to look into and solve that.
>> According to the BCM2835 datasheet (GPIO Function Select Registers
>> GPFSELn on p.92) a pin can be configured as
>>
>> 0 = input
>> 1 = output
>> 2 = alternative function 5 (UART, PWM, ...)
>> 3 = alternative function 4 (SPI, JTAG)
>> ...
>>
>> so it's not possible to have GPIO and a alternative function at the same
>> time. So in case bcm2835_pmx_gpio_set_direction is called the original
>> function get lost. From this i conclude strict should set to true.
>>
>> BUT i must check that this doesn't break any BCM283x board.
>>
>> Does this make sense to you?
> Yep. However I think (vague feeling) that people sometimes have
> specified their pin muxes as non-strict for various reasons, like that
> it may only work properly if the pinmux implements
> .gpio_request_enable() etc as a shortcut for activating GPIO
> lines.
>
> The BCM2835 driver does not do that and instead has explicit
> per-pin groups for each GPIO but curiously implements
> .gpio_disable_free() so I don't know what the story is here.

looking at the initial commit [1] reveal the following:

Upstreaming changes by Stephen Warren:
...

* Don't set GPIO_IN function select in gpio_request_enable() to avoid
glitches; defer this to gpio_set_direction(). Consequently, removed
empty bcm2835_pmx_gpio_request_enable().

So maybe this "side effect" wasn't intended.

[1] - https://patchwork.kernel.org/patch/1516561/

>
> This is because the pin control core must be able to distinguish
> between GPIO uses and other random muxing to achieve
> strictness (and the nicer debugfs that you want).
>
> So if the driver is not using these callbacks, the pinmux
> core needs some other way to infer if a pin is used as GPIO
> or not.
>
> Yours,
> Linus Walleij

Patch
diff mbox series

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..0a5bab2 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -71,6 +71,23 @@  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);
+
+	if (!desc) {
+		dev_err(pctldev->dev,
+			"pin %u is not registered so it cannot be requested\n",
+			pin);
+		return false;
+	}
+
+	if (desc->mux_usecount)
+		return true;
+
+	return !!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;