[U-Boot] [PATCH] py: test_gpio: Add support for GPIO pytest

[Stephen Warren]

On 09/20/2017 01:55 AM, Michal Simek wrote:
> From: Vipul Kumar <vipul.kumar at xilinx.com>
> 
> This patch tests the gpio commands using the
> gpio data from boardenv file.
> 
> Also one test will show the default status of all the gpio's
> supported by the processor.

Nit: Wrap the commit description to something like 74 characters; the 
text above is far too narrow.

Nit: GPIO not gpio in any free-form text (here and in comments in the 
source file)

Ah, I'd briefly though about GPIO tests before, but was wondering how to 
integrate e.g. a USB GPIO device to the host PC and interface that with 
the target. Loopback GPIOs make this a lot easier:-)

> diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py

> +"""
> +Note: This test relies on boardenv_* containing configuration values to define
> +which the gpio available for testing. Without this, this test will be  automat-

Nit: Double space before automatically.

> +For example:
> +
> +# A list of gpio's that are going to be tested in order to validate the
> +# test.
> +env__gpio_val = {
> +     "gpio": [0,1,2],
> +}

Why is this a dictionary with only one key? Better to just assign the 
array directly to env__gpio_val, or put both the "gpio" and 
"list_of_gpios" into a single dictionary.

> +# A list of gpio's that are shorted on the hardware board and first gpio of
> +# each group is configured as input and the other as output. If the pins are
> +# not shorted properly, then the test will be fail.
> +env__gpio_input_output = {
> +     "list_of_gpios": [ [36, 37], [38, 39]],
> +}

Let's make the examples for env__gpio_val and env__gpio_input_output use 
the same GPIO numbers so that they match, and it's obvious how the two 
relate to each-other. I assume they're meant to?

Actually, why do we even need env__gpio_val if env__gpio_input_output 
already lists all the pairs of GPIOs?

Again, let's collapse this dict/list pair into just a list without the 
dict wrapper.

"gpio_val", "gpio", "gpio_input_output", and "list_of_gpios" aren't 
exactly great names. At least for "gpio_input_output"/"list_of_gpios", 
I'd suggest a more semantic name is "env__looped_back_gpios", which is a 
plain list (of pairs of GPIO numbers).

> +def gpio_input(u_boot_console, gpio):
> +    u_boot_console.run_command("gpio input %d" %gpio)

Nit: Space after % operator (the second % outside the string). Same 
comment in many other cases.

> +    response = u_boot_console.run_command("gpio status -a %d" %gpio)
> +    expected_response = "%d: input:" %gpio
> +    assert(expected_response in response)
> +
> +def gpio_set(u_boot_console, gpio):
> +    expected_response = "%d: output: 1" %gpio
> +    u_boot_console.run_command("gpio set %d" %gpio)
> +    response = u_boot_console.run_command("gpio status -a %d" %gpio)
> +    assert(expected_response in response)

Why not make all these functions do things in the same order; 
gpio_input() above sets expected_response immediately before the assert 
(which I think is the right place to do it since it relates to the 
assert and not the gpio set/clear/toggle command which is executed 
earlier), whereas the other functions set expected_response at the start 
of the function.

> + at pytest.mark.buildconfigspec("cmd_gpio")
> +def test_gpio_status(u_boot_console):
> +    response = u_boot_console.run_command("gpio status -a")
> +    expected_response = "0: input:"
> +    assert(expected_response in response)

This test assumes that GPIO 0 is expected to be configured as an input 
by default (at boot) on all HW that supports GPIOs. I don't think this 
is a valid assumption.

> + at pytest.mark.buildconfigspec("cmd_gpio")
> +def test_gpio(u_boot_console):
> +    f = u_boot_console.config.env.get('env__gpio_val', None)
> +    if not f:
> +        pytest.skip('No GPIO readable file to read')

What "file" is this referring to? A better message might be 
'env__gpio_val not defined for this board'.

> +    gpin = f.get("gpio", None)
> +
> +    for gpio in gpin:
> +        gpio_input(u_boot_console, gpio)
> +        gpio_set(u_boot_console, gpio)
> +        gpio_clear(u_boot_console, gpio)
> +        gpio_toggle(u_boot_console, gpio)

So this is simply to test executing those commands? Why not rely on 
testing them as a side-effect of the loopback test below? the only one 
untested is toggle, and that could be added to the loopback test.

> + at pytest.mark.buildconfigspec("cmd_gpio")
> +def test_gpio_input_output(u_boot_console):
> +    f = u_boot_console.config.env.get('env__gpio_input_output', None)
> +    if not f:
> +        pytest.skip('No GPIO readable file to read')
> +
> +    list_of_gpios = f.get("list_of_gpios", None)
> +
> +    flag = 0
> +    for list in list_of_gpios:
> +        for i in list:
> +            if flag == 0:
> +               gpio_in = i
> +               expected_response = "%d: input:" %gpio_in
> +               u_boot_console.run_command("gpio input %d" %gpio_in)
> +               response = u_boot_console.run_command("gpio status -a %d" %gpio_in)
> +               assert(expected_response in response)
> +               flag = 1
> +
> +            else:
> +               gpio_out = i
> +               expected_response = "%d: output:" %gpio_out
> +               u_boot_console.run_command("gpio set %d" %gpio_out)
> +               response = u_boot_console.run_command("gpio status -a %d" %gpio_out)
> +               assert(expected_response in response)
> +               flag = 0

This (the inner "for i in list") doesn't need to be a loop, since it's 
using flag to do completely different things on each iteration anyway, 
and that logic is pretty confusing. Instead, just do:

for list in list_of_gpios:
     gpio_in = list[0]
     configure input GPIO
     gpio_out = list[1]
     set gpio_out to 0
     validate gpio_in value is 0
     set gpio_out to 1
     validate gpio_in value is 1

(and "list" isn't a great variable name especially since it aliases with 
a Python type constructor function; gpio_pair would be better)

... and actually it might be better to do something like the following:

for every GPIO pair:
     set gpio_in to input
for every GPIO pair:
     set gpio_out to 0
for every GPIO pair:
     set gpio_out to 1
     for every GPIO pair:
         if inner gpio_in == output gpio_in:
             expected_val = 1
         else:
             expected_val = 0
         validate inner gpio_in is expected_val
     set gpio_out to 0

... since that will both test that each gpio_out value affects the 
associated gpio_in value, but also that no gpio_out value affects any 
unexpected/unassociated gpio_in value.