[PATCH] gpio: exar set value handling for hw with gpio pull-up or pull-down

[PATCH] gpio: exar set value handling for hw with gpio pull-up or pull-down

[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 input to output,
exar_set_value is set to 1, but since direction is input when
exar_set_value is called, _regmap_update_bits reads a 1 due to an
external pull-up.  When force_write is not set (regmap_set_bits has
force_write = false), the value 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

Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
---
 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 482f678c893e..de5ce73159cb 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] gpio: exar set value handling for hw with gpio pull-up or pull-down

[Matthew McClain]

Hi all,

I worked with Sai on this.

We discovered this problem when we upgraded our kernel from 5.10 to 6.1.
The behavior changed with the switch to regmap in 5.11.

commit 36fb7218e87833b17e3042e77f3b102c75129e8f
Author: Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Mon Sep 28 17:00:26 2020 +0200

     gpio: exar: switch to using regmap

     We can simplify the code in gpio-exar by using regmap. This allows
     us to drop the mutex (regmap provides its own locking) and we can
     also reuse regmap's bit operations instead of implementing our own
     update function.

     Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
     Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


We noticed because we had some gpios tied to reset pins and when
we set direction to high, value went to 0, and put devices in reset.

Thanks,
Matthew


On 7/30/24 08:46, 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 input to output,
> exar_set_value is set to 1, but since direction is input when
> exar_set_value is called, _regmap_update_bits reads a 1 due to an
> external pull-up.  When force_write is not set (regmap_set_bits has
> force_write = false), the value 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
>
> Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com>
> ---
>   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 482f678c893e..de5ce73159cb 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,

Re: [PATCH] gpio: exar set value handling for hw with gpio pull-up or pull-down

[Andy Shevchenko]

On Tue, Jul 30, 2024 at 07:16:10PM +0530, Sai Kumar Cholleti wrote:

Please, refer to the functions in the text as func(), e.g. exar_set_value().
Use proper acronym, i.e. GPIO (capitalised).

> 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 input to output,
> exar_set_value is set to 1, but since direction is input when
> exar_set_value is called, _regmap_update_bits reads a 1 due to an
> external pull-up.  When force_write is not set (regmap_set_bits has
> force_write = false), the value 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

Okay, shouldn't you instead mark the respective registers as volatile or so?
I believe regmap has some settings for this case. But I haven't checked myself.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: exar set value handling for hw with gpio pull-up or pull-down

[Matthew McClain]

On 8/12/24 12:22, Andy Shevchenko wrote:
 > On Tue, Jul 30, 2024 at 07:16:10PM +0530, Sai Kumar Cholleti wrote:
 >
 > Please, refer to the functions in the text as func(), e.g. 
exar_set_value().
 > Use proper acronym, i.e. GPIO (capitalised).

We will update the patch and send a new version out if the current
approach is acceptable.

 >> Before the gpio direction is changed from input to output,
 >> exar_set_value is set to 1, but since direction is input when
 >> exar_set_value is called, _regmap_update_bits reads a 1 due to an
 >> external pull-up.  When force_write is not set (regmap_set_bits has
 >> force_write = false), the value 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.

 > Okay, shouldn't you instead mark the respective registers as volatile 
or so?
 > I believe regmap has some settings for this case. But I haven't 
checked myself.

Unfortunately, in addition to marking the regmap volatile, we'd need to
define reg_update_bits which means we'd be partially undoing the work
from 36fb7218e87833b17e3042e77f3b102c75129e8f to reuse regmap locking
and update functions.

Below is the relevant section of _regmap_update_bits().

static int _regmap_update_bits(struct regmap *map, unsigned int reg,
                                unsigned int mask, unsigned int val,
                                bool *change, bool force_write)
...
         if (regmap_volatile(map, reg) && map->reg_update_bits) {
...
         } else {
...
                 if (force_write || (tmp != orig) || 
map->force_write_field) {
                         ret = _regmap_write(map, reg, tmp);
                         if (ret == 0 && change)
                                 *change = true;
...

I suspect this might be a common problem with other GPIO drivers as
well.

Thank you,
Matthew

Re: [PATCH] gpio: exar set value handling for hw with gpio pull-up or pull-down

[Andy Shevchenko]

On Tue, Aug 13, 2024 at 10:07:37AM -0500, Matthew McClain wrote:
> On 8/12/24 12:22, Andy Shevchenko wrote:
> > On Tue, Jul 30, 2024 at 07:16:10PM +0530, Sai Kumar Cholleti wrote:
> >
> > Please, refer to the functions in the text as func(), e.g.
> exar_set_value().
> > Use proper acronym, i.e. GPIO (capitalised).
> 
> We will update the patch and send a new version out if the current
> approach is acceptable.
> 
> >> Before the gpio direction is changed from input to output,
> >> exar_set_value is set to 1, but since direction is input when
> >> exar_set_value is called, _regmap_update_bits reads a 1 due to an
> >> external pull-up.  When force_write is not set (regmap_set_bits has
> >> force_write = false), the value 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.
> 
> > Okay, shouldn't you instead mark the respective registers as volatile or
> so?
> > I believe regmap has some settings for this case. But I haven't checked
> myself.
> 
> Unfortunately, in addition to marking the regmap volatile, we'd need to
> define reg_update_bits which means we'd be partially undoing the work
> from 36fb7218e87833b17e3042e77f3b102c75129e8f to reuse regmap locking
> and update functions.
> 
> Below is the relevant section of _regmap_update_bits().
> 
> static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>                                unsigned int mask, unsigned int val,
>                                bool *change, bool force_write)
> ...
>         if (regmap_volatile(map, reg) && map->reg_update_bits) {
> ...
>         } else {
> ...
>                 if (force_write || (tmp != orig) || map->force_write_field)
> {
>                         ret = _regmap_write(map, reg, tmp);
>                         if (ret == 0 && change)
>                                 *change = true;
> ...
> 
> I suspect this might be a common problem with other GPIO drivers as
> well.

Sorry, this message went to cracks somehow. Can you update the commit
message and comments as mentioned above, add Fixes tag and send a v2
for a review. Also use 'gpio: exar: ...' in the Subject.

-- 
With Best Regards,
Andy Shevchenko

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