Support ROHM BU27034 ALS 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.

Add initial dt-bindings.

Quoted 5 lines · Matti Vaittinen

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 initial dt-bindings.

Driver can be "initial", but bindings better to be closer to complete, even if not used by the driver currently.

Quoted 6 lines

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

---
  .../bindings/iio/light/rohm-bu27034.yaml      | 46 +++++++++++++++++++
  1 file changed, 46 insertions(+)

create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml

Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml

Quoted 5 lines

Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml new file +0 -0

new file mode 100644
index 000000000000..a3a642c259e8

Comma as a separator, so:
rohm,bu27034.yaml

Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml line 4

Quoted 5 lines

Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml new file +4 -0

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

With filename and $id fix:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Hi dee Ho Krzysztof,

Thanks for the review! It's nice you had the time to take a look on RFC :)

Quoted 7 lines · Krzysztof Kozlowski

Quoted 5 lines · Matti Vaittinen

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 initial dt-bindings.

Quoted 2 lines

Driver can be "initial", but bindings better to be closer to complete, even if not used by the driver currently.

Out of the curiosity - why is that? (Please, don't take me wrong, I am not trying to argue against this - just learn the reason behind). I can't immediately see the harm caused by adding new properties later when we learn more of hardware. (and no, I don't expect this simple IC to gain at least many properties).

Quoted 8 lines · Matti

Quoted 7 lines · Krzysztof Kozlowski

Quoted 5 lines · Matti Vaittinen

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 initial dt-bindings.

Quoted 8 lines

Quoted 2 lines

Driver can be "initial", but bindings better to be closer to complete, even if not used by the driver currently.

Out of the curiosity - why is that? (Please, don't take me wrong, I am not trying to argue against this - just learn the reason behind). I can't immediately see the harm caused by adding new properties later when we learn more of hardware. (and no, I don't expect this simple IC to gain at least many properties).

Linux drivers change, but the hardware does not, thus DTS, which describes the hardware, can be complete. It should be written based on the hardware, not based on Linux drivers. If you add incomplete bindings, this suggests you wrote them to match your driver, not to match hardware. This in turn (adjusting bindings to driver) makes them less portable, narrowed to one specific driver implementation and more ABI-break-prone later.

Imagine you that clock inputs, which you skipped in the binding, were actually needed but on your board they were enabled by bootloader. The binding is then used on other systems or by out of tree users. On your new system the clocks are not enabled by bootloader anymore, thus you add them to the binding. They are actually required for device to work, so you make them required. But all these other users cannot be fixed...

What's more, incomplete binding/DTS is then used together with other pieces - DTS and driver, e.g. via some graphs or other phandles/supplies/pinctrl. So some other DTS or driver code might rely on your particular binding. Imagine you had only vdd-supply regulator, but no reset pins, so the only way to power-cycle device was to turn off/on regulator supply. Then you figure out that you have reset pins and it would be useful to add and use it. But already drivers are written to power cycle via regulator... or even someone wrote new driver regulator-pwrseq to power cycle your device due to missing reset GPIOs...

Quoted 12 lines · Matti

Hi dee Ho Krzysztof,

Thanks for the review! It's nice you had the time to take a look on RFC :)

Quoted 7 lines · Krzysztof Kozlowski

Quoted 5 lines · Matti Vaittinen

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 initial dt-bindings.

Quoted 8 lines

Quoted 2 lines

Driver can be "initial", but bindings better to be closer to complete, even if not used by the driver currently.

Out of the curiosity - why is that? (Please, don't take me wrong, I am not trying to argue against this - just learn the reason behind). I can't immediately see the harm caused by adding new properties later when we learn more of hardware. (and no, I don't expect this simple IC to gain at least many properties).

Linux drivers change, but the hardware does not, thus DTS, which describes the hardware, can be complete. It should be written based on the hardware, not based on Linux drivers. If you add incomplete bindings, this suggests you wrote them to match your driver, not to match hardware. This in turn (adjusting bindings to driver) makes them less portable, narrowed to one specific driver implementation and more ABI-break-prone later.

Imagine you that clock inputs, which you skipped in the binding, were actually needed but on your board they were enabled by bootloader. The binding is then used on other systems or by out of tree users. On your new system the clocks are not enabled by bootloader anymore, thus you add them to the binding. They are actually required for device to work, so you make them required. But all these other users cannot be fixed...

What's more, incomplete binding/DTS is then used together with other pieces - DTS and driver, e.g. via some graphs or other phandles/supplies/pinctrl. So some other DTS or driver code might rely on your particular binding. Imagine you had only vdd-supply regulator, but no reset pins, so the only way to power-cycle device was to turn off/on regulator supply. Then you figure out that you have reset pins and it would be useful to add and use it. But already drivers are written to power cycle via regulator... or even someone wrote new driver regulator-pwrseq to power cycle your device due to missing reset GPIOs...

Best regards,
Krzysztof

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.

---
 drivers/iio/light/Kconfig                  |   3 +
 drivers/iio/light/Makefile                 |   1 +
 drivers/iio/light/gain-time-scale-helper.c | 446 +++++++++++++++++++++
 drivers/iio/light/gain-time-scale-helper.h | 111 +++++
 4 files changed, 561 insertions(+)
 create mode 100644 drivers/iio/light/gain-time-scale-helper.c
 create mode 100644 drivers/iio/light/gain-time-scale-helper.h

drivers/iio/light/Kconfig +3 -0

index 0d4447df7200..671d84f98c56 100644
@@ -183,6 +183,9 @@ config IIO_CROS_EC_LIGHT_PROX
 	  To compile this driver as a module, choose M here:
 	  the module will be called cros_ec_light_prox.
 
+config IIO_GTS_HELPER
+	tristate
+
 config GP2AP002
 	tristate "Sharp GP2AP002 Proximity/ALS sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index 6f23817fae6f..f4705fac7a96 100644
@@ -20,6 +20,7 @@ obj-$(CONFIG_CM3323)		+= cm3323.o
 obj-$(CONFIG_CM3605)		+= cm3605.o
 obj-$(CONFIG_CM36651)		+= cm36651.o
 obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
+obj-$(CONFIG_IIO_GTS_HELPER)	+= gain-time-scale-helper.o
 obj-$(CONFIG_GP2AP002)		+= gp2ap002.o
 obj-$(CONFIG_GP2AP020A00F)	+= gp2ap020a00f.o
 obj-$(CONFIG_HID_SENSOR_ALS)	+= hid-sensor-als.o

drivers/iio/light/gain-time-scale-helper.c new file +4 -0

new file mode 100644
index 000000000000..bd8fc11802ee
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>

drivers/iio/light/gain-time-scale-helper.c new file +8 -0

+		/* Risk of overflow */
+		if (max - scale < scale)
+			return 1;
+
+		while (max - scale > scale * (u64) tmp)
+			tmp++;
+
+		return tmp + 1;

drivers/iio/light/gain-time-scale-helper.c new file +20 -0

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

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

+{
+	int frac;
+
+	if (scaler > NANO || !scaler)
+		return -EINVAL;
+

drivers/iio/light/gain-time-scale-helper.c new file +17 -0

+{
+	/*
+	 * 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_init_iio_gts - Initialize the gain-time-scale helper
+ * @max_scale_int:	integer part of the maximum scale value

The search functions like the
+ * iio_gts_find_time_and_gain_sel_for_scale() start search from first
+ * provided time.
+ *
+ * Return: 0 on success.
+ */

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

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

drivers/iio/light/gain-time-scale-helper.c new file +8 -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++)

drivers/iio/light/gain-time-scale-helper.c new file +266 -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_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;
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_int_time_gain_multiplier_by_sel);
+
+/**
+ * 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.
+ */
+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, &scale_linear, NANO);
+	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 || !iio_gts_valid_gain(gts, *gain))gts
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_gain_for_scale_using_time);
+
+/*
+ * 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_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int,
+					     int scale_nano, int *gain_sel,
+					     int *time_sel)
+{
+	int ret, i;
+
+	for (i = 0; i < gts->num_itime; i++) {
+		*time_sel = gts->itime_table[i].sel;
+		ret = iio_gts_find_gain_sel_for_scale_using_time(gts, *time_sel,
+					scale_int, scale_nano, gain_sel);
+		if (!ret)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_time_and_gain_sel_for_scale);
+
+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);
+
+int iio_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel)
+{
+	int ret, tmp;
+
+	ret = iio_gts_find_gain_by_sel(gts, gsel);
+	if (ret < 0)
+		return ret;
+
+	tmp = ret;
+
+	/*
+	 * TODO: Would these helpers provde any value for cases where we just
+	 * use table of gains and no integration time? This would be a standard
+	 * format for gain table representation and regval => gain / gain =>
+	 * regval conversions. OTOH, a dummy table based conversion is a memory
+	 * hog in cases where the gain could be computed simply based on simple
+	 * multiplication / bit-shift or by linear_ranges helpers.
+	 *
+	 * Currently we return an error if int-time table is not populated.
+	 */
+	ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, tsel);
+	if (ret < 0)
+		return ret;
+
+	return tmp * ret;
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_total_gain_by_sel);
+
+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;
+}
+EXPORT_SYMBOL(iio_gts_get_total_gain);
+
+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, scale_int, scale_nano, 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 <[email protected]>");
+MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");

