[PATCH] gpio/generic: initialize the shadow of direction register

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

Same as what the driver does for data register shadow bgc->data,
the bgpio_init should also initialize direction register shadow
bgc->dir to preserve the existing setting.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpio/gpio-generic.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index e38dd0c..53222c7 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -394,6 +394,7 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
 		return ret;
 
 	bgc->data = bgc->read_reg(bgc->reg_dat);
+	bgc->dir = bgc->read_reg(bgc->reg_dir);
 
 	return ret;
 }
-- 
1.7.4.1

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> Same as what the driver does for data register shadow bgc->data,
> the bgpio_init should also initialize direction register shadow
> bgc->dir to preserve the existing setting.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpio/gpio-generic.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> index e38dd0c..53222c7 100644
> --- a/drivers/gpio/gpio-generic.c
> +++ b/drivers/gpio/gpio-generic.c
> @@ -394,6 +394,7 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
>  		return ret;
>  
>  	bgc->data = bgc->read_reg(bgc->reg_dat);
> +	bgc->dir = bgc->read_reg(bgc->reg_dir);
>  
This will blow up in your face when calling bgpio_init() without any
direction register (which is well supported by the generic driver).

You need to do this in the platform specific driver that uses the
generic driver. See gpio-mxc.c for reference:
|        err = bgpio_init(&port->bgc, &pdev->dev, 4,
|                         port->base + GPIO_PSR,
|                         port->base + GPIO_DR, NULL,
|                         port->base + GPIO_GDIR, NULL, false);
|        if (err)
|                goto out_iounmap;
|
|        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);


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Tue, May 15, 2012 at 09:17:50AM +0200, Lothar Wa?mann wrote:
> This will blow up in your face when calling bgpio_init() without any
> direction register (which is well supported by the generic driver).
> 
> You need to do this in the platform specific driver that uses the
> generic driver. See gpio-mxc.c for reference:
> |        err = bgpio_init(&port->bgc, &pdev->dev, 4,
> |                         port->base + GPIO_PSR,
> |                         port->base + GPIO_DR, NULL,
> |                         port->base + GPIO_GDIR, NULL, false);
> |        if (err)
> |                goto out_iounmap;
> |
> |        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);
> 
Ah, thanks for the info.  I did not know that we ran into the issue
on gpio-mxc and had it fixed there.  But instead of asking every
single platform gpio driver to do the fixup, we should really fix it
in generic driver.  Imaging if you fixed the issue in gpio-generic
when you ran into the problem on MXC/IMX, I wouldn't have spent half
day to track the issue down on MXS :)

So back to the right fixing, does the following one look good to you?

@@ -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;
 }

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Tue, May 15, 2012 at 09:17:50AM +0200, Lothar Wa?mann wrote:
> > This will blow up in your face when calling bgpio_init() without any
> > direction register (which is well supported by the generic driver).
> > 
> > You need to do this in the platform specific driver that uses the
> > generic driver. See gpio-mxc.c for reference:
> > |        err = bgpio_init(&port->bgc, &pdev->dev, 4,
> > |                         port->base + GPIO_PSR,
> > |                         port->base + GPIO_DR, NULL,
> > |                         port->base + GPIO_GDIR, NULL, false);
> > |        if (err)
> > |                goto out_iounmap;
> > |
> > |        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);
> > 
> Ah, thanks for the info.  I did not know that we ran into the issue
> on gpio-mxc and had it fixed there.  But instead of asking every
> single platform gpio driver to do the fixup, we should really fix it
> in generic driver.  Imaging if you fixed the issue in gpio-generic
> when you ran into the problem on MXC/IMX, I wouldn't have spent half
> day to track the issue down on MXS :)
> 
> So back to the right fixing, does the following one look good to you?
> 
> @@ -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;
>  }
> 
That might change the behaviour of existing implementations.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Wed, May 16, 2012 at 08:53:42AM +0200, Lothar Wa?mann wrote:
> > @@ -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;
> >  }
> > 
> That might change the behaviour of existing implementations.
> 
Can you be specific?  I do not quite follow on that.

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Shawn Guo writes:
> On Wed, May 16, 2012 at 08:53:42AM +0200, Lothar Wa?mann wrote:
> > > @@ -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;
> > >  }
> > > 
> > That might change the behaviour of existing implementations.
> > 
> Can you be specific?  I do not quite follow on that.
> 
You initialize a variable to a specific value that defaulted to zero
before.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On 16 May 2012 15:34, Lothar Wa?mann <LW@karo-electronics.de> wrote:
> You initialize a variable to a specific value that defaulted to zero
> before.
>
The variable defaulted to zero is causing a problem.  I'm fixing that problem.

Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On 16 May 2012 15:34, Lothar Wa?mann <LW@karo-electronics.de> wrote:
> > You initialize a variable to a specific value that defaulted to zero
> > before.
> >
> The variable defaulted to zero is causing a problem.  I'm fixing that problem.
> 
But you are changing the behaviour of existing software that
apparently did not suffer from this problem. You should make sure that
the fix for your problem is not causing new problems for other users!


