On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen mazziesaccount@gmail.com wrote:
On 3/24/23 08:34, David Gow wrote:
On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen mazziesaccount@gmail.com wrote:
On 3/23/23 18:36, Maxime Ripard wrote:
On Thu, Mar 23, 2023 at 03:02:03PM +0200, Matti Vaittinen wrote:
On 3/23/23 14:29, Maxime Ripard wrote:
On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
Ok. Fair enough. Besides, if the root-device was sufficient - then I would actually not see the need for a helper. People could in that case directly use the root_device_register(). So, if helpers are provided they should be backed up by a device with a bus then.
I think there is _some_ value in helpers even without a bus, but it's much more limited:
- It's less confusing if KUnit test devices are using kunit labelled
structs and functions.
- Helpers could use KUnit's resource management API to ensure any
device created is properly unregistered and removed when the test exits (particularly if it exits early due to, e.g., an assertion).
Ah. That's true. Being able to abort the test on error w/o being forced to do a clean-up dance for the dummy device would be convenient.
I've played around implementing those with a proper struct kunit_device and the automatic cleanup on test failure, and thus far it -- like root_device_register -- works for all of the tests except the drm-test-managed one.
So if we really wanted to, we could use KUnit-specific helpers for just those tests which currently work with root_device_register(), but if we're going to try to implement a KUnit bus -- which I think is at least worth investigating -- I'd rather not either hold up otherwise good tests on helper development, or rush a helper out only to have to change it a lot when we see exactly what the bus implementation would look like.
It's easy for me to agree.
As I said, in my very specific IIO related test the test device does not need a bus. Hence I'll drop the 'generic helpers' from this series.
I think that sounds like a good strategy for now, and we can work on a set of 'generic helpers' which have an associated bus and struct kunit_device in the meantime. If we can continue to use root_device_register until those are ready, that'd be very convenient.
Would it be a tiny bit more acceptable if we did add a very simple:
#define kunit_root_device_register(name) root_device_register(name) #define kunit_root_device_unregister(dev) root_device_unregister(dev)
to include/kunit/device.h (or somesuch)
This should help us later to at least spot the places where root_device_[un]register() is abused and (potentially mass-)covert them to use the proper helpers when they're available.
Great idea.
The code I've been playing with has the following in include/kunit/device.h:
/* Register a new device against a KUnit test. */ struct device *kunit_device_register(struct kunit *test, const char *name); /* Unregister a device created by kunit_device_register() early (i.e., before test cleanup). */ void kunit_device_unregister(struct kunit *test, struct device *dev);
If we used the same names, and just forwarded them to root_device_register() and root_device_unregister() for now (discarding the struct kunit pointer), then I expect we could just swap out the implementation to gain the extra functionality.
It's a little less explicit, though, so I could see the value in using macros with "root_device" in the name to make the current implementation clearer, and the eventual change more obvious.
Cheers, -- David