Support ROHM BU27034 ALS sensor

The creation of a dummy device in order to test managed interfaces is a generally useful test feature. The drm test helpers test_kunit_helper_alloc_device() and test_kunit_helper_free_device() are doing exactly this. It makes no sense that each and every component which intends to be testing managed interfaces will create similar helpers.

Move these functions to place where it is more obvious they can be used also by other subsystems but drm.

Signed-off-by Matti Vaittinen <[email protected]>

---

Please note that there's something similat ongoing in the CCF:
https://lore.kernel.org/all/[email protected]/

I do like the simplicity of these DRM-originated helpers so I think they're worth. I do equally like the Stephen's idea of having the "dummy platform device" related helpers under drivers/base and the header being in include/kunit/platform_device.h which is similar to real platform device under include/linux/platform_device.h - so, in the end of the day I hope Stephen's changes as well as the changes this patch introduces to end up in those files. This, however, will require some co-operation to avoid conflicts.

Hi Matti,

Quoted 17 lines · Matti Vaittinen

The creation of a dummy device in order to test managed interfaces is a generally useful test feature. The drm test helpers test_kunit_helper_alloc_device() and test_kunit_helper_free_device() are doing exactly this. It makes no sense that each and every component which intends to be testing managed interfaces will create similar helpers.

Move these functions to place where it is more obvious they can be used also by other subsystems but drm.

Signed-off-by Matti Vaittinen <[email protected]>

---

Please note that there's something similat ongoing in the CCF:
https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

I do like the simplicity of these DRM-originated helpers so I think they're worth. I do equally like the Stephen's idea of having the "dummy platform device" related helpers under drivers/base and the header being in include/kunit/platform_device.h which is similar to real platform device under include/linux/platform_device.h - so, in the end of the day I hope Stephen's changes as well as the changes this patch introduces to end up in those files. This, however, will require some co-operation to avoid conflicts.

I think you would have an easier time if you just copied and renamed them into the kunit folder as an preparation series.

That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just create new helpers that can be reused/converted to by everyone eventually

Quoted 3 lines

I think you would have an easier time if you just copied and renamed them into the kunit folder as an preparation series.

Yes. That would simplify the syncing between the trees. It slightly bugs me to add dublicate code in kernel-but the clean-up series for DRM users could be prepared at the same time. It would be even possible to just change the drm-helper to be a wrapper for the generic one - and leave the callers intact - although it leaves some seemingly unnecessary "onion code" there.

Quoted 2 lines

That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just create new helpers that can be reused/converted to by everyone eventually

Yes. Thanks - I think I may go with this approach for the v5 :)

Quoting Matti Vaittinen (2023-03-18 23:36:20)

Quoted 11 lines

Quoted 3 lines

I think you would have an easier time if you just copied and renamed them into the kunit folder as an preparation series.

Yes. That would simplify the syncing between the trees. It slightly bugs me to add dublicate code in kernel-but the clean-up series for DRM users could be prepared at the same time. It would be even possible to just change the drm-helper to be a wrapper for the generic one - and leave the callers intact - although it leaves some seemingly unnecessary "onion code" there.

Quoted 4 lines

Quoted 2 lines

That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just create new helpers that can be reused/converted to by everyone eventually

Yes. Thanks - I think I may go with this approach for the v5 :)

Which kunit directory?

I imagine if there are conflicts they will be
trivial so it probably doesn't matter.

Have you Cced kunit folks and the
list on the kunit patches? They may have some opinion.

Morning Stephen,

Quoted 15 lines · Stephen Boyd

Quoting Matti Vaittinen (2023-03-18 23:36:20)

Quoted 14 lines

Quoted 3 lines

I think you would have an easier time if you just copied and renamed them into the kunit folder as an preparation series.

Yes. That would simplify the syncing between the trees. It slightly bugs me to add dublicate code in kernel-but the clean-up series for DRM users could be prepared at the same time. It would be even possible to just change the drm-helper to be a wrapper for the generic one - and leave the callers intact - although it leaves some seemingly unnecessary "onion code" there.

Quoted 2 lines

That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just create new helpers that can be reused/converted to by everyone eventually

Quoted 3 lines

Quoted 1 line

Yes. Thanks - I think I may go with this approach for the v5 :)

Which kunit directory?

I was thinking of adding the platform_device.h (I liked your suggestion) in the include/kunit/

Quoted 3 lines

I imagine if there are conflicts they will be
trivial so it probably doesn't matter.

Probably so. Still, I am not the one who needs to deal with the conflicts. Hence I like at least asking if people see good way to avoid them in the first place.

Besides, I was not sure if you were planning to add similar helper or just wrappers to individual functions. Wanted to ping you just in case this has some impact to what you do.

