[PATCH v4 0/8] Support ROHM BU27034 ALS sensor

[PATCH v4 0/8] Support ROHM BU27034 ALS sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 8764 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'. There were neat helpers in DRM tests for creating and freeing
such a device. This series moves those helpers to more generic location.
What is worth noting is that there is something similar ongoing in the
CCF territory:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
These efforts should be somehow coordinated in order to avoid any ajor
conflicts.

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

Revision history:
v3 => v4: (Stil ostly 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 (8):
  drm/tests: helpers: rename generic helpers
  kunit: drm/tests: move generic helpers
  dt-bindings: iio: light: Support ROHM BU27034
  iio: light: Add gain-time-scale helpers
  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/base/test/Kconfig                     |    5 +
 drivers/base/test/Makefile                    |    2 +
 drivers/base/test/test_kunit_device.c         |   83 +
 drivers/gpu/drm/Kconfig                       |    2 +
 .../gpu/drm/tests/drm_client_modeset_test.c   |    5 +-
 drivers/gpu/drm/tests/drm_kunit_helpers.c     |   69 -
 drivers/gpu/drm/tests/drm_managed_test.c      |    5 +-
 drivers/gpu/drm/tests/drm_modes_test.c        |    5 +-
 drivers/gpu/drm/tests/drm_probe_helper_test.c |    5 +-
 drivers/gpu/drm/vc4/Kconfig                   |    1 +
 drivers/gpu/drm/vc4/tests/vc4_mock.c          |    3 +-
 .../gpu/drm/vc4/tests/vc4_test_pv_muxing.c    |    9 +-
 drivers/iio/Kconfig                           |    3 +
 drivers/iio/Makefile                          |    1 +
 drivers/iio/industrialio-gts-helper.c         |  990 +++++++++++
 drivers/iio/light/Kconfig                     |   14 +
 drivers/iio/light/Makefile                    |    1 +
 drivers/iio/light/rohm-bu27034.c              | 1491 +++++++++++++++++
 drivers/iio/test/Kconfig                      |   16 +
 drivers/iio/test/Makefile                     |    1 +
 drivers/iio/test/iio-test-gts.c               |  461 +++++
 include/drm/drm_kunit_helpers.h               |    7 +-
 include/linux/iio/iio-gts-helper.h            |  113 ++
 25 files changed, 3265 insertions(+), 87 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
 create mode 100644 drivers/base/test/test_kunit_device.c
 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/linux/iio/iio-gts-helper.h


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 v4 3/8] dt-bindings: iio: light: Support ROHM BU27034

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

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

diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
new file mode 100644
index 000000000000..30a109a1bf3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
@@ -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 <mazziesaccount@gmail.com>
+
+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 =] 

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

[PATCH v4 4/8] iio: light: Add gain-time-scale helpers

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
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:
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 | 990 ++++++++++++++++++++++++++
 include/linux/iio/iio-gts-helper.h    | 113 +++
 4 files changed, 1107 insertions(+)
 create mode 100644 drivers/iio/industrialio-gts-helper.c
 create mode 100644 include/linux/iio/iio-gts-helper.h

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b190846c3dc2..52eb46ef84c1 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -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
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 3be08cdadd7e..9622347a1c1b 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -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
diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
new file mode 100644
index 000000000000..9494ea7cdbcf
--- /dev/null
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -0,0 +1,990 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/types.h>
+
+#define DEFAULT_SYMBOL_NAMESPACE IIO_GTS_HELPER
+
+/**
+ * iio_gts_get_gain - Convert scale to total gain
+ *
+ * Internal helper for converting scale to total gain.
+ *
+ * @max:	Maximum linearized scale. As an example, when scale is created
+ *		in magnitude of NANOs and max scale is 64.1 - The linearized
+ *		scale is 64 100 000 000.
+ * @scale:	Linearized scale to compte the gain for.
+ *
+ * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
+ *		is invalid.
+ */
+static int iio_gts_get_gain(const u64 max, const u64 scale)
+{
+	u64 full = max;
+	int tmp = 1;
+
+	if (scale > full || !scale)
+		return -EINVAL;
+
+	if (U64_MAX - full < scale) {
+		/* Risk of overflow */
+		if (full - scale < scale)
+			return 1;
+
+		full -= scale;
+		tmp++;
+	}
+
+	while (full > scale * (u64)tmp)
+		tmp++;
+
+	return tmp;
+}
+
+/**
+ * gain_get_scale_fraction - get the gain or time based on scale and known one
+ *
+ * @max:	Maximum linearized scale. As an example, when scale is created
+ *		in magnitude of NANOs and max scale is 64.1 - The linearized
+ *		scale is 64 100 000 000.
+ * @scale:	Linearized scale to compute the gain/time for.
+ * @known:	Either integration time or gain depending on which one is known
+ * @unknown:	Pointer to variable where the computed gain/time is stored
+ *
+ * Internal helper for computing unknown fraction of total gain.
+ * Compute either gain or time based on scale and either the gain or time
+ * depending on which one is known.
+ *
+ * Return:	0 on success.
+ */
+static int gain_get_scale_fraction(const u64 max, u64 scale, int known,
+				   int *unknown)
+{
+	int tot_gain;
+
+	tot_gain = iio_gts_get_gain(max, scale);
+	if (tot_gain < 0)
+		return tot_gain;
+
+	*unknown = tot_gain / known;
+
+	/* We require total gain to be exact multiple of known * unknown */
+	if (!*unknown || *unknown * known != tot_gain)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct iio_itime_sel_mul *
+iio_gts_find_itime_by_time(struct iio_gts *gts, int time)
+{
+	int i;
+
+	if (!gts->num_itime)
+		return NULL;
+
+	for (i = 0; i < gts->num_itime; i++)
+		if (gts->itime_table[i].time_us == time)
+			return &gts->itime_table[i];
+
+	return NULL;
+}
+
+static const struct iio_itime_sel_mul *
+iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel)
+{
+	int i;
+
+	for (i = 0; i < gts->num_itime; i++)
+		if (gts->itime_table[i].sel == sel)
+			return &gts->itime_table[i];
+
+	return NULL;
+}
+
+static int iio_gts_delinearize(u64 lin_scale, unsigned long scaler,
+			       int *scale_whole, int *scale_nano)
+{
+	int frac;
+
+	if (scaler > NANO)
+		return -EOVERFLOW;
+
+	if (!scaler)
+		return -EINVAL;
+
+	frac = do_div(lin_scale, scaler);
+
+	*scale_whole = lin_scale;
+	*scale_nano = frac * (NANO / scaler);
+
+	return 0;
+}
+
+static int iio_gts_linearize(int scale_whole, int scale_nano,
+			     unsigned long scaler, u64 *lin_scale)
+{
+	/*
+	 * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
+	 * multiplication followed by division to avoid overflow.
+	 */
+	if (scaler > NANO || !scaler)
+		return -EINVAL;
+
+	*lin_scale = (u64)scale_whole * (u64)scaler +
+		     (u64)(scale_nano / (NANO / scaler));
+
+	return 0;
+}
+
+/**
+ * iio_gts_total_gain_to_scale - convert gain to scale
+ * @gts:	Gain time scale descriptor
+ * @total_gain:	the gain to be converted
+ * @scale_int:	Pointer to integral part of the scale (typically val1)
+ * @scale_nano:	Pointer to fractional part of the scale (nano or ppb)
+ *
+ * Convert the total gain value to scale. NOTE: This does not separate gain
+ * generated by hwgain or integration time. It is up to caller to decide what
+ * part of the total gain is due to integration time and what due to hw-gain.
+ *
+ * Return: 0 on success. Negative errno on failure.
+ */
+int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
+				int *scale_int, int *scale_nano)
+{
+	u64 tmp;
+
+	tmp = gts->max_scale;
+
+	do_div(tmp, total_gain);
+
+	return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
+}
+EXPORT_SYMBOL_GPL(iio_gts_total_gain_to_scale);
+
+/**
+ * iio_init_iio_gts - Initialize the gain-time-scale helper
+ * @max_scale_int:	integer part of the maximum scale value
+ * @max_scale_nano:	fraction part of the maximum scale value
+ * @gain_tbl:		table describing supported gains
+ * @num_gain:		number of gains in the gaintable
+ * @tim_tbl:		table describing supported integration times. Provide
+ *			the integration time table sorted so that the preferred
+ *			integration time is in the first array index. The search
+ *			functions like the
+ *			iio_gts_find_time_and_gain_sel_for_scale() start search
+ *			from first provided time.
+ * @num_times:		number of times in the time table
+ * @gts:		pointer to the helper struct
+ *
+ * Initialize the gain-time-scale helper for use.
+ *
+ * Return: 0 on success.
+ */
+int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
+		     const struct iio_gain_sel_pair *gain_tbl, int num_gain,
+		     const struct iio_itime_sel_mul *tim_tbl, int num_times,
+		     struct iio_gts *gts)
+{
+	int ret;
+
+	memset(gts, 0, sizeof(*gts));
+
+	ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
+				   &gts->max_scale);
+	if (ret)
+		return ret;
+
+	gts->hwgain_table = gain_tbl;
+	gts->num_hwgain = num_gain;
+	gts->itime_table = tim_tbl;
+	gts->num_itime = num_times;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_init_iio_gts);
+
+/**
+ * iio_gts_purge_avail_scale_table - free-up the available scale tables
+ * @gts:	Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
+ * that the helpers for getting available scales like the
+ * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
+ * be only called after these helpers can no longer be called (Eg. after
+ * the iio-device has been deregistered).
+ */
+static void iio_gts_purge_avail_scale_table(struct iio_gts *gts)
+{
+	int i;
+
+	if (gts->per_time_avail_scale_tables) {
+		for (i = 0; i < gts->num_itime; i++)
+			kfree(gts->per_time_avail_scale_tables[i]);
+
+		kfree(gts->per_time_avail_scale_tables);
+		gts->per_time_avail_scale_tables = NULL;
+	}
+
+	kfree(gts->avail_all_scales_table);
+	gts->avail_all_scales_table = NULL;
+
+	gts->num_avail_all_scales = 0;
+}
+
+static int iio_gts_gain_cmp(const void *a, const void *b)
+{
+	return *(int *)a - *(int *)b;
+}
+
+static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
+{
+	int ret, i, j, new_idx, time_idx;
+	int *all_gains;
+	size_t gain_bytes;
+
+	for (i = 0; i < gts->num_itime; i++) {
+		/*
+		 * Sort the tables for nice output and for easier finding of
+		 * unique values.
+		 */
+		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
+		     NULL);
+
+		/* Convert gains to scales */
+		for (j = 0; j < gts->num_hwgain; j++) {
+			ret = iio_gts_total_gain_to_scale(gts, gains[i][j],
+							  &scales[i][2 * j],
+							  &scales[i][2 * j + 1]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
+	all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
+	if (!all_gains)
+		return -ENOMEM;
+
+	/*
+	 * We assume all the gains for same integration time were unique.
+	 * It is likely the first time table had greatest time multiplier as
+	 * the times are in the order of preference and greater times are
+	 * usually preferred. Hence we start from the last table which is likely
+	 * to have the smallest total gains.
+	 */
+	time_idx = gts->num_itime - 1;
+	memcpy(all_gains, gains[time_idx], gain_bytes);
+	new_idx = gts->num_hwgain;
+
+	while (time_idx--) {
+		for (j = 0; j < gts->num_hwgain; j++) {
+			int candidate = gains[time_idx][j];
+			int chk;
+
+			if (candidate > all_gains[new_idx - 1]) {
+				all_gains[new_idx] = candidate;
+				new_idx++;
+
+				continue;
+			}
+			for (chk = 0; chk < new_idx; chk++)
+				if (candidate <= all_gains[chk])
+					break;
+
+			if (candidate == all_gains[chk])
+				continue;
+
+			memmove(&all_gains[chk + 1], &all_gains[chk],
+				(new_idx - chk) * sizeof(int));
+			all_gains[chk] = candidate;
+			new_idx++;
+		}
+	}
+
+	gts->num_avail_all_scales = new_idx;
+	gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
+					      2 * sizeof(int), GFP_KERNEL);
+	if (!gts->avail_all_scales_table)
+		ret = -ENOMEM;
+	else
+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);
+
+	kfree(all_gains);
+	if (ret)
+		kfree(gts->avail_all_scales_table);
+
+	return ret;
+}
+
+/**
+ * iio_gts_build_avail_scale_table - create tables of available scales
+ * @gts:	Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales based on the
+ * originally given gain and time tables. When both time and gain tables are
+ * given this results:
+ * 1. A set of tables representing available scales for each supported
+ *    integration time.
+ * 2. A single table listing all the unique scales that any combination of
+ *    supported gains and times can provide.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
+{
+	int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
+
+	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+	if (!per_time_gains)
+		return ret;
+
+	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+	if (!per_time_scales)
+		goto free_gains;
+
+	for (i = 0; i < gts->num_itime; i++) {
+		per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int),
+					     GFP_KERNEL);
+		if (!per_time_scales[i])
+			goto err_free_out;
+
+		per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int),
+					    GFP_KERNEL);
+		if (!per_time_gains[i])
+			goto err_free_scale_out;
+
+
+		for (j = 0; j < gts->num_hwgain; j++)
+			per_time_gains[i][j] = gts->hwgain_table[j].gain *
+					       gts->itime_table[i].mul;
+	}
+
+	ret = gain_to_scaletables(gts, per_time_gains, per_time_scales);
+	if (ret)
+		goto err_free_out;
+
+	kfree(per_time_gains);
+	gts->per_time_avail_scale_tables = per_time_scales;
+
+	return 0;
+
+err_free_scale_out:
+	kfree(per_time_scales[i]);
+err_free_out:
+	for (i--; i; i--) {
+		kfree(per_time_scales[i]);
+		kfree(per_time_gains[i]);
+	}
+	kfree(per_time_scales);
+free_gains:
+	kfree(per_time_gains);
+
+	return ret;
+}
+
+/**
+ * iio_gts_build_avail_time_table - build table of available integration times
+ * @gts:	Gain time scale descriptor
+ *
+ * Build the table which can represent the available times to be returned
+ * to users using the read_avail-callback.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_time_table() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+static int iio_gts_build_avail_time_table(struct iio_gts *gts)
+{
+	int *times, i, j, idx = 0;
+
+	if (!gts->num_itime)
+		return 0;
+
+	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
+	if (!times)
+		return -ENOMEM;
+
+	for (i = gts->num_itime - 1; i >= 0; i--) {
+		int new = gts->itime_table[i].time_us;
+
+		if (times[idx] < new) {
+			times[idx++] = new;
+			continue;
+		}
+
+		for (j = 0; j <= idx; j++) {
+			if (times[j] > new) {
+				memmove(&times[j + 1], &times[j],
+					(idx - j) * sizeof(int));
+				times[j] = new;
+				idx++;
+			}
+		}
+	}
+	gts->avail_time_tables = times;
+	/*
+	 * This is just to survive a unlikely corner-case where times in the
+	 * given time table were not unique. Else we could just trust the
+	 * gts->num_itime.
+	 */
+	gts->num_avail_time_tables = idx;
+
+	return 0;
+}
+
+/**
+ * iio_gts_purge_avail_time_table - free-up the available integration time table
+ * @gts:	Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_time_table(). Please note
+ * that the helpers for getting available integration times like the
+ * iio_gts_avail_times() are not usable after this call. Thus, this should
+ * be only called after these helpers can no longer be called (Eg. after
+ * the iio-device has been deregistered).
+ */
+static void iio_gts_purge_avail_time_table(struct iio_gts *gts)
+{
+	if (gts->num_avail_time_tables) {
+		kfree(gts->avail_time_tables);
+		gts->avail_time_tables = NULL;
+		gts->num_avail_time_tables = 0;
+	}
+}
+
+/**
+ * iio_gts_build_avail_tables - create tables of available scales and int times
+ * @gts:	Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales and available
+ * integration times. Availability tables are built based on the originally
+ * given gain and given time tables.
+ *
+ * When both time and gain tables are
+ * given this results:
+ * 1. A set of sorted tables representing available scales for each supported
+ *    integration time.
+ * 2. A single sorted table listing all the unique scales that any combination
+ *    of supported gains and times can provide.
+ * 3. A sorted table of supported integration times
+ *
+ * After these tables are built one can use the iio_gts_all_avail_scales(),
+ * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
+ * implement the read_avail opeations.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_tables() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+static int iio_gts_build_avail_tables(struct iio_gts *gts)
+{
+	int ret;
+
+	ret = iio_gts_build_avail_scale_table(gts);
+	if (ret)
+		return ret;
+
+	ret = iio_gts_build_avail_time_table(gts);
+	if (ret)
+		iio_gts_purge_avail_scale_table(gts);
+
+	return ret;
+}
+
+/**
+ * iio_gts_purge_avail_tables - free-up the availability tables
+ * @gts:	Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_tables(). Frees both the
+ * integration time and scale tables.
+ *
+ * Note  that the helpers for getting available integration times or scales
+ * like the iio_gts_avail_times() are not usable after this call. Thus, this
+ * should be only called after these helpers can no longer be called (Eg.
+ * after the iio-device has been deregistered).
+ */
+static void iio_gts_purge_avail_tables(struct iio_gts *gts)
+{
+	iio_gts_purge_avail_time_table(gts);
+	iio_gts_purge_avail_scale_table(gts);
+}
+
+static void devm_iio_gts_avail_all_drop(void *res)
+{
+	iio_gts_purge_avail_tables(res);
+}
+
+/**
+ * devm_iio_gts_build_avail_tables - manged add availability tables
+ * @dev:	Pointer to the device whose lifetime tables are bound
+ * @gts:	Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales and available
+ * integration times. Availability tables are built based on the originally
+ * given gain and given time tables.
+ *
+ * When both time and gain tables are
+ * given this results:
+ * 1. A set of sorted tables representing available scales for each supported
+ *    integration time.
+ * 2. A single sorted table listing all the unique scales that any combination
+ *    of supported gains and times can provide.
+ * 3. A sorted table of supported integration times
+ *
+ * After these tables are built one can use the iio_gts_all_avail_scales(),
+ * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
+ * implement the read_avail opeations.
+ *
+ * The tables are automatically released upon device detach.
+ *
+ * NOTE: after the tables have been purged, the helpers for getting
+ * available scales / integration times are no longer usable. Care must be
+ * taken that unwinding is done in correct order (iio device is deregistered
+ * prior purging the tables).
+ *
+ * Return: 0 on success.
+ */
+int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts)
+{
+	int ret;
+
+	ret = iio_gts_build_avail_tables(gts);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_iio_gts_avail_all_drop, gts);
+}
+EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_tables);
+
+/**
+ * iio_gts_all_avail_scales - helper for listing all available scales
+ * @gts:	Gain time scale descriptor
+ * @vals:	Returned array of supported scales
+ * @type:	Type of returned scale values
+ * @length:	Amount of returned values in array
+ *
+ * Return: a value suitable to be returned from read_avail or a negative error.
+ */
+int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
+			     int *length)
+{
+	if (!gts->num_avail_all_scales)
+		return -EINVAL;
+
+	*vals = gts->avail_all_scales_table;
+	*type = IIO_VAL_INT_PLUS_NANO;
+	*length = gts->num_avail_all_scales * 2;
+
+	return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_GPL(iio_gts_all_avail_scales);
+
+/**
+ * iio_gts_avail_scales_for_time - list scales for integration time
+ * @gts:	Gain time scale descriptor
+ * @time:	Integration time for which the scales are listed
+ * @vals:	Returned array of supported scales
+ * @type:	Type of returned scale values
+ * @length:	Amount of returned values in array
+ *
+ * Drivers which do not allow scale setting to change integration time can
+ * use this helper to list only the scales whic are valid for given integration
+ * time.
+ *
+ * Return: a value suitable to be returned from read_avail or a negative error.
+ */
+int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
+				  const int **vals, int *type, int *length)
+{
+	int i;
+
+	for (i = 0; i < gts->num_itime; i++)
+		if (gts->itime_table[i].time_us == time)
+			break;
+
+	if (i == gts->num_itime)
+		return -EINVAL;
+
+	*vals = gts->per_time_avail_scale_tables[i];
+	*type = IIO_VAL_INT_PLUS_NANO;
+	*length = gts->num_hwgain * 2;
+
+	return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_GPL(iio_gts_avail_scales_for_time);
+
+/**
+ * iio_gts_avail_times - helper for listing available integration times
+ * @gts:	Gain time scale descriptor
+ * @vals:	Returned array of supported timees
+ * @type:	Type of returned scale values
+ * @length:	Amount of returned values in array
+ *
+ * Return: a value suitable to be returned from read_avail or a negative error.
+ */
+int iio_gts_avail_times(struct iio_gts *gts,  const int **vals, int *type,
+			int *length)
+{
+	if (!gts->num_avail_time_tables)
+		return -EINVAL;
+
+	*vals = gts->avail_time_tables;
+	*type = IIO_VAL_INT;
+	*length = gts->num_avail_time_tables;
+
+	return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_GPL(iio_gts_avail_times);
+
+/**
+ * iio_gts_valid_time - check if given integration time is valid
+ * @gts:	Gain time scale descriptor
+ * @time_us:	Integration time to check
+ *
+ * Return:	True if given time is supported by device. False if not.
+ */
+bool iio_gts_valid_time(struct iio_gts *gts, int time_us)
+{
+	return iio_gts_find_itime_by_time(gts, time_us);
+}
+EXPORT_SYMBOL_GPL(iio_gts_valid_time);
+
+int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
+{
+	int i;
+
+	for (i = 0; i < gts->num_hwgain; i++)
+		if (gts->hwgain_table[i].gain == gain)
+			return gts->hwgain_table[i].sel;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
+
+bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
+{
+	return iio_gts_find_sel_by_gain(gts, gain) >= 0;
+}
+EXPORT_SYMBOL_GPL(iio_gts_valid_gain);
+
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
+{
+	int i;
+
+	for (i = 0; i < gts->num_hwgain; i++)
+		if (gts->hwgain_table[i].sel == sel)
+			return gts->hwgain_table[i].gain;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
+
+int iio_gts_get_min_gain(struct iio_gts *gts)
+{
+	int i, min = -EINVAL;
+
+	for (i = 0; i < gts->num_hwgain; i++) {
+		int gain = gts->hwgain_table[i].gain;
+
+		if (min == -EINVAL)
+			min = gain;
+		else
+			min = min(min, gain);
+	}
+
+	return min;
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_min_gain);
+
+/**
+ * iio_find_closest_gain_low - Find the closest lower matching gain
+ * @gts:	Gain time scale descriptor
+ * @gain:	reference gain for which the closest match is searched
+ * @in_range:	indicate if the reference gain was actually in the range of
+ *		supported gains.
+ *
+ * Search for closest supported gain that is lower than or equal to the
+ * gain given as a parameter. This is usable for drivers which do not require
+ * user to request exact matching gain but rather fo rounding to a supported
+ * gain value which is equal or lower (setting lower gain is typical for
+ * avoiding saturation)
+ *
+ * Return:	The closest matching supported gain or -EINVAL is reference
+ *		gain was smaller than the smallest supported gain.
+ */
+int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range)
+{
+	int i, diff = 0;
+	int best = -1;
+
+	*in_range = false;
+
+	for (i = 0; i < gts->num_hwgain; i++) {
+		/*
+		 * It is not expected this function is called for an exactly
+		 * matching gain.
+		 */
+		if (unlikely(gain == gts->hwgain_table[i].gain)) {
+			*in_range = true;
+			return gain;
+		}
+
+		if (gain > gts->hwgain_table[i].gain) {
+			if (!diff) {
+				diff = gain - gts->hwgain_table[i].gain;
+				best = i;
+			} else {
+				int tmp = gain - gts->hwgain_table[i].gain;
+
+				if (tmp < diff) {
+					diff = tmp;
+					best = i;
+				}
+			}
+		} else {
+			/*
+			 * We found valid hwgain which is greater than
+			 * reference. So, unless we return a failure below we
+			 * will have found an in-range gain
+			 */
+			*in_range = true;
+		}
+	}
+	/* The requested gain was smaller than anything we support */
+	if (!diff) {
+		*in_range = false;
+
+		return -EINVAL;
+	}
+
+	return gts->hwgain_table[best].gain;
+}
+EXPORT_SYMBOL_GPL(iio_find_closest_gain_low);
+
+static int iio_gts_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts,
+						       int sel)
+{
+	const struct iio_itime_sel_mul *time;
+
+	time = iio_gts_find_itime_by_sel(gts, sel);
+	if (!time)
+		return -EINVAL;
+
+	return time->mul;
+}
+
+/**
+ * iio_gts_find_gain_for_scale_using_time - Find gain by time and scale
+ * @gts:	Gain time scale descriptor
+ * @time_sel:	Integration time selector correspondig to the time gain is
+ *		searhed for
+ * @scale_int:	Integral part of the scale (typically val1)
+ * @scale_nano:	Fractional part of the scale (nano or ppb)
+ * @gain:	Pointer to value where gain is stored.
+ *
+ * In some cases the light sensors may want to find a gain setting which
+ * corresponds given scale and integration time. Sensors which fill the
+ * gain and time tables may use this helper to retrieve the gain.
+ *
+ * Return:	0 on success. -EINVAL if gain matching the parameters is not
+ *		found.
+ */
+static int iio_gts_find_gain_for_scale_using_time(struct iio_gts *gts, int time_sel,
+						  int scale_int, int scale_nano,
+						  int *gain)
+{
+	u64 scale_linear;
+	int ret, mul;
+
+	ret = iio_gts_linearize(scale_int, scale_nano, NANO, &scale_linear);
+	if (ret)
+		return ret;
+
+	ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, time_sel);
+	if (ret < 0)
+		return ret;
+
+	mul = ret;
+
+	ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);
+	if (ret)
+		return ret;
+
+	if (!iio_gts_valid_gain(gts, *gain))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * iio_gts_find_gain_sel_for_scale_using_time - Fetch gain selector.
+ * See iio_gts_find_gain_for_scale_using_time() for more information
+ */
+int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel,
+					       int scale_int, int scale_nano,
+					       int *gain_sel)
+{
+	int gain, ret;
+
+	ret = iio_gts_find_gain_for_scale_using_time(gts, time_sel, scale_int,
+						     scale_nano, &gain);
+	if (ret)
+		return ret;
+
+	ret = iio_gts_find_sel_by_gain(gts, gain);
+	if (ret < 0)
+		return ret;
+
+	*gain_sel = ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_gain_sel_for_scale_using_time);
+
+int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel)
+{
+	const struct iio_itime_sel_mul *itime;
+
+	itime = iio_gts_find_itime_by_sel(gts, sel);
+	if (!itime)
+		return -EINVAL;
+
+	return itime->time_us;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel);
+
+int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time)
+{
+	const struct iio_itime_sel_mul *itime;
+
+	itime = iio_gts_find_itime_by_time(gts, time);
+	if (!itime)
+		return -EINVAL;
+
+	return itime->sel;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time);
+
+static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
+{
+	const struct iio_itime_sel_mul *itime;
+
+	if (!iio_gts_valid_gain(gts, gain))
+		return -EINVAL;
+
+	if (!gts->num_itime)
+		return gain;
+
+	itime = iio_gts_find_itime_by_time(gts, time);
+	if (!itime)
+		return -EINVAL;
+
+	return gain * itime->mul;
+}
+
+static int iio_gts_get_scale_linear(struct iio_gts *gts, int gain, int time,
+				    u64 *scale)
+{
+	int total_gain;
+	u64 tmp;
+
+	total_gain = iio_gts_get_total_gain(gts, gain, time);
+	if (total_gain < 0)
+		return total_gain;
+
+	tmp = gts->max_scale;
+
+	do_div(tmp, total_gain);
+
+	*scale = tmp;
+
+	return 0;
+}
+
+int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int,
+		      int *scale_nano)
+{
+	u64 lin_scale;
+	int ret;
+
+	ret = iio_gts_get_scale_linear(gts, gain, time, &lin_scale);
+	if (ret)
+		return ret;
+
+	return iio_gts_delinearize(lin_scale, NANO, scale_int, scale_nano);
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_scale);
+
+/**
+ * iio_gts_find_new_gain_sel_by_old_gain_time - compensate time change
+ * @gts:		Gain time scale descriptor
+ * @old_gain:		Previously set gain
+ * @old_time_sel:	Selector corresponding previously set time
+ * @new_time_sel:	Selector corresponding new time to be set
+ * @new_gain:		Pointer to value where new gain is to be written
+ *
+ * We may want to mitigate the scale change caused by setting a new integration
+ * time (for a light sensor) by also updating the (HW)gain. This helper computes
+ * new gain value to maintain the scale with new integration time.
+ *
+ * Return: 0 on success. -EINVAL if gain matching the new time is not found.
+ */
+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;
+
+	itime_old = iio_gts_find_itime_by_sel(gts, old_time_sel);
+	if (!itime_old)
+		return -EINVAL;
+
+	itime_new = iio_gts_find_itime_by_sel(gts, new_time_sel);
+	if (!itime_new)
+		return -EINVAL;
+
+	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
+				       &scale);
+	if (ret)
+		return ret;
+
+	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
+				      new_gain);
+	if (ret)
+		return -EINVAL;
+
+	if (!iio_gts_valid_gain(gts, *new_gain))
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_new_gain_sel_by_old_gain_time);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
diff --git a/include/linux/iio/iio-gts-helper.h b/include/linux/iio/iio-gts-helper.h
new file mode 100644
index 000000000000..95c712007962
--- /dev/null
+++ b/include/linux/iio/iio-gts-helper.h
@@ -0,0 +1,113 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#ifndef __IIO_GTS_HELPER__
+#define __IIO_GTS_HELPER__
+
+#include <linux/types.h>
+
+struct device;
+
+/**
+ * struct iio_gain_sel_pair - gain - selector values
+ *
+ * In many cases devices like light sensors allow setting signal amplification
+ * (gain) using a register interface. This structure describes amplification
+ * and corresponding selector (register value)
+ *
+ * @gain:	Gain (multiplication) value.
+ * @sel:	Selector (usually register value) used to indicate this gain
+ */
+struct iio_gain_sel_pair {
+	int gain;
+	int sel;
+};
+
+/**
+ * struct iio_itime_sel_mul - integration time description
+ *
+ * In many cases devices like light sensors allow setting the duration of
+ * collecting data. Typically this duration has also an impact to the magnitude
+ * of measured values (gain). This structure describes the relation of
+ * integration time and amplification as well as corresponding selector
+ * (register value).
+ *
+ * An example could be a sensor allowing 50, 100, 200 and 400 mS times. The
+ * respective multiplication values could be 50 mS => 1, 100 mS => 2,
+ * 200 mS => 4 and 400 mS => 8 assuming the impact of integration time would be
+ * linear in a way that when collecting data for 50 mS caused value X, doubling
+ * the data collection time caused value 2X etc..
+ *
+ * @time_us:	Integration time in microseconds.
+ * @sel:	Selector (usually register value) used to indicate this time
+ * @mul:	Multiplication to the values caused by this time.
+ */
+struct iio_itime_sel_mul {
+	int time_us;
+	int sel;
+	int mul;
+};
+
+struct iio_gts {
+	u64 max_scale;
+	const struct iio_gain_sel_pair *hwgain_table;
+	int num_hwgain;
+	const struct iio_itime_sel_mul *itime_table;
+	int num_itime;
+	int **per_time_avail_scale_tables;
+	int *avail_all_scales_table;
+	int num_avail_all_scales;
+	int *avail_time_tables;
+	int num_avail_time_tables;
+};
+
+#define GAIN_SCALE_GAIN(_gain, _sel)			\
+{							\
+	.gain = (_gain),				\
+	.sel = (_sel),					\
+}
+
+#define GAIN_SCALE_ITIME_US(_itime, _sel, _mul)		\
+{							\
+	.time_us = (_itime),				\
+	.sel = (_sel),					\
+	.mul = (_mul),					\
+}
+
+int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
+		     const struct iio_gain_sel_pair *gain_tbl, int num_gain,
+		     const struct iio_itime_sel_mul *tim_tbl, int num_times,
+		     struct iio_gts *gts);
+
+bool iio_gts_valid_gain(struct iio_gts *gts, int gain);
+bool iio_gts_valid_time(struct iio_gts *gts, int time_us);
+
+int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range);
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel);
+int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain);
+int iio_gts_get_min_gain(struct iio_gts *gts);
+int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel);
+int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time);
+
+int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
+				int *scale_int, int *scale_nano);
+int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel,
+					       int scale_int, int scale_nano,
+					       int *gain_sel);
+int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int,
+		      int *scale_nano);
+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);
+int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
+int iio_gts_avail_times(struct iio_gts *gts,  const int **vals, int *type,
+			int *length);
+int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
+			     int *length);
+int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
+				  const int **vals, int *type, int *length);
+
+#endif
-- 
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 v4 5/8] iio: test: test gain-time-scale helpers

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Changes:
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        |  16 ++
 drivers/iio/test/Makefile       |   1 +
 drivers/iio/test/iio-test-gts.c | 461 ++++++++++++++++++++++++++++++++
 3 files changed, 478 insertions(+)
 create mode 100644 drivers/iio/test/iio-test-gts.c

diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
index 0b6e4e278a2f..4d5cfb9fe60b 100644
--- a/drivers/iio/test/Kconfig
+++ b/drivers/iio/test/Kconfig
@@ -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
@@ -27,3 +41,5 @@ config IIO_FORMAT_KUNIT_TEST
 	  to the KUnit documentation in Documentation/dev-tools/kunit/.
 
 	  If unsure, say N.
+
+
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index d76eaf36da82..e9a4cf1ff57f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -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)
diff --git a/drivers/iio/test/iio-test-gts.c b/drivers/iio/test/iio-test-gts.c
new file mode 100644
index 000000000000..ff9311acd0bb
--- /dev/null
+++ b/drivers/iio/test/iio-test-gts.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Unit tests for IIO light sensor gain-time-scale helpers
+ *
+ * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#include <kunit/platform_device.h>
+#include <kunit/test.h>
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/types.h>
+
+/*
+ * Please, read the "rant" from the top of the lib/test_linear_ranges.c if
+ * you see a line of helper code which is not being tested.
+ *
+ * Then, please look at the line which is not being tested. Is this line
+ * somehow unusually complex? If answer is "no", then chances are that the
+ * "development inertia" caused by adding a test exceeds the benefits.
+ *
+ * If yes, then adding a test is probably a good idea but please stop for a
+ * moment and consider the effort of changing all the tests when code gets
+ * refactored. Eventually it neeeds to be.
+ */
+
+#define TEST_TSEL_50		1
+#define TEST_TSEL_X_MIN		TEST_TSEL_50
+#define TEST_TSEL_100		0
+#define TEST_TSEL_200		2
+#define TEST_TSEL_400		4
+#define TEST_TSEL_X_MAX		TEST_TSEL_400
+
+#define TEST_GSEL_1		0x00
+#define TEST_GSEL_X_MIN		TEST_GSEL_1
+#define TEST_GSEL_4		0x08
+#define TEST_GSEL_16		0x0a
+#define TEST_GSEL_32		0x0b
+#define TEST_GSEL_64		0x0c
+#define TEST_GSEL_256		0x18
+#define TEST_GSEL_512		0x19
+#define TEST_GSEL_1024		0x1a
+#define TEST_GSEL_2048		0x1b
+#define TEST_GSEL_4096		0x1c
+#define TEST_GSEL_X_MAX		TEST_GSEL_4096
+
+#define TEST_SCALE_1X		64
+#define TEST_SCALE_MIN_X	TEST_SCALE_1X
+#define TEST_SCALE_2X		32
+#define TEST_SCALE_4X		16
+#define TEST_SCALE_8X		8
+#define TEST_SCALE_16X		4
+#define TEST_SCALE_32X		2
+#define TEST_SCALE_64X		1
+
+#define TEST_SCALE_NANO_128X	500000000
+#define TEST_SCALE_NANO_256X	250000000
+#define TEST_SCALE_NANO_512X	125000000
+#define TEST_SCALE_NANO_1024X	62500000
+#define TEST_SCALE_NANO_2048X	31250000
+#define TEST_SCALE_NANO_4096X	15625000
+#define TEST_SCALE_NANO_4096X2	7812500
+#define TEST_SCALE_NANO_4096X4	3906250
+#define TEST_SCALE_NANO_4096X8	1953125
+
+#define TEST_SCALE_NANO_MAX_X TEST_SCALE_NANO_4096X8
+
+static const struct iio_gain_sel_pair gts_test_gains[] = {
+	GAIN_SCALE_GAIN(1, TEST_GSEL_1),
+	GAIN_SCALE_GAIN(4, TEST_GSEL_4),
+	GAIN_SCALE_GAIN(16, TEST_GSEL_16),
+	GAIN_SCALE_GAIN(32, TEST_GSEL_32),
+	GAIN_SCALE_GAIN(64, TEST_GSEL_64),
+	GAIN_SCALE_GAIN(256, TEST_GSEL_256),
+	GAIN_SCALE_GAIN(512, TEST_GSEL_512),
+	GAIN_SCALE_GAIN(1024, TEST_GSEL_1024),
+	GAIN_SCALE_GAIN(2048, TEST_GSEL_2048),
+	GAIN_SCALE_GAIN(4096, TEST_GSEL_4096),
+#define HWGAIN_MAX 4096
+};
+
+static const struct iio_itime_sel_mul gts_test_itimes[] = {
+	GAIN_SCALE_ITIME_US(400 * 1000, TEST_TSEL_400, 8),
+	GAIN_SCALE_ITIME_US(200 * 1000, TEST_TSEL_200, 4),
+	GAIN_SCALE_ITIME_US(100 * 1000, TEST_TSEL_100, 2),
+	GAIN_SCALE_ITIME_US(50 * 1000, TEST_TSEL_50, 1),
+#define TIMEGAIN_MAX 8
+};
+#define TOTAL_GAIN_MAX	(HWGAIN_MAX * TIMEGAIN_MAX)
+
+static int test_init_iio_gain_scale(struct iio_gts *gts, int max_scale_int,
+				int max_scale_nano)
+{
+	int ret;
+
+	ret = iio_init_iio_gts(max_scale_int, max_scale_nano, gts_test_gains,
+			       ARRAY_SIZE(gts_test_gains), gts_test_itimes,
+			       ARRAY_SIZE(gts_test_itimes), gts);
+
+	return ret;
+}
+
+static void test_iio_gts_find_gain_for_scale_using_time(struct kunit *test)
+{
+	struct iio_gts gts;
+	int ret, gain_sel;
+
+	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_100,
+						TEST_SCALE_8X, 0, &gain_sel);
+	/*
+	 * Meas time 100 => gain by time 2x
+	 * TEST_SCALE_8X matches total gain 8x
+	 * => required HWGAIN 4x
+	 */
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_GSEL_4, gain_sel);
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_200, 0,
+						TEST_SCALE_NANO_256X, &gain_sel);
+	/*
+	 * Meas time 200 => gain by time 4x
+	 * TEST_SCALE_256X matches total gain 256x
+	 * => required HWGAIN 256/4 => 64x
+	 */
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_GSEL_64, gain_sel);
+
+	/* Min time, Min gain */
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_X_MIN,
+						TEST_SCALE_MIN_X, 0, &gain_sel);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_GSEL_1, gain_sel);
+
+	/* Max time, Max gain */
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_X_MAX,
+					0, TEST_SCALE_NANO_MAX_X, &gain_sel);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_GSEL_4096, gain_sel);
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_100, 0,
+						TEST_SCALE_NANO_256X, &gain_sel);
+	/*
+	 * Meas time 100 => gain by time 2x
+	 * TEST_SCALE_256X matches total gain 256x
+	 * => required HWGAIN 256/2 => 128x (not in gain-table - unsupported)
+	 */
+	KUNIT_EXPECT_NE(test, 0, ret);
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_200, 0,
+						TEST_SCALE_NANO_MAX_X, &gain_sel);
+	/* We can't reach the max gain with integration time smaller than MAX */
+	KUNIT_EXPECT_NE(test, 0, ret);
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&gts, TEST_TSEL_50, 0,
+						TEST_SCALE_NANO_MAX_X, &gain_sel);
+	/* We can't reach the max gain with integration time smaller than MAX */
+	KUNIT_EXPECT_NE(test, 0, ret);
+}
+
+static void test_iio_gts_find_new_gain_sel_by_old_gain_time(struct kunit *test)
+{
+	struct iio_gts gts;
+	int ret, old_gain, new_gain, old_time_sel, new_time_sel;
+
+	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	old_gain = 32;
+	old_time_sel = TEST_TSEL_200;
+	new_time_sel = TEST_TSEL_400;
+
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	/*
+	 * Doubling the integration time doubles the total gain - so old
+	 * (hw)gain must be divided by two to compensate. => 32 / 2 => 16
+	 */
+	KUNIT_EXPECT_EQ(test, 16, new_gain);
+
+	old_gain = 4;
+	old_time_sel = TEST_TSEL_50;
+	new_time_sel = TEST_TSEL_200;
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	/*
+	 * gain by time 1x => 4x - (hw)gain 4x => 1x
+	 */
+	KUNIT_EXPECT_EQ(test, 1, new_gain);
+
+	old_gain = 512;
+	old_time_sel = TEST_TSEL_400;
+	new_time_sel = TEST_TSEL_50;
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	/*
+	 * gain by time 8x => 1x - (hw)gain 512x => 4096x)
+	 */
+	KUNIT_EXPECT_EQ(test, 4096, new_gain);
+
+	/* Unsupported gain 2x */
+	old_gain = 4;
+	old_time_sel = TEST_TSEL_200;
+	new_time_sel = TEST_TSEL_400;
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_NE(test, 0, ret);
+
+	/* Too small gain */
+	old_gain = 4;
+	old_time_sel = TEST_TSEL_50;
+	new_time_sel = TEST_TSEL_400;
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_NE(test, 0, ret);
+
+	/* Too big gain */
+	old_gain = 1024;
+	old_time_sel = TEST_TSEL_400;
+	new_time_sel = TEST_TSEL_50;
+	ret = iio_gts_find_new_gain_sel_by_old_gain_time(&gts, old_gain,
+					old_time_sel, new_time_sel, &new_gain);
+	KUNIT_EXPECT_NE(test, 0, ret);
+}
+
+static void test_iio_find_closest_gain_low(struct kunit *test)
+{
+	struct iio_gts gts;
+	bool in_range;
+	int ret;
+
+	const struct iio_gain_sel_pair gts_test_gains_gain_low[] = {
+		GAIN_SCALE_GAIN(4, TEST_GSEL_4),
+		GAIN_SCALE_GAIN(16, TEST_GSEL_16),
+		GAIN_SCALE_GAIN(32, TEST_GSEL_32),
+	};
+
+	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	ret = iio_find_closest_gain_low(&gts, 2, &in_range);
+	KUNIT_EXPECT_EQ(test, 1, ret);
+	KUNIT_EXPECT_EQ(test, true, in_range);
+
+	ret = iio_find_closest_gain_low(&gts, 1, &in_range);
+	KUNIT_EXPECT_EQ(test, 1, ret);
+	KUNIT_EXPECT_EQ(test, true, in_range);
+
+	ret = iio_find_closest_gain_low(&gts, 4095, &in_range);
+	KUNIT_EXPECT_EQ(test, 2048, ret);
+	KUNIT_EXPECT_EQ(test, true, in_range);
+
+	ret = iio_find_closest_gain_low(&gts, 4097, &in_range);
+	KUNIT_EXPECT_EQ(test, 4096, ret);
+	KUNIT_EXPECT_EQ(test, false, in_range);
+
+	ret = iio_init_iio_gts(TEST_SCALE_1X, 0, gts_test_gains_gain_low,
+			       ARRAY_SIZE(gts_test_gains_gain_low),
+			       gts_test_itimes, ARRAY_SIZE(gts_test_itimes),
+			       &gts);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	ret = iio_find_closest_gain_low(&gts, 3, &in_range);
+	KUNIT_EXPECT_EQ(test, -EINVAL, ret);
+	KUNIT_EXPECT_EQ(test, false, in_range);
+}
+
+static void test_iio_gts_total_gain_to_scale(struct kunit *test)
+{
+	struct iio_gts gts;
+	int ret, scale_int, scale_nano;
+
+	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	ret = iio_gts_total_gain_to_scale(&gts, 1, &scale_int, &scale_nano);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
+	KUNIT_EXPECT_EQ(test, 0, scale_nano);
+
+	ret = iio_gts_total_gain_to_scale(&gts, 1, &scale_int, &scale_nano);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
+	KUNIT_EXPECT_EQ(test, 0, scale_nano);
+
+	ret = iio_gts_total_gain_to_scale(&gts, 4096 * 8, &scale_int,
+					  &scale_nano);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	KUNIT_EXPECT_EQ(test, 0, scale_int);
+	KUNIT_EXPECT_EQ(test, TEST_SCALE_NANO_4096X8, scale_nano);
+}
+
+static void test_iio_gts_chk_times(struct kunit *test, const int *vals)
+{
+	static const int expected[] = {50000, 100000, 200000, 400000};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(expected); i++)
+		KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_chk_scales_all(struct kunit *test, struct iio_gts *gts,
+					const int *vals, int len)
+{
+	static const int gains[] = {1, 2, 4, 8, 16, 32, 64, 128, 256, 512,
+				    1024, 2048, 4096, 4096 * 2, 4096 * 4,
+				    4096 * 8};
+
+	int expected[ARRAY_SIZE(gains) * 2];
+	int i, ret;
+	int exp_len = ARRAY_SIZE(gains) * 2;
+
+	KUNIT_EXPECT_EQ(test, exp_len, len);
+	if (len != exp_len)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(gains); i++) {
+		ret = iio_gts_total_gain_to_scale(gts, gains[i],
+						  &expected[2 * i],
+						  &expected[2 * i + 1]);
+		KUNIT_EXPECT_EQ(test, 0, ret);
+		if (ret)
+			return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(expected); i++)
+		KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_chk_scales_t200(struct kunit *test, struct iio_gts *gts,
+					 const int *vals, int len)
+{
+	/* The gain caused by time 200 is 4x */
+	static const int gains[] = {
+		1 * 4,
+		4 * 4,
+		16 * 4,
+		32 * 4,
+		64 * 4,
+		256 * 4,
+		512 * 4,
+		1024 * 4,
+		2048 * 4,
+		4096 * 4
+	};
+	int expected[ARRAY_SIZE(gains) * 2];
+	int i, ret;
+
+	KUNIT_EXPECT_EQ(test, 2 * ARRAY_SIZE(gains), len);
+	if (len < 2 * ARRAY_SIZE(gains))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(gains); i++) {
+		ret = iio_gts_total_gain_to_scale(gts, gains[i],
+						  &expected[2 * i],
+						  &expected[2 * i + 1]);
+		KUNIT_EXPECT_EQ(test, 0, ret);
+		if (ret)
+			return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(expected); i++)
+		KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_avail_test(struct kunit *test)
+{
+	struct iio_gts gts;
+	int ret;
+	int type, len;
+	const int *vals;
+	struct device *dev;
+
+	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		return;
+
+	dev = test_kunit_helper_alloc_device(test);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
+	if (!dev)
+		return;
+
+	ret = devm_iio_gts_build_avail_tables(dev, &gts);
+	KUNIT_EXPECT_EQ(test, 0, ret);
+	if (ret)
+		goto drop_testdev;
+
+	/* test table building for times and iio_gts_avail_times() */
+	ret = iio_gts_avail_times(&gts, &vals, &type, &len);
+	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+	if (ret)
+		goto drop_testdev;
+
+	KUNIT_EXPECT_EQ(test, IIO_VAL_INT, type);
+	KUNIT_EXPECT_EQ(test, 4, len);
+	if (len < 4)
+		goto drop_testdev;
+
+	test_iio_gts_chk_times(test, vals);
+
+	/* Test table building for all scales and iio_gts_all_avail_scales() */
+	ret = iio_gts_all_avail_scales(&gts, &vals, &type, &len);
+	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+	if (ret)
+		goto drop_testdev;
+
+	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
+
+	test_iio_gts_chk_scales_all(test, &gts, vals, len);
+
+	/*
+	 * Test table building for scales/time and
+	 * iio_gts_avail_scales_for_time()
+	 */
+	ret = iio_gts_avail_scales_for_time(&gts, 200000, &vals, &type, &len);
+	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+	if (ret)
+		goto drop_testdev;
+
+	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
+	test_iio_gts_chk_scales_t200(test, &gts, vals, len);
+
+drop_testdev:
+	test_kunit_helper_free_device(test, dev);
+}
+
+static struct kunit_case iio_gts_test_cases[] = {
+		KUNIT_CASE(test_iio_gts_find_gain_for_scale_using_time),
+		KUNIT_CASE(test_iio_gts_find_new_gain_sel_by_old_gain_time),
+		KUNIT_CASE(test_iio_find_closest_gain_low),
+		KUNIT_CASE(test_iio_gts_total_gain_to_scale),
+		KUNIT_CASE(test_iio_gts_avail_test),
+		{}
+};
+
+static struct kunit_suite iio_gts_test_suite = {
+	.name = "iio-gain-time-scale",
+	.test_cases = iio_gts_test_cases,
+};
+
+kunit_test_suite(iio_gts_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
+
-- 
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 v4 7/8] iio: light: ROHM BU27034 Ambient Light Sensor

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Changes
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 | 1491 ++++++++++++++++++++++++++++++
 3 files changed, 1506 insertions(+)
 create mode 100644 drivers/iio/light/rohm-bu27034.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..6fa31fcd71a1 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -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
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index d74d2b5ff14c..985f6feaccd4 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -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
diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
new file mode 100644
index 000000000000..db1bbb0c0d35
--- /dev/null
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -0,0 +1,1491 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * BU27034 ROHM Ambient Light Sensor
+ *
+ * Copyright (c) 2023, ROHM Semiconductor.
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/kfifo_buf.h>
+
+
+#define BU27034_REG_SYSTEM_CONTROL	0x40
+#define BU27034_MASK_SW_RESET		BIT(7)
+#define BU27034_MASK_PART_ID		GENMASK(5, 0)
+#define BU27034_ID			0x19
+#define BU27034_REG_MODE_CONTROL1	0x41
+#define BU27034_MASK_MEAS_MODE		GENMASK(2, 0)
+
+#define BU27034_REG_MODE_CONTROL2	0x42
+#define BU27034_MASK_D01_GAIN		GENMASK(7, 3)
+#define BU27034_SHIFT_D01_GAIN		3
+#define BU27034_MASK_D2_GAIN_HI		GENMASK(7, 6)
+#define BU27034_MASK_D2_GAIN_LO		GENMASK(2, 0)
+
+#define BU27034_REG_MODE_CONTROL3	0x43
+#define BU27034_REG_MODE_CONTROL4	0x44
+#define BU27034_MASK_MEAS_EN		BIT(0)
+#define BU27034_MASK_VALID		BIT(7)
+#define BU27034_REG_DATA0_LO		0x50
+#define BU27034_REG_DATA1_LO		0x52
+#define BU27034_REG_DATA2_LO		0x54
+#define BU27034_REG_DATA2_HI		0x55
+#define BU27034_REG_MANUFACTURER_ID	0x92
+#define BU27034_REG_MAX BU27034_REG_MANUFACTURER_ID
+
+/*
+ * The BU27034 does not have interrupt to trigger the data read when a
+ * measurement has finished. Hence we poll the VALID bit in a thread. We will
+ * try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS milliseconds before
+ * the expected sampling time to prevent the drifting.
+ *
+ * If we constantly wake up a bit too late we would eventually skip a sample.
+ * And because the sleep can't wake up _exactly_ at given time this would be
+ * inevitable even if the sensor clock would be perfectly phase-locked to CPU
+ * clock - which we can't say is the case.
+ *
+ * This is still fragile. No matter how big advance do we have, we will still
+ * risk of losing a sample because things can in a rainy-day skenario be
+ * delayed a lot. Yet, more we reserve the time for polling, more we also lose
+ * the performance by spending cycles polling the register. So, selecting this
+ * value is a balancing dance between severity of wasting CPU time and severity
+ * of losing samples.
+ *
+ * In most cases losing the samples is not _that_ crucial because light levels
+ * tend to change slowly.
+ *
+ * Other option that was pointed to me would be always sleeping 1/2 of the
+ * measurement time, checking the VALID bit and just sleeping again if the bit
+ * was not set. That should be pretty tolerant against missing samples due to
+ * the scheduling delays while also not wasting much of cycles for polling.
+ * Downside is that the time-stamps would be very inaccurate as the wake-up
+ * would not really be tied to the senso toggling the valid bit. This would also
+ * result 'jumps' in the time-stamps when the delay drifted so that wake-up was
+ * performed during teo consecutive wake-ups (Or, when sensor and CPU clocks
+ * were very different and scheduling the wake-ups was very close to given
+ * timeout - and when the time-outs were very close to the actual sensor
+ * sampling, Eg. once in a blue moon, two consequtive time-outs would occur
+ * without having a sample ready).
+ */
+#define BU27034_MEAS_WAIT_PREMATURE_MS	5
+#define BU27034_DATA_WAIT_TIME_US	1000
+#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000)
+
+#define BU27034_RETRY_LIMIT 18
+
+enum {
+	BU27034_CHAN_ALS,
+	BU27034_CHAN_DATA0,
+	BU27034_CHAN_DATA1,
+	BU27034_CHAN_DATA2,
+	BU27034_NUM_CHANS
+};
+
+static const unsigned long bu27034_scan_masks[] = {
+	BIT(BU27034_CHAN_ALS) | BIT(BU27034_CHAN_DATA0) |
+	BIT(BU27034_CHAN_DATA1) | BIT(BU27034_CHAN_DATA2), 0
+};
+
+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X	64
+
+/* See the data sheet for the "Gain Setting" table */
+#define BU27034_GSEL_1X		0x00 /* 00000 */
+#define BU27034_GSEL_4X		0x08 /* 01000 */
+#define BU27034_GSEL_16X	0x0a /* 01010 */
+#define BU27034_GSEL_32X	0x0b /* 01011 */
+#define BU27034_GSEL_64X	0x0c /* 01100 */
+#define BU27034_GSEL_256X	0x18 /* 11000 */
+#define BU27034_GSEL_512X	0x19 /* 11001 */
+#define BU27034_GSEL_1024X	0x1a /* 11010 */
+#define BU27034_GSEL_2048X	0x1b /* 11011 */
+#define BU27034_GSEL_4096X	0x1c /* 11100 */
+
+/* Available gain settings */
+static const struct iio_gain_sel_pair bu27034_gains[] = {
+	GAIN_SCALE_GAIN(1, BU27034_GSEL_1X),
+	GAIN_SCALE_GAIN(4, BU27034_GSEL_4X),
+	GAIN_SCALE_GAIN(16, BU27034_GSEL_16X),
+	GAIN_SCALE_GAIN(32, BU27034_GSEL_32X),
+	GAIN_SCALE_GAIN(64, BU27034_GSEL_64X),
+	GAIN_SCALE_GAIN(256, BU27034_GSEL_256X),
+	GAIN_SCALE_GAIN(512, BU27034_GSEL_512X),
+	GAIN_SCALE_GAIN(1024, BU27034_GSEL_1024X),
+	GAIN_SCALE_GAIN(2048, BU27034_GSEL_2048X),
+	GAIN_SCALE_GAIN(4096, BU27034_GSEL_4096X),
+};
+
+/*
+ * The IC has 5 modes for sampling time. 5 mS mode is exceptional as it limits
+ * the data collection to data0-channel only and cuts the supported range to
+ * 10 bit. It is not supported by the driver.
+ *
+ * "normal" modes are 55, 100, 200 and 400 mS modes - which do have direct
+ * multiplying impact to the register values (similar to gain).
+ *
+ * This means that if meas-mode is changed for example from 400 => 200,
+ * the scale is doubled. Eg, time impact to total gain is x1, x2, x4, x8.
+ */
+#define BU27034_MEAS_MODE_100MS		0
+#define BU27034_MEAS_MODE_55MS		1
+#define BU27034_MEAS_MODE_200MS		2
+#define BU27034_MEAS_MODE_400MS		4
+
+static const struct iio_itime_sel_mul bu27034_itimes[] = {
+	GAIN_SCALE_ITIME_US(400000, BU27034_MEAS_MODE_400MS, 8),
+	GAIN_SCALE_ITIME_US(200000, BU27034_MEAS_MODE_200MS, 4),
+	GAIN_SCALE_ITIME_US(100000, BU27034_MEAS_MODE_100MS, 2),
+	GAIN_SCALE_ITIME_US(55000, BU27034_MEAS_MODE_55MS, 1),
+};
+
+#define BU27034_CHAN_DATA(_name, _ch2)					\
+{									\
+	.type = IIO_INTENSITY,						\
+	.channel = BU27034_CHAN_##_name,				\
+	.channel2 = (_ch2),						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+			      BIT(IIO_CHAN_INFO_SCALE),			\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),		\
+	.info_mask_shared_by_all_available =				\
+					BIT(IIO_CHAN_INFO_INT_TIME),	\
+	.address = BU27034_REG_##_name##_LO,				\
+	.scan_index = BU27034_CHAN_##_name,				\
+	.scan_type = {							\
+		.sign = 'u',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+	.indexed = 1,							\
+}
+
+static const struct iio_chan_spec bu27034_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.channel = BU27034_CHAN_ALS,
+		.scan_index = BU27034_CHAN_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
+	/*
+	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
+	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
+	 * These wave lengths are pretty much on the border of colours making
+	 * these a poor candidates for R/G/B standardization. Hence they're both
+	 * marked as clear channels
+	 */
+	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
+	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
+	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+struct bu27034_data {
+	struct regmap *regmap;
+	struct device *dev;
+	/*
+	 * Protect gain and time during scale adjustment and data reading.
+	 * Protect measurement enabling/disabling.
+	 */
+	struct mutex mutex;
+	struct iio_gts gts;
+	struct task_struct *task;
+	__le16 raw[3];
+	struct {
+		u32 lux;
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+};
+
+struct bu27034_result {
+	u16 ch0;
+	u16 ch1;
+	u16 ch2;
+};
+
+static const struct regmap_range bu27034_volatile_ranges[] = {
+	{
+		.range_min = BU27034_REG_MODE_CONTROL4,
+		.range_max = BU27034_REG_MODE_CONTROL4,
+	}, {
+		.range_min = BU27034_REG_DATA0_LO,
+		.range_max = BU27034_REG_DATA2_HI,
+	},
+};
+
+static const struct regmap_access_table bu27034_volatile_regs = {
+	.yes_ranges = &bu27034_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bu27034_volatile_ranges),
+};
+
+static const struct regmap_range bu27034_read_only_ranges[] = {
+	{
+		.range_min = BU27034_REG_DATA0_LO,
+		.range_max = BU27034_REG_DATA2_HI,
+	}, {
+		.range_min = BU27034_REG_MANUFACTURER_ID,
+		.range_max = BU27034_REG_MANUFACTURER_ID,
+	}
+};
+
+static const struct regmap_access_table bu27034_ro_regs = {
+	.no_ranges = &bu27034_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(bu27034_read_only_ranges),
+};
+
+static const struct regmap_config bu27034_regmap = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+
+	.max_register	= BU27034_REG_MAX,
+	.cache_type	= REGCACHE_RBTREE,
+	.volatile_table = &bu27034_volatile_regs,
+	.wr_table	= &bu27034_ro_regs,
+};
+
+struct bu27034_gain_check {
+	int old_gain;
+	int new_gain;
+	int chan;
+};
+
+static int bu27034_get_gain_sel(struct bu27034_data *data, int chan)
+{
+	int ret, val;
+
+	switch (chan) {
+	case BU27034_CHAN_DATA0:
+	case BU27034_CHAN_DATA1:
+	{
+		int reg[] = {
+			[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+			[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+		};
+		ret = regmap_read(data->regmap, reg[chan], &val);
+		if (ret)
+			return ret;
+
+		return FIELD_GET(BU27034_MASK_D01_GAIN, val);
+	}
+	case BU27034_CHAN_DATA2:
+	{
+		int d2_lo_bits = fls(BU27034_MASK_D2_GAIN_LO);
+
+		ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val);
+		if (ret)
+			return ret;
+
+		/*
+		 * The data2 channel gain is composed by 5 non continuous bits
+		 * [7:6], [2:0]. Thus when we combine the 5-bit 'selector'
+		 * from register value we must right shift the high bits by 3.
+		 */
+		return FIELD_GET(BU27034_MASK_D2_GAIN_HI, val) << d2_lo_bits |
+		       FIELD_GET(BU27034_MASK_D2_GAIN_LO, val);
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bu27034_get_gain(struct bu27034_data *data, int chan, int *gain)
+{
+	int ret, sel;
+
+	ret = bu27034_get_gain_sel(data, chan);
+	if (ret < 0)
+		return ret;
+
+	sel = ret;
+
+	ret = iio_gts_find_gain_by_sel(&data->gts, sel);
+	if (ret < 0) {
+		dev_err(data->dev, "chan %u: unknown gain value 0x%x\n", chan,
+			sel);
+
+		return ret;
+	}
+
+	*gain = ret;
+
+	return 0;
+}
+
+static int bu27034_get_int_time(struct bu27034_data *data)
+{
+	int ret, sel;
+
+	ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel);
+	if (ret)
+		return ret;
+
+	return iio_gts_find_int_time_by_sel(&data->gts,
+					    sel & BU27034_MASK_MEAS_MODE);
+}
+
+static int _bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+			      int *val2)
+{
+	int gain, ret;
+
+	ret = bu27034_get_gain(data, channel, &gain);
+	if (ret)
+		return ret;
+
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		return ret;
+
+	return iio_gts_get_scale(&data->gts, gain, ret, val, val2);
+}
+
+static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+			      int *val2)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = _bu27034_get_scale(data, channel, val, val2);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+/* Caller should hold the lock to protect lux reading */
+static int bu27034_write_gain_sel(struct bu27034_data *data, int chan, int sel)
+{
+	static const int reg[] = {
+		[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+		[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+	};
+	int mask, val;
+
+	if (chan != BU27034_CHAN_DATA0 && chan != BU27034_CHAN_DATA1)
+		return -EINVAL;
+
+	val = sel << BU27034_SHIFT_D01_GAIN;
+	mask = BU27034_MASK_D01_GAIN;
+
+	if (chan == BU27034_CHAN_DATA0) {
+		/*
+		 * We keep the same gain for channel 2 as we set for channel 0
+		 * We can't allow them to be individually controlled because
+		 * setting one will impact also the other. Also, if we don't
+		 * always update both gains we may result unsupported bit
+		 * combinations.
+		 *
+		 * This is not nice but this is yet another place where the
+		 * user space must be prepared to surprizes. Namely, see chan 2
+		 * gain changed when chan 0 gain is changed.
+		 *
+		 * This is not fatal for most users though. I don't expect the
+		 * channel 2 to be used in any generic cases - the intensity
+		 * values provided by the sensor for IR area are not openly
+		 * documented. Also, channel 2 is not used for visible light.
+		 *
+		 * So, if there is application which is written to utilize the
+		 * channel 2 - then it is probably specifically targeted to this
+		 * sensor and knows how to utilize those values. It is safe to
+		 * hope such user can also cope with the gain changes.
+		 */
+		mask |=  BU27034_MASK_D2_GAIN_LO;
+
+		/*
+		 * The D2 gain bits are directly the lowest bits of selector.
+		 * Just do add those bits to the value
+		 */
+		val |= sel & BU27034_MASK_D2_GAIN_LO;
+	}
+
+	return regmap_update_bits(data->regmap, reg[chan], mask, val);
+}
+
+static int bu27034_set_gain(struct bu27034_data *data, int chan, int gain)
+{
+	int ret;
+
+	/*
+	 * We don't allow setting channel 2 gain as it messes up the
+	 * gain for channel 0 - which shares the high bits
+	 */
+	if (chan != BU27034_CHAN_DATA0 && chan != BU27034_CHAN_DATA1)
+		return -EINVAL;
+
+	ret = iio_gts_find_sel_by_gain(&data->gts, gain);
+	if (ret < 0)
+		return ret;
+
+	return bu27034_write_gain_sel(data, chan, ret);
+}
+
+/* Caller should hold the lock to protect data->int_time */
+static int bu27034_set_int_time(struct bu27034_data *data, int time)
+{
+	int ret;
+
+	ret = iio_gts_find_sel_by_int_time(&data->gts, time);
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
+				 BU27034_MASK_MEAS_MODE, ret);
+}
+
+/*
+ * We try to change the time in such way that the scale is maintained for
+ * given channels by adjusting gain so that it compensates the time change.
+ */
+static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
+{
+	struct bu27034_gain_check gains[] = {
+		{ .chan = BU27034_CHAN_DATA0 },
+		{ .chan = BU27034_CHAN_DATA1 },
+	};
+	int numg = ARRAY_SIZE(gains);
+	int ret, int_time_old, i;
+
+	mutex_lock(&data->mutex);
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		goto unlock_out;
+
+	int_time_old = ret;
+
+	if (!iio_gts_valid_time(&data->gts, time_us)) {
+		dev_err(data->dev, "Unsupported integration time %u\n",
+			time_us);
+		ret = -EINVAL;
+
+		goto unlock_out;
+	}
+
+	if (time_us == int_time_old) {
+		ret = 0;
+		goto unlock_out;
+	}
+
+	for (i = 0; i < numg; i++) {
+		ret = bu27034_get_gain(data, gains[i].chan, &gains[i].old_gain);
+		if (ret)
+			goto unlock_out;
+
+		gains[i].new_gain = gains[i].old_gain * int_time_old / time_us;
+
+		if (!iio_gts_valid_gain(&data->gts, gains[i].new_gain)) {
+			int scale1, scale2;
+			bool ok;
+
+			_bu27034_get_scale(data, gains[i].chan, &scale1, &scale2);
+			dev_dbg(data->dev,
+				"chan %u, can't support time %u with scale %u %u\n",
+				gains[i].chan, time_us, scale1, scale2);
+
+			/*
+			 * If caller requests for integration time change and we
+			 * can't support the scale - then the caller should be
+			 * prepared to 'pick up the pieces and deal with the
+			 * fact that the scale changed'.
+			 */
+			ret = iio_find_closest_gain_low(&data->gts,
+							gains[i].new_gain, &ok);
+
+			if (!ok) {
+				dev_dbg(data->dev,
+					"optimal gain out of range for chan %u\n",
+					gains[i].chan);
+			}
+			if (ret < 0) {
+				dev_dbg(data->dev,
+					 "Total gain increase. Risk of saturation");
+				ret = iio_gts_get_min_gain(&data->gts);
+				if (ret < 0)
+					goto unlock_out;
+			}
+			dev_dbg(data->dev, "chan %u scale changed\n",
+				 gains[i].chan);
+			gains[i].new_gain = ret;
+			dev_dbg(data->dev, "chan %u new gain %u\n",
+				gains[i].chan, gains[i].new_gain);
+		}
+	}
+
+	for (i = 0; i < numg; i++) {
+		ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
+		if (ret)
+			goto unlock_out;
+	}
+
+	ret = bu27034_set_int_time(data, time_us);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bu27034_set_scale(struct bu27034_data *data, int chan,
+			    int val, int val2)
+{
+	int ret, time_sel, gain_sel, i;
+	bool found = false;
+
+	if (chan == BU27034_CHAN_DATA2)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel);
+	if (ret)
+		goto unlock_out;
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
+						val, val2 * 1000, &gain_sel);
+	if (ret) {
+		/*
+		 * Could not support scale with given time. Need to change time.
+		 * We still want to maintain the scale for all channels
+		 */
+		struct bu27034_gain_check gain;
+		int new_time_sel;
+
+		/*
+		 * Populate information for the other channel which should also
+		 * maintain the scale. (Due to the HW limitations the chan2
+		 * gets the same gain as chan0, so we only need to explicitly
+		 * set the chan 0 and 1).
+		 */
+		if (chan == BU27034_CHAN_DATA0)
+			gain.chan = BU27034_CHAN_DATA1;
+		else if (chan == BU27034_CHAN_DATA1)
+			gain.chan = BU27034_CHAN_DATA0;
+
+		ret = bu27034_get_gain(data, gain.chan, &gain.old_gain);
+		if (ret)
+			goto unlock_out;
+
+		/*
+		 * Iterate through all the times to see if we find one which
+		 * can support requested scale for requested channel, while
+		 * maintaining the scale for other channels
+		 */
+		for (i = 0; i < data->gts.num_itime; i++) {
+			new_time_sel = data->gts.itime_table[i].sel;
+
+			if (new_time_sel == time_sel)
+				continue;
+
+			/* Can we provide requested scale with this time? */
+			ret = iio_gts_find_gain_sel_for_scale_using_time(
+				&data->gts, new_time_sel, val, val2 * 1000,
+				&gain_sel);
+			if (ret)
+				continue;
+
+			/* Can the othe channel(s) maintain scale? */
+			ret = iio_gts_find_new_gain_sel_by_old_gain_time(
+				&data->gts, gain.old_gain, time_sel,
+				new_time_sel, &gain.new_gain);
+			if (!ret) {
+				/* Yes - we found suitable time */
+				found = true;
+				break;
+			}
+		}
+		if (!found) {
+			dev_dbg(data->dev,
+				"Can't set scale maintaining other channels\n");
+			ret = -EINVAL;
+
+			goto unlock_out;
+		}
+
+		for (i = 0; i < 2; i++) {
+			ret = bu27034_set_gain(data, gain.chan,
+						gain.new_gain);
+			if (ret)
+				goto unlock_out;
+		}
+
+		ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
+				  BU27034_MASK_MEAS_MODE, new_time_sel);
+		if (ret)
+			goto unlock_out;
+	}
+
+	ret = bu27034_write_gain_sel(data, chan, gain_sel);
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+/*
+ * for (D1/D0 < 0.87):
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 +
+ *	0.00012213 * D1 * D1 / D0
+ *
+ * =>	115.7400832 * ch1 / gain1 / mt -
+ *	68.1982976 * ch0 / gain0 / mt +
+ *	0.00012213 * 25600 * (ch1 / gain1 / mt) * 25600 *
+ *	(ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ *
+ * A =	0.00012213 * 25600 * (ch1 /gain1 / mt) * 25600 *
+ *	(ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ * =>	0.00012213 * 25600 * (ch1 /gain1 / mt) *
+ *	(ch1 /gain1 / mt) / (ch0 / gain0 / mt)
+ * =>	0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) /
+ *	(ch0 / gain0)
+ * =>	0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) *
+ *	gain0 / ch0
+ * =>	3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /ch0
+ *
+ * lx = (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ *	mt + A
+ * =>	(115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ *	mt + 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /
+ *	ch0
+ *
+ * =>	(115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0 +
+ *	  3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) /
+ *	  mt
+ *
+ * For (0.87 <= D1/D0 < 1.00)
+ * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 0.87) * (0.385) + 1)
+ * =>	(0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ *	100 * ch1 / gain1 / mt) * ((D1/D0 -  0.87) * (0.385) + 1)
+ * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	((D1/D0 -  0.87) * (0.385) + 1)
+ * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	(0.385 * D1/D0 - 0.66505)
+ * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	(0.385 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) - 0.66505)
+ * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	(9856 * ch1 / gain1 / mt / (25600 * ch0 / gain0 / mt) + 0.66505)
+ * =>	13.118336 * ch1 / (gain1 * mt)
+ *	+ 22.66064768 * ch0 / (gain0 * mt)
+ *	+ 8931.90144 * ch1 * ch1 * gain0 /
+ *	  (25600 * ch0 * gain1 * gain1 * mt)
+ *	+ 0.602694912 * ch1 / (gain1 * mt)
+ *
+ * =>	[0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1)
+ *	 + 22.66064768 * ch0 / gain0
+ *	 + 13.721030912 * ch1 / gain1
+ *	] / mt
+ *
+ * For (D1/D0 >= 1.00)
+ *
+ * lx	= (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 2.0) * (-0.05) + 1)
+ *	=> (0.001331* D0 + 0.0000354 * D1) * (-0.05D1/D0 + 1.1)
+ *	=> (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ *	   100 * ch1 / gain1 / mt) * (-0.05D1/D0 + 1.1)
+ *	=> (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	   (-0.05 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) + 1.1)
+ *	=> (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ *	   (-1280 * ch1 / (gain1 * mt * 25600 * ch0 / gain0 / mt) + 1.1)
+ *	=> (34.0736 * ch0 * -1280 * ch1 * gain0 * mt /( gain0 * mt * gain1 * mt * 25600 * ch0)
+ *	    + 34.0736 * 1.1 * ch0 / (gain0 * mt)
+ *	    + 0.90624 * ch1 * -1280 * ch1 *gain0 * mt / (gain1 * mt *gain1 * mt * 25600 * ch0)
+ *	    + 1.1 * 0.90624 * ch1 / (gain1 * mt)
+ *	=> -43614.208 * ch1 / (gain1 * mt * 25600)
+ *	    + 37.48096  ch0 / (gain0 * mt)
+ *	    - 1159.9872 * ch1 * ch1 * gain0 / (gain1 * gain1 * mt * 25600 * ch0)
+ *	    + 0.996864 ch1 / (gain1 * mt)
+ *	=> [
+ *		- 0.045312 * ch1 * ch1 * gain0 / (gain1 * gain1 * ch0)
+ *		- 0.706816 * ch1 / gain1
+ *		+ 37.48096  ch0 /gain0
+ *	   ] * mt
+ *
+ *
+ * So, the first case (D1/D0 < 0.87) can be computed to a form:
+ *
+ * lx = (3.126528 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ *	 115.7400832 * ch1 / gain1 +
+ *	-68.1982976 * ch0 / gain0
+ *	 / mt
+ *
+ * Second case (0.87 <= D1/D0 < 1.00) goes to form:
+ *
+ *	=> [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ *	    13.721030912 * ch1 / gain1 +
+ *	    22.66064768 * ch0 / gain0
+ *	   ] / mt
+ *
+ * Third case (D1/D0 >= 1.00) goes to form:
+ *	=> [-0.045312 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ *	    -0.706816 * ch1 / gain1 +
+ *	    37.48096  ch0 /(gain0
+ *	   ] / mt
+ *
+ * This can be unified to format:
+ * lx = [
+ *	 A * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ *	 B * ch1 / gain1 +
+ *	 C * ch0 / gain0
+ *	] / mt
+ *
+ * For case 1:
+ * A = 3.126528,
+ * B = 115.7400832
+ * C = -68.1982976
+ *
+ * For case 2:
+ * A = 0.3489024
+ * B = 13.721030912
+ * C = 22.66064768
+ *
+ * For case 3:
+ * A = -0.045312
+ * B = -0.706816
+ * C = 37.48096
+ */
+
+struct bu27034_lx_coeff {
+	unsigned int A;
+	unsigned int B;
+	unsigned int C;
+	/* Indicate which of the coefficients above are negative */
+	bool is_neg[3];
+};
+
+static inline u64 gain_mul_div_helper(u64 val, unsigned int gain,
+				      unsigned int div)
+{
+	/*
+	 * Max gain for a channel is 4096. The max u64 (0xffffffffffffffffULL)
+	 * divided by 4096 is 0xFFFFFFFFFFFFF (GENMASK_ULL(51, 0)) (floored).
+	 * Thus, the 0xFFFFFFFFFFFFF is the largest value we can safely multiply
+	 * with the gain, no matter what gain is set.
+	 *
+	 * So, multiplication with max gain may overflow if val is greater than
+	 * 0xFFFFFFFFFFFFF (52 bits set)..
+	 *
+	 * If this is the case we divide first.
+	 */
+	if (val < GENMASK_ULL(51, 0)) {
+		val *= gain;
+		do_div(val, div);
+	} else {
+		do_div(val, div);
+		val *= gain;
+	}
+
+	return val;
+}
+
+static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
+				unsigned int ch1, unsigned int gain0,
+				unsigned int gain1)
+{
+	unsigned int helper, tmp;
+	u64 helper64;
+
+	/*
+	 * Here we could overflow even the 64bit value. Hence we
+	 * multiply with gain0 only after the divisions - even though
+	 * it may result loss of accuracy
+	 */
+	helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
+	helper = coeff * ch1 * ch1;
+	tmp = helper * gain0;
+
+	if (helper == helper64 && (tmp / gain0 == helper))
+		return tmp / (gain1 * gain1) / ch0;
+
+	helper = gain1 * gain1;
+	if (helper > ch0) {
+		do_div(helper64, helper);
+
+		return  gain_mul_div_helper(helper64, gain0, ch0);
+	}
+
+	do_div(helper64, ch0);
+
+	return  gain_mul_div_helper(helper64, gain0, helper);
+}
+
+static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
+				 unsigned int gain)
+{
+	unsigned int helper;
+	u64 helper64;
+
+	helper64 = (u64)coeff * (u64)ch;
+	helper = coeff * ch;
+
+	if (helper == helper64)
+		return helper / gain;
+
+	do_div(helper64, gain);
+
+	return helper64;
+}
+
+static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1,
+				unsigned int gain0, unsigned int gain1,
+				unsigned int meastime, int coeff_idx)
+{
+	static const struct bu27034_lx_coeff coeff[] = {
+		{
+			.A = 31265280,		/* 3.126528 */
+			.B = 1157400832,	/*115.7400832 */
+			.C = 681982976,		/* -68.1982976 */
+			.is_neg = {false, false, true},
+		}, {
+			.A = 3489024,		/* 0.3489024 */
+			.B = 137210309,		/* 13.721030912 */
+			.C = 226606476,		/* 22.66064768 */
+			/* All terms positive */
+		}, {
+			.A = 453120,		/* -0.045312 */
+			.B = 7068160,		/* -0.706816 */
+			.C = 374809600,		/* 37.48096 */
+			.is_neg = {true, true, false},
+		}
+	};
+	const struct bu27034_lx_coeff *c = &coeff[coeff_idx];
+	u64 res = 0, terms[3];
+	int i;
+
+	if (coeff_idx >= ARRAY_SIZE(coeff))
+		return -EINVAL;
+
+	terms[0] = bu27034_fixp_calc_t1(c->A, ch0, ch1, gain0, gain1);
+	terms[1] = bu27034_fixp_calc_t23(c->B, ch1, gain1);
+	terms[2] = bu27034_fixp_calc_t23(c->C, ch0, gain0);
+
+	/* First, add positive terms */
+	for (i = 0; i < 3; i++)
+		if (!c->is_neg[i])
+			res += terms[i];
+
+	/* No positive term => zero lux */
+	if (!res)
+		return 0;
+
+	/* Then, subtract negative terms (if any) */
+	for (i = 0; i < 3; i++)
+		if (c->is_neg[i]) {
+			/*
+			 * If the negative term is greater than positive - then
+			 * the darknes has taken over and we are all doomed! Eh,
+			 * I mean, then we can just return 0 lx and go out
+			 */
+			if (terms[i] >= res)
+				return 0;
+
+			res -= terms[i];
+		}
+
+	meastime *= 10000;
+	do_div(res, meastime);
+
+	return (int) res;
+}
+
+static bool bu27034_has_valid_sample(struct bu27034_data *data)
+{
+	int ret, val;
+
+	ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL4, &val);
+	if (ret) {
+		dev_err(data->dev, "Read failed %d\n", ret);
+
+		return false;
+	}
+
+	return val & BU27034_MASK_VALID;
+}
+
+/*
+ * Reading the register where VALID bit is clears this bit. (So does changing
+ * any gain / integration time configuration registers) The bit gets
+ * set when we have acquired new data. We use this bit to indicate data
+ * validity.
+ */
+static void bu27034_invalidate_read_data(struct bu27034_data *data)
+{
+	bu27034_has_valid_sample(data);
+}
+
+static int bu27034_read_result(struct bu27034_data *data, int chan, int *res)
+{
+	int reg[] = {
+		[BU27034_CHAN_DATA0] = BU27034_REG_DATA0_LO,
+		[BU27034_CHAN_DATA1] = BU27034_REG_DATA1_LO,
+		[BU27034_CHAN_DATA2] = BU27034_REG_DATA2_LO,
+	};
+	int valid, ret;
+	__le16 val;
+
+	ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
+				       valid, (valid & BU27034_MASK_VALID),
+				       BU27034_DATA_WAIT_TIME_US, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, reg[chan], &val, sizeof(val));
+	if (ret)
+		return ret;
+
+	*res = le16_to_cpu(val);
+
+	return 0;
+}
+
+static int bu27034_get_result_unlocked(struct bu27034_data *data, __le16 *res,
+				       int size)
+{
+	int ret = 0, retry_cnt = 0;
+
+retry:
+	/* Get new value from sensor if data is ready */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+				       res, size);
+		if (ret)
+			return ret;
+
+		bu27034_invalidate_read_data(data);
+	} else {
+		retry_cnt++;
+
+		/* No new data in sensor. Wait and retry */
+		msleep(25);
+
+		if (retry_cnt > BU27034_RETRY_LIMIT) {
+			dev_err(data->dev, "No data from sensor\n");
+
+			return -ETIMEDOUT;
+		}
+
+		goto retry;
+	}
+
+	return ret;
+}
+
+static int bu27034_meas_set(struct bu27034_data *data, bool en)
+{
+	if (en)
+		return regmap_set_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+				       BU27034_MASK_MEAS_EN);
+
+	return regmap_clear_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+				 BU27034_MASK_MEAS_EN);
+}
+
+static int bu27034_get_single_result(struct bu27034_data *data, int chan,
+				     int *val)
+{
+	int ret;
+
+	ret = bu27034_meas_set(data, true);
+	if (ret)
+		return ret;
+
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		return ret;
+
+	msleep(ret / 1000);
+
+	return bu27034_read_result(data, chan, val);
+}
+
+/*
+ * The formula given by vendor for computing luxes out of data0 and data1
+ * (in open air) is as follows:
+ *
+ * Let's mark:
+ * D0 = data0/ch0_gain/meas_time_ms * 25600
+ * D1 = data1/ch1_gain/meas_time_ms * 25600
+ *
+ * Then:
+ * if (D1/D0 < 0.87)
+ *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1)
+ * else if (D1/D0 < 1)
+ *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1)
+ * else
+ *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1)
+ *
+ * We use it here. Users who have for example some colored lens
+ * need to modify the calculation but I hope this gives a starting point for
+ * those working with such devices.
+ */
+
+static int bu27034_calc_lux(struct bu27034_data *data, __le16 *res, int *val)
+{
+	unsigned int gain0, gain1, meastime;
+	unsigned int d1_d0_ratio_scaled;
+	u16  ch0, ch1;
+	u64 helper64;
+	int ret;
+
+	/*
+	 * We return 0 luxes if calculation fails. This should be reasonably
+	 * easy to spot from the buffers especially if raw-data channels show
+	 * valid values
+	 */
+	*val = 0;
+
+	ch0 = max_t(u16, 1, le16_to_cpu(res[0]));
+	ch1 = max_t(u16, 1, le16_to_cpu(res[1]));
+
+	ret = bu27034_get_gain(data, BU27034_CHAN_DATA0, &gain0);
+	if (ret)
+		return ret;
+
+	ret = bu27034_get_gain(data, BU27034_CHAN_DATA1, &gain1);
+	if (ret)
+		return ret;
+
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		return ret;
+
+	meastime = ret;
+
+	d1_d0_ratio_scaled = (unsigned int)ch1 * (unsigned int)gain0 * 100;
+	helper64 = (u64)ch1 * (u64)gain0 * 100LLU;
+
+	if (helper64 != d1_d0_ratio_scaled) {
+		unsigned int div = (unsigned int)ch0 * gain1;
+
+		do_div(helper64, div);
+		d1_d0_ratio_scaled = helper64;
+	} else {
+		d1_d0_ratio_scaled /= ch0 * gain1;
+	}
+
+	if (d1_d0_ratio_scaled < 87)
+		ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0);
+	else if (d1_d0_ratio_scaled < 100)
+		ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1);
+	else
+		ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2);
+
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+
+}
+
+static int bu27034_get_lux(struct bu27034_data *data, int *val)
+{
+	__le16 res[3];
+	int ret;
+
+	ret = bu27034_meas_set(data, true);
+	if (ret)
+		return ret;
+
+	ret = bu27034_get_result_unlocked(data, &res[0], sizeof(res));
+	if (ret)
+		return ret;
+
+	ret =  bu27034_calc_lux(data, res, val);
+	if (ret)
+		return ret;
+
+	ret = bu27034_meas_set(data, false);
+	if (ret)
+		dev_err(data->dev, "failed to disable measurement\n");
+
+	return 0;
+}
+
+static int bu27034_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = bu27034_get_int_time(data);
+		if (*val < 0)
+			return *val;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		return bu27034_get_scale(data, chan->channel, val, val2);
+
+	case IIO_CHAN_INFO_RAW:
+	{
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)
+			return -EINVAL;
+
+		/* Don't mess with measurement enabling while buffering */
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		/*
+		 * Reading one channel at a time is ineffiecient but we don't
+		 * care here. Buffered version should be used if performance is
+		 * an issue.
+		 */
+		ret = bu27034_get_single_result(data, chan->channel, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+
+		/* Don't mess with measurement enabling while buffering */
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+
+		ret = bu27034_get_lux(data, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+
+	}
+}
+
+static int bu27034_write_raw(struct iio_dev *idev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	int ret;
+
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = bu27034_set_scale(data, chan->channel, val, val2);
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = bu27034_try_set_int_time(data, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	iio_device_release_direct_mode(idev);
+
+	return ret;
+}
+
+static int bu27034_read_avail(struct iio_dev *idev,
+			      struct iio_chan_spec const *chan, const int **vals,
+			      int *type, int *length, long mask)
+{
+	struct bu27034_data *data = iio_priv(idev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bu27034_info = {
+	.read_raw = &bu27034_read_raw,
+	.write_raw = &bu27034_write_raw,
+	.read_avail = &bu27034_read_avail,
+};
+
+static int bu27034_chip_init(struct bu27034_data *data)
+{
+	int ret, sel;
+
+	/* Reset */
+	ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+			   BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+	msleep(1);
+	/*
+	 * Read integration time here to ensure it is in regmap cache. We do
+	 * this to speed-up the int-time acquisition in the start of the buffer
+	 * handling thread where longer delays could make it more likely we end
+	 * up skipping a sample, and where the longer delays make timestamps
+	 * less accurate.
+	 */
+	ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel);
+	if (ret)
+		dev_err(data->dev, "reading integration time failed\n");
+
+	return 0;
+}
+
+static int bu27034_wait_for_data(struct bu27034_data *data)
+{
+	int ret, val;
+
+	ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
+				       val, val & BU27034_MASK_VALID,
+				       BU27034_DATA_WAIT_TIME_US,
+				       BU27034_TOTAL_DATA_WAIT_TIME_US);
+	if (ret) {
+		dev_err(data->dev, "data polling %s\n",
+			!(val & BU27034_MASK_VALID) ? "timeout" : "fail");
+
+		return ret;
+	}
+
+	ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+			       &data->scan.channels[0],
+			       sizeof(data->scan.channels));
+	if (ret)
+		return ret;
+
+	bu27034_invalidate_read_data(data);
+
+	return 0;
+}
+
+static int bu27034_buffer_thread(void *arg)
+{
+	struct iio_dev *idev = arg;
+	struct bu27034_data *data;
+	int wait_ms;
+
+	data = iio_priv(idev);
+
+	wait_ms = bu27034_get_int_time(data);
+	wait_ms /= 1000;
+
+	wait_ms -=  BU27034_MEAS_WAIT_PREMATURE_MS;
+
+	while (!kthread_should_stop()) {
+		int ret;
+		int64_t tstamp;
+
+		msleep(wait_ms);
+		ret = bu27034_wait_for_data(data);
+		if (ret)
+			continue;
+
+		tstamp = iio_get_time_ns(idev);
+
+		if (test_bit(BU27034_CHAN_ALS, idev->active_scan_mask)) {
+			int lux;
+
+			ret = bu27034_calc_lux(data, &data->scan.channels[0],
+					       &lux);
+			if (ret)
+				dev_err(data->dev, "failed to calculate lux\n");
+
+			/*
+			 * The maximum milli lux value we get with gain 1x time
+			 * 55mS data ch0 = 0xffff ch1 = 0xffff fits in 26 bits
+			 * so there should be no problem returning int from
+			 * computations and casting it to u32
+			 */
+			data->scan.lux = (u32)lux;
+		}
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+	}
+
+	return 0;
+}
+
+static int bu27034_buffer_enable(struct iio_dev *idev)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	struct task_struct *task;
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = bu27034_meas_set(data, true);
+	if (ret)
+		goto unlock_out;
+
+	task = kthread_run(bu27034_buffer_thread, idev,
+				 "bu27034-buffering-%u",
+				 iio_device_id(idev));
+	if (IS_ERR(task)) {
+		ret = PTR_ERR(task);
+		goto unlock_out;
+	}
+
+	data->task = task;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bu27034_buffer_disable(struct iio_dev *idev)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	if (data->task) {
+		kthread_stop(data->task);
+		data->task = NULL;
+	}
+
+	ret = bu27034_meas_set(data, false);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops bu27034_buffer_ops = {
+	.postenable = &bu27034_buffer_enable,
+	.predisable = &bu27034_buffer_disable,
+};
+
+static int bu27034_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct bu27034_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	unsigned int part_id, reg;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	data = iio_priv(idev);
+
+	ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &reg);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	part_id = FIELD_GET(BU27034_MASK_PART_ID, reg);
+
+	if (part_id != BU27034_ID)
+		dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+	ret = iio_init_iio_gts(BU27034_SCALE_1X, 0, bu27034_gains,
+			       ARRAY_SIZE(bu27034_gains), bu27034_itimes,
+			       ARRAY_SIZE(bu27034_itimes), &data->gts);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_gts_build_avail_tables(dev, &data->gts);
+	if (ret)
+		return ret;
+
+	mutex_init(&data->mutex);
+	data->regmap = regmap;
+	data->dev = dev;
+
+	idev->channels = bu27034_channels;
+	idev->num_channels = ARRAY_SIZE(bu27034_channels);
+	idev->name = "bu27034";
+	idev->info = &bu27034_info;
+
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	idev->available_scan_masks = bu27034_scan_masks;
+
+	ret = bu27034_chip_init(data);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops);
+	if (ret)
+		return dev_err_probe(dev, ret, "buffer setup failed\n");
+
+	ret = devm_iio_device_register(dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return ret;
+}
+
+static const struct of_device_id bu27034_of_match[] = {
+	{ .compatible = "rohm,bu27034" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bu27034_of_match);
+
+static struct i2c_driver bu27034_i2c_driver = {
+	.driver = {
+		.name = "bu27034-als",
+		.of_match_table = bu27034_of_match,
+	  },
+	.probe_new = bu27034_probe,
+};
+module_i2c_driver(bu27034_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("ROHM BU27034 ambient light sensor driver");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
-- 
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 v4 7/8] iio: light: ROHM BU27034 Ambient Light Sensor

[Matti Vaittinen]

On 3/17/23 16:44, Matti Vaittinen wrote:
> 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Changes
> 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
> 

And right after sending out this version I realized I forgot to run 
spell-checker for the comments. I will do that for v5 - please bear with me.

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 v4 5/8] iio: test: test gain-time-scale helpers

