[PATCH v2] gpio: exar: set value when external pull-up or pull-down is present

[PATCH v2] gpio: exar: set value when external pull-up or pull-down is present

[Sai Kumar Cholleti]

Setting GPIO direction = high, sometimes results in GPIO value = 0.

If a GPIO is pulled high, the following construction results in the
value being 0 when the desired value is 1:

$ echo "high" > /sys/class/gpio/gpio336/direction
$ cat /sys/class/gpio/gpio336/value
0

Before the GPIO direction is changed from an input to an output,
exar_set_value() is called with value = 1, but since the GPIO is an
input when exar_set_value() is called, _regmap_update_bits() reads a 1
due to an external pull-up.  regmap_set_bits() sets force_write =
false, so the value (1) is not written.  When the direction is then
changed, the GPIO becomes an output with the value of 0 (the hardware
default).

regmap_write_bits() sets force_write = true, so the value is always
written by exar_set_value() and an external pull-up doesn't affect the
outcome of setting direction = high.


The same can happen when a GPIO is pulled low, but the scenario is a
little more complicated.

$ echo high > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
1

$ echo in > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
0

$ echo low > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
1

Fixes: 36fb7218e878 ("gpio: exar: switch to using regmap") 
Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
Signed-off-by: Matthew McClain <mmcclain@noprivs.com>
Cc: <stable@vger.kernel.org>
---
 drivers/gpio/gpio-exar.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 5170fe7599cd..dfc7a9ca3e62 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -99,11 +99,13 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+	unsigned int bit_value = value ? BIT(bit) : 0;
 
-	if (value)
-		regmap_set_bits(exar_gpio->regmap, addr, BIT(bit));
-	else
-		regmap_clear_bits(exar_gpio->regmap, addr, BIT(bit));
+	/*
+	 * regmap_write_bits forces value to be written when an external
+	 * pull up/down might otherwise indicate value was already set
+	 */
+	regmap_write_bits(exar_gpio->regmap, addr, BIT(bit), bit_value);
 }
 
 static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
-- 
2.34.1

[PATCH v2] gpio: exar: set value when external pull-up or pull-down is present

[Sai Kumar Cholleti]

Setting GPIO direction = high, sometimes results in GPIO value = 0.

If a GPIO is pulled high, the following construction results in the
value being 0 when the desired value is 1:

$ echo "high" > /sys/class/gpio/gpio336/direction
$ cat /sys/class/gpio/gpio336/value
0

Before the GPIO direction is changed from an input to an output,
exar_set_value() is called with value = 1, but since the GPIO is an
input when exar_set_value() is called, _regmap_update_bits() reads a 1
due to an external pull-up.  regmap_set_bits() sets force_write =
false, so the value (1) is not written.  When the direction is then
changed, the GPIO becomes an output with the value of 0 (the hardware
default).

regmap_write_bits() sets force_write = true, so the value is always
written by exar_set_value() and an external pull-up doesn't affect the
outcome of setting direction = high.


The same can happen when a GPIO is pulled low, but the scenario is a
little more complicated.

$ echo high > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
1

$ echo in > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
0

$ echo low > /sys/class/gpio/gpio351/direction 
$ cat /sys/class/gpio/gpio351/value
1

Fixes: 36fb7218e878 ("gpio: exar: switch to using regmap") 
Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
Signed-off-by: Matthew McClain <mmcclain@noprivs.com>
Cc: <stable@vger.kernel.org>
---
 drivers/gpio/gpio-exar.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 5170fe7599cd..dfc7a9ca3e62 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -99,11 +99,13 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+	unsigned int bit_value = value ? BIT(bit) : 0;
 
-	if (value)
-		regmap_set_bits(exar_gpio->regmap, addr, BIT(bit));
-	else
-		regmap_clear_bits(exar_gpio->regmap, addr, BIT(bit));
+	/*
+	 * regmap_write_bits forces value to be written when an external
+	 * pull up/down might otherwise indicate value was already set
+	 */
+	regmap_write_bits(exar_gpio->regmap, addr, BIT(bit), bit_value);
 }
 
 static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