Quoted 3 lines

Have you Cced kunit folks and the
list on the kunit patches? They may have some opinion.

This patch was should have contained the include/kunit/platform_device.h. That file was pulling the Kunit people in recipients but I messed up things with last minute changes so both the header and people were dropped. I'll fix this for v5.

Quoting Matti Vaittinen (2023-03-20 22:45:52)

Quoted 3 lines

Morning Stephen,

On 3/20/23 21:23, Stephen Boyd wrote:

Quoted 4 lines

Quoted 1 line

Quoting Matti Vaittinen (2023-03-18 23:36:20)

Besides, I was not sure if you were planning to add similar helper or just wrappers to individual functions. Wanted to ping you just in case this has some impact to what you do.

I don't have a need to bind a device to a driver to satisfy devm APIs currently. I could probably use it though to test some devm code in the clk APIs. The only impact is that we're modifying the same files.

Quoted 8 lines

Quoted 2 lines

Have you Cced kunit folks and the
list on the kunit patches? They may have some opinion.

This patch was should have contained the include/kunit/platform_device.h. That file was pulling the Kunit people in recipients but I messed up things with last minute changes so both the header and people were dropped. I'll fix this for v5.

Ok, I'll be on the lookout for v5. From what I can tell kunit goes through the kernel selftest tree and there isn't a kunit git tree as such.

Quoted 5 lines · Stephen Boyd

Quoting Matti Vaittinen (2023-03-20 22:45:52)

Quoted 2 lines · Stephen Boyd

Quoted 1 line

Quoting Matti Vaittinen (2023-03-18 23:36:20)

Quoted 7 lines

Quoted 3 lines

Besides, I was not sure if you were planning to add similar helper or just wrappers to individual functions. Wanted to ping you just in case this has some impact to what you do.

I don't have a need to bind a device to a driver to satisfy devm APIs currently. I could probably use it though to test some devm code in the clk APIs. The only impact is that we're modifying the same files.

Thanks for clarifying this.

Quoted 11 lines

Quoted 7 lines

Quoted 2 lines

Have you Cced kunit folks and the
list on the kunit patches? They may have some opinion.

This patch was should have contained the include/kunit/platform_device.h. That file was pulling the Kunit people in recipients but I messed up things with last minute changes so both the header and people were dropped. I'll fix this for v5.

Ok, I'll be on the lookout for v5. From what I can tell kunit goes through the kernel selftest tree and there isn't a kunit git tree as such.

Right. I am not sure what will be the best tree to carry the testability changes. It seems that all of the IIO-tests in v5 will depend on the kunit stuff, and I think I will also include the DRM test fixes in this series as well just to keep things sorted in my mailbox. Anyways, I hope to finish the changes for v5 soon(ish) - maybe already Today and in any case during this week. I'll be CC:ing you and Brendan with (relevant patches of) v5 as well.

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

---
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/industrialio-gts-helper.c new file +0 -0

new file mode 100644
index 000000000000..9494ea7cdbcf
@@ -0,0 +1,990 @@

drivers/iio/industrialio-gts-helper.c new file +12 -0

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

+ */

drivers/iio/industrialio-gts-helper.c new file +69 -0

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

drivers/iio/industrialio-gts-helper.c new file +16 -0

+
+	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.
+ *

drivers/iio/industrialio-gts-helper.c new file +7 -0

+	if (!per_time_gains)
+		return ret;
+
+	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+	if (!per_time_scales)
+		goto free_gains;
+

drivers/iio/industrialio-gts-helper.c new file +36 -0

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

drivers/iio/industrialio-gts-helper.c new file +9 -0

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

drivers/iio/industrialio-gts-helper.c new file +5 -0

+	 */
+	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).

+ */

drivers/iio/industrialio-gts-helper.c new file +12 -0

+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);
+}
+
+/**

+ * 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.
+ */

drivers/iio/industrialio-gts-helper.c new file +11 -0

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

drivers/iio/industrialio-gts-helper.c new file +10 -0

+}
+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;

drivers/iio/industrialio-gts-helper.c new file +4 -0

+
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
+{
+	int i;

drivers/iio/industrialio-gts-helper.c new file +20 -0

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

drivers/iio/industrialio-gts-helper.c new file +37 -0

+		 */
+		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

+ */

drivers/iio/industrialio-gts-helper.c new file +20 -0

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

drivers/iio/industrialio-gts-helper.c new file +8 -0

+}
+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))

drivers/iio/industrialio-gts-helper.c new file +24 -0

