| Message ID | 1547414494-8556-1-git-send-email-stefan.wahren@i2se.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] gpiolib: Take MUX usage into account | expand |
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
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
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
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
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;
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