drivers/iio/light/gain-time-scale-helper.h new file +56 -0

new file mode 100644
index 000000000000..70a952a8de92
@@ -0,0 +1,111 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>
+ */
+
+#ifndef __GAIN_TIME_SCALE_HELPER__
+#define ___GAIN_TIME_SCALE_HELPER__
+
+/**
+ * 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;
+};

drivers/iio/light/gain-time-scale-helper.h new file +45 -0

+	.sel = (_sel),					\
+	.mul = (_mul),					\
+}
+
+#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_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel);
+int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time);
+
+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_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_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts,
+						       int sel);
+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_find_gain_for_scale_using_time(struct iio_gts *gts, int time_sel,
+					   int scale_int, int scale_nano,
+					   int *gain);
+int iio_gts_find_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int,
+					     int scale_nano, int *gain_sel,
+					     int *time_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);
+
+#endif

Quoted 16 lines · On Wed, 22 Feb 2023 18:14:45 +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.

Quoted 2 lines

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

Do you allow for approximately correct gains? Often there are significant restrictions in what is possible and it would be reasonable to allow a little slack (though obviously the sysfs value would change a bit to reflect that).

Quoted 5 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]>

I was going to suggest just rolling this into the IIO core, but as it's a less than trivial amount of code, maybe not!

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

---
  drivers/iio/light/Kconfig                  |   3 +
  drivers/iio/light/Makefile                 |   1 +
  drivers/iio/light/gain-time-scale-helper.c | 446 +++++++++++++++++++++
  drivers/iio/light/gain-time-scale-helper.h | 111 +++++
  4 files changed, 561 insertions(+)

create mode 100644 drivers/iio/light/gain-time-scale-helper.c create mode 100644 drivers/iio/light/gain-time-scale-helper.h

drivers/iio/light/Kconfig +3 -0

index 0d4447df7200..671d84f98c56 100644
@@ -183,6 +183,9 @@ config IIO_CROS_EC_LIGHT_PROX
  	  To compile this driver as a module, choose M here:
  	  the module will be called cros_ec_light_prox.
 
+config IIO_GTS_HELPER
+	tristate
+
  config GP2AP002
  	tristate "Sharp GP2AP002 Proximity/ALS sensor"
  	depends on I2C

drivers/iio/light/Makefile +1 -0

index 6f23817fae6f..f4705fac7a96 100644
@@ -20,6 +20,7 @@ obj-$(CONFIG_CM3323)		+= cm3323.o
  obj-$(CONFIG_CM3605)		+= cm3605.o
  obj-$(CONFIG_CM36651)		+= cm36651.o
  obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
+obj-$(CONFIG_IIO_GTS_HELPER)	+= gain-time-scale-helper.o
  obj-$(CONFIG_GP2AP002)		+= gp2ap002.o
  obj-$(CONFIG_GP2AP020A00F)	+= gp2ap020a00f.o
  obj-$(CONFIG_HID_SENSOR_ALS)	+= hid-sensor-als.o

drivers/iio/light/gain-time-scale-helper.c new file +4 -0

new file mode 100644
index 000000000000..bd8fc11802ee
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>

drivers/iio/light/gain-time-scale-helper.c line 14

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/units.h>
+
+#include "gain-time-scale-helper.h"
+
+static int iio_gts_get_gain(const u64 max, u64 scale)

trivial but scale equally const in here.
Not immediately obvious what this function does from name, so add some docs.

drivers/iio/light/gain-time-scale-helper.c line 21

Quoted 7 lines

drivers/iio/light/gain-time-scale-helper.c new file +7 -0

+{
+	int tmp = 1;
+
+	if (scale > max || !scale)
+		return -EINVAL;
+
+	if (0xffffffffffffffffLLU - max < scale) {

U64_MAX from limits.h?

Quoted 8 lines

drivers/iio/light/gain-time-scale-helper.c new file +8 -0

+		/* Risk of overflow */
+		if (max - scale < scale)
+			return 1;
+
+		while (max - scale > scale * (u64) tmp)
+			tmp++;
+
+		return tmp + 1;

