gpio/generic: initialize the shadow of direction register

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 <[email protected]>

---
 drivers/gpio/gpio-generic.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/gpio/gpio-generic.c +0 -0

index e38dd0c..53222c7 100644

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);

Quoted 5 lines · Lothar Wa?mann

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:

Quoted 12 lines

|        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?

Quoted 9 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);
 

Review int bgpio_init(struct bgpio_chip *bgc, struct device *dev, · line 403

Quoted 3 lines

(quoted diff) +0 -0

        return ret;
 }

That might change the behaviour of existing implementations.

Quoted 9 lines · Lothar Wa?mann

Quoted 9 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);
 

Quoted 5 lines

Review int bgpio_init(struct bgpio_chip *bgc, struct device *dev, · line 403

Quoted 3 lines

(quoted diff) +0 -0

        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.

Quoted 3 lines · Lothar Wa?mann

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!

Quoted 2 lines · Lothar Wa?mann

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.

Quoted 3 lines

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?

Quoted 2 lines

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!

Quoted 3 lines

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.

Quoted 5 lines · Lothar Wa?mann

Quoted 2 lines

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!

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines · Lothar Wa?mann

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

Quoted 8 lines · Lothar Wa??mann

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.

Quoted 4 lines

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.