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