drivers/iio/light/gain-time-scale-helper.c line 39

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+	}
+
+	while (max > scale * (u64) tmp)
+		tmp++;
+
+	return tmp;
+}
+
+static int gain_get_scale_fraction(const u64 max, u64 scale, int known,
+				   int *unknown)

Needs some basic docs.

drivers/iio/light/gain-time-scale-helper.c line 44

Quoted 5 lines

drivers/iio/light/gain-time-scale-helper.c new file +5 -0

+{
+	int tot_gain;
+
+	if (!known)
+		return -EINVAL;

We don't normally bother checking for NULL pointers unless calling with one is meaningful or the compiler is warning. If it's warning add a comment to say this is to suppress the warning.

drivers/iio/light/gain-time-scale-helper.c

Quoted 6 lines

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

+
+	tot_gain = iio_gts_get_gain(max, scale);
+	if (tot_gain < 0)
+		return tot_gain;
+
+	*unknown = tot_gain/known;gts_valid_gain

spaces around /

Quoted 20 lines

drivers/iio/light/gain-time-scale-helper.c new file +20 -0

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

drivers/iio/light/gain-time-scale-helper.c line 80

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+	return NULL;
+}
+
+static const struct iio_itime_sel_mul *
+			iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel)
+{
+	int i;
+
+	if (!gts->num_itime)
+		return NULL;

As mentioned below. I'd check this in the init function and after that assume that it is fine. In this particular case for loop won't do anything anwyay as i = 0 is not less than 0

drivers/iio/light/gain-time-scale-helper.c line 90

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+
+	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, int *scale_whole, int *scale_nano,
+			       unsigned long scaler)

Same comment as below about about parameter ordering.

Quoted 6 lines

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

+{
+	int frac;
+
+	if (scaler > NANO || !scaler)
+		return -EINVAL;
+

drivers/iio/light/gain-time-scale-helper.c line 106

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+	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, u64 *lin_scale,
+			     unsigned long scaler)

Mixing up inputs and outputs in parameter order is not a common thing to do. I'd do all the inputs first then the outputs.

Quoted 17 lines

drivers/iio/light/gain-time-scale-helper.c new file +17 -0

+{
+	/*
+	 * 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_init_iio_gts - Initialize the gain-time-scale helper
+ * @max_scale_int:	integer part of the maximum scale value

drivers/iio/light/gain-time-scale-helper.c

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+ * @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
+ * @num_times:		number of times in the time table
+ * @gts:		pointer to the helper struct
+ *
+ * Initialize the gain-time-scale helper for use. Please, provide the
+ * integration time table sorted so that the preferred integration time is
+ * in the first array index.

Document that in the parameter descriptions not here. And skip the please :)

Quoted 12 lines

The search functions like the
+ * iio_gts_find_time_and_gain_sel_for_scale() start search from first
+ * provided time.
+ *
+ * Return: 0 on success.
+ */

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

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

drivers/iio/light/gain-time-scale-helper.c line 154

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+
+	ret = iio_gts_linearize(max_scale_int, max_scale_nano,
+				   &gts->max_scale, NANO);
+	if (ret)
+		return ret;
+
+	gts->hwgain_table = gain_tbl;
+	gts->num_hwgain = num_gain;
+	gts->itime_table = tim_tbl;
+	gts->num_itime = num_times;

I think all these need to be provided for this to do anything useful. So check them here and drop the checks in the various other functions.

drivers/iio/light/gain-time-scale-helper.c line 162

Quoted 8 lines

drivers/iio/light/gain-time-scale-helper.c new file +8 -0

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_init_iio_gts);
+
+bool iio_gts_valid_time(struct iio_gts *gts, int time_us)
+{
+	return !!iio_gts_find_itime_by_time(gts, time_us);

it's return a bool, no need for the !! dance.

Quoted 8 lines

drivers/iio/light/gain-time-scale-helper.c new file +8 -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++)

drivers/iio/light/gain-time-scale-helper.c line 180

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

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

return iio_gts_find_sel_by_gain >= 0;

Quoted 328 lines

