Fix basic_mmio_gpio shadow initialization
Subsystems (2)
cover Fix basic_mmio_gpio shadow initialization
I recently ran into a problem with gpio-generic driver on mxs platform. It turns out it's the exactly same problem that Lothar had run into on imx/mxc platform, which was fixed up in gpio-mxc driver as below.
commit fb1492186276ba52d99b58121b8a9a87f20cc9f3
Author: Lothar Wa?mann <[email protected]> Date: Thu Jul 7 14:50:16 2011 +0200
gpio/mxc: add missing initialization of basic_mmio_gpio shadow variables
The bgpio_init() function does not initialise the shadow register for the GPIO direction register. Thus, when configuring the first GPIO with gpio_set_direction() all other GPIOs of the same bank will be configured as inputs. Since the bgpio layer cannot know whether the register is readable, the initialisation should be done by the caller of bgpio_init().
Also, the 'data' shadow variable that is used inside basic_mmio_gpio to cache the current value of the GPIO_DR register is initialised from the GPIO_PSR register within bgpio_init(). Thus when setting the output value of a certain GPIO, the other GPIO outputs of the same bank will be set or cleared depending on the pin state of the GPIO inputs during bgpio_init().
I think it's a problem of gpio-generic rather than platform specific driver, so I'm coming up this series to have the problem fixed in gpio-generic driver itself and then removes the fixup in gpio-mxc. Thus, other platform gpio drivers based on gpio-generic do not have to suffer from the same problem and fix it up in individually over and over again.
Per suggestion from Lothar, I'm copying the maintainers of those platform gpio drivers using gpio-generic to inform the default behavior changing on gpio-generic. Please let me know if anyone gets impacted by the changes.
Regards,
Shawn
Shawn Guo (2):
gpio/generic: initialize basic_mmio_gpio shadow variables properly gpio/mxc: remove basic_mmio_gpio shadow fixup
drivers/gpio/gpio-generic.c | 4 ++++ drivers/gpio/gpio-mxc.c | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.5.4
PATCH 1/2 gpio/generic: initialize basic_mmio_gpio shadow variables properly
It fixes the issue in gpio-generic that commit fb14921 (gpio/mxc: add missing initialization of basic_mmio_gpio shadow variables) manged to fix in gpio-mxc driver, so that other platform specific drivers do not suffer from the same problem over and over again.
--- drivers/gpio/gpio-generic.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/gpio/gpio-generic.c +4 -0
@@ -394,6 +394,10 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,return ret; bgc->data = bgc->read_reg (bgc->reg_dat); +if (bgc->gc.set == bgpio_set_set) + bgc->data = bgc->read_reg (bgc->reg_set); +if (bgc->reg_dir) + bgc->dir = bgc->read_reg (bgc->reg_dir);return ret; }
Signature · 1 line
1.7.5.4
RE: [PATCH 1/2] gpio/generic: initialize basic_mmio_gpio shadow variables properly
Quoted 15 lines · Shawn Guo
It fixes the issue in gpio-generic that commit fb14921 (gpio/mxc: add missing initialization of
basic_mmio_gpioshadow variables) manged to fix in gpio-mxc driver, so that other platform specific drivers do not suffer from the same problem over and over again.Signed-off-by Shawn Guo <[email protected]>--- drivers/gpio/gpio-generic.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)drivers/gpio/gpio-generic.c +0
-0
drivers/gpio/gpio-generic.c int bgpio_init(struct bgpio_chip *bgc, struct device *dev, · line 403
Quoted 10 lines
drivers/gpio/gpio-generic.c +4
-0@@ -394,6 +394,10 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,return ret; bgc->data = bgc->read_reg (bgc->reg_dat); +if (bgc->gc.set == bgpio_set_set) + bgc->data = bgc->read_reg (bgc->reg_set); +if (bgc->reg_dir) + bgc->dir = bgc->read_reg (bgc->reg_dir);return ret; }
This change is fine for ep93xx.
Re: [PATCH 1/2] gpio/generic: initialize basic_mmio_gpio shadow variables properly
In reply to Shawn GuoHi Grant, Linus,
Can we manage to get it in with the upcoming merge window, as it fixes a bug that we may not want to live with for another cycle?
Regards,
Shawn
Quoted 28 lines · Shawn Guo
It fixes the issue in gpio-generic that commit fb14921 (gpio/mxc: add missing initialization of
basic_mmio_gpioshadow variables) manged to fix in gpio-mxc driver, so that other platform specific drivers do not suffer from the same problem over and over again.Signed-off-by Shawn Guo <[email protected]>--- drivers/gpio/gpio-generic.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)drivers/gpio/gpio-generic.c +4
-0@@ -394,6 +394,10 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,return ret; bgc->data = bgc->read_reg (bgc->reg_dat); +if (bgc->gc.set == bgpio_set_set) + bgc->data = bgc->read_reg (bgc->reg_set); +if (bgc->reg_dir) + bgc->dir = bgc->read_reg (bgc->reg_dir);return ret; }Signature · 1 line
1.7.5.4
PATCH 1/2 gpio/generic: initialize basic_mmio_gpio shadow variables properly
In reply to Shawn GuoQuoted 15 lines · Shawn Guo
It fixes the issue in gpio-generic that commit fb14921 (gpio/mxc: add missing initialization of
basic_mmio_gpioshadow variables) manged to fix in gpio-mxc driver, so that other platform specific drivers do not suffer from the same problem over and over again.Signed-off-by Shawn Guo <[email protected]>--- drivers/gpio/gpio-generic.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)drivers/gpio/gpio-generic.c +0
-0
drivers/gpio/gpio-generic.c int bgpio_init(struct bgpio_chip *bgc, struct device *dev, · line 400
Quoted 7 lines
drivers/gpio/gpio-generic.c +4
-0@@ -394,6 +394,10 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,return ret; bgc->data = bgc->read_reg (bgc->reg_dat); +if (bgc->gc.set == bgpio_set_set) + bgc->data = bgc->read_reg (bgc->reg_set); +if (bgc->reg_dir) + bgc->dir = bgc->read_reg (bgc->reg_dir);
This assumes that the set and dir registers are actually readable which isn't the case on some hardware. There needs to be a mechanism for drivers using bgpio_init to control how data & dir are initialized (possibly with some flags; maybe replace the big_endian arg with a flags arg).
g.
Re: [PATCH 1/2] gpio/generic: initialize basic_mmio_gpio shadow variables properly
Quoted 18 lines · Grant Likely
Quoted 17 lines · Shawn Guo
It fixes the issue in gpio-generic that commit fb14921 (gpio/mxc: add missing initialization of
basic_mmio_gpioshadow variables) manged to fix in gpio-mxc driver, so that other platform specific drivers do not suffer from the same problem over and over again.Signed-off-by Shawn Guo <[email protected]>---
?drivers/gpio/gpio-generic.c | ? ?4 ++++
?1 files changed, 4 insertions(+), 0 deletions(-)drivers/gpio/gpio-generic.c +0
-0@@ -394,6 +394,10 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev, ? ? ? ? ? ? ? return ret;
Quoted 11 lines
drivers/gpio/gpio-generic.c int bgpio_init(struct bgpio_chip *bgc, struct device *dev, · line 400
Quoted 5 lines
drivers/gpio/gpio-generic.c +4
-0? ? ? bgc->data = bgc->read_reg(bgc->reg_dat); + ? ? if (bgc->gc.set == bgpio_set_set) + ? ? ? ? ? ? bgc->data = bgc->read_reg(bgc->reg_set); + ? ? if (bgc->reg_dir) + ? ? ? ? ? ? bgc->dir = bgc->read_reg(bgc->reg_dir);This assumes that the set and dir registers are actually readable which isn't the case on some hardware. ?There needs to be a mechanism for drivers using
bgpio_initto control how data & dir are initialized (possibly with some flags; maybe replace thebig_endianarg with a flags arg).
If you can turn it around quickly, I'll still try to get it in for v3.5
g.
PATCH 2/2 gpio/mxc: remove basic_mmio_gpio shadow fixup
In reply to Shawn GuoWith commit 7cc9c7c (gpio/generic: initialize basic_mmio_gpio shadow variables properly) fixing the basic_mmio_gpio shadow variables in gpio-generic, the fixup in gpio-mxc can be removed now.
--- drivers/gpio/gpio-mxc.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/gpio/gpio-mxc.c +0 -2
@@ -423,8 +423,6 @@ static int __devinit mxc_gpio_probe(struct platform_device *pdev) port->bgc.gc.to_irq = mxc_gpio_to_irq; port->bgc.gc.base = pdev->id *32 ; - port->bgc.dir = port->bgc.read_reg (port->bgc.reg_dir); - port->bgc.data = port->bgc.read_reg (port->bgc.reg_set); err =gpiochip_add (&port->bgc.gc);if (err)
Signature · 1 line
1.7.5.4