[kernel test robot]

Hi Matti,

I love your patch! Yet something to improve:

[auto build test ERROR on eeac8ede17557680855031c6f305ece2378af326]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/drm-tests-helpers-rename-generic-helpers/20230317-224949
base:   eeac8ede17557680855031c6f305ece2378af326
patch link:    https://lore.kernel.org/r/31cf5765078b2d808d9e66eb623cde70ee6478ac.1679062529.git.mazziesaccount%40gmail.com
patch subject: [PATCH v4 5/8] iio: test: test gain-time-scale helpers
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230318/202303180046.bcS0xv8j-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/37a2c62820ca249a8d3ab9f1b80eaa71e0d5749b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matti-Vaittinen/drm-tests-helpers-rename-generic-helpers/20230317-224949
        git checkout 37a2c62820ca249a8d3ab9f1b80eaa71e0d5749b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/base/test/ drivers/iio/test/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303180046.bcS0xv8j-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/test/iio-test-gts.c:7:10: fatal error: kunit/platform_device.h: No such file or directory
       7 | #include <kunit/platform_device.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.
--
>> drivers/base/test/test_kunit_device.c:11:10: fatal error: kunit/platform_device.h: No such file or directory
      11 | #include <kunit/platform_device.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +7 drivers/iio/test/iio-test-gts.c

   > 7	#include <kunit/platform_device.h>
     8	#include <kunit/test.h>
     9	#include <linux/iio/iio-gts-helper.h>
    10	#include <linux/iio/types.h>
    11	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v4 0/8] Support ROHM BU27034 ALS sensor