drivers/iio/light/gain-time-scale-helper.c new file +266 -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_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;
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_int_time_gain_multiplier_by_sel);
+
+/**
+ * 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.
+ */
+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, &scale_linear, NANO);
+	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 || !iio_gts_valid_gain(gts, *gain))gts
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_gain_for_scale_using_time);
+
+/*
+ * 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_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int,
+					     int scale_nano, int *gain_sel,
+					     int *time_sel)
+{
+	int ret, i;
+
+	for (i = 0; i < gts->num_itime; i++) {
+		*time_sel = gts->itime_table[i].sel;
+		ret = iio_gts_find_gain_sel_for_scale_using_time(gts, *time_sel,
+					scale_int, scale_nano, gain_sel);
+		if (!ret)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_time_and_gain_sel_for_scale);
+
+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);
+
+int iio_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel)
+{
+	int ret, tmp;
+
+	ret = iio_gts_find_gain_by_sel(gts, gsel);
+	if (ret < 0)
+		return ret;
+
+	tmp = ret;
+
+	/*
+	 * TODO: Would these helpers provde any value for cases where we just
+	 * use table of gains and no integration time? This would be a standard
+	 * format for gain table representation and regval => gain / gain =>
+	 * regval conversions. OTOH, a dummy table based conversion is a memory
+	 * hog in cases where the gain could be computed simply based on simple
+	 * multiplication / bit-shift or by linear_ranges helpers.
+	 *
+	 * Currently we return an error if int-time table is not populated.
+	 */
+	ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, tsel);
+	if (ret < 0)
+		return ret;
+
+	return tmp * ret;
+}
+EXPORT_SYMBOL_GPL(iio_gts_get_total_gain_by_sel);
+
+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;
+}
+EXPORT_SYMBOL(iio_gts_get_total_gain);
+
+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, scale_int, scale_nano, 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 <[email protected]>");
+MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");

drivers/iio/light/gain-time-scale-helper.h new file +56 -0

new file mode 100644
index 000000000000..70a952a8de92
@@ -0,0 +1,111 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>
+ */
+
+#ifndef __GAIN_TIME_SCALE_HELPER__
+#define ___GAIN_TIME_SCALE_HELPER__
+
+/**
+ * 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;
+};

drivers/iio/light/gain-time-scale-helper.h line 66

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.h new file +10 -0

+
+#define GAIN_SCALE_GAIN(_gain, _sel)			\
+{							\
+	.gain = (_gain),				\
+	.sel = (_sel),					\
+}
+
+#define GAIN_SCALE_ITIME_MS(_itime, _sel, _mul)		\
+{							\
+	.time_us = (_itime) * 1000,				\

Not sure this macro adds much. Just use the * 1000 at callers.

Quoted 45 lines

drivers/iio/light/gain-time-scale-helper.h new file +45 -0

+	.sel = (_sel),					\
+	.mul = (_mul),					\
+}
+
+#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_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel);
+int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time);
+
+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_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_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts,
+						       int sel);
+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_find_gain_for_scale_using_time(struct iio_gts *gts, int time_sel,
+					   int scale_int, int scale_nano,
+					   int *gain);
+int iio_gts_find_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int,
+					     int scale_nano, int *gain_sel,
+					     int *time_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);
+
+#endif

Review

Quoted 2 lines · On Wed, 22 Feb 2023 18:14:45 +0200 Matti Vaittinen <[email protected]>…

(quoted diff) +2 -0

+
+static int iio_gts_get_gain(const u64 max, u64 scale)

trivial but scale equally const in here.

Not immediately obvious what this function does from name, so add some docs.

Quoted 10 lines

drivers/iio/light/gain-time-scale-helper.c new file +10 -0

+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;
+
+	ret = iio_gts_linearize(max_scale_int, max_scale_nano,
+				   &gts->max_scale, NANO);
+	if (ret)

drivers/iio/light/gain-time-scale-helper.c line 154

Quoted 6 lines

drivers/iio/light/gain-time-scale-helper.c new file +6 -0

+		return ret;
+
+	gts->hwgain_table = gain_tbl;
+	gts->num_hwgain = num_gain;
+	gts->itime_table = tim_tbl;
+	gts->num_itime = num_times;

I think all these need to be provided for this to do anything useful.

So check them here and drop the checks in the various other functions.

- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

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

+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 (val & BU27034_MASK_VALID);
+}
+
+static void bu27034_invalidate_read_data(struct bu27034_data *data)
+{
+	bu27034_has_valid_sample(data);
+}
+
+static int _bu27034_get_result(struct bu27034_data *data, u16 *res, bool lock)
+{
+	int ret = 0;
+
+retry:
+	if (lock)
+		mutex_lock(&data->mutex);
+	/* Get new value from sensor if data is ready - or use cached value */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+				       &data->raw[0], sizeof(data->raw));
+		if (ret)
+			goto unlock_out;
+
+		data->cached = true;
+		bu27034_invalidate_read_data(data);
+	} else if (unlikely(!data->cached)) {
+		/* No new data in sensor and no value cached. Wait and retry */
+		if (lock)
+			mutex_unlock(&data->mutex);
+		msleep(25);
+
+		goto retry;
+	}
+	res[0] = le16_to_cpu(data->raw[0]);
+	res[1] = le16_to_cpu(data->raw[1]);
+	res[2] = le16_to_cpu(data->raw[2]);
+
+unlock_out:
+	if (lock)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}

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

+	case IIO_CHAN_INFO_RAW:
+	{
+		u16 res[3];
+
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)
+			return -EINVAL;
+		/*
+		 * Reading one channel at a time is inefficient.
+		 *
+		 * Hence we run the measurement on the background and always
+		 * read all the channels. There are following caveats:
+		 * 1) The VALID bit handling is racy. Valid bit clearing is not
+		 * tied to reading the data in the hardware. We clear the
+		 * valid-bit manually _after_ we have read the data - but this
+		 * means there is a small time-window where new result may
+		 * arrive between read and clear. This means we can miss a
+		 * sample. For normal use this should not be fatal because
+		 * usually the light is changing slowly. There might be
+		 * use-cases for measuring more rapidly changing light but this
+		 * driver is unsuitable for those cases anyways. (Smallest
+		 * measurement time we support is 55 mS.)
+		 * 2) Data readings more frequent than the meas_time will return
+		 * the same cached values. This should not be a problem for the

On 2/22/23 18:15, Matti Vaittinen wrote:

//snip

Quoted 7 lines

- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

//snip

Quoted 50 lines

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

+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 (val & BU27034_MASK_VALID);
+}
+
+static void bu27034_invalidate_read_data(struct bu27034_data *data)
+{
+	bu27034_has_valid_sample(data);
+}
+
+static int _bu27034_get_result(struct bu27034_data *data, u16 *res, bool lock)
+{
+	int ret = 0;
+
+retry:
+	if (lock)
+		mutex_lock(&data->mutex);
+	/* Get new value from sensor if data is ready - or use cached value */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+				       &data->raw[0], sizeof(data->raw));
+		if (ret)
+			goto unlock_out;
+
+		data->cached = true;
+		bu27034_invalidate_read_data(data);
+	} else if (unlikely(!data->cached)) {
+		/* No new data in sensor and no value cached. Wait and retry */
+		if (lock)
+			mutex_unlock(&data->mutex);
+		msleep(25);
+
+		goto retry;
+	}
+	res[0] = le16_to_cpu(data->raw[0]);
+	res[1] = le16_to_cpu(data->raw[1]);
+	res[2] = le16_to_cpu(data->raw[2]);
+
+unlock_out:
+	if (lock)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}

