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

[Simon Glass]

Hi Patrick,

On Mon, 8 Feb 2021 at 11:13, Patrick DELAUNAY
<patrick.delaunay at 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 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).

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