+}
+
+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)
+{

+ * @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.
+ */

drivers/iio/industrialio-gts-helper.c new file +32 -0

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

Quoted 19 lines · On Fri, 17 Mar 2023 16:43:23 +0200 Matti Vaittinen <[email protected]>…

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.

Quoted 4 lines

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

Signed-off-by Matti Vaittinen <[email protected]>

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.

Quoted 6 lines

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

Quoted 10 lines

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_

Quoted 34 lines

- 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/industrialio-gts-helper.c new file +0 -0

new file mode 100644
index 000000000000..9494ea7cdbcf
@@ -0,0 +1,990 @@

...

drivers/iio/industrialio-gts-helper.c line 93

Quoted 3 lines

drivers/iio/industrialio-gts-helper.c new file +3 -0

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

Quoted 13 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

+{
+	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;
+}

...

Quoted 9 lines

+/**
+ * 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.

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

+	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],

drivers/iio/industrialio-gts-helper.c line 333

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

+					&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.

Quoted 21 lines

drivers/iio/industrialio-gts-helper.c new file +21 -0

+
+	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.
+ */

drivers/iio/industrialio-gts-helper.c line 359

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

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

Quoted 7 lines

drivers/iio/industrialio-gts-helper.c new file +7 -0

+	if (!per_time_gains)
+		return ret;
+
+	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+	if (!per_time_scales)
+		goto free_gains;
+

drivers/iio/industrialio-gts-helper.c line 376

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

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

drivers/iio/industrialio-gts-helper.c line 386

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

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

Quoted 41 lines

drivers/iio/industrialio-gts-helper.c new file +40 -0

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

drivers/iio/industrialio-gts-helper.c line 432

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

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

Quoted 16 lines

drivers/iio/industrialio-gts-helper.c new file +15 -0

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

drivers/iio/industrialio-gts-helper.c line 451

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

+	/*
+	 * 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.

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

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

...

Quoted 11 lines

+/**
+ * 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.

Quoted 13 lines

+ */

drivers/iio/industrialio-gts-helper.c new file +12 -0

+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);
+}
+
+/**

drivers/iio/industrialio-gts-helper.c line 550

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

+ * 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.

Quoted 8 lines

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

Quoted 8 lines

+ * 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.

Quoted 16 lines

+ *
+ * Return: 0 on success.
+ */

drivers/iio/industrialio-gts-helper.c new file +11 -0

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

drivers/iio/industrialio-gts-helper.c

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

+/**
+ * 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?

Quoted 17 lines

drivers/iio/industrialio-gts-helper.c new file +15 -0

+}
+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);
+

drivers/iio/industrialio-gts-helper.c line 691

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

+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

Quoted 13 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

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

drivers/iio/industrialio-gts-helper.c line 705

Quoted 2 lines

drivers/iio/industrialio-gts-helper.c new file +2 -0

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

Quoted 25 lines

drivers/iio/industrialio-gts-helper.c new file +24 -0

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

Quoted 6 lines

+ * 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.

Quoted 4 lines

+ * gain was smaller than the smallest supported gain.
+ */

drivers/iio/industrialio-gts-helper.c new file +1 -0

+int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range)

drivers/iio/industrialio-gts-helper.c line 748

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

+{
+	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.

Quoted 3 lines

+/*
+ * 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.

Quoted 29 lines

+ */

drivers/iio/industrialio-gts-helper.c new file +27 -0

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

drivers/iio/industrialio-gts-helper.c line 875

Quoted 3 lines

drivers/iio/industrialio-gts-helper.c new file +3 -0

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

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c new file +8 -0

+}
+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);

drivers/iio/industrialio-gts-helper.c line 887

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

+	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()

Quoted 12 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

+}
+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;

drivers/iio/industrialio-gts-helper.c line 905

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

+
+	itime = iio_gts_find_itime_by_time(gts, time);
+	if (!itime)
+		return -EINVAL;
+
+	return gain * itime->mul;

Check for overflow perhaps?

Quoted 29 lines

drivers/iio/industrialio-gts-helper.c new file +29 -0

+}
+
+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)

drivers/iio/industrialio-gts-helper.c line 939

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

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

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.

drivers/iio/industrialio-gts-helper.c line 942

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +3 -0

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

compensate for time change

Quoted 5 lines · Jonathan Cameron

On Fri, 17 Mar 2023 16:43:23 +0200
Matti Vaittinen <[email protected]> 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...

Quoted 4 lines

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

Quoted 22 lines · Jonathan Cameron

Quoted 19 lines · On Fri, 17 Mar 2023 16:43:23 +0200 Matti Vaittinen <[email protected]>…

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.

Quoted 8 lines

Quoted 4 lines

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

Signed-off-by Matti Vaittinen <[email protected]>

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.

Quoted 6 lines

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

Quoted 5 lines

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

Quoted 17 lines

Quoted 6 lines

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