drivers/iio/light/rohm-bu27034.c line 864

Quoted 10 lines

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

+
+static int bu27034_get_result_unlocked(struct bu27034_data *data, u16 *res)
+{
+	return _bu27034_get_result(data, res, false);
+}
+
+static int bu27034_get_result(struct bu27034_data *data, u16 *res)
+{
+	return _bu27034_get_result(data, res, true);
+}  

//snip

Quoted 27 lines

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

+	case IIO_CHAN_INFO_RAW:
+	{
+		u16 res[3];
+
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)
+			return -EINVAL;
+		/*
+		 * Reading one channel at a time is inefficient.
+		 *
+		 * Hence we run the measurement on the background and always
+		 * read all the channels. There are following caveats:
+		 * 1) The VALID bit handling is racy. Valid bit clearing is not
+		 * tied to reading the data in the hardware. We clear the
+		 * valid-bit manually _after_ we have read the data - but this
+		 * means there is a small time-window where new result may
+		 * arrive between read and clear. This means we can miss a
+		 * sample. For normal use this should not be fatal because
+		 * usually the light is changing slowly. There might be
+		 * use-cases for measuring more rapidly changing light but this
+		 * driver is unsuitable for those cases anyways. (Smallest
+		 * measurement time we support is 55 mS.)
+		 * 2) Data readings more frequent than the meas_time will return
+		 * the same cached values. This should not be a problem for the

drivers/iio/light/rohm-bu27034.c line 1056

Quoted 10 lines

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

+		 * very same reason 1) is not a problem.
+		 */
+		ret = bu27034_get_result(data, &res[0]);
+		if (ret)
+			return ret;
+
+		*val = res[chan->channel - BU27034_CHAN_DATA0];
+
+		return IIO_VAL_INT;
+	}  

//snip

drivers/iio/light/rohm-bu27034.c line 1099

Quoted 2 lines

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

+static int bu27034_chip_init(struct bu27034_data *data)
+{  

//snip

drivers/iio/light/rohm-bu27034.c line 1124

Quoted 11 lines

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

+
+	/*
+	 * Consider disabling the measurement (and powering off the sensor) for
+	 * runtime pm
+	 */
+	ret = bu27034_meas_en(data);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(data->dev, bu27034_meas_stop, data);
+}  

Well, this "works on my machine" - but I am slightly unhappy with this. I have a feeling I am effectively making a poor, reduced version of data buffering here. I am starting to think that I should

a) Not start measurement at chip init. (saves power)
b) Start measurement at raw-read and just block for damn long for each raw-read. Yep, it probably means that users who want to raw-read all channels will be blocking 4 * measurement time when they are reading all channels one after another. Yes, this is in worst case 4 * 400 mS. Horrible. But see (c) below.

c) Implement triggered_buffer mode. Here my lack of IIO-experience shows up again. I have no idea if there is - or what is - the "de facto" way for implementing this when our device has no IRQ? I could cook-up some 'tiny bit shorter than the measurement time' period timer which would kick the driver to poll the VALID-bit - or, because we need anyways to poll the valid bit from process context - just a kthread which polls the VALID-bit. Naturally the thread/timer should be only activated when the trigger is enabled.

Actually, my question (with this driver, the big question in the RFC is the gain-time-scale helper) seems to be - should I implement triggered_buffer and do we have some generic IIO trigger (timer or thread or whatever) the driver could use or should each driver (which needs this) implement own one?

Thanks for the patience :)
-- Matti

Quoted 6 lines · On Fri, 24 Feb 2023 12:41:46 +0200 Matti Vaittinen <[email protected]>…

On 2/22/23 18:15, Matti Vaittinen wrote:

Well, this "works on my machine" - but I am slightly unhappy with this. I have a feeling I am effectively making a poor, reduced version of data buffering here. I am starting to think that I should

Quoted 6 lines

a) Not start measurement at chip init. (saves power)
b) Start measurement at raw-read and just block for damn long for each raw-read. Yep, it probably means that users who want to raw-read all channels will be blocking 4 * measurement time when they are reading all channels one after another. Yes, this is in worst case 4 * 400 mS. Horrible. But see (c) below.

Hmm. Light sensors tend to be slow in some modes, but rarely do people actually have such low light levels that they are using them with 400mS integration times.

Quoted 8 lines

c) Implement triggered_buffer mode. Here my lack of IIO-experience shows up again. I have no idea if there is - or what is - the "de facto" way for implementing this when our device has no IRQ? I could cook-up some 'tiny bit shorter than the measurement time' period timer which would kick the driver to poll the VALID-bit - or, because we need anyways to poll the valid bit from process context - just a kthread which polls the VALID-bit. Naturally the thread/timer should be only activated when the trigger is enabled.

Firstly you don't have to have a trigger. In a case where it's a bit hacky and unlikely to be particularly useful for other devices, you can just implement a buffer directly.

There are various options that exist..
1) iio-trig-loop - this is nasty but occasionally useful approach. You then make the iio_poll_func wait on the flag.

Quoted 28 lines · Jonathan Cameron

Quoted 6 lines · On Fri, 24 Feb 2023 12:41:46 +0200 Matti Vaittinen <[email protected]>…

On 2/22/23 18:15, Matti Vaittinen wrote:

