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.