[Jonathan Cameron]

On Fri, 17 Mar 2023 16:40:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Support ROHM BU27034 ALS sensor

Hi Matti,

For ease of when this is ready to apply, better to just keep
key mailing lists and individuals cc'd on all patches.

Mind you cc list is random enough I'm guessing it wasn't
deliberate (like the maintainers patch 8 only went to lkml
where no one will notice it)

I can scrape these all of lore, but it's a step that not
all reviewers are going to bother with.

Jonathan



> 
> 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'. There were neat helpers in DRM tests for creating and freeing
> such a device. This series moves those helpers to more generic location.
> What is worth noting is that there is something similar ongoing in the
> CCF territory:
> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
> These efforts should be somehow coordinated in order to avoid any ajor
> conflicts.
> 
> 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).
> 
> Revision history:
> v3 => v4: (Stil ostly 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 (8):
>   drm/tests: helpers: rename generic helpers
>   kunit: drm/tests: move generic helpers
>   dt-bindings: iio: light: Support ROHM BU27034
>   iio: light: Add gain-time-scale helpers
>   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/base/test/Kconfig                     |    5 +
>  drivers/base/test/Makefile                    |    2 +
>  drivers/base/test/test_kunit_device.c         |   83 +
>  drivers/gpu/drm/Kconfig                       |    2 +
>  .../gpu/drm/tests/drm_client_modeset_test.c   |    5 +-
>  drivers/gpu/drm/tests/drm_kunit_helpers.c     |   69 -
>  drivers/gpu/drm/tests/drm_managed_test.c      |    5 +-
>  drivers/gpu/drm/tests/drm_modes_test.c        |    5 +-
>  drivers/gpu/drm/tests/drm_probe_helper_test.c |    5 +-
>  drivers/gpu/drm/vc4/Kconfig                   |    1 +
>  drivers/gpu/drm/vc4/tests/vc4_mock.c          |    3 +-
>  .../gpu/drm/vc4/tests/vc4_test_pv_muxing.c    |    9 +-
>  drivers/iio/Kconfig                           |    3 +
>  drivers/iio/Makefile                          |    1 +
>  drivers/iio/industrialio-gts-helper.c         |  990 +++++++++++
>  drivers/iio/light/Kconfig                     |   14 +
>  drivers/iio/light/Makefile                    |    1 +
>  drivers/iio/light/rohm-bu27034.c              | 1491 +++++++++++++++++
>  drivers/iio/test/Kconfig                      |   16 +
>  drivers/iio/test/Makefile                     |    1 +
>  drivers/iio/test/iio-test-gts.c               |  461 +++++
>  include/drm/drm_kunit_helpers.h               |    7 +-
>  include/linux/iio/iio-gts-helper.h            |  113 ++
>  25 files changed, 3265 insertions(+), 87 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
>  create mode 100644 drivers/base/test/test_kunit_device.c
>  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/linux/iio/iio-gts-helper.h
> 
> 
> base-commit: eeac8ede17557680855031c6f305ece2378af326

Re: [PATCH v4 4/8] iio: light: Add gain-time-scale helpers

[Jonathan Cameron]

On Fri, 17 Mar 2023 16:43:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Whilst you use it in the tests currently I'm not convinced there is a good
reason to separate iio_init_iio_gts() from devm_iio_gts_build_avail_tables()
as I'd expect them to be called as a pair in all drivers that use this.

Perhaps it's worth reworking the tests to do that even if it's not strictly
necessary for specific tests.

I think a bit more care is need with storage of time (unsigned) + decide
whether to allow for negative gains. Whilst they happen I'm not that bothered
if that subtlety becomes a device driver problem when calling this.  I'm not
sure I've seen a sensor that does both positive and negative gains for a single
channel.

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

Ah. I've always regretted not using iio_ for the prefix on those so I'm fine
if you would prefer to stick to iio_


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

> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> new file mode 100644
> index 000000000000..9494ea7cdbcf
> --- /dev/null
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -0,0 +1,990 @@
...

> +
> +static const struct iio_itime_sel_mul *
> +iio_gts_find_itime_by_time(struct iio_gts *gts, int time)

Time going to be positive (I hope!)
So as below, I'd make all time values unsigned.

> +{
> +	int i;
> +
> +	if (!gts->num_itime)
> +		return NULL;
> +
> +	for (i = 0; i < gts->num_itime; i++)
> +		if (gts->itime_table[i].time_us == time)
> +			return &gts->itime_table[i];
> +
> +	return NULL;
> +}

...

> +/**
> + * iio_gts_purge_avail_scale_table - free-up the available scale tables
> + * @gts:	Gain time scale descriptor
> + *
> + * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
> + * that the helpers for getting available scales like the
> + * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
> + * be only called after these helpers can no longer be called (Eg. after
> + * the iio-device has been deregistered).

Whilst I'm not that keen on the comment in general, if you really really want to
have it we need to figure out one place to put it rather than lots of duplicates.

> + */
...


> +static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> +{
> +	int ret, i, j, new_idx, time_idx;
> +	int *all_gains;
> +	size_t gain_bytes;
> +
> +	for (i = 0; i < gts->num_itime; i++) {
> +		/*
> +		 * Sort the tables for nice output and for easier finding of
> +		 * unique values.
> +		 */
> +		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
> +		     NULL);
> +
> +		/* Convert gains to scales */
> +		for (j = 0; j < gts->num_hwgain; j++) {
> +			ret = iio_gts_total_gain_to_scale(gts, gains[i][j],
> +							  &scales[i][2 * j],
> +							  &scales[i][2 * j + 1]);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	gain_bytes = array_size(gts->num_hwgain, sizeof(int));
> +	all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
> +	if (!all_gains)
> +		return -ENOMEM;
> +
> +	/*
> +	 * We assume all the gains for same integration time were unique.
> +	 * It is likely the first time table had greatest time multiplier as
> +	 * the times are in the order of preference and greater times are
> +	 * usually preferred. Hence we start from the last table which is likely
> +	 * to have the smallest total gains.
> +	 */
> +	time_idx = gts->num_itime - 1;
> +	memcpy(all_gains, gains[time_idx], gain_bytes);
> +	new_idx = gts->num_hwgain;
> +
> +	while (time_idx--) {
> +		for (j = 0; j < gts->num_hwgain; j++) {
> +			int candidate = gains[time_idx][j];
> +			int chk;
> +
> +			if (candidate > all_gains[new_idx - 1]) {
> +				all_gains[new_idx] = candidate;
> +				new_idx++;
> +
> +				continue;
> +			}
> +			for (chk = 0; chk < new_idx; chk++)
> +				if (candidate <= all_gains[chk])
> +					break;
> +
> +			if (candidate == all_gains[chk])
> +				continue;
> +
> +			memmove(&all_gains[chk + 1], &all_gains[chk],
> +				(new_idx - chk) * sizeof(int));
> +			all_gains[chk] = candidate;
> +			new_idx++;
> +		}
> +	}
> +
> +	gts->num_avail_all_scales = new_idx;
> +	gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
> +					      2 * sizeof(int), GFP_KERNEL);
> +	if (!gts->avail_all_scales_table)
> +		ret = -ENOMEM;
> +	else
> +		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
> +			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
> +					&gts->avail_all_scales_table[i * 2],
> +					&gts->avail_all_scales_table[i * 2 + 1]);
> +
> +	kfree(all_gains);
> +	if (ret)
> +		kfree(gts->avail_all_scales_table);

This is getting too clever.  I'd have an error handling block and gotos
even though it duplicates one line.  
> +
> +	return ret;
> +}
> +
> +/**
> + * iio_gts_build_avail_scale_table - create tables of available scales
> + * @gts:	Gain time scale descriptor
> + *
> + * Build the tables which can represent the available scales based on the
> + * originally given gain and time tables. When both time and gain tables are
> + * given this results:
> + * 1. A set of tables representing available scales for each supported
> + *    integration time.
> + * 2. A single table listing all the unique scales that any combination of
> + *    supported gains and times can provide.
> + *
> + * NOTE: Space allocated for the tables must be freed using
> + * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
> + *
> + * Return: 0 on success.
> + */
> +static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
> +{
> +	int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
> +
> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);