Well, this "works on my machine" - but I am slightly unhappy with this. I have a feeling I am effectively making a poor, reduced version of data buffering here. I am starting to think that I should

Quoted 6 lines

a) Not start measurement at chip init. (saves power)
b) Start measurement at raw-read and just block for damn long for each raw-read. Yep, it probably means that users who want to raw-read all channels will be blocking 4 * measurement time when they are reading all channels one after another. Yes, this is in worst case 4 * 400 mS. Horrible. But see (c) below.

Hmm. Light sensors tend to be slow in some modes, but rarely do people actually have such low light levels that they are using them with 400mS integration times.

Quoted 8 lines

c) Implement triggered_buffer mode. Here my lack of IIO-experience shows up again. I have no idea if there is - or what is - the "de facto" way for implementing this when our device has no IRQ? I could cook-up some 'tiny bit shorter than the measurement time' period timer which would kick the driver to poll the VALID-bit - or, because we need anyways to poll the valid bit from process context - just a kthread which polls the VALID-bit. Naturally the thread/timer should be only activated when the trigger is enabled.

Quoted 3 lines

Firstly you don't have to have a trigger. In a case where it's a bit hacky and unlikely to be particularly useful for other devices, you can just implement a buffer directly.

This is the approach I took for the next attempt. I just used the iio_kfifo_buffer.

Quoted 3 lines

There are various options that exist..
1) iio-trig-loop - this is nasty but occasionally useful approach. You then make the iio_poll_func wait on the flag.

I actually did take a look at this. The loop trigger had pretty much everything I would have needed - except configurability from the driver.

It had the enable/disable with protected start of the thread and the thread stopping all in place. Really, as you said, the only thing that was missing was "hinting the timing". For a moment I was playing with a thought of trying to implement a simple generic thread-loop code which could take the sleep-time + callback for 'ensuring we slept long enough'
+ a callback for code to execute (collect data + push to buffers) - but it felt like re-implementing existing mechanisms. Besides, as you said, I don't probably need a trigger

I'll do some clean-ups and look through the feedback and try to get the v2 out still during this week.

Yours,
-- Matti

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 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. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has adjustible GAIN values ranging from 1x to 4096x.
- Sensor has adjustible measurement times 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 only can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed.

- Runtime PM is not implemented.
- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

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

---

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

drivers/iio/light/Kconfig +2 -0

index 671d84f98c56..594228bd1f7f 100644
@@ -292,6 +292,19 @@ 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"

drivers/iio/light/Kconfig +1 -0

+
 

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

new file mode 100644
index 000000000000..235be7dee6e0
@@ -0,0 +1,1212 @@
+// 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/bits.h>

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

+/*
+ * 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 aupported 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_MS(400, BU27034_MEAS_MODE_400MS, 8),
+	GAIN_SCALE_ITIME_MS(200, BU27034_MEAS_MODE_200MS, 4),
+	GAIN_SCALE_ITIME_MS(100, BU27034_MEAS_MODE_100MS, 2),
+	GAIN_SCALE_ITIME_MS(50, BU27034_MEAS_MODE_55MS, 1),
+};
+
+#define BU27034_CHAN_DATA(_name, _ch2)					\
+{									\
+	.type = IIO_INTENSITY,						\
+	.channel = BU27034_CHAN_##_name,				\
+	.channel2 = (_ch2),						\

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

+	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+};
 
+
+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;
+
+		val &= BU27034_MASK_D01_GAIN;
+		return val >> BU27034_SHIFT_D01_GAIN;
+	}

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

+
+	return -EINVAL;
+}
+

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

+
+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;
+
+	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) {
+		/* We need to maintain the scale for all channels */
+		int new_time_sel;
+		struct bu27034_gain_check gains[2];
+
+		if (chan == BU27034_CHAN_DATA0) {
+			gains[0].chan = BU27034_CHAN_DATA1;
+			gains[1].chan = BU27034_CHAN_DATA2;
+		} else if (chan == BU27034_CHAN_DATA1) {
+			gains[0].chan = BU27034_CHAN_DATA0;
+			gains[1].chan = BU27034_CHAN_DATA2;
+		} else {
+			gains[0].chan = BU27034_CHAN_DATA0;
+			gains[1].chan = BU27034_CHAN_DATA1;
+		}
+		for (i = 0; i < 2; i++) {
+			ret = bu27034_get_gain(data, gains[i].chan,
+					       &gains[i].old_gain);
+			if (ret)
+				goto unlock_out;
+		}
+
+		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;
+
+			ret = iio_gts_find_gain_sel_for_scale_using_time(
+				&data->gts, new_time_sel, val, val2 * 1000,
+				&gain_sel);
+			if (ret)
+				continue;
+
+			ret = iio_gts_find_new_gain_sel_by_old_gain_time(
+				&data->gts, gains[0].old_gain, time_sel,
+				new_time_sel, &gains[0].new_gain);
+			if (ret)
+				continue;
+
+			ret = iio_gts_find_new_gain_sel_by_old_gain_time(
+				&data->gts, gains[1].old_gain, time_sel,
+				new_time_sel, &gains[1].new_gain);
+			if (!ret) {
+				found = true;
+				break;
+			}

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

+			ret = _bu27034_set_gain(data, gains[0].chan,
+						gains[0].new_gain);
+			if (ret)
+				goto unlock_out;
+
+			ret = _bu27034_set_gain(data, gains[1].chan,
+						gains[1].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; /* * (u64)gain0 */
+	helper = coeff * ch1 * ch1; /* * gain0*/
+	tmp = helper * gain0;
+
+	if (helper == helper64 && (tmp / gain0 == helper))
+		return tmp / (gain1 * gain1) / ch0;
+

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

+		helper64 *= gain0;
+		do_div(helper64, helper);
+	} else {
+		do_div(helper64, helper);
+		helper64 *= gain0;
+	}
+
+	return helper64;
+}
+

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

