gpiolib: Don't allow drivers to specify a base with DT

Message ID 1406808468-12559-1-git-send-email-broonie@kernel.org
State New
Headers show

Commit Message

Mark Brown July 31, 2014, 12:07 p.m.
From: Mark Brown <broonie@linaro.org>

DT based systems should have no reason to use fixed GPIO numbers but some
drivers that work on both DT and non-DT platforms specify them anyway. In
order to improve robustness in cases where drivers use gpio_is_valid() to
check for a valid GPIO on data initialized to zero as a default and avoid
bugs due to assuptions about fixed numbers creeping in ignore any specified
base when DT is in use.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexandre Courbot Aug. 4, 2014, 4:21 a.m. | #1
On Thu, Jul 31, 2014 at 9:07 PM, Mark Brown <broonie@kernel.org> wrote:
> From: Mark Brown <broonie@linaro.org>
>
> DT based systems should have no reason to use fixed GPIO numbers but some
> drivers that work on both DT and non-DT platforms specify them anyway. In
> order to improve robustness in cases where drivers use gpio_is_valid() to
> check for a valid GPIO on data initialized to zero as a default and avoid
> bugs due to assuptions about fixed numbers creeping in ignore any specified
> base when DT is in use.

I agree that DT users should not use the base number at all - but the
fact is some of them are doing it. Aren't we going to break some
user-space users that will expect to find a GPIO under a given number?

Also, how is this going to help with gpio_is_valid() against
zero-initialized data?

>
> Signed-off-by: Mark Brown <broonie@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 768f0831db18..11d3cf1cbca7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -234,7 +234,7 @@ int gpiochip_add(struct gpio_chip *chip)
>
>         spin_lock_irqsave(&gpio_lock, flags);
>
> -       if (base < 0) {
> +       if (base < 0 || of_have_populated_dt()) {
>                 base = gpiochip_find_base(chip->ngpio);
>                 if (base < 0) {
>                         status = base;
> --
> 2.0.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 11, 2014, 7:26 a.m. | #2
On Thu, Jul 31, 2014 at 2:07 PM, Mark Brown <broonie@kernel.org> wrote:

> From: Mark Brown <broonie@linaro.org>
>
> DT based systems should have no reason to use fixed GPIO numbers but some
> drivers that work on both DT and non-DT platforms specify them anyway. In
> order to improve robustness in cases where drivers use gpio_is_valid() to
> check for a valid GPIO on data initialized to zero as a default and avoid
> bugs due to assuptions about fixed numbers creeping in ignore any specified
> base when DT is in use.

Hm in principle you are right...

> -       if (base < 0) {
> +       if (base < 0 || of_have_populated_dt()) {
>                 base = gpiochip_find_base(chip->ngpio);

But here I worry about breaking in-transition systems, e.g.
defining part of the peripherals through DT but adding a
GPIO device with AUXDATA, setting up base that way.

Not that it should stay that way for sure, but it does need
some consideration...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Aug. 11, 2014, 12:31 p.m. | #3
On Mon, Aug 11, 2014 at 09:26:49AM +0200, Linus Walleij wrote:
> On Thu, Jul 31, 2014 at 2:07 PM, Mark Brown <broonie@kernel.org> wrote:

> > -       if (base < 0) {
> > +       if (base < 0 || of_have_populated_dt()) {
> >                 base = gpiochip_find_base(chip->ngpio);

> But here I worry about breaking in-transition systems, e.g.
> defining part of the peripherals through DT but adding a
> GPIO device with AUXDATA, setting up base that way.

> Not that it should stay that way for sure, but it does need
> some consideration...

Could we add a flag that the boards can use to override this behaviour?
Mark Brown Aug. 11, 2014, 12:38 p.m. | #4
On Mon, Aug 04, 2014 at 01:21:57PM +0900, Alexandre Courbot wrote:
> On Thu, Jul 31, 2014 at 9:07 PM, Mark Brown <broonie@kernel.org> wrote:

> > DT based systems should have no reason to use fixed GPIO numbers but some
> > drivers that work on both DT and non-DT platforms specify them anyway. In
> > order to improve robustness in cases where drivers use gpio_is_valid() to
> > check for a valid GPIO on data initialized to zero as a default and avoid
> > bugs due to assuptions about fixed numbers creeping in ignore any specified
> > base when DT is in use.

> I agree that DT users should not use the base number at all - but the
> fact is some of them are doing it. Aren't we going to break some
> user-space users that will expect to find a GPIO under a given number?

Is this actually a realistic thing that people do with mainline - I'm
not convinced that it's going to be robust in general (a lot of DT
conversions will have broken things for entire architectures) and hard
coded numbers aren't the sort of things that make it into distros.

> Also, how is this going to help with gpio_is_valid() against
> zero-initialized data?

I should have said check for zero instead, sorry - though making 0 not
be a valid GPIO would help overall.

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 768f0831db18..11d3cf1cbca7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -234,7 +234,7 @@  int gpiochip_add(struct gpio_chip *chip)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (base < 0) {
+	if (base < 0 || of_have_populated_dt()) {
 		base = gpiochip_find_base(chip->ngpio);
 		if (base < 0) {
 			status = base;