As per other thread, I much prefer reviewing code with sizeof(*per_time_gains)
as it requires fewer brain cells.

> +	if (!per_time_gains)
> +		return ret;
> +
> +	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> +	if (!per_time_scales)
> +		goto free_gains;
> +
> +	for (i = 0; i < gts->num_itime; i++) {
> +		per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int),
> +					     GFP_KERNEL);
> +		if (!per_time_scales[i])
> +			goto err_free_out;
> +
> +		per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int),
> +					    GFP_KERNEL);
> +		if (!per_time_gains[i])
> +			goto err_free_scale_out;

As below. I'd put kfree(per_time_scales[i]); here to simplify the paths to
the error handling block.

> +
> +
> +		for (j = 0; j < gts->num_hwgain; j++)
> +			per_time_gains[i][j] = gts->hwgain_table[j].gain *
> +					       gts->itime_table[i].mul;
> +	}
> +
> +	ret = gain_to_scaletables(gts, per_time_gains, per_time_scales);
> +	if (ret)
> +		goto err_free_out;

I'm not a fan of dancing backwards and forwards with exit paths. As such I'd move
the kfree(per_time_scales[i]) to the one condition where it matters above.

> +
> +	kfree(per_time_gains);
> +	gts->per_time_avail_scale_tables = per_time_scales;
> +
> +	return 0;
> +
> +err_free_scale_out:
> +	kfree(per_time_scales[i]);
> +err_free_out:
> +	for (i--; i; i--) {
> +		kfree(per_time_scales[i]);
> +		kfree(per_time_gains[i]);
> +	}
> +	kfree(per_time_scales);
> +free_gains:
> +	kfree(per_time_gains);
> +
> +	return ret;
> +}
> +
> +/**
> + * iio_gts_build_avail_time_table - build table of available integration times
> + * @gts:	Gain time scale descriptor
> + *
> + * Build the table which can represent the available times to be returned
> + * to users using the read_avail-callback.
> + *
> + * NOTE: Space allocated for the tables must be freed using
> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
> + *
> + * Return: 0 on success.
> + */
> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
> +{
> +	int *times, i, j, idx = 0;
> +
> +	if (!gts->num_itime)
> +		return 0;
> +
> +	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
> +	if (!times)
> +		return -ENOMEM;
> +
> +	for (i = gts->num_itime - 1; i >= 0; i--) {
> +		int new = gts->itime_table[i].time_us;
> +

This looks like a sort routine.  Don't we have something generic that will work?

> +		if (times[idx] < new) {
> +			times[idx++] = new;
> +			continue;
> +		}
> +
> +		for (j = 0; j <= idx; j++) {
> +			if (times[j] > new) {
> +				memmove(&times[j + 1], &times[j],
> +					(idx - j) * sizeof(int));
> +				times[j] = new;
> +				idx++;
> +			}
> +		}
> +	}
> +	gts->avail_time_tables = times;
> +	/*
> +	 * This is just to survive a unlikely corner-case where times in the
> +	 * given time table were not unique. Else we could just trust the
> +	 * gts->num_itime.

If integration times aren't unique I'd count it as driver bug and error out
/scream.  Papering over things like this just make code harder to review
to deal with what is probably a driver bug.

> +	 */
> +	gts->num_avail_time_tables = idx;
> +
> +	return 0;
> +}

...

> +/**
> + * iio_gts_purge_avail_tables - free-up the availability tables
> + * @gts:	Gain time scale descriptor
> + *
> + * Free the space reserved by iio_gts_build_avail_tables(). Frees both the
> + * integration time and scale tables.
> + *
> + * Note  that the helpers for getting available integration times or scales
> + * like the iio_gts_avail_times() are not usable after this call. Thus, this
> + * should be only called after these helpers can no longer be called (Eg.
> + * after the iio-device has been deregistered).
As below, I'm not sure the note adds much over normal use after free...
Also different formatting to the one below.

> + */
> +static void iio_gts_purge_avail_tables(struct iio_gts *gts)
> +{
> +	iio_gts_purge_avail_time_table(gts);
> +	iio_gts_purge_avail_scale_table(gts);
> +}
> +
> +static void devm_iio_gts_avail_all_drop(void *res)
> +{
> +	iio_gts_purge_avail_tables(res);
> +}
> +
> +/**
> + * devm_iio_gts_build_avail_tables - manged add availability tables
> + * @dev:	Pointer to the device whose lifetime tables are bound
> + * @gts:	Gain time scale descriptor
> + *
> + * Build the tables which can represent the available scales and available
> + * integration times. Availability tables are built based on the originally
> + * given gain and given time tables.
> + *
> + * When both time and gain tables are
> + * given this results:

odd line break.

> + * 1. A set of sorted tables representing available scales for each supported
> + *    integration time.
> + * 2. A single sorted table listing all the unique scales that any combination
> + *    of supported gains and times can provide.
> + * 3. A sorted table of supported integration times
> + *
> + * After these tables are built one can use the iio_gts_all_avail_scales(),
> + * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
> + * implement the read_avail opeations.
> + *
> + * The tables are automatically released upon device detach.
> + *
> + * NOTE: after the tables have been purged, the helpers for getting
> + * available scales / integration times are no longer usable. Care must be
> + * taken that unwinding is done in correct order (iio device is deregistered
> + * prior purging the tables).

Hmm. I think this note is calling out one potential path (even if it's the most
common one). I'd not bother with it as a driver should use no resources after
they've been freed and this is typically one of many.

> + *
> + * Return: 0 on success.
> + */
> +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts)
> +{
> +	int ret;
> +
> +	ret = iio_gts_build_avail_tables(gts);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, devm_iio_gts_avail_all_drop, gts);
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_tables);

> +/**
> + * iio_gts_valid_time - check if given integration time is valid
> + * @gts:	Gain time scale descriptor
> + * @time_us:	Integration time to check
> + *
> + * Return:	True if given time is supported by device. False if not.
> + */
> +bool iio_gts_valid_time(struct iio_gts *gts, int time_us)
> +{
> +	return iio_gts_find_itime_by_time(gts, time_us);
I'd make this a little more explicit as implicit casting pointer to bool is
rather unusual.
	!= NULL; maybe?

	
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_valid_time);
> +
> +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
> +{
> +	int i;
> +
> +	for (i = 0; i < gts->num_hwgain; i++)
> +		if (gts->hwgain_table[i].gain == gain)
> +			return gts->hwgain_table[i].sel;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
> +
> +bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
> +{
> +	return iio_gts_find_sel_by_gain(gts, gain) >= 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_valid_gain);

For the _valid_xxx functions, I wonder if you shouldn't just
push them to the header as static inline

> +
> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
> +{
> +	int i;
> +
> +	for (i = 0; i < gts->num_hwgain; i++)
> +		if (gts->hwgain_table[i].sel == sel)
> +			return gts->hwgain_table[i].gain;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
> +
> +int iio_gts_get_min_gain(struct iio_gts *gts)

Could just use min = INT_MAX;
(indirectly from linux/limits.h, it's actually in vdso/limits.h
but should not include that directly I think)
then I don't hink you need the special casing for the
first entry.

> +{
> +	int i, min = -EINVAL;
> +
> +	for (i = 0; i < gts->num_hwgain; i++) {
> +		int gain = gts->hwgain_table[i].gain;
> +
> +		if (min == -EINVAL)
> +			min = gain;
> +		else
> +			min = min(min, gain);
> +	}
> +
> +	return min;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_get_min_gain);
> +
> +/**
> + * iio_find_closest_gain_low - Find the closest lower matching gain
> + * @gts:	Gain time scale descriptor
> + * @gain:	reference gain for which the closest match is searched
> + * @in_range:	indicate if the reference gain was actually in the range of
> + *		supported gains.
> + *
> + * Search for closest supported gain that is lower than or equal to the
> + * gain given as a parameter. This is usable for drivers which do not require
> + * user to request exact matching gain but rather fo rounding to a supported
> + * gain value which is equal or lower (setting lower gain is typical for
> + * avoiding saturation)
> + *
> + * Return:	The closest matching supported gain or -EINVAL is reference

Maybe just say @gain was smaller.  reference gain does not have clear meaning to me.

> + *		gain was smaller than the smallest supported gain.
> + */
> +int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range)
> +{
> +	int i, diff = 0;
> +	int best = -1;
> +
> +	*in_range = false;
> +
> +	for (i = 0; i < gts->num_hwgain; i++) {
> +		/*
> +		 * It is not expected this function is called for an exactly
> +		 * matching gain.
I'd not mark it unlikely even so (or comment on it).
This is unlikely to be a particularly hot path so this unlikely() seems like a
premature optimization which adds complexity to the code.

> +		 */
> +		if (unlikely(gain == gts->hwgain_table[i].gain)) {
> +			*in_range = true;
> +			return gain;
> +		}
> +
> +		if (gain > gts->hwgain_table[i].gain) {
> +			if (!diff) {
> +				diff = gain - gts->hwgain_table[i].gain;
> +				best = i;
> +			} else {
> +				int tmp = gain - gts->hwgain_table[i].gain;
> +
> +				if (tmp < diff) {
> +					diff = tmp;
> +					best = i;
> +				}
> +			}
> +		} else {
> +			/*
> +			 * We found valid hwgain which is greater than
> +			 * reference. So, unless we return a failure below we
> +			 * will have found an in-range gain
> +			 */
> +			*in_range = true;
> +		}
> +	}
> +	/* The requested gain was smaller than anything we support */
> +	if (!diff) {
> +		*in_range = false;
> +
> +		return -EINVAL;
> +	}
> +
> +	return gts->hwgain_table[best].gain;
> +}
> +EXPORT_SYMBOL_GPL(iio_find_closest_gain_low);


...


> +/*
> + * iio_gts_find_gain_sel_for_scale_using_time - Fetch gain selector.
> + * See iio_gts_find_gain_for_scale_using_time() for more information

This is exported, so I'd rather see kernel-doc style comments.

> + */
> +int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel,
> +					       int scale_int, int scale_nano,
> +					       int *gain_sel)
> +{
> +	int gain, ret;
> +
> +	ret = iio_gts_find_gain_for_scale_using_time(gts, time_sel, scale_int,
> +						     scale_nano, &gain);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_gts_find_sel_by_gain(gts, gain);
> +	if (ret < 0)
> +		return ret;
> +
> +	*gain_sel = ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_sel_for_scale_using_time);
> +
> +int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel)
> +{
> +	const struct iio_itime_sel_mul *itime;
> +
> +	itime = iio_gts_find_itime_by_sel(gts, sel);
> +	if (!itime)
> +		return -EINVAL;
> +
> +	return itime->time_us;

Currently can be negative.  Even when you stop that being the case
by makign time unsigned, you need to be careful with ranges here.
You may be better off separating the error handling from the values
to avoid any issues even though that makes it a little harder to use.

> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel);
> +
> +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time)
> +{
> +	const struct iio_itime_sel_mul *itime;
> +
> +	itime = iio_gts_find_itime_by_time(gts, time);
> +	if (!itime)
> +		return -EINVAL;
> +
> +	return itime->sel;

itime->sel can be negative.  I wonder if you should just make that
u16 so that you can always return it as a positive integer but
having it as unsigned in the structure.

Otherwise you need to add some docs on those limits and probably
sanity check them during the _init()


> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time);
> +
> +static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> +{
> +	const struct iio_itime_sel_mul *itime;
> +
> +	if (!iio_gts_valid_gain(gts, gain))
> +		return -EINVAL;
> +
> +	if (!gts->num_itime)
> +		return gain;
> +
> +	itime = iio_gts_find_itime_by_time(gts, time);
> +	if (!itime)
> +		return -EINVAL;
> +
> +	return gain * itime->mul;

Check for overflow perhaps?

> +}
> +
> +static int iio_gts_get_scale_linear(struct iio_gts *gts, int gain, int time,
> +				    u64 *scale)
> +{
> +	int total_gain;
> +	u64 tmp
> +
> +	total_gain = iio_gts_get_total_gain(gts, gain, time);
> +	if (total_gain < 0)
> +		return total_gain;
> +
> +	tmp = gts->max_scale;
> +
> +	do_div(tmp, total_gain);
> +
> +	*scale = tmp;
> +
> +	return 0;
> +}
> +
> +int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int,
> +		      int *scale_nano)
> +{
> +	u64 lin_scale;
> +	int ret;
> +
> +	ret = iio_gts_get_scale_linear(gts, gain, time, &lin_scale);
> +	if (ret)
> +		return ret;
> +
> +	return iio_gts_delinearize(lin_scale, NANO, scale_int, scale_nano);
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_get_scale);

