Support ROHM BU27034 ALS sensor
This series adds support for ROHM BU27034 Ambient Light Sensor.
The BU27034 has configurable gain and measurement (integration) time settings. Both of these have inversely proportional relation to the sensor's intensity channel scale.
Many users only set the scale, which means that many drivers attempt to 'guess' the best gain+time combination to meet the scale. Usually this is the biggest integration time which allows setting the requested scale. Typically, increasing the integration time has better accuracy than increasing the gain, which often amplifies the noise as well as the real signal.
However, there may be cases where more responsive sensors are needed. So, in some cases the longest integration times may not be what the user prefers. The driver has no way of knowing this.
Hence, the approach taken by this series is to allow user to set both the scale and the integration time with following logic:
1. When scale is set, the existing integration time is tried to be maintained as a first priority. 1a) If the requested scale can't be met by current time, then also other time + gain combinations are searched. If scale can be met by some other integration time, then the new time may be applied. If the time setting is common for all channels, then also other channels must be able to maintain their scale with this new time (by changing their gain). The new times are scanned in the order of preference (typically the longest times first). 1b) If the requested scale can be met using current time, then only the gain for the channel is changed.
2. When the integration time change - scale is tried to be maintained. When integration time change is requested also gain for all impacted channels is adjusted so that the scale is not changed, or is chaned as little as possible. This is different from the RFCv1 where the request was rejected if suitable gain couldn't be found for some channel(s).
This logic is simple. When total gain (either caused by time or hw-gain) is doubled, the scale gets halved. Also, the supported times are given a 'multiplier' value which tells how much they increase the total gain.
However, when I wrote this logic in bu27034 driver, I made quite a few errors on the way - and driver got pretty big. As I am writing drivers for two other sensors (RGB C/IR + flicker BU27010 and RGB C/IR BU27008) with similar gain-time-scale logic I thought that adding common helpers for these computations might be wise. I hope this way all the bugs will be concentrated in one place and not in every individual driver ;)
Hence, this series also intriduces IIO gain-time-scale helpers (abbreviated as gts-helpers) + a couple of KUnit tests for the most hairy parts.
Speaking of which - testing the devm interfaces requires a 'dummy device'. I've learned that there has been at least two ways of handling this kind of a dependecy.
1) Using a root_device_[un]register() functions (with or without a wrapper)
2) Using dummy platform_device.
Way 2) is seen as abusing platform_devices to something they should not be used.
Way 1) is also seen sub-optimal - and after a discussion a 'kunit dummy device' is being worked on by David Gow: https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@go...
David's work relies on not yet in-tree kunit deferring API. Schedule for this work is - as always in case of upstream development - unkonwn. In order to be self-contained while still easily 'fixable when David's work is completed' this series introduces warappers named similar to what was suggested by david - and which are intended to have similar behaviour (automatic clean-up upon test completion). These wrappers do still use root-device APIs underneath but this should be fixed by David's work.
Finally, these added helpers do provide some value also for drivers which only: a) allow gain change or b) allow changing both the time and gain while trying to maintain the scale.
For a) we provide the gain - selector (register value) table format + selector to gain look-ups, gain <-> scale conversions and the available scales helpers.
For latter case we also provide the time-tables, and actually all the APIs should be usable by setting the time multiplier to 1. (not testeted thoroughly though).
The patch 1/7 introduces the helpers for creating/dropping a test device for devm-tests. It can be applied alone.
The patch 4/7 (IIO GTS tests) also depends on the patch 1/7 (and also other patches in the series).
Rest of the series should be Ok to be applied with/without the patches 1/7 and 4/7 - although the 4/7 (which depends on 1/7) would be "nice to have" together with the rest of the series for the testability reasons.
Revision history: v5 => v6: - Just a minor fixes in iio-gts-helpers and bu27034 driver. - Kunit device helper for a test device creation. - IIO GTS tests use kunit device helper.
v4 => v5: Mostly fixes to review comments from Andy and Jonathan. - more accurate change-log in individual patches - copy code from DRM test helper instead of moving it to simplify merging - document all exported GTS helpers. - inline a few GTS helpers - use again Milli lux for the bu27034 with RAW IIO_LIGHT channel and scale - Fix bug from added in v4 bu27034 time setting.
v3 => v4: (Still mostly fixes to review comments from Andy and Jonathan) - more accurate change-log in individual patches - dt-binding and maintainer patches unchanged. - dropped unused helpers and converted ones currently used only internally to static. - extracted "dummy device" creation helpers from DRM tests. - added tests for devm APIs - dropped scale for PROCESSED channel in BU27034 and converted mLux values to luxes - dropped channel 2 GAIN setting which can't be done due to HW limitations.
v2 => v3: (Mostly fixes to review comments from Andy and Jonathan) - dt-binding and maintainer patches unchanged. - iio-gts-helper tests: Use namespaces - iio-gts-helpers + bu27034 plenty of changes. See more comprehensive changelog in individual patches.
RFCv1 => v2: dt-bindings: - Fix binding file name and id by using comma instead of a hyphen to separate the vendor and part names. gts-helpers: - fix include guardian - Improve kernel doc for iio_init_iio_gts. - Add iio_gts_scale_to_total_gain - Add iio_gts_total_gain_to_scale - Fix review comments from Jonathan - add documentation to few functions - replace 0xffffffffffffffffLLU by U64_MAX - some styling fixes - drop unnecessary NULL checks - order function arguments by in / out purpose - drop GAIN_SCALE_ITIME_MS() - Add helpers for available scales and times - Rename to iio-gts-helpers gts-tests: - add tests for available scales/times helpers - adapt to renamed iio-gts-helpers.h header bu27034-driver: - (really) protect read-only registers - fix get and set gain - buffered mode - Protect the whole sequences including meas_en/meas_dis to avoid messing up the enable / disable order - typofixes / doc improvements - change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US() - use more accurate scale for lux channel (milli lux) - provide available scales / integration times (using helpers). - adapt to renamed iio-gts-helpers.h file - bu27034 - longer lines in Kconfig - Drop bu27034_meas_en and bu27034_meas_dis wrappers. - Change device-name from bu27034-als to bu27034 MAINTAINERS: - Add iio-list
---
Matti Vaittinen (7): dt-bindings: iio: light: Support ROHM BU27034 iio: light: Add gain-time-scale helpers kunit: Add kunit wrappers for (root) device creation iio: test: test gain-time-scale helpers MAINTAINERS: Add IIO gain-time-scale helpers iio: light: ROHM BU27034 Ambient Light Sensor MAINTAINERS: Add ROHM BU27034
.../bindings/iio/light/rohm,bu27034.yaml | 46 + MAINTAINERS | 14 + drivers/iio/Kconfig | 3 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-gts-helper.c | 1057 ++++++++++++ drivers/iio/light/Kconfig | 14 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27034.c | 1496 +++++++++++++++++ drivers/iio/test/Kconfig | 14 + drivers/iio/test/Makefile | 1 + drivers/iio/test/iio-test-gts.c | 517 ++++++ include/kunit/device.h | 18 + include/linux/iio/iio-gts-helper.h | 206 +++ lib/kunit/Makefile | 3 +- lib/kunit/device.c | 36 + 15 files changed, 3426 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml create mode 100644 drivers/iio/industrialio-gts-helper.c create mode 100644 drivers/iio/light/rohm-bu27034.c create mode 100644 drivers/iio/test/iio-test-gts.c create mode 100644 include/kunit/device.h create mode 100644 include/linux/iio/iio-gts-helper.h create mode 100644 lib/kunit/device.c
base-commit: eeac8ede17557680855031c6f305ece2378af326
A few tests need to have a valid struct device. One such example is tests which want to be testing devm-managed interfaces.
Add kunit wrapper for root_device_[un]register(), which create a root device and also add a kunit managed clean-up routine for the device destruction upon test exit.
Special note: In some cases the device reference-count does not reach zero and devm-unwinding is not done if device is not sitting on a bus. The root_device_[un]register() are dealing with such devices and thus this interface may not be usable by all in its current form. More information can be found from: https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
The use of root-devices in the kunit helpers is intended to be an intermediate solution to allow tests which do not require device to sit on a bus avoid directly abusing the root_device_[un]register() while proper kunit device solution is being worked on. Related discussion can be found from: https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA...
Signed-off-by: Matti Vaittinen mazziesaccount@gmail.com
--- Change history: v5 => v6: - Kunit resource-managed root_device creation wrapper (new patch)
Please note: This patch uses root-devices (as was suggested) until there is a proper dummy device creation mechanism added in kunit. The root devices are embedded in kunit wrappers to simplify replacing the root-devices with proper solution when it is available.
David Gow has sent out an RFC[1] which should implement these helpers using not-yet-in-tree deferring API. This RFC aims to support kunit_device which should be _the right thing to do_. I added this implementation here because it may (or may not) take a while for the David's RFC to make it's way in-kernel. So, in order to not delay this series I added these helpers which use the existing kunit resource management for clean-up while the new deferring kunit API is not yet in place.
[1] https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@go...
--- include/kunit/device.h | 18 ++++++++++++++++++ lib/kunit/Makefile | 3 ++- lib/kunit/device.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 include/kunit/device.h create mode 100644 lib/kunit/device.c
diff --git a/include/kunit/device.h b/include/kunit/device.h new file mode 100644 index 000000000000..f02740b7583b --- /dev/null +++ b/include/kunit/device.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __KUNIT_DEVICE_H__ +#define __KUNIT_DEVICE_H__ + +#include <kunit/test.h> + +struct device; + +/* 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); + +#endif diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index cb417f504996..64449549b990 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -6,7 +6,8 @@ kunit-objs += test.o \ string-stream.o \ assert.o \ try-catch.o \ - executor.o + executor.o \ + device.o
ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o diff --git a/lib/kunit/device.c b/lib/kunit/device.c new file mode 100644 index 000000000000..425f6d62ebd7 --- /dev/null +++ b/lib/kunit/device.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <kunit/device.h> +#include <kunit/test.h> + +#include <linux/device.h> + +static void kunit_device_drop(struct kunit_resource *res) +{ + root_device_unregister(res->data); +} + +struct device *kunit_device_register(struct kunit *test, const char *name) +{ + struct device *dev; + + dev = root_device_register(name); + if (IS_ERR_OR_NULL(dev)) + return dev; + + return kunit_alloc_resource(test, NULL, kunit_device_drop, GFP_KERNEL, + dev); +} +EXPORT_SYMBOL_GPL(kunit_device_register); + +static bool kunit_device_match(struct kunit *test, struct kunit_resource *res, + void *match_data) +{ + return res->data == match_data; +} + +void kunit_device_unregister(struct kunit *test, struct device *dev) +{ + kunit_destroy_resource(test, kunit_device_match, dev); +} +EXPORT_SYMBOL_GPL(kunit_device_unregister);
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
A few tests need to have a valid struct device. One such example is tests which want to be testing devm-managed interfaces.
Add kunit wrapper for root_device_[un]register(), which create a root device and also add a kunit managed clean-up routine for the device destruction upon test exit.
I really do not like this as a "root device" is a horrible hack and should only be used if you have to hang other devices off of it and you don't have a real device to tie those devices to.
Here you are abusing it and attempting to treat it as a real device, which it is not at all, because:
Special note: In some cases the device reference-count does not reach zero and devm-unwinding is not done if device is not sitting on a bus. The root_device_[un]register() are dealing with such devices and thus this interface may not be usable by all in its current form. More information can be found from: https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
See, not a real device, doesn't follow normal "struct device" rules and lifetimes, don't try to use it for a test as it will only cause problems and you will be forced to work around that in a test.
Do the right thing here, create a fake bus and add devices to it.
Heck, I'll even write that code if you want it, what's the requirement, something like: struct device *kunit_device_create(struct kunit *test, const char *name); void kunit_device_destroy(struct device *dev); ?
Why do you want a "match" function? You don't provide documentation here for it so I have no idea.
Anything else needed?
The use of root-devices in the kunit helpers is intended to be an intermediate solution to allow tests which do not require device to sit on a bus avoid directly abusing the root_device_[un]register() while proper kunit device solution is being worked on. Related discussion can be found from: https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA...
Again, no, please let's not get this wrong now and say "we will fix this later" as that's not how kernel development should work...
thanks,
greg k-h
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
A few tests need to have a valid struct device. One such example is tests which want to be testing devm-managed interfaces.
Add kunit wrapper for root_device_[un]register(), which create a root device and also add a kunit managed clean-up routine for the device destruction upon test exit.
I really do not like this as a "root device" is a horrible hack and should only be used if you have to hang other devices off of it and you don't have a real device to tie those devices to.
Here you are abusing it and attempting to treat it as a real device, which it is not at all, because:
Special note: In some cases the device reference-count does not reach zero and devm-unwinding is not done if device is not sitting on a bus. The root_device_[un]register() are dealing with such devices and thus this interface may not be usable by all in its current form. More information can be found from: https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
See, not a real device, doesn't follow normal "struct device" rules and lifetimes, don't try to use it for a test as it will only cause problems and you will be forced to work around that in a test.
Ok. I understood using the root-device has been a work-around in some other tests. Thus continuing use it for tests where we don't need the bus until we have a proper alternative was suggested by David.
Do the right thing here, create a fake bus and add devices to it.
Heck, I'll even write that code if you want it, what's the requirement, something like: struct device *kunit_device_create(struct kunit *test, const char *name); void kunit_device_destroy(struct device *dev);
Thanks for the offer Greg. This, however, is being already worked on by David. I don't want to step on his toes by writing the same thing, nor do I think I should be pushing him to rush on his work.
Why do you want a "match" function? You don't provide documentation here for it so I have no idea.
Anything else needed?
The use of root-devices in the kunit helpers is intended to be an intermediate solution to allow tests which do not require device to sit on a bus avoid directly abusing the root_device_[un]register() while proper kunit device solution is being worked on. Related discussion can be found from: https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA...
Again, no, please let's not get this wrong now and say "we will fix this later" as that's not how kernel development should work...
Ok. In that case I need to drop the tests from the series until we get the new APIs in place. It really sucks but I guess I understand the rationale for not wanting to "intermediate" solutions merged. Yes, I hoped it'd be Ok as David is already working on it - but I was still kind of expecting your response. This is why I made it very clear in the cover-letter and this commit message what is suggested here.
Jonathan, should I re-spin the series without patches 3/7 and 5/7 or can you please review this and I'll just drop those for the next version?
Thanks for the review Greg, I think this case is now "closed".
Yours, -- Matti
On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
A few tests need to have a valid struct device. One such example is tests which want to be testing devm-managed interfaces.
Add kunit wrapper for root_device_[un]register(), which create a root device and also add a kunit managed clean-up routine for the device destruction upon test exit.
I really do not like this as a "root device" is a horrible hack and should only be used if you have to hang other devices off of it and you don't have a real device to tie those devices to.
Here you are abusing it and attempting to treat it as a real device, which it is not at all, because:
Special note: In some cases the device reference-count does not reach zero and devm-unwinding is not done if device is not sitting on a bus. The root_device_[un]register() are dealing with such devices and thus this interface may not be usable by all in its current form. More information can be found from: https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
See, not a real device, doesn't follow normal "struct device" rules and lifetimes, don't try to use it for a test as it will only cause problems and you will be forced to work around that in a test.
Ok. I understood using the root-device has been a work-around in some other tests. Thus continuing use it for tests where we don't need the bus until we have a proper alternative was suggested by David.
Do the right thing here, create a fake bus and add devices to it.
Heck, I'll even write that code if you want it, what's the requirement, something like: struct device *kunit_device_create(struct kunit *test, const char *name); void kunit_device_destroy(struct device *dev);
Thanks for the offer Greg. This, however, is being already worked on by David. I don't want to step on his toes by writing the same thing, nor do I think I should be pushing him to rush on his work.
Ok, David, my offer stands, if you want me to write this I will be glad to do so.
thanks,
greg k-h
Thanks, Gred and Matti.
On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
A few tests need to have a valid struct device. One such example is tests which want to be testing devm-managed interfaces.
Add kunit wrapper for root_device_[un]register(), which create a root device and also add a kunit managed clean-up routine for the device destruction upon test exit.
I really do not like this as a "root device" is a horrible hack and should only be used if you have to hang other devices off of it and you don't have a real device to tie those devices to.
Here you are abusing it and attempting to treat it as a real device, which it is not at all, because:
There's a tradeoff here in that we want to pull in as little code (and complexity) as possible into unit tests, both to make them as easy as possible to write, and to make them as targeted as possible. This leads to a lot of tests manually filling out structures with the bare minimum to get the code being tested to run, and creating "root devices" seems to have been a convenient way of doing that while only registering _one_ thing in a big global list per test. So having a "real device" is not something I'd consider a _necessity_ in designing these sorts of helpers: a convincing enough fake is sometimes better.
That being said, now that I've got a bit of a better understanding of the device model, I agree that "root devices" are not an ideal solution, even if they are a convenient one. I'm still not thrilled by the prospect of having to register extra things like drivers to get these simple tests to work, but when wrapped behind helpers, it's a nice solution in practice.
Special note: In some cases the device reference-count does not reach zero and devm-unwinding is not done if device is not sitting on a bus. The root_device_[un]register() are dealing with such devices and thus this interface may not be usable by all in its current form. More information can be found from: https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
See, not a real device, doesn't follow normal "struct device" rules and lifetimes, don't try to use it for a test as it will only cause problems and you will be forced to work around that in a test.
I think there's still some confusion around exactly what these issues are, and if they're indeed bugs or expected behaviour. I think it hangs off the question of what uses of a device with no driver attached are valid. My initial reading through of the various bits of the devres implementation seemed to imply that using it with such an unattached device was supported, but I'm less certain now. In any case, Maxime's tests in the other thread are a good starting point to clarify this behaviour, and if we use the bus-based KUnit helpers, it won't matter either way.
Ok. I understood using the root-device has been a work-around in some other tests. Thus continuing use it for tests where we don't need the bus until we have a proper alternative was suggested by David.
Do the right thing here, create a fake bus and add devices to it.
Heck, I'll even write that code if you want it, what's the requirement, something like: struct device *kunit_device_create(struct kunit *test, const char *name); void kunit_device_destroy(struct device *dev);
Thanks for the offer Greg. This, however, is being already worked on by David. I don't want to step on his toes by writing the same thing, nor do I think I should be pushing him to rush on his work.
Ok, David, my offer stands, if you want me to write this I will be glad to do so.
I'm happy to keep working on this, but would definitely appreciate your feedback.
I've put my work-in-progress code here: https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21...
It creates a "kunit" bus, and adds a few helpers to create both devices and drivers on that bus, and clean them up when the test exits. It seems to work on all of the tests which used root_device_register so far (except those -- only test_iio_find_closest_gain_low so far -- which create multiple devices with the same name, as the driver name won't be unique), and the drm tests work fine when ported to it as well.
There's still a lot of cleanup to do and questions which need answering, including: - Working out how best to provide an owning module (it's currently just kunit, but probably should be the module which contains the actual tests) - Improving the API around drivers: probably exposing the helper to create a driver, and add a way of cleaning it up early. - Properly testing it with modules, not just with kunit.py (including looking at what actually appears in sysfs) - Experimenting with using probe, etc, callbacks. - Cleaning up lots of ugly, still experimental code, documenting, testing, etc.
The thought of trying to expand the match function to support, e.g., devicetree occurred to me, but I _think_ that devicetree-based tests are probably still better off using a platform_device. Regardless, it can probably wait to a follow-up
In any case, does this seem like the right way forward?
Cheers, -- David
Hi David & Greg and thanks for working with this!
On 3/28/23 15:45, David Gow wrote:
Thanks, Gred and Matti.
On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
I'm happy to keep working on this, but would definitely appreciate your feedback.
I've put my work-in-progress code here: https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21...
It creates a "kunit" bus, and adds a few helpers to create both devices and drivers on that bus, and clean them up when the test exits. It seems to work on all of the tests which used root_device_register so far (except those -- only test_iio_find_closest_gain_low so far -- which create multiple devices with the same name, as the driver name won't be unique),
I wouldn't worry about it for as long as it's just because an iio-gts test does something silly. Those tests are currently only in my personal playground and changing those tests should be pretty trivial.
And right after saying that - the test_iio_find_closest_gain_low test does
a) register a 'test' device b) perform test on devm_ API c) unregister the 'test' device
d) register a 'test' device (same name as at step a) e) perform test on devm_ API f) unregister the 'test' device
My assumption is that the test device would be gone after step c) because there should be no references to it anywhere. Hence, I wonder why registering at step d) fails? (Or did I misunderstand something?)
and the drm tests work fine when ported to it as well.
There's still a lot of cleanup to do and questions which need answering, including:
- Working out how best to provide an owning module (it's currently
just kunit, but probably should be the module which contains the actual tests)
Maybe there is something I am not seeing but how about wrapping the kunit_device_register() in a macro and getting the THIS_MODULE in caller's context?
In any case, does this seem like the right way forward?
I am by no means an expert on this but this does look good to me. I would keep this as clean, lean and simple as possible in order to keep understanding / debugging the problems exposed by the tests as simple as possible. At some point someone is wondering why a test fails, and ends up looking through these helpers to ensure problem is no lurking there... Hence, I'd kept the code there in minimum - meaning, I might not add kunit class or even a driver until tests require that. (Even if it would not look as good in the sysfs - as far as I understand the kunit sysfs entries are a 'test feature' which should not be present in 'production systems'. This is not an excuse to make things bad - but (in my opinion) this is a good reason to prioritize simplicity.
Anyways, thanks for the work!
Yours, -- Matti
On Tue, 28 Mar 2023 at 21:22, Matti Vaittinen mazziesaccount@gmail.com wrote:
Hi David & Greg and thanks for working with this!
On 3/28/23 15:45, David Gow wrote:
Thanks, Gred and Matti.
On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
I'm happy to keep working on this, but would definitely appreciate your feedback.
I've put my work-in-progress code here: https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21...
It creates a "kunit" bus, and adds a few helpers to create both devices and drivers on that bus, and clean them up when the test exits. It seems to work on all of the tests which used root_device_register so far (except those -- only test_iio_find_closest_gain_low so far -- which create multiple devices with the same name, as the driver name won't be unique),
I wouldn't worry about it for as long as it's just because an iio-gts test does something silly. Those tests are currently only in my personal playground and changing those tests should be pretty trivial.
Yeah: this isn't anything to worry about. It's as much my note as to "what works with this code as-is", rather than indicative of a more fundamental flaw.
And right after saying that - the test_iio_find_closest_gain_low test does
a) register a 'test' device b) perform test on devm_ API c) unregister the 'test' device
d) register a 'test' device (same name as at step a) e) perform test on devm_ API f) unregister the 'test' device
My assumption is that the test device would be gone after step c) because there should be no references to it anywhere. Hence, I wonder why registering at step d) fails? (Or did I misunderstand something?)
This is because I'm now creating a struct device_driver as well as a device, and it's not being cleaned up until the end of the test.
The two solutions here would be to either: - Add a way to clean up the driver earlier. (Shouldn't be too hard, I just haven't got around to it yet), or: - Use the same struct device_driver for both tests (there's a new kunit_device_register_with_driver which allows you to pass a custom driver in, rather than creating your own)
I think the latter's probably better, but I'll probably implement both as I clean up the API further.
and the drm tests work fine when ported to it as well.
There's still a lot of cleanup to do and questions which need answering, including:
- Working out how best to provide an owning module (it's currently
just kunit, but probably should be the module which contains the actual tests)
Maybe there is something I am not seeing but how about wrapping the kunit_device_register() in a macro and getting the THIS_MODULE in caller's context?
Yeah: that's probably what I'll do. The other possibility is to store the module pointer in the struct kunit context.
In any case, does this seem like the right way forward?
I am by no means an expert on this but this does look good to me. I would keep this as clean, lean and simple as possible in order to keep understanding / debugging the problems exposed by the tests as simple as possible. At some point someone is wondering why a test fails, and ends up looking through these helpers to ensure problem is no lurking there... Hence, I'd kept the code there in minimum - meaning, I might not add kunit class or even a driver until tests require that. (Even if it would not look as good in the sysfs - as far as I understand the kunit sysfs entries are a 'test feature' which should not be present in 'production systems'. This is not an excuse to make things bad - but (in my opinion) this is a good reason to prioritize simplicity.
I agree with you that it's best to avoid complexity for as long as we reasonably can. I think that there are enough things which would benefit from having the driver stuff to make it worth having _something_ there, particularly since it makes this a more "normal" device, so hopefully will be less surprising.
For sysfs, it's one of those things which shows up pretty rarely in day-to-day KUnit use, as most people are using the kunit.py script which has all of the tests built-in, and no userspace to look at sysfs from. That being said, that doesn't cover all use cases, and I definitely would rather not make sysfs unusably ugly for everyone else, so I'm happy to tidy it up. (I'm not planning to do a kunit class at the moment, though.)
I _think_ this approach (once the details of the API and implementation are tidied up a bit) should sit in about the sweet spot for complexity, assuming there's nothing horribly wrong with it I haven't noticed. I suspect we're better off leaving some of the more advanced use-cases to implement their own way of instantiating devices, at least for now, and focus on getting these common cases right.
Cheers, -- David
Hi,
On Tue, Mar 28, 2023 at 08:45:09PM +0800, David Gow wrote:
Ok. I understood using the root-device has been a work-around in some other tests. Thus continuing use it for tests where we don't need the bus until we have a proper alternative was suggested by David.
Do the right thing here, create a fake bus and add devices to it.
Heck, I'll even write that code if you want it, what's the requirement, something like: struct device *kunit_device_create(struct kunit *test, const char *name); void kunit_device_destroy(struct device *dev);
Thanks for the offer Greg. This, however, is being already worked on by David. I don't want to step on his toes by writing the same thing, nor do I think I should be pushing him to rush on his work.
Ok, David, my offer stands, if you want me to write this I will be glad to do so.
I'm happy to keep working on this, but would definitely appreciate your feedback.
I've put my work-in-progress code here: https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21...
It creates a "kunit" bus, and adds a few helpers to create both devices and drivers on that bus, and clean them up when the test exits. It seems to work on all of the tests which used root_device_register so far (except those -- only test_iio_find_closest_gain_low so far -- which create multiple devices with the same name, as the driver name won't be unique), and the drm tests work fine when ported to it as well.
There's still a lot of cleanup to do and questions which need answering, including:
- Working out how best to provide an owning module (it's currently
just kunit, but probably should be the module which contains the actual tests)
- Improving the API around drivers: probably exposing the helper to
create a driver, and add a way of cleaning it up early.
I'm not sure we need to give the ability for a test to pass a driver. I'd expect there's two main use-cases: either we want to test a function that uses a device as an argument, or we want to probe the whole driver and test it.
The former is covered by kunit_device_register(), and I'd expect the latter to be covered by the work Stephen is doing, where we will load an entire overlay, which will in turn probe the driver.
- Properly testing it with modules, not just with kunit.py (including
looking at what actually appears in sysfs)
- Experimenting with using probe, etc, callbacks.
- Cleaning up lots of ugly, still experimental code, documenting, testing, etc.
The thought of trying to expand the match function to support, e.g., devicetree occurred to me, but I _think_ that devicetree-based tests are probably still better off using a platform_device. Regardless, it can probably wait to a follow-up
Yeah, probing the entire driver will require to instantiate the device the driver expects, hence why relying on the overlays also makes a lot of sense.
Maxime
On 3/27/23 14:27, Matti Vaittinen wrote:
Support ROHM BU27034 ALS sensor
The patch 1/7 introduces the helpers for creating/dropping a test device for devm-tests. It can be applied alone.
Sorry folks. The wrapper is 3/7 not 1/7
The patch 4/7 (IIO GTS tests) also depends on the patch 1/7 (and also other patches in the series).
Rest of the series should be Ok to be applied with/without the patches 1/7 and 4/7 - although the 4/7 (which depends on 1/7) would be "nice to have" together with the rest of the series for the testability reasons.
snip.
Matti Vaittinen (7): dt-bindings: iio: light: Support ROHM BU27034 iio: light: Add gain-time-scale helpers kunit: Add kunit wrappers for (root) device creation
Here.
iio: test: test gain-time-scale helpers MAINTAINERS: Add IIO gain-time-scale helpers iio: light: ROHM BU27034 Ambient Light Sensor MAINTAINERS: Add ROHM BU27034
Yours, -- Matti
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Greg Kroah-Hartman -
Matti Vaittinen -
Maxime Ripard