Re: [PATCH v4 16/16] gpio: Add a way to read 3-way strapping pins

[Patrick DELAUNAY]

Hi Simon,

On 2/5/21 5:22 AM, Simon Glass wrote:
Using the internal vs. external pull resistors it is possible to get
27 different combinations from 3 strapping pins. Add an implementation
of this.
This involves updating the sandbox GPIO driver to model external and
(weaker) internal pull resistors. The get_value() method now takes account
of what is driving a pin:

    sandbox: GPIOD_EXT_DRIVEN - in which case GPIO_EXT_HIGH provides the
           value
    outside source - in which case GPIO_EXT_PULL_UP/DOWN indicates the
           external state and we work the final state using those flags and
           the internal GPIOD_PULL_UP/DOWN flags

Of course the outside source does not really exist in sandbox. We are just
modelling it for test purpose.

Signed-off-by: Simon Glass <s...@chromium.org>
---

(no changes since v3)

Changes in v3:
- Use bits 28, 29 for the new flags
- Assert that count parameter is within range
- Redo digit logic to be easier to understand
- Update function comment to explain the meaning of the digits
- Fix 'compare' typo

  arch/sandbox/include/asm/gpio.h |  5 +-
  drivers/gpio/gpio-uclass.c      | 81 +++++++++++++++++++++++++++
  drivers/gpio/sandbox.c          | 13 +++--
  include/asm-generic/gpio.h      | 40 ++++++++++++++
  test/dm/gpio.c                  | 98 +++++++++++++++++++++++++++++++++
  5 files changed, 232 insertions(+), 5 deletions(-)

(...)
diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
index 700098446b5..d008fdd2224 100644
--- a/drivers/gpio/sandbox.c
+++ b/drivers/gpio/sandbox.c
@@ -19,7 +19,6 @@
  #include <dt-bindings/gpio/gpio.h>
  #include <dt-bindings/gpio/sandbox-gpio.h>
  
-
  struct gpio_state {
        const char *label;      /* label given by requester */
        ulong flags;            /* flags (GPIOD_...) */
@@ -81,10 +80,16 @@ int sandbox_gpio_get_value(struct udevice *dev, unsigned 
offset)
        if (get_gpio_flag(dev, offset, GPIOD_IS_OUT))
                debug("sandbox_gpio: get_value on output gpio %u\n", offset);
  
-	if (state->flags & GPIOD_EXT_DRIVEN)
+       if (state->flags & GPIOD_EXT_DRIVEN) {
                val = state->flags & GPIOD_EXT_HIGH;

bool here, not int

+ val = !!(state->flags & GPIOD_EXT_HIGH);


-       else
-               val = false;
+       } else {
+               if (state->flags & GPIOD_EXT_PULL_UP)
+                       val = true;
+               else if (state->flags & GPIOD_EXT_PULL_DOWN)
+                       val = false;
+               else
+                       val = state->flags & GPIOD_PULL_UP;

bool also

+ val = !!(state->flags & GPIOD_PULL_UP );


+       }
  
  	return val;
  }


(...)

Just to be sure that the sandbox gpio value is 0 or 1

with the current code, sandbox_gpio_get_value can return GPIOD_EXT_HIGH 
or GPIOD_PULL_UP, and it is strange (even if result in int).


with these 2 changes, you can add my  Reviewed-by


Regards

Patrick