-- 
2.34.1

Re: [PATCH v2] gpio: exar: set value when external pull-up or pull-down is present

[Andy Shevchenko]

On Mon, Nov 04, 2024 at 09:17:57PM +0530, Sai Kumar Cholleti wrote:
> Setting GPIO direction = high, sometimes results in GPIO value = 0.
> 
> If a GPIO is pulled high, the following construction results in the
> value being 0 when the desired value is 1:
> 
> $ echo "high" > /sys/class/gpio/gpio336/direction
> $ cat /sys/class/gpio/gpio336/value
> 0
> 
> Before the GPIO direction is changed from an input to an output,
> exar_set_value() is called with value = 1, but since the GPIO is an
> input when exar_set_value() is called, _regmap_update_bits() reads a 1
> due to an external pull-up.  regmap_set_bits() sets force_write =
> false, so the value (1) is not written.  When the direction is then
> changed, the GPIO becomes an output with the value of 0 (the hardware
> default).
> 
> regmap_write_bits() sets force_write = true, so the value is always
> written by exar_set_value() and an external pull-up doesn't affect the
> outcome of setting direction = high.
> 
> 
> The same can happen when a GPIO is pulled low, but the scenario is a
> little more complicated.
> 

> $ echo high > /sys/class/gpio/gpio351/direction 

This...

> $ cat /sys/class/gpio/gpio351/value
> 1
> 
> $ echo in > /sys/class/gpio/gpio351/direction 

...this...

> $ cat /sys/class/gpio/gpio351/value
> 0
> 
> $ echo low > /sys/class/gpio/gpio351/direction 

...this...

> $ cat /sys/class/gpio/gpio351/value
> 1

> Fixes: 36fb7218e878 ("gpio: exar: switch to using regmap") 

...and this lines have a trailing space.

> Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
> Signed-off-by: Matthew McClain <mmcclain@noprivs.com>

Hmm... Missing Co-developed-by?
This SoB chain puzzles me a bit, the committer should go last AFAIR.

...

> +	/*
> +	 * regmap_write_bits forces value to be written when an external

regmap_write_bits()


> +	 * pull up/down might otherwise indicate value was already set

Missing period at the end.

> +	 */

...

Other than above LGTM.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Sai Kumar Cholleti]

Setting GPIO direction = high, sometimes results in GPIO value = 0.

If a GPIO is pulled high, the following construction results in the
value being 0 when the desired value is 1:

$ echo "high" > /sys/class/gpio/gpio336/direction
$ cat /sys/class/gpio/gpio336/value
0

Before the GPIO direction is changed from an input to an output,
exar_set_value() is called with value = 1, but since the GPIO is an
input when exar_set_value() is called, _regmap_update_bits() reads a 1
due to an external pull-up.  regmap_set_bits() sets force_write =
false, so the value (1) is not written.  When the direction is then
changed, the GPIO becomes an output with the value of 0 (the hardware
default).

regmap_write_bits() sets force_write = true, so the value is always
written by exar_set_value() and an external pull-up doesn't affect the
outcome of setting direction = high.


The same can happen when a GPIO is pulled low, but the scenario is a
little more complicated.

$ echo high > /sys/class/gpio/gpio351/direction
$ cat /sys/class/gpio/gpio351/value
1

$ echo in > /sys/class/gpio/gpio351/direction
$ cat /sys/class/gpio/gpio351/value
0

$ echo low > /sys/class/gpio/gpio351/direction
$ cat /sys/class/gpio/gpio351/value
1

Fixes: 36fb7218e878 ("gpio: exar: switch to using regmap")
Co-developed-by: Matthew McClain <mmcclain@noprivs.com>
Signed-off-by: Matthew McClain <mmcclain@noprivs.com>
Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
Cc: <stable@vger.kernel.org>
---
 drivers/gpio/gpio-exar.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 5170fe7599cd..d5909a4f0433 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -99,11 +99,13 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+	unsigned int bit_value = value ? BIT(bit) : 0;
 
