diff mbox series

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

Message ID 20210115140500.846307-8-sjg@chromium.org
State Superseded
Delegated to: Simon Glass
Headers show
Series gpio: Update and simplify the uclass API | expand

Commit Message

Simon Glass Jan. 15, 2021, 2:04 p.m. UTC
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(-)

Comments

Patrick DELAUNAY Jan. 21, 2021, 9:33 a.m. UTC | #1
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
diff mbox series

Patch

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)
+
 /**
  * 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;
+	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)