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

[Michal Simek]

Hi Stephen,

On 20.9.2017 19:05, Stephen Warren wrote:
> 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:-)

good.

> 
>> 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?

The reason for two values is that you do that loopback between certain
pins. It means you have to list pairs.
I have tested this on the board where I can choose whatever
configuration by external wires. Also this list is not the same with the
above one because you are choosing which one is loopback but not all can
be done as loopback.

Also I have tested that you can specify 36,37 as one pair and then 37,36
pair which simply saying which pin is used as output and which as input.

I don't think it is right to say that both of them can bidirectional
because at least in our chip with have some pins which are just output
only and input only.


> 
> 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).

Vipul: Please look at these comments.

> 
>> +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.

ok. I think this can be removed - the key point of this test was to list
all of them to see the status. It means return to shell should be fine.

Vipul: Please comment the rest of Stephen's comments and prepare v2.

Thanks,
Michal