It is good practice to add kernel-doc for all the exported functions,
even if some of them will be fairly obvious.  In this case the function
definition doesn't make it clear how time and gain are specified.
usecs? an index into a set of provided values?

If you can make the units explicit in the parameter that's even better.
I will note that negative times seem unlikely so maybe that should always
be unsigned?  gain probably can be negative even if that's unusual.
That may lead to problems though as lin_scale is in turn unsigned.

> +
> +/**
> + * iio_gts_find_new_gain_sel_by_old_gain_time - compensate time change

compensate for time change

> + * @gts:		Gain time scale descriptor
> + * @old_gain:		Previously set gain
> + * @old_time_sel:	Selector corresponding previously set time
> + * @new_time_sel:	Selector corresponding new time to be set
> + * @new_gain:		Pointer to value where new gain is to be written
> + *
> + * We may want to mitigate the scale change caused by setting a new integration
> + * time (for a light sensor) by also updating the (HW)gain. This helper computes
> + * new gain value to maintain the scale with new integration time.
> + *
> + * Return: 0 on success. -EINVAL if gain matching the new time is not found.
> + */
> +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;
> +
> +	itime_old = iio_gts_find_itime_by_sel(gts, old_time_sel);
> +	if (!itime_old)
> +		return -EINVAL;
> +
> +	itime_new = iio_gts_find_itime_by_sel(gts, new_time_sel);
> +	if (!itime_new)
> +		return -EINVAL;
> +
> +	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
> +				       &scale);
> +	if (ret)
> +		return ret;
> +
> +	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
> +				      new_gain);
> +	if (ret)
> +		return -EINVAL;
> +
> +	if (!iio_gts_valid_gain(gts, *new_gain))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_new_gain_sel_by_old_gain_time);

Re: [PATCH v4 5/8] iio: test: test gain-time-scale helpers

[Jonathan Cameron]

On Fri, 17 Mar 2023 16:43:49 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

A few comments inline.

As mentioned in previous patch I wonder if you should get rid
of the separation between init and build_tables code. That would require
always building tables in here even when you don't use them, but that may
be a sensible choice as drivers would never expect to not build the tables
anyway (I think).

Jonathan

> diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
> index 0b6e4e278a2f..4d5cfb9fe60b 100644
> --- a/drivers/iio/test/Kconfig
> +++ b/drivers/iio/test/Kconfig
> @@ -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
> @@ -27,3 +41,5 @@ config IIO_FORMAT_KUNIT_TEST
>  	  to the KUnit documentation in Documentation/dev-tools/kunit/.
>  
>  	  If unsure, say N.
> +
> +

Several stray blank lines that shouldn't be in here.

> diff --git a/drivers/iio/test/iio-test-gts.c b/drivers/iio/test/iio-test-gts.c
> new file mode 100644
> index 000000000000..ff9311acd0bb
> --- /dev/null
> +++ b/drivers/iio/test/iio-test-gts.c
> @@ -0,0 +1,461 @@

...


> +static int test_init_iio_gain_scale(struct iio_gts *gts, int max_scale_int,
> +				int max_scale_nano)
> +{
> +	int ret;
> +
> +	ret = iio_init_iio_gts(max_scale_int, max_scale_nano, gts_test_gains,
> +			       ARRAY_SIZE(gts_test_gains), gts_test_itimes,
> +			       ARRAY_SIZE(gts_test_itimes), gts);

	return iio_init...

Or get rid of this wrapper entirely.

> +
> +	return ret;
> +}
> +

> +
> +static void test_iio_gts_avail_test(struct kunit *test)
> +{
> +	struct iio_gts gts;
> +	int ret;
> +	int type, len;
> +	const int *vals;
> +	struct device *dev;
> +
> +	ret = test_init_iio_gain_scale(&gts, TEST_SCALE_1X, 0);

I don't follow why the wrapper is useful here.  Why not just call it
directly and have the arrays passed in nice and obvious here.

> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	if (ret)
> +		return;
> +
> +	dev = test_kunit_helper_alloc_device(test);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
> +	if (!dev)
> +		return;
> +
> +	ret = devm_iio_gts_build_avail_tables(dev, &gts);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	if (ret)
> +		goto drop_testdev;
> +
> +	/* test table building for times and iio_gts_avail_times() */
> +	ret = iio_gts_avail_times(&gts, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		goto drop_testdev;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT, type);
> +	KUNIT_EXPECT_EQ(test, 4, len);
> +	if (len < 4)
> +		goto drop_testdev;
> +
> +	test_iio_gts_chk_times(test, vals);
> +
> +	/* Test table building for all scales and iio_gts_all_avail_scales() */
> +	ret = iio_gts_all_avail_scales(&gts, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		goto drop_testdev;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> +
> +	test_iio_gts_chk_scales_all(test, &gts, vals, len);
> +
> +	/*
> +	 * Test table building for scales/time and
> +	 * iio_gts_avail_scales_for_time()
> +	 */
> +	ret = iio_gts_avail_scales_for_time(&gts, 200000, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		goto drop_testdev;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> +	test_iio_gts_chk_scales_t200(test, &gts, vals, len);
> +
> +drop_testdev:
> +	test_kunit_helper_free_device(test, dev);
> +}
> +
> +static struct kunit_case iio_gts_test_cases[] = {
> +		KUNIT_CASE(test_iio_gts_find_gain_for_scale_using_time),
> +		KUNIT_CASE(test_iio_gts_find_new_gain_sel_by_old_gain_time),
> +		KUNIT_CASE(test_iio_find_closest_gain_low),
> +		KUNIT_CASE(test_iio_gts_total_gain_to_scale),
> +		KUNIT_CASE(test_iio_gts_avail_test),
> +		{}
> +};
> +
> +static struct kunit_suite iio_gts_test_suite = {
> +	.name = "iio-gain-time-scale",
> +	.test_cases = iio_gts_test_cases,
> +};
> +
> +kunit_test_suite(iio_gts_test_suite);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
> +MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
> +MODULE_IMPORT_NS(IIO_GTS_HELPER);
> +
Looks like a stray blank line at end of file.

Re: [PATCH v4 7/8] iio: light: ROHM BU27034 Ambient Light Sensor

[Jonathan Cameron]

On Fri, 17 Mar 2023 16:44:40 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

I've run out of time / stamina for reviewing today, so just a few quick
comments to add to the various things discussed in the ongoing responses
to previous version.

Thanks,

Jonathan

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

...

> +
> +static const struct regmap_range bu27034_volatile_ranges[] = {
> +	{
> +		.range_min = BU27034_REG_MODE_CONTROL4,
> +		.range_max = BU27034_REG_MODE_CONTROL4,
> +	}, {
> +		.range_min = BU27034_REG_DATA0_LO,
> +		.range_max = BU27034_REG_DATA2_HI,
> +	},
> +};
> +
> +static const struct regmap_access_table bu27034_volatile_regs = {
> +	.yes_ranges = &bu27034_volatile_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(bu27034_volatile_ranges),
> +};
> +
> +static const struct regmap_range bu27034_read_only_ranges[] = {
> +	{
> +		.range_min = BU27034_REG_DATA0_LO,
> +		.range_max = BU27034_REG_DATA2_HI,
> +	}, {
> +		.range_min = BU27034_REG_MANUFACTURER_ID,
> +		.range_max = BU27034_REG_MANUFACTURER_ID,
> +	}
> +};
> +
> +static const struct regmap_access_table bu27034_ro_regs = {
> +	.no_ranges = &bu27034_read_only_ranges[0],
> +	.n_no_ranges = ARRAY_SIZE(bu27034_read_only_ranges),
> +};
> +
> +static const struct regmap_config bu27034_regmap = {
> +	.reg_bits	= 8,

I wouldn't do this indenting. You don't do it consistently
(see directly above) and it so often goes wrong or makes things noisy
that I prefer people just don't try to do it.

It doesn't really make much different to readability even if it looks
pretty.

> +	.val_bits	= 8,
> +
> +	.max_register	= BU27034_REG_MAX,
> +	.cache_type	= REGCACHE_RBTREE,
> +	.volatile_table = &bu27034_volatile_regs,
> +	.wr_table	= &bu27034_ro_regs,
> +};
> +
> +struct bu27034_gain_check {
> +	int old_gain;
> +	int new_gain;
> +	int chan;
> +};

> +
> +static int bu27034_set_scale(struct bu27034_data *data, int chan,
> +			    int val, int val2)
> +{
> +	int ret, time_sel, gain_sel, i;
> +	bool found = false;
> +
> +	if (chan == BU27034_CHAN_DATA2)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->mutex);
> +	ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel);
> +	if (ret)
> +		goto unlock_out;
> +
> +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
> +						val, val2 * 1000, &gain_sel);
> +	if (ret) {
> +		/*
> +		 * Could not support scale with given time. Need to change time.
> +		 * We still want to maintain the scale for all channels
> +		 */
> +		struct bu27034_gain_check gain;
> +		int new_time_sel;
> +
> +		/*
> +		 * Populate information for the other channel which should also
> +		 * maintain the scale. (Due to the HW limitations the chan2
> +		 * gets the same gain as chan0, so we only need to explicitly
> +		 * set the chan 0 and 1).
> +		 */
> +		if (chan == BU27034_CHAN_DATA0)
> +			gain.chan = BU27034_CHAN_DATA1;
> +		else if (chan == BU27034_CHAN_DATA1)
> +			gain.chan = BU27034_CHAN_DATA0;
> +
> +		ret = bu27034_get_gain(data, gain.chan, &gain.old_gain);
> +		if (ret)
> +			goto unlock_out;
> +
> +		/*
> +		 * Iterate through all the times to see if we find one which
> +		 * can support requested scale for requested channel, while
> +		 * maintaining the scale for other channels
> +		 */
> +		for (i = 0; i < data->gts.num_itime; i++) {
> +			new_time_sel = data->gts.itime_table[i].sel;
> +
> +			if (new_time_sel == time_sel)
> +				continue;
> +
> +			/* Can we provide requested scale with this time? */
> +			ret = iio_gts_find_gain_sel_for_scale_using_time(
> +				&data->gts, new_time_sel, val, val2 * 1000,
> +				&gain_sel);
> +			if (ret)
> +				continue;
> +
> +			/* Can the othe channel(s) maintain scale? */
> +			ret = iio_gts_find_new_gain_sel_by_old_gain_time(
> +				&data->gts, gain.old_gain, time_sel,
> +				new_time_sel, &gain.new_gain);
> +			if (!ret) {
> +				/* Yes - we found suitable time */
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found) {
> +			dev_dbg(data->dev,
> +				"Can't set scale maintaining other channels\n");
> +			ret = -EINVAL;
> +
> +			goto unlock_out;
> +		}
> +
> +		for (i = 0; i < 2; i++) {

Why the loop?

> +			ret = bu27034_set_gain(data, gain.chan,
> +						gain.new_gain);
> +			if (ret)
> +				goto unlock_out;
> +		}
> +
> +		ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
> +				  BU27034_MASK_MEAS_MODE, new_time_sel);
> +		if (ret)
> +			goto unlock_out;
> +	}
> +
> +	ret = bu27034_write_gain_sel(data, chan, gain_sel);
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}

...

> +static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
> +				unsigned int ch1, unsigned int gain0,
> +				unsigned int gain1)
> +{
> +	unsigned int helper, tmp;
> +	u64 helper64;
> +
> +	/*
> +	 * Here we could overflow even the 64bit value. Hence we
> +	 * multiply with gain0 only after the divisions - even though
> +	 * it may result loss of accuracy
> +	 */
> +	helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
> +	helper = coeff * ch1 * ch1;
> +	tmp = helper * gain0;
> +
> +	if (helper == helper64 && (tmp / gain0 == helper))
> +		return tmp / (gain1 * gain1) / ch0;
> +
> +	helper = gain1 * gain1;
> +	if (helper > ch0) {
> +		do_div(helper64, helper);
> +
> +		return  gain_mul_div_helper(helper64, gain0, ch0);
> +	}
> +
> +	do_div(helper64, ch0);
> +
> +	return  gain_mul_div_helper(helper64, gain0, helper);

Looks like an extra space after return

+ another one just above.

> +}
...


> +static int bu27034_get_result_unlocked(struct bu27034_data *data, __le16 *res,
> +				       int size)
> +{
> +	int ret = 0, retry_cnt = 0;
> +
> +retry:
> +	/* Get new value from sensor if data is ready */
> +	if (bu27034_has_valid_sample(data)) {
> +		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
> +				       res, size);
> +		if (ret)
> +			return ret;
> +
> +		bu27034_invalidate_read_data(data);
> +	} else {
> +		retry_cnt++;
> +
> +		/* No new data in sensor. Wait and retry */
> +		msleep(25);
> +

Might as well do this before the msleep given erroring out anyway.

> +		if (retry_cnt > BU27034_RETRY_LIMIT) {
> +			dev_err(data->dev, "No data from sensor\n");
> +
> +			return -ETIMEDOUT;
> +		}
> +
> +		goto retry;
> +	}
> +
> +	return ret;
> +}

Re: [PATCH v4 0/8] Support ROHM BU27034 ALS sensor

[Matti Vaittinen]

On 3/19/23 18:57, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 16:40:16 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Support ROHM BU27034 ALS sensor
> 
> Hi Matti,
> 
> For ease of when this is ready to apply, better to just keep
> key mailing lists and individuals cc'd on all patches.

Right. Sorry about this. I kind of rushed the sending at last friday - 
which resulted bunch of errors in the process. I forgot to do the 
spell-check, missed a header and messed the recipients... I should 
really learn to not try meeting artificial deadlines like friday EOB. 
There is Saturday and Sunday - and even if I spent weekend off the 
computer there will likely be the next Monday. (and if there is not, 
then I should probably not care about sending the patches).

> Mind you cc list is random enough I'm guessing it wasn't
> deliberate (like the maintainers patch 8 only went to lkml
> where no one will notice it)

I am using a script which generates the recipients "per patch" using the 
get_maintaner.pl underneath because in many cases certain people are 
only interested in seeing a subset of a series. This avoids polluting 
inboxes when sending large series. For v2 and v3 I did manually add the 
relevant lists / recipients to MAINTAINERS patches which only pick-up 
the LKML list.

> I can scrape these all of lore, but it's a step that not
> all reviewers are going to bother with.

I appreciate the extra mile you're ready to go here as well :) However, 
you should not need to do that. This whole series should've been CC'd to 
you and the iio-list. Sorry again.


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 v4 4/8] iio: light: Add gain-time-scale helpers

[Matti Vaittinen]

On 3/19/23 20:08, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 16:43:23 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Whilst you use it in the tests currently I'm not convinced there is a good
> reason to separate iio_init_iio_gts() from devm_iio_gts_build_avail_tables()
> as I'd expect them to be called as a pair in all drivers that use this.

I was wondering if I should only provide the:

[devm_]iio_init_iio_gts() and always unconditionally allocate and build 
the tables inside the initialization routine.

I don't really care so much about how the tests are done. In my opinion 
the testability needs should rarely be determining what the production 
code looks like. In this case it is a waste of time / resources for 
drivers which do not tell the available scales/times to user-space, or 
do need some special routine for this. This is why I did make 
build_avail_tables() optional. Still not sure what would be the best 
approach though.

> Perhaps it's worth reworking the tests to do that even if it's not strictly
> necessary for specific tests.
> 
> I think a bit more care is need with storage of time (unsigned) + decide
> whether to allow for negative gains.

My approach was just pretty simple "int is big enough for the times" 
(2000+ seconds when using usec as time units felt like more than enough 
for light sensors) and "gains are always positive".

I have not tested the negative gains at all - but I agree this should've 
been documented. Currently there is no gts-helper users who need 
negative gain (or large times for that matter) - so I was not handling them.

I'll try to check what it would mean code-wise if we converted times to 
unsigned. Negative times make no sense but allowing negative error 
values is a simple way to go.

As for the negative gains - I have no problem of someone adding a 
support for those if needed, but I don't currently see much point in 
investing time in that...

> Whilst they happen I'm not that bothered
> if that subtlety becomes a device driver problem when calling this.  I'm not
> sure I've seen a sensor that does both positive and negative gains for a single
> channel.

I agree. If driver needs negative gains, then the driver needs to deal 
with it. I have no objections if driver authors want to improve these 
helpers by adding support for negative gains, but if they don't, then 
they have the exactly same problem they would have without these helpers :)

>> ---
>> 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:
>> 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)
> 
> Ah. I've always regretted not using iio_ for the prefix on those so I'm fine
> if you would prefer to stick to iio_

I do like iio better. However, I think we should have common prefix for 
these files. Having both iio- and industrialio- will be confusing for 
newcomers. If I saw just one iio- prefixed file I would have assumed it 
is for a specific use, not for common use as the other "IIO-core" files.

One option would be converting all these industrialio-*.c files to 
iio_*.c - but I am not sure if it is worth the hassle.

>> - 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
> 
>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>> new file mode 100644
>> index 000000000000..9494ea7cdbcf
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-gts-helper.c
>> @@ -0,0 +1,990 @@
> ...
> 
>> +
>> +static const struct iio_itime_sel_mul *
>> +iio_gts_find_itime_by_time(struct iio_gts *gts, int time)
> 
> Time going to be positive (I hope!)
> So as below, I'd make all time values unsigned.

I'll see what this would result. But as I said below, I am not sure it's 
worth the added complexity.

> 
>> +{
>> +	int i;
>> +
>> +	if (!gts->num_itime)
>> +		return NULL;
>> +
>> +	for (i = 0; i < gts->num_itime; i++)
>> +		if (gts->itime_table[i].time_us == time)
>> +			return &gts->itime_table[i];
>> +
>> +	return NULL;
>> +}
> 
> ...
> 
>> +/**
>> + * iio_gts_purge_avail_scale_table - free-up the available scale tables
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
>> + * that the helpers for getting available scales like the
>> + * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
>> + * be only called after these helpers can no longer be called (Eg. after
>> + * the iio-device has been deregistered).
> 
> Whilst I'm not that keen on the comment in general, if you really really want to
> have it we need to figure out one place to put it rather than lots of duplicates.

I have seen way too many bugs with the unwinding errors. Usually with 
the IRQs but also when user-space has access to driver stuff. I placed 
this comment here hoping it would prevent at least one such bug as those 
tend to be really nasty to debug. If we avoid one, it is well worth of 
few lines of comment (IMO).


>> +	if (!gts->avail_all_scales_table)
>> +		ret = -ENOMEM;
>> +	else
>> +		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
>> +			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
>> +					&gts->avail_all_scales_table[i * 2],
>> +					&gts->avail_all_scales_table[i * 2 + 1]);
>> +
>> +	kfree(all_gains);
>> +	if (ret)
>> +		kfree(gts->avail_all_scales_table);
> 
> This is getting too clever.  I'd have an error handling block and gotos
> even though it duplicates one line.

gah. As I discussed with Andy, I do hate changing this. Well, I guess I 
have no choice.

>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * iio_gts_build_avail_scale_table - create tables of available scales
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Build the tables which can represent the available scales based on the
>> + * originally given gain and time tables. When both time and gain tables are
>> + * given this results:
>> + * 1. A set of tables representing available scales for each supported
>> + *    integration time.
>> + * 2. A single table listing all the unique scales that any combination of
>> + *    supported gains and times can provide.
>> + *
>> + * NOTE: Space allocated for the tables must be freed using
>> + * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
>> + *
>> + * Return: 0 on success.
>> + */
>> +static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
>> +{
>> +	int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
>> +
>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> 
> As per other thread, I much prefer reviewing code with sizeof(*per_time_gains)
> as it requires fewer brain cells.

Hm. I think it depends on whether one wants to understand how many bytes 
the sizeof() is actually referring. Well, again, I guess I have no 
choice here.

>> +	if (!per_time_gains)
>> +		return ret;
>> +
>> +	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>> +	if (!per_time_scales)
>> +		goto free_gains;
>> +
>> +	for (i = 0; i < gts->num_itime; i++) {
>> +		per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int),
>> +					     GFP_KERNEL);
>> +		if (!per_time_scales[i])
>> +			goto err_free_out;
>> +
>> +		per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int),
>> +					    GFP_KERNEL);
>> +		if (!per_time_gains[i])
>> +			goto err_free_scale_out;
> 
> As below. I'd put kfree(per_time_scales[i]); here to simplify the paths to
> the error handling block.
> 
>> +
>> +
>> +		for (j = 0; j < gts->num_hwgain; j++)
>> +			per_time_gains[i][j] = gts->hwgain_table[j].gain *
>> +					       gts->itime_table[i].mul;
>> +	}
>> +
>> +	ret = gain_to_scaletables(gts, per_time_gains, per_time_scales);
>> +	if (ret)
>> +		goto err_free_out;
> 
> I'm not a fan of dancing backwards and forwards with exit paths. As such I'd move
> the kfree(per_time_scales[i]) to the one condition where it matters above.

