Support ROHM BU27034 ALS sensor

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/[email protected]/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 =] 

ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add dt-bindings.

---
v2 =>
- No changes

Changes since RFCv1 => v2
- Fix binding file name and id by using comma instead of a hyphen to separate the vendor and part names.

---
 .../bindings/iio/light/rohm,bu27034.yaml      | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml

new file mode 100644
index 000000000000..30a109a1bf3b
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm,bu27034.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27034 ambient light sensor
+
+maintainers:
+  - Matti Vaittinen <[email protected]>
+
+description: |
+  ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes
+  capable of detecting a very wide range of illuminance. Typical application
+  is adjusting LCD and backlight power of TVs and mobile phones.
+  https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf
+
+properties:
+  compatible:
+    const: rohm,bu27034
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      light-sensor@38 {
+        compatible = "rohm,bu27034";
+        reg = <0x38>;
+        vdd-supply = <&vdd>;
+      };
+    };
+
+...

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 =] 

Some light sensors can adjust both the HW-gain and integration time. There are cases where adjusting the integration time has similar impact to the scale of the reported values as gain setting has.

IIO users do typically expect to handle scale by a single writable 'scale' entry. Driver should then adjust the gain/time accordingly.

It however is difficult for a driver to know whether it should change gain or integration time to meet the requested scale. Usually it is preferred to have longer integration time which usually improves accuracy, but there may be use-cases where long measurement times can be an issue. Thus it can be preferable to allow also changing the integration time - but mitigate the scale impact by also changing the gain underneath. Eg, if integration time change doubles the measured values, the driver can reduce the HW-gain to half.

The theory of the computations of gain-time-scale is simple. However, some people (undersigned) got that implemented wrong for more than once.

Add some gain-time-scale helpers in order to not dublicate errors in all drivers needing these computations.

---
Currently it is only BU27034 using these in this series. I am however working with drivers for RGB sensors BU27008 and BU27010 which have similar [gain - integration time - scale] - relation. I hope sending those follows soon after the BU27034 is done.

Changes:
v5 => v6:
- drop unnecessary comments about using validity tables after free

v4 => v5:
- drop DEFAULT_SYMBOL_NAMESPACE again
- spellcheck
- always build availability tables
- combine table-build and gts-init. Drop no longer needed interfaces
- check for invalid values at init
- document limitations of gain, time, selector and multiplier values
- document all exported functions
- move and inline valid gain / valid time and related functions to header.
NOTE: As a side-effect this makes the internal iteration functions public as well.
- drop unlikely() from "cold path"
- add iio_gts_find_new_gain_by_old_gain_time()

v3 => v4:
- doc styling
- use memset to zero the helper struct at init
- drop unnecessary min calculation at iio_find_closest_gain_low()
- use namespace to all exports
- many minor stylings
- make available outside iio/light (move code to drivers/iio and move the header under include
- rename to look like other files under drivers/iio (s/iio/industrialio)
- drop unused functions
- don't export only internally used functions and make them static Note, I decided to keep iio_gts_total_gain_to_scale() exported as it is currently needed by the tests outside the helpers.

v2 => v3: (mostly fixes based on review by Andy)
- Fix typos
- Styling fixes
- Use namespace for exported symbols
- Protect allocs against argument overflow
- Fix include protection name
- add types.h inclusion and struct device forward declaration

RFCv1 => v2:
- 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

---
 drivers/iio/Kconfig                   |    3 +
 drivers/iio/Makefile                  |    1 +
 drivers/iio/industrialio-gts-helper.c | 1057 +++++++++++++++++++++++++
 include/linux/iio/iio-gts-helper.h    |  206 +++++
 4 files changed, 1267 insertions(+)
 create mode 100644 drivers/iio/industrialio-gts-helper.c
 create mode 100644 include/linux/iio/iio-gts-helper.h

index b190846c3dc2..52eb46ef84c1 100644
@@ -30,6 +30,9 @@ config IIO_CONFIGFS
 	  (e.g. software triggers). For more info see
 	  Documentation/iio/iio_configfs.rst.
 
+config IIO_GTS_HELPER
+	tristate
+
 config IIO_TRIGGER
 	bool "Enable triggered sampling support"
 	help

index 3be08cdadd7e..9622347a1c1b 100644
@@ -9,6 +9,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
 industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
 
 obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
+obj-$(CONFIG_IIO_GTS_HELPER) += industrialio-gts-helper.o
 obj-$(CONFIG_IIO_SW_DEVICE) += industrialio-sw-device.o
 obj-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
 obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o

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 =] 

Here we need to document another return value denote whether the @new_gain was updated.

Looks like I keep discussing with myself. This however was not a good solution as we might detect non integer gain to be required in the gain_get_scale_fraction(). And deciding if that function should return -ERANGE or -EINVAL got things even worse.

