[PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Andrew Bresticker]

The AS3722_GPIO_INV bit will always be blindly overwritten by
as3722_pinctrl_gpio_set_direction() and will be ignored when
setting the value of the GPIO in as3722_gpio_set() since the
enable_gpio_invert flag is never set.  This will cause an
initially inverted GPIO to toggle when requested as an output,
which could be problematic if, for example, the GPIO controls
a critical regulator.

Instead of setting up the enable_gpio_invert flag, uust leave
the invert bit alone and check it before setting the GPIO value.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/pinctrl/pinctrl-as3722.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c
index 92ed4b2..c862f9c0 100644
--- a/drivers/pinctrl/pinctrl-as3722.c
+++ b/drivers/pinctrl/pinctrl-as3722.c
@@ -64,7 +64,6 @@ struct as3722_pin_function {
 };
 
 struct as3722_gpio_pin_control {
-	bool enable_gpio_invert;
 	unsigned mode_prop;
 	int io_function;
 };
@@ -320,10 +319,8 @@ static int as3722_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
 		return mode;
 	}
 
-	if (as_pci->gpio_control[offset].enable_gpio_invert)
-		mode |= AS3722_GPIO_INV;
-
-	return as3722_write(as3722, AS3722_GPIOn_CONTROL_REG(offset), mode);
+	return as3722_update_bits(as3722, AS3722_GPIOn_CONTROL_REG(offset),
+				AS3722_GPIO_MODE_MASK, mode);
 }
 
 static const struct pinmux_ops as3722_pinmux_ops = {
@@ -496,10 +493,18 @@ static void as3722_gpio_set(struct gpio_chip *chip, unsigned offset,
 {
 	struct as3722_pctrl_info *as_pci = to_as_pci(chip);
 	struct as3722 *as3722 = as_pci->as3722;
-	int en_invert = as_pci->gpio_control[offset].enable_gpio_invert;
+	int en_invert;
 	u32 val;
 	int ret;
 
+	ret = as3722_read(as3722, AS3722_GPIOn_CONTROL_REG(offset), &val);
+	if (ret < 0) {
+		dev_err(as_pci->dev,
+			"GPIO_CONTROL%d_REG read failed: %d\n", offset, ret);
+		return;
+	}
+	en_invert = !!(val & AS3722_GPIO_INV);
+
 	if (value)
 		val = (en_invert) ? 0 : AS3722_GPIOn_SIGNAL(offset);
 	else
-- 
1.9.1.423.g4596e3a

Re: [PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Stephen Warren]

On 04/15/2014 07:25 PM, Andrew Bresticker wrote:
> The AS3722_GPIO_INV bit will always be blindly overwritten by
> as3722_pinctrl_gpio_set_direction() and will be ignored when
> setting the value of the GPIO in as3722_gpio_set() since the
> enable_gpio_invert flag is never set.  This will cause an
> initially inverted GPIO to toggle when requested as an output,
> which could be problematic if, for example, the GPIO controls
> a critical regulator.
> 
> Instead of setting up the enable_gpio_invert flag, uust leave
> the invert bit alone and check it before setting the GPIO value.

Reviewed-by: Stephen Warren <swarren@nvidia.com>
Tested-by: Stephen Warren <swarren@nvidia.com>

(not with a 'scope or anything, but an affected system boots in a stable
fashion with this applied)

Should this be CC: stable?

Re: [PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Andrew Bresticker]

On Wed, Apr 16, 2014 at 9:21 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/15/2014 07:25 PM, Andrew Bresticker wrote:
>> The AS3722_GPIO_INV bit will always be blindly overwritten by
>> as3722_pinctrl_gpio_set_direction() and will be ignored when
>> setting the value of the GPIO in as3722_gpio_set() since the
>> enable_gpio_invert flag is never set.  This will cause an
>> initially inverted GPIO to toggle when requested as an output,
>> which could be problematic if, for example, the GPIO controls
>> a critical regulator.
>>
>> Instead of setting up the enable_gpio_invert flag, uust leave
>> the invert bit alone and check it before setting the GPIO value.
>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
>
> (not with a 'scope or anything, but an affected system boots in a stable
> fashion with this applied)
>
> Should this be CC: stable?

I think so.  Kernels with venice2 support should probably have this.
I'll resend and fix the typo in the commit message.

[PATCH v2] pinctrl: as3722: fix handling of GPIO invert bit

[Andrew Bresticker]

The AS3722_GPIO_INV bit will always be blindly overwritten by
as3722_pinctrl_gpio_set_direction() and will be ignored when
setting the value of the GPIO in as3722_gpio_set() since the
enable_gpio_invert flag is never set.  This will cause an
initially inverted GPIO to toggle when requested as an output,
which could be problematic if, for example, the GPIO controls
a critical regulator.

Instead of setting up the enable_gpio_invert flag, just leave
the invert bit alone and check it before setting the GPIO value.

Cc: <stable@vger.kernel.org> # v3.14+
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Stephen Warren <swarren@nvidia.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
---
Changes from v1:
 - fixed typo
---
 drivers/pinctrl/pinctrl-as3722.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c
index 92ed4b2..c862f9c0 100644
--- a/drivers/pinctrl/pinctrl-as3722.c
+++ b/drivers/pinctrl/pinctrl-as3722.c
@@ -64,7 +64,6 @@ struct as3722_pin_function {
 };
 
 struct as3722_gpio_pin_control {
-	bool enable_gpio_invert;
 	unsigned mode_prop;
 	int io_function;
 };
@@ -320,10 +319,8 @@ static int as3722_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
 		return mode;
 	}
 
-	if (as_pci->gpio_control[offset].enable_gpio_invert)
-		mode |= AS3722_GPIO_INV;
-
-	return as3722_write(as3722, AS3722_GPIOn_CONTROL_REG(offset), mode);
+	return as3722_update_bits(as3722, AS3722_GPIOn_CONTROL_REG(offset),
+				AS3722_GPIO_MODE_MASK, mode);
 }
 
 static const struct pinmux_ops as3722_pinmux_ops = {
@@ -496,10 +493,18 @@ static void as3722_gpio_set(struct gpio_chip *chip, unsigned offset,
 {
 	struct as3722_pctrl_info *as_pci = to_as_pci(chip);
 	struct as3722 *as3722 = as_pci->as3722;
-	int en_invert = as_pci->gpio_control[offset].enable_gpio_invert;
+	int en_invert;
 	u32 val;
 	int ret;
 
+	ret = as3722_read(as3722, AS3722_GPIOn_CONTROL_REG(offset), &val);
+	if (ret < 0) {
+		dev_err(as_pci->dev,
+			"GPIO_CONTROL%d_REG read failed: %d\n", offset, ret);
+		return;
+	}
+	en_invert = !!(val & AS3722_GPIO_INV);
+
 	if (value)
 		val = (en_invert) ? 0 : AS3722_GPIOn_SIGNAL(offset);
 	else
-- 
1.9.1.423.g4596e3a

Re: [PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Laxman Dewangan]

Hi Andrew,

On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote:
> diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c
> index 92ed4b2..c862f9c0 100644
> --- a/drivers/pinctrl/pinctrl-as3722.c
> +++ b/drivers/pinctrl/pinctrl-as3722.c
> @@ -64,7 +64,6 @@ struct as3722_pin_function {
>   };
>   
>   struct as3722_gpio_pin_control {
> -	bool enable_gpio_invert;
>   	unsigned mode_prop;
>   	int io_function;
>   };

Instead of removing this flag and  calling read of register on every 
gpio set, better if we update this flag on probe once and use this for 
entire life.

So adding the code in probe() as

         for (i = 0; i < ARRAY_SIZE(as3722_pingroups); ++i) {
                 int gpio_cntr_reg = AS3722_GPIOn_CONTROL_REG(i);
                 u32 val;

                 ret = as3722_read(as3722, gpio_cntr_reg, &val);
                 if (!ret)
as_pci->gpio_control[i].enable_gpio_invert =
                                         !!(val & AS3722_GPIO_INV);
         }


Thanks,
Laxman

Re: [PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Andrew Bresticker]

On Thu, Apr 17, 2014 at 2:48 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Hi Andrew,
>
>
> On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote:
>>
>> diff --git a/drivers/pinctrl/pinctrl-as3722.c
>> b/drivers/pinctrl/pinctrl-as3722.c
>> index 92ed4b2..c862f9c0 100644
>> --- a/drivers/pinctrl/pinctrl-as3722.c
>> +++ b/drivers/pinctrl/pinctrl-as3722.c
>> @@ -64,7 +64,6 @@ struct as3722_pin_function {
>>   };
>>     struct as3722_gpio_pin_control {
>> -       bool enable_gpio_invert;
>>         unsigned mode_prop;
>>         int io_function;
>>   };
>
>
> Instead of removing this flag and  calling read of register on every gpio
> set, better if we update this flag on probe once and use this for entire
> life.

Because of regcache, the reads won't result in any additional I2C
transfers.  I can respin though if you want.

-Andrew

>
> So adding the code in probe() as
>
>         for (i = 0; i < ARRAY_SIZE(as3722_pingroups); ++i) {
>                 int gpio_cntr_reg = AS3722_GPIOn_CONTROL_REG(i);
>                 u32 val;
>
>                 ret = as3722_read(as3722, gpio_cntr_reg, &val);
>                 if (!ret)
> as_pci->gpio_control[i].enable_gpio_invert =
>                                         !!(val & AS3722_GPIO_INV);
>         }
>
>
> Thanks,
> Laxman

Re: [PATCH] pinctrl: as3722: fix handling of GPIO invert bit

[Laxman Dewangan]

On Thursday 17 April 2014 10:13 PM, Andrew Bresticker wrote:
> On Thu, Apr 17, 2014 at 2:48 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> Hi Andrew,
>>
>>
>> On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote:
>>> diff --git a/drivers/pinctrl/pinctrl-as3722.c
>>> b/drivers/pinctrl/pinctrl-as3722.c
>>> index 92ed4b2..c862f9c0 100644
>>> --- a/drivers/pinctrl/pinctrl-as3722.c
>>> +++ b/drivers/pinctrl/pinctrl-as3722.c
>>> @@ -64,7 +64,6 @@ struct as3722_pin_function {
>>>    };
>>>      struct as3722_gpio_pin_control {
>>> -       bool enable_gpio_invert;
>>>          unsigned mode_prop;
>>>          int io_function;
>>>    };
>>
>> Instead of removing this flag and  calling read of register on every gpio
>> set, better if we update this flag on probe once and use this for entire
>> life.
> Because of regcache, the reads won't result in any additional I2C
> transfers.  I can respin though if you want.

Yaah, if regcache is enabled on this then it will reduce i2c calls.

Just wanted to keep this invert bit so in future, if we need it, we can 
make it configurable. But not much issue here.

I am acking it
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

Re: [PATCH v2] pinctrl: as3722: fix handling of GPIO invert bit

[Linus Walleij]

On Wed, Apr 16, 2014 at 10:40 PM, Andrew Bresticker
<abrestic@chromium.org> wrote:

> The AS3722_GPIO_INV bit will always be blindly overwritten by
> as3722_pinctrl_gpio_set_direction() and will be ignored when
> setting the value of the GPIO in as3722_gpio_set() since the
> enable_gpio_invert flag is never set.  This will cause an
> initially inverted GPIO to toggle when requested as an output,
> which could be problematic if, for example, the GPIO controls
> a critical regulator.
>
> Instead of setting up the enable_gpio_invert flag, just leave
> the invert bit alone and check it before setting the GPIO value.
>
> Cc: <stable@vger.kernel.org> # v3.14+
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes from v1:
>  - fixed typo

This v2 version applied for fixes with Laxman's ACK.

Yours,
Linus Walleij