Ok.

> 
>> +
>> +	kfree(per_time_gains);
>> +	gts->per_time_avail_scale_tables = per_time_scales;
>> +
>> +	return 0;
>> +
>> +err_free_scale_out:
>> +	kfree(per_time_scales[i]);
>> +err_free_out:
>> +	for (i--; i; i--) {
>> +		kfree(per_time_scales[i]);
>> +		kfree(per_time_gains[i]);
>> +	}
>> +	kfree(per_time_scales);
>> +free_gains:
>> +	kfree(per_time_gains);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * iio_gts_build_avail_time_table - build table of available integration times
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Build the table which can represent the available times to be returned
>> + * to users using the read_avail-callback.
>> + *
>> + * NOTE: Space allocated for the tables must be freed using
>> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
>> + *
>> + * Return: 0 on success.
>> + */
>> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
>> +{
>> +	int *times, i, j, idx = 0;
>> +
>> +	if (!gts->num_itime)
>> +		return 0;
>> +
>> +	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
>> +	if (!times)
>> +		return -ENOMEM;
>> +
>> +	for (i = gts->num_itime - 1; i >= 0; i--) {
>> +		int new = gts->itime_table[i].time_us;
>> +
> 
> This looks like a sort routine.  Don't we have something generic that will work?

I think this is "combine and sort many tables into one while dropping 
duplicates". I must admit I don't know what sort routines we have 
in-kernel. If we have one which removes duplicates, then we could 
probably copy all the tables into one array and run such sort on it.

Or then we can leave this as is and add a comment about telling is going 
on here :)

> 
>> +		if (times[idx] < new) {
>> +			times[idx++] = new;
>> +			continue;
>> +		}
>> +
>> +		for (j = 0; j <= idx; j++) {
>> +			if (times[j] > new) {
>> +				memmove(&times[j + 1], &times[j],
>> +					(idx - j) * sizeof(int));
>> +				times[j] = new;
>> +				idx++;
>> +			}
>> +		}
>> +	}
>> +	gts->avail_time_tables = times;
>> +	/*
>> +	 * This is just to survive a unlikely corner-case where times in the
>> +	 * given time table were not unique. Else we could just trust the
>> +	 * gts->num_itime.
> 
> If integration times aren't unique I'd count it as driver bug and error out
> /scream.  Papering over things like this just make code harder to review
> to deal with what is probably a driver bug.

I am not entirely sure. I don't know the sensor ICs in details, but I've 
seen plenty of other ICs where we may have different register values 
that mean same physical measure. One such example is almost all ROHM 
PMICs, where we often see voltage selection registers like:

register val 0 to <foo>:
  - 1.0V + (val * 10 mV)
register val <A> to <MAX>:
  - 3.3 V

If we have similar registers for the time, then it may be good idea to 
accept selectors A...MAX to have the same time. This allows the 
gts-helpers to be used to convert the register values to times also for 
such devices. If we don't allow same times to be in the tables, then 
there may be unknown but valid register values read from the IC.

>> +	 */
>> +	gts->num_avail_time_tables = idx;
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +/**
>> + * iio_gts_purge_avail_tables - free-up the availability tables
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Free the space reserved by iio_gts_build_avail_tables(). Frees both the
>> + * integration time and scale tables.
>> + *
>> + * Note  that the helpers for getting available integration times or scales
>> + * like the iio_gts_avail_times() are not usable after this call. Thus, this
>> + * should be only called after these helpers can no longer be called (Eg.
>> + * after the iio-device has been deregistered).
> As below, I'm not sure the note adds much over normal use after free...

It is just use-after-free indeed. But people keep making these mistakes 
- and when they involve devive removal + unfortunate timing of 
user-space action to trigger they get really nasty to debug. Hence I 
hoped we could stop the code writer for a moment and ask him/her to take 
a look at his/her rewinding routine for this type of errors... Avoiding 
even one such bug is well worth couple of lines of text don't you think ;)

> Also different formatting to the one below.
> 
>> + */
>> +static void iio_gts_purge_avail_tables(struct iio_gts *gts)
>> +{
>> +	iio_gts_purge_avail_time_table(gts);
>> +	iio_gts_purge_avail_scale_table(gts);
>> +}
>> +
>> +static void devm_iio_gts_avail_all_drop(void *res)
>> +{
>> +	iio_gts_purge_avail_tables(res);
>> +}
>> +
>> +/**
>> + * devm_iio_gts_build_avail_tables - manged add availability tables
>> + * @dev:	Pointer to the device whose lifetime tables are bound
>> + * @gts:	Gain time scale descriptor
>> + *
>> + * Build the tables which can represent the available scales and available
>> + * integration times. Availability tables are built based on the originally
>> + * given gain and given time tables.
>> + *
>> + * When both time and gain tables are
>> + * given this results:
> 
> odd line break.
> 
>> + * 1. A set of sorted tables representing available scales for each supported
>> + *    integration time.
>> + * 2. A single sorted table listing all the unique scales that any combination
>> + *    of supported gains and times can provide.
>> + * 3. A sorted table of supported integration times
>> + *
>> + * After these tables are built one can use the iio_gts_all_avail_scales(),
>> + * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
>> + * implement the read_avail opeations.
>> + *
>> + * The tables are automatically released upon device detach.
>> + *
>> + * NOTE: after the tables have been purged, the helpers for getting
>> + * available scales / integration times are no longer usable. Care must be
>> + * taken that unwinding is done in correct order (iio device is deregistered
>> + * prior purging the tables).
> 
> Hmm. I think this note is calling out one potential path (even if it's the most
> common one). I'd not bother with it as a driver should use no resources after
> they've been freed and this is typically one of many.

Well, I guess we both know by now why I added the note :) Hence I won't 
repeat my reasoning. I can drop the note if you think it serves no purpose.

> 
>> + *
>> + * Return: 0 on success.
>> + */
>> +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts)
>> +{
>> +	int ret;
>> +
>> +	ret = iio_gts_build_avail_tables(gts);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_add_action_or_reset(dev, devm_iio_gts_avail_all_drop, gts);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_tables);
> 
>> +/**
>> + * iio_gts_valid_time - check if given integration time is valid
>> + * @gts:	Gain time scale descriptor
>> + * @time_us:	Integration time to check
>> + *
>> + * Return:	True if given time is supported by device. False if not.
>> + */
>> +bool iio_gts_valid_time(struct iio_gts *gts, int time_us)
>> +{
>> +	return iio_gts_find_itime_by_time(gts, time_us);
> I'd make this a little more explicit as implicit casting pointer to bool is
> rather unusual.
> 	!= NULL; maybe?

I think I may have had the !! here in the beginning. Yes, I can do != NULL.

> 
> 	
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_valid_time);
>> +
>> +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < gts->num_hwgain; i++)
>> +		if (gts->hwgain_table[i].gain == gain)
>> +			return gts->hwgain_table[i].sel;
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
>> +
>> +bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
>> +{
>> +	return iio_gts_find_sel_by_gain(gts, gain) >= 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_valid_gain);
> 
> For the _valid_xxx functions, I wonder if you shouldn't just
> push them to the header as static inline

Well, I guess I can. No problem.

> 
>> +
>> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < gts->num_hwgain; i++)
>> +		if (gts->hwgain_table[i].sel == sel)
>> +			return gts->hwgain_table[i].gain;
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
>> +
>> +int iio_gts_get_min_gain(struct iio_gts *gts)
> 
> Could just use min = INT_MAX;
> (indirectly from linux/limits.h, it's actually in vdso/limits.h
> but should not include that directly I think)
> then I don't hink you need the special casing for the
> first entry.

Hmm. I guess you think we don't need to handle case num_hwgain == 0 here 
as it should be checked in the initialization routine. Not sure what to 
think about it.

> 
>> +{
>> +	int i, min = -EINVAL;
>> +
>> +	for (i = 0; i < gts->num_hwgain; i++) {
>> +		int gain = gts->hwgain_table[i].gain;
>> +
>> +		if (min == -EINVAL)
>> +			min = gain;
>> +		else
>> +			min = min(min, gain);
>> +	}
>> +
>> +	return min;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_get_min_gain);
>> +
>> +/**
>> + * iio_find_closest_gain_low - Find the closest lower matching gain
>> + * @gts:	Gain time scale descriptor
>> + * @gain:	reference gain for which the closest match is searched
>> + * @in_range:	indicate if the reference gain was actually in the range of
>> + *		supported gains.
>> + *
>> + * Search for closest supported gain that is lower than or equal to the
>> + * gain given as a parameter. This is usable for drivers which do not require
>> + * user to request exact matching gain but rather fo rounding to a supported
>> + * gain value which is equal or lower (setting lower gain is typical for
>> + * avoiding saturation)
>> + *
>> + * Return:	The closest matching supported gain or -EINVAL is reference
> 
> Maybe just say @gain was smaller.  reference gain does not have clear meaning to me.

Ok.

> 
>> + *		gain was smaller than the smallest supported gain.
>> + */
>> +int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range)
>> +{
>> +	int i, diff = 0;
>> +	int best = -1;
>> +
>> +	*in_range = false;
>> +
>> +	for (i = 0; i < gts->num_hwgain; i++) {
>> +		/*
>> +		 * It is not expected this function is called for an exactly
>> +		 * matching gain.
> I'd not mark it unlikely even so (or comment on it).
> This is unlikely to be a particularly hot path so this unlikely() seems like a
> premature optimization which adds complexity to the code.

Hm. I don't really see the increased complexity here - but I don't also 
think this is going to be very performance critical either. So, I can 
drop this even though I personally think that small optimizations here 
and there (when not really adding extra complexity) should actually be 
encouraged.

>> +/*
>> + * iio_gts_find_gain_sel_for_scale_using_time - Fetch gain selector.
>> + * See iio_gts_find_gain_for_scale_using_time() for more information
> 
> This is exported, so I'd rather see kernel-doc style comments.

Ok.

> 
>> + */
>> +int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel,
>> +					       int scale_int, int scale_nano,
>> +					       int *gain_sel)
>> +{
>> +	int gain, ret;
>> +
>> +	ret = iio_gts_find_gain_for_scale_using_time(gts, time_sel, scale_int,
>> +						     scale_nano, &gain);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iio_gts_find_sel_by_gain(gts, gain);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*gain_sel = ret;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_sel_for_scale_using_time);
>> +
>> +int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel)
>> +{
>> +	const struct iio_itime_sel_mul *itime;
>> +
>> +	itime = iio_gts_find_itime_by_sel(gts, sel);
>> +	if (!itime)
>> +		return -EINVAL;
>> +
>> +	return itime->time_us;
> 
> Currently can be negative.  Even when you stop that being the case
> by makign time unsigned, you need to be careful with ranges here.
> You may be better off separating the error handling from the values
> to avoid any issues even though that makes it a little harder to use.

Yes. As I wrote above, I thought that the driver author needs to ensure 
the valid times would always be positive. I was guessing usec is going 
to be used as unit for most cases and 2000+ seconds is probably 
sufficient. But yes, I guess I should have documented this.

Hmm. Do you think we should support times larger than can be represented 
by signed int? I'd rather not support that if it is not needed as it 
will make this quite a bit more complex.

>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel);
>> +
>> +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time)
>> +{
>> +	const struct iio_itime_sel_mul *itime;
>> +
>> +	itime = iio_gts_find_itime_by_time(gts, time);
>> +	if (!itime)
>> +		return -EINVAL;
>> +
>> +	return itime->sel;
> 
> itime->sel can be negative.  I wonder if you should just make that
> u16 so that you can always return it as a positive integer but
> having it as unsigned in the structure.

Here I did the same assumption of sel sizes. I don't expect we to see 32 
bit selectors. To tell the truth, I just followed the linear_ranges 
logic which is heavily used in the regulator drivers.

> Otherwise you need to add some docs on those limits and probably
> sanity check them during the _init()

I am almost certain the sanity check is going to be an overkill, but it 
sure is doable. The onlu corner case I can think of is if register 
really accepts the time itself as a "selector" - but even then having 
such large values they would use the whole 32bits would be unlikely.

>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time);
>> +
>> +static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
>> +{
>> +	const struct iio_itime_sel_mul *itime;
>> +
>> +	if (!iio_gts_valid_gain(gts, gain))
>> +		return -EINVAL;
>> +
>> +	if (!gts->num_itime)
>> +		return gain;
>> +
>> +	itime = iio_gts_find_itime_by_time(gts, time);
>> +	if (!itime)
>> +		return -EINVAL;
>> +
>> +	return gain * itime->mul;
> 
> Check for overflow perhaps?

I think that if we want to add the overflow checks, we should do that 
already in init. That way we can check all the combinations before they 
are used - so that the driver authors get the errors even if they did 
not test all the times/gains their HW is supporting. I am not really 
convinced it's worth though.

>> +}
>> +
>> +static int iio_gts_get_scale_linear(struct iio_gts *gts, int gain, int time,
>> +				    u64 *scale)
>> +{
>> +	int total_gain;
>> +	u64 tmp
>> +
>> +	total_gain = iio_gts_get_total_gain(gts, gain, time);
>> +	if (total_gain < 0)
>> +		return total_gain;
>> +
>> +	tmp = gts->max_scale;
>> +
>> +	do_div(tmp, total_gain);
>> +
>> +	*scale = tmp;
>> +
>> +	return 0;
>> +}
>> +
>> +int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int,
>> +		      int *scale_nano)
>> +{
>> +	u64 lin_scale;
>> +	int ret;
>> +
>> +	ret = iio_gts_get_scale_linear(gts, gain, time, &lin_scale);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return iio_gts_delinearize(lin_scale, NANO, scale_int, scale_nano);
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_get_scale);
> 
> It is good practice to add kernel-doc for all the exported functions,
> even if some of them will be fairly obvious.

Makes sense. I'll add docs to all exported ones.

> If you can make the units explicit in the parameter that's even better.

