Re: [PATCH 1/2] gpio: bgpio: Teach gpio-generic about the pinctrl subsystem

[Michael van der Westhuizen <michael@xxxxxxxxxxxxxxxx>]

> On 20 Jul 2015, at 10:46 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> 
> On Fri, Jun 19, 2015 at 2:58 PM, Michael van der Westhuizen
> <michael@xxxxxxxxxxxxxxxx> wrote:
> 
>> The basic MMIO gpio driver is unaware of the pinctrl subsystem,
>> and this lack of pinctrl awareness extends to drivers using bgpio
>> unless the drivers handle pinctrl interaction by overriding the
>> request and free methods of gpiochip.
>> 
>> Since pinctrl consumer interfaces are stubbed when pinctrl is not
>> enabled this implementation is very simple, and there is no
>> behaviour change when pinctrl is not enabled.
>> 
>> Tested on picoXcell pc3x3 using gpio-dwapb.
>> 
>> Signed-off-by: Michael van der Westhuizen <michael@xxxxxxxxxxxxxxxx>
> 
>> static int bgpio_request(struct gpio_chip *chip, unsigned gpio_pin)
>> {
>>        if (gpio_pin < chip->ngpio)
>> -               return 0;
>> +               return pinctrl_request_gpio(chip->base + gpio_pin);
>> 
>>        return -EINVAL;
>> }
>> 
>> +static void bgpio_free(struct gpio_chip *chip, unsigned gpio_pin)
>> +{
>> +       if (gpio_pin < chip->ngpio)
>> +               pinctrl_free_gpio(chip->base + gpio_pin);
>> +}
> 
> I found a problem with this patch unfortunately.
> 
> If you have several MMIO controllers on the same system, some
> that use a backing pin controller and some that doesn't, this
> will not work, because all instances will call
> pinctrl_[request|free]_gpio() regardless if there is a pin
> controller behind it or not.
> 
> Can you please augment the driver to handle this... I don't
> know the best way for boardfiles, but I guess (see
> drivers/gpio/gpio-pl061.c for exampe):
> 
> - In struct bgpio_chip, introduce bool uses_pinctrl
> 
> - Assume no backing pin controller so this will be false
>  by default (propbably no code needed)
> 
> - Only call pinctrl_[request|free]_gpio if uses_pinctrl
>  is true
> 
> - Explicitly set it to true for drivers that have backing pin
>  control (like Jonas' system) or pass that flag from
>  platform data.
> 
> - A quirk like this for DT enabled systems:
>          if (of_property_read_bool(dev->of_node, "gpio-ranges"))
>                chip->uses_pinctrl = true;
> 
> I need to move the BGPIO header to include/linux/gpio too…

I will update the series.

Michael

> 
> Yours,
> Linus Walleij

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html