Re: [PATCH v2] gpiolib: Take MUX usage into account

[Stefan Wahren]

Am 13.08.19 um 08:10 schrieb Fried, Ramon:
>
> On 8/13/2019 08:38, Stefan Wahren wrote:
>> Hi Ramon,
>>
>> On 13.08.19 03:42, Ramon Fried wrote:
>>> From: Stefan Wahren <stefan.wahren@xxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>>> Tested-by: Ramon Fried <rfried.dev@xxxxxxxxx>
>>> Signed-off-by: Ramon Fried <rfried.dev@xxxxxxxxx>
>>> ---
>>> v2: Address review from linus:
>>> * ** Please notive logic was reversed **
>>> * renamed pinctrl_gpio_is_in_use() to pinctrl_gpio_can_use_line()
>>> * renamed pinmux_is_in_use() to pinmux_can_be_used_for_gpio()
>>> * changed dev_err to dev_dbg (Linus suggested removing it altogether, I
>>> ÂÂ find it better to keep it for debug).
>> thanks for taking care of this.
>>> Â drivers/gpio/gpiolib.cÂÂÂÂÂÂÂÂÂÂ |Â 3 ++-
>>> Â drivers/pinctrl/core.cÂÂÂÂÂÂÂÂÂÂ | 28 ++++++++++++++++++++++++++++
>>> Â drivers/pinctrl/pinmux.cÂÂÂÂÂÂÂÂ | 27 +++++++++++++++++++++++++++
>>> Â drivers/pinctrl/pinmux.hÂÂÂÂÂÂÂÂ |Â 8 ++++++++
>>> Â include/linux/pinctrl/consumer.h |Â 6 ++++++
>>> Â 5 files changed, 71 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index f497003f119c..52937bf8e514 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -1084,7 +1084,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_can_use_line(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 b70df27874d1..2bbd8ee93507 100644
>>> --- a/drivers/pinctrl/core.c
>>> +++ b/drivers/pinctrl/core.c
>>> @@ -736,6 +736,34 @@ int pinctrl_get_group_selector(struct
>>> pinctrl_dev *pctldev,
>>> ÂÂÂÂÂ return -EINVAL;
>>> Â }
>>> Â +bool pinctrl_gpio_can_use_line(unsigned gpio)
>>> +{
>>> +ÂÂÂ struct pinctrl_dev *pctldev;
>>> +ÂÂÂ struct pinctrl_gpio_range *range;
>>> +ÂÂÂ bool result;
>>> +ÂÂÂ int pin;
>>> +
>>> +ÂÂÂ /*
>>> +ÂÂÂÂ * Try to obtain GPIO range, if it fails
>>> +ÂÂÂÂ * we're probably dealing with GPIO driver
>>> +ÂÂÂÂ * without a backing pin controller - bail out.
>>> +ÂÂÂÂ */
>>> +ÂÂÂ if (pinctrl_get_device_gpio_range(gpio, &pctldev, &range))
>>> +ÂÂÂÂÂÂÂ return true;
>>> +
>>> +ÂÂÂ mutex_lock(&pctldev->mutex);
>>> +
>>> +ÂÂÂ /* Convert to the pin controllers number space */
>>> +ÂÂÂ pin = gpio_to_pin(range, gpio);
>>> +
>>> +ÂÂÂ result = pinmux_can_be_used_for_gpio(pctldev, pin);
>>> +
>>> +ÂÂÂ mutex_unlock(&pctldev->mutex);
>>> +
>>> +ÂÂÂ return result;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pinctrl_gpio_can_use_line);
>>> +
>>> Â /**
>>> ÂÂ * 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 020e54f843f9..7e42a5738d82 100644
>>> --- a/drivers/pinctrl/pinmux.c
>>> +++ b/drivers/pinctrl/pinmux.c
>>> @@ -70,6 +70,33 @@ int pinmux_validate_map(const struct pinctrl_map
>>> *map, int i)
>>> ÂÂÂÂÂ return 0;
>>> Â }
>>> Â +/**
>>> + * pinmux_can_be_used_for_gpio() - check if a specific pin
>>> + *ÂÂÂ is either muxed to a different function or used as gpio.
>>> + *
>>> + * @pin: the pin number in the global pin space
>>> + *
>>> + * Controllers not defined as strict will always return true,
>>> + * menaning that the gpio can be used.
>>> + */
>>> +bool pinmux_can_be_used_for_gpio(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_dbg(pctldev->dev,
>>> +ÂÂÂÂÂÂÂÂÂÂÂ "pin %u is not registered so it cannot be requested\n",
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pin);
>>> +ÂÂÂÂÂÂÂ return true;
>> This return value looks strange to me.
>
> Basically, it's just the reversed return value you returned in the
> original patch,
> It means in this context that if the pin is not owned by a
> pin-controller it can be used for GPIO.
As long as the provided pin is valid. Btw shouldn't we change the logic
in the debug message?
>
> Thanks,
> Ramon.
>
>>
>> Stefan
>>