Lothar Wa?amnn
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Wed, May 16, 2012 at 02:48:23PM +0200, Lothar Wa?mann wrote:
> > But you are changing the behaviour of existing software that
> > apparently did not suffer from this problem.
> 
> It's exactly the reason why you chose to fix the problem in gpio-mxc
> instead of gpio-generic, right?  You thought other gpio-generic users
> like gpio-mxs does not suffer from this problem.  Sadly, it's not the
> case.
> 
> The updated patch simply corrects behaviour of gpio-generic for all
> the users.
> 
> > You should make sure that
> > the fix for your problem is not causing new problems for other users!
> > 
> I'm not fixing a gpio-mxs problem but a gpio-generic one.  Can you help
>
Yes, I know. And that's exaclty why you should make sure that you
don't break other platforms by fixing yours!

> me understand what the new problems could possibly be introduced for
> other gpio-generic users by the changes? 
> 
Another platform using might be working just _because_ of the "broken"
behaviour. By fixing that brokenness for your driver you may break
that other platform.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Wed, May 16, 2012 at 02:48:23PM +0200, Lothar Wa?mann wrote:
> But you are changing the behaviour of existing software that
> apparently did not suffer from this problem.

It's exactly the reason why you chose to fix the problem in gpio-mxc
instead of gpio-generic, right?  You thought other gpio-generic users
like gpio-mxs does not suffer from this problem.  Sadly, it's not the
case.

The updated patch simply corrects behaviour of gpio-generic for all
the users.

> You should make sure that
> the fix for your problem is not causing new problems for other users!
> 
I'm not fixing a gpio-mxs problem but a gpio-generic one.  Can you help
me understand what the new problems could possibly be introduced for
other gpio-generic users by the changes? 

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On 16 May 2012 15:34, Lothar Wa?mann <LW@karo-electronics.de> wrote:
> > You initialize a variable to a specific value that defaulted to zero
> > before.
> >
> The variable defaulted to zero is causing a problem.  I'm fixing that problem.
> 
Actually I had sent a similar patch in July last year which was
rejected with the following reasoning:

| I don't know if this is a valid thing to do or not. Not every GPIO
| controller supports reading back the initial state from the direction
| register. I suspect it is better to leave initialization of the
| direction to the gpio controller driver itself.

See: http://lkml.indiana.edu/hypermail/linux/kernel/1107.0/00908.html

That's why I came up with the patch to gpio-mxc.c


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Wed, May 16, 2012 at 03:33:26PM +0200, Lothar Wa?mann wrote:
> > > I'm not fixing a gpio-mxs problem but a gpio-generic one.  Can you help
> > >
> > Yes, I know. And that's exaclty why you should make sure that you
> > don't break other platforms by fixing yours!
> > 
> > > me understand what the new problems could possibly be introduced for
> > > other gpio-generic users by the changes? 
> > > 
> > Another platform using might be working just _because_ of the "broken"
> > behaviour. By fixing that brokenness for your driver you may break
> > that other platform.
> > 
> Can you point me any such platform?
> 
_You_ want to introduce a change that will change the behaviour of
existing code, so it's _your_ responsibility to make sure that your
change doesn't break other users.