-	if (value)
-		regmap_set_bits(exar_gpio->regmap, addr, BIT(bit));
-	else
-		regmap_clear_bits(exar_gpio->regmap, addr, BIT(bit));
+	/*
+	 * regmap_write_bits() forces value to be written when an external
+	 * pull up/down might otherwise indicate value was already set.
+	 */
+	regmap_write_bits(exar_gpio->regmap, addr, BIT(bit), bit_value);
 }
 
 static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
-- 
2.34.1

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Andy Shevchenko]

On Tue, Nov 05, 2024 at 12:45:23PM +0530, Sai Kumar Cholleti wrote:
> Setting GPIO direction = high, sometimes results in GPIO value = 0.
> 
> If a GPIO is pulled high, the following construction results in the
> value being 0 when the desired value is 1:
> 
> $ echo "high" > /sys/class/gpio/gpio336/direction
> $ cat /sys/class/gpio/gpio336/value
> 0
> 
> Before the GPIO direction is changed from an input to an output,
> exar_set_value() is called with value = 1, but since the GPIO is an
> input when exar_set_value() is called, _regmap_update_bits() reads a 1
> due to an external pull-up.  regmap_set_bits() sets force_write =
> false, so the value (1) is not written.  When the direction is then
> changed, the GPIO becomes an output with the value of 0 (the hardware
> default).
> 
> regmap_write_bits() sets force_write = true, so the value is always
> written by exar_set_value() and an external pull-up doesn't affect the
> outcome of setting direction = high.
> 
> 
> The same can happen when a GPIO is pulled low, but the scenario is a
> little more complicated.
> 
> $ echo high > /sys/class/gpio/gpio351/direction
> $ cat /sys/class/gpio/gpio351/value
> 1
> 
> $ echo in > /sys/class/gpio/gpio351/direction
> $ cat /sys/class/gpio/gpio351/value
> 0
> 
> $ echo low > /sys/class/gpio/gpio351/direction
> $ cat /sys/class/gpio/gpio351/value
> 1

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Andy Shevchenko]

On Tue, Nov 05, 2024 at 04:39:38PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 05, 2024 at 12:45:23PM +0530, Sai Kumar Cholleti wrote:
> > Setting GPIO direction = high, sometimes results in GPIO value = 0.
> > 
> > If a GPIO is pulled high, the following construction results in the
> > value being 0 when the desired value is 1:
> > 
> > $ echo "high" > /sys/class/gpio/gpio336/direction
> > $ cat /sys/class/gpio/gpio336/value
> > 0
> > 
> > Before the GPIO direction is changed from an input to an output,
> > exar_set_value() is called with value = 1, but since the GPIO is an
> > input when exar_set_value() is called, _regmap_update_bits() reads a 1
> > due to an external pull-up.  regmap_set_bits() sets force_write =
> > false, so the value (1) is not written.  When the direction is then
> > changed, the GPIO becomes an output with the value of 0 (the hardware
> > default).
> > 
> > regmap_write_bits() sets force_write = true, so the value is always
> > written by exar_set_value() and an external pull-up doesn't affect the
> > outcome of setting direction = high.
> > 
> > 
> > The same can happen when a GPIO is pulled low, but the scenario is a
> > little more complicated.
> > 
> > $ echo high > /sys/class/gpio/gpio351/direction
> > $ cat /sys/class/gpio/gpio351/value
> > 1
> > 
> > $ echo in > /sys/class/gpio/gpio351/direction
> > $ cat /sys/class/gpio/gpio351/value
> > 0
> > 
> > $ echo low > /sys/class/gpio/gpio351/direction
> > $ cat /sys/class/gpio/gpio351/value
> > 1
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Does this need to be applied, Bart?
Seems it is missed in your branches...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Bartosz Golaszewski]

