On 09/20/2017 01:55 AM, Michal Simek wrote:
From: Vipul Kumar <vipul.ku...@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.
+@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.
+@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.
+@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
