| Message ID | 20220311162445.346685-10-vincent.whitchurch@axis.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | roadtest: a driver testing framework | expand |
On Fri, Mar 11, 2022 at 05:24:44PM +0100, Vincent Whitchurch wrote: This looks like it could be useful, modulo the general concerns with mocking stuff. I've not looked at the broader framework stuff in any meanigful way. > + @classmethod > + def setUpClass(cls) -> None: > + insmod("tps6286x-regulator") Shouldn't this get figured out when the device gets created in DT (if it doesn't I guess the tests found a bug...)? > + def setUp(self) -> None: > + self.driver = I2CDriver("tps6286x") > + self.hw = Hardware("i2c") > + self.hw.load_model(TPS62864) This feels like there could be some syntactic sugar to say "create this I2C device" in one call? In general a lot of the frameworkish stuff feels verbose. > + def test_voltage(self) -> None: > + with ( > + self.driver.bind(self.dts["normal"]), > + PlatformDriver("reg-virt-consumer").bind( > + "tps62864_normal_consumer" > + ) as consumerdev, > + ): > + maxfile = consumerdev.path / "max_microvolts" > + minfile = consumerdev.path / "min_microvolts" > + > + write_int(maxfile, 1675000) > + write_int(minfile, 800000) > + > + mock = self.hw.update_mock() > + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 5) > + mock.assert_reg_write_once(self, REG_VOUT1, 0x50) > + mock.reset_mock() Some comments about the assertations here would seem to be in order. It's not altogether clear what this is testing - it looks to be verifying that the regulator is enabled with the voltage set to 800mV mapping to 0x50 in VOUT1 but I'm not sure that the idle reader would pick that up. > + mV = 1000 > + data = [ > + (400 * mV, 0x00), > + (900 * mV, 0x64), > + (1675 * mV, 0xFF), > + ] > + > + for voltage, val in data: > + write_int(minfile, voltage) > + mock = self.hw.update_mock() > + mock.assert_reg_write_once(self, REG_VOUT1, val) > + mock.reset_mock() For covering regulators in general (especially those like this that use the generic helpers) I'd be inclined to go through every single voltage that can be set which isn't so interesting for this driver with it's linear voltage control but more interesting for something that's not continuous. I'd also put a cross check in that the voltage and enable state that's reported via the read interface in sysfs is the one that we think we've just set, that'd validate that the framework's model of what's going on matches both what the driver did to the "hardware" and what the running kernel thinks is going on so we're joined up top to bottom (for the regulator framework the read values come from the driver so it is actually covering the driver). This all feels like it could readily be factored out into a generic helper, much as the actual drivers are especially when they're more data driven. Ideally with the ability to override the default I/O operations for things with sequences that need to be followed instead of just a bitfield to update. Callbacks to validate enable state, voltage, mode and so on in the hardware. If we did that then rather than open coding every single test for every single device we could approach things at the framework level and give people working on a given device a pile of off the shelf tests which are more likely to catch things that an individual driver author might've missed, it also avoids the test coverage being more laborious than writing the actual driver. This does raise the questions I mentioned about how useful the testing really is of course, even more so when someone works out how to generate the data tables for the test and the driver from the same source, but that's just generally an issue for mocked tests at the conceptual level and clearly it's an approach that's fairly widely used and people get value from.
On Fri, Mar 11, 2022 at 06:06:54PM +0000, Mark Brown wrote: > On Fri, Mar 11, 2022 at 05:24:44PM +0100, Vincent Whitchurch wrote: > This looks like it could be useful, modulo the general concerns with > mocking stuff. I've not looked at the broader framework stuff in any > meanigful way. Thank you for having a look! Here's a bit of background story about how I used this particular test, which hopefully shows an example of where I've seen the benefits of mocking hardware: When I wrote this tps6286x driver a while ago, I tested it as one usually does, checking with i2cdump that the correct register values are written, measuring the voltages with a multimeter, rising and repeating with different devicetree properties, and so on. (This framework didn't exist at that point.) Later, when preparing the driver for mainline submission, I wanted a quick way to check that any changes or cleanups that I needed to do during that process didn't invalidate all my and others' earlier testing. The easiest way to do that was to ensure that the driver continued to write the same bits in the same registers when given the same set of inputs and devicetree properties, and that is where I found the mocking to be useful. In this case where there is no external input, the testing could of course have all been done manually with the real hardware, but there was little reason to do so when the hardware was the one factor which had not changed. The abilitly to create multiple devices with different devicetree properties (such as fast mode on/off) was a real time-saver too. > > + @classmethod > > + def setUpClass(cls) -> None: > > + insmod("tps6286x-regulator") > > Shouldn't this get figured out when the device gets created in DT (if it > doesn't I guess the tests found a bug...)? The system isn't set up to load modules automatically. The reason for this is to give the test cases full control of when the module is loaded and unload, since the tests could want to load the module with specific options. Also, the framework splits up logs and shows errors that occurs during each specific test if the tests fail, and this would become less useful if all modules for all the devices in the devicetree get loaded on startup when the devicetree is parsed and one of the modules failed to load or crashed when loaded. > > > + def setUp(self) -> None: > > + self.driver = I2CDriver("tps6286x") > > + self.hw = Hardware("i2c") > > + self.hw.load_model(TPS62864) > > This feels like there could be some syntactic sugar to say "create this > I2C device" in one call? In general a lot of the frameworkish stuff > feels verbose. Yes, I agree this could be simplified. I think the update_mock/reset_mock dance could also potentially be simplified with a with statement. Beyond that, yes, there is some boilerplate setup for each test to bind the devices. This can differ between drivers and subsystems so I'm not sure how much could be shared, but I guess some of them could be separated out into a internal function for this particular test. > > + def test_voltage(self) -> None: > > + with ( > > + self.driver.bind(self.dts["normal"]), > > + PlatformDriver("reg-virt-consumer").bind( > > + "tps62864_normal_consumer" > > + ) as consumerdev, > > + ): > > + maxfile = consumerdev.path / "max_microvolts" > > + minfile = consumerdev.path / "min_microvolts" > > + > > + write_int(maxfile, 1675000) > > + write_int(minfile, 800000) > > + > > + mock = self.hw.update_mock() > > + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 5) > > + mock.assert_reg_write_once(self, REG_VOUT1, 0x50) > > + mock.reset_mock() > > Some comments about the assertations here would seem to be in order. > It's not altogether clear what this is testing - it looks to be > verifying that the regulator is enabled with the voltage set to 800mV > mapping to 0x50 in VOUT1 but I'm not sure that the idle reader would > pick that up. Yes, I will add some comments. I also made some of the bit fields use constants in some of the other driver, that could be done here too. > > > + mV = 1000 > > + data = [ > > + (400 * mV, 0x00), > > + (900 * mV, 0x64), > > + (1675 * mV, 0xFF), > > + ] > > + > > + for voltage, val in data: > > + write_int(minfile, voltage) > > + mock = self.hw.update_mock() > > + mock.assert_reg_write_once(self, REG_VOUT1, val) > > + mock.reset_mock() > > For covering regulators in general (especially those like this that use > the generic helpers) I'd be inclined to go through every single voltage > that can be set which isn't so interesting for this driver with it's > linear voltage control but more interesting for something that's not > continuous. That could be useful in some cases, but if going through all the voltages in a loop requires that the test implement the exact same voltage-to-bitfield conversion function as the driver, then the benefit of that part of the test is unclear. That's the reason why for example the OPT3001 test uses known values from the datasheet rather than just copying the conversion function in the driver to Python. > I'd also put a cross check in that the voltage and enable > state that's reported via the read interface in sysfs is the one that we > think we've just set, that'd validate that the framework's model of > what's going on matches both what the driver did to the "hardware" and > what the running kernel thinks is going on so we're joined up top to > bottom (for the regulator framework the read values come from the > driver so it is actually covering the driver). Makes sense, I can add that. > This all feels like it could readily be factored out into a generic > helper, much as the actual drivers are especially when they're more data > driven. Ideally with the ability to override the default I/O operations > for things with sequences that need to be followed instead of just a > bitfield to update. Callbacks to validate enable state, voltage, mode > and so on in the hardware. If we did that then rather than open coding > every single test for every single device we could approach things at > the framework level and give people working on a given device a pile of > off the shelf tests which are more likely to catch things that an > individual driver author might've missed, it also avoids the test > coverage being more laborious than writing the actual driver. Things could certainly be factored out in the future, but I'm a bit wary of attempting to do that when we have a test for only one regulator driver, and a very minimal regulator driver at that. > This does raise the questions I mentioned about how useful the testing > really is of course, even more so when someone works out how to generate > the data tables for the test and the driver from the same source, but > that's just generally an issue for mocked tests at the conceptual level > and clearly it's an approach that's fairly widely used and people get > value from. For the regulator drivers which are purely-data driven such as the ones mostly implemented by setting the various fields in struct regulator_desc along with the helpers in the framework, it could perhaps be useful to implement kunit tests in the regulator subsystem which test that using the various fields actually results in the expected consumer-visible behaviour with the regulator API. Then, for the indivudal drivers themselves, roadtests could cover things like probe handling, functions implemented without using helpers, checks that the correct variant's registers are used in drivers supporting multiple variants, custom devicetree properties, interrupt handling, and the like.
On Thu, Mar 17, 2022 at 04:13:26PM +0100, Vincent Whitchurch wrote: > On Fri, Mar 11, 2022 at 06:06:54PM +0000, Mark Brown wrote: > > > + @classmethod > > > + def setUpClass(cls) -> None: > > > + insmod("tps6286x-regulator") > > Shouldn't this get figured out when the device gets created in DT (if it > > doesn't I guess the tests found a bug...)? > The system isn't set up to load modules automatically. The reason for > this is to give the test cases full control of when the module is loaded > and unload, since the tests could want to load the module with specific > options. That seems like the uncommon case which could remove the module if it explicitly needed it. > Also, the framework splits up logs and shows errors that occurs during > each specific test if the tests fail, and this would become less useful > if all modules for all the devices in the devicetree get loaded on > startup when the devicetree is parsed and one of the modules failed to > load or crashed when loaded. That sounds like stuff that would be covered already by normal boot testing? > > > + write_int(minfile, voltage) > > > + mock = self.hw.update_mock() > > > + mock.assert_reg_write_once(self, REG_VOUT1, val) > > > + mock.reset_mock() > > For covering regulators in general (especially those like this that use > > the generic helpers) I'd be inclined to go through every single voltage > > that can be set which isn't so interesting for this driver with it's > > linear voltage control but more interesting for something that's not > > continuous. > That could be useful in some cases, but if going through all the > voltages in a loop requires that the test implement the exact same > voltage-to-bitfield conversion function as the driver, then the benefit > of that part of the test is unclear. That's the reason why for example > the OPT3001 test uses known values from the datasheet rather than just > copying the conversion function in the driver to Python. That's just a generic problem with mocking though - ultimately you have to type the same values into the mock and the driver somehow, it's just a question of if you type in all the values or some of the values and if you use the same format to type them in. My inclination is to get better coverage since it makes it more likely that the interesting cases will be picked up, and you can make tests that do things like combine multiple settings which might turn something up. > > This all feels like it could readily be factored out into a generic > > helper, much as the actual drivers are especially when they're more data > > driven. Ideally with the ability to override the default I/O operations > > for things with sequences that need to be followed instead of just a > > bitfield to update. Callbacks to validate enable state, voltage, mode > > and so on in the hardware. If we did that then rather than open coding > > every single test for every single device we could approach things at > > the framework level and give people working on a given device a pile of > > off the shelf tests which are more likely to catch things that an > > individual driver author might've missed, it also avoids the test > > coverage being more laborious than writing the actual driver. > Things could certainly be factored out in the future, but I'm a bit wary > of attempting to do that when we have a test for only one regulator > driver, and a very minimal regulator driver at that. My thinking here is that since the driver is so minimal and data driven it's clear that any tests for it can also be generalised to cover at the very least all similarly data driven drivers. > > This does raise the questions I mentioned about how useful the testing > > really is of course, even more so when someone works out how to generate > > the data tables for the test and the driver from the same source, but > > that's just generally an issue for mocked tests at the conceptual level > > and clearly it's an approach that's fairly widely used and people get > > value from. > For the regulator drivers which are purely-data driven such as the ones > mostly implemented by setting the various fields in struct > regulator_desc along with the helpers in the framework, it could perhaps > be useful to implement kunit tests in the regulator subsystem which test > that using the various fields actually results in the expected > consumer-visible behaviour with the regulator API. > Then, for the indivudal drivers themselves, roadtests could cover things > like probe handling, functions implemented without using helpers, checks > that the correct variant's registers are used in drivers supporting > multiple variants, custom devicetree properties, interrupt handling, and > the like. That would be the more obvious approach than roadtest, but that's what's there in the patch I was reviewing so... There is also the fact that the external pattern for the operations is the same no matter if they're for a simple data driven driver or one using custom ops so you should really be able to get the core coverage for every driver set up in a way that lets you plug in a model of how the I/O performing each operation is expected to work and then have the framework crunch through combinations of actions to make sure that all the corner cases check out.
On Thu, Mar 17, 2022 at 05:53:22PM +0000, Mark Brown wrote: > On Thu, Mar 17, 2022 at 04:13:26PM +0100, Vincent Whitchurch wrote: > > On Fri, Mar 11, 2022 at 06:06:54PM +0000, Mark Brown wrote: > > > > > + @classmethod > > > > + def setUpClass(cls) -> None: > > > > + insmod("tps6286x-regulator") > > > > Shouldn't this get figured out when the device gets created in DT (if it > > > doesn't I guess the tests found a bug...)? > > > The system isn't set up to load modules automatically. The reason for > > this is to give the test cases full control of when the module is loaded > > and unload, since the tests could want to load the module with specific > > options. > > That seems like the uncommon case which could remove the module if it > explicitly needed it. Another reason was to get the tests to test module unloading since I've seen a lot of especially new driver writers forget to test that, but I realise that for most normal drivers that should be mostly covered by the fact that we test device unbinding. So I went ahead and implemented this and it seems to work. As you hinted earlier, this also means that the modalias stuff gets tested, and as we know that's been broken in the recent past for a bunch of drivers, so that's another advantage to automatic module loading, besides the boilerplate reduction in the tests.
diff --git a/tools/testing/roadtest/roadtest/tests/regulator/__init__.py b/tools/testing/roadtest/roadtest/tests/regulator/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tools/testing/roadtest/roadtest/tests/regulator/config b/tools/testing/roadtest/roadtest/tests/regulator/config new file mode 100644 index 000000000000..b2b503947e70 --- /dev/null +++ b/tools/testing/roadtest/roadtest/tests/regulator/config @@ -0,0 +1,4 @@ +CONFIG_REGULATOR=y +CONFIG_REGULATOR_DEBUG=y +CONFIG_REGULATOR_VIRTUAL_CONSUMER=y +CONFIG_REGULATOR_TPS6286X=m diff --git a/tools/testing/roadtest/roadtest/tests/regulator/test_tps62864.py b/tools/testing/roadtest/roadtest/tests/regulator/test_tps62864.py new file mode 100644 index 000000000000..f7db4293d840 --- /dev/null +++ b/tools/testing/roadtest/roadtest/tests/regulator/test_tps62864.py @@ -0,0 +1,187 @@ +# SPDX-License-Identifier: GPL-2.0-only +# Copyright Axis Communications AB + +from typing import Any, Final + +from roadtest.backend.i2c import SimpleSMBusModel +from roadtest.core.devicetree import DtFragment, DtVar +from roadtest.core.hardware import Hardware +from roadtest.core.modules import insmod, rmmod +from roadtest.core.suite import UMLTestCase +from roadtest.core.sysfs import ( + I2CDriver, + PlatformDriver, + read_str, + write_int, + write_str, +) + +REG_VOUT1: Final = 0x01 +REG_VOUT2: Final = 0x02 +REG_CONTROL: Final = 0x03 +REG_STATUS: Final = 0x05 + + +class TPS62864(SimpleSMBusModel): + def __init__(self, **kwargs: Any) -> None: + super().__init__( + # From datasheet section 8.6 Register map + # XXX does not match reality -- recheck + regs={ + REG_VOUT1: 0x64, + REG_VOUT2: 0x64, + REG_CONTROL: 0x00, + REG_STATUS: 0x00, + }, + regbytes=1, + **kwargs, + ) + + +class TestTPS62864(UMLTestCase): + dts = DtFragment( + src=""" +#include <dt-bindings/regulator/ti,tps62864.h> + +&i2c { + regulator@$normal$ { + compatible = "ti,tps62864"; + reg = <0x$normal$>; + + regulators { + tps62864_normal: SW { + regulator-name = "+0.85V"; + regulator-min-microvolt = <400000>; + regulator-max-microvolt = <1675000>; + regulator-allowed-modes = <TPS62864_MODE_NORMAL TPS62864_MODE_FPWM>; + }; + }; + }; + + regulator@$fpwm$ { + compatible = "ti,tps62864"; + reg = <0x$fpwm$>; + + regulators { + tps62864_fpwm: SW { + regulator-name = "+0.85V"; + regulator-min-microvolt = <400000>; + regulator-max-microvolt = <1675000>; + regulator-initial-mode = <TPS62864_MODE_FPWM>; + }; + }; + }; +}; + +/ { + tps62864_normal_consumer { + compatible = "regulator-virtual-consumer"; + default-supply = <&tps62864_normal>; + }; + + tps62864_fpwm_consumer { + compatible = "regulator-virtual-consumer"; + default-supply = <&tps62864_fpwm>; + }; +}; + """, + variables={ + "normal": DtVar.I2C_ADDR, + "fpwm": DtVar.I2C_ADDR, + }, + ) + + @classmethod + def setUpClass(cls) -> None: + insmod("tps6286x-regulator") + + @classmethod + def tearDownClass(cls) -> None: + rmmod("tps6286x-regulator") + + def setUp(self) -> None: + self.driver = I2CDriver("tps6286x") + self.hw = Hardware("i2c") + self.hw.load_model(TPS62864) + + def tearDown(self) -> None: + self.hw.close() + + def test_voltage(self) -> None: + with ( + self.driver.bind(self.dts["normal"]), + PlatformDriver("reg-virt-consumer").bind( + "tps62864_normal_consumer" + ) as consumerdev, + ): + maxfile = consumerdev.path / "max_microvolts" + minfile = consumerdev.path / "min_microvolts" + + write_int(maxfile, 1675000) + write_int(minfile, 800000) + + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 5) + mock.assert_reg_write_once(self, REG_VOUT1, 0x50) + mock.reset_mock() + + mV = 1000 + data = [ + (400 * mV, 0x00), + (900 * mV, 0x64), + (1675 * mV, 0xFF), + ] + + for voltage, val in data: + write_int(minfile, voltage) + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_VOUT1, val) + mock.reset_mock() + + write_int(minfile, 0) + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 0) + mock.reset_mock() + + def test_modes(self) -> None: + with ( + self.driver.bind(self.dts["normal"]), + PlatformDriver("reg-virt-consumer").bind( + "tps62864_normal_consumer" + ) as consumerdev, + ): + modefile = consumerdev.path / "mode" + write_str(modefile, "fast") + + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 4) + mock.reset_mock() + + write_str(modefile, "normal") + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 0) + mock.reset_mock() + + def test_dt_force_pwm(self) -> None: + with ( + self.driver.bind(self.dts["fpwm"]), + PlatformDriver("reg-virt-consumer").bind( + "tps62864_fpwm_consumer" + ) as consumerdev, + ): + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 4) + mock.reset_mock() + + modefile = consumerdev.path / "mode" + self.assertEquals(read_str(modefile), "fast") + + maxfile = consumerdev.path / "max_microvolts" + minfile = consumerdev.path / "min_microvolts" + + write_int(maxfile, 1675000) + write_int(minfile, 800000) + + mock = self.hw.update_mock() + mock.assert_reg_write_once(self, REG_CONTROL, 1 << 5 | 1 << 4) + mock.reset_mock()
Add a roadtest for the recently-added tps62864 regulator driver. It tests voltage setting, mode setting, as well as devicetree mode translation. It uses the recently-added devicetree support in regulator-virtual-consumer. All the variants supported by the driver have identical register interfaces so only one test/model is added. It requires the following patches which are, as of writing, not in mainline: - regulator: Add support for TPS6286x https://lore.kernel.org/lkml/20220204155241.576342-3-vincent.whitchurch@axis.com/ - regulator: virtual: add devicetree support https://lore.kernel.org/lkml/20220301111831.3742383-4-vincent.whitchurch@axis.com/ Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- .../roadtest/tests/regulator/__init__.py | 0 .../roadtest/roadtest/tests/regulator/config | 4 + .../roadtest/tests/regulator/test_tps62864.py | 187 ++++++++++++++++++ 3 files changed, 191 insertions(+) create mode 100644 tools/testing/roadtest/roadtest/tests/regulator/__init__.py create mode 100644 tools/testing/roadtest/roadtest/tests/regulator/config create mode 100644 tools/testing/roadtest/roadtest/tests/regulator/test_tps62864.py