My very initial idea was that the driver should know the units and that 
these helpers would do no unit conversions. I am unsure what road to 
take here now. I kind of like fixing the units - but on the other hand, 
allowing driver authors to decide the units makes this more flexible (as 
units can be chosen so that times won't overflow).

OTOH, now that there is the iio_gts_avail_times() - helper, it would be 
good to have the returned times in correct units. I guess we have same 
integration-time unit specified for all types of sensors? If yes, then 
it would be cleanest to require units in this format, especially if it 
is not likely to cause problems with the overflows.

> I will note that negative times seem unlikely so maybe that should always
> be unsigned?  gain probably can be negative even if that's unusual.
> That may lead to problems though as lin_scale is in turn unsigned.

Yes. I plan to support only positive gains. At least for now.

> 
>> +
>> +/**
>> + * iio_gts_find_new_gain_sel_by_old_gain_time - compensate time change
> 
> compensate for time change

thanks :)

Oh, and by the way - I appreciate the review and suggestions even when I 
do not always agree with them. So, thanks again for all the effort!

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 v4 4/8] iio: light: Add gain-time-scale helpers

[Matti Vaittinen]

On 3/20/23 14:01, Matti Vaittinen wrote:
> On 3/19/23 20:08, Jonathan Cameron wrote:
>> On Fri, 17 Mar 2023 16:43:23 +0200
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> I think a bit more care is need with storage of time (unsigned) + decide
>> whether to allow for negative gains.
> 
> My approach was just pretty simple "int is big enough for the times" 
> (2000+ seconds when using usec as time units felt like more than enough 
> for light sensors) and "gains are always positive".
> 
> I have not tested the negative gains at all - but I agree this should've 
> been documented. Currently there is no gts-helper users who need 
> negative gain (or large times for that matter) - so I was not handling 
> them.
> 
> I'll try to check what it would mean code-wise if we converted times to 
> unsigned. Negative times make no sense but allowing negative error 
> values is a simple way to go.
> 
> As for the negative gains - I have no problem of someone adding a 
> support for those if needed, but I don't currently see much point in 
> investing time in that...
> 
>> Whilst they happen I'm not that bothered
>> if that subtlety becomes a device driver problem when calling this.  
>> I'm not
>> sure I've seen a sensor that does both positive and negative gains for 
>> a single
>> channel.
> 
> I agree. If driver needs negative gains, then the driver needs to deal 
> with it. I have no objections if driver authors want to improve these 
> helpers by adding support for negative gains, but if they don't, then 
> they have the exactly same problem they would have without these helpers :)

Back at this. I started reworking things to use unsigned times / gains 
but I am not really happy about how it starts to look like. Using the 
int values but reserving negative values to denote errors keeps things 
cleaner. Also, I don't think we need the extra bit for extending the 
range of supported values - It's hard for me to think we would really 
need gains or times exceeding the maximum signed int. I think negative 
gains are actually more likely so keeping int as type may help one who 
wants to add support for negative gains.

(Although, I assume the integration time multiplying logic with negative 
gains would not work in a same way as with positive gains - so 
supporting negative gains would probably require more than that, or work 
only as a dummy selector <=> gain converter without the time tables).

So, the v5 will likely still use int as type for times and gain but also 
have a check in initialization enforcing this. I will also document this 
restriction in the gain/time struct and init function documentation.

I don't think the v5 is final version, especially because it will be the 
first version looping in the Kunit people. So we can keep iterating this 
for v6 if you still feel using ints is unacceptable :)

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 v4 4/8] iio: light: Add gain-time-scale helpers

[Jonathan Cameron]

On Mon, 20 Mar 2023 14:01:55 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/19/23 20:08, Jonathan Cameron wrote:
> > On Fri, 17 Mar 2023 16:43:23 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> 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.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> > 
> > Whilst you use it in the tests currently I'm not convinced there is a good
> > reason to separate iio_init_iio_gts() from devm_iio_gts_build_avail_tables()
> > as I'd expect them to be called as a pair in all drivers that use this.  
> 
> I was wondering if I should only provide the:
> 
> [devm_]iio_init_iio_gts() and always unconditionally allocate and build 
> the tables inside the initialization routine.
> 
> I don't really care so much about how the tests are done. In my opinion 
> the testability needs should rarely be determining what the production 
> code looks like. In this case it is a waste of time / resources for 
> drivers which do not tell the available scales/times to user-space, or 
> do need some special routine for this. This is why I did make 
> build_avail_tables() optional. Still not sure what would be the best 
> approach though.

Given it should be 'easy to do' after you have this infrastructure, why would
a driver not provide the _available tables?

> 
> > Perhaps it's worth reworking the tests to do that even if it's not strictly
> > necessary for specific tests.
> > 
> > I think a bit more care is need with storage of time (unsigned) + decide
> > whether to allow for negative gains.  
> 
> My approach was just pretty simple "int is big enough for the times" 
> (2000+ seconds when using usec as time units felt like more than enough 
> for light sensors) and "gains are always positive".
> 
> I have not tested the negative gains at all - but I agree this should've 
> been documented. Currently there is no gts-helper users who need 
> negative gain (or large times for that matter) - so I was not handling them.
> 
> I'll try to check what it would mean code-wise if we converted times to 
> unsigned. Negative times make no sense but allowing negative error 
> values is a simple way to go.
> 
> As for the negative gains - I have no problem of someone adding a 
> support for those if needed, but I don't currently see much point in 
> investing time in that...

I'm fine with keeping them all signed, but you probably need some checks
to ensure the data provided isn't negative.

> 
> > Whilst they happen I'm not that bothered
> > if that subtlety becomes a device driver problem when calling this.  I'm not
> > sure I've seen a sensor that does both positive and negative gains for a single
> > channel.  
> 
> I agree. If driver needs negative gains, then the driver needs to deal 
> with it. I have no objections if driver authors want to improve these 
> helpers by adding support for negative gains, but if they don't, then 
> they have the exactly same problem they would have without these helpers :)
> 
> >> ---
> >> 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:
> >> 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)  
> > 
> > Ah. I've always regretted not using iio_ for the prefix on those so I'm fine
> > if you would prefer to stick to iio_  
> 
> I do like iio better. However, I think we should have common prefix for 
> these files. Having both iio- and industrialio- will be confusing for 
> newcomers. If I saw just one iio- prefixed file I would have assumed it 
> is for a specific use, not for common use as the other "IIO-core" files.
> 
> One option would be converting all these industrialio-*.c files to 
> iio_*.c - but I am not sure if it is worth the hassle.

It gets messy because then you have to fix up the module names. People get
annoyed if those change.


...

> >   
> >> +/**
> >> + * iio_gts_purge_avail_scale_table - free-up the available scale tables
> >> + * @gts:	Gain time scale descriptor
> >> + *
> >> + * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
> >> + * that the helpers for getting available scales like the
> >> + * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
> >> + * be only called after these helpers can no longer be called (Eg. after
> >> + * the iio-device has been deregistered).  
> > 
> > Whilst I'm not that keen on the comment in general, if you really really want to
> > have it we need to figure out one place to put it rather than lots of duplicates.  
> 
> I have seen way too many bugs with the unwinding errors. Usually with 
> the IRQs but also when user-space has access to driver stuff. I placed 
> this comment here hoping it would prevent at least one such bug as those 
> tend to be really nasty to debug. If we avoid one, it is well worth of 
> few lines of comment (IMO).

I'd argue this particular code doesn't have the subtleties that irqs and stuff
directly accessed from userspace has (where such comments would sometimes be
helpful!).  Meh I don't care that much.


> >> +
> >> +/**
> >> + * iio_gts_build_avail_scale_table - create tables of available scales
> >> + * @gts:	Gain time scale descriptor
> >> + *
> >> + * Build the tables which can represent the available scales based on the
> >> + * originally given gain and time tables. When both time and gain tables are
> >> + * given this results:
> >> + * 1. A set of tables representing available scales for each supported
> >> + *    integration time.
> >> + * 2. A single table listing all the unique scales that any combination of
> >> + *    supported gains and times can provide.
> >> + *
> >> + * NOTE: Space allocated for the tables must be freed using
> >> + * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
> >> + *
> >> + * Return: 0 on success.
> >> + */
> >> +static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
> >> +{
> >> +	int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
> >> +
> >> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);  
> > 
> > As per other thread, I much prefer reviewing code with sizeof(*per_time_gains)
> > as it requires fewer brain cells.  
> 
> Hm. I think it depends on whether one wants to understand how many bytes 
> the sizeof() is actually referring. Well, again, I guess I have no 
> choice here.

Why would one care?  You care about the number of objects, for a kcalloc call
but the size is rarely useful to have immediately visible.


> >> +
> >> +/**
> >> + * iio_gts_build_avail_time_table - build table of available integration times
> >> + * @gts:	Gain time scale descriptor
> >> + *
> >> + * Build the table which can represent the available times to be returned
> >> + * to users using the read_avail-callback.
> >> + *
> >> + * NOTE: Space allocated for the tables must be freed using
> >> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
> >> + *
> >> + * Return: 0 on success.
> >> + */
> >> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
> >> +{
> >> +	int *times, i, j, idx = 0;
> >> +
> >> +	if (!gts->num_itime)
> >> +		return 0;
> >> +
> >> +	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
> >> +	if (!times)
> >> +		return -ENOMEM;
> >> +
> >> +	for (i = gts->num_itime - 1; i >= 0; i--) {
> >> +		int new = gts->itime_table[i].time_us;
> >> +  
> > 
> > This looks like a sort routine.  Don't we have something generic that will work?  
> 
> I think this is "combine and sort many tables into one while dropping 
> duplicates". I must admit I don't know what sort routines we have 
> in-kernel. If we have one which removes duplicates, then we could 
> probably copy all the tables into one array and run such sort on it.
> 
> Or then we can leave this as is and add a comment about telling is going 
> on here :)

Perfect.

> 
> >   
> >> +		if (times[idx] < new) {
> >> +			times[idx++] = new;
> >> +			continue;
> >> +		}
> >> +
> >> +		for (j = 0; j <= idx; j++) {
> >> +			if (times[j] > new) {
> >> +				memmove(&times[j + 1], &times[j],
> >> +					(idx - j) * sizeof(int));
> >> +				times[j] = new;
> >> +				idx++;
> >> +			}
> >> +		}
> >> +	}
> >> +	gts->avail_time_tables = times;
> >> +	/*
> >> +	 * This is just to survive a unlikely corner-case where times in the
> >> +	 * given time table were not unique. Else we could just trust the
> >> +	 * gts->num_itime.  
> > 
> > If integration times aren't unique I'd count it as driver bug and error out
> > /scream.  Papering over things like this just make code harder to review
> > to deal with what is probably a driver bug.  
> 
> I am not entirely sure. I don't know the sensor ICs in details, but I've 
> seen plenty of other ICs where we may have different register values 
> that mean same physical measure. One such example is almost all ROHM 
> PMICs, where we often see voltage selection registers like:

> 
> register val 0 to <foo>:
>   - 1.0V + (val * 10 mV)
> register val <A> to <MAX>:
>   - 3.3 V
> 
> If we have similar registers for the time, then it may be good idea to 
> accept selectors A...MAX to have the same time. This allows the 
> gts-helpers to be used to convert the register values to times also for 
> such devices. If we don't allow same times to be in the tables, then 
> there may be unknown but valid register values read from the IC.

That I agree with.  Should the driver support more that one such option.
No it shouldn't.   Snarky comments about repeated values are fine ;)
I'd argue the driver has a bug if it hasn't hammered the device into a
known state (typically via a documented reset value).   We always have
fun corners where devices will accept reserved values and driver very
rarely handle reading them back right - because we arrange that they are
definitely not set by resetting the device.



> >> +
> >> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < gts->num_hwgain; i++)
> >> +		if (gts->hwgain_table[i].sel == sel)
> >> +			return gts->hwgain_table[i].gain;
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
> >> +
> >> +int iio_gts_get_min_gain(struct iio_gts *gts)  
> > 
> > Could just use min = INT_MAX;
> > (indirectly from linux/limits.h, it's actually in vdso/limits.h
> > but should not include that directly I think)
> > then I don't hink you need the special casing for the
> > first entry.  
> 
> Hmm. I guess you think we don't need to handle case num_hwgain == 0 here 
> as it should be checked in the initialization routine. Not sure what to 
> think about it.

Yup. Using this code without having multiple hardware gains would be odd.
Even if you did I still feel that num_hwgain == 1 would be correct setting.


....

> > 
> > Currently can be negative.  Even when you stop that being the case
> > by makign time unsigned, you need to be careful with ranges here.
> > You may be better off separating the error handling from the values
> > to avoid any issues even though that makes it a little harder to use.  
> 
> Yes. As I wrote above, I thought that the driver author needs to ensure 
> the valid times would always be positive. I was guessing usec is going 
> to be used as unit for most cases and 2000+ seconds is probably 
> sufficient. But yes, I guess I should have documented this.

Why not just check this at init?  Otherwise this will be a tricky bug
to track down. Documentation is good, but code that tells you no is even
better.

> 
> Hmm. Do you think we should support times larger than can be represented 
> by signed int? I'd rather not support that if it is not needed as it 
> will make this quite a bit more complex.

Probably not - if needed by someone else they can add it.

> 
> >> +}
> >> +EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel);
> >> +
> >> +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time)
> >> +{
> >> +	const struct iio_itime_sel_mul *itime;
> >> +
> >> +	itime = iio_gts_find_itime_by_time(gts, time);
> >> +	if (!itime)
> >> +		return -EINVAL;
> >> +
> >> +	return itime->sel;  
> > 
> > itime->sel can be negative.  I wonder if you should just make that
> > u16 so that you can always return it as a positive integer but
> > having it as unsigned in the structure.  
> 
> Here I did the same assumption of sel sizes. I don't expect we to see 32 
> bit selectors. To tell the truth, I just followed the linear_ranges 
> logic which is heavily used in the regulator drivers.
> 
> > Otherwise you need to add some docs on those limits and probably
> > sanity check them during the _init()  
> 
> I am almost certain the sanity check is going to be an overkill, but it 
> sure is doable. The onlu corner case I can think of is if register 
> really accepts the time itself as a "selector" - but even then having 
> such large values they would use the whole 32bits would be unlikely.

I'd be fine with clear documentation of the limits, but it it's trivial
to check they are being obeyed, that makes for an easier to use bit
of code.

> 
> >> +}
> >> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time);
> >> +
> >> +static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> >> +{
> >> +	const struct iio_itime_sel_mul *itime;
> >> +
> >> +	if (!iio_gts_valid_gain(gts, gain))
> >> +		return -EINVAL;
> >> +
> >> +	if (!gts->num_itime)
> >> +		return gain;
> >> +
> >> +	itime = iio_gts_find_itime_by_time(gts, time);
> >> +	if (!itime)
> >> +		return -EINVAL;
> >> +
> >> +	return gain * itime->mul;  
> > 
> > Check for overflow perhaps?  
> 
> I think that if we want to add the overflow checks, we should do that 
> already in init. That way we can check all the combinations before they 
> are used - so that the driver authors get the errors even if they did 
> not test all the times/gains their HW is supporting. I am not really 
> convinced it's worth though.

Gains can get very very large, so in this one case I'd check it. Fine
to do it at init though.  Note the large gains sometimes come about because
SI units sometimes mean the obvious base unit is very small or very big.



> 
> > If you can make the units explicit in the parameter that's even better.  
> 
> My very initial idea was that the driver should know the units and that 
> these helpers would do no unit conversions. I am unsure what road to 
> take here now. I kind of like fixing the units - but on the other hand, 
> allowing driver authors to decide the units makes this more flexible (as 
> units can be chosen so that times won't overflow).
> 
> OTOH, now that there is the iio_gts_avail_times() - helper, it would be 
> good to have the returned times in correct units. I guess we have same 
> integration-time unit specified for all types of sensors? If yes, then 
> it would be cleanest to require units in this format, especially if it 
> is not likely to cause problems with the overflows.
> 
> > I will note that negative times seem unlikely so maybe that should always
> > be unsigned?  gain probably can be negative even if that's unusual.
> > That may lead to problems though as lin_scale is in turn unsigned.  
> 
> Yes. I plan to support only positive gains. At least for now.
> 
> >   
> >> +
> >> +/**
> >> + * iio_gts_find_new_gain_sel_by_old_gain_time - compensate time change  
> > 
> > compensate for time change  
> 
> thanks :)
> 
> Oh, and by the way - I appreciate the review and suggestions even when I 
> do not always agree with them. So, thanks again for all the effort!

No problem

J
> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v4 4/8] iio: light: Add gain-time-scale helpers

[Vaittinen, Matti]

On 3/25/23 20:29, Jonathan Cameron wrote:
> On Mon, 20 Mar 2023 14:01:55 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 3/19/23 20:08, Jonathan Cameron wrote:
>>> On Fri, 17 Mar 2023 16:43:23 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> 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.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>
>>> Whilst you use it in the tests currently I'm not convinced there is a good
>>> reason to separate iio_init_iio_gts() from devm_iio_gts_build_avail_tables()
>>> as I'd expect them to be called as a pair in all drivers that use this.
>>
>> I was wondering if I should only provide the:
>>
>> [devm_]iio_init_iio_gts() and always unconditionally allocate and build
>> the tables inside the initialization routine.
>>
>> I don't really care so much about how the tests are done. In my opinion
>> the testability needs should rarely be determining what the production
>> code looks like. In this case it is a waste of time / resources for
>> drivers which do not tell the available scales/times to user-space, or
>> do need some special routine for this. This is why I did make
>> build_avail_tables() optional. Still not sure what would be the best
>> approach though.
> 
> Given it should be 'easy to do' after you have this infrastructure, why would
> a driver not provide the _available tables?

Pretty much the only thing I can think of is when a HW has some "funny" 
limitations regarding available values. Eg, when the device supports 
only certain values depending on a seemingly unrelated config X.

In such case the driver might not want to always advertise all the 
available values - and in such case the driver could not use the tables.

And as this is probably really unlikely scenario - In v5 I did just put 
the table building unconditionally in gts-init as you suggested.

>>> Perhaps it's worth reworking the tests to do that even if it's not strictly
>>> necessary for specific tests.
>>>
>>> I think a bit more care is need with storage of time (unsigned) + decide
>>> whether to allow for negative gains.
>>
>> My approach was just pretty simple "int is big enough for the times"
>> (2000+ seconds when using usec as time units felt like more than enough
>> for light sensors) and "gains are always positive".
>>
>> I have not tested the negative gains at all - but I agree this should've
>> been documented. Currently there is no gts-helper users who need
>> negative gain (or large times for that matter) - so I was not handling them.
>>
>> I'll try to check what it would mean code-wise if we converted times to
>> unsigned. Negative times make no sense but allowing negative error
>> values is a simple way to go.
>>
>> As for the negative gains - I have no problem of someone adding a
>> support for those if needed, but I don't currently see much point in
>> investing time in that...
> 
> I'm fine with keeping them all signed, but you probably need some checks
> to ensure the data provided isn't negative.

Yep. I agree. Added the checks to init in v5 :)

>>> Whilst they happen I'm not that bothered
>>> if that subtlety becomes a device driver problem when calling this.  I'm not
>>> sure I've seen a sensor that does both positive and negative gains for a single
>>> channel.
>>
>> I agree. If driver needs negative gains, then the driver needs to deal
>> with it. I have no objections if driver authors want to improve these
>> helpers by adding support for negative gains, but if they don't, then
>> they have the exactly same problem they would have without these helpers :)
>>
>>>> ---
>>>> 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:
>>>> 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)
>>>
>>> Ah. I've always regretted not using iio_ for the prefix on those so I'm fine
>>> if you would prefer to stick to iio_
>>
>> I do like iio better. However, I think we should have common prefix for
>> these files. Having both iio- and industrialio- will be confusing for
>> newcomers. If I saw just one iio- prefixed file I would have assumed it
>> is for a specific use, not for common use as the other "IIO-core" files.
>>
>> One option would be converting all these industrialio-*.c files to
>> iio_*.c - but I am not sure if it is worth the hassle.
> 
> It gets messy because then you have to fix up the module names. People get
> annoyed if those change.

Ah. Very valid point. I should've seen that.

So, I'll keep the long filename just for the sake of the consistency - 
unless you object.

> ...
> 
>>>    
>>>> +/**
>>>> + * iio_gts_purge_avail_scale_table - free-up the available scale tables
>>>> + * @gts:	Gain time scale descriptor
>>>> + *
>>>> + * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
>>>> + * that the helpers for getting available scales like the
>>>> + * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
>>>> + * be only called after these helpers can no longer be called (Eg. after
>>>> + * the iio-device has been deregistered).
>>>
>>> Whilst I'm not that keen on the comment in general, if you really really want to
>>> have it we need to figure out one place to put it rather than lots of duplicates.
>>
>> I have seen way too many bugs with the unwinding errors. Usually with
>> the IRQs but also when user-space has access to driver stuff. I placed
>> this comment here hoping it would prevent at least one such bug as those
>> tend to be really nasty to debug. If we avoid one, it is well worth of
>> few lines of comment (IMO).
> 
> I'd argue this particular code doesn't have the subtleties that irqs and stuff
> directly accessed from userspace has (where such comments would sometimes be
> helpful!).  Meh I don't care that much.

Hm. I didn't check how IIO handles the user-space request to available 
scales/times - but I thought that would go directly to the tables for as 
long as the IIO-device stays registered.

In any case, moving the table creation in gts-init made these functions 
internal - so I think this piece of doc should go now.

> 
> 
>>>> +
>>>> +/**
>>>> + * iio_gts_build_avail_scale_table - create tables of available scales
>>>> + * @gts:	Gain time scale descriptor
>>>> + *
>>>> + * Build the tables which can represent the available scales based on the
>>>> + * originally given gain and time tables. When both time and gain tables are
>>>> + * given this results:
>>>> + * 1. A set of tables representing available scales for each supported
>>>> + *    integration time.
>>>> + * 2. A single table listing all the unique scales that any combination of
>>>> + *    supported gains and times can provide.
>>>> + *
>>>> + * NOTE: Space allocated for the tables must be freed using
>>>> + * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
>>>> + *
>>>> + * Return: 0 on success.
>>>> + */
>>>> +static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
>>>> +{
>>>> +	int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
>>>> +
>>>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>>>
>>> As per other thread, I much prefer reviewing code with sizeof(*per_time_gains)
>>> as it requires fewer brain cells.
>>
>> Hm. I think it depends on whether one wants to understand how many bytes
>> the sizeof() is actually referring. Well, again, I guess I have no
>> choice here.
> 
> Why would one care?  You care about the number of objects, for a kcalloc call
> but the size is rarely useful to have immediately visible.

I think this is just how my mind works. I do like to see not only how 
many "items" there is, but also the size of the "items". I guess it 
rarely plays a big role, but sometimes it's nice knowing for example how 
much memory an array takes (especially in cases where the array is from 
the stack, but sometimes also for heap allocations). Furthermore, 
occasionally seeing the alignment is nice too.

Yes, yes - this is not relevant here - but this is probably why I prefer 
seeing the sizeof(type *) instead of the sizeof(variable) - when the 
variable is a pointer.

I think I changed this to sizeof(*per_time_gains) for v5.

>>>> +
>>>> +/**
>>>> + * iio_gts_build_avail_time_table - build table of available integration times
>>>> + * @gts:	Gain time scale descriptor
>>>> + *
>>>> + * Build the table which can represent the available times to be returned
>>>> + * to users using the read_avail-callback.
>>>> + *
>>>> + * NOTE: Space allocated for the tables must be freed using
>>>> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
>>>> + *
>>>> + * Return: 0 on success.
>>>> + */
>>>> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
>>>> +{
>>>> +	int *times, i, j, idx = 0;
>>>> +
>>>> +	if (!gts->num_itime)
>>>> +		return 0;
>>>> +
>>>> +	times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
>>>> +	if (!times)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for (i = gts->num_itime - 1; i >= 0; i--) {
>>>> +		int new = gts->itime_table[i].time_us;
>>>> +
>>>
>>> This looks like a sort routine.  Don't we have something generic that will work?
>>
>> I think this is "combine and sort many tables into one while dropping
>> duplicates". I must admit I don't know what sort routines we have
>> in-kernel. If we have one which removes duplicates, then we could
>> probably copy all the tables into one array and run such sort on it.
>>
>> Or then we can leave this as is and add a comment about telling is going
>> on here :)
> 
> Perfect.

Now I wonder if I remembered to actually add the comment...

> 
>>
>>>    
>>>> +		if (times[idx] < new) {
>>>> +			times[idx++] = new;
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		for (j = 0; j <= idx; j++) {
>>>> +			if (times[j] > new) {
>>>> +				memmove(&times[j + 1], &times[j],
>>>> +					(idx - j) * sizeof(int));
>>>> +				times[j] = new;
>>>> +				idx++;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +	gts->avail_time_tables = times;
>>>> +	/*
>>>> +	 * This is just to survive a unlikely corner-case where times in the
>>>> +	 * given time table were not unique. Else we could just trust the
>>>> +	 * gts->num_itime.
>>>
>>> If integration times aren't unique I'd count it as driver bug and error out
>>> /scream.  Papering over things like this just make code harder to review
>>> to deal with what is probably a driver bug.
>>
>> I am not entirely sure. I don't know the sensor ICs in details, but I've
>> seen plenty of other ICs where we may have different register values
>> that mean same physical measure. One such example is almost all ROHM
>> PMICs, where we often see voltage selection registers like:
> 
>>
>> register val 0 to <foo>:
>>    - 1.0V + (val * 10 mV)
>> register val <A> to <MAX>:
>>    - 3.3 V
>>
>> If we have similar registers for the time, then it may be good idea to
>> accept selectors A...MAX to have the same time. This allows the
>> gts-helpers to be used to convert the register values to times also for
>> such devices. If we don't allow same times to be in the tables, then
>> there may be unknown but valid register values read from the IC.
> 
> That I agree with.  Should the driver support more that one such option.
> No it shouldn't.   Snarky comments about repeated values are fine ;)
> I'd argue the driver has a bug if it hasn't hammered the device into a
> known state (typically via a documented reset value).   We always have
> fun corners where devices will accept reserved values and driver very
> rarely handle reading them back right - because we arrange that they are
> definitely not set by resetting the device.

Hm. I guess the fundamental difference compared to PMICs here is the 
ability to reset the device at probe. PMICs can't really be reset as 
boot may have already changed the values.

I am still not sure supporting all register values hurts. Again, being 
worked a while with the ROHM PMICs I can't help of thinking situation 
where 'default values' for a device are load from OTP at device start. 
(This is usually how 'power-on' voltages are handled in PMICs). I think 
it is quite standard thing to change the default voltages for variants 
targeted to different systems. And when this is done, no one guarantees 
the same register value is used for default that is set in driver to 
represent certain voltage. Thus it is (in my opinion) better to support 
all known register values. Here a big thing is that regulators do often 
use 'linear-ranges' - which means supporting all the register values 
does not really consume much of memory - unlike with the table based 
implementation we have with the times here.

Furthermore, there are user-space tools to write register values past 
the driver. And yes, If this is done then th euser should be prepared 
for the driver to fail. Still, it does not mean we shouldn't handle this 
gracefully (best we can) in driver(s) - when it does not cost much.

>>>> +
>>>> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < gts->num_hwgain; i++)
>>>> +		if (gts->hwgain_table[i].sel == sel)
>>>> +			return gts->hwgain_table[i].gain;
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
>>>> +
>>>> +int iio_gts_get_min_gain(struct iio_gts *gts)
>>>
>>> Could just use min = INT_MAX;
>>> (indirectly from linux/limits.h, it's actually in vdso/limits.h
>>> but should not include that directly I think)
>>> then I don't hink you need the special casing for the
>>> first entry.
>>
>> Hmm. I guess you think we don't need to handle case num_hwgain == 0 here
>> as it should be checked in the initialization routine. Not sure what to
>> think about it.
> 
> Yup. Using this code without having multiple hardware gains would be odd.
> Even if you did I still feel that num_hwgain == 1 would be correct setting.

The only case I can think of would be not providing hardware-gains at 
all and only using the times to support the 'reg <=> time' conversions. 
But yes, in that case ending up in this routine would probably be odd - 
and I think using these helpers just for the 'reg <=> time' conversion 
might be an overkill.

> ....
> 
>>>
>>> Currently can be negative.  Even when you stop that being the case
>>> by makign time unsigned, you need to be careful with ranges here.
>>> You may be better off separating the error handling from the values
>>> to avoid any issues even though that makes it a little harder to use.
>>
>> Yes. As I wrote above, I thought that the driver author needs to ensure
>> the valid times would always be positive. I was guessing usec is going
>> to be used as unit for most cases and 2000+ seconds is probably
>> sufficient. But yes, I guess I should have documented this.
> 
> Why not just check this at init?  Otherwise this will be a tricky bug
> to track down. Documentation is good, but code that tells you no is even
> better.
> 

I agree. And I added these checks in init at v5 :)

>>>> +}
>>>> +EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel);
>>>> +
>>>> +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time)
>>>> +{
>>>> +	const struct iio_itime_sel_mul *itime;
>>>> +
>>>> +	itime = iio_gts_find_itime_by_time(gts, time);
>>>> +	if (!itime)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return itime->sel;
>>>
>>> itime->sel can be negative.  I wonder if you should just make that
>>> u16 so that you can always return it as a positive integer but
>>> having it as unsigned in the structure.
>>
>> Here I did the same assumption of sel sizes. I don't expect we to see 32
>> bit selectors. To tell the truth, I just followed the linear_ranges
>> logic which is heavily used in the regulator drivers.
>>
>>> Otherwise you need to add some docs on those limits and probably
>>> sanity check them during the _init()
>>
>> I am almost certain the sanity check is going to be an overkill, but it
>> sure is doable. The onlu corner case I can think of is if register
>> really accepts the time itself as a "selector" - but even then having
>> such large values they would use the whole 32bits would be unlikely.
> 
> I'd be fine with clear documentation of the limits, but it it's trivial
> to check they are being obeyed, that makes for an easier to use bit
> of code.

True.

> 
>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time);
>>>> +
>>>> +static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
>>>> +{
>>>> +	const struct iio_itime_sel_mul *itime;
>>>> +
>>>> +	if (!iio_gts_valid_gain(gts, gain))
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (!gts->num_itime)
>>>> +		return gain;
>>>> +
>>>> +	itime = iio_gts_find_itime_by_time(gts, time);
>>>> +	if (!itime)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return gain * itime->mul;
>>>
>>> Check for overflow perhaps?
>>
>> I think that if we want to add the overflow checks, we should do that
>> already in init. That way we can check all the combinations before they
>> are used - so that the driver authors get the errors even if they did
>> not test all the times/gains their HW is supporting. I am not really
>> convinced it's worth though.
> 
> Gains can get very very large, so in this one case I'd check it. Fine
> to do it at init though.  Note the large gains sometimes come about because
> SI units sometimes mean the obvious base unit is very small or very big.

I think I added the check to init in v5 :)

Thanks again!

Yours,
	--Matti


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

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