On Tue, Nov 12, 2024 at 5:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 05, 2024 at 04:39:38PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 05, 2024 at 12:45:23PM +0530, Sai Kumar Cholleti wrote:
> > > Setting GPIO direction = high, sometimes results in GPIO value = 0.
> > >
> > > If a GPIO is pulled high, the following construction results in the
> > > value being 0 when the desired value is 1:
> > >
> > > $ echo "high" > /sys/class/gpio/gpio336/direction
> > > $ cat /sys/class/gpio/gpio336/value
> > > 0
> > >
> > > Before the GPIO direction is changed from an input to an output,
> > > exar_set_value() is called with value = 1, but since the GPIO is an
> > > input when exar_set_value() is called, _regmap_update_bits() reads a 1
> > > due to an external pull-up.  regmap_set_bits() sets force_write =
> > > false, so the value (1) is not written.  When the direction is then
> > > changed, the GPIO becomes an output with the value of 0 (the hardware
> > > default).
> > >
> > > regmap_write_bits() sets force_write = true, so the value is always
> > > written by exar_set_value() and an external pull-up doesn't affect the
> > > outcome of setting direction = high.
> > >
> > >
> > > The same can happen when a GPIO is pulled low, but the scenario is a
> > > little more complicated.
> > >
> > > $ echo high > /sys/class/gpio/gpio351/direction
> > > $ cat /sys/class/gpio/gpio351/value
> > > 1
> > >
> > > $ echo in > /sys/class/gpio/gpio351/direction
> > > $ cat /sys/class/gpio/gpio351/value
> > > 0
> > >
> > > $ echo low > /sys/class/gpio/gpio351/direction
> > > $ cat /sys/class/gpio/gpio351/value
> > > 1
> >
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Does this need to be applied, Bart?
> Seems it is missed in your branches...
>

Maybe if the author used get_maintainers.pl as they should, I would
have noticed this earlier?

I have some other fixes to pick up so I'll send this later in the merge window.

Bart

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Andy Shevchenko]

On Mon, Nov 18, 2024 at 12:00:00PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 12, 2024 at 5:09 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Nov 05, 2024 at 04:39:38PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 05, 2024 at 12:45:23PM +0530, Sai Kumar Cholleti wrote:

...

> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Does this need to be applied, Bart?
> > Seems it is missed in your branches...
> 
> Maybe if the author used get_maintainers.pl as they should, I would
> have noticed this earlier?

Ah good catch!

Sai, FYI, I use my script [1] which does all required stuff for me.
Feel free to use it, patch, comment, etc...

> I have some other fixes to pick up so I'll send this later in the merge window.

Thanks!

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[sai kumar]

Thanks Andy and Bart.

On Mon, Nov 18, 2024 at 6:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Nov 18, 2024 at 12:00:00PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 12, 2024 at 5:09 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Nov 05, 2024 at 04:39:38PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Nov 05, 2024 at 12:45:23PM +0530, Sai Kumar Cholleti wrote:
>
> ...
>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Does this need to be applied, Bart?
> > > Seems it is missed in your branches...
> >
> > Maybe if the author used get_maintainers.pl as they should, I would
> > have noticed this earlier?
>
> Ah good catch!
>
> Sai, FYI, I use my script [1] which does all required stuff for me.
> Feel free to use it, patch, comment, etc...
>
> > I have some other fixes to pick up so I'll send this later in the merge window.
>
> Thanks!
>
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] gpio: exar: set value when external pull-up or pull-down is present

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 05 Nov 2024 12:45:23 +0530, Sai Kumar Cholleti wrote:
> Setting GPIO direction = high, sometimes results in GPIO value = 0.
> 
> If a GPIO is pulled high, the following construction results in the
> value being 0 when the desired value is 1:
> 
> $ echo "high" > /sys/class/gpio/gpio336/direction
> $ cat /sys/class/gpio/gpio336/value
> 0
> 
> [...]

Applied, thanks!

[1/1] gpio: exar: set value when external pull-up or pull-down is present
      commit: 72cef64180de04a7b055b4773c138d78f4ebdb77

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>