+static bool bu27034_has_valid_sample(struct bu27034_data *data)
+{
+	int ret, val;
+

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

+}
+
+static int _bu27034_get_result(struct bu27034_data *data, u16 *res, bool lock)
+{
+	int ret = 0;
+
+retry:
+	if (lock)
+		mutex_lock(&data->mutex);
+	/* Get new value from sensor if data is ready - or use cached value */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,

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

+		msleep(25);
+
+		goto retry;
+	}
+	res[0] = le16_to_cpu(data->raw[0]);
+	res[1] = le16_to_cpu(data->raw[1]);
+	res[2] = le16_to_cpu(data->raw[2]);
+
+unlock_out:
+	if (lock)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bu27034_get_result_unlocked(struct bu27034_data *data, u16 *res)
+{
+	return _bu27034_get_result(data, res, false);
+}
+
+static int bu27034_get_result(struct bu27034_data *data, u16 *res)
+{
+	return _bu27034_get_result(data, res, true);
+}
+
+/*
+ * 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

+ * need to modify the calculation but I hope this gives a starting point for
+ * those working with such devices.

+ *
+ * The first case (D1/D0 < 0.87) can be computed to a form:
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 + 0.00012213 * D1 * D1 / D0
+ */

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

+		ch0 = 1;
+	else
+		ch0 = res[0];
+
+	if (!res[1])
+		ch1 = 1;
+	else
+		ch1 = res[1];
+
+
+	ret = bu27034_get_gain(data, BU27034_CHAN_DATA0, &gain0);
+	if (ret)
+		goto unlock_out;
+
+	ret = bu27034_get_gain(data, BU27034_CHAN_DATA1, &gain1);
+	if (ret)
+		goto unlock_out;
+
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		goto unlock_out;
+
+	meastime = ret;
+
+	mutex_unlock(&data->mutex);
+
+	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)
+		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0);
+	else if (d1_d0_ratio_scaled < 100)
+		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1);
+	else
+		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2);
+
+	return 0;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+

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

+
+		ret = bu27034_get_int_time(data);
+		if (ret < 0)
+			return ret;

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

+
+	case IIO_CHAN_INFO_SCALE:
+		ret = bu27034_get_scale(data, chan->channel, val, val2);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT_PLUS_NANO;
+
+	case IIO_CHAN_INFO_RAW:
+	{
+		u16 res[3];
+
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)
+			return -EINVAL;
+		/*
+		 * Reading one channel at a time is inefficient.
+		 *
+		 * Hence we run the measurement on the background and always
+		 * read all the channels. There are following caveats:

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

+		 */
+		ret = bu27034_get_result(data, &res[0]);
+		if (ret)
+			return ret;
+
+		*val = res[chan->channel - BU27034_CHAN_DATA0];
+

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

+	return -EINVAL;
+}
+
+static int bu27034_write_raw(struct iio_dev *idev,

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

+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info bu27034_info = {
+	.read_raw = &bu27034_read_raw,
+	.write_raw = &bu27034_write_raw,
+};
+
+static void bu27034_meas_stop(void *data)
+{
+	bu27034_meas_dis(data);
+}
+
+static int bu27034_chip_init(struct bu27034_data *data)
+{
+	int ret;
+

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

+	 */
+	ret = bu27034_meas_en(data);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(data->dev, bu27034_meas_stop, data);
+}
+
+static int bu27034_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct fwnode_handle *fwnode;
+	struct bu27034_data *data;
+	struct regmap *regmap;

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

+		return -EINVAL;
+	}
+
+	ret = iio_init_iio_gts(BU27034_SCALE_1X, 0, bu27034_gains,
+			       ARRAY_SIZE(bu27034_gains), bu27034_itimes,
+			       ARRAY_SIZE(bu27034_itimes), &data->gts);

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

+	idev->info = &bu27034_info;

drivers/iio/light/rohm-bu27034.c line 129

Quoted 5 lines · On Wed, 22 Feb 2023 18:15:58 +0200 Matti Vaittinen <[email protected]>…

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

+	.scan_type = {							\
+		.sign = 'u',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\

Unless you have buffered support, anything scan_* is unused and shouldn't be set.

drivers/iio/light/rohm-bu27034.c line 141

Quoted 13 lines

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

+	},								\
+	.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,
+	},
+	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
+	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),

That's unusual. Why does it have two clear channels?
Perhaps add a comment on how they differ. From a quick glance at the datasheet they have different sensitivities, but indeed both in the visible light range (mostly)

You could argue one is blue and one is red based on peaks of the curves but they are very broad so perhaps clear is the best naming.

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+		for (i = 0; i < 2; i++) {

Why twice?

Quoted 10 lines

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

+static int _bu27034_get_result(struct bu27034_data *data, u16 *res, bool lock)
+{
+	int ret = 0;
+
+retry:
+	if (lock)
+		mutex_lock(&data->mutex);
+	/* Get new value from sensor if data is ready - or use cached value */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,

drivers/iio/light/rohm-bu27034.c line 840

Quoted 10 lines

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

+				       &data->raw[0], sizeof(data->raw));
+		if (ret)
+			goto unlock_out;
+
+		data->cached = true;
+		bu27034_invalidate_read_data(data);
+	} else if (unlikely(!data->cached)) {
+		/* No new data in sensor and no value cached. Wait and retry */
+		if (lock)
+			mutex_unlock(&data->mutex);

Hmm. We don't really need to fix this in driver. Could just return -EAGAIN and let userspace work out that it needs to try again after a while?
I guess not all userspace is going to be smart enough to handle that though and you need this to ensure we get a new value after a parameter change.

Quoted 1 line

If you do that then the locking dance gets much simpler.

Quoted 9 lines

+ * 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 try implementing it here. Users who have for example some colored lens

There is no try, there is just do :)

Quoted 3 lines

+ * need to modify the calculation but I hope this gives a starting point for
+ * those working with such devices.

That will need some dt bindings - though for now I guess we have no idea what they would be unless there are some hints on the datasheet?

Quoted 10 lines

+ *
+ * The first case (D1/D0 < 0.87) can be computed to a form:
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 + 0.00012213 * D1 * D1 / D0
+ */

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

