Support ROHM BU27034 ALS sensor
This series adds support for ROHM BU27034 Ambient Light Sensor.
The BU27034 has configurable gain and measurement (integration) time
settings. Both of these have direct, inversely proportional relation to
the sensor's intensity channel scale.
Many users only set the scale, which means that many drivers attempt to
'guess' the best gain+time combination to meet the scale. Usually this
is the biggest integration time which allows setting the requested
scale. Typically, increasing the integration time has better accuracy
than increasing the gain, which often amplifies the noise as well as the
real signal.
However, there may be cases where more responsive sensors are needed.
So, in some cases the longest integration times may not be what the user
prefers. The driver has no way of knowing this.
Hence, the approach taken by this series is to allow user to set both
the scale and the integration time with following logic:
1. When scale is set, the existing integration time is tried to be
maintained as a first priority.
1a) If the requested scale can't be met by current time, then also
other time + gain combinations are searched. If scale can be met
by some other integration time, then the new time may be applied.
If the time setting is common for all channels, then also other
channels must be able to maintain their scale with this new time
(by changing their gain). The new times are scanned in the order
of preference (typically the longest times first).
1b) If the requested scale can be met using current time, then only
the gain for the channel is changed.
2. When the integration time change - scale is maintained.
When integration time change is requested also gain for all impacted
channels is adjusted so that the scale is not changed. If gain can't
be changed for some channel, then the request is rejected.
I think this fits the existing 'modes' where scale setting 'guesses' the
best scale + integration time config - and integration time setting does
not change the scale.
This logic is really simple. When total gain (either caused by time or
hw-gain) is doubled, the scale gets halved. Also, the supported times
are given a 'multiplier' value which tells how much they increase the
total gain. However, when I wrote this logic in bu27034 driver, I made
quite a few errors on the way - and driver got pretty big. As I am
writing drivers for two other sensors (RGB C/IR + flicker BU27010 and RGB
C/IR BU27008) with similar gain-time-scale logic I thought that adding
common helpers for these computations might be wise. I hope this way all
the bugs will be concentrated in one place and not in every individual
driver ;) Hence, this RFC also intriduces IIO gain-time-scale helpers +
couple of KUnit tests for the most hairy parts.
I can't help thinking that there should've been simpler way of computing
the gain-time-scale conversions. Also, pretty good speed improvements
might be available if some of the do_div()s could be replaced by >>.
This, however, is not a priority for my light-sensor use-case where
speed demands are not that big. I am open to all improvements and
suggestions though!
What is still missing is advertising the available scales / integration
times. The list of available integration times is not static but depend
on channel gain configurations. Hence, I wonder if there is a way to
not only advertise available integration times with current gain
configuration - but also the available scales with different gains?
Finally, this patch series is an RFC becasue the helper logic could
benefit from extra pairs of eyes - and because the sensor has been
only very limitedly tested this far.
Matti Vaittinen (6):
dt-bindings: iio: light: Support ROHM BU27034
iio: light: Add gain-time-scale helpers
iio: test: test gain-time-scale helpers
MAINTAINERS: Add IIO gain-time-scale helpers
iio: light: ROHM BU27034 Ambient Light Sensor
MAINTAINERS: Add ROHM BU27034
.../bindings/iio/light/rohm-bu27034.yaml | 46 +
MAINTAINERS | 13 +
drivers/iio/light/Kconfig | 16 +
drivers/iio/light/Makefile | 2 +
drivers/iio/light/gain-time-scale-helper.c | 446 ++++++
drivers/iio/light/gain-time-scale-helper.h | 111 ++
drivers/iio/light/rohm-bu27034.c | 1212 +++++++++++++++++
drivers/iio/test/Kconfig | 15 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-gts.c | 331 +++++
10 files changed, 2193 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
create mode 100644 drivers/iio/light/gain-time-scale-helper.c
create mode 100644 drivers/iio/light/gain-time-scale-helper.h
create mode 100644 drivers/iio/light/rohm-bu27034.c
create mode 100644 drivers/iio/test/iio-test-gts.c
base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes
capable of detecting a very wide range of illuminance. Typical application
is adjusting LCD and backlight power of TVs and mobile phones.
Add initial dt-bindings.
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
diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
new file mode 100644
index 000000000000..a3a642c259e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm-bu27034.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27034 ambient light sensor
+
+maintainers:
+ - Matti Vaittinen <[email protected]>
+
+description: |
+ ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes
+ capable of detecting a very wide range of illuminance. Typical application
+ is adjusting LCD and backlight power of TVs and mobile phones.
+ https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf
+
+properties:
+ compatible:
+ const: rohm,bu27034
+
+ reg:
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light-sensor@38 {
+ compatible = "rohm,bu27034";
+ reg = <0x38>;
+ vdd-supply = <&vdd>;
+ };
+ };
+
+...
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Some light sensors can adjust both the HW-gain and integration time.
There are cases where adjusting the integration time has similar impact
to the scale of the reported values as gain setting has.
IIO users do typically expect to handle scale by a single writable 'scale'
entry. Driver should then adjust the gain/time accordingly.
It however is difficult for a driver to know whether it should change
gain or integration time to meet the requested scale. Usually it is
preferred to have longer integration time which usually improves
accuracy, but there may be use-cases where long measurement times can be
an issue. Thus it can be preferable to allow also changing the
integration time - but mitigate the scale impact by also changing the gain
underneath. Eg, if integration time change doubles the measured values,
the driver can reduce the HW-gain to half.
The theory of the computations of gain-time-scale is simple. However,
some people (undersigned) got that implemented wrong for more than once.
Add some gain-time-scale helpers in order to not dublicate errors in all
drivers needing these computations.
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
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..671d84f98c56 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -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
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 6f23817fae6f..f4705fac7a96 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -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
diff --git a/drivers/iio/light/gain-time-scale-helper.c b/drivers/iio/light/gain-time-scale-helper.c
new file mode 100644
index 000000000000..bd8fc11802ee
--- /dev/null
+++ b/drivers/iio/light/gain-time-scale-helper.c
@@ -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]>
+ */
+
+#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)
+{
+ int tmp = 1;
+
+ if (scale > max || !scale)
+ return -EINVAL;
+
+ if (0xffffffffffffffffLLU - max < scale) {
+ /* Risk of overflow */
+ if (max - scale < scale)
+ return 1;
+
+ while (max - scale > scale * (u64) tmp)
+ tmp++;
+
+ return tmp + 1;
+ }
+
+ while (max > scale * (u64) tmp)
+ tmp++;
+
+ return tmp;
+}
+
+static int gain_get_scale_fraction(const u64 max, u64 scale, int known,
+ int *unknown)
+{
+ int tot_gain;
+
+ if (!known)
+ return -EINVAL;
+
+ tot_gain = iio_gts_get_gain(max, scale);
+ if (tot_gain < 0)
+ return tot_gain;
+
+ *unknown = tot_gain/known;
+
+ /* We require total gain to be exact multiple of known * unknown */
+ if (!*unknown || *unknown * known != tot_gain)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct iio_itime_sel_mul *
+ iio_gts_find_itime_by_time(struct iio_gts *gts, int time)
+{
+ int i;
+
+ if (!gts->num_itime)
+ return NULL;
+
+ for (i = 0; i < gts->num_itime; i++)
+ if (gts->itime_table[i].time_us == time)
+ return >s->itime_table[i];
+
+ return NULL;
+}
+
+static const struct iio_itime_sel_mul *
+ iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel)
+{
+ int i;
+
+ if (!gts->num_itime)
+ return NULL;
+
+ for (i = 0; i < gts->num_itime; i++)
+ if (gts->itime_table[i].sel == sel)
+ return >s->itime_table[i];
+
+ return NULL;
+}
+
+static int iio_gts_delinearize(u64 lin_scale, int *scale_whole, int *scale_nano,
+ unsigned long scaler)
+{
+ int frac;
+
+ if (scaler > NANO || !scaler)
+ return -EINVAL;
+
+ frac = do_div(lin_scale, scaler);
+
+ *scale_whole = lin_scale;
+ *scale_nano = frac * (NANO / scaler);
+
+ return 0;
+}
+
+static int iio_gts_linearize(int scale_whole, int scale_nano, u64 *lin_scale,
+ unsigned long scaler)
+{
+ /*
+ * 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
+ * @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. The search functions like the
+ * iio_gts_find_time_and_gain_sel_for_scale() start search from first
+ * provided time.
+ *
+ * Return: 0 on success.
+ */
+int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
+ const struct iio_gain_sel_pair *gain_tbl, int num_gain,
+ const struct iio_itime_sel_mul *tim_tbl, int num_times,
+ struct iio_gts *gts)
+{
+ int ret;
+
+ ret = iio_gts_linearize(max_scale_int, max_scale_nano,
+ >s->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;
+
+ 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);
+}
+EXPORT_SYMBOL_GPL(iio_gts_valid_time);
+
+int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
+{
+ int i;
+
+ for (i = 0; i < gts->num_hwgain; i++)
+ if (gts->hwgain_table[i].gain == gain)
+ return gts->hwgain_table[i].sel;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
+
+bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
+{
+ return !(iio_gts_find_sel_by_gain(gts, gain) < 0);
+}
+EXPORT_SYMBOL_GPL(iio_gts_valid_gain);
+
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
+{
+ int i;
+
+ for (i = 0; i < gts->num_hwgain; i++)
+ if (gts->hwgain_table[i].sel == sel)
+ return gts->hwgain_table[i].gain;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
+
+int iio_gts_get_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))
+ 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");
diff --git a/drivers/iio/light/gain-time-scale-helper.h b/drivers/iio/light/gain-time-scale-helper.h
new file mode 100644
index 000000000000..70a952a8de92
--- /dev/null
+++ b/drivers/iio/light/gain-time-scale-helper.h
@@ -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;
+};
+
+#define GAIN_SCALE_GAIN(_gain, _sel) \
+{ \
+ .gain = (_gain), \
+ .sel = (_sel), \
+}
+
+#define GAIN_SCALE_ITIME_MS(_itime, _sel, _mul) \
+{ \
+ .time_us = (_itime) * 1000, \
+ .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
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Some light sensors can adjust both the HW-gain and integration time.
There are cases where adjusting the integration time has similar impact
to the scale of the reported values as gain setting has.
IIO users do typically expect to handle scale by a single writable 'scale'
entry. Driver should then adjust the gain/time accordingly.
It however is difficult for a driver to know whether it should change
gain or integration time to meet the requested scale. Usually it is
preferred to have longer integration time which usually improves
accuracy, but there may be use-cases where long measurement times can be
an issue. Thus it can be preferable to allow also changing the
integration time - but mitigate the scale impact by also changing the gain
underneath. Eg, if integration time change doubles the measured values,
the driver can reduce the HW-gain to half.
The theory of the computations of gain-time-scale is simple. However,
some people (undersigned) got that implemented wrong for more than once.
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 | 15 ++
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-gts.c | 331 ++++++++++++++++++++++++++++++++
3 files changed, 347 insertions(+)
create mode 100644 drivers/iio/test/iio-test-gts.c
diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
index 0b6e4e278a2f..b57f1fc440e6 100644
--- a/drivers/iio/test/Kconfig
+++ b/drivers/iio/test/Kconfig
@@ -4,6 +4,19 @@
#
# 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
+ default KUNIT_ALL_TESTS
+ help
+ build unit tests for the IIO light sensor gain-time-scale helpers.
+
+ For more information on KUnit and unit tests in general, please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N. Keep in alphabetical order
+
config IIO_RESCALE_KUNIT_TEST
tristate "Test IIO rescale conversion functions" if !KUNIT_ALL_TESTS
depends on KUNIT && IIO_RESCALE
@@ -27,3 +40,5 @@ config IIO_FORMAT_KUNIT_TEST
to the KUnit documentation in Documentation/dev-tools/kunit/.
If unsure, say N.
+
+
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index d76eaf36da82..e9a4cf1ff57f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -6,4 +6,5 @@
# Keep in alphabetical order
obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o
obj-$(CONFIG_IIO_FORMAT_KUNIT_TEST) += iio-test-format.o
+obj-$(CONFIG_IIO_GTS_KUNIT_TEST) += iio-test-gts.o
CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/iio/test/iio-test-gts.c b/drivers/iio/test/iio-test-gts.c
new file mode 100644
index 000000000000..0096dd19e009
--- /dev/null
+++ b/drivers/iio/test/iio-test-gts.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Unit tests for IIO light sensor gain-time-scale helpers
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>
+ */
+
+#include <kunit/test.h>
+#include "../light/gain-time-scale-helper.h"
+
+/*
+ * Please, read the "rant" from the top of the lib/test_linear_ranges.c if
+ * you see a line of helper code which is not being tested.
+ *
+ * Then, please look at the line which is not being tested. Is this line
+ * somehow unusually complex? If answer is "no", then chances are that the
+ * "development inertia" caused by adding a test exceeds the benefits.
+ *
+ * If yes, then adding a test is probably a good idea but please stop for a
+ * moment and consider the effort of changing all the tests when code gets
+ * refactored. Eventually it neeeds to be.
+ */
+
+#define TEST_TSEL_50 1
+#define TEST_TSEL_X_MIN TEST_TSEL_50
+#define TEST_TSEL_100 0
+#define TEST_TSEL_200 2
+#define TEST_TSEL_400 4
+#define TEST_TSEL_X_MAX TEST_TSEL_400
+
+#define TEST_GSEL_1 0x00
+#define TEST_GSEL_X_MIN TEST_GSEL_1
+#define TEST_GSEL_4 0x08
+#define TEST_GSEL_16 0x0a
+#define TEST_GSEL_32 0x0b
+#define TEST_GSEL_64 0x0c
+#define TEST_GSEL_256 0x18
+#define TEST_GSEL_512 0x19
+#define TEST_GSEL_1024 0x1a
+#define TEST_GSEL_2048 0x1b
+#define TEST_GSEL_4096 0x1c
+#define TEST_GSEL_X_MAX TEST_GSEL_4096
+
+#define TEST_SCALE_1X 64
+#define TEST_SCALE_MIN_X TEST_SCALE_1X
+#define TEST_SCALE_2X 32
+#define TEST_SCALE_4X 16
+#define TEST_SCALE_8X 8
+#define TEST_SCALE_16X 4
+#define TEST_SCALE_32X 2
+#define TEST_SCALE_64X 1
+
+#define TEST_SCALE_NANO_128X 500000000
+#define TEST_SCALE_NANO_256X 250000000
+#define TEST_SCALE_NANO_512X 125000000
+#define TEST_SCALE_NANO_1024X 62500000
+#define TEST_SCALE_NANO_2048X 31250000
+#define TEST_SCALE_NANO_4096X 15625000
+#define TEST_SCALE_NANO_4096X2 7812500
+#define TEST_SCALE_NANO_4096X4 3906250
+#define TEST_SCALE_NANO_4096X8 1953125
+
+#define TEST_SCALE_NANO_MAX_X TEST_SCALE_NANO_4096X8
+
+static const struct iio_gain_sel_pair gts_test_gains[] = {
+ GAIN_SCALE_GAIN(1, TEST_GSEL_1),
+ GAIN_SCALE_GAIN(4, TEST_GSEL_4),
+ GAIN_SCALE_GAIN(16, TEST_GSEL_16),
+ GAIN_SCALE_GAIN(32, TEST_GSEL_32),
+ GAIN_SCALE_GAIN(64, TEST_GSEL_64),
+ GAIN_SCALE_GAIN(256, TEST_GSEL_256),
+ GAIN_SCALE_GAIN(512, TEST_GSEL_512),
+ GAIN_SCALE_GAIN(1024, TEST_GSEL_1024),
+ GAIN_SCALE_GAIN(2048, TEST_GSEL_2048),
+ GAIN_SCALE_GAIN(4096, TEST_GSEL_4096),
+#define HWGAIN_MAX 4096
+};
+
+static const struct iio_itime_sel_mul gts_test_itimes[] = {
+ GAIN_SCALE_ITIME_MS(400, TEST_TSEL_400, 8),
+ GAIN_SCALE_ITIME_MS(200, TEST_TSEL_200, 4),
+ GAIN_SCALE_ITIME_MS(100, TEST_TSEL_100, 2),
+ GAIN_SCALE_ITIME_MS(50, TEST_TSEL_50, 1),
+#define TIMEGAIN_MAX 8
+};
+#define TOTAL_GAIN_MAX (HWGAIN_MAX * TIMEGAIN_MAX)
+
+static int test_init_iio_gain_scale(struct iio_gts *gts, int max_scale_int,
+ int max_scale_nano)
+{
+ int ret;
+
+ ret = iio_init_iio_gts(max_scale_int, max_scale_nano, gts_test_gains,
+ ARRAY_SIZE(gts_test_gains), gts_test_itimes,
+ ARRAY_SIZE(gts_test_itimes), gts);
+
+ return ret;
+}
+
+static void test_iio_gts_find_gain_for_scale_using_time(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, gain;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_100,
+ TEST_SCALE_8X, 0, &gain);
+ /*
+ * Meas time 100 => gain by time 2x
+ * TEST_SCALE_8X matches total gain 8x
+ * => required HWGAIN 4x
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 4, gain);
+
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_200, 0,
+ TEST_SCALE_NANO_256X, &gain);
+ /*
+ * Meas time 200 => gain by time 4x
+ * TEST_SCALE_256X matches total gain 256x
+ * => required HWGAIN 256/4 => 64x
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 64, gain);
+
+ /* Min time, Min gain */
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_X_MIN,
+ TEST_SCALE_MIN_X, 0, &gain);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 1, gain);
+
+ /* Max time, Max gain */
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_X_MAX, 0,
+ TEST_SCALE_NANO_MAX_X, &gain);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 4096, gain);
+
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_100, 0,
+ TEST_SCALE_NANO_256X, &gain);
+ /*
+ * Meas time 100 => gain by time 2x
+ * TEST_SCALE_256X matches total gain 256x
+ * => required HWGAIN 256/2 => 128x (not in gain-table - unsupported)
+ */
+ KUNIT_EXPECT_NE(test, 0, ret);
+
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_200, 0,
+ TEST_SCALE_NANO_MAX_X, &gain);
+ /* We can't reach the max gain with integration time smaller than MAX */
+ KUNIT_EXPECT_NE(test, 0, ret);
+
+ ret = iio_gts_find_gain_for_scale_using_time(>s, TEST_TSEL_50, 0,
+ TEST_SCALE_NANO_MAX_X, &gain);
+ /* We can't reach the max gain with integration time smaller than MAX */
+ KUNIT_EXPECT_NE(test, 0, ret);
+}
+
+static void test_iio_gts_find_time_and_gain_sel_for_scale(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, gain_sel, time_sel;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_gts_find_time_and_gain_sel_for_scale(>s, 0,
+ TEST_SCALE_NANO_256X, &gain_sel, &time_sel);
+ /*
+ * We should find time 400 (8x) and gain 256/8 => 32x because the
+ * time 400 is listed first
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_GSEL_32, gain_sel);
+ KUNIT_EXPECT_EQ(test, TEST_TSEL_400, time_sel);
+
+ ret = iio_gts_find_time_and_gain_sel_for_scale(>s, TEST_SCALE_64X,
+ 0, &gain_sel, &time_sel);
+ /*
+ * We should find time 200 (4x) and gain 64/4 => 16x. The most
+ * preferred time 400 (8x) would require gain 8x - which is not
+ * "supported".
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_GSEL_16, gain_sel);
+ KUNIT_EXPECT_EQ(test, TEST_TSEL_200, time_sel);
+
+ /* Min gain */
+ ret = iio_gts_find_time_and_gain_sel_for_scale(>s, TEST_SCALE_MIN_X,
+ 0, &gain_sel, &time_sel);
+ /*
+ * We should find time 400 (8x) and gain 256/8 => 32x because the
+ * time 400 is listed first
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_GSEL_1, gain_sel);
+ KUNIT_EXPECT_EQ(test, TEST_TSEL_50, time_sel);
+
+ /* Max gain */
+ ret = iio_gts_find_time_and_gain_sel_for_scale(>s, 0,
+ TEST_SCALE_NANO_MAX_X, &gain_sel, &time_sel);
+ /*
+ * We should find time 400 (8x) and gain 256/8 => 32x because the
+ * time 400 is listed first
+ */
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_GSEL_X_MAX, gain_sel);
+ KUNIT_EXPECT_EQ(test, TEST_TSEL_X_MAX, time_sel);
+}
+
+static void test_iio_gts_get_total_gain_by_sel(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, gain_sel, time_sel;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ /* gain x32, time x4 => total gain 32 * 4 = 128 */
+ gain_sel = TEST_GSEL_32;
+ time_sel = TEST_TSEL_200;
+
+ ret = iio_gts_get_total_gain_by_sel(>s, gain_sel, time_sel);
+ /* gain x32, time x8 => total gain 32 * 4 = 128 */
+ KUNIT_EXPECT_EQ(test, 128, ret);
+
+ gain_sel = TEST_GSEL_X_MAX;
+ time_sel = TEST_TSEL_X_MAX;
+ ret = iio_gts_get_total_gain_by_sel(>s, gain_sel, time_sel);
+ KUNIT_EXPECT_EQ(test, TOTAL_GAIN_MAX, ret);
+
+ gain_sel = TEST_GSEL_X_MIN;
+ time_sel = TEST_TSEL_X_MIN;
+ ret = iio_gts_get_total_gain_by_sel(>s, gain_sel, time_sel);
+ KUNIT_EXPECT_EQ(test, 1, ret);
+}
+
+static void test_iio_gts_find_new_gain_sel_by_old_gain_time(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, old_gain, new_gain, old_time_sel, new_time_sel;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ old_gain = 32;
+ old_time_sel = TEST_TSEL_200;
+ new_time_sel = TEST_TSEL_400;
+
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ /*
+ * Doubling the integration time doubles the total gain - so old
+ * (hw)gain must be divided by two to compensate. => 32 / 2 => 16
+ */
+ KUNIT_EXPECT_EQ(test, 16, new_gain);
+
+ old_gain = 4;
+ old_time_sel = TEST_TSEL_50;
+ new_time_sel = TEST_TSEL_200;
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ /*
+ * gain by time 1x => 4x - (hw)gain 4x => 1x
+ */
+ KUNIT_EXPECT_EQ(test, 1, new_gain);
+
+ old_gain = 512;
+ old_time_sel = TEST_TSEL_400;
+ new_time_sel = TEST_TSEL_50;
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ /*
+ * gain by time 8x => 1x - (hw)gain 512x => 4096x)
+ */
+ KUNIT_EXPECT_EQ(test, 4096, new_gain);
+
+ /* Unsupported gain 2x */
+ old_gain = 4;
+ old_time_sel = TEST_TSEL_200;
+ new_time_sel = TEST_TSEL_400;
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_NE(test, 0, ret);
+
+ /* Too small gain */
+ old_gain = 4;
+ old_time_sel = TEST_TSEL_50;
+ new_time_sel = TEST_TSEL_400;
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_NE(test, 0, ret);
+
+ /* Too big gain */
+ old_gain = 1024;
+ old_time_sel = TEST_TSEL_400;
+ new_time_sel = TEST_TSEL_50;
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(>s, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ KUNIT_EXPECT_NE(test, 0, ret);
+}
+
+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_time_and_gain_sel_for_scale),
+ KUNIT_CASE(test_iio_gts_get_total_gain_by_sel),
+ KUNIT_CASE(test_iio_gts_find_new_gain_sel_by_old_gain_time),
+ {}
+};
+
+static struct kunit_suite iio_gts_test_suite = {
+ .name = "iio-gain-time-scale",
+ .test_cases = iio_gts_test_cases,
+};
+
+kunit_test_suite(iio_gts_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Add myself as a maintainer for IIO light sensor helpers (helpers for
maintaining the scale while adjusting intergration time or gain) and
related Kunit tests.
Signed-off-by: Matti Vaittinen <[email protected]>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 42fc47c6edfd..43f5a024daa2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10052,6 +10052,14 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
F: Documentation/devicetree/bindings/iio/adc/envelope-detector.yaml
F: drivers/iio/adc/envelope-detector.c
+IIO LIGHT SENSOR GAIN-TIME-SCALE HELPERS
+M: Matti Vaittinen <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/light/gain-time-scale-helper.c
+F: drivers/iio/light/gain-time-scale-helper.h
+F: drivers/iio/test/iio-test-gts.c
+
IIO MULTIPLEXER
M: Peter Rosin <[email protected]>
L: [email protected]
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
ROHM BU27034 is an ambient light 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
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 671d84f98c56..594228bd1f7f 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -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"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_GTS_HELPER
+ help
+ Enable support for the ROHM BU27034 ambient light sensor.
+ ROHM BU27034 is an ambient light sesnor with 3 channels and
+ 3 photo diodes capable of detecting a very wide range of
+ illuminance.
+ Typical application is adjusting LCD and backlight power
+ of TVs and mobile phones.
+
config RPR0521
tristate "ROHM RPR0521 ALS and proximity sensor driver"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index f4705fac7a96..d34a0f7bf6ce 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_MAX44009) += max44009.o
obj-$(CONFIG_NOA1305) += noa1305.o
obj-$(CONFIG_OPT3001) += opt3001.o
obj-$(CONFIG_PA12203001) += pa12203001.o
+obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
obj-$(CONFIG_RPR0521) += rpr0521.o
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
obj-$(CONFIG_SI1133) += si1133.o
diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
new file mode 100644
index 000000000000..235be7dee6e0
--- /dev/null
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -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>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include "gain-time-scale-helper.h"
+
+#define BU27034_REG_SYSTEM_CONTROL 0x40
+#define BU27034_MASK_SW_RESET BIT(7)
+#define BU27034_MASK_PART_ID GENMASK(5, 0)
+#define BU27034_ID 0x19
+#define BU27034_REG_MODE_CONTROL1 0x41
+#define BU27034_MASK_MEAS_MODE GENMASK(2, 0)
+
+#define BU27034_REG_MODE_CONTROL2 0x42
+#define BU27034_MASK_D01_GAIN GENMASK(7, 3)
+#define BU27034_SHIFT_D01_GAIN 3
+#define BU27034_MASK_D2_GAIN_HI GENMASK(7, 6)
+#define BU27034_MASK_D2_GAIN_LO GENMASK(2, 0)
+#define BU27034_SHIFT_D2_GAIN 3
+
+#define BU27034_REG_MODE_CONTROL3 0x43
+#define BU27034_REG_MODE_CONTROL4 0x44
+#define BU27034_MASK_MEAS_EN BIT(0)
+#define BU27034_MASK_VALID BIT(7)
+#define BU27034_REG_DATA0_LO 0x50
+#define BU27034_REG_DATA1_LO 0x52
+#define BU27034_REG_DATA2_LO 0x54
+#define BU27034_REG_DATA2_HI 0x55
+#define BU27034_REG_MANUFACTURER_ID 0x92
+#define BU27034_REG_MAX BU27034_REG_MANUFACTURER_ID
+
+enum {
+ BU27034_CHAN_ALS,
+ BU27034_CHAN_DATA0,
+ BU27034_CHAN_DATA1,
+ BU27034_CHAN_DATA2,
+ BU27034_NUM_CHANS
+};
+
+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X 64
+
+#define BU27034_GSEL_1X 0x00
+#define BU27034_GSEL_4X 0x08
+#define BU27034_GSEL_16X 0x0a
+#define BU27034_GSEL_32X 0x0b
+#define BU27034_GSEL_64X 0x0c
+#define BU27034_GSEL_256X 0x18
+#define BU27034_GSEL_512X 0x19
+#define BU27034_GSEL_1024X 0x1a
+#define BU27034_GSEL_2048X 0x1b
+#define BU27034_GSEL_4096X 0x1c
+
+/* Available gain settings */
+static const struct iio_gain_sel_pair bu27034_gains[] = {
+ GAIN_SCALE_GAIN(1, BU27034_GSEL_1X),
+ GAIN_SCALE_GAIN(4, BU27034_GSEL_4X),
+ GAIN_SCALE_GAIN(16, BU27034_GSEL_16X),
+ GAIN_SCALE_GAIN(32, BU27034_GSEL_32X),
+ GAIN_SCALE_GAIN(64, BU27034_GSEL_64X),
+ GAIN_SCALE_GAIN(256, BU27034_GSEL_256X),
+ GAIN_SCALE_GAIN(512, BU27034_GSEL_512X),
+ GAIN_SCALE_GAIN(1024, BU27034_GSEL_1024X),
+ GAIN_SCALE_GAIN(2048, BU27034_GSEL_2048X),
+ GAIN_SCALE_GAIN(4096, BU27034_GSEL_4096X),
+};
+
+/*
+ * The IC has 5 modes for sampling time. 5 mS mode is exceptional as it limits
+ * the data collection to data0-channel only and cuts the supported range to
+ * 10 bit. It is not 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), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .address = BU27034_REG_##_name##_LO, \
+ .scan_index = BU27034_CHAN_##_name, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
+ .indexed = 1 \
+}
+
+static const struct iio_chan_spec bu27034_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .channel = BU27034_CHAN_ALS,
+ },
+ BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
+ BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
+ BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+};
+
+struct bu27034_data {
+ struct regmap *regmap;
+ struct device *dev;
+ /*
+ * Protect gain and time during scale adjustment and data reading as
+ * well as the channel data 'cached' flag.
+ */
+ struct mutex mutex;
+ struct iio_gts gts;
+ bool cached;
+ __le16 raw[3];
+};
+
+struct bu27034_result {
+ u16 ch0;
+ u16 ch1;
+ u16 ch2;
+};
+
+static const struct regmap_range bu27034_volatile_ranges[] = {
+ {
+ .range_min = BU27034_REG_MODE_CONTROL4,
+ .range_max = BU27034_REG_MODE_CONTROL4,
+ }, {
+ .range_min = BU27034_REG_DATA0_LO,
+ .range_max = BU27034_REG_DATA2_HI,
+ },
+};
+
+static const struct regmap_access_table bu27034_volatile_regs = {
+ .yes_ranges = &bu27034_volatile_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(bu27034_volatile_ranges),
+};
+
+static const struct regmap_range bu27034_read_only_ranges[] = {
+ {
+ .range_min = BU27034_REG_DATA0_LO,
+ .range_max = BU27034_REG_DATA2_HI,
+ }, {
+ .range_min = BU27034_REG_MANUFACTURER_ID,
+ .range_max = BU27034_REG_MANUFACTURER_ID,
+ }
+};
+
+static const struct regmap_access_table bu27034_ro_regs = {
+ .no_ranges = &bu27034_read_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(bu27034_read_only_ranges),
+};
+
+static const struct regmap_config bu27034_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = BU27034_REG_MAX,
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_table = &bu27034_volatile_regs,
+};
+
+static int bu27034_validate_int_time(struct bu27034_data *data, int time_us)
+{
+ /*
+ * The BU27034 has 55 mS integration time which is in the vendor tests
+ * handled as 50 mS in all of the internal computations. We keep same
+ * approach here.
+ */
+ if (time_us == 55000)
+ return 50000;
+
+ if (iio_gts_valid_time(&data->gts, time_us))
+ return time_us;
+
+ return -EINVAL;
+}
+
+struct bu27034_gain_check {
+ int old_gain;
+ int new_gain;
+ int chan;
+};
+
+static int bu27034_get_gain_sel(struct bu27034_data *data, int chan)
+{
+ int ret, val;
+
+ switch (chan) {
+ case BU27034_CHAN_DATA0:
+ case BU27034_CHAN_DATA1:
+ {
+ int reg[] = {
+ [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+ [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+ };
+ ret = regmap_read(data->regmap, reg[chan], &val);
+ if (ret)
+ return ret;
+
+ val &= BU27034_MASK_D01_GAIN;
+ return val >> BU27034_SHIFT_D01_GAIN;
+ }
+ case BU27034_CHAN_DATA2:
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val);
+ if (ret)
+ return ret;
+
+ return (val & BU27034_MASK_D2_GAIN_HI) >> BU27034_SHIFT_D2_GAIN
+ | (val & BU27034_MASK_D2_GAIN_LO);
+ }
+
+ dev_err(data->dev, "Can't get gain for channel %d\n", chan);
+
+ return -EINVAL;
+}
+
+static int bu27034_get_gain(struct bu27034_data *data, int chan, int *gain)
+{
+ int ret, sel;
+
+ ret = bu27034_get_gain_sel(data, chan);
+ if (ret < 0)
+ return ret;
+
+ sel = ret;
+
+ ret = iio_gts_find_gain_by_sel(&data->gts, sel);
+ if (ret < 0)
+ dev_err(data->dev, "chan %u: unknown gain value 0x%x\n", chan,
+ sel);
+
+ *gain = ret;
+
+ return 0;
+}
+
+static int bu27034_get_int_time(struct bu27034_data *data)
+{
+ int ret, sel;
+
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel);
+ if (ret)
+ return ret;
+
+ return iio_gts_find_int_time_by_sel(&data->gts,
+ sel & BU27034_MASK_MEAS_MODE);
+}
+
+static int _bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+ int *val2)
+{
+ int gain, ret;
+
+ ret = bu27034_get_gain(data, channel, &gain);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ return iio_gts_get_scale(&data->gts, gain, ret, val, val2);
+}
+
+static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+ int *val2)
+{
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = _bu27034_get_scale(data, channel, val, val2);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+/* Caller should hold the lock to protect data->cached */
+static int bu27034_write_gain_sel(struct bu27034_data *data, int chan, int sel)
+{
+ static const int reg[] = {
+ [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+ [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+ [BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2
+ };
+ int mask;
+
+ if (chan < BU27034_CHAN_DATA0 || chan > BU27034_CHAN_DATA2)
+ return -EINVAL;
+
+ if (chan == BU27034_CHAN_DATA0 || chan == BU27034_CHAN_DATA1) {
+ sel <<= BU27034_SHIFT_D01_GAIN;
+ mask = BU27034_MASK_D01_GAIN;
+ } else {
+ /*
+ * We don't allow setting high bits for DATA2 gain because
+ * that impacts to DATA0 as well.
+ */
+ mask = BU27034_MASK_D2_GAIN_LO;
+ }
+
+ /* We are changing gain so we need to invalidate cached results. */
+ data->cached = false;
+
+ return regmap_update_bits(data->regmap, reg[chan], mask, sel);
+}
+
+static int _bu27034_set_gain(struct bu27034_data *data, int chan, int gain)
+{
+ int ret, sel;
+
+ ret = iio_gts_find_sel_by_gain(&data->gts, gain);
+ if (ret < 0)
+ return ret;
+
+ return bu27034_write_gain_sel(data, chan, sel);
+}
+
+/* Caller should hold the lock to protect data->cached */
+static int bu27034_set_int_time(struct bu27034_data *data, int time)
+{
+ int ret;
+
+ ret = iio_gts_find_sel_by_int_time(&data->gts, time);
+ if (ret < 0)
+ return ret;
+
+ /* We are changing int time so we need to invalidate cached results. */
+ data->cached = false;
+
+ return regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
+ BU27034_MASK_MEAS_MODE, ret);
+}
+
+/*
+ * We try to change the time in such way that the scale is maintained for
+ * given channels by adjusting gain so that it compensates the time change.
+ */
+static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
+{
+ int ret, int_time_old, int_time_new, i;
+ struct bu27034_gain_check gains[3] = {
+ { .chan = BU27034_CHAN_DATA0, },
+ { .chan = BU27034_CHAN_DATA1, },
+ { .chan = BU27034_CHAN_DATA2 }
+ };
+ int numg = ARRAY_SIZE(gains);
+
+ mutex_lock(&data->mutex);
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ goto unlock_out;
+
+ int_time_old = ret;
+
+ ret = bu27034_validate_int_time(data, time_us);
+ if (ret < 0) {
+ dev_err(data->dev, "Unsupported integration time %u\n",
+ time_us);
+
+ goto unlock_out;
+ }
+
+ int_time_new = ret;
+
+ if (int_time_new == int_time_old) {
+ ret = 0;
+ goto unlock_out;
+ }
+
+ for (i = 0; i < numg; i++) {
+ ret = bu27034_get_gain(data, gains[i].chan,
+ &gains[i].old_gain);
+ if (ret)
+ goto unlock_out;
+
+ gains[i].new_gain = gains[i].old_gain * int_time_old /
+ int_time_new;
+
+ if (!iio_gts_valid_gain(&data->gts, gains[i].new_gain)) {
+ int scale1, scale2;
+
+ _bu27034_get_scale(data, gains[i].chan, &scale1, &scale2);
+ dev_err(data->dev,
+ "chan %u, can't support time %u with scale %u %u\n",
+ gains[i].chan, time_us, scale1, scale2);
+
+ ret = -EINVAL;
+ goto unlock_out;
+ }
+ }
+
+ /*
+ * The new integration time can be supported while keeping the scale of
+ * channels intact by tuning the gains.
+ */
+ for (i = 0; i < numg; i++) {
+ ret = _bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
+ if (ret)
+ goto unlock_out;
+ }
+
+ ret = bu27034_set_int_time(data, int_time_new);
+
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27034_set_scale(struct bu27034_data *data, int chan,
+ int val, int val2)
+{
+ int ret, time_sel, gain_sel, i;
+ bool found = false;
+
+ 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;
+ }
+ }
+ if (!found) {
+ dev_err(data->dev,
+ "Can't set scale maintaining other channels\n");
+ ret = -EINVAL;
+
+ goto unlock_out;
+ }
+
+ for (i = 0; i < 2; i++) {
+ ret = _bu27034_set_gain(data, 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;
+}
+
+/*
+ * for (D1/D0 < 0.87):
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 +
+ * 0.00012213 * D1 * D1 / D0
+ *
+ * => 115.7400832 * ch1 / gain1 / mt -
+ * 68.1982976 * ch0 / gain0 / mt +
+ * 0.00012213 * 25600 * (ch1 / gain1 / mt) * 25600 *
+ * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ *
+ * A = 0.00012213 * 25600 * (ch1 /gain1 / mt) * 25600 *
+ * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ * => 0.00012213 * 25600 * (ch1 /gain1 / mt) *
+ * (ch1 /gain1 / mt) / (ch0 / gain0 / mt)
+ * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) /
+ * (ch0 / gain0)
+ * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) *
+ * gain0 / ch0
+ * => 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /ch0
+ *
+ * lx = (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ * mt + A
+ * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ * mt + 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /
+ * ch0
+ *
+ * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0 +
+ * 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) /
+ * mt
+ *
+ * For (0.87 <= D1/D0 < 1.00)
+ * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 0.87) * (0.385) + 1)
+ * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ * 100 * ch1 / gain1 / mt) * ((D1/D0 - 0.87) * (0.385) + 1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * ((D1/D0 - 0.87) * (0.385) + 1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (0.385 * D1/D0 - 0.66505)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (0.385 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) - 0.66505)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (9856 * ch1 / gain1 / mt / (25600 * ch0 / gain0 / mt) + 0.66505)
+ * => 13.118336 * ch1 / (gain1 * mt)
+ * + 22.66064768 * ch0 / (gain0 * mt)
+ * + 8931.90144 * ch1 * ch1 * gain0 /
+ * (25600 * ch0 * gain1 * gain1 * mt)
+ * + 0.602694912 * ch1 / (gain1 * mt)
+ *
+ * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1)
+ * + 22.66064768 * ch0 / gain0
+ * + 13.721030912 * ch1 / gain1
+ * ] / mt
+ *
+ * For (D1/D0 >= 1.00)
+ *
+ * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 2.0) * (-0.05) + 1)
+ * => (0.001331* D0 + 0.0000354 * D1) * (-0.05D1/D0 + 1.1)
+ * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ * 100 * ch1 / gain1 / mt) * (-0.05D1/D0 + 1.1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (-0.05 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) + 1.1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (-1280 * ch1 / (gain1 * mt * 25600 * ch0 / gain0 / mt) + 1.1)
+ * => (34.0736 * ch0 * -1280 * ch1 * gain0 * mt /( gain0 * mt * gain1 * mt * 25600 * ch0)
+ * + 34.0736 * 1.1 * ch0 / (gain0 * mt)
+ * + 0.90624 * ch1 * -1280 * ch1 *gain0 * mt / (gain1 * mt *gain1 * mt * 25600 * ch0)
+ * + 1.1 * 0.90624 * ch1 / (gain1 * mt)
+ * => -43614.208 * ch1 / (gain1 * mt * 25600)
+ * + 37.48096 ch0 / (gain0 * mt)
+ * - 1159.9872 * ch1 * ch1 * gain0 / (gain1 * gain1 * mt * 25600 * ch0)
+ * + 0.996864 ch1 / (gain1 * mt)
+ * => [
+ * - 0.045312 * ch1 * ch1 * gain0 / (gain1 * gain1 * ch0)
+ * - 0.706816 * ch1 / gain1
+ * + 37.48096 ch0 /gain0
+ * ] * mt
+ *
+ *
+ * So, the first case (D1/D0 < 0.87) can be computed to a form:
+ *
+ * lx = (3.126528 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * 115.7400832 * ch1 / gain1 +
+ * -68.1982976 * ch0 / gain0
+ * / mt
+ *
+ * Second case (0.87 <= D1/D0 < 1.00) goes to form:
+ *
+ * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * 13.721030912 * ch1 / gain1 +
+ * 22.66064768 * ch0 / gain0
+ * ] / mt
+ *
+ * Third case (D1/D0 >= 1.00) goes to form:
+ * => [-0.045312 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * -0.706816 * ch1 / gain1 +
+ * 37.48096 ch0 /(gain0
+ * ] / mt
+ *
+ * This can be unified to format:
+ * lx = [
+ * A * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * B * ch1 / gain1 +
+ * C * ch0 / gain0
+ * ] / mt
+ *
+ * For case 1:
+ * A = 3.126528,
+ * B = 115.7400832
+ * C = -68.1982976
+ *
+ * For case 2:
+ * A = 0.3489024
+ * B = 13.721030912
+ * C = 22.66064768
+ *
+ * For case 3:
+ * A = -0.045312
+ * B = -0.706816
+ * C = 37.48096
+ */
+
+struct bu27034_lx_coeff {
+ unsigned int A;
+ unsigned int B;
+ unsigned int C;
+ /* Indicate which of the coefficients above are negative */
+ bool is_neg[3];
+};
+
+static 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;
+
+ helper = gain1 * gain1;
+ if (helper > ch0) {
+ do_div(helper64, helper);
+ /*
+ * multiplication with max gain may overflow
+ * if helper64 is greater than 0xFFFFFFFFFFFFF.
+ *
+ * If this is the case we divide first.
+ */
+ if (helper64 < 0xFFFFFFFFFFFFFLLU) {
+ helper64 *= gain0;
+ do_div(helper64, ch0);
+ } else {
+ do_div(helper64, ch0);
+ helper64 *= gain0;
+ }
+
+ return helper64;
+ }
+
+ do_div(helper64, ch0);
+ /* Same overflow check here */
+ if (helper64 < 0xFFFFFFFFFFFFFLLU) {
+ helper64 *= gain0;
+ do_div(helper64, helper);
+ } else {
+ do_div(helper64, helper);
+ helper64 *= gain0;
+ }
+
+ return helper64;
+}
+
+static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
+ unsigned int gain)
+{
+ unsigned int helper;
+ u64 helper64;
+
+ helper64 = (u64)coeff * (u64)ch;
+ helper = coeff * ch;
+
+ if (helper == helper64)
+ return helper / gain;
+
+ do_div(helper64, gain);
+
+ return helper64;
+}
+
+static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1,
+ unsigned int gain0, unsigned int gain1,
+ unsigned int meastime, int coeff_idx)
+{
+ static const struct bu27034_lx_coeff coeff[] = {
+ {
+ .A = 31265280, /* 3.126528 */
+ .B = 1157400832, /*115.7400832 */
+ .C = 681982976, /* -68.1982976 */
+ .is_neg = {false, false, true},
+ }, {
+ .A = 3489024, /* 0.3489024 */
+ .B = 137210309, /* 13.721030912 */
+ .C = 226606476, /* 22.66064768 */
+ /* All terms positive */
+ }, {
+ .A = 453120, /* -0.045312 */
+ .B = 7068160, /* -0.706816 */
+ .C = 374809600, /* 37.48096 */
+ .is_neg = {true, true, false},
+ }
+ };
+ const struct bu27034_lx_coeff *c = &coeff[coeff_idx];
+ u64 res = 0, terms[3];
+ int i;
+
+ if (coeff_idx >= ARRAY_SIZE(coeff))
+ return -EINVAL;
+
+ terms[0] = bu27034_fixp_calc_t1(c->A, ch0, ch1, gain0, gain1);
+ terms[1] = bu27034_fixp_calc_t23(c->B, ch1, gain1);
+ terms[2] = bu27034_fixp_calc_t23(c->C, ch0, gain0);
+
+ /* First, add positive terms */
+ for (i = 0; i < 3; i++)
+ if (!c->is_neg[i])
+ res += terms[i];
+
+ /* No positive term => zero lux */
+ if (!res)
+ return 0;
+
+ /* Then, subtract negative terms (if any) */
+ for (i = 0; i < 3; i++)
+ if (c->is_neg[i]) {
+ /*
+ * If the negative term is greater than positive - then
+ * the darknes has taken over and we are all doomed! Eh,
+ * I mean, then we can just return 0 lx and go out
+ */
+ if (terms[i] >= res)
+ return 0;
+
+ res -= terms[i];
+ }
+
+ meastime *= 10000;
+ do_div(res, meastime);
+
+ return (int) res;
+}
+
+static bool bu27034_has_valid_sample(struct bu27034_data *data)
+{
+ int ret, val;
+
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL4, &val);
+ if (ret)
+ dev_err(data->dev, "Read failed %d\n", ret);
+
+ return (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;
+}
+
+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
+ *
+ * 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
+ * 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
+ */
+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;
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = bu27034_get_result_unlocked(data, &res[0]);
+ if (ret)
+ goto unlock_out;
+
+ /* Avoid div by zero */
+ if (!res[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;
+}
+
+static int bu27034_meas_set(struct bu27034_data *data, bool en)
+{
+ int ret;
+
+ mutex_lock(&data->mutex);
+ if (en)
+ ret = regmap_set_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+ BU27034_MASK_MEAS_EN);
+ else
+ ret = regmap_clear_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+ BU27034_MASK_MEAS_EN);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27034_meas_en(struct bu27034_data *data)
+{
+ return bu27034_meas_set(data, true);
+}
+
+static int bu27034_meas_dis(struct bu27034_data *data)
+{
+ return bu27034_meas_set(data, false);
+}
+
+static int bu27034_read_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * We use 50000 uS internally for all calculations and only
+ * convert it to 55000 before returning it to the user.
+ *
+ * This is becaise data-sheet says the time is 55 mS - but
+ * vendor provided computations used 50 mS.
+ */
+ if (ret == 50000)
+ ret = 55000;
+
+ *val2 = 0;
+ *val = ret;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+
+ 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:
+ * 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
+ * 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;
+ }
+
+ case IIO_CHAN_INFO_PROCESSED:
+ if (chan->type != IIO_LIGHT)
+ return -EINVAL;
+
+ ret = bu27034_get_lux(data, val);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+
+ }
+
+ return -EINVAL;
+}
+
+static int bu27034_write_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return bu27034_set_scale(data, chan->channel, val, val2);
+ case IIO_CHAN_INFO_INT_TIME:
+ return bu27034_try_set_int_time(data, val);
+ }
+
+ 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;
+
+ /* Reset */
+ ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+ BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+ /*
+ * 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
+ */
+ msleep(1);
+
+ /*
+ * 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);
+}
+
+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);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize Regmap\n");
+
+ fwnode = dev_fwnode(dev);
+ if (!fwnode)
+ return -ENODEV;
+
+ idev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!idev)
+ return -ENOMEM;
+
+ ret = devm_regulator_get_enable_optional(dev, "vdd");
+ 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;
+
+ if (part_id != BU27034_ID) {
+ dev_err(dev, "unsupported device 0x%x\n", part_id);
+ 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);
+ if (ret)
+ return ret;
+
+ mutex_init(&data->mutex);
+ data->regmap = regmap;
+ data->dev = dev;
+
+ idev->channels = bu27034_channels;
+ idev->num_channels = ARRAY_SIZE(bu27034_channels);
+ idev->name = "bu27034-als";
+ idev->info = &bu27034_info;
+
+ idev->modes = INDIO_DIRECT_MODE;
+
+ ret = bu27034_chip_init(data);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_device_register(data->dev, idev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Unable to register iio device\n");
+
+ return ret;
+}
+
+static const struct of_device_id bu27034_of_match[] = {
+ { .compatible = "rohm,bu27034", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bu27034_of_match);
+
+static struct i2c_driver bu27034_i2c_driver = {
+ .driver = {
+ .name = "bu27034-i2c",
+ .of_match_table = bu27034_of_match,
+ },
+ .probe_new = bu27034_probe,
+};
+module_i2c_driver(bu27034_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("ROHM BU27034 ambient light sensor driver");
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Add myself as a maintainer for ROHM BU27034 ALS driver.
Signed-off-by: Matti Vaittinen <[email protected]>
---
MAINTAINERS | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 43f5a024daa2..8d31ef852372 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18090,6 +18090,11 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/light/bh1750.yaml
F: drivers/iio/light/bh1750.c
+ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
+M: Matti Vaittinen <[email protected]>
+S: Supported
+F: drivers/iio/light/rohm-bu27034.c
+
ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
M: Marek Vasut <[email protected]>
L: [email protected]
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On 22/02/2023 17:14, Matti Vaittinen wrote:
> 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.
>
> 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
>
> diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
> new file mode 100644
> index 000000000000..a3a642c259e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
Comma as a separator, so:
rohm,bu27034.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/rohm-bu27034.yaml#
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 :)
On 2/22/23 20:57, Krzysztof Kozlowski wrote:
> On 22/02/2023 17:14, Matti Vaittinen wrote:
>> 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.
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).
>> 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
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
>> new file mode 100644
>> index 000000000000..a3a642c259e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
>
>
> Comma as a separator, so:
> rohm,bu27034.yaml
Oh, yes. So it seems.
Strange, I could have sworn I used hyphen in binding file names
previously although the comma has been used in the compatible. I had to
go back in time (lore,kernel.org) to check my earlier submissions. Well,
my mind seems to be playing tricks on me @_@. I'll fix this before
sending out non RFC series :)
Good catch (as always)! Thanks!
>> @@ -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
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 23/02/2023 07:20, Vaittinen, Matti wrote:
> Hi dee Ho Krzysztof,
>
> Thanks for the review! It's nice you had the time to take a look on RFC :)
>
> On 2/22/23 20:57, Krzysztof Kozlowski wrote:
>> On 22/02/2023 17:14, Matti Vaittinen wrote:
>>> 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.
>
> 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
On 2/23/23 11:26, Krzysztof Kozlowski wrote:
> On 23/02/2023 07:20, Vaittinen, Matti wrote:
>> On 2/22/23 20:57, Krzysztof Kozlowski wrote:
>>> On 22/02/2023 17:14, Matti Vaittinen wrote:
>>>> 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.
>>
>> 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...
Thanks for explanation Krzysztof. I think that what you wrote here makes
sense. Still, I don't think this "adding features only later can cause
problems to others" is in any way fundamentally different for bindings
and software. Sure this clock example is a valid thing, adding a clock
later could cause kernel to suddenly be aware of it can disable it - but
disabling the clock would still require a new piece of clk driver too...
I think same problems can happen when lower layer SW does not implement
all the features - upper layers may need to implement some odd quircks
and workarounds to get things working, and all that can be useless or
even incompatible with the new low-level SW which finally adds the
missing implementation.
I guess the 'fundamental' difference I was looking for is that the
hardware itself should not change - so in theory we should know the HW
from the day 1. Still, we (I) at times notice we need some information
about the hardware only when we are (I am) writing the drivers ;)
Unfortunately there are companies where all the information about the
hardware is not immediately available ...
Out of the curiosity 2 (an no need to respond if you're in hurry) - how
should one treat hardware logic which is implemented on FPGA? I have in
the past worked for a good while on a project where FPGA blocks were
also described in dt - but this _really_ blurs the line between
"immutable" hardware and "mutable" software. (And yes, we had a great
deal of "fun" with updating the FPGA images, FPGA device-trees, linux
images and board device-trees...)
Anyways, I agree with you. It would be good to have as complete bindings
as possible from the day 1.
By the way - planning to attend ELCE next summer? It'd be great to have
a lecture part II about writing the bindings ;)
Yours,
--Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 2/22/23 18:15, Matti Vaittinen wrote:
//snip
> - 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
> +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;
> +}
> +
> +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
> + 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
> + * 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
> +static int bu27034_chip_init(struct bu27034_data *data)
> +{
//snip
> +
> + /*
> + * 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Thu, 23 Feb 2023 10:26:02 +0100
Krzysztof Kozlowski <[email protected]> wrote:
> On 23/02/2023 07:20, Vaittinen, Matti wrote:
> > Hi dee Ho Krzysztof,
> >
> > Thanks for the review! It's nice you had the time to take a look on RFC :)
> >
> > On 2/22/23 20:57, Krzysztof Kozlowski wrote:
> >> On 22/02/2023 17:14, Matti Vaittinen wrote:
> >>> 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.
> >
> > 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...
To add one wrinkle here, board designers have an annoying habit of not
wiring reset pins up so if there is any easy way to support an alternative
(often there is a software reset command over a bus) then keep the reset
pins as optional and implement that in the driver from day one.
Same is true of interrupts, though that can be harder to deal with so
the binding may say they are optional but the driver fail to load without
them.
Nice reply Krzyzstof, I'll try and remember to point people at this one
as the question comes up pretty often.
Thanks,
Jonathan
>
>
> Best regards,
> Krzysztof
>
On Wed, 22 Feb 2023 18:16:18 +0200
Matti Vaittinen <[email protected]> wrote:
> Add myself as a maintainer for ROHM BU27034 ALS driver.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> MAINTAINERS | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43f5a024daa2..8d31ef852372 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18090,6 +18090,11 @@ S: Maintained
> F: Documentation/devicetree/bindings/iio/light/bh1750.yaml
> F: drivers/iio/light/bh1750.c
>
> +ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
> +M: Matti Vaittinen <[email protected]>
Whilst the wild cards stuff for IIO should also catch this, it's (fairly)
conventional to still add the list entry to make it easy for
people reading the file directly.
We are a bit inconsistent on it though.
> +S: Supported
> +F: drivers/iio/light/rohm-bu27034.c
> +
> ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
> M: Marek Vasut <[email protected]>
> L: [email protected]
On Fri, 24 Feb 2023 12:41:46 +0200
Matti Vaittinen <[email protected]> wrote:
> On 2/22/23 18:15, Matti Vaittinen wrote:
>
> //snip
>
> > - 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
>
> > +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;
> > +}
> > +
> > +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
>
> > + 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
> > + * 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
>
> > +static int bu27034_chip_init(struct bu27034_data *data)
> > +{
>
> //snip
>
> > +
> > + /*
> > + * 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.
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.
> 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.
2) Drivers that do exactly what you describe with their own management of timing.
grep for kthread should find something.
>
> 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?
It's a bit hard because we don't generally know how to hint the timing to
a trigger. But we do have the loop trigger that spins as fast as possible
thus allowing devices to then take a long time to read. It was added for
a similar case (something like a pressure sensor on a drone IIRC) though
not sure anyone uses it much currently.
Jonathan
>
>
> Thanks for the patience :)
> -- Matti
>
On Wed, 22 Feb 2023 18:14:45 +0200
Matti Vaittinen <[email protected]> wrote:
> Some light sensors can adjust both the HW-gain and integration time.
> There are cases where adjusting the integration time has similar impact
> to the scale of the reported values as gain setting has.
>
> IIO users do typically expect to handle scale by a single writable 'scale'
> entry. Driver should then adjust the gain/time accordingly.
>
> It however is difficult for a driver to know whether it should change
> gain or integration time to meet the requested scale. Usually it is
> preferred to have longer integration time which usually improves
> accuracy, but there may be use-cases where long measurement times can be
> an issue. Thus it can be preferable to allow also changing the
> integration time - but mitigate the scale impact by also changing the gain
> underneath. Eg, if integration time change doubles the measured values,
> the driver can reduce the HW-gain to half.
>
> The theory of the computations of gain-time-scale is simple. However,
> some people (undersigned) got that implemented wrong for more than once.
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).
>
> 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!
>
> ---
> 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
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 0d4447df7200..671d84f98c56 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -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
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 6f23817fae6f..f4705fac7a96 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -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
> diff --git a/drivers/iio/light/gain-time-scale-helper.c b/drivers/iio/light/gain-time-scale-helper.c
> new file mode 100644
> index 000000000000..bd8fc11802ee
> --- /dev/null
> +++ b/drivers/iio/light/gain-time-scale-helper.c
> @@ -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]>
> + */
> +
> +#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.
> +{
> + int tmp = 1;
> +
> + if (scale > max || !scale)
> + return -EINVAL;
> +
> + if (0xffffffffffffffffLLU - max < scale) {
U64_MAX from limits.h?
> + /* Risk of overflow */
> + if (max - scale < scale)
> + return 1;
> +
> + while (max - scale > scale * (u64) tmp)
> + tmp++;
> +
> + return tmp + 1;
> + }
> +
> + 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.
> +{
> + 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.
> +
> + tot_gain = iio_gts_get_gain(max, scale);
> + if (tot_gain < 0)
> + return tot_gain;
> +
> + *unknown = tot_gain/known;gts_valid_gain
spaces around /
> +
> + /* 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 >s->itime_table[i];
> +
> + return NULL;
> +}
> +
> +static const struct iio_itime_sel_mul *
> + iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel)
> +{
> + int i;
> +
> + 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
> +
> + for (i = 0; i < gts->num_itime; i++)
> + if (gts->itime_table[i].sel == sel)
> + return >s->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.
> +{
> + int frac;
> +
> + if (scaler > NANO || !scaler)
> + return -EINVAL;
> +
> + frac = do_div(lin_scale, scaler);
> +
> + *scale_whole = lin_scale;
> + *scale_nano = frac * (NANO / scaler);
> +
> + return 0;
> +}
> +
> +static int iio_gts_linearize(int scale_whole, int scale_nano, 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.
> +{
> + /*
> + * 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
> + * @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 :)
> The search functions like the
> + * iio_gts_find_time_and_gain_sel_for_scale() start search from first
> + * provided time.
> + *
> + * Return: 0 on success.
> + */
> +int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
> + const struct iio_gain_sel_pair *gain_tbl, int num_gain,
> + const struct iio_itime_sel_mul *tim_tbl, int num_times,
> + struct iio_gts *gts)
> +{
> + int ret;
> +
> + ret = iio_gts_linearize(max_scale_int, max_scale_nano,
> + >s->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.
> +
> + 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.
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_valid_time);
> +
> +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
> +{
> + int i;
> +
> + for (i = 0; i < gts->num_hwgain; i++)
> + if (gts->hwgain_table[i].gain == gain)
> + return gts->hwgain_table[i].sel;
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
> +
> +bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
> +{
> + return !(iio_gts_find_sel_by_gain(gts, gain) < 0);
return iio_gts_find_sel_by_gain >= 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_valid_gain);
> +
> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
> +{
> + int i;
> +
> + for (i = 0; i < gts->num_hwgain; i++)
> + if (gts->hwgain_table[i].sel == sel)
> + return gts->hwgain_table[i].gain;
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
> +
> +int iio_gts_get_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");
> diff --git a/drivers/iio/light/gain-time-scale-helper.h b/drivers/iio/light/gain-time-scale-helper.h
> new file mode 100644
> index 000000000000..70a952a8de92
> --- /dev/null
> +++ b/drivers/iio/light/gain-time-scale-helper.h
> @@ -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;
> +};
> +
> +#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.
> + .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
On Wed, 22 Feb 2023 18:13:43 +0200
Matti Vaittinen <[email protected]> wrote:
> Support ROHM BU27034 ALS sensor
>
> This series adds support for ROHM BU27034 Ambient Light Sensor.
>
> The BU27034 has configurable gain and measurement (integration) time
> settings. Both of these have direct, inversely proportional relation to
> the sensor's intensity channel scale.
>
> Many users only set the scale, which means that many drivers attempt to
> 'guess' the best gain+time combination to meet the scale. Usually this
> is the biggest integration time which allows setting the requested
> scale. Typically, increasing the integration time has better accuracy
> than increasing the gain, which often amplifies the noise as well as the
> real signal.
>
> However, there may be cases where more responsive sensors are needed.
> So, in some cases the longest integration times may not be what the user
> prefers. The driver has no way of knowing this.
>
> Hence, the approach taken by this series is to allow user to set both
> the scale and the integration time with following logic:
>
> 1. When scale is set, the existing integration time is tried to be
> maintained as a first priority.
> 1a) If the requested scale can't be met by current time, then also
> other time + gain combinations are searched. If scale can be met
> by some other integration time, then the new time may be applied.
> If the time setting is common for all channels, then also other
> channels must be able to maintain their scale with this new time
> (by changing their gain). The new times are scanned in the order
> of preference (typically the longest times first).
> 1b) If the requested scale can be met using current time, then only
> the gain for the channel is changed.
>
> 2. When the integration time change - scale is maintained.
> When integration time change is requested also gain for all impacted
> channels is adjusted so that the scale is not changed. If gain can't
> be changed for some channel, then the request is rejected.
>
> I think this fits the existing 'modes' where scale setting 'guesses' the
> best scale + integration time config - and integration time setting does
> not change the scale.
>
> This logic is really simple. When total gain (either caused by time or
> hw-gain) is doubled, the scale gets halved. Also, the supported times
> are given a 'multiplier' value which tells how much they increase the
> total gain. However, when I wrote this logic in bu27034 driver, I made
> quite a few errors on the way - and driver got pretty big. As I am
> writing drivers for two other sensors (RGB C/IR + flicker BU27010 and RGB
> C/IR BU27008) with similar gain-time-scale logic I thought that adding
> common helpers for these computations might be wise. I hope this way all
> the bugs will be concentrated in one place and not in every individual
> driver ;) Hence, this RFC also intriduces IIO gain-time-scale helpers +
> couple of KUnit tests for the most hairy parts.
>
> I can't help thinking that there should've been simpler way of computing
> the gain-time-scale conversions. Also, pretty good speed improvements
> might be available if some of the do_div()s could be replaced by >>.
> This, however, is not a priority for my light-sensor use-case where
> speed demands are not that big. I am open to all improvements and
> suggestions though!
I wouldn't worry too much on performance as we hopefully aren't doing this
often or in fast paths.
>
> What is still missing is advertising the available scales / integration
> times. The list of available integration times is not static but depend
> on channel gain configurations. Hence, I wonder if there is a way to
> not only advertise available integration times with current gain
> configuration - but also the available scales with different gains?
We may want to just keep going for 'best match' rather than ruling out any
particular integration time. If the scale can't be maintained - change it.
That way if someone really wants to change the integration time they get
to pick up the pieces and deal with the fact the scaling just changed.
That's how we typically handle really nasty interacting choices where
we can't do the stuff you've done here because there isn't an obvious
'right' answer.
>
> Finally, this patch series is an RFC becasue the helper logic could
> benefit from extra pairs of eyes - and because the sensor has been
> only very limitedly tested this far.
>
>
> Matti Vaittinen (6):
> dt-bindings: iio: light: Support ROHM BU27034
> iio: light: Add gain-time-scale helpers
> iio: test: test gain-time-scale helpers
> MAINTAINERS: Add IIO gain-time-scale helpers
> iio: light: ROHM BU27034 Ambient Light Sensor
> MAINTAINERS: Add ROHM BU27034
>
> .../bindings/iio/light/rohm-bu27034.yaml | 46 +
> MAINTAINERS | 13 +
> drivers/iio/light/Kconfig | 16 +
> drivers/iio/light/Makefile | 2 +
> drivers/iio/light/gain-time-scale-helper.c | 446 ++++++
> drivers/iio/light/gain-time-scale-helper.h | 111 ++
> drivers/iio/light/rohm-bu27034.c | 1212 +++++++++++++++++
> drivers/iio/test/Kconfig | 15 +
> drivers/iio/test/Makefile | 1 +
> drivers/iio/test/iio-test-gts.c | 331 +++++
> 10 files changed, 2193 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27034.yaml
> create mode 100644 drivers/iio/light/gain-time-scale-helper.c
> create mode 100644 drivers/iio/light/gain-time-scale-helper.h
> create mode 100644 drivers/iio/light/rohm-bu27034.c
> create mode 100644 drivers/iio/test/iio-test-gts.c
>
>
> base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
On Wed, 22 Feb 2023 18:15:58 +0200
Matti Vaittinen <[email protected]> wrote:
> 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.
I didn't dive into the maths in the luminance calculation ( I rarely do
as those things are horrible!), but otherwise just a few comments inline.
Thanks,
Jonathan
>
> 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
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 671d84f98c56..594228bd1f7f 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -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"
> + depends on I2C
> + select REGMAP_I2C
> + select IIO_GTS_HELPER
> + help
> + Enable support for the ROHM BU27034 ambient light sensor.
> + ROHM BU27034 is an ambient light sesnor with 3 channels and
> + 3 photo diodes capable of detecting a very wide range of
> + illuminance.
> + Typical application is adjusting LCD and backlight power
> + of TVs and mobile phones.
Short lines. Wrap nearer 80 chars.
> +
> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
> new file mode 100644
> index 000000000000..235be7dee6e0
> --- /dev/null
> +++ b/drivers/iio/light/rohm-bu27034.c
> @@ -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>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
I didn't see any custom attrs, so you shouldn't need this header.
Sometimes I think the main purpose of that header these days is to highlight drivers
where a careful look at the ABI is needed :)
> +/*
> + * 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), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), \
> + .address = BU27034_REG_##_name##_LO, \
> + .scan_index = BU27034_CHAN_##_name, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
Unless you have buffered support, anything scan_* is unused and shouldn't be
set.
> + }, \
> + .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.
> + 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;
> + }
> + case BU27034_CHAN_DATA2:
> + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val);
> + if (ret)
> + return ret;
> +
> + return (val & BU27034_MASK_D2_GAIN_HI) >> BU27034_SHIFT_D2_GAIN
> + | (val & BU27034_MASK_D2_GAIN_LO);
> + }
> +
> + dev_err(data->dev, "Can't get gain for channel %d\n", chan);
Given you should never get here, I'd drop the dev_err()
> +
> + return -EINVAL;
> +}
> +
...
> +
> +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;
> + }
> + }
> + if (!found) {
> + dev_err(data->dev,
> + "Can't set scale maintaining other channels\n");
> + ret = -EINVAL;
> +
> + goto unlock_out;
> + }
> +
> + for (i = 0; i < 2; i++) {
Why twice?
> + 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;
> +
> + helper = gain1 * gain1;
> + if (helper > ch0) {
> + do_div(helper64, helper);
> + /*
> + * multiplication with max gain may overflow
> + * if helper64 is greater than 0xFFFFFFFFFFFFF.
> + *
> + * If this is the case we divide first.
> + */
> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
as below.
> + helper64 *= gain0;
> + do_div(helper64, ch0);
> + } else {
> + do_div(helper64, ch0);
> + helper64 *= gain0;
> + }
> +
> + return helper64;
> + }
> +
> + do_div(helper64, ch0);
> + /* Same overflow check here */
> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
Any simple bit of maths that can let us know why that is the overflow check?
> + helper64 *= gain0;
> + do_div(helper64, helper);
> + } else {
> + do_div(helper64, helper);
> + helper64 *= gain0;
> + }
> +
> + return helper64;
> +}
> +
...
> +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);
Not obvious that a read of that register invalidates anything. Perhaps a comment to that affect?
> +}
> +
> +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);
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.
If you do that then the locking dance gets much simpler.
> + 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
> + *
> + * 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 :)
> + * 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?
> + *
> + * 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
> + */
> +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;
> + 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.
> + if (ret)
> + goto unlock_out;
> +
> + /* Avoid div by zero */
> + if (!res[0])
res[0] = max(1, res[0]); perhaps?
> + 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;
> +}
> +
...
> +
> +static int bu27034_meas_dis(struct bu27034_data *data)
> +{
> + return bu27034_meas_set(data, false);
Don't bother with wrappers that do so little - just call meas_set
> +}
> +
> +static int bu27034_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bu27034_data *data = iio_priv(idev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
Why?
> +
> + ret = bu27034_get_int_time(data);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * We use 50000 uS internally for all calculations and only
> + * convert it to 55000 before returning it to the user.
> + *
> + * This is becaise data-sheet says the time is 55 mS - but
> + * vendor provided computations used 50 mS.
> + */
> + if (ret == 50000)
> + ret = 55000;
Set val directly rather than dancing with ret here.
> +
> + *val2 = 0;
> + *val = ret;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
val2 looks to always be zero in which case IIO_VAL_INT
and drop setting val2.
> +
> + 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:
> + * 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.
> + * 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.
> + */
> + ret = bu27034_get_result(data, &res[0]);
> + if (ret)
> + return ret;
> +
> + *val = res[chan->channel - BU27034_CHAN_DATA0];
> +
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + if (chan->type != IIO_LIGHT)
> + return -EINVAL;
> +
> + ret = bu27034_get_lux(data, val);
> + if (ret)
> + return ret;
Trivial. Blank line here and in similar places after error checks and before
an unconnected statement preferred.
> + return IIO_VAL_INT;
> +
> + }
> +
Pull into the switch as a default for same reason given below.
> + return -EINVAL;
> +}
> +
> +static int bu27034_write_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct bu27034_data *data = iio_priv(idev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return bu27034_set_scale(data, chan->channel, val, val2);
> + case IIO_CHAN_INFO_INT_TIME:
> + return bu27034_try_set_int_time(data, val);
one of the static analysis bots likes to complain about unhandled cases.
Cut that off by
default:
return -EINVAL;
and drop the one below.
Same for similar cases. It's possible the bot has become less fussy about this,
but making the default explicit is good practice anyway as helps long term maintainability
when the code gets more complex.
> + }
> +
> + 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;
> +
> + /* Reset */
> + ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
> + BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> +
> + /*
> + * 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.
> + */
> + 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'.
> + */
> + 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);
> + 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.
> + if (!fwnode)
> + return -ENODEV;
> +
> + 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.
The aim of optional for regulators is to handle the case where the driver does
something different if a particular supply isn't there. An example would be
a reference voltage to a device that also has an internal regulator. If the vref
isn't there, we set the device to use the internal reference.
> + 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;
> +
> + 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.
> + 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);
> + if (ret)
> + return ret;
> +
> + mutex_init(&data->mutex);
> + data->regmap = regmap;
> + data->dev = dev;
> +
> + idev->channels = bu27034_channels;
> + idev->num_channels = ARRAY_SIZE(bu27034_channels);
> + idev->name = "bu27034-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.
> + idev->info = &bu27034_info;
On 2/26/23 15:42, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:16:18 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> Add myself as a maintainer for ROHM BU27034 ALS driver.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>> ---
>> MAINTAINERS | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 43f5a024daa2..8d31ef852372 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18090,6 +18090,11 @@ S: Maintained
>> F: Documentation/devicetree/bindings/iio/light/bh1750.yaml
>> F: drivers/iio/light/bh1750.c
>>
>> +ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
>> +M: Matti Vaittinen <[email protected]>
> Whilst the wild cards stuff for IIO should also catch this, it's (fairly)
> conventional to still add the list entry to make it easy for
> people reading the file directly.
Good catch, thanks! Will add:
L: [email protected]
for v2.
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 2/26/23 15:52, Jonathan Cameron wrote:
> On Fri, 24 Feb 2023 12:41:46 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> 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
>>
>> 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.
>
>> 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.
This is the approach I took for the next attempt. I just used the
iio_kfifo_buffer.
> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 2/26/23 18:21, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:14:45 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> Some light sensors can adjust both the HW-gain and integration time.
>> There are cases where adjusting the integration time has similar impact
>> to the scale of the reported values as gain setting has.
>>
>> IIO users do typically expect to handle scale by a single writable 'scale'
>> entry. Driver should then adjust the gain/time accordingly.
>>
>> It however is difficult for a driver to know whether it should change
>> gain or integration time to meet the requested scale. Usually it is
>> preferred to have longer integration time which usually improves
>> accuracy, but there may be use-cases where long measurement times can be
>> an issue. Thus it can be preferable to allow also changing the
>> integration time - but mitigate the scale impact by also changing the gain
>> underneath. Eg, if integration time change doubles the measured values,
>> the driver can reduce the HW-gain to half.
>>
>> The theory of the computations of gain-time-scale is simple. However,
>> some people (undersigned) got that implemented wrong for more than once.
>
> 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).
Right now I did accept only the exact scales/gains. It keeps the code
simpler. As you say, the consequence is that the user-space must be able
to ask the exactly supported scales - which for any generic usage pretty
much requires that the available scales are advertised by the driver.
I think we could take the same approach as with the 'linear_range'
helpers and - when required - add gain-time-scale APIs which return the
next supported lower gain (or higher scale) if the exact computed one is
not supported.
>>
>> 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!
I think this warrants it's own file. And, I would not be surprized if
this was gaining extensions in the future - like the 'find closest lower
gain' stuff mentioned above.
Actually, the theory is not at all gain-time-scale specific - but could
be generally applicable also for other cases where a value (total gain)
is formed by multiplying two other values (time and hw-gain) and where
some conversion to inverse (scale) value is needed.
Yet, in order to keep things simple we need to make assumptions for
example about the sizes of integers - and giving abstract names to
gain/time/scale variables would have resulted quite a mess... Making a
'generic helper' out of this would have probably ended up me to
providing fancy named wrappers around the very basic arithmetic
operations :) But yes, I was initially wondering if this could be
written using more re-usable naming and be put under the /lib.
After all this talk - I think the file name is too long and should be
prefixed with iio. Maybe iio-gts-helpers.c and iio-gts-helpers.h
I will see the comments below and fix the issues to v2 :) Thanks for the
review!
-- Matti
>
>>
>> ---
>> 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
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 0d4447df7200..671d84f98c56 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -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
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index 6f23817fae6f..f4705fac7a96 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -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
>> diff --git a/drivers/iio/light/gain-time-scale-helper.c b/drivers/iio/light/gain-time-scale-helper.c
>> new file mode 100644
>> index 000000000000..bd8fc11802ee
>> --- /dev/null
>> +++ b/drivers/iio/light/gain-time-scale-helper.c
>> @@ -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]>
>> + */
>> +
>> +#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.
>
>> +{
>> + int tmp = 1;
>> +
>> + if (scale > max || !scale)
>> + return -EINVAL;
>> +
>> + if (0xffffffffffffffffLLU - max < scale) {
>
> U64_MAX from limits.h?
>
>> + /* Risk of overflow */
>> + if (max - scale < scale)
>> + return 1;
>> +
>> + while (max - scale > scale * (u64) tmp)
>> + tmp++;
>> +
>> + return tmp + 1;
>> + }
>> +
>> + 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.
>
>> +{
>> + 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.
>
>> +
>> + tot_gain = iio_gts_get_gain(max, scale);
>> + if (tot_gain < 0)
>> + return tot_gain;
>> +
>> + *unknown = tot_gain/known;gts_valid_gain
> spaces around /
>> +
>> + /* 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 >s->itime_table[i];
>> +
>> + return NULL;
>> +}
>> +
>> +static const struct iio_itime_sel_mul *
>> + iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel)
>> +{
>> + int i;
>> +
>> + 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
>
>> +
>> + for (i = 0; i < gts->num_itime; i++)
>> + if (gts->itime_table[i].sel == sel)
>> + return >s->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.
>
>> +{
>> + int frac;
>> +
>> + if (scaler > NANO || !scaler)
>> + return -EINVAL;
>> +
>> + frac = do_div(lin_scale, scaler);
>> +
>> + *scale_whole = lin_scale;
>> + *scale_nano = frac * (NANO / scaler);
>> +
>> + return 0;
>> +}
>> +
>> +static int iio_gts_linearize(int scale_whole, int scale_nano, 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.
>
>> +{
>> + /*
>> + * 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
>> + * @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 :)
>
>
>> The search functions like the
>> + * iio_gts_find_time_and_gain_sel_for_scale() start search from first
>> + * provided time.
>> + *
>> + * Return: 0 on success.
>> + */
>> +int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
>> + const struct iio_gain_sel_pair *gain_tbl, int num_gain,
>> + const struct iio_itime_sel_mul *tim_tbl, int num_times,
>> + struct iio_gts *gts)
>> +{
>> + int ret;
>> +
>> + ret = iio_gts_linearize(max_scale_int, max_scale_nano,
>> + >s->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.
>
>> +
>> + 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.
>
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_valid_time);
>> +
>> +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < gts->num_hwgain; i++)
>> + if (gts->hwgain_table[i].gain == gain)
>> + return gts->hwgain_table[i].sel;
>> +
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain);
>> +
>> +bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
>> +{
>> + return !(iio_gts_find_sel_by_gain(gts, gain) < 0);
>
> return iio_gts_find_sel_by_gain >= 0;
>
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_valid_gain);
>> +
>> +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < gts->num_hwgain; i++)
>> + if (gts->hwgain_table[i].sel == sel)
>> + return gts->hwgain_table[i].gain;
>> +
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel);
>> +
>> +int iio_gts_get_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");
>> diff --git a/drivers/iio/light/gain-time-scale-helper.h b/drivers/iio/light/gain-time-scale-helper.h
>> new file mode 100644
>> index 000000000000..70a952a8de92
>> --- /dev/null
>> +++ b/drivers/iio/light/gain-time-scale-helper.h
>> @@ -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;
>> +};
>> +
>> +#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.
>
>> + .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
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Hi Jonathan,
I started fixing the comments. I think I have just one thing to discuss ;)
On 2/26/23 18:21, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:14:45 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> +
>> +static int iio_gts_get_gain(const u64 max, u64 scale)
>
> trivial but scale equally const in here.
Yes. For this function that's true. I'll update the signature. Still,
the reason why max is marked const is that the max should remain const
in general, throughout the lifetime of the "helper object". It is
fundamentally const value where as the gain, scale and integration time
may all change over the course.
> Not immediately obvious what this function does from name, so add
> some docs.
I added doc (but not kernel doc) as I hope it helps the readers - but
would like to point out that this is meant to be internal only function.
>> +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,
>> + >s->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.
I was also pondering this. I actually think I at some point had a TODO
comment somewhere about deciding if the integration time table(s) should
be required.
Well, during my 'trial and error' change for bu27034 discussed in the
other thread (attempting to shift the data to hide the scale impact of
integration time) I still ended up using these helpers for the gain. I
liked the ability of providing the gain table without re-inventing the
table structs and initialization macros in driver. Additionally I still
used the selector <=> gain conversions. Finally I did add a scale <=>
"total_gain" helpers - which allowed me to do the get/set scale
functions in a simple way.
I still ended up having also the integration time tables for the very
same reason - but just set the multiplication factor to 1 for all times
(because data shifting was supposed to hide the scale impact until I
finally understood what you meant with the saturation detection
problem... <imagine a facepalm emoji here>).
> So check them here and drop the checks in the various other functions.
The rehearsal described above made me to think that (some of) these
helpers can be useful also when there are only gain tables provided.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Thanks for the review again!
I reworked the code for v2 I am about to send out. I think I ended up
having quite a bit changes but I have tried to address most of what you
pointed out. Thanks for all the improvements and the time you have
invested to this already!
On 2/26/23 19:13, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:15:58 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> + .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.
Buffer implemented for v2 :)
>
>> + }, \
>> + .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.
Yep. I was not thrilled about this myself. The sensitivity peaks are at
500 nm and 600 nm - which (according to some quickly checked spectrum
pictures in the webs - no proper fact check done -
https://chem.libretexts.org/Bookshelves/Physical_and_Theoretical_Chemistry_Textbook_Maps/Physical_Chemistry_(LibreTexts)/13%3A_Molecular_Spectroscopy/13.01%3A_The_Electromagnetic_Spectrum
) are landing somewhere between the blue and green and near the orange
areas. Yet, especially the data0 has a very wide sensitivity area as you
pointed out. But yes, this warrants at least a comment.
This is actually also a topic I sent a very low priority email earlier
:) I was wondering if exporting set of data-points representing these
curves via sysfs entry would be something user-space applications could
use... Describing/categorizing the arbitrarily shaped curves may be
othervice hard. Userland could then be running different fitting
algorithms depending on their needs - and see the error levels on the
area they are interested in. But as I said in that mail thread - I don't
have use-case for this so this was just some low priority pondering.
Nothing that should be mixed with this patch mail ;)
> ...
>> +
>> + for (i = 0; i < 2; i++) {
>
> Why twice?
To test the sharpness of reviewers? BTW, You passed with excellent grade!
(No, really because I was interrupted in the middle of writing the code
^_^; Was originally writing a loop to read the channels - but forgot it
when continued. I find it impressive you spotted this - thanks a lot!)
>> +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);
>
> 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.
> >
> If you do that then the locking dance gets much simpler.
I changed the approach for the v2 (to be sent soon(ish)). I did
implement the buffering - and gave up any attempt of caching values for
raw_reads. Instead, I will always start the measurement - wait - read
result - stop the measurement for each read_raw.
It means that:
a) The (typical occasional?) user can still read the processed channel
with read_raw every now and then. It will be slow, but it will only be
1x meas-time slow.
b) Reading all the channels with read_raw using long integration times
will be really slow - and if light levels are changing between the
reads, the channel values are not reflecting the same light levels.
c) We get the power-saving as measurement is not running all the time.
d) Any user who needs some performance - or is interested in getting
data for all the channels - can use the buffered mode.
So, a) and b) mean that read_raw is pretty much only usable for users
interested in reading one channel at a time - or doing some debugging. I
was actually considering dropping the read_raw support for
intensity-channels, but didn't do it because those who use these
raw-values should really know what they represent (they probably know
the sensor and it's limitations). Also, reading for example only the
data2 channel (which I think is not properly described in the
data-sheet) can be a valid use-case for someone interesting in the IR-area.
In any case, the data-reading code got some changes for v2...
>> + * 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 :)
It didn't feel like that when I was implementing the code :rolleyes:
>
>> + * 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?
>
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.
>> + *
>> + * 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
>> + */
>> +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;
>> + 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]).
I think I didn't change this for v2. I in any case expect to see v3 and
probably a few others as well - so I will change this to some of the
later versions if I didn't get you convinced that the &res[0] is Ok.
>
>> + if (ret)
>> + goto unlock_out;
>> +
>> + /* Avoid div by zero */
>> + if (!res[0])
>
> res[0] = max(1, res[0]); perhaps?
This would have been better, yes. However, I did change the data
collection quite a bit for v2 - and there these values may not be in
native byte order - so check for !res[0] feels more correct for v2 than
comparing to value when the value format is not "correct".
>> + 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.)
>
> Given there is no general fix for that, not much you can do even if you don't want to
> miss the data.
>
>> + * 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.
Well, I dropped the caching of values for read_raw. I think it got rid
of these particular problems. The issue 1) is still there for buffered
mode but I guess we just need to live with it. On the bright side,
missing a sample once in a blue moon is not fatal for most of the
use-cases I can think of right now. (Besides, there is no general fix as
you said so worrying about the unknown use-cases right now does not feel
like the sanest thing. I have enough of worrying with the things that
really are a problem...)
>> + /*
>> + * 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.
Well, the documentation is to suppress review comments regarding 1mS
msleep :) And, I can't blame reviewers as the checkpatch is picking this
up too. Hence I think it's Ok to tell that: "Yes, I know the sleep is
likely to last longer than the requested 1 mS but it does not matter for
our use-case so we still consciously chose to use msleep()."
>
>> + */
>> + 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'.
I hope I killed the worst power consumption at v2 by not running the
measurement all the time at the background. I don't at the moment have a
use-case for runtime pm - and as runtime pm tends to be "not trivial" -
I will leave those bugs to be made only when needed... But yes, this
comment can go as it adds pretty much no value.
>
>> + */
>> + 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);
>> + 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.
True! I didn't even think of such a case.
>> + 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.
Yes. I think we will also have a warning in a log - which I was not
liking. OTOH, as the component clearly needs the VDD, maybe the warning
about missing one in DT is also Ok.
>> + 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;
>> +
>> + 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.
Makes sense. Besides, we should be able to trust the dt has correct
compatibles - I'm not sure we should do these runtime checks for part
IDs at all. I dropped the error - return and changed the print to warn.
>> +
>> + 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.
Ok, thanks!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, 28 Feb 2023 08:43:28 +0000
"Vaittinen, Matti" <[email protected]> wrote:
> On 2/26/23 15:52, Jonathan Cameron wrote:
> > On Fri, 24 Feb 2023 12:41:46 +0200
> > Matti Vaittinen <[email protected]> wrote:
> >
> >> 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
> >>
> >> 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.
> >
> >> 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.
>
> This is the approach I took for the next attempt. I just used the
> iio_kfifo_buffer.
>
> > 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's purpose was a originally a bit different, so I'm not surprised it
didn't really fit. The target was a sensor that needed explicit triggering
but then took a while to get the data. Aim was to grab data as quick as we
could. So there were no problems with alignment.
> 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
>
> >
> >> + * 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?
> >
>
> 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.
We have had app notes in the past that suggest particular useful factors
for common filters or indeed the correction factor format that should be used
if the device has some non volatile memory to store it in. If neither
is true here, it's going to be 'interesting' to support.
>
> 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.
Leave it for now.
>
> >> + *
> >> + * 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
> >> + */
> >> +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;
> >> + 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]).
>
> I think I didn't change this for v2. I in any case expect to see v3 and
> probably a few others as well - so I will change this to some of the
> later versions if I didn't get you convinced that the &res[0] is Ok.
I don't care that strongly. Though to me res is no less clear than
&res[0] when we are intending to access the whole array.
If we had a series of individual element accesses or other partial
writes of the array in the functions then I'd agree with you.
>
> >
> >> + if (ret)
> >> + goto unlock_out;
> >> +
> >> + /* Avoid div by zero */
> >> + if (!res[0])
> >
> > res[0] = max(1, res[0]); perhaps?
>
> This would have been better, yes. However, I did change the data
> collection quite a bit for v2 - and there these values may not be in
> native byte order - so check for !res[0] feels more correct for v2 than
> comparing to value when the value format is not "correct".
Ok. Maybe... I'll take a look.
>
> >> + 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.)
> >
> > Given there is no general fix for that, not much you can do even if you don't want to
> > miss the data.
> >
> >> + * 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.
>
> Well, I dropped the caching of values for read_raw. I think it got rid
> of these particular problems. The issue 1) is still there for buffered
> mode but I guess we just need to live with it. On the bright side,
> missing a sample once in a blue moon is not fatal for most of the
> use-cases I can think of right now. (Besides, there is no general fix as
> you said so worrying about the unknown use-cases right now does not feel
> like the sanest thing. I have enough of worrying with the things that
> really are a problem...)
>
> >> + /*
> >> + * 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.
>
> Well, the documentation is to suppress review comments regarding 1mS
> msleep :) And, I can't blame reviewers as the checkpatch is picking this
> up too. Hence I think it's Ok to tell that: "Yes, I know the sleep is
> likely to last longer than the requested 1 mS but it does not matter for
> our use-case so we still consciously chose to use msleep()."
I understand how you ended up with it, but meh, reviewers have seen those
warnings lots of times already!
:)
..
> > 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.
>
> Makes sense. Besides, we should be able to trust the dt has correct
> compatibles - I'm not sure we should do these runtime checks for part
> IDs at all. I dropped the error - return and changed the print to warn.
Experience says the checks are useful. Lots of boards have turned up with the
wrong part, so warning at least is a nice to have!
We had to argue a bit with the DT maintainers to get them to let us have
the warnings ;)
Jonathan