Quoted 10 lines

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)

Quoted 2 lines

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.

Quoted 11 lines

Quoted 10 lines

+/**
+ * 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).

Quoted 2 lines

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

Quoted 18 lines

Quoted 18 lines

drivers/iio/industrialio-gts-helper.c new file +18 -0

+
+/**
+ * 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.
+ */

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 359

Quoted 5 lines

drivers/iio/industrialio-gts-helper.c new file +5 -0

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

Quoted 21 lines

Quoted 21 lines

drivers/iio/industrialio-gts-helper.c new file +21 -0

+
+/**
+ * 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);

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 432

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

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

Quoted 17 lines

Quoted 17 lines

drivers/iio/industrialio-gts-helper.c new file +15 -0

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

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 451

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

+	/*
+	 * 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.

Quoted 12 lines

Quoted 12 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

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

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 705

Quoted 2 lines

drivers/iio/industrialio-gts-helper.c new file +2 -0

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

Quoted 5 lines

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.

Quoted 17 lines

drivers/iio/industrialio-gts-helper.c line 887

Quoted 13 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

+}
+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.

Quoted 2 lines

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.

Quoted 13 lines

Quoted 13 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

+}
+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;

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 905

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

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

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c line 942

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +3 -0

+
+/**
+ * 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

Quoted 37 lines · On Mon, 20 Mar 2023 14:01:55 +0200 Matti Vaittinen <[email protected]>…

Quoted 22 lines · Jonathan Cameron

Quoted 19 lines · On Fri, 17 Mar 2023 16:43:23 +0200 Matti Vaittinen <[email protected]>…

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.

Quoted 8 lines

Quoted 4 lines

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

Signed-off-by Matti Vaittinen <[email protected]>

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.

Quoted 7 lines

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?

Quoted 18 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 35 lines

Quoted 4 lines

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

Quoted 17 lines

Quoted 6 lines

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

Quoted 10 lines

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)

Quoted 2 lines

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.

Quoted 2 lines

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.

...

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

+/**
+ * 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).

Quoted 8 lines

Quoted 2 lines

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.

Quoted 29 lines

Quoted 28 lines

Quoted 15 lines

drivers/iio/industrialio-gts-helper.c new file +13 -0

+
+/**
+ * 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.
+ *

drivers/iio/industrialio-gts-helper.c line 359

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

+ * 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.

Quoted 3 lines

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.

Quoted 35 lines

Quoted 21 lines

Quoted 21 lines

drivers/iio/industrialio-gts-helper.c new file +21 -0

+
+/**
+ * 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);

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 432

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

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

Quoted 2 lines

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

Perfect.

Quoted 38 lines

Quoted 18 lines

Quoted 18 lines

drivers/iio/industrialio-gts-helper.c new file +15 -0

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

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 451

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

+	/*
+	 * 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

Quoted 5 lines

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.

Quoted 21 lines

Quoted 20 lines

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

+
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
+{
+	int i;

drivers/iio/industrialio-gts-helper.c line 705

Quoted 10 lines

drivers/iio/industrialio-gts-helper.c new file +10 -0

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

Quoted 3 lines

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.

....

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 24 lines

Quoted 8 lines

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c new file +8 -0

+}
+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);

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 887

Quoted 4 lines

drivers/iio/industrialio-gts-helper.c new file +4 -0

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

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

drivers/iio/industrialio-gts-helper.c new file +12 -0

+}
+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;

Quoted 8 lines

drivers/iio/industrialio-gts-helper.c line 905

Quoted 6 lines

drivers/iio/industrialio-gts-helper.c new file +6 -0

+
+	itime = iio_gts_find_itime_by_time(gts, time);
+	if (!itime)
+		return -EINVAL;
+
+	return gain * itime->mul;

Check for overflow perhaps?

Quoted 5 lines

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.

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

drivers/iio/test/Kconfig +14 -0

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

drivers/iio/test/iio-test-gts.c new file +0 -0

new file mode 100644
index 000000000000..ff9311acd0bb
@@ -0,0 +1,461 @@

drivers/iio/test/iio-test-gts.c new file +4 -0

+
+	return ret;
+}
+
 

drivers/iio/test/iio-test-gts.c new file +64 -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",

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

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

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

---
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/rohm-bu27034.c new file +23 -0

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

drivers/iio/light/rohm-bu27034.c new file +80 -0

+	.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;
+			}

drivers/iio/light/rohm-bu27034.c new file +18 -0

+			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;
+}

drivers/iio/light/rohm-bu27034.c new file +19 -0

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

+}

drivers/iio/light/rohm-bu27034.c new file +10 -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);

drivers/iio/light/rohm-bu27034.c new file +11 -0

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