Re: [RFC v1 09/10] regulator: tps62864: add roadtest

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.