[PATCH 07/15] gpio: sandbox: Use a separate flag for the value

[Patrick DELAUNAY <patrick.delaunay@foss.st.com>]

Hi Simon,

On 1/15/21 3:04 PM, Simon Glass wrote:
> At present with the sandbox GPIO driver it is not possible to change the
> value of GPIOD_IS_OUT_ACTIVE unless the GPIO is an output. This makes it
> hard to test changing the flags since we need to be aware of the internal
> workings of the driver.
>
> The feature is designed to aid testing.
>
> Split this feature out into a separate sandbox-specific flag, so that the
> flags can change unimpeded. This will make it easier to allow updating the
> flags in a future patch.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   arch/sandbox/include/asm/gpio.h |  5 ++++
>   drivers/gpio/sandbox.c          | 43 +++++++++++++++------------------
>   2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/arch/sandbox/include/asm/gpio.h b/arch/sandbox/include/asm/gpio.h
> index 20d78296551..3f267797644 100644
> --- a/arch/sandbox/include/asm/gpio.h
> +++ b/arch/sandbox/include/asm/gpio.h
> @@ -23,6 +23,11 @@
>    */
>   #include <asm-generic/gpio.h>
>   
> +/* Our own private GPIO flags, which musn't conflict with GPIOD_... */
> +#define GPIOD_EXT_HIGH		BIT(20)	/* external source is high (else low) */
> +
> +#define GPIOD_SANDBOX_MASK	BIT(20)
> +


This internal SANDBOX is starting at 20 to avoid possible conflict with? 
GPIOD_

It should start at BIT(32) and decreasing order to allow more possible

GPIOD for SANDOX or in include/asm/gpio.h


>   /**
>    * Return the simulated value of a GPIO (used only in sandbox test code)
>    *
> diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
> index 9ce5d823505..52e73e2300a 100644
> --- a/drivers/gpio/sandbox.c
> +++ b/drivers/gpio/sandbox.c
> @@ -59,12 +59,12 @@ static int get_gpio_flag(struct udevice *dev, unsigned int offset, ulong flag)
>   static int set_gpio_flag(struct udevice *dev, unsigned int offset, ulong flag,
>   			 int value)
>   {
> -	ulong *gpio = get_gpio_flags(dev, offset);
> +	struct gpio_state *state = get_gpio_state(dev, offset);
>   
>   	if (value)
> -		*gpio |= flag;
> +		state->flags |= flag;
>   	else
> -		*gpio &= ~flag;
> +		state->flags &= ~flag;
>   
>   	return 0;
>   }
> @@ -75,14 +75,19 @@ static int set_gpio_flag(struct udevice *dev, unsigned int offset, ulong flag,
>   
>   int sandbox_gpio_get_value(struct udevice *dev, unsigned offset)
>   {
> +	struct gpio_state *state = get_gpio_state(dev, offset);
> +
>   	if (get_gpio_flag(dev, offset, GPIOD_IS_OUT))
>   		debug("sandbox_gpio: get_value on output gpio %u\n", offset);
> -	return get_gpio_flag(dev, offset, GPIOD_IS_OUT_ACTIVE);
> +
> +	return state->flags & GPIOD_EXT_HIGH ? true : false;
>   }
>   
>   int sandbox_gpio_set_value(struct udevice *dev, unsigned offset, int value)
>   {
> -	return set_gpio_flag(dev, offset, GPIOD_IS_OUT_ACTIVE, value);
> +	set_gpio_flag(dev, offset, GPIOD_IS_OUT_ACTIVE | GPIOD_EXT_HIGH, value);
> +
> +	return 0;
>   }
>   
>   int sandbox_gpio_get_direction(struct udevice *dev, unsigned offset)
> @@ -93,19 +98,25 @@ int sandbox_gpio_get_direction(struct udevice *dev, unsigned offset)
>   int sandbox_gpio_set_direction(struct udevice *dev, unsigned offset, int output)
>   {
>   	set_gpio_flag(dev, offset, GPIOD_IS_OUT, output);
> -	set_gpio_flag(dev, offset, GPIOD_IS_IN, !(output));
> +	set_gpio_flag(dev, offset, GPIOD_IS_IN, !output);
>   
>   	return 0;
>   }
>   
>   ulong sandbox_gpio_get_flags(struct udevice *dev, uint offset)
>   {
> -	return *get_gpio_flags(dev, offset);
> +	ulong flags = *get_gpio_flags(dev, offset);
> +
> +	return flags & ~GPIOD_SANDBOX_MASK;
>   }
>   
>   int sandbox_gpio_set_flags(struct udevice *dev, uint offset, ulong flags)
>   {
> -	*get_gpio_flags(dev, offset) = flags;
> +	struct gpio_state *state = get_gpio_state(dev, offset);
> +
> +	if (flags & GPIOD_IS_OUT_ACTIVE)
> +		flags |= GPIOD_EXT_HIGH;


when you set GPIO out active, you force the EXT_HIGH for sandbox

but if the the test request LOW output it is not managed ?

I just ask if it is normal...

here the EXT_HIGH bit could be cleared with:

 ? else

      flags &= ~GPIOD_EXT_HIGH;

> +	state->flags = (state->flags & GPIOD_SANDBOX_MASK) | flags;
>   
>   	return 0;
>   }
> @@ -188,23 +199,9 @@ static int sb_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
>   
>   static int sb_gpio_update_flags(struct udevice *dev, uint offset, ulong newf)
>   {
> -	ulong *flags;
> -
>   	debug("%s: offset:%u, flags = %lx\n", __func__, offset, newf);
>   
> -	flags = get_gpio_flags(dev, offset);
> -
> -	/*
> -	 * For testing purposes keep the output value when switching to input.
> -	 * This allows us to manipulate the input value via the gpio command.
> -	 */
> -	if (newf & GPIOD_IS_IN)
> -		*flags = (newf & ~GPIOD_IS_OUT_ACTIVE) |
> -			(*flags & GPIOD_IS_OUT_ACTIVE);
> -	else
> -		*flags = newf;
> -
> -	return 0;
> +	return sandbox_gpio_set_flags(dev, offset, newf);
>   }
>   
>   static int sb_gpio_get_flags(struct udevice *dev, uint offset, ulong *flagsp)


Patrick