[PATCH v6 0/7] Support ROHM BU27034 ALS sensor

[PATCH v6 0/7] Support ROHM BU27034 ALS sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 9846 bytes --]

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@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

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
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4911 bytes --]

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-dTQ@mail.gmail.com/

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@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

---
 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);
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/7] Support ROHM BU27034 ALS sensor

[Matti Vaittinen]

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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Greg Kroah-Hartman]

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-dTQ@mail.gmail.com/

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

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Matti Vaittinen]

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-dTQ@mail.gmail.com/
> 
> 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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Greg Kroah-Hartman]

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

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5571 bytes --]

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/#F0

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Matti Vaittinen]

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/#F0
> 
> 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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5174 bytes --]

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/#F0
> >
> > 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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

[Maxime Ripard]

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/#F0
> 
> 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