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

Hi Patrick,

On Mon, 8 Feb 2021 at 11:13, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:

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 sjg@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).

I think that is a hack to work around not having a 'bool' type, but we do have one in U-Boot, so I feel it is better to use it, instead using !!

See the 'bool val;' in that function.

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

Regards, Simon