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

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

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.