So, the take N (where N is a positive integer, much greater than 1) is that I'll do:

int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
int old_gain, int old_time_sel, int new_time_sel, int *new_gain)

{
    const struct iio_itime_sel_mul *itime_old, *itime_new;
    u64 scale;
    int ret;

    *new_gain = -1;

and add return value doc like:

* Return: 0 if an exactly matching supported new gain was found. When
a
* non-zero value is returned, the @new_gain will be set to a negative or
* positive value. The negative value means that no gain could be computed.
* Positive value will be the "best possible new gain there could be".
There
* can be two reasons why finding the "best possible" new gain is not
deemed
* successful. 1) This new value cannot be supported by the hardware.
2) The new
* gain required to maintain the scale would not be an integer. In this
case,
* the "best possible" new gain will be a floored optimal gain, which
may or
* may not be supported by the hardware.

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

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/

---
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/[email protected]/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

new file mode 100644
index 000000000000..f02740b7583b
@@ -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

index cb417f504996..64449549b990 100644
@@ -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

new file mode 100644
index 000000000000..425f6d62ebd7
@@ -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 =] 

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:

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?

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

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.

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. 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! ~~

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.

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.

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.

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

Hi David & Greg and thanks for working with this!

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?)

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?

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! ~~

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.

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.

Yeah: that's probably what I'll do. The other possibility is to store the module pointer in the struct kunit context.

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,

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.

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

Some light sensors can adjust both the HW-gain and integration time. There are cases where adjusting the integration time has similar impact to the scale of the reported values as gain setting has.

IIO users do typically expect to handle scale by a single writable 'scale' entry. Driver should then adjust the gain/time accordingly.

It however is difficult for a driver to know whether it should change gain or integration time to meet the requested scale. Usually it is preferred to have longer integration time which usually improves accuracy, but there may be use-cases where long measurement times can be an issue. Thus it can be preferable to allow also changing the integration time - but mitigate the scale impact by also changing the gain underneath. Eg, if integration time change doubles the measured values, the driver can reduce the HW-gain to half.

The theory of the computations of gain-time-scale is simple. However, some people (undersigned) got that implemented wrong for more than once. Hence some gain-time-scale helpers were introduced.

Add some simple tests to verify the most hairy functions.

---
Changes:
v5 => v6:
- Use kunit root-device registration wrapper

v4 => v5:
- remove empty lines from Kconfig
- adapt to drop of the non devm iio_init
- test also init with couple of invalid tables

v3 => v4:
- use dummy device to test devm interfaces
- adapt to the new header location
- drop tests for dropped interfaces

v2 => v3:
- Use namespace for iio-gts-helpers

RFCv1 => v2:
- add tests for available scales/times helpers
- adapt to renamed iio-gts-helpers.h header

---
 drivers/iio/test/Kconfig        |  14 +
 drivers/iio/test/Makefile       |   1 +
 drivers/iio/test/iio-test-gts.c | 517 ++++++++++++++++++++++++++++++++
 3 files changed, 532 insertions(+)
 create mode 100644 drivers/iio/test/iio-test-gts.c

index 0b6e4e278a2f..33cca49c8058 100644
@@ -4,6 +4,20 @@
 #
 
 # Keep in alphabetical order
+config IIO_GTS_KUNIT_TEST
+	tristate "Test IIO formatting functions" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	select IIO_GTS_HELPER
+	select TEST_KUNIT_DEVICE_HELPERS
+	default KUNIT_ALL_TESTS
+	help
+	  build unit tests for the IIO light sensor gain-time-scale helpers.
+
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N. Keep in alphabetical order
+
 config IIO_RESCALE_KUNIT_TEST
 	tristate "Test IIO rescale conversion functions" if !KUNIT_ALL_TESTS
 	depends on KUNIT && IIO_RESCALE

index d76eaf36da82..e9a4cf1ff57f 100644
@@ -6,4 +6,5 @@
 # Keep in alphabetical order
 obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o
 obj-$(CONFIG_IIO_FORMAT_KUNIT_TEST) += iio-test-format.o
+obj-$(CONFIG_IIO_GTS_KUNIT_TEST) += iio-test-gts.o
 CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)

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 =] 

Add myself as a maintainer for IIO light sensor helpers (helpers for maintaining the scale while adjusting intergration time or gain) and related Kunit tests.

---
RFCv1 =>
- No changes

---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

index ec57c42ed544..6ec9326f4ce9 100644
@@ -9938,6 +9938,14 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
 F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.yaml
 F:	drivers/iio/adc/envelope-detector.c
 