+static int bu27034_get_lux(struct bu27034_data *data, int *val)
+{
+	unsigned int gain0, gain1, meastime;
+	unsigned int d1_d0_ratio_scaled;
+	u16 res[3], ch0, ch1;
+	u64 helper64;

drivers/iio/light/rohm-bu27034.c line 898

Quoted 4 lines

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

+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = bu27034_get_result_unlocked(data, &res[0]);

res
as it is expecting a point to an array so that is more natural than pointing to the first element even if that's the same result.

drivers/iio/light/rohm-bu27034.c line 903

Quoted 6 lines

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

+	if (ret)
+		goto unlock_out;
+
+	/* Avoid div by zero */
+	if (!res[0])

res[0] = max(1, res[0]); perhaps?

Quoted 15 lines

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

+	case IIO_CHAN_INFO_RAW:
+	{
+		u16 res[3];
+
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)
+			return -EINVAL;
+		/*
+		 * Reading one channel at a time is inefficient.
+		 *
+		 * Hence we run the measurement on the background and always
+		 * read all the channels. There are following caveats:

drivers/iio/light/rohm-bu27034.c line 1044

Quoted 10 lines

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

+		 * 1) The VALID bit handling is racy. Valid bit clearing is not
+		 * tied to reading the data in the hardware. We clear the
+		 * valid-bit manually _after_ we have read the data - but this
+		 * means there is a small time-window where new result may
+		 * arrive between read and clear. This means we can miss a
+		 * sample. For normal use this should not be fatal because
+		 * usually the light is changing slowly. There might be
+		 * use-cases for measuring more rapidly changing light but this
+		 * driver is unsuitable for those cases anyways. (Smallest
+		 * measurement time we support is 55 mS.)

Given there is no general fix for that, not much you can do even if you don't want to miss the data.

drivers/iio/light/rohm-bu27034.c line 1047

Quoted 3 lines

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

+		 * 2) Data readings more frequent than the meas_time will return
+		 * the same cached values. This should not be a problem for the
+		 * very same reason 1) is not a problem.

Hmm. I'm never that keen on drivers doing that if we can avoid it but perhaps we can't here.

drivers/iio/light/rohm-bu27034.c line 1111

Quoted 4 lines

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

+	/*
+	 * Delay to allow IC to initialize. We don't care if we delay
+	 * for more than 1 ms so msleep() is Ok. We just don't want to
+	 * block

The msleep bit is kind of obvious for a reset. I'd not bother documenting that detail.

drivers/iio/light/rohm-bu27034.c line 1117

Quoted 7 lines

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

+	 */
+	msleep(1);
+
+	/*
+	 * Consider disabling the measurement (and powering off the sensor) for
+	 * runtime pm

Notes like this probably want to go away once the driver is 'finished'.

Quoted 20 lines

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

+	 */
+	ret = bu27034_meas_en(data);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(data->dev, bu27034_meas_stop, data);
+}
+
+static int bu27034_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct fwnode_handle *fwnode;
+	struct bu27034_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	unsigned int part_id;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);

drivers/iio/light/rohm-bu27034.c line 1141

Quoted 5 lines

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

+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	fwnode = dev_fwnode(dev);

why do we care? So far this should work fine with the other types of i2c probe.

drivers/iio/light/rohm-bu27034.c line 1149

Quoted 5 lines

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

+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	ret = devm_regulator_get_enable_optional(dev, "vdd");

vdd isn't optional - or at least it would be an unusual device that doesn't need that supply line.

Key here is that optional in DT is different from this call.
If not present in DT and devm_regulator_get_enable() called then we'll normally get a stub regulator.

Quoted 11 lines

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

+	if (ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	data = iio_priv(idev);
+
+	ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	part_id &= BU27034_MASK_PART_ID;
+

drivers/iio/light/rohm-bu27034.c line 1162

Quoted 2 lines

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

+	if (part_id != BU27034_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", part_id);

Fallback compatibles require that on a failure to match ID we still let the driver carry on. However we can print something in the log to say we don't recognise the device. The intent is that at future part can be supported by old kernels just be having the dt list multiple compatibles if the device really are backwards compatible with parts already supported.

drivers/iio/light/rohm-bu27034.c line 1178

Quoted 4 lines

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

+
+	idev->channels = bu27034_channels;
+	idev->num_channels = ARRAY_SIZE(bu27034_channels);
+	idev->name = "bu27034-als";

If the chip doesn't have a multiple functions (and multiple iio_devs), we'd normally not bother with the als part in the name. Add a comment if there is a reason for it here.

Quoted 6 lines

Quoted 5 lines

+ * need to modify the calculation but I hope this gives a starting point for
+ * those working with such devices.

Quoted 3 lines

That will need some dt bindings - though for now I guess we have no idea what they would be unless there are some hints on the datasheet?

Yes. That is the problem. And even though we would hope we get the complete bindings from day 1 - I don't see it really a problem to add things like the 'lens-whateveritwillbe' when needed. What should be ensured then is the "property not found" case will default to the open-air.

The one thing we could add is sysfs attribute stating the 'open-air' for those who use the raw-values as they will be impacted by lens and won't be compensated by the driver like preprocessed values should be. Not sure if we want to do it yet though as I don't know if there will be any use for it in upstream.

Quoted 11 lines

Quoted 11 lines

+ *
+ * The first case (D1/D0 < 0.87) can be computed to a form:
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 + 0.00012213 * D1 * D1 / D0
+ */

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

+static int bu27034_get_lux(struct bu27034_data *data, int *val)
+{
+	unsigned int gain0, gain1, meastime;
+	unsigned int d1_d0_ratio_scaled;
+	u16 res[3], ch0, ch1;
+	u64 helper64;

Quoted 8 lines

drivers/iio/light/rohm-bu27034.c line 898

Quoted 4 lines

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

+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = bu27034_get_result_unlocked(data, &res[0]);  

res
as it is expecting a point to an array so that is more natural than pointing to the first element even if that's the same result.

This is pretty much the only thing I disagree with you :) For me it has always been much clearer to use pointer to first element - as the type of first element is what we are using. Type of an array (in my head) is something less well defined. I think this difference is best visible with the sizeof(arr) Vs. sizeof(&arr[0]).