Those are the current users of bgpio:
./drivers/gpio/gpio-sodaville.c:        ret = bgpio_init(&sd->bgpio, &pdev->dev, 4,
./drivers/gpio/gpio-ep93xx.c:   err = bgpio_init(bgc, dev, 1, data, NULL, NULL, dir, NULL, false);
./drivers/gpio/gpio-mxc.c:      err = bgpio_init(&port->bgc, &pdev->dev, 4,
./drivers/gpio/gpio-mxs.c:      err = bgpio_init(&port->bgc, &pdev->dev, 4,

You should at least CC: their maintainers to inform them that the code
they are relying on will change its behaviour.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Wed, May 16, 2012 at 03:33:26PM +0200, Lothar Wa?mann wrote:
> > I'm not fixing a gpio-mxs problem but a gpio-generic one.  Can you help
> >
> Yes, I know. And that's exaclty why you should make sure that you
> don't break other platforms by fixing yours!
> 
> > me understand what the new problems could possibly be introduced for
> > other gpio-generic users by the changes? 
> > 
> Another platform using might be working just _because_ of the "broken"
> behaviour. By fixing that brokenness for your driver you may break
> that other platform.
> 
Can you point me any such platform?

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Wed, May 16, 2012 at 03:54:37PM +0200, Lothar Wa?mann wrote:
> Actually I had sent a similar patch in July last year which was
> rejected with the following reasoning:
> 
> | I don't know if this is a valid thing to do or not. Not every GPIO
> | controller supports reading back the initial state from the direction
> | register. I suspect it is better to leave initialization of the
> | direction to the gpio controller driver itself.
> 
> See: http://lkml.indiana.edu/hypermail/linux/kernel/1107.0/00908.html
> 
> That's why I came up with the patch to gpio-mxc.c
> 
Thanks for the whole story, Lothar.  Hopefully, with the additional
condition checking added, Grant would agree it's the right thing to do.
I will send v2 of the patch to see what we get from people.

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Shawn Guo]

On Wed, May 16, 2012 at 04:03:51PM +0200, Lothar Wa?mann wrote:
> _You_ want to introduce a change that will change the behaviour of
> existing code, so it's _your_ responsibility to make sure that your
> change doesn't break other users.
> 
> Those are the current users of bgpio:
> ./drivers/gpio/gpio-sodaville.c:        ret = bgpio_init(&sd->bgpio, &pdev->dev, 4,
> ./drivers/gpio/gpio-ep93xx.c:   err = bgpio_init(bgc, dev, 1, data, NULL, NULL, dir, NULL, false);
> ./drivers/gpio/gpio-mxc.c:      err = bgpio_init(&port->bgc, &pdev->dev, 4,
> ./drivers/gpio/gpio-mxs.c:      err = bgpio_init(&port->bgc, &pdev->dev, 4,
> 
> You should at least CC: their maintainers to inform them that the code
> they are relying on will change its behaviour.
> 
Yeah, considering it's impossible for me to test the change on all
those hardwares, that's the only thing I can do.  Thanks for pointing
those users to me.

-- 
Regards,
Shawn

[PATCH] gpio/generic: initialize the shadow of direction register

[Grant Likely]

On Thu, 17 May 2012 00:17:27 +0800, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Wed, May 16, 2012 at 03:54:37PM +0200, Lothar Wa??mann wrote:
> > Actually I had sent a similar patch in July last year which was
> > rejected with the following reasoning:
> > 
> > | I don't know if this is a valid thing to do or not. Not every GPIO
> > | controller supports reading back the initial state from the direction
> > | register. I suspect it is better to leave initialization of the
> > | direction to the gpio controller driver itself.
> > 
> > See: http://lkml.indiana.edu/hypermail/linux/kernel/1107.0/00908.html
> > 
> > That's why I came up with the patch to gpio-mxc.c
> > 
> Thanks for the whole story, Lothar.  Hopefully, with the additional
> condition checking added, Grant would agree it's the right thing to do.
> I will send v2 of the patch to see what we get from people.

Yup, if there was some kind of conditional checking then it would be
fine.

g.