Support ROHM BU27034 ALS sensor
This series adds support for ROHM BU27034 Ambient Light Sensor.
The BU27034 has configurable gain and measurement (integration) time
settings. Both of these have inversely proportional relation to the
sensor's intensity channel scale.
Many users only set the scale, which means that many drivers attempt to
'guess' the best gain+time combination to meet the scale. Usually this
is the biggest integration time which allows setting the requested
scale. Typically, increasing the integration time has better accuracy
than increasing the gain, which often amplifies the noise as well as the
real signal.
However, there may be cases where more responsive sensors are needed.
So, in some cases the longest integration times may not be what the user
prefers. The driver has no way of knowing this.
Hence, the approach taken by this series is to allow user to set both
the scale and the integration time with following logic:
1. When scale is set, the existing integration time is tried to be
maintained as a first priority.
1a) If the requested scale can't be met by current time, then also
other time + gain combinations are searched. If scale can be met
by some other integration time, then the new time may be applied.
If the time setting is common for all channels, then also other
channels must be able to maintain their scale with this new time
(by changing their gain). The new times are scanned in the order
of preference (typically the longest times first).
1b) If the requested scale can be met using current time, then only
the gain for the channel is changed.
2. When the integration time change - scale is tried to be maintained.
When integration time change is requested also gain for all impacted
channels is adjusted so that the scale is not changed, or is chaned
as little as possible. This is different from the RFCv1 where the
request was rejected if suitable gain couldn't be found for some
channel(s).
This logic is simple. When total gain (either caused by time or hw-gain)
is doubled, the scale gets halved. Also, the supported times are given a
'multiplier' value which tells how much they increase the total gain.
However, when I wrote this logic in bu27034 driver, I made quite a few
errors on the way - and driver got pretty big. As I am writing drivers
for two other sensors (RGB C/IR + flicker BU27010 and RGB C/IR BU27008)
with similar gain-time-scale logic I thought that adding common helpers
for these computations might be wise. I hope this way all the bugs will
be concentrated in one place and not in every individual driver ;)
Hence, this series also intriduces IIO gain-time-scale helpers
(abbreviated as gts-helpers) + a couple of KUnit tests for the most
hairy parts.
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.
Finally, these added helpers do provide some value also for drivers
which only:
a) allow gain change
or
b) allow changing both the time and gain but so that the time-change is
not reflected in register values.
For a) we provide the gain - selector (register value) table format +
selector to gain look-ups, gain <-> scale conversions and the available
scales helpers.
For latter case we also provide the time-tables, and actually all the
APIs should be usable by setting the time multiplier to 1. (not testeted
thoroughly though).
Revision history:
v2 => v3: (Mostly fixes to review comments from Andy and Jonathan)
- dt-binding and maintainer patches unchanged.
- iio-gts-helper tests: Use namespaces
- iio-gts-helpers + bu27034 plenty of changes. See more comprehensive
changelog in individual patches.
RFCv1 => v2:
dt-bindings:
- Fix binding file name and id by using comma instead of a hyphen to
separate the vendor and part names.
gts-helpers:
- fix include guardian
- Improve kernel doc for iio_init_iio_gts.
- Add iio_gts_scale_to_total_gain
- Add iio_gts_total_gain_to_scale
- Fix review comments from Jonathan
- add documentation to few functions
- replace 0xffffffffffffffffLLU by U64_MAX
- some styling fixes
- drop unnecessary NULL checks
- order function arguments by in / out purpose
- drop GAIN_SCALE_ITIME_MS()
- Add helpers for available scales and times
- Rename to iio-gts-helpers
gts-tests:
- add tests for available scales/times helpers
- adapt to renamed iio-gts-helpers.h header
bu27034-driver:
- (really) protect read-only registers
- fix get and set gain
- buffered mode
- Protect the whole sequences including meas_en/meas_dis to avoid messing
up the enable / disable order
- typofixes / doc improvements
- change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US()
- use more accurate scale for lux channel (milli lux)
- provide available scales / integration times (using helpers).
- adapt to renamed iio-gts-helpers.h file
- bu27034 - longer lines in Kconfig
- Drop bu27034_meas_en and bu27034_meas_dis wrappers.
- Change device-name from bu27034-als to bu27034
MAINTAINERS:
- Add iio-list
---
Matti Vaittinen (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 | 14 +
drivers/iio/light/Kconfig | 17 +
drivers/iio/light/Makefile | 2 +
drivers/iio/light/iio-gts-helper.c | 1158 +++++++++++++
drivers/iio/light/iio-gts-helper.h | 134 ++
drivers/iio/light/rohm-bu27034.c | 1501 +++++++++++++++++
drivers/iio/test/Kconfig | 15 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-gts.c | 538 ++++++
10 files changed, 3426 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
create mode 100644 drivers/iio/light/iio-gts-helper.c
create mode 100644 drivers/iio/light/iio-gts-helper.h
create mode 100644 drivers/iio/light/rohm-bu27034.c
create mode 100644 drivers/iio/test/iio-test-gts.c
base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes
capable of detecting a very wide range of illuminance. Typical application
is adjusting LCD and backlight power of TVs and mobile phones.
Add dt-bindings.
Signed-off-by: Matti Vaittinen <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
v2 =>
- No changes
Changes since RFCv1 => v2
- Fix binding file name and id by using comma instead of a hyphen to
separate the vendor and part names.
---
.../bindings/iio/light/rohm,bu27034.yaml | 46 +++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
new file mode 100644
index 000000000000..30a109a1bf3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm,bu27034.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27034 ambient light sensor
+
+maintainers:
+ - Matti Vaittinen <[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.
Changes:
v2 => v3: (mostly fixes based on review by Andy)
- Fix typos
- Styling fixes
- Use namespace for exported symbols
- Protect allocs against argument overflow
- Fix include protection name
- add types.h inclusion and struct device forward declaration
RFCv1 => v2:
- fix include guardian
- Improve kernel doc for iio_init_iio_gts.
- Add iio_gts_scale_to_total_gain
- Add iio_gts_total_gain_to_scale
- Fix review comments from Jonathan
- add documentation to few functions
- replace 0xffffffffffffffffLLU by U64_MAX
- some styling fixes
- drop unnecessary NULL checks
- order function arguments by in / out purpose
- drop GAIN_SCALE_ITIME_MS()
- Add helpers for available scales and times
- Rename to iio-gts-helpers
---
drivers/iio/light/Kconfig | 3 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/iio-gts-helper.c | 1158 ++++++++++++++++++++++++++++
drivers/iio/light/iio-gts-helper.h | 134 ++++
4 files changed, 1296 insertions(+)
create mode 100644 drivers/iio/light/iio-gts-helper.c
create mode 100644 drivers/iio/light/iio-gts-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..82d14ebd3c11 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) += iio-gts-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/iio-gts-helper.c b/drivers/iio/light/iio-gts-helper.c
new file mode 100644
index 000000000000..9494a66c6f1d
--- /dev/null
+++ b/drivers/iio/light/iio-gts-helper.c
@@ -0,0 +1,1158 @@
+// 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/device.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/types.h>
+
+#include "iio-gts-helper.h"
+
+/*
+ * iio_gts_get_gain - Convert scale to total gain
+ *
+ * Internal helper for converting scale to total gain.
+ *
+ * @max: Maximum linearized scale. As an example, when scale is created
+ * in magnitude of NANOs and max scale is 64.1 - The linearized
+ * scale is 64 100 000 000.
+ * @scale: Linearized scale to compte the gain for.
+ *
+ * Return: (floored) gain corresponding to the scale. -EINVAL if scale
+ * is invalid.
+ */
+static int iio_gts_get_gain(const u64 max, const u64 scale)
+{
+ int tmp = 1;
+
+ if (scale > max || !scale)
+ return -EINVAL;
+
+ if (U64_MAX - 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;
+}
+
+/*
+ * gain_get_scale_fraction - get the gain or time based on scale and known one
+ *
+ * Internal helper for computing unknown fraction of total gain.
+ * Compute either gain or time based on scale and either the gain or time
+ * depending on which one is known.
+ *
+ * @max: Maximum linearized scale. As an example, when scale is created
+ * in magnitude of NANOs and max scale is 64.1 - The linearized
+ * scale is 64 100 000 000.
+ * @scale: Linearized scale to compute the gain/time for.
+ * @known: Either integration time or gain depending on which one is known
+ * @unknown: Pointer to variable where the computed gain/time is stored
+ *
+ * Return: 0 on success
+ */
+static int gain_get_scale_fraction(const u64 max, u64 scale, int known,
+ int *unknown)
+{
+ int tot_gain;
+
+ tot_gain = iio_gts_get_gain(max, scale);
+ if (tot_gain < 0)
+ return tot_gain;
+
+ *unknown = tot_gain / known;
+
+ /* We require total gain to be exact multiple of known * unknown */
+ if (!*unknown || *unknown * known != tot_gain)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct iio_itime_sel_mul *
+iio_gts_find_itime_by_time(struct iio_gts *gts, int time)
+{
+ int i;
+
+ if (!gts->num_itime)
+ return NULL;
+
+ for (i = 0; i < gts->num_itime; i++)
+ if (gts->itime_table[i].time_us == time)
+ return >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;
+
+ 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, unsigned long scaler,
+ int *scale_whole, int *scale_nano)
+{
+ 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,
+ unsigned long scaler, u64 *lin_scale)
+{
+ /*
+ * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
+ * multiplication followed by division to avoid overflow
+ */
+ if (scaler > NANO || !scaler)
+ return -EINVAL;
+
+ *lin_scale = (u64) scale_whole * (u64)scaler +
+ (u64)(scale_nano / (NANO / scaler));
+
+ return 0;
+}
+
+/**
+ * iio_gts_total_gain_to_scale - convert gain to scale
+ * @gts: Gain time scale descriptor
+ * @scale_int: Pointer to integral part of the scale (typically val1)
+ * @scale_nano: Pointer to ractional part of the scale (nano or ppb)
+ * @gain_tot: the gain to be converted
+ *
+ * Convert the total gain value to scale. NOTE: This does not separate gain
+ * generated by hwgain or integration time. It is up to caller to decide what
+ * part of the total gain is due to integration time and what due to hw-gain.
+ *
+ * Return: 0 on success. Negative errno on failure
+ */
+int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
+ int *scale_int, int *scale_nano)
+{
+ u64 tmp;
+
+ tmp = gts->max_scale;
+
+ do_div(tmp, total_gain);
+
+ return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_scale_to_total_gain - convert scale to gain
+ * @gts: Gain time scale descriptor
+ * @scale_int: Integral part of the scale (typically val1)
+ * @scale_nano: Fractional part of the scale (nano or ppb)
+ * @gain_tot: pointer to variable where gain is stored
+ *
+ * Convert the scale value to total gain. NOTE: This does not separate gain
+ * generated by hwgain or integration time. It is up to caller to decide what
+ * part of the total gain is due to integration time and what due to hw-gain.
+ *
+ * Return: 0 on success. Negative errno on failure
+ */
+int iio_gts_scale_to_total_gain(struct iio_gts *gts, int scale_int,
+ int scale_nano, int *gain_tot)
+{
+ u64 lin_scale;
+ int ret;
+
+ ret = iio_gts_linearize(scale_int, scale_nano, NANO, &lin_scale);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_get_gain(gts->max_scale, lin_scale);
+ if (ret < 0)
+ return ret;
+
+ *gain_tot = ret;
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_scale_to_total_gain, IIO_GTS_HELPER);
+
+/**
+ * iio_init_iio_gts - Initialize the gain-time-scale helper
+ * @max_scale_int: integer part of the maximum scale value
+ * @max_scale_nano: fraction part of the maximum scale value
+ * @gain_tbl: table describing supported gains
+ * @num_gain: number of gains in the gaintable
+ * @tim_tbl: table describing supported integration times. Provide
+ * the integration time table sorted so that the preferred
+ * integration time is in the first array index. The search
+ * functions like the
+ * iio_gts_find_time_and_gain_sel_for_scale() start search
+ * from first provided time.
+ * @num_times: number of times in the time table
+ * @gts: pointer to the helper struct
+ *
+ * Initialize the gain-time-scale helper for use.
+ *
+ * Return: 0 on success.
+ */
+int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
+ const struct iio_gain_sel_pair *gain_tbl, int num_gain,
+ const struct iio_itime_sel_mul *tim_tbl, int num_times,
+ struct iio_gts *gts)
+{
+ int ret;
+
+ ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
+ >s->max_scale);
+ if (ret)
+ return ret;
+
+ gts->hwgain_table = gain_tbl;
+ gts->num_hwgain = num_gain;
+ gts->itime_table = tim_tbl;
+ gts->num_itime = num_times;
+ gts->per_time_avail_scale_tables = NULL;
+ gts->avail_time_tables = NULL;
+ gts->avail_all_scales_table = NULL;
+ gts->num_avail_all_scales = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_init_iio_gts, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_purge_avail_scale_table - free-up the available scale tables
+ * @gts: Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_scale_table(). Please note
+ * that the helpers for getting available scales like the
+ * iio_gts_all_avail_scales() are not usable after this call. Thus, this should
+ * be only called after these helpers can no longer be called (Eg. after
+ * the iio-device has been deregistered).
+ */
+void iio_gts_purge_avail_scale_table(struct iio_gts *gts)
+{
+ int i;
+
+ if (gts->per_time_avail_scale_tables) {
+ for (i = 0; i < gts->num_itime; i++)
+ kfree(gts->per_time_avail_scale_tables[i]);
+
+ kfree(gts->per_time_avail_scale_tables);
+ gts->per_time_avail_scale_tables = NULL;
+ }
+
+ kfree(gts->avail_all_scales_table);
+ gts->avail_all_scales_table = NULL;
+
+ gts->num_avail_all_scales = 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_purge_avail_scale_table, IIO_GTS_HELPER);
+
+static int iio_gts_gain_cmp(const void *a, const void *b)
+{
+ return *(int *)a - *(int *)b;
+}
+
+static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
+{
+ int ret, i, j, new_idx, time_idx;
+ int *all_gains;
+ size_t gain_bytes;
+
+ for (i = 0; i < gts->num_itime; i++) {
+ /*
+ * Sort the tables for nice output and for easier finding of
+ * unique values
+ */
+ sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
+ NULL);
+
+ /* Convert gains to scales */
+ for (j = 0; j < gts->num_hwgain; j++) {
+ ret = iio_gts_total_gain_to_scale(gts, gains[i][j],
+ &scales[i][2 * j],
+ &scales[i][2 * j + 1]);
+ if (ret)
+ return ret;
+ }
+ }
+
+ gain_bytes = array_size(gts->num_hwgain, sizeof(int));
+ all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
+ if (!all_gains)
+ return -ENOMEM;
+
+ /*
+ * We assume all the gains for same integration time were unique.
+ * It is likely the first time table had greatest time multiplier as
+ * the times are in the order of preference and greater times are
+ * usually preferred. Hence we start from the last table which is likely
+ * to have the smallest total gains
+ */
+ time_idx = gts->num_itime - 1;
+ memcpy(all_gains, gains[time_idx], gain_bytes);
+ new_idx = gts->num_hwgain;
+
+ while (time_idx--) {
+ for (j = 0; j < gts->num_hwgain; j++) {
+ int candidate = gains[time_idx][j];
+ int chk;
+
+ if (candidate > all_gains[new_idx - 1]) {
+ all_gains[new_idx] = candidate;
+ new_idx++;
+
+ continue;
+ }
+ for (chk = 0; chk < new_idx; chk++)
+ if (candidate <= all_gains[chk])
+ break;
+
+ if (candidate == all_gains[chk])
+ continue;
+
+ memmove(&all_gains[chk + 1], &all_gains[chk],
+ (new_idx - chk) * sizeof(int));
+ all_gains[chk] = candidate;
+ new_idx++;
+ }
+ }
+
+ gts->num_avail_all_scales = new_idx;
+ gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
+ 2 * sizeof(int), GFP_KERNEL);
+ if (!gts->avail_all_scales_table)
+ ret = -ENOMEM;
+ else
+ for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
+ ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+ >s->avail_all_scales_table[i * 2],
+ >s->avail_all_scales_table[i * 2 + 1]);
+
+ kfree(all_gains);
+ if (ret && gts->avail_all_scales_table)
+ kfree(gts->avail_all_scales_table);
+
+ return ret;
+}
+
+/**
+ * iio_gts_build_avail_scale_table - create tables of available scales
+ * @gts: Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales based on the
+ * originally given gain and time tables. When both time and gain tables are
+ * given this results:
+ * 1. A set of tables representing available scales for each supported
+ * integration time.
+ * 2. A single table listing all the unique scales that any combination of
+ * supported gains and times can provide.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_scale_table() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+int iio_gts_build_avail_scale_table(struct iio_gts *gts)
+{
+ int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
+
+ per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+ if (!per_time_gains)
+ return ret;
+
+ per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
+ if (!per_time_scales)
+ goto free_gains;
+
+ for (i = 0; i < gts->num_itime; i++) {
+ per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int),
+ GFP_KERNEL);
+ if (!per_time_scales[i])
+ goto err_free_out;
+
+ per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int),
+ GFP_KERNEL);
+ if (!per_time_gains[i])
+ goto err_free_out;
+
+
+ for (j = 0; j < gts->num_hwgain; j++)
+ per_time_gains[i][j] = gts->hwgain_table[j].gain *
+ gts->itime_table[i].mul;
+ }
+
+ ret = gain_to_scaletables(gts, per_time_gains, per_time_scales);
+ if (ret)
+ goto err_free_out;
+
+ kfree(per_time_gains);
+ gts->per_time_avail_scale_tables = per_time_scales;
+
+ return 0;
+
+err_free_out:
+ while (i) {
+ /*
+ * It does not matter if i'th alloc was not succesfull as
+ * kfree(NULL) is safe.
+ */
+ kfree(per_time_scales[i]);
+ kfree(per_time_gains[i]);
+
+ i--;
+ }
+ kfree(per_time_scales);
+free_gains:
+ kfree(per_time_gains);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_build_avail_scale_table, IIO_GTS_HELPER);
+
+static inline void devm_iio_gts_avail_scale_drop(void *res)
+{
+ iio_gts_purge_avail_scale_table(res);
+}
+
+/**
+ * devm_iio_gts_build_avail_scale_table - do managed tables of available scales
+ * @gts: Gain time scale descriptor
+ * @dev: Device whose life-time tables are bound to
+ *
+ * Create tables of available scales which are freed upon the device detach.
+ * See the iio_gts_build_avail_scale_table() for more information.
+ *
+ * NOTE: after the tables have been purged, the helpers for getting the
+ * available scales are no longer usable. Care must be taken that unwinding
+ * is done in correct order (iio device is deregistered prior purging the
+ * tables).
+ *
+ * Return: 0 on success.
+ */
+int devm_iio_gts_build_avail_scale_table(struct device *dev,
+ struct iio_gts *gts)
+{
+ int ret;
+
+ ret = iio_gts_build_avail_scale_table(gts);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_iio_gts_avail_scale_drop, gts);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_gts_build_avail_scale_table, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_build_avail_time_table - build table of available integration times
+ * @gts: Gain time scale descriptor
+ *
+ * Build the table which can represent the available times to be returned
+ * to users using the read_avail-callback.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_time_table() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+int iio_gts_build_avail_time_table(struct iio_gts *gts)
+{
+ int *times, i, j, idx = 0;
+
+ if (!gts->num_itime)
+ return 0;
+
+ times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL);
+ if (!times)
+ return -ENOMEM;
+
+ for (i = gts->num_itime - 1; i >= 0; i--) {
+ int new = gts->itime_table[i].time_us;
+
+ if (times[idx] < new) {
+ times[idx++] = new;
+ continue;
+ }
+
+ for (j = 0; j <= idx; j++) {
+ if (times[j] > new) {
+ memmove(×[j + 1], ×[j], (idx - j) * sizeof(int));
+ times[j] = new;
+ idx++;
+ }
+ }
+ }
+ gts->avail_time_tables = times;
+ /*
+ * This is just to survive a unlikely corner-case where times in the
+ * given time table were not unique. Else we could just trust the
+ * gts->num_itime.
+ */
+ gts->num_avail_time_tables = idx;
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_build_avail_time_table, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_purge_avail_time_table - free-up the available integration time table
+ * @gts: Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_time_table(). Please note
+ * that the helpers for getting available integration times like the
+ * iio_gts_avail_times() are not usable after this call. Thus, this should
+ * be only called after these helpers can no longer be called (Eg. after
+ * the iio-device has been deregistered).
+ */
+void iio_gts_purge_avail_time_table(struct iio_gts *gts)
+{
+ if (gts->num_avail_time_tables) {
+ kfree(gts->avail_time_tables);
+ gts->avail_time_tables = NULL;
+ gts->num_avail_time_tables = 0;
+ }
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_purge_avail_time_table, IIO_GTS_HELPER);
+
+static inline void devm_iio_gts_avail_time_drop(void *res)
+{
+ iio_gts_purge_avail_time_table(res);
+}
+
+/**
+ * devm_iio_gts_build_avail_time_table - do managed tables of available times
+ * @gts: Gain time scale descriptor
+ * @dev: Device whose life-time tables are bound to
+ *
+ * Create a table of available integration times. Table is freed upon the
+ * device detach. See the iio_gts_build_avail_time_table() for more information.
+ *
+ * NOTE: after the tables have been purged, the helpers for getting
+ * available integration times are no longer usable. Care must be taken that
+ * unwinding is done in correct order (iio device is deregistered prior
+ * purging the tables).
+ *
+ * Return: 0 on success.
+ */
+int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts)
+{
+ int ret;
+
+ if (!gts->num_itime)
+ return 0;
+
+ ret = iio_gts_build_avail_time_table(gts);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_iio_gts_avail_time_drop, gts);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_gts_build_avail_time_table, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_build_avail_tables - create tables of available scales and int times
+ * @gts: Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales and available
+ * integration times. Availability tables are built based on the originally
+ * given gain and given time tables.
+ *
+ * When both time and gain tables are
+ * given this results:
+ * 1. A set of sorted tables representing available scales for each supported
+ * integration time.
+ * 2. A single sorted table listing all the unique scales that any combination
+ * of supported gains and times can provide.
+ * 3. A sorted table of supported integration times
+ *
+ * After these tables are built one can use the iio_gts_all_avail_scales(),
+ * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
+ * implement the read_avail opeations.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_tables() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+int iio_gts_build_avail_tables(struct iio_gts *gts)
+{
+ int ret;
+
+ ret = iio_gts_build_avail_scale_table(gts);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_build_avail_time_table(gts);
+ if (ret)
+ iio_gts_purge_avail_scale_table(gts);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_build_avail_tables, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_purge_avail_tables - free-up the availability tables
+ * @gts: Gain time scale descriptor
+ *
+ * Free the space reserved by iio_gts_build_avail_tables(). Frees both the
+ * integration time and scale tables.
+ *
+ * Note that the helpers for getting available integration times or scales
+ * like the iio_gts_avail_times() are not usable after this call. Thus, this
+ * should be only called after these helpers can no longer be called (Eg.
+ * after the iio-device has been deregistered).
+ */
+void iio_gts_purge_avail_tables(struct iio_gts *gts)
+{
+ iio_gts_purge_avail_time_table(gts);
+ iio_gts_purge_avail_scale_table(gts);
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_purge_avail_tables, IIO_GTS_HELPER);
+
+static void devm_iio_gts_avail_all_drop(void *res)
+{
+ iio_gts_purge_avail_tables(res);
+}
+
+/**
+ * devm_iio_gts_build_avail_tables - manged add availability tables
+ * @gts: Gain time scale descriptor
+ *
+ * Build the tables which can represent the available scales and available
+ * integration times. Availability tables are built based on the originally
+ * given gain and given time tables.
+ *
+ * When both time and gain tables are
+ * given this results:
+ * 1. A set of sorted tables representing available scales for each supported
+ * integration time.
+ * 2. A single sorted table listing all the unique scales that any combination
+ * of supported gains and times can provide.
+ * 3. A sorted table of supported integration times
+ *
+ * After these tables are built one can use the iio_gts_all_avail_scales(),
+ * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to
+ * implement the read_avail opeations.
+ *
+ * The tables are automatically released upon device detach.
+ *
+ * NOTE: after the tables have been purged, the helpers for getting
+ * available scales / integration times are no longer usable. Care must be
+ * taken that unwinding is done in correct order (iio device is deregistered
+ * prior purging the tables).
+ *
+ * Return: 0 on success.
+ */
+int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts)
+{
+ int ret;
+
+ ret = iio_gts_build_avail_tables(gts);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_iio_gts_avail_all_drop, gts);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_gts_build_avail_tables, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_all_avail_scales - helper for listing all available scales
+ * @gts: Gain time scale descriptor
+ * @vals: Returned array of supported scales
+ * @type: Type of returned scale values
+ * @length: Amount of returned values in array
+ *
+ * Returns a value suitable to be returned from read_avail or a negative error
+ */
+int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
+ int *length)
+{
+ /*
+ * Using this function prior building the tables is a driver-error
+ * which should be fixed when the driver is tested for a first time
+ */
+ if (WARN_ON(!gts->num_avail_all_scales))
+ return -EINVAL;
+
+ *vals = gts->avail_all_scales_table;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ *length = gts->num_avail_all_scales * 2;
+
+ return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_all_avail_scales, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_avail_scales_for_time - list scales for integration time
+ * @gts: Gain time scale descriptor
+ * @time: Integration time for which the scales are listed
+ * @vals: Returned array of supported scales
+ * @type: Type of returned scale values
+ * @length: Amount of returned values in array
+ *
+ * Drivers which do not allow scale setting to change integration time can
+ * use this helper to list only the scales whic are valid for given integration
+ * time.
+ *
+ * Returns a value suitable to be returned from read_avail or a negative error
+ */
+int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
+ const int **vals, int *type, int *length)
+{
+ int i;
+
+ for (i = 0; i < gts->num_itime; i++)
+ if (gts->itime_table[i].time_us == time)
+ break;
+
+ if (i == gts->num_itime)
+ return -EINVAL;
+
+ *vals = gts->per_time_avail_scale_tables[i];
+ *type = IIO_VAL_INT_PLUS_NANO;
+ *length = gts->num_hwgain * 2;
+
+ return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_avail_scales_for_time, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_avail_times - helper for listing available integration times
+ * @gts: Gain time scale descriptor
+ * @vals: Returned array of supported timees
+ * @type: Type of returned scale values
+ * @length: Amount of returned values in array
+ *
+ * Returns a value suitable to be returned from read_avail or a negative error
+ */
+int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type,
+ int *length)
+{
+ /*
+ * Using this function prior building the tables is a driver-error
+ * which should be fixed when the driver is tested for a first time
+ */
+ if (WARN_ON(!gts->num_avail_time_tables))
+ return -EINVAL;
+
+ *vals = gts->avail_time_tables;
+ *type = IIO_VAL_INT;
+ *length = gts->num_avail_time_tables;
+
+ return IIO_AVAIL_LIST;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_avail_times, IIO_GTS_HELPER);
+
+/**
+ * iio_gts_valid_time - check if given integration time is valid
+ * @gts: Gain time scale descriptor
+ * @time_us: Integration time to check
+ *
+ * Return: True if given time is supported by device. False if not
+ */
+bool iio_gts_valid_time(struct iio_gts *gts, int time_us)
+{
+ return iio_gts_find_itime_by_time(gts, time_us);
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_valid_time, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_find_sel_by_gain, IIO_GTS_HELPER);
+
+bool iio_gts_valid_gain(struct iio_gts *gts, int gain)
+{
+ return iio_gts_find_sel_by_gain(gts, gain) >= 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_valid_gain, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_find_gain_by_sel, IIO_GTS_HELPER);
+
+int iio_gts_get_min_gain(struct iio_gts *gts)
+{
+ int i, min = -EINVAL;
+
+ for (i = 0; i < gts->num_hwgain; i++) {
+ int gain = gts->hwgain_table[i].gain;
+
+ if (min == -EINVAL)
+ min = gain;
+ else
+ min = min(min, gain);
+ }
+
+ return min;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_get_min_gain, IIO_GTS_HELPER);
+
+/**
+ * iio_find_closest_gain_low - Find the closest lower matching gain
+ * @gts: Gain time scale descriptor
+ * @gain: reference gain for which the closest match is searched
+ * @in_range: indicate if the reference gain was actually in the range of
+ * supported gains.
+ *
+ * Search for closest supported gain that is lower than or equal to the
+ * gain given as a parameter. This is usable for drivers which do not require
+ * user to request exact matching gain but rather fo rounding to a supported
+ * gain value which is equal or lower (setting lower gain is typical for
+ * avoiding saturation)
+ *
+ * Return: The closest matching supported gain or -EINVAL is reference
+ * gain was smaller than the smallest supported gain.
+ */
+int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range)
+{
+ int i, diff = 0, min = 0;
+ int best = -1;
+
+ *in_range = false;
+
+ for (i = 0; i < gts->num_hwgain; i++) {
+ /*
+ * It is not expected this function is called for an exactly
+ * matching gain.
+ */
+ if (unlikely(gain == gts->hwgain_table[i].gain)) {
+ *in_range = true;
+ return gain;
+ }
+ if (!min)
+ min = gts->hwgain_table[i].gain;
+ else
+ min = min(min, gts->hwgain_table[i].gain);
+ if (gain > gts->hwgain_table[i].gain) {
+ if (!diff) {
+ diff = gain - gts->hwgain_table[i].gain;
+ best = i;
+ } else {
+ int tmp = gain - gts->hwgain_table[i].gain;
+
+ if (tmp < diff) {
+ diff = tmp;
+ best = i;
+ }
+ }
+ } else {
+ /*
+ * We found valid hwgain which is greater than
+ * reference. So, unless we return a failure below we
+ * will have found an in-range gain
+ */
+ *in_range = true;
+ }
+ }
+ /* The requested gain was smaller than anything we support */
+ if (!diff) {
+ *in_range = false;
+
+ return -EINVAL;
+ }
+
+ return gts->hwgain_table[best].gain;
+}
+EXPORT_SYMBOL_NS_GPL(iio_find_closest_gain_low, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_get_int_time_gain_multiplier_by_sel, IIO_GTS_HELPER);
+
+/**
+ * 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, NANO, &scale_linear);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, time_sel);
+ if (ret < 0)
+ return ret;
+
+ mul = ret;
+
+ ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);
+ if (ret)
+ return ret;
+
+ if (!iio_gts_valid_gain(gts, *gain))
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_for_scale_using_time, IIO_GTS_HELPER);
+
+/*
+ * 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_NS_GPL(iio_gts_find_gain_sel_for_scale_using_time, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_find_time_and_gain_sel_for_scale, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_find_int_time_by_sel, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_find_sel_by_int_time, IIO_GTS_HELPER);
+
+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_NS_GPL(iio_gts_get_total_gain_by_sel, IIO_GTS_HELPER);
+
+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, NANO, scale_int, scale_nano);
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_get_scale, IIO_GTS_HELPER);
+
+/**
+ * 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_NS_GPL(iio_gts_find_new_gain_sel_by_old_gain_time, IIO_GTS_HELPER);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
new file mode 100644
index 000000000000..4b5a417946f4
--- /dev/null
+++ b/drivers/iio/light/iio-gts-helper.h
@@ -0,0 +1,134 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* gain-time-scale conversion helpers for IIO light sensors
+ *
+ * Copyright (c) 2023 Matti Vaittinen <[email protected]>
+ */
+
+#ifndef __IIO_GTS_HELPER__
+#define __IIO_GTS_HELPER__
+
+#include <linux/types.h>
+
+struct device;
+
+/**
+ * struct iio_gain_sel_pair - gain - selector values
+ *
+ * In many cases devices like light sensors allow setting signal amplification
+ * (gain) using a register interface. This structure describes amplification
+ * and corresponding selector (register value)
+ *
+ * @gain: Gain (multiplication) value.
+ * @sel: Selector (usually register value) used to indicate this gain
+ */
+struct iio_gain_sel_pair {
+ int gain;
+ int sel;
+};
+
+/**
+ * struct iio_itime_sel_mul - integration time description
+ *
+ * In many cases devices like light sensors allow setting the duration of
+ * collecting data. Typically this duration has also an impact to the magnitude
+ * of measured values (gain). This structure describes the relation of
+ * integration time and amplification as well as corresponding selector
+ * (register value).
+ *
+ * An example could be a sensor allowing 50, 100, 200 and 400 mS times. The
+ * respective multiplication values could be 50 mS => 1, 100 mS => 2,
+ * 200 mS => 4 and 400 mS => 8 assuming the impact of integration time would be
+ * linear in a way that when collecting data for 50 mS caused value X, doubling
+ * the data collection time caused value 2X etc..
+ *
+ * @time_us: Integration time in microseconds.
+ * @sel: Selector (usually register value) used to indicate this time
+ * @mul: Multiplication to the values caused by this time.
+ */
+struct iio_itime_sel_mul {
+ int time_us;
+ int sel;
+ int mul;
+};
+
+struct iio_gts {
+ u64 max_scale;
+ const struct iio_gain_sel_pair *hwgain_table;
+ int num_hwgain;
+ const struct iio_itime_sel_mul *itime_table;
+ int num_itime;
+ int **per_time_avail_scale_tables;
+ int *avail_all_scales_table;
+ int num_avail_all_scales;
+ int *avail_time_tables;
+ int num_avail_time_tables;
+};
+
+#define GAIN_SCALE_GAIN(_gain, _sel) \
+{ \
+ .gain = (_gain), \
+ .sel = (_sel), \
+}
+
+#define GAIN_SCALE_ITIME_US(_itime, _sel, _mul) \
+{ \
+ .time_us = (_itime), \
+ .sel = (_sel), \
+ .mul = (_mul), \
+}
+
+int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
+ const struct iio_gain_sel_pair *gain_tbl, int num_gain,
+ const struct iio_itime_sel_mul *tim_tbl, int num_times,
+ struct iio_gts *gts);
+
+bool iio_gts_valid_gain(struct iio_gts *gts, int gain);
+bool iio_gts_valid_time(struct iio_gts *gts, int time_us);
+
+int iio_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_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range);
+int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel);
+int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain);
+int iio_gts_get_min_gain(struct iio_gts *gts);
+int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel);
+int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time);
+
+int iio_gts_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts,
+ int sel);
+int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
+ int *scale_int, int *scale_nano);
+int iio_gts_scale_to_total_gain(struct iio_gts *gts, int scale_int,
+ int scale_nano, int *gain_tot);
+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);
+int iio_gts_build_avail_tables(struct iio_gts *gts);
+int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
+int iio_gts_build_avail_scale_table(struct iio_gts *gts);
+int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts);
+int iio_gts_build_avail_time_table(struct iio_gts *gts);
+int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts);
+void iio_gts_purge_avail_scale_table(struct iio_gts *gts);
+void iio_gts_purge_avail_time_table(struct iio_gts *gts);
+void iio_gts_purge_avail_tables(struct iio_gts *gts);
+int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type,
+ int *length);
+int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
+ int *length);
+int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
+ const int **vals, int *type, int *length);
+
+#endif
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
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]>
---
Changes:
v2 => v3:
- Use namespace for iio-gts-helpers
RFCv1 => v2:
- add tests for available scales/times helpers
- adapt to renamed iio-gts-helpers.h header
---
drivers/iio/test/Kconfig | 15 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-gts.c | 538 ++++++++++++++++++++++++++++++++
3 files changed, 554 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..68655cb41f11
--- /dev/null
+++ b/drivers/iio/test/iio-test-gts.c
@@ -0,0 +1,538 @@
+// 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 <linux/iio/types.h>
+
+#include "../light/iio-gts-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_US(400 * 1000, TEST_TSEL_400, 8),
+ GAIN_SCALE_ITIME_US(200 * 1000, TEST_TSEL_200, 4),
+ GAIN_SCALE_ITIME_US(100 * 1000, TEST_TSEL_100, 2),
+ GAIN_SCALE_ITIME_US(50 * 1000, TEST_TSEL_50, 1),
+#define TIMEGAIN_MAX 8
+};
+#define TOTAL_GAIN_MAX (HWGAIN_MAX * TIMEGAIN_MAX)
+
+static int test_init_iio_gain_scale(struct iio_gts *gts, int max_scale_int,
+ int max_scale_nano)
+{
+ int ret;
+
+ ret = iio_init_iio_gts(max_scale_int, max_scale_nano, gts_test_gains,
+ ARRAY_SIZE(gts_test_gains), gts_test_itimes,
+ ARRAY_SIZE(gts_test_itimes), gts);
+
+ return ret;
+}
+
+static void test_iio_gts_find_gain_for_scale_using_time(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, gain;
+
+ 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 void test_iio_find_closest_gain_low(struct kunit *test)
+{
+ struct iio_gts gts;
+ bool in_range;
+ int ret;
+
+ const struct iio_gain_sel_pair gts_test_gains_gain_low[] = {
+ GAIN_SCALE_GAIN(4, TEST_GSEL_4),
+ GAIN_SCALE_GAIN(16, TEST_GSEL_16),
+ GAIN_SCALE_GAIN(32, TEST_GSEL_32),
+ };
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_find_closest_gain_low(>s, 2, &in_range);
+ KUNIT_EXPECT_EQ(test, 1, ret);
+ KUNIT_EXPECT_EQ(test, true, in_range);
+
+ ret = iio_find_closest_gain_low(>s, 1, &in_range);
+ KUNIT_EXPECT_EQ(test, 1, ret);
+ KUNIT_EXPECT_EQ(test, true, in_range);
+
+ ret = iio_find_closest_gain_low(>s, 4095, &in_range);
+ KUNIT_EXPECT_EQ(test, 2048, ret);
+ KUNIT_EXPECT_EQ(test, true, in_range);
+
+ ret = iio_find_closest_gain_low(>s, 4097, &in_range);
+ KUNIT_EXPECT_EQ(test, 4096, ret);
+ KUNIT_EXPECT_EQ(test, false, in_range);
+
+ ret = iio_init_iio_gts(TEST_SCALE_1X, 0, gts_test_gains_gain_low,
+ ARRAY_SIZE(gts_test_gains_gain_low),
+ gts_test_itimes, ARRAY_SIZE(gts_test_itimes),
+ >s);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_find_closest_gain_low(>s, 3, &in_range);
+ KUNIT_EXPECT_EQ(test, -EINVAL, ret);
+ KUNIT_EXPECT_EQ(test, false, in_range);
+}
+
+static void test_iio_gts_total_gain_to_scale(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret, scale_int, scale_nano;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_gts_total_gain_to_scale(>s, 1, &scale_int, &scale_nano);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
+ KUNIT_EXPECT_EQ(test, 0, scale_nano);
+
+ ret = iio_gts_total_gain_to_scale(>s, 1, &scale_int, &scale_nano);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
+ KUNIT_EXPECT_EQ(test, 0, scale_nano);
+
+ ret = iio_gts_total_gain_to_scale(>s, 4096 * 8, &scale_int,
+ &scale_nano);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 0, scale_int);
+ KUNIT_EXPECT_EQ(test, TEST_SCALE_NANO_4096X8, scale_nano);
+}
+
+static void test_iio_gts_chk_times(struct kunit *test, const int *vals)
+{
+ static const int expected[] = {50000, 100000, 200000, 400000};
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(expected); i++)
+ KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_chk_scales_all(struct kunit *test, struct iio_gts *gts,
+ const int *vals, int len)
+{
+ static const int gains[] = {1, 2, 4, 8, 16, 32, 64, 128, 256, 512,
+ 1024, 2048, 4096, 4096 * 2, 4096 * 4,
+ 4096 * 8};
+
+ int expected[ARRAY_SIZE(gains) * 2];
+ int i, ret;
+ int exp_len = ARRAY_SIZE(gains) * 2;
+
+ KUNIT_EXPECT_EQ(test, exp_len, len);
+ if (len != exp_len)
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(gains); i++) {
+ ret = iio_gts_total_gain_to_scale(gts, gains[i],
+ &expected[2 * i],
+ &expected[2 * i + 1]);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(expected); i++)
+ KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_chk_scales_t200(struct kunit *test, struct iio_gts *gts,
+ const int *vals, int len)
+{
+ /* The gain caused by time 200 is 4x */
+ static const int gains[] = {
+ 1 * 4,
+ 4 * 4,
+ 16 * 4,
+ 32 * 4,
+ 64 * 4,
+ 256 * 4,
+ 512 * 4,
+ 1024 * 4,
+ 2048 * 4,
+ 4096 * 4
+ };
+ int expected[ARRAY_SIZE(gains) * 2];
+ int i, ret;
+
+ KUNIT_EXPECT_EQ(test, 2 * ARRAY_SIZE(gains), len);
+ if (len < 2 * ARRAY_SIZE(gains))
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(gains); i++) {
+ ret = iio_gts_total_gain_to_scale(gts, gains[i],
+ &expected[2 * i],
+ &expected[2 * i + 1]);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(expected); i++)
+ KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
+}
+
+static void test_iio_gts_avail_test(struct kunit *test)
+{
+ struct iio_gts gts;
+ int ret;
+ int type, len;
+ const int *vals;
+
+ ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ ret = iio_gts_build_avail_tables(>s);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ if (ret)
+ return;
+
+ /* test table building for times and iio_gts_avail_times() */
+ ret = iio_gts_avail_times(>s, &vals, &type, &len);
+ KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+ if (ret)
+ return;
+
+ KUNIT_EXPECT_EQ(test, IIO_VAL_INT, type);
+ KUNIT_EXPECT_EQ(test, 4, len);
+ if (len < 4)
+ return;
+
+ test_iio_gts_chk_times(test, vals);
+
+ /* Test table building for all scales and iio_gts_all_avail_scales() */
+ ret = iio_gts_all_avail_scales(>s, &vals, &type, &len);
+ KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+ if (ret)
+ return;
+
+ KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
+
+ test_iio_gts_chk_scales_all(test, >s, vals, len);
+
+ /*
+ * Test table building for scales/time and
+ * iio_gts_avail_scales_for_time()
+ */
+ ret = iio_gts_avail_scales_for_time(>s, 200000, &vals, &type, &len);
+ KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
+ if (ret)
+ return;
+
+ KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
+ test_iio_gts_chk_scales_t200(test, >s, vals, len);
+
+ iio_gts_purge_avail_tables(>s);
+}
+
+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),
+ KUNIT_CASE(test_iio_find_closest_gain_low),
+ KUNIT_CASE(test_iio_gts_total_gain_to_scale),
+ KUNIT_CASE(test_iio_gts_avail_test),
+ {}
+};
+
+static struct kunit_suite iio_gts_test_suite = {
+ .name = "iio-gain-time-scale",
+ .test_cases = iio_gts_test_cases,
+};
+
+kunit_test_suite(iio_gts_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
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]>
---
RFCv1 =>
- No changes
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 135d93368d36..af8516d5df36 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10059,6 +10059,14 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
F: Documentation/devicetree/bindings/iio/adc/envelope-detector.yaml
F: drivers/iio/adc/envelope-detector.c
+IIO LIGHT SENSOR GAIN-TIME-SCALE HELPERS
+M: Matti Vaittinen <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/light/gain-time-scale-helper.c
+F: drivers/iio/light/gain-time-scale-helper.h
+F: drivers/iio/test/iio-test-gts.c
+
IIO MULTIPLEXER
M: Peter Rosin <[email protected]>
L: [email protected]
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
capable of detecting a very wide range of illuminance. Typical application
is adjusting LCD and backlight power of TVs and mobile phones.
Add initial support for the ROHM BU27034 ambient light sensor.
NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the
calculated lux values based on measured data from diodes #0 and
#1. In addition, 3 IIO_INTENSITY channels are emitting the raw
register data from all diodes for more intense user-space
computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 5, 55, 100, 200 and
400 mS. Driver does not support 5 mS which has special
limitations.
- Driver exposes standard 'scale' adjustment which is
implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment alone can't provide requested
scale, adjusting both the time and the gain is
attempted.
- Driver exposes writable INT_TIME property that can be used
for adjusting the measurement time. Time adjustment will also
cause the driver to try to adjust the GAIN so that the
overall scale is kept as close to the original as possible.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Please note: There are few "unfinished" discussions regarding at least:
- PROCESSED channel scale.
- Implementation details when avoiding division by zero.
I have implemented those for this version in a way that I see the best.
It would have been better to wait for discussions to finish - but I will
be away from the computer for a week - so I wanted to send out the v3
with fixes so that people who are interested can do a review while I am
away.
Changes
v2 => v3:
- commit message update and typofixes
- switch warning messages to dbg
- drop incorrect comment about unchanged scales
- return 'no new data' if valid bit read failed
- shorten the 'div by zero' checks
- don't use u32 pointer when int * is epected in lux calculation
- add a comment clarifying why it is safe to return int from lux calculation
- simplify read_raw() by refactoring the measurement start / stop in
another function and dropping the goto based unlocking.
- Styling fixes
- select IIO_BUFFER and IIO_KFIFO_BUF
- Alphabetical order of header includes
- Split multipication w/ overflow check to own function
- Do not hang in read_raw() if sensor does not return valid sample
- Spelling fix
- Do not require fwnode
- Use namespace for gts helpers
RFCv1 => v2:
- (really) protect read-only registers
- fix get and set gain
- buffered mode
- Protect the whole sequences including meas_en/meas_dis to avoid messing
up the enable / disable order
- typofixes / doc improvements
- change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US()
- use more accurate scale for lux channel (milli lux)
- provide available scales / integration times (using helpers).
- adapt to renamed iio-gts-helpers.h file
- bu27034 - longer lines in Kconfig
- Drop bu27034_meas_en and bu27034_meas_dis wrappers.
- Change device-name from bu27034-als to bu27034
---
drivers/iio/light/Kconfig | 14 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/rohm-bu27034.c | 1501 ++++++++++++++++++++++++++++++
3 files changed, 1516 insertions(+)
create mode 100644 drivers/iio/light/rohm-bu27034.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 671d84f98c56..53e6102f7805 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -292,6 +292,20 @@ config JSA1212
To compile this driver as a module, choose M here:
the module will be called jsa1212.
+config ROHM_BU27034
+ tristate "ROHM BU27034 ambient light sensor"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_GTS_HELPER
+ select IIO_BUFFER
+ select IIO_KFIFO_BUF
+ help
+ Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034
+ is an ambient light sesnor with 3 channels and 3 photo diodes capable
+ of detecting a very wide range of illuminance.
+ Typical application is adjusting LCD and backlight power of TVs and
+ mobile phones.
+
config RPR0521
tristate "ROHM RPR0521 ALS and proximity sensor driver"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 82d14ebd3c11..e81c5ce60ccd 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..6135a8a2a4f3
--- /dev/null
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -0,0 +1,1501 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * BU27034 ROHM Ambient Light Sensor
+ *
+ * Copyright (c) 2023, ROHM Semiconductor.
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+
+#include "iio-gts-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
+
+/*
+ * The BU27034 does not have interrupt or any other mechanism of triggering
+ * the data read when measurement has finished. Hence we poll the VALID bit in
+ * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS
+ * milliseconds before the expected sampling time to prevent the drifting. Eg,
+ * If we constantly wake up a bit too late we would eventually skip a sample.
+ * And because the sleep can't wake up _exactly_ at given time this would be
+ * inevitable even if the sensor clock would be perfectly phase-locked to CPU
+ * clock - which we can't say is the case.
+ *
+ * This is still fragile. No matter how big advance do we have, we will still
+ * risk of losing a sample because things can in a rainy-day skenario be
+ * delayed a lot. Yet, more we reserve the time for polling, more we also lose
+ * the performance by spending cycles polling the register. So, selecting this
+ * value is a balancing dance between severity of wasting CPU time and severity
+ * of losing samples.
+ *
+ * In most cases losing the samples is not _that_ crucial because light levels
+ * tend to change slowly.
+ */
+#define BU27034_MEAS_WAIT_PREMATURE_MS 5
+#define BU27034_DATA_WAIT_TIME_US 1000
+#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000)
+
+#define BU27034_RETRY_LIMIT 18
+
+enum {
+ BU27034_CHAN_ALS,
+ BU27034_CHAN_DATA0,
+ BU27034_CHAN_DATA1,
+ BU27034_CHAN_DATA2,
+ BU27034_NUM_CHANS
+};
+
+static const unsigned long bu27034_scan_masks[] = {
+ BIT(BU27034_CHAN_ALS) | BIT(BU27034_CHAN_DATA0) |
+ BIT(BU27034_CHAN_DATA1) | BIT(BU27034_CHAN_DATA2), 0
+};
+
+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X 64
+
+/* See the data sheet for the "Gain Setting" table */
+#define BU27034_GSEL_1X 0x00
+#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 supported by the driver.
+ *
+ * "normal" modes are 55, 100, 200 and 400 mS modes - which do have direct
+ * multiplying impact to the register values (similar to gain).
+ *
+ * This means that if meas-mode is changed for example from 400 => 200,
+ * the scale is doubled. Eg, time impact to total gain is x1, x2, x4, x8.
+ */
+#define BU27034_MEAS_MODE_100MS 0
+#define BU27034_MEAS_MODE_55MS 1
+#define BU27034_MEAS_MODE_200MS 2
+#define BU27034_MEAS_MODE_400MS 4
+
+static const struct iio_itime_sel_mul bu27034_itimes[] = {
+ GAIN_SCALE_ITIME_US(400000, BU27034_MEAS_MODE_400MS, 8),
+ GAIN_SCALE_ITIME_US(200000, BU27034_MEAS_MODE_200MS, 4),
+ GAIN_SCALE_ITIME_US(100000, BU27034_MEAS_MODE_100MS, 2),
+ GAIN_SCALE_ITIME_US(50000, BU27034_MEAS_MODE_55MS, 1),
+};
+
+#define BU27034_CHAN_DATA(_name, _ch2) \
+{ \
+ .type = IIO_INTENSITY, \
+ .channel = BU27034_CHAN_##_name, \
+ .channel2 = (_ch2), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_INT_TIME), \
+ .address = BU27034_REG_##_name##_LO, \
+ .scan_index = BU27034_CHAN_##_name, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
+ .indexed = 1, \
+}
+
+static const struct iio_chan_spec bu27034_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .channel = BU27034_CHAN_ALS,
+ .scan_index = BU27034_CHAN_ALS,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ /*
+ * The BU27034 DATA0 and DATA1 channels are both on the visible light
+ * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
+ * These wave lengths are pretty much on the border of colours making
+ * these a poor candidates for R/G/B standardization. Hence they're both
+ * marked as clear channels
+ */
+ BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
+ BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
+ BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+struct bu27034_data {
+ struct regmap *regmap;
+ struct device *dev;
+ /*
+ * Protect gain and time during scale adjustment and data reading.
+ * Protect measurement enabling/disabling.
+ */
+ struct mutex mutex;
+ struct iio_gts gts;
+ struct task_struct *task;
+ __le16 raw[3];
+ struct {
+ u32 mlux;
+ __le16 channels[3];
+ s64 ts __aligned(8);
+ } scan;
+};
+
+struct bu27034_result {
+ u16 ch0;
+ u16 ch1;
+ u16 ch2;
+};
+
+static const struct regmap_range bu27034_volatile_ranges[] = {
+ {
+ .range_min = BU27034_REG_MODE_CONTROL4,
+ .range_max = BU27034_REG_MODE_CONTROL4,
+ }, {
+ .range_min = BU27034_REG_DATA0_LO,
+ .range_max = BU27034_REG_DATA2_HI,
+ },
+};
+
+static const struct regmap_access_table bu27034_volatile_regs = {
+ .yes_ranges = &bu27034_volatile_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(bu27034_volatile_ranges),
+};
+
+static const struct regmap_range bu27034_read_only_ranges[] = {
+ {
+ .range_min = BU27034_REG_DATA0_LO,
+ .range_max = BU27034_REG_DATA2_HI,
+ }, {
+ .range_min = BU27034_REG_MANUFACTURER_ID,
+ .range_max = BU27034_REG_MANUFACTURER_ID,
+ }
+};
+
+static const struct regmap_access_table bu27034_ro_regs = {
+ .no_ranges = &bu27034_read_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(bu27034_read_only_ranges),
+};
+
+static const struct regmap_config bu27034_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = BU27034_REG_MAX,
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_table = &bu27034_volatile_regs,
+ .wr_table = &bu27034_ro_regs,
+};
+
+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);
+ }
+
+ return -EINVAL;
+}
+
+static int bu27034_get_gain(struct bu27034_data *data, int chan, int *gain)
+{
+ int ret, sel;
+
+ ret = bu27034_get_gain_sel(data, chan);
+ if (ret < 0)
+ return ret;
+
+ sel = ret;
+
+ ret = iio_gts_find_gain_by_sel(&data->gts, sel);
+ if (ret < 0) {
+ dev_err(data->dev, "chan %u: unknown gain value 0x%x\n", chan,
+ sel);
+
+ return ret;
+ }
+
+ *gain = ret;
+
+ return 0;
+}
+
+static int bu27034_get_int_time(struct bu27034_data *data)
+{
+ int ret, sel;
+
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel);
+ if (ret)
+ return ret;
+
+ return iio_gts_find_int_time_by_sel(&data->gts,
+ sel & BU27034_MASK_MEAS_MODE);
+}
+
+static int _bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+ int *val2)
+{
+ int gain, ret;
+
+ ret = bu27034_get_gain(data, channel, &gain);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ return iio_gts_get_scale(&data->gts, gain, ret, val, val2);
+}
+
+static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+ int *val2)
+{
+ int ret;
+
+ if (channel == BU27034_CHAN_ALS) {
+ *val = 0;
+ *val2 = 1000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+
+ mutex_lock(&data->mutex);
+ ret = _bu27034_get_scale(data, channel, val, val2);
+ mutex_unlock(&data->mutex);
+ if (!ret)
+ return IIO_VAL_INT_PLUS_NANO;
+
+ return ret;
+}
+
+/* Caller should hold the lock to protect lux reading */
+static int bu27034_write_gain_sel(struct bu27034_data *data, int chan, int sel)
+{
+ static const int reg[] = {
+ [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+ [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+ [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;
+ }
+
+ 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;
+
+ ret = iio_gts_find_sel_by_gain(&data->gts, gain);
+ if (ret < 0)
+ return ret;
+
+ return bu27034_write_gain_sel(data, chan, ret);
+}
+
+/* Caller should hold the lock to protect data->int_time */
+static int bu27034_set_int_time(struct bu27034_data *data, int time)
+{
+ int ret;
+
+ ret = iio_gts_find_sel_by_int_time(&data->gts, time);
+ if (ret < 0)
+ return ret;
+
+ return regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
+ BU27034_MASK_MEAS_MODE, ret);
+}
+
+/*
+ * We try to change the time in such way that the scale is maintained for
+ * given channels by adjusting gain so that it compensates the time change.
+ */
+static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
+{
+ 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;
+ bool ok;
+
+ _bu27034_get_scale(data, gains[i].chan, &scale1, &scale2);
+ dev_dbg(data->dev,
+ "chan %u, can't support time %u with scale %u %u\n",
+ gains[i].chan, time_us, scale1, scale2);
+
+ /*
+ * If caller requests for integration time change and we
+ * can't support the scale - then the caller should be
+ * prepared to 'pick up the pieces and deal with the
+ * fact that the scale changed'.
+ */
+ ret = iio_find_closest_gain_low(&data->gts,
+ gains[i].new_gain, &ok);
+
+ if (!ok) {
+ dev_dbg(data->dev,
+ "optimal gain out of range for chan %u\n",
+ gains[i].chan);
+ }
+ if (ret < 0) {
+ dev_dbg(data->dev,
+ "Total gain increase. Risk of saturation");
+ ret = iio_gts_get_min_gain(&data->gts);
+ if (ret < 0)
+ goto unlock_out;
+ }
+ dev_dbg(data->dev, "chan %u scale changed\n",
+ gains[i].chan);
+ gains[i].new_gain = ret;
+ dev_dbg(data->dev, "chan %u new gain %u\n",
+ gains[i].chan, gains[i].new_gain);
+ }
+ }
+
+ for (i = 0; i < numg; i++) {
+ ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
+ if (ret)
+ goto unlock_out;
+ }
+
+ ret = bu27034_set_int_time(data, 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;
+
+ if (chan == BU27034_CHAN_ALS) {
+ if (val == 0 && val2 == 1000)
+ return 0;
+
+ return -EINVAL;
+ }
+
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel);
+ if (ret)
+ goto unlock_out;
+
+ ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
+ val, val2 * 1000, &gain_sel);
+ if (ret) {
+ /*
+ * Could not support scale with given time. Need to change time.
+ * We still want to maintain the scale for all channels
+ */
+ int new_time_sel;
+ struct bu27034_gain_check gains[2];
+
+ /*
+ * Populate information for the two other channels which should
+ * maintain the scale.
+ */
+ 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;
+ }
+
+ /*
+ * Iterate through all the times to see if we find one which
+ * can support requested scale for requested channel, while
+ * maintaining the scale for other channels
+ */
+ for (i = 0; i < data->gts.num_itime; i++) {
+ new_time_sel = data->gts.itime_table[i].sel;
+
+ if (new_time_sel == time_sel)
+ continue;
+
+ /* Can we provide requested scale with this time? */
+ ret = iio_gts_find_gain_sel_for_scale_using_time(
+ &data->gts, new_time_sel, val, val2 * 1000,
+ &gain_sel);
+ if (ret)
+ continue;
+
+ /* Can the two other channels maintain scales? */
+ 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) {
+ /* Yes - we found suitable time */
+ found = true;
+ break;
+ }
+ }
+ if (!found) {
+ dev_dbg(data->dev,
+ "Can't set scale maintaining other channels\n");
+ ret = -EINVAL;
+
+ goto unlock_out;
+ }
+
+ for (i = 0; i < 2; i++) {
+ ret = bu27034_set_gain(data, gains[i].chan,
+ gains[i].new_gain);
+ if (ret)
+ goto unlock_out;
+ }
+
+ ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
+ BU27034_MASK_MEAS_MODE, new_time_sel);
+ if (ret)
+ goto unlock_out;
+ }
+
+ ret = bu27034_write_gain_sel(data, chan, gain_sel);
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+/*
+ * for (D1/D0 < 0.87):
+ * lx = 0.004521097 * D1 - 0.002663996 * D0 +
+ * 0.00012213 * D1 * D1 / D0
+ *
+ * => 115.7400832 * ch1 / gain1 / mt -
+ * 68.1982976 * ch0 / gain0 / mt +
+ * 0.00012213 * 25600 * (ch1 / gain1 / mt) * 25600 *
+ * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ *
+ * A = 0.00012213 * 25600 * (ch1 /gain1 / mt) * 25600 *
+ * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ * => 0.00012213 * 25600 * (ch1 /gain1 / mt) *
+ * (ch1 /gain1 / mt) / (ch0 / gain0 / mt)
+ * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) /
+ * (ch0 / gain0)
+ * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) *
+ * gain0 / ch0
+ * => 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /ch0
+ *
+ * lx = (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ * mt + A
+ * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
+ * mt + 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /
+ * ch0
+ *
+ * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0 +
+ * 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) /
+ * mt
+ *
+ * For (0.87 <= D1/D0 < 1.00)
+ * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 0.87) * (0.385) + 1)
+ * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ * 100 * ch1 / gain1 / mt) * ((D1/D0 - 0.87) * (0.385) + 1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * ((D1/D0 - 0.87) * (0.385) + 1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (0.385 * D1/D0 - 0.66505)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (0.385 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) - 0.66505)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (9856 * ch1 / gain1 / mt / (25600 * ch0 / gain0 / mt) + 0.66505)
+ * => 13.118336 * ch1 / (gain1 * mt)
+ * + 22.66064768 * ch0 / (gain0 * mt)
+ * + 8931.90144 * ch1 * ch1 * gain0 /
+ * (25600 * ch0 * gain1 * gain1 * mt)
+ * + 0.602694912 * ch1 / (gain1 * mt)
+ *
+ * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1)
+ * + 22.66064768 * ch0 / gain0
+ * + 13.721030912 * ch1 / gain1
+ * ] / mt
+ *
+ * For (D1/D0 >= 1.00)
+ *
+ * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 2.0) * (-0.05) + 1)
+ * => (0.001331* D0 + 0.0000354 * D1) * (-0.05D1/D0 + 1.1)
+ * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
+ * 100 * ch1 / gain1 / mt) * (-0.05D1/D0 + 1.1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (-0.05 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) + 1.1)
+ * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
+ * (-1280 * ch1 / (gain1 * mt * 25600 * ch0 / gain0 / mt) + 1.1)
+ * => (34.0736 * ch0 * -1280 * ch1 * gain0 * mt /( gain0 * mt * gain1 * mt * 25600 * ch0)
+ * + 34.0736 * 1.1 * ch0 / (gain0 * mt)
+ * + 0.90624 * ch1 * -1280 * ch1 *gain0 * mt / (gain1 * mt *gain1 * mt * 25600 * ch0)
+ * + 1.1 * 0.90624 * ch1 / (gain1 * mt)
+ * => -43614.208 * ch1 / (gain1 * mt * 25600)
+ * + 37.48096 ch0 / (gain0 * mt)
+ * - 1159.9872 * ch1 * ch1 * gain0 / (gain1 * gain1 * mt * 25600 * ch0)
+ * + 0.996864 ch1 / (gain1 * mt)
+ * => [
+ * - 0.045312 * ch1 * ch1 * gain0 / (gain1 * gain1 * ch0)
+ * - 0.706816 * ch1 / gain1
+ * + 37.48096 ch0 /gain0
+ * ] * mt
+ *
+ *
+ * So, the first case (D1/D0 < 0.87) can be computed to a form:
+ *
+ * lx = (3.126528 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * 115.7400832 * ch1 / gain1 +
+ * -68.1982976 * ch0 / gain0
+ * / mt
+ *
+ * Second case (0.87 <= D1/D0 < 1.00) goes to form:
+ *
+ * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * 13.721030912 * ch1 / gain1 +
+ * 22.66064768 * ch0 / gain0
+ * ] / mt
+ *
+ * Third case (D1/D0 >= 1.00) goes to form:
+ * => [-0.045312 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * -0.706816 * ch1 / gain1 +
+ * 37.48096 ch0 /(gain0
+ * ] / mt
+ *
+ * This can be unified to format:
+ * lx = [
+ * A * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
+ * B * ch1 / gain1 +
+ * C * ch0 / gain0
+ * ] / mt
+ *
+ * For case 1:
+ * A = 3.126528,
+ * B = 115.7400832
+ * C = -68.1982976
+ *
+ * For case 2:
+ * A = 0.3489024
+ * B = 13.721030912
+ * C = 22.66064768
+ *
+ * For case 3:
+ * A = -0.045312
+ * B = -0.706816
+ * C = 37.48096
+ */
+
+struct bu27034_lx_coeff {
+ unsigned int A;
+ unsigned int B;
+ unsigned int C;
+ /* Indicate which of the coefficients above are negative */
+ bool is_neg[3];
+};
+
+static inline u64 gain_mul_div_helper(u64 val, unsigned int gain,
+ unsigned int div)
+{
+ /*
+ * Max gain for a channel is 4096. The max u64 (0xffffffffffffffffULL)
+ * divided by 4096 is 0xFFFFFFFFFFFFF (GENMASK_ULL(51, 0)) (floored).
+ * Thus, the 0xFFFFFFFFFFFFF is the largest value we can safely multiply
+ * with the gain, no matter what gain is set.
+ *
+ * So, multiplication with max gain may overflow if val is greater than
+ * 0xFFFFFFFFFFFFF (52 bits set)..
+ *
+ * If this is the case we divide first.
+ */
+ if (val < GENMASK_ULL(51, 0)) {
+ val *= gain;
+ do_div(val, div);
+ } else {
+ do_div(val, div);
+ val *= gain;
+ }
+
+ return val;
+}
+
+static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
+ unsigned int ch1, unsigned int gain0,
+ unsigned int gain1)
+{
+ unsigned int helper, tmp;
+ u64 helper64;
+
+ /*
+ * Here we could overflow even the 64bit value. Hence we
+ * multiply with gain0 only after the divisions - even though
+ * it may result loss of accuracy
+ */
+ helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
+ helper = coeff * ch1 * ch1;
+ tmp = helper * gain0;
+
+ if (helper == helper64 && (tmp / gain0 == helper))
+ return tmp / (gain1 * gain1) / ch0;
+
+ helper = gain1 * gain1;
+ if (helper > ch0) {
+ do_div(helper64, helper);
+
+ return gain_mul_div_helper(helper64, gain0, ch0);
+ }
+
+ do_div(helper64, ch0);
+
+ return gain_mul_div_helper(helper64, gain0, helper);
+}
+
+static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
+ unsigned int gain)
+{
+ unsigned int helper;
+ u64 helper64;
+
+ helper64 = (u64)coeff * (u64)ch;
+ helper = coeff * ch;
+
+ if (helper == helper64)
+ return helper / gain;
+
+ do_div(helper64, gain);
+
+ return helper64;
+}
+
+static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1,
+ unsigned int gain0, unsigned int gain1,
+ unsigned int meastime, int coeff_idx)
+{
+ static const struct bu27034_lx_coeff coeff[] = {
+ {
+ .A = 31265280, /* 3.126528 */
+ .B = 1157400832, /*115.7400832 */
+ .C = 681982976, /* -68.1982976 */
+ .is_neg = {false, false, true},
+ }, {
+ .A = 3489024, /* 0.3489024 */
+ .B = 137210309, /* 13.721030912 */
+ .C = 226606476, /* 22.66064768 */
+ /* All terms positive */
+ }, {
+ .A = 453120, /* -0.045312 */
+ .B = 7068160, /* -0.706816 */
+ .C = 374809600, /* 37.48096 */
+ .is_neg = {true, true, false},
+ }
+ };
+ const struct bu27034_lx_coeff *c = &coeff[coeff_idx];
+ u64 res = 0, terms[3];
+ int i;
+
+ if (coeff_idx >= ARRAY_SIZE(coeff))
+ return -EINVAL;
+
+ terms[0] = bu27034_fixp_calc_t1(c->A, ch0, ch1, gain0, gain1);
+ terms[1] = bu27034_fixp_calc_t23(c->B, ch1, gain1);
+ terms[2] = bu27034_fixp_calc_t23(c->C, ch0, gain0);
+
+ /* First, add positive terms */
+ for (i = 0; i < 3; i++)
+ if (!c->is_neg[i])
+ res += terms[i];
+
+ /* No positive term => zero lux */
+ if (!res)
+ return 0;
+
+ /* Then, subtract negative terms (if any) */
+ for (i = 0; i < 3; i++)
+ if (c->is_neg[i]) {
+ /*
+ * If the negative term is greater than positive - then
+ * the darknes has taken over and we are all doomed! Eh,
+ * I mean, then we can just return 0 lx and go out
+ */
+ if (terms[i] >= res)
+ return 0;
+
+ res -= terms[i];
+ }
+
+ meastime *= 10;
+ do_div(res, meastime);
+
+ return (int) res;
+}
+
+static bool bu27034_has_valid_sample(struct bu27034_data *data)
+{
+ int ret, val;
+
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL4, &val);
+ if (ret) {
+ dev_err(data->dev, "Read failed %d\n", ret);
+
+ return false;
+ }
+
+ return val & BU27034_MASK_VALID;
+}
+
+/*
+ * Reading the register where VALID bit is clears this bit. (So does changing
+ * any gain / integration time configuration registers) The bit gets
+ * set when we have acquired new data. We use this bit to indicate data
+ * validity.
+ */
+static void bu27034_invalidate_read_data(struct bu27034_data *data)
+{
+ bu27034_has_valid_sample(data);
+}
+
+static int bu27034_read_result(struct bu27034_data *data, int chan, int *res)
+{
+ int reg[] = {
+ [BU27034_CHAN_DATA0] = BU27034_REG_DATA0_LO,
+ [BU27034_CHAN_DATA1] = BU27034_REG_DATA1_LO,
+ [BU27034_CHAN_DATA2] = BU27034_REG_DATA2_LO,
+ };
+ int valid, ret;
+ __le16 val;
+
+ ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
+ valid, (valid & BU27034_MASK_VALID),
+ BU27034_DATA_WAIT_TIME_US, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_bulk_read(data->regmap, reg[chan], &val, sizeof(val));
+ if (ret)
+ return ret;
+
+ *res = le16_to_cpu(val);
+
+ return 0;
+}
+
+static int bu27034_get_result_unlocked(struct bu27034_data *data, __le16 *res,
+ int size)
+{
+ int ret = 0, retry_cnt = 0;
+
+retry:
+ /* Get new value from sensor if data is ready */
+ if (bu27034_has_valid_sample(data)) {
+ ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+ res, size);
+ if (ret)
+ return ret;
+
+ bu27034_invalidate_read_data(data);
+ } else {
+ retry_cnt++;
+
+ /* No new data in sensor. Wait and retry */
+ msleep(25);
+
+ if (retry_cnt > BU27034_RETRY_LIMIT) {
+ dev_err(data->dev, "No data from sensor\n");
+
+ return -ETIMEDOUT;
+ }
+
+ goto retry;
+ }
+
+ return ret;
+}
+
+static int bu27034_meas_set(struct bu27034_data *data, bool en)
+{
+ if (en)
+ return regmap_set_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+ BU27034_MASK_MEAS_EN);
+
+ return regmap_clear_bits(data->regmap, BU27034_REG_MODE_CONTROL4,
+ BU27034_MASK_MEAS_EN);
+}
+
+static int bu27034_get_single_result(struct bu27034_data *data, int chan,
+ int *val)
+{
+ int ret;
+
+ ret = bu27034_meas_set(data, true);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ msleep(ret / 1000);
+
+ return bu27034_read_result(data, chan, val);
+}
+
+/*
+ * The formula given by vendor for computing luxes out of data0 and data1
+ * (in open air) is as follows:
+ *
+ * Let's mark:
+ * D0 = data0/ch0_gain/meas_time_ms * 25600
+ * D1 = data1/ch1_gain/meas_time_ms * 25600
+ *
+ * Then:
+ * if (D1/D0 < 0.87)
+ * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1)
+ * else if (D1/D0 < 1)
+ * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1)
+ * else
+ * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1)
+ *
+ * We use it here. Users who have for example some colored lens
+ * need to modify the calculation but I hope this gives a starting point for
+ * those working with such devices.
+ */
+
+static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
+{
+ unsigned int gain0, gain1, meastime;
+ unsigned int d1_d0_ratio_scaled;
+ u16 ch0, ch1;
+ u64 helper64;
+ int ret;
+
+ /*
+ * We return 0 luxes if calculation fails. This should be reasonably
+ * easy to spot from the buffers especially if raw-data channels show
+ * valid values
+ */
+ *val = 0;
+
+ /* Avoid div by zero. */
+ if (!res[0])
+ ch0 = 1;
+ else
+ ch0 = le16_to_cpu(res[0]);
+
+ if (!res[1])
+ ch1 = 1;
+ else
+ ch1 = le16_to_cpu(res[1]);
+
+ ret = bu27034_get_gain(data, BU27034_CHAN_DATA0, &gain0);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_gain(data, BU27034_CHAN_DATA1, &gain1);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ meastime = ret;
+
+ d1_d0_ratio_scaled = (unsigned int)ch1 * (unsigned int)gain0 * 100;
+ helper64 = (u64)ch1 * (u64)gain0 * 100LLU;
+
+ if (helper64 != d1_d0_ratio_scaled) {
+ unsigned int div = (unsigned int)ch0 * gain1;
+
+ do_div(helper64, div);
+ d1_d0_ratio_scaled = helper64;
+ } else {
+ d1_d0_ratio_scaled /= ch0 * gain1;
+ }
+
+ if (d1_d0_ratio_scaled < 87)
+ ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0);
+ else if (d1_d0_ratio_scaled < 100)
+ ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1);
+ else
+ ret = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2);
+
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+
+}
+
+static int bu27034_get_mlux(struct bu27034_data *data, int *val)
+{
+ __le16 res[3];
+ int ret;
+
+ ret = bu27034_meas_set(data, true);
+ if (ret)
+ return ret;
+
+ ret = bu27034_get_result_unlocked(data, &res[0], sizeof(res));
+ if (ret)
+ return ret;
+
+ ret = bu27034_calc_mlux(data, res, val);
+ if (ret)
+ return ret;
+
+ ret = bu27034_meas_set(data, false);
+ if (ret)
+ dev_err(data->dev, "failed to disable measurement\n");
+
+ return 0;
+}
+
+static int bu27034_read_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = bu27034_get_int_time(data);
+ if (*val < 0)
+ return *val;
+
+ /*
+ * We use 50000 uS internally for all calculations and only
+ * convert it to 55000 before returning it to the user.
+ *
+ * This is because the data-sheet says the time is 55 mS - but
+ * vendor provided computations used 50 mS.
+ */
+ if (*val == 50000)
+ *val = 55000;
+
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ return bu27034_get_scale(data, chan->channel, val, val2);
+
+ case IIO_CHAN_INFO_RAW:
+ {
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+
+ if (chan->channel < BU27034_CHAN_DATA0 ||
+ chan->channel > BU27034_CHAN_DATA2)
+ return -EINVAL;
+
+ /* Don't mess with measurement enabling while buffering */
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&data->mutex);
+ /*
+ * Reading one channel at a time is ineffiecient but we don't
+ * care here. Buffered version should be used if performance is
+ * an issue.
+ */
+ ret = bu27034_get_single_result(data, chan->channel, val);
+
+ mutex_unlock(&data->mutex);
+ iio_device_release_direct_mode(idev);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ }
+
+ case IIO_CHAN_INFO_PROCESSED:
+ if (chan->type != IIO_LIGHT)
+ return -EINVAL;
+
+ /* Don't mess with measurement enabling while buffering */
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&data->mutex);
+
+ ret = bu27034_get_mlux(data, val);
+
+ mutex_unlock(&data->mutex);
+ iio_device_release_direct_mode(idev);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+
+ }
+
+ return ret;
+}
+
+static int bu27034_write_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ int ret;
+
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ ret = bu27034_set_scale(data, chan->channel, val, val2);
+ break;
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = bu27034_try_set_int_time(data, val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ iio_device_release_direct_mode(idev);
+
+ return ret;
+}
+
+static int bu27034_read_avail(struct iio_dev *idev,
+ struct iio_chan_spec const *chan, const int **vals,
+ int *type, int *length, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ return iio_gts_avail_times(&data->gts, vals, type, length);
+ case IIO_CHAN_INFO_SCALE:
+ return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info bu27034_info = {
+ .read_raw = &bu27034_read_raw,
+ .write_raw = &bu27034_write_raw,
+ .read_avail = &bu27034_read_avail,
+};
+
+static int bu27034_chip_init(struct bu27034_data *data)
+{
+ int ret, sel;
+
+ /* Reset */
+ ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+ BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+ msleep(1);
+ /*
+ * Read integration time here to ensure it is in regmap cache. We do
+ * this to speed-up the int-time acquisition in the start of the buffer
+ * handling thread where longer delays could make it more likely we end
+ * up skipping a sample, and where the longer delays make timestamps
+ * less accurate.
+ */
+ ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel);
+ if (ret)
+ dev_err(data->dev, "reading integration time failed\n");
+
+ return 0;
+}
+
+static int bu27034_wait_for_data(struct bu27034_data *data)
+{
+ int ret, val;
+
+ ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
+ val, val & BU27034_MASK_VALID,
+ BU27034_DATA_WAIT_TIME_US,
+ BU27034_TOTAL_DATA_WAIT_TIME_US);
+ if (ret) {
+ dev_err(data->dev, "data polling %s\n",
+ !(val & BU27034_MASK_VALID) ? "timeout" : "fail");
+ return ret;
+ }
+ ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
+ &data->scan.channels[0],
+ sizeof(data->scan.channels));
+ bu27034_invalidate_read_data(data);
+
+ return ret;
+}
+
+static int bu27034_buffer_thread(void *arg)
+{
+ struct iio_dev *idev = arg;
+ struct bu27034_data *data;
+ int wait_ms;
+
+ data = iio_priv(idev);
+
+ wait_ms = bu27034_get_int_time(data);
+ wait_ms /= 1000;
+
+ wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
+
+ while (!kthread_should_stop()) {
+ int ret;
+ int64_t tstamp;
+
+ msleep(wait_ms);
+ ret = bu27034_wait_for_data(data);
+ if (ret)
+ continue;
+
+ tstamp = iio_get_time_ns(idev);
+
+ if (*idev->active_scan_mask & BIT(BU27034_CHAN_ALS)) {
+ int mlux;
+
+ ret = bu27034_calc_mlux(data, &data->scan.channels[0],
+ &mlux);
+ if (ret)
+ dev_err(data->dev, "failed to calculate lux\n");
+
+ /*
+ * The maximum milli lux value we get with gain 1x time
+ * 55mS data ch0 = 0xffff ch1 = 0xffff fits in 26 bits
+ * so there should be no problem returning int from
+ * computations and casting it to u32
+ */
+ data->scan.mlux = (u32)mlux;
+ }
+ iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+ }
+
+ return 0;
+}
+
+static int bu27034_buffer_enable(struct iio_dev *idev)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ struct task_struct *task;
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = bu27034_meas_set(data, true);
+ if (ret)
+ goto unlock_out;
+
+ task = kthread_run(bu27034_buffer_thread, idev,
+ "bu27034-buffering-%u",
+ iio_device_id(idev));
+ if (IS_ERR(task)) {
+ ret = PTR_ERR(task);
+ goto unlock_out;
+ }
+
+ data->task = task;
+
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27034_buffer_disable(struct iio_dev *idev)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ if (data->task) {
+ kthread_stop(data->task);
+ data->task = NULL;
+ }
+
+ ret = bu27034_meas_set(data, false);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_buffer_setup_ops bu27034_buffer_ops = {
+ .postenable = &bu27034_buffer_enable,
+ .predisable = &bu27034_buffer_disable,
+};
+
+static int bu27034_probe(struct i2c_client *i2c)
+{
+ struct device *dev = &i2c->dev;
+ struct bu27034_data *data;
+ struct regmap *regmap;
+ struct iio_dev *idev;
+ unsigned int part_id, reg;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize Regmap\n");
+
+ idev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!idev)
+ return -ENOMEM;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret && 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, ®);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+ part_id = FIELD_GET(BU27034_MASK_PART_ID, reg);
+
+ if (part_id != BU27034_ID)
+ dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+ ret = iio_init_iio_gts(BU27034_SCALE_1X, 0, bu27034_gains,
+ ARRAY_SIZE(bu27034_gains), bu27034_itimes,
+ ARRAY_SIZE(bu27034_itimes), &data->gts);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_gts_build_avail_tables(dev, &data->gts);
+ if (ret)
+ return ret;
+
+ mutex_init(&data->mutex);
+ data->regmap = regmap;
+ data->dev = dev;
+
+ idev->channels = bu27034_channels;
+ idev->num_channels = ARRAY_SIZE(bu27034_channels);
+ idev->name = "bu27034";
+ idev->info = &bu27034_info;
+
+ idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+ idev->available_scan_masks = bu27034_scan_masks;
+
+ ret = bu27034_chip_init(data);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops);
+ if (ret)
+ return dev_err_probe(dev, ret, "buffer setup failed\n");
+
+ ret = devm_iio_device_register(dev, idev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Unable to register iio device\n");
+
+ return ret;
+}
+
+static const struct of_device_id bu27034_of_match[] = {
+ { .compatible = "rohm,bu27034" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bu27034_of_match);
+
+static struct i2c_driver bu27034_i2c_driver = {
+ .driver = {
+ .name = "bu27034-als",
+ .of_match_table = bu27034_of_match,
+ },
+ .probe_new = bu27034_probe,
+};
+module_i2c_driver(bu27034_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("ROHM BU27034 ambient light sensor driver");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Add myself as a maintainer for ROHM BU27034 ALS driver.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Changes
v2 =>
- No changes
sRFCv1 => v2:
- Add iio-list
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index af8516d5df36..f75b38e6052d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18102,6 +18102,12 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/light/bh1750.yaml
F: drivers/iio/light/bh1750.c
+ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
+M: Matti Vaittinen <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/iio/light/rohm-bu27034.c
+
ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
M: Marek Vasut <[email protected]>
L: [email protected]
--
2.39.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Mon, Mar 06, 2023 at 11:15:10AM +0200, Matti Vaittinen 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 inversely proportional relation to the
> sensor's intensity channel scale.
>
> Many users only set the scale, which means that many drivers attempt to
> 'guess' the best gain+time combination to meet the scale. Usually this
> is the biggest integration time which allows setting the requested
> scale. Typically, increasing the integration time has better accuracy
> than increasing the gain, which often amplifies the noise as well as the
> real signal.
>
> However, there may be cases where more responsive sensors are needed.
> So, in some cases the longest integration times may not be what the user
> prefers. The driver has no way of knowing this.
>
> Hence, the approach taken by this series is to allow user to set both
> the scale and the integration time with following logic:
>
> 1. When scale is set, the existing integration time is tried to be
> maintained as a first priority.
> 1a) If the requested scale can't be met by current time, then also
> other time + gain combinations are searched. If scale can be met
> by some other integration time, then the new time may be applied.
> If the time setting is common for all channels, then also other
> channels must be able to maintain their scale with this new time
> (by changing their gain). The new times are scanned in the order
> of preference (typically the longest times first).
> 1b) If the requested scale can be met using current time, then only
> the gain for the channel is changed.
>
> 2. When the integration time change - scale is tried to be maintained.
> When integration time change is requested also gain for all impacted
> channels is adjusted so that the scale is not changed, or is chaned
> as little as possible. This is different from the RFCv1 where the
> request was rejected if suitable gain couldn't be found for some
> channel(s).
>
> This logic is simple. When total gain (either caused by time or hw-gain)
> is doubled, the scale gets halved. Also, the supported times are given a
> 'multiplier' value which tells how much they increase the total gain.
>
> However, when I wrote this logic in bu27034 driver, I made quite a few
> errors on the way - and driver got pretty big. As I am writing drivers
> for two other sensors (RGB C/IR + flicker BU27010 and RGB C/IR BU27008)
> with similar gain-time-scale logic I thought that adding common helpers
> for these computations might be wise. I hope this way all the bugs will
> be concentrated in one place and not in every individual driver ;)
>
> Hence, this series also intriduces IIO gain-time-scale helpers
> (abbreviated as gts-helpers) + a couple of KUnit tests for the most
> hairy parts.
>
> 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.
>
> Finally, these added helpers do provide some value also for drivers
> which only:
> a) allow gain change
> or
> b) allow changing both the time and gain but so that the time-change is
> not reflected in register values.
>
> For a) we provide the gain - selector (register value) table format +
> selector to gain look-ups, gain <-> scale conversions and the available
> scales helpers.
>
> For latter case we also provide the time-tables, and actually all the
> APIs should be usable by setting the time multiplier to 1. (not testeted
> thoroughly though).
A few comments can still be applied here.
Can you comment on the discussion against the previous version?
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> Some light sensors can adjust both the HW-gain and integration time.
> There are cases where adjusting the integration time has similar impact
> to the scale of the reported values as gain setting has.
>
> IIO users do typically expect to handle scale by a single writable 'scale'
> entry. Driver should then adjust the gain/time accordingly.
>
> It however is difficult for a driver to know whether it should change
> gain or integration time to meet the requested scale. Usually it is
> preferred to have longer integration time which usually improves
> accuracy, but there may be use-cases where long measurement times can be
> an issue. Thus it can be preferable to allow also changing the
> integration time - but mitigate the scale impact by also changing the gain
> underneath. Eg, if integration time change doubles the measured values,
> the driver can reduce the HW-gain to half.
>
> The theory of the computations of gain-time-scale is simple. However,
> some people (undersigned) got that implemented wrong for more than once.
>
> Add some gain-time-scale helpers in order to not dublicate errors in all
> drivers needing these computations.
...
> +/*
If it's deliberately not a kernel doc, why to bother to have it looking as one?
It's really a provocative to some people who will come with a patches to "fix"
this...
> + * iio_gts_get_gain - Convert scale to total gain
> + *
> + * Internal helper for converting scale to total gain.
> + *
> + * @max: Maximum linearized scale. As an example, when scale is created
> + * in magnitude of NANOs and max scale is 64.1 - The linearized
> + * scale is 64 100 000 000.
> + * @scale: Linearized scale to compte the gain for.
> + *
> + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
> + * is invalid.
> + */
> +static int iio_gts_get_gain(const u64 max, const u64 scale)
> +{
> + int tmp = 1;
> +
> + if (scale > max || !scale)
> + return -EINVAL;
> +
> + if (U64_MAX - max < scale) {
> + /* Risk of overflow */
> + if (max - scale < scale)
> + return 1;
> + while (max - scale > scale * (u64)tmp)
> + tmp++;
> +
> + return tmp + 1;
Can you wait for the comments to appear a bit longer, please?
I have answered to your query in the previous discussion.
> + }
> +
> + while (max > scale * (u64) tmp)
No space for castings?
> + tmp++;
> +
> + return tmp;
> +}
...
> + /*
> + * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
> + * multiplication followed by division to avoid overflow
Missing period.
> + */
> + if (scaler > NANO || !scaler)
> + return -EINVAL;
Shouldn't be OVERFLOW for the first one?
...
> + *lin_scale = (u64) scale_whole * (u64)scaler +
No space for casting?
> + (u64)(scale_nano / (NANO / scaler));
...
> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
I would say _HELPER part is too much, but fine with me.
...
> + ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
> + >s->max_scale);
> + if (ret)
> + return ret;
> +
> + gts->hwgain_table = gain_tbl;
> + gts->num_hwgain = num_gain;
> + gts->itime_table = tim_tbl;
> + gts->num_itime = num_times;
> + gts->per_time_avail_scale_tables = NULL;
> + gts->avail_time_tables = NULL;
> + gts->avail_all_scales_table = NULL;
> + gts->num_avail_all_scales = 0;
Just wondering why we can't simply
memset(0)
beforehand and drop all these 0 assignments?
...
> + /*
> + * Sort the tables for nice output and for easier finding of
> + * unique values
Missing period. Please, check the style of multi-line comments. I believe it's
even mentioned in the documentation.
> + */
...
> + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
> + NULL);
One line reads better?
...
> + if (ret && gts->avail_all_scales_table)
In one case you commented that free(NULL) is okay, in the other, you add
a duplicative check. Why?
> + kfree(gts->avail_all_scales_table);
...
> + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
sizeof(type) is error prone in comparison to sizeof(*var).
> + if (!per_time_gains)
> + return ret;
> +
> + per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
Ditto.
> + if (!per_time_scales)
> + goto free_gains;
...
> +err_free_out:
> + while (i) {
> + /*
> + * It does not matter if i'th alloc was not succesfull as
> + * kfree(NULL) is safe.
> + */
Instead, can be just a free of the known allocated i:th member first followed
by traditional pattern. In that case comment will become redundant.
> + kfree(per_time_scales[i]);
> + kfree(per_time_gains[i]);
> +
> + i--;
> + }
...
> + for (i = gts->num_itime - 1; i >= 0; i--) {
while (i--) {
makes it easier to parse.
> +/**
> + * iio_gts_all_avail_scales - helper for listing all available scales
> + * @gts: Gain time scale descriptor
> + * @vals: Returned array of supported scales
> + * @type: Type of returned scale values
> + * @length: Amount of returned values in array
> + *
> + * Returns a value suitable to be returned from read_avail or a negative error
Missing a return section. Have you run kernel doc to validate this?
Missing period.
Seems these problems occur in many function descriptions.
> + */
...
> + /*
> + * Using this function prior building the tables is a driver-error
> + * which should be fixed when the driver is tested for a first time
Missing period.
> + */
> + if (WARN_ON(!gts->num_avail_all_scales))
Does this justify panic? Note, that any WARN() can become an Oops followed by
panic and reboot.
> + return -EINVAL;
...
> + for (i = 0; i < gts->num_hwgain; i++) {
> + /*
> + * It is not expected this function is called for an exactly
> + * matching gain.
> + */
> + if (unlikely(gain == gts->hwgain_table[i].gain)) {
> + *in_range = true;
> + return gain;
> + }
> + if (!min)
> + min = gts->hwgain_table[i].gain;
> + else
> + min = min(min, gts->hwgain_table[i].gain);
I was staring at this and have got no clue why it's not a dead code.
> + if (gain > gts->hwgain_table[i].gain) {
> + if (!diff) {
> + diff = gain - gts->hwgain_table[i].gain;
> + best = i;
> + } else {
> + int tmp = gain - gts->hwgain_table[i].gain;
> +
> + if (tmp < diff) {
> + diff = tmp;
> + best = i;
> + }
> + }
int tmp = gain - gts->hwgain_table[i].gain;
if (!diff || tmp < diff) {
diff = tmp;
best = i;
}
?
Or did you miss using 'min'? (And I'm wondering how variable name and min()
macro are not conflicting with each other.
> + } else {
> + /*
> + * We found valid hwgain which is greater than
> + * reference. So, unless we return a failure below we
> + * will have found an in-range gain
> + */
> + *in_range = true;
> + }
> + }
> + /* The requested gain was smaller than anything we support */
> + if (!diff) {
> + *in_range = false;
> +
> + return -EINVAL;
> + }
> +
> + return gts->hwgain_table[best].gain;
...
> + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
> + &scale);
Still can be one line.
> + if (ret)
> + return ret;
> +
> + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
> + new_gain);
Ditto.
> + if (ret)
> + return -EINVAL;
...
> +++ b/drivers/iio/light/iio-gts-helper.h
Is it _only_ for a Light type of sensors?
...
> +#ifndef __IIO_GTS_HELPER__
> +#define __IIO_GTS_HELPER__
If yes, perhaps adding LIGHT here?
--
With Best Regards,
Andy Shevchenko
On Mon, 6 Mar 2023 14:52:57 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> > Some light sensors can adjust both the HW-gain and integration time.
> > There are cases where adjusting the integration time has similar impact
> > to the scale of the reported values as gain setting has.
> >
> > IIO users do typically expect to handle scale by a single writable 'scale'
> > entry. Driver should then adjust the gain/time accordingly.
> >
> > It however is difficult for a driver to know whether it should change
> > gain or integration time to meet the requested scale. Usually it is
> > preferred to have longer integration time which usually improves
> > accuracy, but there may be use-cases where long measurement times can be
> > an issue. Thus it can be preferable to allow also changing the
> > integration time - but mitigate the scale impact by also changing the gain
> > underneath. Eg, if integration time change doubles the measured values,
> > the driver can reduce the HW-gain to half.
> >
> > The theory of the computations of gain-time-scale is simple. However,
> > some people (undersigned) got that implemented wrong for more than once.
> >
> > Add some gain-time-scale helpers in order to not dublicate errors in all
> > drivers needing these computations.
>
> ...
>
> > +/*
>
> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> It's really a provocative to some people who will come with a patches to "fix"
> this...
Just make it kernel-doc.
>
> > + * iio_gts_get_gain - Convert scale to total gain
> > + *
> > + * Internal helper for converting scale to total gain.
> > + *
> > + * @max: Maximum linearized scale. As an example, when scale is created
> > + * in magnitude of NANOs and max scale is 64.1 - The linearized
> > + * scale is 64 100 000 000.
> > + * @scale: Linearized scale to compte the gain for.
> > + *
> > + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
> > + * is invalid.
> > + */
> ...
>
> > +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
>
> I would say _HELPER part is too much, but fine with me.
Hmm. I think I like the HELPER bit as separates it from being a driver.
Of course I might change my mind after a few sleeps.
> > +++ b/drivers/iio/light/iio-gts-helper.h
>
> Is it _only_ for a Light type of sensors?
I'd move it up a directory and allow for other users.
Jonathan
On Mon, 6 Mar 2023 11:17:15 +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.
>
> 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]>
Trying not to duplicate what Andy has raised...
At some stage I want to go through the maths very carefully but it's
not happening today and I don't want to delay resolving other remaining comments
so that can wait for a later version. I'm sure it's fine but I like to be
paranoid :)
> +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);
All of them want to be in the namespace.
> diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
> new file mode 100644
> index 000000000000..4b5a417946f4
> --- /dev/null
> +++ b/drivers/iio/light/iio-gts-helper.h
...
> +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> + int old_gain, int old_time_sel,
> + int new_time_sel, int *new_gain);
> +int iio_gts_build_avail_tables(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
> +int iio_gts_build_avail_scale_table(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts);
> +int iio_gts_build_avail_time_table(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts);
Given most modern IIO drivers use fully devm_ based probing, for now I would not
expose anything else. That will reduce the interface a lot which I think
is probably a good thing at this stage.
Keep the non devm stuff internally though as it is a nice structure to have
an I can see we may want some of these in non devm form in the future.
Similarly - for now don't expose the individual table building functions
as we may never need them in drivers. We (more or less) only support interfaces
that are used and so far they aren't.
For other functions it's worth thinking about whether to not export them
initially. I haven't been through them all to figure out what is not currently used.
> +void iio_gts_purge_avail_scale_table(struct iio_gts *gts);
> +void iio_gts_purge_avail_time_table(struct iio_gts *gts);
> +void iio_gts_purge_avail_tables(struct iio_gts *gts);
> +int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type,
> + int *length);
> +int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
> + int *length);
> +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
> + const int **vals, int *type, int *length);
> +
> +#endif
On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:
> On Mon, 6 Mar 2023 11:17:15 +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.
> >
> > 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]>
>
> Trying not to duplicate what Andy has raised...
>
>
> At some stage I want to go through the maths very carefully but it's
> not happening today and I don't want to delay resolving other remaining comments
> so that can wait for a later version. I'm sure it's fine but I like to be
> paranoid :)
>
> > +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);
>
> All of them want to be in the namespace.
>
>
>
> > diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
> > new file mode 100644
> > index 000000000000..4b5a417946f4
> > --- /dev/null
> > +++ b/drivers/iio/light/iio-gts-helper.h
>
> ...
>
> > +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> > + int old_gain, int old_time_sel,
> > + int new_time_sel, int *new_gain);
> > +int iio_gts_build_avail_tables(struct iio_gts *gts);
> > +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
> > +int iio_gts_build_avail_scale_table(struct iio_gts *gts);
> > +int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts);
> > +int iio_gts_build_avail_time_table(struct iio_gts *gts);
> > +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts);
>
> Given most modern IIO drivers use fully devm_ based probing, for now I would not
> expose anything else. That will reduce the interface a lot which I think
> is probably a good thing at this stage.
>
> Keep the non devm stuff internally though as it is a nice structure to have
> an I can see we may want some of these in non devm form in the future.
>
> Similarly - for now don't expose the individual table building functions
> as we may never need them in drivers. We (more or less) only support interfaces
> that are used and so far they aren't.
>
> For other functions it's worth thinking about whether to not export them
> initially. I haven't been through them all to figure out what is not currently used.
>
Ah. I forgot the tests that don't have a device so can't use devm.
Ah well I guess we have to keep some of the other cases.
> > +void iio_gts_purge_avail_scale_table(struct iio_gts *gts);
> > +void iio_gts_purge_avail_time_table(struct iio_gts *gts);
> > +void iio_gts_purge_avail_tables(struct iio_gts *gts);
> > +int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type,
> > + int *length);
> > +int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
> > + int *length);
> > +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
> > + const int **vals, int *type, int *length);
> > +
> > +#endif
>
On Mon, 6 Mar 2023 11:23:50 +0200
Matti Vaittinen <[email protected]> wrote:
> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
> capable of detecting a very wide range of illuminance. Typical application
> is adjusting LCD and backlight power of TVs and mobile phones.
>
> Add initial support for the ROHM BU27034 ambient light sensor.
>
> NOTE:
> - Driver exposes 4 channels. One IIO_LIGHT channel providing the
> calculated lux values based on measured data from diodes #0 and
> #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
> register data from all diodes for more intense user-space
> computations.
> - Sensor has GAIN values that can be adjusted from 1x to 4096x.
> - Sensor has adjustible measurement times of 5, 55, 100, 200 and
> 400 mS. Driver does not support 5 mS which has special
> limitations.
> - Driver exposes standard 'scale' adjustment which is
> implemented by:
> 1) Trying to adjust only the GAIN
> 2) If GAIN adjustment alone can't provide requested
> scale, adjusting both the time and the gain is
> attempted.
> - Driver exposes writable INT_TIME property that can be used
> for adjusting the measurement time. Time adjustment will also
> cause the driver to try to adjust the GAIN so that the
> overall scale is kept as close to the original as possible.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---
>
> Please note: There are few "unfinished" discussions regarding at least:
>
> - PROCESSED channel scale.
Hopefully the reply in the other thread covered this.
It's not PROCESSED as you need to apply the * 1000 so just
make it _RAW.
> - Implementation details when avoiding division by zero.
>
> I have implemented those for this version in a way that I see the best.
> It would have been better to wait for discussions to finish - but I will
> be away from the computer for a week - so I wanted to send out the v3
> with fixes so that people who are interested can do a review while I am
> away.
I'm getting behind with review anyway so a week delay on this sounds great to
me ;) I might get to your other series as a result though don't think I'll get there today.
>
> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
> new file mode 100644
> index 000000000000..6135a8a2a4f3
> --- /dev/null
> +++ b/drivers/iio/light/rohm-bu27034.c
> @@ -0,0 +1,1501 @@
...
> +
> +#include "iio-gts-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
Use FIELD_GET() and you can get rid of this SHIFT.
> +#define BU27034_MASK_D2_GAIN_HI GENMASK(7, 6)
> +#define BU27034_MASK_D2_GAIN_LO GENMASK(2, 0)
> +#define BU27034_SHIFT_D2_GAIN 3
Mentioned below. I don't like this. It's not obvious
at all that the shift only applies to the HI part.
Perhaps renaming the shift is enough.
> +
> +#define BU27034_REG_MODE_CONTROL3 0x43
> +#define BU27034_REG_MODE_CONTROL4 0x44
> +#define BU27034_MASK_MEAS_EN BIT(0)
> +#define BU27034_MASK_VALID BIT(7)
> +#define BU27034_REG_DATA0_LO 0x50
> +#define BU27034_REG_DATA1_LO 0x52
> +#define BU27034_REG_DATA2_LO 0x54
> +#define BU27034_REG_DATA2_HI 0x55
> +#define BU27034_REG_MANUFACTURER_ID 0x92
> +#define BU27034_REG_MAX BU27034_REG_MANUFACTURER_ID
> +
> +/*
> + * The BU27034 does not have interrupt or any other mechanism of triggering
I'd drop the 'other means' bit as no idea what they might be.
"does not have interrupt to trigger the data read.."
> + * the data read when measurement has finished. Hence we poll the VALID bit in
> + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS
> + * milliseconds before the expected sampling time to prevent the drifting. Eg,
> + * If we constantly wake up a bit too late we would eventually skip a sample.
> + * And because the sleep can't wake up _exactly_ at given time this would be
> + * inevitable even if the sensor clock would be perfectly phase-locked to CPU
> + * clock - which we can't say is the case.
> + *
> + * This is still fragile. No matter how big advance do we have, we will still
> + * risk of losing a sample because things can in a rainy-day skenario be
> + * delayed a lot. Yet, more we reserve the time for polling, more we also lose
> + * the performance by spending cycles polling the register. So, selecting this
> + * value is a balancing dance between severity of wasting CPU time and severity
> + * of losing samples.
> + *
> + * In most cases losing the samples is not _that_ crucial because light levels
> + * tend to change slowly.
> + */
> +#define BU27034_MEAS_WAIT_PREMATURE_MS 5
> +#define BU27034_DATA_WAIT_TIME_US 1000
> +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000)
...
> +static const struct iio_chan_spec bu27034_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
Not processed because scale needs to be applied.
> + BIT(IIO_CHAN_INFO_SCALE),
> + .channel = BU27034_CHAN_ALS,
> + .scan_index = BU27034_CHAN_ALS,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> + },
> + },
> + /*
> + * The BU27034 DATA0 and DATA1 channels are both on the visible light
> + * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
> + * These wave lengths are pretty much on the border of colours making
> + * these a poor candidates for R/G/B standardization. Hence they're both
> + * marked as clear channels
> + */
> + BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
> + BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
> + BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
...
> +
> +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;
FIELD_GET() and get rid of separation SHIFT definitions.
> + }
> + 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);
This one is messy but I'd prefer it as two field extractions that are then combined.
The shift of 3 is a weird artefact of the field layout and not easily described via a define.
The current name is definitely not descriptive enough.
FIELD_GET(val, BU27034_MASK_D2_GAIN_HI) << 3 |
FIELD_GET(val, BU27034_MASK_D2_GAIN_LO)
> + }
> +
> + return -EINVAL;
> +}
> +static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
> + int *val2)
> +{
> + int ret;
> +
> + if (channel == BU27034_CHAN_ALS) {
> + *val = 0;
> + *val2 = 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> +
> + mutex_lock(&data->mutex);
> + ret = _bu27034_get_scale(data, channel, val, val2);
> + mutex_unlock(&data->mutex);
> + if (!ret)
> + return IIO_VAL_INT_PLUS_NANO;
Prefer this flipped so the error path is out of line.
if (ret)
return ret;
return IIO_VAL_INT_PLUS_NANO;
> +
> + return ret;
> +}
...
> +static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
> +{
> + unsigned int gain0, gain1, meastime;
> + unsigned int d1_d0_ratio_scaled;
> + u16 ch0, ch1;
> + u64 helper64;
> + int ret;
> +
> + /*
> + * We return 0 luxes if calculation fails. This should be reasonably
> + * easy to spot from the buffers especially if raw-data channels show
> + * valid values
> + */
> + *val = 0;
> +
> + /* Avoid div by zero. */
> + if (!res[0])
> + ch0 = 1;
> + else
> + ch0 = le16_to_cpu(res[0]);
> +
> + if (!res[1])
> + ch1 = 1;
> + else
> + ch1 = le16_to_cpu(res[1]);
> +
As per other thread. Really don't like the check on 0 before
the endian conversion. Sure it can be done, but it's a micro optimization that
seems unnecessary.
...
> +
> +static int bu27034_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bu27034_data *data = iio_priv(idev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = bu27034_get_int_time(data);
> + if (*val < 0)
> + return *val;
> +
> + /*
> + * We use 50000 uS internally for all calculations and only
> + * convert it to 55000 before returning it to the user.
> + *
> + * This is because the data-sheet says the time is 55 mS - but
> + * vendor provided computations used 50 mS.
No chance of a clarification? would be lovely to not do this!
> + */
> + if (*val == 50000)
> + *val = 55000;
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + return bu27034_get_scale(data, chan->channel, val, val2);
> +
> + case IIO_CHAN_INFO_RAW:
> + {
> + if (chan->type != IIO_INTENSITY)
> + return -EINVAL;
> +
> + if (chan->channel < BU27034_CHAN_DATA0 ||
> + chan->channel > BU27034_CHAN_DATA2)
> + return -EINVAL;
> +
> + /* Don't mess with measurement enabling while buffering */
> + ret = iio_device_claim_direct_mode(idev);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&data->mutex);
> + /*
> + * Reading one channel at a time is ineffiecient but we don't
spell check comments.
> + * care here. Buffered version should be used if performance is
> + * an issue.
> + */
> + ret = bu27034_get_single_result(data, chan->channel, val);
> +
> + mutex_unlock(&data->mutex);
> + iio_device_release_direct_mode(idev);
> +
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + if (chan->type != IIO_LIGHT)
> + return -EINVAL;
> +
> + /* Don't mess with measurement enabling while buffering */
> + ret = iio_device_claim_direct_mode(idev);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = bu27034_get_mlux(data, val);
> +
> + mutex_unlock(&data->mutex);
> + iio_device_release_direct_mode(idev);
> +
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> +
> + }
> +
> + return ret;
I would hope you can't get here. If you can fix that so
that the lack of return here allows the compiler to know you didn't
intend to get here and hence complain about it.
> +}
> +
> +
> +static int bu27034_wait_for_data(struct bu27034_data *data)
> +{
> + int ret, val;
> +
> + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
> + val, val & BU27034_MASK_VALID,
> + BU27034_DATA_WAIT_TIME_US,
> + BU27034_TOTAL_DATA_WAIT_TIME_US);
> + if (ret) {
> + dev_err(data->dev, "data polling %s\n",
> + !(val & BU27034_MASK_VALID) ? "timeout" : "fail");
> + return ret;
> + }
Blank line here to separate above error handling from this bit.
> + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
> + &data->scan.channels[0],
> + sizeof(data->scan.channels));
Why should we carry on if ret < 0? Needs a comment as not obvious from
code.
> + bu27034_invalidate_read_data(data);
> +
> + return ret;
> +}
> +
> +static int bu27034_buffer_thread(void *arg)
> +{
> + struct iio_dev *idev = arg;
> + struct bu27034_data *data;
> + int wait_ms;
> +
> + data = iio_priv(idev);
> +
> + wait_ms = bu27034_get_int_time(data);
> + wait_ms /= 1000;
> +
> + wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
> +
> + while (!kthread_should_stop()) {
> + int ret;
> + int64_t tstamp;
> +
> + msleep(wait_ms);
> + ret = bu27034_wait_for_data(data);
> + if (ret)
> + continue;
> +
> + tstamp = iio_get_time_ns(idev);
> +
> + if (*idev->active_scan_mask & BIT(BU27034_CHAN_ALS)) {
Scan mask is a bitmap so you should use the checkers for that rather than
making assumptions about it fitting in one long (even it if obviously does
fit).
test_bit()
> + int mlux;
> +
> + ret = bu27034_calc_mlux(data, &data->scan.channels[0],
> + &mlux);
> + if (ret)
> + dev_err(data->dev, "failed to calculate lux\n");
> +
> + /*
> + * The maximum milli lux value we get with gain 1x time
> + * 55mS data ch0 = 0xffff ch1 = 0xffff fits in 26 bits
> + * so there should be no problem returning int from
> + * computations and casting it to u32
> + */
> + data->scan.mlux = (u32)mlux;
> + }
> + iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
> + }
> +
> + return 0;
> +}
...
> +static int bu27034_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct bu27034_data *data;
> + struct regmap *regmap;
> + struct iio_dev *idev;
> + unsigned int part_id, reg;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to initialize Regmap\n");
> +
> + idev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!idev)
> + return -ENOMEM;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret && ret != -ENODEV)
Why the special -ENODEV handling?
You should get a stub regulator if one isn't provided by firmware.
If you don't get a stub, or a real regulator that's a failure so we should
return the error code and fail the probe.
> + return dev_err_probe(dev, ret, "Failed to get regulator\n");
> +
On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> On Sun, 12 Mar 2023 17:06:38 +0000
> Jonathan Cameron <[email protected]> wrote:
> > On Mon, 6 Mar 2023 11:17:15 +0200
> > Matti Vaittinen <[email protected]> wrote:
...
> > Given most modern IIO drivers use fully devm_ based probing, for now I would not
> > expose anything else. That will reduce the interface a lot which I think
> > is probably a good thing at this stage.
> >
> > Keep the non devm stuff internally though as it is a nice structure to have
> > an I can see we may want some of these in non devm form in the future.
> >
> > Similarly - for now don't expose the individual table building functions
> > as we may never need them in drivers. We (more or less) only support interfaces
> > that are used and so far they aren't.
> >
> > For other functions it's worth thinking about whether to not export them
> > initially. I haven't been through them all to figure out what is not currently used.
> >
> Ah. I forgot the tests that don't have a device so can't use devm.
Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
for that.
--
With Best Regards,
Andy Shevchenko
On 3/6/23 14:52, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>> Some light sensors can adjust both the HW-gain and integration time.
>> There are cases where adjusting the integration time has similar impact
>> to the scale of the reported values as gain setting has.
>>
>> IIO users do typically expect to handle scale by a single writable 'scale'
>> entry. Driver should then adjust the gain/time accordingly.
>>
>> It however is difficult for a driver to know whether it should change
>> gain or integration time to meet the requested scale. Usually it is
>> preferred to have longer integration time which usually improves
>> accuracy, but there may be use-cases where long measurement times can be
>> an issue. Thus it can be preferable to allow also changing the
>> integration time - but mitigate the scale impact by also changing the gain
>> underneath. Eg, if integration time change doubles the measured values,
>> the driver can reduce the HW-gain to half.
>>
>> The theory of the computations of gain-time-scale is simple. However,
>> some people (undersigned) got that implemented wrong for more than once.
>>
>> Add some gain-time-scale helpers in order to not dublicate errors in all
>> drivers needing these computations.
>
> ...
>
>> +/*
>
> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> It's really a provocative to some people who will come with a patches to "fix"
> this...
I just liked the kernel-doc format. It's a standard way of explaining
the parameters and returned value. Function however is intended to be
internal and thus I don't see a need to make this "official kernel doc".
>
>> + * iio_gts_get_gain - Convert scale to total gain
>> + *
>> + * Internal helper for converting scale to total gain.
>> + *
>> + * @max: Maximum linearized scale. As an example, when scale is created
>> + * in magnitude of NANOs and max scale is 64.1 - The linearized
>> + * scale is 64 100 000 000.
>> + * @scale: Linearized scale to compte the gain for.
>> + *
>> + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
>> + * is invalid.
>> + */
>> +static int iio_gts_get_gain(const u64 max, const u64 scale)
>> +{
>> + int tmp = 1;
>> +
>> + if (scale > max || !scale)
>> + return -EINVAL;
>> +
>> + if (U64_MAX - max < scale) {
>> + /* Risk of overflow */
>> + if (max - scale < scale)
>> + return 1;
>
>> + while (max - scale > scale * (u64)tmp)
>> + tmp++;
>> +
>> + return tmp + 1;
>
> Can you wait for the comments to appear a bit longer, please?
> I have answered to your query in the previous discussion.
>
Yep. Sorry for that. I just wanted to get the v3 out before I left the
computer for a week to allow potential reviewers to check the quite a
bit reworked series.
>> + }
>> +
>> + while (max > scale * (u64) tmp)
>
> No space for castings?
Thanks,
>
>> + tmp++;
>> +
>> + return tmp;
>> +}
>
> ...
>
>> + /*
>> + * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
>> + * multiplication followed by division to avoid overflow
>
> Missing period.
>
>> + */
>> + if (scaler > NANO || !scaler)
>> + return -EINVAL;
>
> Shouldn't be OVERFLOW for the first one?
-EOVERFLOW? I guess it could be. Thanks.
> ...
>
>> + *lin_scale = (u64) scale_whole * (u64)scaler +
>
> No space for casting?
Yes. Thanks
> ...
>
>> + ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
>> + >s->max_scale);
>> + if (ret)
>> + return ret;
>> +
>> + gts->hwgain_table = gain_tbl;
>> + gts->num_hwgain = num_gain;
>> + gts->itime_table = tim_tbl;
>> + gts->num_itime = num_times;
>> + gts->per_time_avail_scale_tables = NULL;
>> + gts->avail_time_tables = NULL;
>> + gts->avail_all_scales_table = NULL;
>> + gts->num_avail_all_scales = 0;
>
> Just wondering why we can't simply
>
> memset(0)
>
> beforehand and drop all these 0 assignments?
>
I guess we can :)
> ...
>
>> + /*
>> + * Sort the tables for nice output and for easier finding of
>> + * unique values
>
> Missing period. Please, check the style of multi-line comments. I believe it's
> even mentioned in the documentation.
>
>> + */
>
> ...
>
>> + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
>> + NULL);
>
> One line reads better?
I try mostly to keep the good old 80 chars as I often have 3 terminal
windows fitted on my laptop screen. It works best with the short lines.
>
> ...
>
>> + if (ret && gts->avail_all_scales_table)
>
> In one case you commented that free(NULL) is okay, in the other, you add
> a duplicative check. Why?
Sorry but what do you mean by dublicative check?
Usually I avoid the kfree(NULL). That's why I commented on it in that
another case where it was not explicitly disallowed. I'll change that
for v4 to avoid kfree(NULL) as you suggested.
>
>> + kfree(gts->avail_all_scales_table);
>
> ...
>
>> + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>
> sizeof(type) is error prone in comparison to sizeof(*var).
Yes and no. In majority of cases where we see sizeof(*var) - the *var is
no longer a pointer as having pointers to pointers is not _that_ common.
When we see sizeof(type *) - we instantly know it is a size of a pointer
and not a size of some other type.
So yes, while having sizeof(*var) makes us tolerant to errors caused by
variable type changes - it makes us prone to human reader errors. Also,
if someone changes type of *var from pointer to some other type - then
he/she is likely to in any case need to revise the array alloactions too.
While I in general agree with you that the sizeof(variable) is better
than sizeof(type) - I see that in cases like this the sizeof(type *) is
clearer.
>
>> + if (!per_time_gains)
>> + return ret;
>> +
>> + per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>
> Ditto.
>
>> + if (!per_time_scales)
>> + goto free_gains;
>
> ...
>
>> +err_free_out:
>> + while (i) {
>> + /*
>> + * It does not matter if i'th alloc was not succesfull as
>> + * kfree(NULL) is safe.
>> + */
>
> Instead, can be just a free of the known allocated i:th member first followed
> by traditional pattern. In that case comment will become redundant.
>
I replied to this in your comments regarding the v2. Sorry for splitting
the discussion by sending v3 so soon.
>> + kfree(per_time_scales[i]);
>> + kfree(per_time_gains[i]);
>> +
>> + i--;
>> + }
>
> ...
>
>> + for (i = gts->num_itime - 1; i >= 0; i--) {
>
> while (i--) {
>
> makes it easier to parse.
This is also something I replied for v2. I think we have a fundamental
disagreement on this one :/
>
>> +/**
>> + * iio_gts_all_avail_scales - helper for listing all available scales
>> + * @gts: Gain time scale descriptor
>> + * @vals: Returned array of supported scales
>> + * @type: Type of returned scale values
>> + * @length: Amount of returned values in array
>> + *
>> + * Returns a value suitable to be returned from read_avail or a negative error
>
> Missing a return section. Have you run kernel doc to validate this?
No. I think I have never run the kernel doc - probably time to do so :)
Thanks.
> Missing period.
>
> Seems these problems occur in many function descriptions.
>
>> + */
>
> ...
>
>> + /*
>> + * Using this function prior building the tables is a driver-error
>> + * which should be fixed when the driver is tested for a first time
>
> Missing period.
>
>> + */
>> + if (WARN_ON(!gts->num_avail_all_scales))
>
> Does this justify panic? Note, that any WARN() can become an Oops followed by
> panic and reboot.
No. My initial thinking was that this could only happen if a driver
which do not build the tables tries to use the helpers. That, in my
books, would have been 100% reproducible driver error, happening when
ever the available scales were read.
I did overlook the case where freeing of tables is done in wrong order.
That type of bug could well escape the initial testing and no - it
should not bring down the machine. I'll drop the WARN. Thanks.
>
>> + return -EINVAL;
>
> ...
>
>> + for (i = 0; i < gts->num_hwgain; i++) {
>> + /*
>> + * It is not expected this function is called for an exactly
>> + * matching gain.
>> + */
>> + if (unlikely(gain == gts->hwgain_table[i].gain)) {
>> + *in_range = true;
>> + return gain;
>> + }
>
>> + if (!min)
>> + min = gts->hwgain_table[i].gain;
>> + else
>> + min = min(min, gts->hwgain_table[i].gain);
>
> I was staring at this and have got no clue why it's not a dead code.
Nor can I. It seems obvious to me that the one who wrote this had no
idea what he was doing XD
Well, I must have had some initial idea of using the minimum value to
something - but I can't remember what would've been the use. Maybe I was
initially thinking that I'll return the smallest value in-range if the
gain given as a parameter was smaller than any of the supported ones.
Thank you for reading this carefully and pointing it out! Well spotted!
>
>> + if (gain > gts->hwgain_table[i].gain) {
>> + if (!diff) {
>> + diff = gain - gts->hwgain_table[i].gain;
>> + best = i;
>> + } else {
>> + int tmp = gain - gts->hwgain_table[i].gain;
>> +
>> + if (tmp < diff) {
>> + diff = tmp;
>> + best = i;
>> + }
>> + }
>
> int tmp = gain - gts->hwgain_table[i].gain;
>
> if (!diff || tmp < diff) {
> diff = tmp;
> best = i;
> }
>
> ?
>
> Or did you miss using 'min'? (And I'm wondering how variable name and min()
> macro are not conflicting with each other.
>
>> + } else {
>> + /*
>> + * We found valid hwgain which is greater than
>> + * reference. So, unless we return a failure below we
>> + * will have found an in-range gain
>> + */
>> + *in_range = true;
>> + }
>> + }
>> + /* The requested gain was smaller than anything we support */
>> + if (!diff) {
>> + *in_range = false;
>> +
>> + return -EINVAL;
>> + }
>> +
>> + return gts->hwgain_table[best].gain;
>
> ...
>
>> + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
>> + &scale);
>
> Still can be one line.
>
>> + if (ret)
>> + return ret;
>> +
>> + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
>> + new_gain);
>
> Ditto.
I still prefer the 80-chars when it does not mean some terribly awkward
split - or really many rows of arguments.
>> + if (ret)
>> + return -EINVAL;
>
> ...
>
>> +++ b/drivers/iio/light/iio-gts-helper.h
>
> Is it _only_ for a Light type of sensors?
This is a very good question. I was asking this from myself but as I
don't know better I just decided to put it under the light where I'll
have the use-cases. We can move it to drivers/iio if there will be users
outside the light sensors.
>
> ...
>
>> +#ifndef __IIO_GTS_HELPER__
>> +#define __IIO_GTS_HELPER__
>
> If yes, perhaps adding LIGHT here?
I'd like to keep this matching the file name, especially when I don't
know for sure this can't be used elsewhere.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/12/23 19:06, Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 11:17:15 +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.
>>
>> 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]>
>
> Trying not to duplicate what Andy has raised...
>
>
> At some stage I want to go through the maths very carefully but it's
> not happening today and I don't want to delay resolving other remaining comments
> so that can wait for a later version. I'm sure it's fine but I like to be
> paranoid :)
>
This is more than welcome! I tried to add some test cases for verifying
some parts - but extra pair of eyes is always more than appreciated!
I've written and read way too many bugs to not appreciate a healthy
amount of paranoia :)
>> +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);
>
> All of them want to be in the namespace.
Seems like I accidentally did not use the EXPORT_SYMBOL_GPL for this
one. It must thus have evaded my conversion to name space one. Thanks!
>
>
>> diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
>> new file mode 100644
>> index 000000000000..4b5a417946f4
>> --- /dev/null
>> +++ b/drivers/iio/light/iio-gts-helper.h
>
> ...
>
>> +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
>> + int old_gain, int old_time_sel,
>> + int new_time_sel, int *new_gain);
>> +int iio_gts_build_avail_tables(struct iio_gts *gts);
>> +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
>> +int iio_gts_build_avail_scale_table(struct iio_gts *gts);
>> +int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts);
>> +int iio_gts_build_avail_time_table(struct iio_gts *gts);
>> +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts);
>
> Given most modern IIO drivers use fully devm_ based probing, for now I would not
> expose anything else. That will reduce the interface a lot which I think
> is probably a good thing at this stage.
>
> Keep the non devm stuff internally though as it is a nice structure to have
> an I can see we may want some of these in non devm form in the future.
>
> Similarly - for now don't expose the individual table building functions
> as we may never need them in drivers. We (more or less) only support interfaces
> that are used and so far they aren't.
>
> For other functions it's worth thinking about whether to not export them
> initially. I haven't been through them all to figure out what is not currently used.
>
>> +void iio_gts_purge_avail_scale_table(struct iio_gts *gts);
>> +void iio_gts_purge_avail_time_table(struct iio_gts *gts);
>> +void iio_gts_purge_avail_tables(struct iio_gts *gts);
>> +int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type,
>> + int *length);
>> +int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
>> + int *length);
>> +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
>> + const int **vals, int *type, int *length);
>> +
>> +#endif
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/12/23 18:51, Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 14:52:57 +0200
> Andy Shevchenko <[email protected]> wrote:
>
>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>>> Some light sensors can adjust both the HW-gain and integration time.
>>> There are cases where adjusting the integration time has similar impact
>>> to the scale of the reported values as gain setting has.
>>>
>>> IIO users do typically expect to handle scale by a single writable 'scale'
>>> entry. Driver should then adjust the gain/time accordingly.
>>>
>>> It however is difficult for a driver to know whether it should change
>>> gain or integration time to meet the requested scale. Usually it is
>>> preferred to have longer integration time which usually improves
>>> accuracy, but there may be use-cases where long measurement times can be
>>> an issue. Thus it can be preferable to allow also changing the
>>> integration time - but mitigate the scale impact by also changing the gain
>>> underneath. Eg, if integration time change doubles the measured values,
>>> the driver can reduce the HW-gain to half.
>>>
>>> The theory of the computations of gain-time-scale is simple. However,
>>> some people (undersigned) got that implemented wrong for more than once.
>>>
>>> Add some gain-time-scale helpers in order to not dublicate errors in all
>>> drivers needing these computations.
>>
>> ...
>>
>>> +/*
>>
>> If it's deliberately not a kernel doc, why to bother to have it looking as one?
>> It's really a provocative to some people who will come with a patches to "fix"
>> this...
>
> Just make it kernel-doc.
>
Are you sure...? I don't like the idea of polluting generated docs with
documentation for this type of tiny internal pieces not usable outside
this component anyways...
>>
>>> + * iio_gts_get_gain - Convert scale to total gain
>>> + *
>>> + * Internal helper for converting scale to total gain.
>>> + *
>>> + * @max: Maximum linearized scale. As an example, when scale is created
>>> + * in magnitude of NANOs and max scale is 64.1 - The linearized
>>> + * scale is 64 100 000 000.
>>> + * @scale: Linearized scale to compte the gain for.
>>> + *
>>> + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
>>> + * is invalid.
>>> + */
>
>> ...
>>
>>> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
>>
>> I would say _HELPER part is too much, but fine with me.
>
> Hmm. I think I like the HELPER bit as separates it from being a driver.
> Of course I might change my mind after a few sleeps.
Ever considered a career as a politician? ;) (No offense intended - and
feel free to change your mind on this. I don't expect this to be done
tomorrow)
>
>>> +++ b/drivers/iio/light/iio-gts-helper.h
>>
>> Is it _only_ for a Light type of sensors?
>
> I'd move it up a directory and allow for other users.
>
Ok. I'll do the move for the next version.
Yours,
-- Matti
> Jonathan
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/13/23 14:40, Andy Shevchenko wrote:
> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>> On Sun, 12 Mar 2023 17:06:38 +0000
>> Jonathan Cameron <[email protected]> wrote:
>>> On Mon, 6 Mar 2023 11:17:15 +0200
>>> Matti Vaittinen <[email protected]> wrote:
>
> ...
>
>>> Given most modern IIO drivers use fully devm_ based probing, for now I would not
>>> expose anything else. That will reduce the interface a lot which I think
>>> is probably a good thing at this stage.
Probably at any stage :)
>>>
>>> Keep the non devm stuff internally though as it is a nice structure to have
>>> an I can see we may want some of these in non devm form in the future.
Ok. I was pondering this while writing these APIs. I was just thinking
that _maybe_ someone has an driver where they do not use devm for a
reason. Allowing a "non devm" variants for such is likely to be needed.
Hence, I was thinking that having a non devm version could be beneficial
from the start to avoid someone being tempted to just mix the readily
available devm with manual unwinding...
>>>
>>> Similarly - for now don't expose the individual table building functions
>>> as we may never need them in drivers. We (more or less) only support interfaces
>>> that are used and so far they aren't.
I was thinking of this too. It was just the small 'avoid extra
operations [like unnecessary endianess conversions :p] when
needed'-voice in me that started screaming when I though of exporting
only the 'build all' and 'purge all' APIs...
>>>
>>> For other functions it's worth thinking about whether to not export them
>>> initially. I haven't been through them all to figure out what is not currently used.
I think I can go through them. There are a few that aren't currently used.
>>>
>> Ah. I forgot the tests that don't have a device so can't use devm.
>
> Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
> for that.
I'd appreciated any pointer for such an example if you have one at hand.
(I can do the digging if you don't though!)
I am not a fan of unit tests. They add huge amount of inertia to
development, and in worst case, they stop people from contributing where
improving a feature requires test code modification(s). And harder the
test code is to understand, worse the unwanted side-effects. Also,
harder the test code is to read, more time and effort it requires to
analyze a test failure... Hence, I am _very_ conservative what comes to
adding size of test code with anything that is not strictly required.
After that being said, unit tests are a great tool when carefully used -
and I assume/hope stubbing a device for devm_ tests does not add much
extra... But let me see if I can find an example :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
> On 3/12/23 18:51, Jonathan Cameron wrote:
> > On Mon, 6 Mar 2023 14:52:57 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
...
> > > > +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
> > >
> > > I would say _HELPER part is too much, but fine with me.
> >
> > Hmm. I think I like the HELPER bit as separates it from being a driver.
> > Of course I might change my mind after a few sleeps.
>
> Ever considered a career as a politician? ;) (No offense intended - and feel
> free to change your mind on this. I don't expect this to be done tomorrow)
It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE
definition.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
> On 3/6/23 14:52, Andy Shevchenko wrote:
> > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
...
> > > +/*
> >
> > If it's deliberately not a kernel doc, why to bother to have it looking as one?
> > It's really a provocative to some people who will come with a patches to "fix"
> > this...
>
> I just liked the kernel-doc format. It's a standard way of explaining the
> parameters and returned value. Function however is intended to be internal
> and thus I don't see a need to make this "official kernel doc".
The problem as I pointed out with your approach it's unmaintainable. And
I even explained why I consider it this way.
> > > + * iio_gts_get_gain - Convert scale to total gain
> > > + *
> > > + * Internal helper for converting scale to total gain.
> > > + *
> > > + * @max: Maximum linearized scale. As an example, when scale is created
> > > + * in magnitude of NANOs and max scale is 64.1 - The linearized
> > > + * scale is 64 100 000 000.
> > > + * @scale: Linearized scale to compte the gain for.
> > > + *
> > > + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
> > > + * is invalid.
> > > + */
...
> > > + if (scaler > NANO || !scaler)
> > > + return -EINVAL;
> >
> > Shouldn't be OVERFLOW for the first one?
>
> -EOVERFLOW? I guess it could be. Thanks.
Yes.
...
> > > + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
> > > + NULL);
> >
> > One line reads better?
>
> I try mostly to keep the good old 80 chars as I often have 3 terminal
> windows fitted on my laptop screen. It works best with the short lines.
With it on one line
sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp, NULL);
You have N at the last column which quite likely suggests that it's NULL.
So, I don't think it's a big issue to put on a single line.
...
> > > + if (ret && gts->avail_all_scales_table)
> >
> > In one case you commented that free(NULL) is okay, in the other, you add
> > a duplicative check. Why?
>
> Sorry but what do you mean by dublicative check?
>
> Usually I avoid the kfree(NULL). That's why I commented on it in that
> another case where it was not explicitly disallowed. I'll change that for v4
> to avoid kfree(NULL) as you suggested.
So, and with it you put now a double check for NULL, do you think it's okay?
I don't.
> > > + kfree(gts->avail_all_scales_table);
...
> > > + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> >
> > sizeof(type) is error prone in comparison to sizeof(*var).
>
> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
> longer a pointer as having pointers to pointers is not _that_ common. When
> we see sizeof(type *) - we instantly know it is a size of a pointer and not
> a size of some other type.
>
> So yes, while having sizeof(*var) makes us tolerant to errors caused by
> variable type changes - it makes us prone to human reader errors. Also, if
> someone changes type of *var from pointer to some other type - then he/she
> is likely to in any case need to revise the array alloactions too.
>
> While I in general agree with you that the sizeof(variable) is better than
> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.
Still get a fundamental disagreement on this. I would insist, but I'm not
a maintainer, so you are lucky :-) if Jonathan will not force you to follow
my way.
...
> > > + per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
Ditto.
...
> > > +err_free_out:
> > > + while (i) {
> > > + /*
> > > + * It does not matter if i'th alloc was not succesfull as
> > > + * kfree(NULL) is safe.
> > > + */
> >
> > Instead, can be just a free of the known allocated i:th member first followed
> > by traditional pattern. In that case comment will become redundant.
> >
>
> I replied to this in your comments regarding the v2. Sorry for splitting the
> discussion by sending v3 so soon.
You can have repeated it here :-)
Yes, two labels and one kfree() makes the comment go away. To me that comment
is more confusing than helping.
> > > + kfree(per_time_scales[i]);
> > > + kfree(per_time_gains[i]);
> > > +
> > > + i--;
> > > + }
...
> > > + for (i = gts->num_itime - 1; i >= 0; i--) {
> >
> > while (i--) {
> >
> > makes it easier to parse.
>
> This is also something I replied for v2. I think we have a fundamental
> disagreement on this one :/
Yes, and I will continue insisting on while (foo--).
That why I won't give you my tags :-)
...
> > > + if (!min)
> > > + min = gts->hwgain_table[i].gain;
> > > + else
> > > + min = min(min, gts->hwgain_table[i].gain);
> >
> > I was staring at this and have got no clue why it's not a dead code.
>
> Nor can I. It seems obvious to me that the one who wrote this had no idea
> what he was doing XD
>
> Well, I must have had some initial idea of using the minimum value to
> something - but I can't remember what would've been the use. Maybe I was
> initially thinking that I'll return the smallest value in-range if the gain
> given as a parameter was smaller than any of the supported ones.
>
> Thank you for reading this carefully and pointing it out! Well spotted!
Hint: run always `make W=1` when building kernel.
It will show defined but not used cases and combined with nowadays
default -Werror won't be compilable.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> On 3/13/23 14:40, Andy Shevchenko wrote:
> > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > Jonathan Cameron <[email protected]> wrote:
...
> > > Ah. I forgot the tests that don't have a device so can't use devm.
> >
> > Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
> > for that.
>
> I'd appreciated any pointer for such an example if you have one at hand. (I
> can do the digging if you don't though!)
>
> I am not a fan of unit tests. They add huge amount of inertia to
> development, and in worst case, they stop people from contributing where
> improving a feature requires test code modification(s). And harder the test
> code is to understand, worse the unwanted side-effects. Also, harder the
> test code is to read, more time and effort it requires to analyze a test
> failure... Hence, I am _very_ conservative what comes to adding size of test
> code with anything that is not strictly required.
>
> After that being said, unit tests are a great tool when carefully used - and
> I assume/hope stubbing a device for devm_ tests does not add much extra...
> But let me see if I can find an example :)
drivers/gpu/drm/tests/drm_managed_test.c ?
(somewhere underneath:
ret = platform_driver_register(&fake_platform_driver);
which suggests... what exactly? :-)
--
With Best Regards,
Andy Shevchenko
On 3/12/23 19:39, Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 11:23:50 +0200
> Matti Vaittinen <[email protected]> wrote:
>
>> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
>> capable of detecting a very wide range of illuminance. Typical application
>> is adjusting LCD and backlight power of TVs and mobile phones.
>>
>> Add initial support for the ROHM BU27034 ambient light sensor.
>>
>> NOTE:
>> - Driver exposes 4 channels. One IIO_LIGHT channel providing the
>> calculated lux values based on measured data from diodes #0 and
>> #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
>> register data from all diodes for more intense user-space
>> computations.
>> - Sensor has GAIN values that can be adjusted from 1x to 4096x.
>> - Sensor has adjustible measurement times of 5, 55, 100, 200 and
>> 400 mS. Driver does not support 5 mS which has special
>> limitations.
>> - Driver exposes standard 'scale' adjustment which is
>> implemented by:
>> 1) Trying to adjust only the GAIN
>> 2) If GAIN adjustment alone can't provide requested
>> scale, adjusting both the time and the gain is
>> attempted.
>> - Driver exposes writable INT_TIME property that can be used
>> for adjusting the measurement time. Time adjustment will also
>> cause the driver to try to adjust the GAIN so that the
>> overall scale is kept as close to the original as possible.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>>
>> ---
>>
>> Please note: There are few "unfinished" discussions regarding at least:
>>
>> - PROCESSED channel scale.
>
> Hopefully the reply in the other thread covered this.
> It's not PROCESSED as you need to apply the * 1000 so just
> make it _RAW.
>
Your reply in the other thread was just fine :) I think I'll round the
result to luxes (which should be sufficient to many users if I am not
mistaken) - and provide the PROCESSED values to user-space for the sake
of the ultimate simplicity for users.
>> - Implementation details when avoiding division by zero.
>>
>> I have implemented those for this version in a way that I see the best.
>> It would have been better to wait for discussions to finish - but I will
>> be away from the computer for a week - so I wanted to send out the v3
>> with fixes so that people who are interested can do a review while I am
>> away.
>
> I'm getting behind with review anyway so a week delay on this sounds great to
> me ;) I might get to your other series as a result though don't think I'll get there today.
>
Oh, the week elapsed already ;) But don't sweat on it - this series grew
quite large so spending time on reviewing makes perfect sense. I _know_
reviewing, and especially careful reviewing, takes time. And energy.
Sometimes we lack of both, occasionally just the other :) Thanks for all
the effort you and Andy have put on this so far!
>
>> +static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
>> +{
>> + unsigned int gain0, gain1, meastime;
>> + unsigned int d1_d0_ratio_scaled;
>> + u16 ch0, ch1;
>> + u64 helper64;
>> + int ret;
>> +
>> + /*
>> + * We return 0 luxes if calculation fails. This should be reasonably
>> + * easy to spot from the buffers especially if raw-data channels show
>> + * valid values
>> + */
>> + *val = 0;
>> +
>> + /* Avoid div by zero. */
>> + if (!res[0])
>> + ch0 = 1;
>> + else
>> + ch0 = le16_to_cpu(res[0]);
>> +
>> + if (!res[1])
>> + ch1 = 1;
>> + else
>> + ch1 = le16_to_cpu(res[1]);
>> +
>
> As per other thread. Really don't like the check on 0 before
> the endian conversion. Sure it can be done, but it's a micro optimization that
> seems unnecessary.
Ugh... Andy pointed me the max_t(). I'll use it even though I do really
dislike that change a lot.
>
> ...
>
>> +
>> +static int bu27034_read_raw(struct iio_dev *idev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct bu27034_data *data = iio_priv(idev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_INT_TIME:
>> + *val = bu27034_get_int_time(data);
>> + if (*val < 0)
>> + return *val;
>> +
>> + /*
>> + * We use 50000 uS internally for all calculations and only
>> + * convert it to 55000 before returning it to the user.
>> + *
>> + * This is because the data-sheet says the time is 55 mS - but
>> + * vendor provided computations used 50 mS.
>
> No chance of a clarification? would be lovely to not do this!
Actually, since the current code uses the multiplier-field in
gts-helpers - it may be we can just use the 55000 in the tables now. I
need to check it. So perhaps we can avoid this one now! Thanks!
>
>> + */
>> + if (*val == 50000)
>> + *val = 55000;
>> +
>> + return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + return bu27034_get_scale(data, chan->channel, val, val2);
>> +
>> + case IIO_CHAN_INFO_RAW:
>> + {
>> + if (chan->type != IIO_INTENSITY)
>> + return -EINVAL;
>> +
>> + if (chan->channel < BU27034_CHAN_DATA0 ||
>> + chan->channel > BU27034_CHAN_DATA2)
>> + return -EINVAL;
>> +
>> + /* Don't mess with measurement enabling while buffering */
>> + ret = iio_device_claim_direct_mode(idev);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&data->mutex);
>> + /*
>> + * Reading one channel at a time is ineffiecient but we don't
>
> spell check comments.
>
>> + * care here. Buffered version should be used if performance is
>> + * an issue.
>> + */
>> + ret = bu27034_get_single_result(data, chan->channel, val);
>> +
>> + mutex_unlock(&data->mutex);
>> + iio_device_release_direct_mode(idev);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> + }
>> +
>> + case IIO_CHAN_INFO_PROCESSED:
>> + if (chan->type != IIO_LIGHT)
>> + return -EINVAL;
>> +
>> + /* Don't mess with measurement enabling while buffering */
>> + ret = iio_device_claim_direct_mode(idev);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + ret = bu27034_get_mlux(data, val);
>> +
>> + mutex_unlock(&data->mutex);
>> + iio_device_release_direct_mode(idev);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> +
>> + default:
>> + return -EINVAL;
>> +
>> + }
>> +
>> + return ret;
>
> I would hope you can't get here. If you can fix that so
> that the lack of return here allows the compiler to know you didn't
> intend to get here and hence complain about it.
>
Sigh. I personally still think functions should return from the end...
Well, I will revise this for v4.
>
>> +static int bu27034_probe(struct i2c_client *i2c)
>> +{
>> + struct device *dev = &i2c->dev;
>> + struct bu27034_data *data;
>> + struct regmap *regmap;
>> + struct iio_dev *idev;
>> + unsigned int part_id, reg;
>> + int ret;
>> +
>> + regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
>> + if (IS_ERR(regmap))
>> + return dev_err_probe(dev, PTR_ERR(regmap),
>> + "Failed to initialize Regmap\n");
>> +
>> + idev = devm_iio_device_alloc(dev, sizeof(*data));
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + ret = devm_regulator_get_enable(dev, "vdd");
>> + if (ret && ret != -ENODEV)
>
> Why the special -ENODEV handling?
> You should get a stub regulator if one isn't provided by firmware.
> If you don't get a stub, or a real regulator that's a failure so we should
> return the error code and fail the probe.
I am actually not sure. Maybe my test setup didn't have the dummy
regulator assigned. I will re-check this.
The comments which I just skipped are something I more or less agree with :)
Best Regards
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/13/23 15:25, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
>> On 3/6/23 14:52, Andy Shevchenko wrote:
>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>
> ...
>
>>>> +/*
>>>
>>> If it's deliberately not a kernel doc, why to bother to have it looking as one?
>>> It's really a provocative to some people who will come with a patches to "fix"
>>> this...
>>
>> I just liked the kernel-doc format. It's a standard way of explaining the
>> parameters and returned value. Function however is intended to be internal
>> and thus I don't see a need to make this "official kernel doc".
>
> The problem as I pointed out with your approach it's unmaintainable. And
> I even explained why I consider it this way.
Yes. You told me that it asks for people to turn it to kernel doc. If
that happens, apply the patch and it is kernel doc. I don't see how
unmaintainable it is. I think this is just creating a problem we may
never face - and if we do, we can solve it by applying the 'problem' then.
>
>>>> + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
>>>> + NULL);
>>>
>>> One line reads better?
>>
>> I try mostly to keep the good old 80 chars as I often have 3 terminal
>> windows fitted on my laptop screen. It works best with the short lines.
>
> With it on one line
>
> sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp, NULL);
>
> You have N at the last column which quite likely suggests that it's NULL.
> So, I don't think it's a big issue to put on a single line.
Trusting suggestions like this in a kernel code would be a big problem
to me. I would ask myself - "do you feel lucky"?
Well, my favourite editor would wrap the line - so I would see the NULL
at the next row. Not indented properly causing it to be harder to read
than the code which is properly manually split and indented. It is much
less of a problem for me to "waste" a row here and see the line properly
split.
>>>> + if (ret && gts->avail_all_scales_table)
>>>
>>> In one case you commented that free(NULL) is okay, in the other, you add
>>> a duplicative check. Why?
>>
>> Sorry but what do you mean by dublicative check?
>>
>> Usually I avoid the kfree(NULL). That's why I commented on it in that
>> another case where it was not explicitly disallowed. I'll change that for v4
>> to avoid kfree(NULL) as you suggested.
>
> So, and with it you put now a double check for NULL, do you think it's okay?
> I don't.
I don't see the double check. I see only one check just above the
kfree()? Where is the other check?
>
>>>> + kfree(gts->avail_all_scales_table);
>
> ...
>
>>>> + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>>>
>>> sizeof(type) is error prone in comparison to sizeof(*var).
>>
>> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
>> longer a pointer as having pointers to pointers is not _that_ common. When
>> we see sizeof(type *) - we instantly know it is a size of a pointer and not
>> a size of some other type.
>>
>> So yes, while having sizeof(*var) makes us tolerant to errors caused by
>> variable type changes - it makes us prone to human reader errors. Also, if
>> someone changes type of *var from pointer to some other type - then he/she
>> is likely to in any case need to revise the array alloactions too.
>>
>> While I in general agree with you that the sizeof(variable) is better than
>> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.
>
> Still get a fundamental disagreement on this. I would insist, but I'm not
> a maintainer, so you are lucky :-) if Jonathan will not force you to follow
> my way.
In a code you are maintaining it is good to have it in your way as
you're responsible for it. This is also why I insist on having things in
a way I can read best for a code I plan to maintain - unless the
subsystem maintainers see it hard to maintain for them. So, let's see if
Jonathan has strong opinions on this one :)
> ...
>
>>>> + for (i = gts->num_itime - 1; i >= 0; i--) {
>>>
>>> while (i--) {
>>>
>>> makes it easier to parse.
>>
>> This is also something I replied for v2. I think we have a fundamental
>> disagreement on this one :/
>
> Yes, and I will continue insisting on while (foo--).
> That why I won't give you my tags :-)
Well, I am planning to keep reading this code when/if it is being
patched. Hence I am so reluctant to change it to something that makes it
harder for me to follow. Meanwhile, I understand that you don't want to
tag something you don't agree with.
> ...
>
>>>> + if (!min)
>>>> + min = gts->hwgain_table[i].gain;
>>>> + else
>>>> + min = min(min, gts->hwgain_table[i].gain);
>>>
>>> I was staring at this and have got no clue why it's not a dead code.
>>
>> Nor can I. It seems obvious to me that the one who wrote this had no idea
>> what he was doing XD
>>
>> Well, I must have had some initial idea of using the minimum value to
>> something - but I can't remember what would've been the use. Maybe I was
>> initially thinking that I'll return the smallest value in-range if the gain
>> given as a parameter was smaller than any of the supported ones.
>>
>> Thank you for reading this carefully and pointing it out! Well spotted!
>
> Hint: run always `make W=1` when building kernel.
Ah. I thought I had that and sparse enabled. It seems I disabled all
extra checks from my build scripts a while ago to speed-up compilation
when I was bisecting...
> It will show defined but not used cases and combined with nowadays
> default -Werror won't be compilable.
>
Thanks for the review.
--Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/13/23 15:29, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>> Jonathan Cameron <[email protected]> wrote:
>
> ...
>
>>>> Ah. I forgot the tests that don't have a device so can't use devm.
>>>
>>> Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
>>> for that.
>>
>> I'd appreciated any pointer for such an example if you have one at hand. (I
>> can do the digging if you don't though!)
>>
>> I am not a fan of unit tests. They add huge amount of inertia to
>> development, and in worst case, they stop people from contributing where
>> improving a feature requires test code modification(s). And harder the test
>> code is to understand, worse the unwanted side-effects. Also, harder the
>> test code is to read, more time and effort it requires to analyze a test
>> failure... Hence, I am _very_ conservative what comes to adding size of test
>> code with anything that is not strictly required.
>>
>> After that being said, unit tests are a great tool when carefully used - and
>> I assume/hope stubbing a device for devm_ tests does not add much extra...
>> But let me see if I can find an example :)
>
> drivers/gpu/drm/tests/drm_managed_test.c ?
>
> (somewhere underneath:
>
> ret = platform_driver_register(&fake_platform_driver);
>
> which suggests... what exactly? :-)
>
Thanks!
--Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, Mar 13, 2023 at 03:59:03PM +0200, Matti Vaittinen wrote:
> On 3/13/23 15:25, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
> > > On 3/6/23 14:52, Andy Shevchenko wrote:
> > > > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
...
> > > > > + if (ret && gts->avail_all_scales_table)
> > > >
> > > > In one case you commented that free(NULL) is okay, in the other, you add
> > > > a duplicative check. Why?
> > >
> > > Sorry but what do you mean by dublicative check?
> > >
> > > Usually I avoid the kfree(NULL). That's why I commented on it in that
> > > another case where it was not explicitly disallowed. I'll change that for v4
> > > to avoid kfree(NULL) as you suggested.
> >
> > So, and with it you put now a double check for NULL, do you think it's okay?
> > I don't.
>
> I don't see the double check. I see only one check just above the kfree()?
> Where is the other check?
if (... gts->avail_all_scales_table)
is a double to one, which is inside kfree(). I.o.w. kfree() is NULL-aware
and you know that.
> > > > > + kfree(gts->avail_all_scales_table);
--
With Best Regards,
Andy Shevchenko
On 3/13/23 16:17, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 03:59:03PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 15:25, Andy Shevchenko wrote:
>>> On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
>>>> On 3/6/23 14:52, Andy Shevchenko wrote:
>>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>
> ...
>
>>>>>> + if (ret && gts->avail_all_scales_table)
>>>>>
>>>>> In one case you commented that free(NULL) is okay, in the other, you add
>>>>> a duplicative check. Why?
>>>>
>>>> Sorry but what do you mean by dublicative check?
>>>>
>>>> Usually I avoid the kfree(NULL). That's why I commented on it in that
>>>> another case where it was not explicitly disallowed. I'll change that for v4
>>>> to avoid kfree(NULL) as you suggested.
>>>
>>> So, and with it you put now a double check for NULL, do you think it's okay?
>>> I don't.
>>
>> I don't see the double check. I see only one check just above the kfree()?
>> Where is the other check?
>
> if (... gts->avail_all_scales_table)
>
> is a double to one, which is inside kfree(). I.o.w. kfree() is NULL-aware
> and you know that.
Ah. I thought you suggested I had double check in the code I wrote. Now
I see what you meant.
Yes, I think that check should be dropped.
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/13/23 15:14, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
>> On 3/12/23 18:51, Jonathan Cameron wrote:
>>> On Mon, 6 Mar 2023 14:52:57 +0200
>>> Andy Shevchenko <[email protected]> wrote:
>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>
> ...
>
>>>>> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
>>>>
>>>> I would say _HELPER part is too much, but fine with me.
>>>
>>> Hmm. I think I like the HELPER bit as separates it from being a driver.
>>> Of course I might change my mind after a few sleeps.
>>
>> Ever considered a career as a politician? ;) (No offense intended - and feel
>> free to change your mind on this. I don't expect this to be done tomorrow)
>
> It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE
> definition.
Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had
forgot it. My memory has never been great and seems to be getting worse
all the time...
I don't know what to think of this define though. I can imagine that
someone who is not familiar with it could be very confused as to why the
symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are
used. OTOH, I think I once saw an error about symbols being in a
namespace (when trying to use one without the namespace). This should
probably just be a good enough hint for finding out what's going on.
Luckily, I think all the exports in this case were oneliners even with
the namespace explicitly spelled. Well, I think that for one or two
exports the semicolon did slip to col 81 or 82 - but I am not sure if
fixing this weighs more than the clarity of explicitly showing the
namespace in export.
Well, I guess I can go with either of these ways - do you have a strong
opinion on using the DEFAULT_SYMBOL_NAMESPACE?
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, Mar 14, 2023 at 06:19:35AM +0000, Vaittinen, Matti wrote:
> On 3/13/23 15:14, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
> >> On 3/12/23 18:51, Jonathan Cameron wrote:
> >>> On Mon, 6 Mar 2023 14:52:57 +0200
> >>> Andy Shevchenko <[email protected]> wrote:
> >>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
...
> >>>>> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
> >>>>
> >>>> I would say _HELPER part is too much, but fine with me.
> >>>
> >>> Hmm. I think I like the HELPER bit as separates it from being a driver.
> >>> Of course I might change my mind after a few sleeps.
> >>
> >> Ever considered a career as a politician? ;) (No offense intended - and feel
> >> free to change your mind on this. I don't expect this to be done tomorrow)
> >
> > It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE
> > definition.
>
> Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had
> forgot it. My memory has never been great and seems to be getting worse
> all the time...
>
> I don't know what to think of this define though. I can imagine that
> someone who is not familiar with it could be very confused as to why the
> symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are
> used. OTOH, I think I once saw an error about symbols being in a
> namespace (when trying to use one without the namespace). This should
> probably just be a good enough hint for finding out what's going on.
>
> Luckily, I think all the exports in this case were oneliners even with
> the namespace explicitly spelled. Well, I think that for one or two
> exports the semicolon did slip to col 81 or 82 - but I am not sure if
> fixing this weighs more than the clarity of explicitly showing the
> namespace in export.
>
> Well, I guess I can go with either of these ways - do you have a strong
> opinion on using the DEFAULT_SYMBOL_NAMESPACE?
If you asking me, I'm fine with either way. Usually the latter makes sense
when we expect APIs in the certain module to:
1) always belong to the single namespace, *and / or*
2) be expanded in the future w/o bothering about their (default) NS, *and not*
3) be a single exported function for the feasible future.
Also you made a good point about line length, but with all respect, I prefer
100 than 80 and I do not believe we ever will have function name + NS longer
than that.
--
With Best Regards,
Andy Shevchenko
On 3/13/23 15:59, Matti Vaittinen wrote:
> On 3/13/23 15:29, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>>> Jonathan Cameron <[email protected]> wrote:
>>
>> ...
>>
>>>>> Ah. I forgot the tests that don't have a device so can't use devm.
>>>>
>>>> Why not? I have seen, IIRC, test cases inside the kernel that fakes
>>>> the device
>>>> for that.
>>>
>>> I'd appreciated any pointer for such an example if you have one at
>>> hand. (I
>>> can do the digging if you don't though!)
>>>
>>> I am not a fan of unit tests. They add huge amount of inertia to
>>> development, and in worst case, they stop people from contributing where
>>> improving a feature requires test code modification(s). And harder
>>> the test
>>> code is to understand, worse the unwanted side-effects. Also, harder the
>>> test code is to read, more time and effort it requires to analyze a test
>>> failure... Hence, I am _very_ conservative what comes to adding size
>>> of test
>>> code with anything that is not strictly required.
>>>
>>> After that being said, unit tests are a great tool when carefully
>>> used - and
>>> I assume/hope stubbing a device for devm_ tests does not add much
>>> extra...
>>> But let me see if I can find an example :)
>>
>> drivers/gpu/drm/tests/drm_managed_test.c ?
>>
>> (somewhere underneath:
>>
>> ret = platform_driver_register(&fake_platform_driver);
>>
>> which suggests... what exactly? :-)
Thanks to pointer from Andy I found the
drm_kunit_helper_[alloc/free]_device() functions. I renamed them to
test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add
declarations to include/kunit/test-helpers.h fixed KConfigs and existing
callers + added the tests for managed interfaces. I have this in place
in my personal playground where I am working towards the v4 of the series.
...
After that I asked from Maxime if he had a reason to not make those
generic and available to other subsystems besides drm in the first place...
And Maxime was kind enough to point me to the fact that something like
this was done in the CCF context:
https://lore.kernel.org/all/[email protected]/
I like the 'single function to get the dummy device which can be passed
to devm'-approach used in drm helpers. I do also like Stephen's idea of
having the prototypes in kunit/platform_device.h which matches the
linux/platform_device.h.
However, I don't know when Stephen's work will be finished and merged to
IIO-tree so that it could be used/extended for the needs of these tests.
Meanwhile, I don't think it makes sense to go forward with my changes
splitting the helpers out of drm until we see what Stephen's changes
will bring us. On the other hand, I don't like delaying the gts-helpers
or the sensor drivers.
So, any suggestions what I should do? I see following options:
1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the
dummy device for devm.
3) Add the tests using the helpers from drm as they are now.
option 1):
I like it as it would be an easy way (for now) - but I hate it as it may
be a hard way as well. In my experience, when a driver/helper lands
upstream it will get first few fixes quite fast - and not having a test
available upstream when this happens is bad. Bad because it means the
out-of-tree test may get broken, and bad because there is no easy way to
test the fixes.
option 2):
I hate it because it makes the test code more complex - and duplicates
the kernel code which is never nice. This could be reworked later when
Stephens work is done though.
option 3):
It's in general not nice to use functions exported for some other
subsystem's specific purposes. This would however keep the test code at
minimum, while leaving the same "I swear I'll fix this later when
dependencies have settled" - possibility as option 2) did.
Oh, in theory there is option 4) to just send out the changes I did(*)
which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM -
but I guess that would lead some extra work to merge this later with
stuff Stephen's series does introduce.
Any suggestions which of the options to proceed with?
(*) For those interested in seeing the result of pulling the
drm_kunit_helper_[alloc/free]_device() out of DRM tests, below are links
to my personal playground with following remarks:
1) code one finds from there may be 100% untested
2) code one finds there may be written just for fun, or for a very
specific purpose
3) code one finds there is generally not maintained, may be rebased, may
vanish or turn into rabbits or turn you into a rabbit when you wear a
top hat.
commits to look at there are
https://github.com/M-Vaittinen/linux/commit/15d07e799f7c7fddc91030b16266d4a8bbaf1cc1
https://github.com/M-Vaittinen/linux/commit/6b4c4ba38b1f838fb0074befd2ca8734604464da
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
> On 3/13/23 15:59, Matti Vaittinen wrote:
> > On 3/13/23 15:29, Andy Shevchenko wrote:
> > > On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> > > > On 3/13/23 14:40, Andy Shevchenko wrote:
> > > > > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > > > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > > > > Jonathan Cameron <[email protected]> wrote:
...
> > > > > > Ah. I forgot the tests that don't have a device so can't use devm.
> > > > >
> > > > > Why not? I have seen, IIRC, test cases inside the kernel
> > > > > that fakes the device
> > > > > for that.
> > > >
> > > > I'd appreciated any pointer for such an example if you have one
> > > > at hand. (I
> > > > can do the digging if you don't though!)
> > > >
> > > > I am not a fan of unit tests. They add huge amount of inertia to
> > > > development, and in worst case, they stop people from contributing where
> > > > improving a feature requires test code modification(s). And
> > > > harder the test
> > > > code is to understand, worse the unwanted side-effects. Also, harder the
> > > > test code is to read, more time and effort it requires to analyze a test
> > > > failure... Hence, I am _very_ conservative what comes to adding
> > > > size of test
> > > > code with anything that is not strictly required.
> > > >
> > > > After that being said, unit tests are a great tool when
> > > > carefully used - and
> > > > I assume/hope stubbing a device for devm_ tests does not add
> > > > much extra...
> > > > But let me see if I can find an example :)
> > >
> > > drivers/gpu/drm/tests/drm_managed_test.c ?
> > >
> > > (somewhere underneath:
> > >
> > > ? ret = platform_driver_register(&fake_platform_driver);
> > >
> > > which suggests... what exactly? :-)
>
> Thanks to pointer from Andy I found the
> drm_kunit_helper_[alloc/free]_device() functions. I renamed them to
> test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add
> declarations to include/kunit/test-helpers.h fixed KConfigs and existing
> callers + added the tests for managed interfaces. I have this in place in my
> personal playground where I am working towards the v4 of the series.
>
> ...
>
> After that I asked from Maxime if he had a reason to not make those generic
> and available to other subsystems besides drm in the first place...
>
> And Maxime was kind enough to point me to the fact that something like this
> was done in the CCF context:
> https://lore.kernel.org/all/[email protected]/
>
> I like the 'single function to get the dummy device which can be passed to
> devm'-approach used in drm helpers. I do also like Stephen's idea of having
> the prototypes in kunit/platform_device.h which matches the
> linux/platform_device.h.
>
> However, I don't know when Stephen's work will be finished and merged to
> IIO-tree so that it could be used/extended for the needs of these tests.
>
> Meanwhile, I don't think it makes sense to go forward with my changes
> splitting the helpers out of drm until we see what Stephen's changes will
> bring us. On the other hand, I don't like delaying the gts-helpers or the
> sensor drivers.
>
> So, any suggestions what I should do? I see following options:
>
> 1) Drop the tests for managed interfaces for now.
> 2) Add the tests with a yet-another duplicate implementation of the
> dummy device for devm.
> 3) Add the tests using the helpers from drm as they are now.
>
> option 1):
> I like it as it would be an easy way (for now) - but I hate it as it may be
> a hard way as well. In my experience, when a driver/helper lands upstream it
> will get first few fixes quite fast - and not having a test available
> upstream when this happens is bad. Bad because it means the out-of-tree test
> may get broken, and bad because there is no easy way to test the fixes.
>
> option 2):
> I hate it because it makes the test code more complex - and duplicates the
> kernel code which is never nice. This could be reworked later when Stephens
> work is done though.
>
> option 3):
> It's in general not nice to use functions exported for some other
> subsystem's specific purposes. This would however keep the test code at
> minimum, while leaving the same "I swear I'll fix this later when
> dependencies have settled" - possibility as option 2) did.
>
> Oh, in theory there is option 4) to just send out the changes I did(*) which
> pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess
> that would lead some extra work to merge this later with stuff Stephen's
> series does introduce.
>
> Any suggestions which of the options to proceed with?
>
> (*) For those interested in seeing the result of pulling the
> drm_kunit_helper_[alloc/free]_device() out of DRM tests, below are links to
> my personal playground with following remarks:
> 1) code one finds from there may be 100% untested
> 2) code one finds there may be written just for fun, or for a very
> specific purpose
> 3) code one finds there is generally not maintained, may be rebased, may
> vanish or turn into rabbits or turn you into a rabbit when you wear a
> top hat.
>
> commits to look at there are
> https://github.com/M-Vaittinen/linux/commit/15d07e799f7c7fddc91030b16266d4a8bbaf1cc1
> https://github.com/M-Vaittinen/linux/commit/6b4c4ba38b1f838fb0074befd2ca8734604464da
My opinion on that, you should not depend on the others work if they are slow.
It might take ages for them to finish and upstream that. Why should you wait?
OTOH you may inform them with your patches that may have a chance to land before
theirs. It might motivate them to speed up their work and coordinate the outcome
that all stakeholders will be happy.
That said, I would choose option 4), i.e. provide a series where you decouple
thingy from DRM and Cc to Stephen with a cover letter note that explains why
you chosen this way and what alternatives you are open to.
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 15, 2023 at 04:12:59PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
...
> My opinion on that, you should not depend on the others work if they are slow.
> It might take ages for them to finish and upstream that. Why should you wait?
>
> OTOH you may inform them with your patches that may have a chance to land before
> theirs. It might motivate them to speed up their work and coordinate the outcome
> that all stakeholders will be happy.
>
> That said, I would choose option 4), i.e. provide a series where you decouple
> thingy from DRM and Cc to Stephen with a cover letter note that explains why
> you chosen this way and what alternatives you are open to.
Ah, and before doing that you actually may ask Stephen on his roadmap about
that. If no answer for a week or so, go with option 4.
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
> On 3/13/23 15:59, Matti Vaittinen wrote:
> > On 3/13/23 15:29, Andy Shevchenko wrote:
> > > On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> > > > On 3/13/23 14:40, Andy Shevchenko wrote:
> > > > > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > > > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > > > > Jonathan Cameron <[email protected]> wrote:
> > >
> > > ...
> > >
> > > > > > Ah. I forgot the tests that don't have a device so can't use devm.
> > > > >
> > > > > Why not? I have seen, IIRC, test cases inside the kernel
> > > > > that fakes the device
> > > > > for that.
> > > >
> > > > I'd appreciated any pointer for such an example if you have one
> > > > at hand. (I
> > > > can do the digging if you don't though!)
> > > >
> > > > I am not a fan of unit tests. They add huge amount of inertia to
> > > > development, and in worst case, they stop people from contributing where
> > > > improving a feature requires test code modification(s). And
> > > > harder the test
> > > > code is to understand, worse the unwanted side-effects. Also, harder the
> > > > test code is to read, more time and effort it requires to analyze a test
> > > > failure... Hence, I am _very_ conservative what comes to adding
> > > > size of test
> > > > code with anything that is not strictly required.
> > > >
> > > > After that being said, unit tests are a great tool when
> > > > carefully used - and
> > > > I assume/hope stubbing a device for devm_ tests does not add
> > > > much extra...
> > > > But let me see if I can find an example :)
> > >
> > > drivers/gpu/drm/tests/drm_managed_test.c ?
> > >
> > > (somewhere underneath:
> > >
> > > ? ret = platform_driver_register(&fake_platform_driver);
> > >
> > > which suggests... what exactly? :-)
>
> Thanks to pointer from Andy I found the
> drm_kunit_helper_[alloc/free]_device() functions. I renamed them to
> test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add
> declarations to include/kunit/test-helpers.h fixed KConfigs and existing
> callers + added the tests for managed interfaces. I have this in place in my
> personal playground where I am working towards the v4 of the series.
>
> ...
>
> After that I asked from Maxime if he had a reason to not make those generic
> and available to other subsystems besides drm in the first place...
>
> And Maxime was kind enough to point me to the fact that something like this
> was done in the CCF context:
> https://lore.kernel.org/all/[email protected]/
>
> I like the 'single function to get the dummy device which can be passed to
> devm'-approach used in drm helpers. I do also like Stephen's idea of having
> the prototypes in kunit/platform_device.h which matches the
> linux/platform_device.h.
>
> However, I don't know when Stephen's work will be finished and merged to
> IIO-tree so that it could be used/extended for the needs of these tests.
>
> Meanwhile, I don't think it makes sense to go forward with my changes
> splitting the helpers out of drm until we see what Stephen's changes will
> bring us. On the other hand, I don't like delaying the gts-helpers or the
> sensor drivers.
>
> So, any suggestions what I should do? I see following options:
>
> 1) Drop the tests for managed interfaces for now.
> 2) Add the tests with a yet-another duplicate implementation of the
> dummy device for devm.
> 3) Add the tests using the helpers from drm as they are now.
>
> option 1):
> I like it as it would be an easy way (for now) - but I hate it as it may be
> a hard way as well. In my experience, when a driver/helper lands upstream it
> will get first few fixes quite fast - and not having a test available
> upstream when this happens is bad. Bad because it means the out-of-tree test
> may get broken, and bad because there is no easy way to test the fixes.
>
> option 2):
> I hate it because it makes the test code more complex - and duplicates the
> kernel code which is never nice. This could be reworked later when Stephens
> work is done though.
>
> option 3):
> It's in general not nice to use functions exported for some other
> subsystem's specific purposes. This would however keep the test code at
> minimum, while leaving the same "I swear I'll fix this later when
> dependencies have settled" - possibility as option 2) did.
>
> Oh, in theory there is option 4) to just send out the changes I did(*) which
> pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess
> that would lead some extra work to merge this later with stuff Stephen's
> series does introduce.
>
> Any suggestions which of the options to proceed with?
I think the best course of action would be to synchronize with Stephen,
and make sure that whatever patch you're doing can be used for his work.
Once it works for both of you, then I guess it can go through the kunit
tree and you will use it both.
Maxime
On 3/17/23 12:19, Maxime Ripard wrote:
> On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 15:59, Matti Vaittinen wrote:
>>> On 3/13/23 15:29, Andy Shevchenko wrote:
>>>> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>>>>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>>>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>>>>> Jonathan Cameron <[email protected]> wrote:
>>>>
>>>> ...
>>>>
>>>>>>> Ah. I forgot the tests that don't have a device so can't use devm.
>>>>>>
>>>>>> Why not? I have seen, IIRC, test cases inside the kernel
>>>>>> that fakes the device
>>>>>> for that.
>>>>>
>>>>> I'd appreciated any pointer for such an example if you have one
>>>>> at hand. (I
>>>>> can do the digging if you don't though!)
>>>>>
>>>>> I am not a fan of unit tests. They add huge amount of inertia to
>>>>> development, and in worst case, they stop people from contributing where
>>>>> improving a feature requires test code modification(s). And
>>>>> harder the test
>>>>> code is to understand, worse the unwanted side-effects. Also, harder the
>>>>> test code is to read, more time and effort it requires to analyze a test
>>>>> failure... Hence, I am _very_ conservative what comes to adding
>>>>> size of test
>>>>> code with anything that is not strictly required.
>>>>>
>>>>> After that being said, unit tests are a great tool when
>>>>> carefully used - and
>>>>> I assume/hope stubbing a device for devm_ tests does not add
>>>>> much extra...
>>>>> But let me see if I can find an example :)
>>>>
>>>> drivers/gpu/drm/tests/drm_managed_test.c ?
>>>>
>>>> (somewhere underneath:
>>>>
>>>> ret = platform_driver_register(&fake_platform_driver);
>>>>
>>>> which suggests... what exactly? :-)
>>
>> Thanks to pointer from Andy I found the
>> drm_kunit_helper_[alloc/free]_device() functions. I renamed them to
>> test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add
>> declarations to include/kunit/test-helpers.h fixed KConfigs and existing
>> callers + added the tests for managed interfaces. I have this in place in my
>> personal playground where I am working towards the v4 of the series.
>>
>> ...
>>
>> After that I asked from Maxime if he had a reason to not make those generic
>> and available to other subsystems besides drm in the first place...
>>
>> And Maxime was kind enough to point me to the fact that something like this
>> was done in the CCF context:
>> https://lore.kernel.org/all/[email protected]/
>>
>> I like the 'single function to get the dummy device which can be passed to
>> devm'-approach used in drm helpers. I do also like Stephen's idea of having
>> the prototypes in kunit/platform_device.h which matches the
>> linux/platform_device.h.
>>
>> However, I don't know when Stephen's work will be finished and merged to
>> IIO-tree so that it could be used/extended for the needs of these tests.
>>
>> Meanwhile, I don't think it makes sense to go forward with my changes
>> splitting the helpers out of drm until we see what Stephen's changes will
>> bring us. On the other hand, I don't like delaying the gts-helpers or the
>> sensor drivers.
>>
>> So, any suggestions what I should do? I see following options:
>>
>> 1) Drop the tests for managed interfaces for now.
>> 2) Add the tests with a yet-another duplicate implementation of the
>> dummy device for devm.
>> 3) Add the tests using the helpers from drm as they are now.
>>
>> option 1):
>> I like it as it would be an easy way (for now) - but I hate it as it may be
>> a hard way as well. In my experience, when a driver/helper lands upstream it
>> will get first few fixes quite fast - and not having a test available
>> upstream when this happens is bad. Bad because it means the out-of-tree test
>> may get broken, and bad because there is no easy way to test the fixes.
>>
>> option 2):
>> I hate it because it makes the test code more complex - and duplicates the
>> kernel code which is never nice. This could be reworked later when Stephens
>> work is done though.
>>
>> option 3):
>> It's in general not nice to use functions exported for some other
>> subsystem's specific purposes. This would however keep the test code at
>> minimum, while leaving the same "I swear I'll fix this later when
>> dependencies have settled" - possibility as option 2) did.
>>
>> Oh, in theory there is option 4) to just send out the changes I did(*) which
>> pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess
>> that would lead some extra work to merge this later with stuff Stephen's
>> series does introduce.
>>
>> Any suggestions which of the options to proceed with?
>
> I think the best course of action would be to synchronize with Stephen,
> and make sure that whatever patch you're doing can be used for his work.
>
> Once it works for both of you, then I guess it can go through the kunit
> tree and you will use it both.
Thanks Andy and Maxime! I appreciate your help.
I guess I'll ping Stephen with a separate mail (although he's in CC
here) and CC the relevant patches to him as well. I assume that gives
him a chance to take a look what I am doing and suggest changes if needed.
My contribution is anyways small - it's mostly just renaming and moving
those two SRm helper APIs + changing the callers. Most of that is
probably outside the scope of his work - it would just fit the same
files he will be adding =)
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, 14 Mar 2023 06:19:35 +0000
"Vaittinen, Matti" <[email protected]> wrote:
> On 3/13/23 15:14, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
> >> On 3/12/23 18:51, Jonathan Cameron wrote:
> >>> On Mon, 6 Mar 2023 14:52:57 +0200
> >>> Andy Shevchenko <[email protected]> wrote:
> >>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> >
> > ...
> >
> >>>>> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
> >>>>
> >>>> I would say _HELPER part is too much, but fine with me.
> >>>
> >>> Hmm. I think I like the HELPER bit as separates it from being a driver.
> >>> Of course I might change my mind after a few sleeps.
> >>
> >> Ever considered a career as a politician? ;) (No offense intended - and feel
> >> free to change your mind on this. I don't expect this to be done tomorrow)
> >
> > It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE
> > definition.
>
> Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had
> forgot it. My memory has never been great and seems to be getting worse
> all the time...
>
> I don't know what to think of this define though. I can imagine that
> someone who is not familiar with it could be very confused as to why the
> symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are
> used. OTOH, I think I once saw an error about symbols being in a
> namespace (when trying to use one without the namespace). This should
> probably just be a good enough hint for finding out what's going on.
>
> Luckily, I think all the exports in this case were oneliners even with
> the namespace explicitly spelled. Well, I think that for one or two
> exports the semicolon did slip to col 81 or 82 - but I am not sure if
> fixing this weighs more than the clarity of explicitly showing the
> namespace in export.
>
> Well, I guess I can go with either of these ways - do you have a strong
> opinion on using the DEFAULT_SYMBOL_NAMESPACE?
>
If it's in the C file, then I can cope with doing it this way.
Don't do it in the compiler options though. That got ripped out of CXL
because it was considered a bad idea to hide the namespace away like that.
Personally I prefer the namespace of the symbols explicit in each export
as they are easy to find that way.
>
> Yours,
> --Matti
>
> >>> +/*
> >>
> >> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> >> It's really a provocative to some people who will come with a patches to "fix"
> >> this...
> >
> > Just make it kernel-doc.
> >
>
> Are you sure...? I don't like the idea of polluting generated docs with
> documentation for this type of tiny internal pieces not usable outside
> this component anyways...
You can use :internal or :external to pick up only the docs you want
when including this stuff form a .rst file.
https://docs.kernel.org/doc-guide/kernel-doc.html
>
> >>
> >>> + * iio_gts_get_gain - Convert scale to total gain
> >>> + *
> >>> + * Internal helper for converting scale to total gain.
> >>> + *
> >>> + * @max: Maximum linearized scale. As an example, when scale is created
> >>> + * in magnitude of NANOs and max scale is 64.1 - The linearized
> >>> + * scale is 64 100 000 000.
> >>> + * @scale: Linearized scale to compte the gain for.
> >>> + *
> >>> + * Return: (floored) gain corresponding to the scale. -EINVAL if scale
> >>> + * is invalid.
> >>> + */
> >
> >> ...
> >>
> >
> >>>> + kfree(gts->avail_all_scales_table);
> >
> > ...
> >
> >>>> + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> >>>
> >>> sizeof(type) is error prone in comparison to sizeof(*var).
> >>
> >> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
> >> longer a pointer as having pointers to pointers is not _that_ common. When
> >> we see sizeof(type *) - we instantly know it is a size of a pointer and not
> >> a size of some other type.
> >>
> >> So yes, while having sizeof(*var) makes us tolerant to errors caused by
> >> variable type changes - it makes us prone to human reader errors. Also, if
> >> someone changes type of *var from pointer to some other type - then he/she
> >> is likely to in any case need to revise the array alloactions too.
> >>
> >> While I in general agree with you that the sizeof(variable) is better than
> >> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.
> >
> > Still get a fundamental disagreement on this. I would insist, but I'm not
> > a maintainer, so you are lucky :-) if Jonathan will not force you to follow
> > my way.
>
> In a code you are maintaining it is good to have it in your way as
> you're responsible for it. This is also why I insist on having things in
> a way I can read best for a code I plan to maintain - unless the
> subsystem maintainers see it hard to maintain for them. So, let's see if
> Jonathan has strong opinions on this one :)
This is one where I strongly prefer sizeof(*per_time_gains)
because it's easier to review. I don't care so much if it's easier to
modify as reality is these rarely get modified.
I often just 'fix' these up whilst applying.
Jonathan
On 3/18/23 19:17, Jonathan Cameron wrote:
> On Tue, 14 Mar 2023 06:19:35 +0000
> "Vaittinen, Matti" <[email protected]> wrote:
>
>> On 3/13/23 15:14, Andy Shevchenko wrote:
>>> On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
>>>> On 3/12/23 18:51, Jonathan Cameron wrote:
>>>>> On Mon, 6 Mar 2023 14:52:57 +0200
>>>>> Andy Shevchenko <[email protected]> wrote:
>>>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>>>
>>> ...
>>>
>>>>>>> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
>>>>>>
>>>>>> I would say _HELPER part is too much, but fine with me.
>>>>>
>>>>> Hmm. I think I like the HELPER bit as separates it from being a driver.
>>>>> Of course I might change my mind after a few sleeps.
>>>>
>>>> Ever considered a career as a politician? ;) (No offense intended - and feel
>>>> free to change your mind on this. I don't expect this to be done tomorrow)
>>>
>>> It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE
>>> definition.
>>
>> Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had
>> forgot it. My memory has never been great and seems to be getting worse
>> all the time...
>
>>
>> I don't know what to think of this define though. I can imagine that
>> someone who is not familiar with it could be very confused as to why the
>> symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are
>> used. OTOH, I think I once saw an error about symbols being in a
>> namespace (when trying to use one without the namespace). This should
>> probably just be a good enough hint for finding out what's going on.
>>
>> Luckily, I think all the exports in this case were oneliners even with
>> the namespace explicitly spelled. Well, I think that for one or two
>> exports the semicolon did slip to col 81 or 82 - but I am not sure if
>> fixing this weighs more than the clarity of explicitly showing the
>> namespace in export.
>>
>> Well, I guess I can go with either of these ways - do you have a strong
>> opinion on using the DEFAULT_SYMBOL_NAMESPACE?
>>
>
> If it's in the C file, then I can cope with doing it this way.
> Don't do it in the compiler options though. That got ripped out of CXL
> because it was considered a bad idea to hide the namespace away like that.
>
> Personally I prefer the namespace of the symbols explicit in each export
> as they are easy to find that way.
I share the same view on this. I did use the DEFAULT_SYMBOL_NAMESPACE
for v4 - but I'll drop that for v5 and go back with the explicit
name-space usage.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~