| Message ID | 1359825953-15663-4-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > gpiochip_find_base() always tries to find valid gpio with descend order. > It's inconvient if gpio information is passing from DTS. Now try to find > valid gpio with ascend order. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> This is more scary stuff. As you know GPIO numbers are exposed to userspace. Systems with this change risk having their dynamically added GPIO controller enumerated in a different fashion. And userspace clients may be relying on these numbers. And we do not break userspace. I know this is not elegant but I'm afraid the descending search needs to be kept for compatibibility reasons. BTW: please CC Grant likely on all patches. Yours, Linus Walleij
On 6 February 2013 01:14, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: > >> gpiochip_find_base() always tries to find valid gpio with descend order. >> It's inconvient if gpio information is passing from DTS. Now try to find >> valid gpio with ascend order. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > > This is more scary stuff. > > As you know GPIO numbers are exposed to userspace. > > Systems with this change risk having their dynamically added > GPIO controller enumerated in a different fashion. And > userspace clients may be relying on these numbers. > > And we do not break userspace. > > I know this is not elegant but I'm afraid the descending search > needs to be kept for compatibibility reasons. > > BTW: please CC Grant likely on all patches. > > Yours, > Linus Walleij But descending search isn't good for reading. I try to allocate all gpio numbers in Hi3620 from gpiochip_find_base(). If it's descending search, GPIO0~7 is mapped to gpio248~255; GPIO8~GPIO15 is mapped to gpio240~gpio247. It's not easy to read, and it breaks the knowledge of gpio number on schematic & datasheet. Unless we don't use allocating gpio numbers dynamically and add a common property to parse gpio base of each chip in DTS file. It's also OK to me add a common property. Regards Haojian
On 02/06/2013 10:59 AM, Haojian Zhuang wrote: > On 6 February 2013 01:14, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang >> <haojian.zhuang@linaro.org> wrote: >> >>> gpiochip_find_base() always tries to find valid gpio with descend order. >>> It's inconvient if gpio information is passing from DTS. Now try to find >>> valid gpio with ascend order. GPIOs in the device tree are typically referenced by a phandle which include the controller and relative HW number. Are there cases where we must use absolute GPIO numbers in the DT? >>> >>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> >> >> This is more scary stuff. >> >> As you know GPIO numbers are exposed to userspace. >> >> Systems with this change risk having their dynamically added >> GPIO controller enumerated in a different fashion. And >> userspace clients may be relying on these numbers. >> >> And we do not break userspace. >> >> I know this is not elegant but I'm afraid the descending search >> needs to be kept for compatibibility reasons. >> >> BTW: please CC Grant likely on all patches. >> >> Yours, >> Linus Walleij > > But descending search isn't good for reading. > > I try to allocate all gpio numbers in Hi3620 from gpiochip_find_base(). > If it's descending search, GPIO0~7 is mapped to gpio248~255; > GPIO8~GPIO15 is mapped to gpio240~gpio247. It's not easy to read, > and it breaks the knowledge of gpio number on schematic & datasheet. > > Unless we don't use allocating gpio numbers dynamically and add > a common property to parse gpio base of each chip in DTS file. > It's also OK to me add a common property. There is also one more problem with this reordering: if a GPIO chip with a base GPIO set gets probed *after* a bunch of chips without a proper base, its range in the number space is likely to have been stolen by one of the "dynamic" chips. Ideally all chips would come with a base GPIO, but we cannot rule out hotplugable interfaces. Even more ideally the integer numberspace would go away altogether with the new gpiod interface and the sysfs interface would be replaced with one where exported GPIOs would be under their chip node, and referenced by their hw number. But that would break userspace even more. Or maybe there could be a config option to choose between the "legacy" integer-space user interface and this new scheme. Eventually, the number space could be deprecated. Alex.
On 6 February 2013 12:33, Alex Courbot <acourbot@nvidia.com> wrote: > On 02/06/2013 10:59 AM, Haojian Zhuang wrote: >> >> On 6 February 2013 01:14, Linus Walleij <linus.walleij@linaro.org> wrote: >>> >>> On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang >>> <haojian.zhuang@linaro.org> wrote: >>> >>>> gpiochip_find_base() always tries to find valid gpio with descend order. >>>> It's inconvient if gpio information is passing from DTS. Now try to find >>>> valid gpio with ascend order. > > > GPIOs in the device tree are typically referenced by a phandle which include > the controller and relative HW number. Are there cases where we must use > absolute GPIO numbers in the DT? > At first, you only consider SoC GPIO pins are in the same GPIO controllers. ARM pl061 is a gpio controller with only 8 GPIO pins. So there're a lot of pl061 controllers in one SoC. In the second, all GPIO pins in one SoC are counter by ascending order. It's also same in the schematic. And there's sysfs interface for GPIO debugging in kernel. With the descending order, gpio248 is real GPIO0 in SoC. It's crazy for develop debugging. > >>>> >>>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> >>> >>> >>> This is more scary stuff. >>> >>> As you know GPIO numbers are exposed to userspace. >>> >>> Systems with this change risk having their dynamically added >>> GPIO controller enumerated in a different fashion. And >>> userspace clients may be relying on these numbers. >>> >>> And we do not break userspace. >>> >>> I know this is not elegant but I'm afraid the descending search >>> needs to be kept for compatibibility reasons. >>> >>> BTW: please CC Grant likely on all patches. >>> >>> Yours, >>> Linus Walleij >> >> >> But descending search isn't good for reading. >> >> I try to allocate all gpio numbers in Hi3620 from gpiochip_find_base(). >> If it's descending search, GPIO0~7 is mapped to gpio248~255; >> GPIO8~GPIO15 is mapped to gpio240~gpio247. It's not easy to read, >> and it breaks the knowledge of gpio number on schematic & datasheet. >> >> Unless we don't use allocating gpio numbers dynamically and add >> a common property to parse gpio base of each chip in DTS file. >> It's also OK to me add a common property. > > > There is also one more problem with this reordering: if a GPIO chip with a > base GPIO set gets probed *after* a bunch of chips without a proper base, > its range in the number space is likely to have been stolen by one of the > "dynamic" chips. > > Ideally all chips would come with a base GPIO, but we cannot rule out > hotplugable interfaces. Even more ideally the integer numberspace would go > away altogether with the new gpiod interface and the sysfs interface would > be replaced with one where exported GPIOs would be under their chip node, > and referenced by their hw number. But that would break userspace even more. > Or maybe there could be a config option to choose between the "legacy" > integer-space user interface and this new scheme. Eventually, the number > space could be deprecated. > > Alex. > How about adding one property on declaring that the GPIO number space is in ascending order? Then it won't break the "legacy" decreasing order. Regards Haojian
On Wed, Feb 6, 2013 at 2:59 AM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > On 6 February 2013 01:14, Linus Walleij <linus.walleij@linaro.org> wrote: >> This is more scary stuff. >> >> As you know GPIO numbers are exposed to userspace. >> >> Systems with this change risk having their dynamically added >> GPIO controller enumerated in a different fashion. And >> userspace clients may be relying on these numbers. >> >> And we do not break userspace. >> >> I know this is not elegant but I'm afraid the descending search >> needs to be kept for compatibibility reasons. >> >> BTW: please CC Grant likely on all patches. >> >> Yours, >> Linus Walleij > > But descending search isn't good for reading. But you may be breaking userspace. When I, as a subsystem maintainer merge a patch that break userspace interfaces, things like this happen: https://lkml.org/lkml/2012/12/23/75 http://developers.slashdot.org/story/12/12/29/018234/linus-chews-up-kernel-maintainer-for-introducing-userspace-bug You can argue all you want about wanting to change things that affect userspace for internal kernel refactoring or fit with device tree or whatever, it's just not going to happen, because the Big Penguin has installed a culture of fear around breaking userspace. If you want the policy changed you can talk to Torvalds. > I try to allocate all gpio numbers in Hi3620 from gpiochip_find_base(). > If it's descending search, GPIO0~7 is mapped to gpio248~255; > GPIO8~GPIO15 is mapped to gpio240~gpio247. It's not easy to read, > and it breaks the knowledge of gpio number on schematic & datasheet. It may make things elegant and nice on your (new) system but break everyone else's, and they were first in the kernel, they may have userspace clients and so, we cannot change this. > Unless we don't use allocating gpio numbers dynamically and add > a common property to parse gpio base of each chip in DTS file. > It's also OK to me add a common property. As explained elsewhere, global GPIO numbers don't belong in the device tree, as it is a Linux-specific pecularity. If this approach was chosen anyway, it would be named something like linux,gpio-base-offset One compromise would be to add global setting like gpio_add_dynamic_gpios_ascendingly() that will change the behaviour on a *specific* system, or maybe on all device tree systems, and keep both code paths. Yes, it is ugly and unelegant, but with the userspace contract, what can we do? We do all sort of ugliness for userspace. After reading this you may be on the clear why I am so hesitant about Roland Stigge's blocked GPIOs as well, that will become one more userspace ABI set in stone FOREVER. I'd like Grant's input on this... he has the big view on GPIO plus device tree. Yours, Linus Walleij
On 6 February 2013 16:44, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Feb 6, 2013 at 2:59 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 6 February 2013 01:14, Linus Walleij <linus.walleij@linaro.org> wrote: > >>> This is more scary stuff. >>> >>> As you know GPIO numbers are exposed to userspace. >>> >>> Systems with this change risk having their dynamically added >>> GPIO controller enumerated in a different fashion. And >>> userspace clients may be relying on these numbers. >>> >>> And we do not break userspace. >>> >>> I know this is not elegant but I'm afraid the descending search >>> needs to be kept for compatibibility reasons. >>> >>> BTW: please CC Grant likely on all patches. >>> >>> Yours, >>> Linus Walleij >> >> But descending search isn't good for reading. > > But you may be breaking userspace. > > When I, as a subsystem maintainer merge a patch that break > userspace interfaces, things like this happen: > > https://lkml.org/lkml/2012/12/23/75 > http://developers.slashdot.org/story/12/12/29/018234/linus-chews-up-kernel-maintainer-for-introducing-userspace-bug > > You can argue all you want about wanting to change things > that affect userspace for internal kernel refactoring or fit > with device tree or whatever, it's just not going to happen, > because the Big Penguin has installed a culture of fear > around breaking userspace. > > If you want the policy changed you can talk to Torvalds. > >> I try to allocate all gpio numbers in Hi3620 from gpiochip_find_base(). >> If it's descending search, GPIO0~7 is mapped to gpio248~255; >> GPIO8~GPIO15 is mapped to gpio240~gpio247. It's not easy to read, >> and it breaks the knowledge of gpio number on schematic & datasheet. > > It may make things elegant and nice on your (new) system but > break everyone else's, and they were first in the kernel, they may > have userspace clients and so, we cannot change this. > >> Unless we don't use allocating gpio numbers dynamically and add >> a common property to parse gpio base of each chip in DTS file. >> It's also OK to me add a common property. > > As explained elsewhere, global GPIO numbers don't belong > in the device tree, as it is a Linux-specific pecularity. > If this approach was chosen anyway, it would be named > something like linux,gpio-base-offset > > One compromise would be to add global setting like > gpio_add_dynamic_gpios_ascendingly() that will change > the behaviour on a *specific* system, or maybe on all > device tree systems, and keep both code paths. > Yes, it is ugly and unelegant, but with the userspace > contract, what can we do? We do all sort of ugliness > for userspace. > > After reading this you may be on the clear why I am so > hesitant about Roland Stigge's blocked GPIOs as well, > that will become one more userspace ABI set in stone > FOREVER. > > I'd like Grant's input on this... he has the big view on > GPIO plus device tree. > > Yours, > Linus Walleij Since it may break the userspace ABI, I agree that we shouldn't change current solution. Thanks for your kindly illustration. In this patch series, I'll initialize pdata->gpio_base first and use aux structure in machine driver. Then I'll try to something like "linux,gpio-base-offset" in GPIO system, and drop aux structure from machine driver. Since I'm expecting GPIO/PINCTRL could be similar as IRQ that everything could be parsed from DT, it could make driver simpler. Best Regards Haojian
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 199fca1..8af57e7 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -128,20 +128,21 @@ static int gpiochip_find_base(int ngpio) int spare = 0; int base = -ENOSPC; - for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) { + for (i = 0, base = 0; i < ARCH_NR_GPIOS; i++) { struct gpio_desc *desc = &gpio_desc[i]; struct gpio_chip *chip = desc->chip; - if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)) { + if (chip) { + spare = 0; + i += chip->ngpio - 1; + base = i + 1; + } else if (test_bit(FLAG_RESERVED, &desc->flags)) { + spare = 0; + base = i + 1; + } else { spare++; - if (spare == ngpio) { - base = i; + if (spare == ngpio) break; - } - } else { - spare = 0; - if (chip) - i -= chip->ngpio - 1; } }
gpiochip_find_base() always tries to find valid gpio with descend order. It's inconvient if gpio information is passing from DTS. Now try to find valid gpio with ascend order. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- drivers/gpio/gpiolib.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)