+IIO LIGHT SENSOR GAIN-TIME-SCALE HELPERS
+M:	Matti Vaittinen <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/iio/light/gain-time-scale-helper.c
+F:	drivers/iio/light/gain-time-scale-helper.h
+F:	drivers/iio/test/iio-test-gts.c
+
 IIO MULTIPLEXER
 M:	Peter Rosin <[email protected]>
 L:	[email protected]

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 =] 

ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. In addition, 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment alone can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property that can be used for adjusting the measurement time. Time adjustment will also cause the driver to try to adjust the GAIN so that the overall scale is kept as close to the original as possible.

---
v5 => v6:
- Use the multiplication with overflow check from overflow.h
- Use FIELD_PREP()

Changes
v4 => v5:
- spellcheck
- back to mlux again
- lux channel PROCESSED => RAW
- use new devm_init for GTS and drop explicit table building
- styling
- drop unnecessary loop in gain setting
- don't unnecessarily delay returning error
- fix integration time change compensation which was broken by v4 change allowing to use the (55 mS) in the time tables. (Rounding error when computing new gain based on times not multipliers).

v3 => v4:
- use min_t() for division by zero check
- adapt to new GTS helper header location
- calculate luxes not milli luxes
- drop scale for PROCESSED channel
- comment improvements
- do not allow changing gain (scale) for channel 2.
- 'tie' channel 2 scale to channel 0 scale
This is because channel 0 and channel 2 GAIN settings share part of the bits in the register. This means that setting one will also impact the other. The v3 of the patches attempted to work-around this by only disallowing the channel 2 gain setting to set the bits which were shared with channel 0 gain. This does not work because setting channel 0 gain (which was allowed to set also the shared bits) could result unsupported bit combinations for channel 2 gain. Thus it is safest to always set also the channel 2 gain to same value as channel 0 gain.
- Use the correct integration time (55 mS) in the gain table as the calcuations can be done based on the time multiplier.
- styling

v2 => v3:
- commit message update and typofixes
- switch warning messages to dbg
- drop incorrect comment about unchanged scales
- return 'no new data' if valid bit read failed
- shorten the 'div by zero' checks
- don't use u32 pointer when int * is epected in lux calculation
- add a comment clarifying why it is safe to return int from lux calculation
- simplify read_raw() by refactoring the measurement start / stop in another function and dropping the goto based unlocking.
- Styling fixes
- select IIO_BUFFER and IIO_KFIFO_BUF
- Alphabetical order of header includes
- Split multipication w/ overflow check to own function
- Do not hang in read_raw() if sensor does not return valid sample
- Spelling fix
- Do not require fwnode
- Use namespace for gts helpers

RFCv1 => v2:
- (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

---
 drivers/iio/light/Kconfig        |   14 +
 drivers/iio/light/Makefile       |    1 +
 drivers/iio/light/rohm-bu27034.c | 1496 ++++++++++++++++++++++++++++++
 3 files changed, 1511 insertions(+)
 create mode 100644 drivers/iio/light/rohm-bu27034.c

index 0d4447df7200..6fa31fcd71a1 100644
@@ -289,6 +289,20 @@ config JSA1212
 	  To compile this driver as a module, choose M here:
 	  the module will be called jsa1212.
 
+config ROHM_BU27034
+	tristate "ROHM BU27034 ambient light sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_GTS_HELPER
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	help
+	  Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034
+	  is an ambient light sesnor with 3 channels and 3 photo diodes capable
+	  of detecting a very wide range of illuminance.
+	  Typical application is adjusting LCD and backlight power of TVs and
+	  mobile phones.
+
 config RPR0521
 	tristate "ROHM RPR0521 ALS and proximity sensor driver"
 	depends on I2C

index d74d2b5ff14c..985f6feaccd4 100644
@@ -38,6 +38,7 @@ obj-$(CONFIG_MAX44009)		+= max44009.o
 obj-$(CONFIG_NOA1305)		+= noa1305.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o
+obj-$(CONFIG_ROHM_BU27034)	+= rohm-bu27034.o
 obj-$(CONFIG_RPR0521)		+= rpr0521.o
 obj-$(CONFIG_SI1133)		+= si1133.o
 obj-$(CONFIG_SI1145)		+= si1145.o

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 =] 

...

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Add myself as a maintainer for ROHM BU27034 ALS driver.

---
Changes
v2 =>
- No changes

sRFCv1 => v2:
- Add iio-list

---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

index 6ec9326f4ce9..3f13466e50fd 100644
@@ -18100,6 +18100,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/bh1750.yaml
 F:	drivers/iio/light/bh1750.c
 
+ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
+M:	Matti Vaittinen <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	drivers/iio/light/rohm-bu27034.c
+
 ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
 M:	Marek Vasut <[email protected]>
 L:	[email protected]

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 =] 

Sorry folks. The wrapper is 3/7 not 1/7

snip.

Here.

Yours,
-- Matti

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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