diff mbox series

[v2] ARM: sa1100: simpad: Correct I2C GPIO offsets

Message ID 20171107201857.5752-1-linus.walleij@linaro.org
State New
Headers show
Series [v2] ARM: sa1100: simpad: Correct I2C GPIO offsets | expand

Commit Message

Linus Walleij Nov. 7, 2017, 8:18 p.m. UTC 
Arnd reported the following build bug bug:

In file included from arch/arm/mach-sa1100/simpad.c:20:0:
arch/arm/mach-sa1100/include/mach/SA-1100.h:1118:18: error: large
integer implicitly truncated to unsigned type [-Werror=overflow]
                      (0x00000001 << (Nb))
                      ^
include/linux/gpio/machine.h:56:16: note: in definition of macro
'GPIO_LOOKUP_IDX'
.chip_hwnum = _chip_hwnum,
              ^~~~~~~~~~~
arch/arm/mach-sa1100/include/mach/SA-1100.h:1140:21: note: in
expansion of macro 'GPIO_GPIO'
                    ^~~~~~~~~
arch/arm/mach-sa1100/simpad.c:331:27: note: in expansion of
macro 'GPIO_GPIO21'
  GPIO_LOOKUP_IDX("gpio", GPIO_GPIO21, NULL, 0,

This is what happened:

commit b2e63555592f81331c8da3afaa607d8cf83e8138
"i2c: gpio: Convert to use descriptors"
commit 4d0ce62c0a02e41a65cfdcfe277f5be430edc371
"i2c: gpio: Augment all boardfiles to use open drain"
together uncovered an old bug in the Simpad board
file: as theGPIO_LOOKUP_IDX() encodes GPIO offsets
on gpiochips in an u16 (see <linux/gpio/machine.h>)
these GPIO "numbers" does not fit, since in
arch/arm/mach-sa1100/include/mach/SA-1100.h it is
defined as:

  #define GPIO_GPIO(Nb) (0x00000001 << (Nb))
  (...)
  #define GPIO_GPIO21 GPIO_GPIO(21) /* GPIO [21] */

This is however provably wrong, since the i2c-gpio
driver uses proper GPIO numbers, albeit earlier from
the global number space, whereas this GPIO_GPIO21
is the local line offset in the GPIO register, which
is used in other code but certainly not in the
gpiolib GPIO driver in drivers/gpio/gpio-sa1100.c, which
has code like this:

static void sa1100_gpio_set(struct gpio_chip *chip,
                            unsigned offset, int value)
{
    int reg = value ? R_GPSR : R_GPCR;

    writel_relaxed(BIT(offset),
        sa1100_gpio_chip(chip)->membase + reg);
}

So far everything however compiled fine as an unsigned
int was used to pass the GPIO numbers in
struct i2c_gpio_platform_data. We can trace the actual error
back to

commit dbd406f9d0a1d33a1303eb75cbe3f9435513d339
"ARM: 7025/1: simpad: add GPIO based device definitions."
This added the i2c_gpio with the wrong offsets.

This commit was before the SA1100 was converted to use
the gpiolib, but as can be seen from the contemporary
gpio.c in mach-sa1100, it was already using:

static int sa1100_gpio_get(struct gpio_chip *chip,
                           unsigned offset)
{
        return GPLR & GPIO_GPIO(offset);
}

And GPIO_GPIO() is essentially the BIT() macro.

Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Jochen Friedrich <jochen@scram.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v1->v2:
- Fix the commit message part that got stubbed out because
  of #define
- Correct Wolfram's email address.
- Put Arnd on CC.

Wolfram: this is in the i2c GPIO refactoring in your tree,
please apply it directly as a fix for v4.15 if there are
no protests.
Jochen: did this ever work? I suspect the patch was simply
developed on top of a different kernel.
---
 arch/arm/mach-sa1100/simpad.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.13.6

Comments

Wolfram Sang Nov. 7, 2017, 8:36 p.m. UTC | #1 
> Wolfram: this is in the i2c GPIO refactoring in your tree,

> please apply it directly as a fix for v4.15 if there are

> no protests.


OK, noted that this is for v4.15. Since it touches arch/arm, some ack
from an arm-maintainer would be good to have...
Arnd Bergmann Nov. 8, 2017, 10:54 a.m. UTC | #2 
On Tue, Nov 7, 2017 at 9:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c

> index 9db483a42826..7d4feb8a49ac 100644

> --- a/arch/arm/mach-sa1100/simpad.c

> +++ b/arch/arm/mach-sa1100/simpad.c

> @@ -328,9 +328,9 @@ static struct platform_device simpad_gpio_leds = {

>  static struct gpiod_lookup_table simpad_i2c_gpiod_table = {

>         .dev_id = "i2c-gpio",

>         .table = {

> -               GPIO_LOOKUP_IDX("gpio", GPIO_GPIO21, NULL, 0,

> +               GPIO_LOOKUP_IDX("gpio", 21, NULL, 0,

>                                 GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),

> -               GPIO_LOOKUP_IDX("gpio", GPIO_GPIO25, NULL, 1,

> +               GPIO_LOOKUP_IDX("gpio", 25, NULL, 1,

>                                 GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),

>         },

>  };


This looks like it's probably correct, but now I'm puzzled by another line in
the same file:

static struct gpio_keys_button simpad_button_table[] = {
        { KEY_POWER, IRQ_GPIO_POWER_BUTTON, 1, "power button" },
};

Here we pass a gpio number as well, but we use the macro for the IRQ
number that is apparently 32 higher than the gpio number. This came from the
same commit that introduced the GPIO_GPIO21 reference, so it may well
be broken, too.

I'm fine with your version getting merged as it certainly can't make things
worse, it fixes the build warning, and it probably fixes one thing that is
wrong today, but I'd also like to see someone figure these numbers out
properly, or maybe we should think about just removing the file if nobody
has noticed that it didn't work in the past six years.

       Arnd
Arnd Bergmann Nov. 8, 2017, 10:55 a.m. UTC | #3 
On Wed, Nov 8, 2017 at 11:54 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Nov 7, 2017 at 9:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>

>> diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c

>> index 9db483a42826..7d4feb8a49ac 100644

>> --- a/arch/arm/mach-sa1100/simpad.c

>> +++ b/arch/arm/mach-sa1100/simpad.c

>> @@ -328,9 +328,9 @@ static struct platform_device simpad_gpio_leds = {

>>  static struct gpiod_lookup_table simpad_i2c_gpiod_table = {

>>         .dev_id = "i2c-gpio",

>>         .table = {

>> -               GPIO_LOOKUP_IDX("gpio", GPIO_GPIO21, NULL, 0,

>> +               GPIO_LOOKUP_IDX("gpio", 21, NULL, 0,

>>                                 GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),

>> -               GPIO_LOOKUP_IDX("gpio", GPIO_GPIO25, NULL, 1,

>> +               GPIO_LOOKUP_IDX("gpio", 25, NULL, 1,

>>                                 GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),

>>         },

>>  };

>

> This looks like it's probably correct, but now I'm puzzled by another line in

> the same file:

>

> static struct gpio_keys_button simpad_button_table[] = {

>         { KEY_POWER, IRQ_GPIO_POWER_BUTTON, 1, "power button" },

> };

>

> Here we pass a gpio number as well, but we use the macro for the IRQ

> number that is apparently 32 higher than the gpio number. This came from the

> same commit that introduced the GPIO_GPIO21 reference, so it may well

> be broken, too.

>

> I'm fine with your version getting merged as it certainly can't make things

> worse, it fixes the build warning, and it probably fixes one thing that is

> wrong today


I also meant to say

Acked-by: Arnd Bergmann <arnd@arndb.de>


for this patch going through the i2c tree.
Wolfram Sang Nov. 10, 2017, 2:50 p.m. UTC | #4 
On Tue, Nov 07, 2017 at 09:18:57PM +0100, Linus Walleij wrote:
> Arnd reported the following build bug bug:

> 

> In file included from arch/arm/mach-sa1100/simpad.c:20:0:

> arch/arm/mach-sa1100/include/mach/SA-1100.h:1118:18: error: large

> integer implicitly truncated to unsigned type [-Werror=overflow]

>                       (0x00000001 << (Nb))

>                       ^

> include/linux/gpio/machine.h:56:16: note: in definition of macro

> 'GPIO_LOOKUP_IDX'

> .chip_hwnum = _chip_hwnum,

>               ^~~~~~~~~~~

> arch/arm/mach-sa1100/include/mach/SA-1100.h:1140:21: note: in

> expansion of macro 'GPIO_GPIO'

>                     ^~~~~~~~~

> arch/arm/mach-sa1100/simpad.c:331:27: note: in expansion of

> macro 'GPIO_GPIO21'

>   GPIO_LOOKUP_IDX("gpio", GPIO_GPIO21, NULL, 0,

> 

> This is what happened:

> 

> commit b2e63555592f81331c8da3afaa607d8cf83e8138

> "i2c: gpio: Convert to use descriptors"

> commit 4d0ce62c0a02e41a65cfdcfe277f5be430edc371

> "i2c: gpio: Augment all boardfiles to use open drain"

> together uncovered an old bug in the Simpad board

> file: as theGPIO_LOOKUP_IDX() encodes GPIO offsets

> on gpiochips in an u16 (see <linux/gpio/machine.h>)

> these GPIO "numbers" does not fit, since in

> arch/arm/mach-sa1100/include/mach/SA-1100.h it is

> defined as:

> 

>   #define GPIO_GPIO(Nb) (0x00000001 << (Nb))

>   (...)

>   #define GPIO_GPIO21 GPIO_GPIO(21) /* GPIO [21] */

> 

> This is however provably wrong, since the i2c-gpio

> driver uses proper GPIO numbers, albeit earlier from

> the global number space, whereas this GPIO_GPIO21

> is the local line offset in the GPIO register, which

> is used in other code but certainly not in the

> gpiolib GPIO driver in drivers/gpio/gpio-sa1100.c, which

> has code like this:

> 

> static void sa1100_gpio_set(struct gpio_chip *chip,

>                             unsigned offset, int value)

> {

>     int reg = value ? R_GPSR : R_GPCR;

> 

>     writel_relaxed(BIT(offset),

>         sa1100_gpio_chip(chip)->membase + reg);

> }

> 

> So far everything however compiled fine as an unsigned

> int was used to pass the GPIO numbers in

> struct i2c_gpio_platform_data. We can trace the actual error

> back to

> 

> commit dbd406f9d0a1d33a1303eb75cbe3f9435513d339

> "ARM: 7025/1: simpad: add GPIO based device definitions."

> This added the i2c_gpio with the wrong offsets.

> 

> This commit was before the SA1100 was converted to use

> the gpiolib, but as can be seen from the contemporary

> gpio.c in mach-sa1100, it was already using:

> 

> static int sa1100_gpio_get(struct gpio_chip *chip,

>                            unsigned offset)

> {

>         return GPLR & GPIO_GPIO(offset);

> }

> 

> And GPIO_GPIO() is essentially the BIT() macro.

> 

> Cc: Wolfram Sang <wsa@the-dreams.de>

> Cc: Jochen Friedrich <jochen@scram.de>

> Cc: linux-arm-kernel@lists.infradead.org

> Cc: Russell King <linux@arm.linux.org.uk>

> Cc: Arnd Bergmann <arnd@arndb.de>

> Reported-by: Arnd Bergmann <arnd@arndb.de>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Applied to for-next, thanks!

This from checkpatch makes sense to me:

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit b2e63555592f ("i2c: gpio: Convert to use descriptors")'
#21: 
commit b2e63555592f81331c8da3afaa607d8cf83e8138
Linus Walleij Nov. 10, 2017, 3:01 p.m. UTC | #5 
On Fri, Nov 10, 2017 at 3:50 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

> Applied to for-next, thanks!


Thanks man.

> This from checkpatch makes sense to me:

>

> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit b2e63555592f ("i2c: gpio: Convert to use descriptors")'

> #21:

> commit b2e63555592f81331c8da3afaa607d8cf83e8138


I never understood that thingie, I thought the check was
only there in order to stop Fixes: tags from running wild.
But feel free to cut it if you want.

Yours,
Linus Walleij
Arnd Bergmann Nov. 10, 2017, 3:17 p.m. UTC | #6 
On Fri, Nov 10, 2017 at 4:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Nov 10, 2017 at 3:50 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

>

>> Applied to for-next, thanks!

>

> Thanks man.

>

>> This from checkpatch makes sense to me:

>>

>> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit b2e63555592f ("i2c: gpio: Convert to use descriptors")'

>> #21:

>> commit b2e63555592f81331c8da3afaa607d8cf83e8138

>

> I never understood that thingie, I thought the check was

> only there in order to stop Fixes: tags from running wild.

> But feel free to cut it if you want.


I have this in my ~/.gitconfig:

[core]
        abbrev = 12
[alias]
        fixes = show --format='Fixes: %h (\"%s\")' -s

So 'git fixes b2e63555592f81331c8da3afaa607d8cf83e8138' produces
Fixes: b2e63555592f ("i2c: gpio: Convert to use descriptors")

which I then copy into a changelog as the last line, or use only the reference.

     Arnd
Linus Walleij Nov. 24, 2017, 10:04 a.m. UTC | #7 
On Fri, Nov 10, 2017 at 4:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> I have this in my ~/.gitconfig:

>

> [core]

>         abbrev = 12

> [alias]

>         fixes = show --format='Fixes: %h (\"%s\")' -s

>

> So 'git fixes b2e63555592f81331c8da3afaa607d8cf83e8138' produces

> Fixes: b2e63555592f ("i2c: gpio: Convert to use descriptors")

>

> which I then copy into a changelog as the last line, or use only the reference.


That's a neat trick.

I stole it :)

Yours,
Linus Walleij
Wolfram Sang Nov. 24, 2017, 12:36 p.m. UTC | #8 
On Fri, Nov 10, 2017 at 04:17:26PM +0100, Arnd Bergmann wrote:
> On Fri, Nov 10, 2017 at 4:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> > On Fri, Nov 10, 2017 at 3:50 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

> >

> >> Applied to for-next, thanks!

> >

> > Thanks man.

> >

> >> This from checkpatch makes sense to me:

> >>

> >> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit b2e63555592f ("i2c: gpio: Convert to use descriptors")'

> >> #21:

> >> commit b2e63555592f81331c8da3afaa607d8cf83e8138

> >

> > I never understood that thingie, I thought the check was

> > only there in order to stop Fixes: tags from running wild.

> > But feel free to cut it if you want.

> 

> I have this in my ~/.gitconfig:

> 

> [core]

>         abbrev = 12

> [alias]

>         fixes = show --format='Fixes: %h (\"%s\")' -s

> 

> So 'git fixes b2e63555592f81331c8da3afaa607d8cf83e8138' produces

> Fixes: b2e63555592f ("i2c: gpio: Convert to use descriptors")

> 

> which I then copy into a changelog as the last line, or use only the reference.


Kinda similar, since I have a white background, I use:

[alias]
lg = log --format='%C(yellow)%h %Cgreen(\"%Creset%s%Cgreen\")'

which is easy on my eye when reading 'git lg' but already has the proper
format when copy pasting its output around.
diff mbox series

Patch

diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c
index 9db483a42826..7d4feb8a49ac 100644
--- a/arch/arm/mach-sa1100/simpad.c
+++ b/arch/arm/mach-sa1100/simpad.c
@@ -328,9 +328,9 @@  static struct platform_device simpad_gpio_leds = {
 static struct gpiod_lookup_table simpad_i2c_gpiod_table = {
 	.dev_id = "i2c-gpio",
 	.table = {
-		GPIO_LOOKUP_IDX("gpio", GPIO_GPIO21, NULL, 0,
+		GPIO_LOOKUP_IDX("gpio", 21, NULL, 0,
 				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),
-		GPIO_LOOKUP_IDX("gpio", GPIO_GPIO25, NULL, 1,
+		GPIO_LOOKUP_IDX("gpio", 25, NULL, 1,
 				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),
 	},
 };