[PATCH] gpio: pca953x: Don't flip bits on PCA957x GPIO expanders when probing them.

[PATCH] gpio: pca953x: Don't flip bits on PCA957x GPIO expanders when probing them.

[Graeme Smecher]

The pca957x driver supports a handful of I2C GPIO expanders from NXP, Maxim,
and TI. For the PCA9574 and PCA9575 devices only, the driver resets the GPIO
level and direction in the pca957x_probe function. This seems like the wrong
thing to do, since it can cause hardware bit twiddles during warm reboots when
the chip state and reset values don't match.

This kind of initialization is best left upstream (in a bootloader) or
downstream (in userspace). It's also an inconsistency across devices supported
by this driver.

Signed-off-by: Graeme Smecher <gsmecher@threespeedlogic.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 drivers/gpio/gpio-pca953x.c |   11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 426c51d..47c08d8 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -683,17 +683,6 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 	int ret;
 	u8 val[MAX_BANK];
 
-	/* Let every port in proper state, that could save power */
-	memset(val, 0, NBANK(chip));
-	pca953x_write_regs(chip, PCA957X_PUPD, val);
-	memset(val, 0xFF, NBANK(chip));
-	pca953x_write_regs(chip, PCA957X_CFG, val);
-	memset(val, 0, NBANK(chip));
-	pca953x_write_regs(chip, PCA957X_OUT, val);
-
-	ret = pca953x_read_regs(chip, PCA957X_IN, val);
-	if (ret)
-		goto out;
 	ret = pca953x_read_regs(chip, PCA957X_OUT, chip->reg_output);
 	if (ret)
 		goto out;
-- 
1.7.10.4

Re: [PATCH] gpio: pca953x: Don't flip bits on PCA957x GPIO expanders when probing them.

[Haojian Zhuang]

On Sat, Aug 3, 2013 at 5:24 AM, Graeme Smecher
<gsmecher@threespeedlogic.com> wrote:
> The pca957x driver supports a handful of I2C GPIO expanders from NXP, Maxim,
> and TI. For the PCA9574 and PCA9575 devices only, the driver resets the GPIO
> level and direction in the pca957x_probe function. This seems like the wrong
> thing to do, since it can cause hardware bit twiddles during warm reboots when
> the chip state and reset values don't match.
>
> This kind of initialization is best left upstream (in a bootloader) or
> downstream (in userspace). It's also an inconsistency across devices supported
> by this driver.
>
> Signed-off-by: Graeme Smecher <gsmecher@threespeedlogic.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  drivers/gpio/gpio-pca953x.c |   11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 426c51d..47c08d8 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -683,17 +683,6 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
>         int ret;
>         u8 val[MAX_BANK];
>
> -       /* Let every port in proper state, that could save power */
> -       memset(val, 0, NBANK(chip));
> -       pca953x_write_regs(chip, PCA957X_PUPD, val);
> -       memset(val, 0xFF, NBANK(chip));
> -       pca953x_write_regs(chip, PCA957X_CFG, val);
> -       memset(val, 0, NBANK(chip));
> -       pca953x_write_regs(chip, PCA957X_OUT, val);
> -
> -       ret = pca953x_read_regs(chip, PCA957X_IN, val);
> -       if (ret)
> -               goto out;
>         ret = pca953x_read_regs(chip, PCA957X_OUT, chip->reg_output);
>         if (ret)
>                 goto out;
> --
> 1.7.10.4
>

Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Re: [PATCH] gpio: pca953x: Don't flip bits on PCA957x GPIO expanders when probing them.

[Graeme Smecher]

Hi Grant,

On 04/08/13 05:35 AM, Haojian Zhuang wrote:
> On Sat, Aug 3, 2013 at 5:24 AM, Graeme Smecher
> <gsmecher@threespeedlogic.com>  wrote:
>> The pca957x driver supports a handful of I2C GPIO expanders from NXP, Maxim,
>> and TI. For the PCA9574 and PCA9575 devices only, the driver resets the GPIO
>> level and direction in the pca957x_probe function. This seems like the wrong
>> thing to do, since it can cause hardware bit twiddles during warm reboots when
>> the chip state and reset values don't match.
>>
>> This kind of initialization is best left upstream (in a bootloader) or
>> downstream (in userspace). It's also an inconsistency across devices supported
>> by this driver.
>>
>> Signed-off-by: Graeme Smecher<gsmecher@threespeedlogic.com>
>> Cc: Grant Likely<grant.likely@linaro.org>
>> Cc: Haojian Zhuang<haojian.zhuang@marvell.com>
>> ---
>>   drivers/gpio/gpio-pca953x.c |   11 -----------
>>   1 file changed, 11 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index 426c51d..47c08d8 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -683,17 +683,6 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
>>          int ret;
>>          u8 val[MAX_BANK];
>>
>> -       /* Let every port in proper state, that could save power */
>> -       memset(val, 0, NBANK(chip));
>> -       pca953x_write_regs(chip, PCA957X_PUPD, val);
>> -       memset(val, 0xFF, NBANK(chip));
>> -       pca953x_write_regs(chip, PCA957X_CFG, val);
>> -       memset(val, 0, NBANK(chip));
>> -       pca953x_write_regs(chip, PCA957X_OUT, val);
>> -
>> -       ret = pca953x_read_regs(chip, PCA957X_IN, val);
>> -       if (ret)
>> -               goto out;
>>          ret = pca953x_read_regs(chip, PCA957X_OUT, chip->reg_output);
>>          if (ret)
>>                  goto out;
>> --
>> 1.7.10.4
>>
> Acked-by: Haojian Zhuang<haojian.zhuang@gmail.com>

Just a quick ping: I think you're the responsible maintainer for this 
patch, and I haven't seen it show up on git.secretlab.ca. Do you want me 
to resubmit with Haojian's e-mail corrected?

I should also note that this patch is NOT boot-tested. My hardware 
containing this part is only supported by the vendor's aging 2.6.37 
branch. (It's a custom ARM board using the TI AM3874.) The pca953x 
driver has evolved enough that the patches for 2.6.37 and mainline 
aren't identical, but they still rhyme.

thanks,
Graeme