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 = 1000data = [(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.