[PATCH 0/2] iio: light: fix scale in veml6030

[PATCH 0/2] iio: light: fix scale in veml6030

[Javier Carrasco]

This series follows a similar approach as recently used for the veml3235
by using iio-gts to manage the scale as stated in the ABI. In its
current form, the driver exposes the hardware gain instead of the
multiplier for the raw value to obtain a value in lux.

Although this driver and the veml3235 have many similarities, there are
two main differences in this series compared to the one used to fix the
other driver:

- The veml6030 has fractional gains, which are not supported by the
  iio-gts helpers. My first attempt was adding support for them, but
  that made the whole iio-gts implementation more complex, cumbersome,
  and the risk of affecting existing clients was not negligible.
  Instead, a x8 factor has been used for the hardware gain to present
  the minimum value (x0.125) as x1, keeping linearity. The scales
  iio-gts generates are therefore right without any extra conversion,
  and they match the values provided in the different datasheets.

- This driver included a processed value for the ambient light, maybe
  because the scale did not follow the ABI and the conversion was not
  direct. To avoid breaking userspace, the functionality has been kept,
  but of course using the fixed scales. That requires using intermediate
  u64 values u64 divisions via div_u64() and do_div() to avoid overflows.

To ease the usage of the iio-gts selectors, a previous patch to support
regfields and caching has been included.

This issue has been present since the original implementation, and it
affects all devices it supports.

This series has been tested with a veml7700 (same gains as veml6030) and
a veml6035 with positive results.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      iio: light: veml6030: extend regmap to support regfields and caching
      iio: light: veml6030: fix scale to conform to ABI

 drivers/iio/light/Kconfig    |   1 +
 drivers/iio/light/veml6030.c | 594 ++++++++++++++++++++-----------------------
 2 files changed, 282 insertions(+), 313 deletions(-)
---
base-commit: 577a66e2e634f712384c57a98f504c44ea4b47da
change-id: 20241231-veml6030-scale-8142f387e7e6

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/2] iio: light: veml6030: extend regmap to support regfields and caching

[Javier Carrasco]

The configuration registers are not volatile and are not affected
by read operations (i.e. not precious), making them suitable to be
cached in order to reduce the number of accesses to the device.

Add support for regfields as well to simplify register operations,
taking into account the different fields for the veml6030/veml7700 and
veml6035.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6030.c | 141 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 116 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index 9b71825eea9bee2146be17ed2f30f5a8f7ad37e3..a6385c6d3fba59a6b22845a3c5e252b619faed65 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -65,6 +65,11 @@ enum veml6030_scan {
 	VEML6030_SCAN_TIMESTAMP,
 };
 
+struct veml6030_rf {
+	struct regmap_field *it;
+	struct regmap_field *gain;
+};
+
 struct veml603x_chip {
 	const char *name;
 	const int(*scale_vals)[][2];
@@ -75,6 +80,7 @@ struct veml603x_chip {
 	int (*set_info)(struct iio_dev *indio_dev);
 	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
 	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
+	int (*regfield_init)(struct iio_dev *indio_dev);
 };
 
 /*
@@ -91,6 +97,7 @@ struct veml603x_chip {
 struct veml6030_data {
 	struct i2c_client *client;
 	struct regmap *regmap;
+	struct veml6030_rf rf;
 	int cur_resolution;
 	int cur_gain;
 	int cur_integration_time;
@@ -319,28 +326,59 @@ static const struct iio_chan_spec veml7700_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(VEML6030_SCAN_TIMESTAMP),
 };
 
+static const struct regmap_range veml6030_readable_ranges[] = {
+	regmap_reg_range(VEML6030_REG_ALS_CONF, VEML6030_REG_ALS_INT),
+};
+
+static const struct regmap_access_table veml6030_readable_table = {
+	.yes_ranges = veml6030_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6030_readable_ranges),
+};
+
+static const struct regmap_range veml6030_writable_ranges[] = {
+	regmap_reg_range(VEML6030_REG_ALS_CONF, VEML6030_REG_ALS_PSM),
+};
+
+static const struct regmap_access_table veml6030_writable_table = {
+	.yes_ranges = veml6030_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6030_writable_ranges),
+};
+
+static const struct regmap_range veml6030_volatile_ranges[] = {
+	regmap_reg_range(VEML6030_REG_ALS_DATA, VEML6030_REG_WH_DATA),
+};
+
+static const struct regmap_access_table veml6030_volatile_table = {
+	.yes_ranges = veml6030_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6030_volatile_ranges),
+};
+
 static const struct regmap_config veml6030_regmap_config = {
 	.name = "veml6030_regmap",
 	.reg_bits = 8,
 	.val_bits = 16,
 	.max_register = VEML6030_REG_ALS_INT,
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.rd_table = &veml6030_readable_table,
+	.wr_table = &veml6030_writable_table,
+	.volatile_table = &veml6030_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static int veml6030_get_intgrn_tm(struct iio_dev *indio_dev,
 						int *val, int *val2)
 {
-	int ret, reg;
+	int it_idx, ret;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
+	ret = regmap_field_read(data->rf.it, &it_idx);
 	if (ret) {
 		dev_err(&data->client->dev,
 				"can't read als conf register %d\n", ret);
 		return ret;
 	}
 
-	switch ((reg >> 6) & 0xF) {
+	switch (it_idx) {
 	case 0:
 		*val2 = 100000;
 		break;
@@ -405,8 +443,7 @@ static int veml6030_set_intgrn_tm(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
-	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
-					VEML6030_ALS_IT, new_int_time);
+	ret = regmap_field_write(data->rf.it, new_int_time);
 	if (ret) {
 		dev_err(&data->client->dev,
 				"can't update als integration time %d\n", ret);
@@ -510,23 +547,22 @@ static int veml6030_set_als_gain(struct iio_dev *indio_dev,
 	struct veml6030_data *data = iio_priv(indio_dev);
 
 	if (val == 0 && val2 == 125000) {
-		new_gain = 0x1000; /* 0x02 << 11 */
+		new_gain = 0x01;
 		gain_idx = 3;
 	} else if (val == 0 && val2 == 250000) {
-		new_gain = 0x1800;
+		new_gain = 0x11;
 		gain_idx = 2;
 	} else if (val == 1 && val2 == 0) {
 		new_gain = 0x00;
 		gain_idx = 1;
 	} else if (val == 2 && val2 == 0) {
-		new_gain = 0x800;
+		new_gain = 0x01;
 		gain_idx = 0;
 	} else {
 		return -EINVAL;
 	}
 
-	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
-					VEML6030_ALS_GAIN, new_gain);
+	ret = regmap_field_write(data->rf.gain, new_gain);
 	if (ret) {
 		dev_err(&data->client->dev,
 				"can't set als gain %d\n", ret);
@@ -544,30 +580,31 @@ static int veml6035_set_als_gain(struct iio_dev *indio_dev, int val, int val2)
 	struct veml6030_data *data = iio_priv(indio_dev);
 
 	if (val == 0 && val2 == 125000) {
-		new_gain = VEML6035_SENS;
+		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS);
 		gain_idx = 5;
 	} else if (val == 0 && val2 == 250000) {
-		new_gain = VEML6035_SENS | VEML6035_GAIN;
+		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS |
+				      VEML6035_GAIN);
 		gain_idx = 4;
 	} else if (val == 0 && val2 == 500000) {
-		new_gain = VEML6035_SENS | VEML6035_GAIN |
-			VEML6035_DG;
+		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS |
+				      VEML6035_GAIN | VEML6035_DG);
 		gain_idx = 3;
 	} else if (val == 1 && val2 == 0) {
 		new_gain = 0x0000;
 		gain_idx = 2;
 	} else if (val == 2 && val2 == 0) {
-		new_gain = VEML6035_GAIN;
+		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN);
 		gain_idx = 1;
 	} else if (val == 4 && val2 == 0) {
-		new_gain = VEML6035_GAIN | VEML6035_DG;
+		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN |
+				      VEML6035_DG);
 		gain_idx = 0;
 	} else {
 		return -EINVAL;
 	}
 
-	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_CONF,
-				 VEML6035_GAIN_M, new_gain);
+	ret = regmap_field_write(data->rf.gain, new_gain);
 	if (ret) {
 		dev_err(&data->client->dev, "can't set als gain %d\n", ret);
 		return ret;
@@ -581,17 +618,17 @@ static int veml6035_set_als_gain(struct iio_dev *indio_dev, int val, int val2)
 static int veml6030_get_als_gain(struct iio_dev *indio_dev,
 						int *val, int *val2)
 {
-	int ret, reg;
+	int gain, ret;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
+	ret = regmap_field_read(data->rf.gain, &gain);
 	if (ret) {
 		dev_err(&data->client->dev,
 				"can't read als conf register %d\n", ret);
 		return ret;
 	}
 
-	switch ((reg >> 11) & 0x03) {
+	switch (gain) {
 	case 0:
 		*val = 1;
 		*val2 = 0;
@@ -617,17 +654,17 @@ static int veml6030_get_als_gain(struct iio_dev *indio_dev,
 
 static int veml6035_get_als_gain(struct iio_dev *indio_dev, int *val, int *val2)
 {
-	int ret, reg;
+	int gain, ret;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = regmap_read(data->regmap, VEML6030_REG_ALS_CONF, &reg);
+	ret = regmap_field_read(data->rf.gain, &gain);
 	if (ret) {
 		dev_err(&data->client->dev,
-			"can't read als conf register %d\n", ret);
+				"can't read als conf register %d\n", ret);
 		return ret;
 	}
 
-	switch (FIELD_GET(VEML6035_GAIN_M, reg)) {
+	switch (gain) {
 	case 0:
 		*val = 1;
 		*val2 = 0;
@@ -990,6 +1027,52 @@ static int veml7700_set_info(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static int veml6030_regfield_init(struct iio_dev *indio_dev)
+{
+	const struct reg_field it = REG_FIELD(VEML6030_REG_ALS_CONF, 6, 9);
+	const struct reg_field gain = REG_FIELD(VEML6030_REG_ALS_CONF, 11, 12);
+	struct veml6030_data *data = iio_priv(indio_dev);
+	struct regmap *regmap = data->regmap;
+	struct device *dev = &data->client->dev;
+	struct regmap_field *rm_field;
+	struct veml6030_rf *rf = &data->rf;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, it);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->it = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, gain);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->gain = rm_field;
+
+	return 0;
+}
+
+static int veml6035_regfield_init(struct iio_dev *indio_dev)
+{
+	const struct reg_field it = REG_FIELD(VEML6030_REG_ALS_CONF, 6, 9);
+	const struct reg_field gain = REG_FIELD(VEML6030_REG_ALS_CONF, 10, 12);
+	struct veml6030_data *data = iio_priv(indio_dev);
+	struct regmap *regmap = data->regmap;
+	struct device *dev = &data->client->dev;
+	struct regmap_field *rm_field;
+	struct veml6030_rf *rf = &data->rf;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, it);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->it = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, gain);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->gain = rm_field;
+
+	return 0;
+}
+
 /*
  * Set ALS gain to 1/8, integration time to 100 ms, PSM to mode 2,
  * persistence to 1 x integration time and the threshold
@@ -1143,6 +1226,11 @@ static int veml6030_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	ret = data->chip->regfield_init(indio_dev);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to init regfields\n");
+
 	ret = data->chip->hw_init(indio_dev, &client->dev);
 	if (ret < 0)
 		return ret;
@@ -1195,6 +1283,7 @@ static const struct veml603x_chip veml6030_chip = {
 	.set_info = veml6030_set_info,
 	.set_als_gain = veml6030_set_als_gain,
 	.get_als_gain = veml6030_get_als_gain,
+	.regfield_init = veml6030_regfield_init,
 };
 
 static const struct veml603x_chip veml6035_chip = {
@@ -1207,6 +1296,7 @@ static const struct veml603x_chip veml6035_chip = {
 	.set_info = veml6030_set_info,
 	.set_als_gain = veml6035_set_als_gain,
 	.get_als_gain = veml6035_get_als_gain,
+	.regfield_init = veml6035_regfield_init,
 };
 
 static const struct veml603x_chip veml7700_chip = {
@@ -1219,6 +1309,7 @@ static const struct veml603x_chip veml7700_chip = {
 	.set_info = veml7700_set_info,
 	.set_als_gain = veml6030_set_als_gain,
 	.get_als_gain = veml6030_get_als_gain,
+	.regfield_init = veml6030_regfield_init,
 };
 
 static const struct of_device_id veml6030_of_match[] = {

-- 
2.43.0

[PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Javier Carrasco]

The current scale is not ABI-compliant as it is just the sensor gain
instead of the value that acts as a multiplier to be applied to the raw
value (there is no offset).

Use the iio-gts helpers to obtain the proper scale values according to
the gain and integration time to match the resolution tables from the
datasheet and drop dedicated variables to store the current values of
the integration time, gain and resolution. When at it, use 'scale'
instead of 'gain' consistently for the get/set functions to avoid
misunderstandings.

Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/Kconfig    |   1 +
 drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
 2 files changed, 189 insertions(+), 311 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index e34e551eef3e8db006de56724ce3873c07b3360a..eb7f56eaeae07c8b021dc7c0db87f46b44ed44d7 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -683,6 +683,7 @@ config VEML6030
 	select REGMAP_I2C
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
+	select IIO_GTS_HELPER
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VEML6030
diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index a6385c6d3fba59a6b22845a3c5e252b619faed65..99c7e073ea664f815a7b1c1bc829a8eff66fd323 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -24,10 +24,12 @@
 #include <linux/regmap.h>
 #include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
+#include <linux/units.h>
 #include <linux/regulator/consumer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
+#include <linux/iio/iio-gts-helper.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
@@ -72,14 +74,10 @@ struct veml6030_rf {
 
 struct veml603x_chip {
 	const char *name;
-	const int(*scale_vals)[][2];
-	const int num_scale_vals;
 	const struct iio_chan_spec *channels;
 	const int num_channels;
 	int (*hw_init)(struct iio_dev *indio_dev, struct device *dev);
 	int (*set_info)(struct iio_dev *indio_dev);
-	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
-	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
 	int (*regfield_init)(struct iio_dev *indio_dev);
 };
 
@@ -98,40 +96,55 @@ struct veml6030_data {
 	struct i2c_client *client;
 	struct regmap *regmap;
 	struct veml6030_rf rf;
-	int cur_resolution;
-	int cur_gain;
-	int cur_integration_time;
 	const struct veml603x_chip *chip;
+	struct iio_gts gts;
+
 };
 
-static const int veml6030_it_times[][2] = {
-	{ 0, 25000 },
-	{ 0, 50000 },
-	{ 0, 100000 },
-	{ 0, 200000 },
-	{ 0, 400000 },
-	{ 0, 800000 },
+#define VEML6030_SEL_IT_25MS  0x0C
+#define VEML6030_SEL_IT_50MS  0x08
+#define VEML6030_SEL_IT_100MS 0x00
+#define VEML6030_SEL_IT_200MS 0x01
+#define VEML6030_SEL_IT_400MS 0x02
+#define VEML6030_SEL_IT_800MS 0x03
+static const struct iio_itime_sel_mul veml6030_it_sel[] = {
+	GAIN_SCALE_ITIME_US(25000, VEML6030_SEL_IT_25MS, 1),
+	GAIN_SCALE_ITIME_US(50000, VEML6030_SEL_IT_50MS, 2),
+	GAIN_SCALE_ITIME_US(100000, VEML6030_SEL_IT_100MS, 4),
+	GAIN_SCALE_ITIME_US(200000, VEML6030_SEL_IT_200MS, 8),
+	GAIN_SCALE_ITIME_US(400000, VEML6030_SEL_IT_400MS, 16),
+	GAIN_SCALE_ITIME_US(800000, VEML6030_SEL_IT_800MS, 32),
 };
 
-/*
- * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
- * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1,
- * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4.
+/* Gains are multiplied by 8 to work with integers. The values in the
+ * iio-gts tables don't need corrections because the maximum value of
+ * the scale refers to GAIN = x1, and the rest of the values are
+ * obtained from the resulting linear function.
  */
-static const int veml6030_scale_vals[][2] = {
-	{ 0, 125000 },
-	{ 0, 250000 },
-	{ 1, 0 },
-	{ 2, 0 },
+#define VEML6030_SEL_GAIN_X125  2
+#define VEML6030_SEL_GAIN_X250  3
+#define VEML6030_SEL_GAIN_X1000 0
+#define VEML6030_SEL_GAIN_X2000 1
+static const struct iio_gain_sel_pair veml6030_gain_sel[] = {
+	GAIN_SCALE_GAIN(1, VEML6030_SEL_GAIN_X125),
+	GAIN_SCALE_GAIN(2, VEML6030_SEL_GAIN_X250),
+	GAIN_SCALE_GAIN(8, VEML6030_SEL_GAIN_X1000),
+	GAIN_SCALE_GAIN(16, VEML6030_SEL_GAIN_X2000),
 };
 
-static const int veml6035_scale_vals[][2] = {
-	{ 0, 125000 },
-	{ 0, 250000 },
-	{ 0, 500000 },
-	{ 1, 0 },
-	{ 2, 0 },
-	{ 4, 0 },
+#define VEML6035_SEL_GAIN_X125  4
+#define VEML6035_SEL_GAIN_X250  5
+#define VEML6035_SEL_GAIN_X500  7
+#define VEML6035_SEL_GAIN_X1000 0
+#define VEML6035_SEL_GAIN_X2000 1
+#define VEML6035_SEL_GAIN_X4000 3
+static const struct iio_gain_sel_pair veml6035_gain_sel[] = {
+	GAIN_SCALE_GAIN(1, VEML6035_SEL_GAIN_X125),
+	GAIN_SCALE_GAIN(2, VEML6035_SEL_GAIN_X250),
+	GAIN_SCALE_GAIN(4, VEML6035_SEL_GAIN_X500),
+	GAIN_SCALE_GAIN(8, VEML6035_SEL_GAIN_X1000),
+	GAIN_SCALE_GAIN(16, VEML6035_SEL_GAIN_X2000),
+	GAIN_SCALE_GAIN(32, VEML6035_SEL_GAIN_X4000),
 };
 
 /*
@@ -365,104 +378,73 @@ static const struct regmap_config veml6030_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
-static int veml6030_get_intgrn_tm(struct iio_dev *indio_dev,
-						int *val, int *val2)
+static int veml6030_get_it(struct veml6030_data *data, int *val, int *val2)
 {
-	int it_idx, ret;
-	struct veml6030_data *data = iio_priv(indio_dev);
+	int ret, it_idx;
 
 	ret = regmap_field_read(data->rf.it, &it_idx);
-	if (ret) {
-		dev_err(&data->client->dev,
-				"can't read als conf register %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
-	switch (it_idx) {
-	case 0:
-		*val2 = 100000;
-		break;
-	case 1:
-		*val2 = 200000;
-		break;
-	case 2:
-		*val2 = 400000;
-		break;
-	case 3:
-		*val2 = 800000;
-		break;
-	case 8:
-		*val2 = 50000;
-		break;
-	case 12:
-		*val2 = 25000;
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
+	if (ret < 0)
+		return ret;
 
+	*val2 = ret;
 	*val = 0;
+
 	return IIO_VAL_INT_PLUS_MICRO;
 }
 
-static int veml6030_set_intgrn_tm(struct iio_dev *indio_dev,
-						int val, int val2)
+static int veml6030_set_it(struct iio_dev *indio_dev, int val, int val2)
 {
-	int ret, new_int_time, int_idx;
 	struct veml6030_data *data = iio_priv(indio_dev);
+	int ret, gain_idx, it_idx, new_gain, prev_gain, prev_it;
+	bool in_range;
 
-	if (val)
+	if (val || !iio_gts_valid_time(&data->gts, val2))
 		return -EINVAL;
 
-	switch (val2) {
-	case 25000:
-		new_int_time = 0x300;
-		int_idx = 5;
-		break;
-	case 50000:
-		new_int_time = 0x200;
-		int_idx = 4;
-		break;
-	case 100000:
-		new_int_time = 0x00;
-		int_idx = 3;
-		break;
-	case 200000:
-		new_int_time = 0x40;
-		int_idx = 2;
-		break;
-	case 400000:
-		new_int_time = 0x80;
-		int_idx = 1;
-		break;
-	case 800000:
-		new_int_time = 0xC0;
-		int_idx = 0;
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = regmap_field_read(data->rf.it, &it_idx);
+	if (ret)
+		return ret;
 
-	ret = regmap_field_write(data->rf.it, new_int_time);
-	if (ret) {
-		dev_err(&data->client->dev,
-				"can't update als integration time %d\n", ret);
+	ret = regmap_field_read(data->rf.gain, &gain_idx);
+	if (ret)
 		return ret;
-	}
 
-	/*
-	 * Cache current integration time and update resolution. For every
-	 * increase in integration time to next level, resolution is halved
-	 * and vice-versa.
-	 */
-	if (data->cur_integration_time < int_idx)
-		data->cur_resolution <<= int_idx - data->cur_integration_time;
-	else if (data->cur_integration_time > int_idx)
-		data->cur_resolution >>= data->cur_integration_time - int_idx;
+	prev_it = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
+	if (prev_it < 0)
+		return prev_it;
 
-	data->cur_integration_time = int_idx;
+	if (prev_it == val2)
+		return 0;
 
-	return ret;
+	prev_gain = iio_gts_find_gain_by_sel(&data->gts, gain_idx);
+	if (prev_gain < 0)
+		return prev_gain;
+
+	ret = iio_gts_find_new_gain_by_gain_time_min(&data->gts, prev_gain, prev_it,
+						     val2, &new_gain, &in_range);
+	if (ret)
+		return ret;
+
+	if (!in_range)
+		dev_dbg(&data->client->dev, "Optimal gain out of range\n");
+
+	ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_field_write(data->rf.it, ret);
+	if (ret)
+		return ret;
+
+	ret = iio_gts_find_sel_by_gain(&data->gts, new_gain);
+	if (ret < 0)
+		return ret;
+
+	return regmap_field_write(data->rf.gain, ret);
 }
 
 static int veml6030_read_persistence(struct iio_dev *indio_dev,
@@ -471,7 +453,7 @@ static int veml6030_read_persistence(struct iio_dev *indio_dev,
 	int ret, reg, period, x, y;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = veml6030_get_intgrn_tm(indio_dev, &x, &y);
+	ret = veml6030_get_it(data, &x, &y);
 	if (ret < 0)
 		return ret;
 
@@ -496,7 +478,7 @@ static int veml6030_write_persistence(struct iio_dev *indio_dev,
 	int ret, period, x, y;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = veml6030_get_intgrn_tm(indio_dev, &x, &y);
+	ret = veml6030_get_it(data, &x, &y);
 	if (ret < 0)
 		return ret;
 
@@ -525,177 +507,29 @@ static int veml6030_write_persistence(struct iio_dev *indio_dev,
 	return ret;
 }
 
-/*
- * Cache currently set gain & update resolution. For every
- * increase in the gain to next level, resolution is halved
- * and vice-versa.
- */
-static void veml6030_update_gain_res(struct veml6030_data *data, int gain_idx)
+static int veml6030_set_scale(struct iio_dev *indio_dev, int val, int val2)
 {
-	if (data->cur_gain < gain_idx)
-		data->cur_resolution <<= gain_idx - data->cur_gain;
-	else if (data->cur_gain > gain_idx)
-		data->cur_resolution >>= data->cur_gain - gain_idx;
-
-	data->cur_gain = gain_idx;
-}
-
-static int veml6030_set_als_gain(struct iio_dev *indio_dev,
-						int val, int val2)
-{
-	int ret, new_gain, gain_idx;
+	int ret, gain_sel, it_idx, it_sel;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
-	if (val == 0 && val2 == 125000) {
-		new_gain = 0x01;
-		gain_idx = 3;
-	} else if (val == 0 && val2 == 250000) {
-		new_gain = 0x11;
-		gain_idx = 2;
-	} else if (val == 1 && val2 == 0) {
-		new_gain = 0x00;
-		gain_idx = 1;
-	} else if (val == 2 && val2 == 0) {
-		new_gain = 0x01;
-		gain_idx = 0;
-	} else {
-		return -EINVAL;
-	}
-
-	ret = regmap_field_write(data->rf.gain, new_gain);
-	if (ret) {
-		dev_err(&data->client->dev,
-				"can't set als gain %d\n", ret);
+	ret = regmap_field_read(data->rf.it, &it_idx);
+	if (ret)
 		return ret;
-	}
 
-	veml6030_update_gain_res(data, gain_idx);
-
-	return 0;
-}
-
-static int veml6035_set_als_gain(struct iio_dev *indio_dev, int val, int val2)
-{
-	int ret, new_gain, gain_idx;
-	struct veml6030_data *data = iio_priv(indio_dev);
-
-	if (val == 0 && val2 == 125000) {
-		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS);
-		gain_idx = 5;
-	} else if (val == 0 && val2 == 250000) {
-		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS |
-				      VEML6035_GAIN);
-		gain_idx = 4;
-	} else if (val == 0 && val2 == 500000) {
-		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS |
-				      VEML6035_GAIN | VEML6035_DG);
-		gain_idx = 3;
-	} else if (val == 1 && val2 == 0) {
-		new_gain = 0x0000;
-		gain_idx = 2;
-	} else if (val == 2 && val2 == 0) {
-		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN);
-		gain_idx = 1;
-	} else if (val == 4 && val2 == 0) {
-		new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN |
-				      VEML6035_DG);
-		gain_idx = 0;
-	} else {
-		return -EINVAL;
-	}
-
-	ret = regmap_field_write(data->rf.gain, new_gain);
-	if (ret) {
-		dev_err(&data->client->dev, "can't set als gain %d\n", ret);
+	ret = iio_gts_find_gain_time_sel_for_scale(&data->gts, val, val2,
+						   &gain_sel, &it_sel);
+	if (ret)
 		return ret;
-	}
-
-	veml6030_update_gain_res(data, gain_idx);
 
-	return 0;
-}
-
-static int veml6030_get_als_gain(struct iio_dev *indio_dev,
-						int *val, int *val2)
-{
-	int gain, ret;
-	struct veml6030_data *data = iio_priv(indio_dev);
-
-	ret = regmap_field_read(data->rf.gain, &gain);
-	if (ret) {
-		dev_err(&data->client->dev,
-				"can't read als conf register %d\n", ret);
+	ret = regmap_field_write(data->rf.it, it_sel);
+	if (ret)
 		return ret;
-	}
-
-	switch (gain) {
-	case 0:
-		*val = 1;
-		*val2 = 0;
-		break;
-	case 1:
-		*val = 2;
-		*val2 = 0;
-		break;
-	case 2:
-		*val = 0;
-		*val2 = 125000;
-		break;
-	case 3:
-		*val = 0;
-		*val2 = 250000;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return IIO_VAL_INT_PLUS_MICRO;
-}
-
-static int veml6035_get_als_gain(struct iio_dev *indio_dev, int *val, int *val2)
-{
-	int gain, ret;
-	struct veml6030_data *data = iio_priv(indio_dev);
 
-	ret = regmap_field_read(data->rf.gain, &gain);
-	if (ret) {
-		dev_err(&data->client->dev,
-				"can't read als conf register %d\n", ret);
+	ret = regmap_field_write(data->rf.gain, gain_sel);
+	if (ret)
 		return ret;
-	}
-
-	switch (gain) {
-	case 0:
-		*val = 1;
-		*val2 = 0;
-		break;
-	case 1:
-	case 2:
-		*val = 2;
-		*val2 = 0;
-		break;
-	case 3:
-		*val = 4;
-		*val2 = 0;
-		break;
-	case 4:
-		*val = 0;
-		*val2 = 125000;
-		break;
-	case 5:
-	case 6:
-		*val = 0;
-		*val2 = 250000;
-		break;
-	case 7:
-		*val = 0;
-		*val2 = 500000;
-		break;
-	default:
-		return -EINVAL;
-	}
 
-	return IIO_VAL_INT_PLUS_MICRO;
+	return 0;
 }
 
 static int veml6030_read_thresh(struct iio_dev *indio_dev,
@@ -742,6 +576,50 @@ static int veml6030_write_thresh(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int veml6030_get_scale(struct veml6030_data *data, int *val, int *val2)
+{
+	int gain, it, reg, ret;
+
+	ret = regmap_field_read(data->rf.gain, &reg);
+	if (ret)
+		return ret;
+
+	gain = iio_gts_find_gain_by_sel(&data->gts, reg);
+	if (gain < 0)
+		return gain;
+
+	ret = regmap_field_read(data->rf.it, &reg);
+	if (ret)
+		return ret;
+
+	it = iio_gts_find_int_time_by_sel(&data->gts, reg);
+	if (it < 0)
+		return it;
+
+	ret = iio_gts_get_scale(&data->gts, gain, it, val, val2);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int veml6030_process_als(struct veml6030_data *data, int raw,
+				int *val, int *val2)
+{
+	int ret, scale_int, scale_nano;
+	u64 tmp;
+
+	ret = veml6030_get_scale(data, &scale_int, &scale_nano);
+	if (ret < 0)
+		return ret;
+
+	tmp = (u64)raw * scale_nano;
+	*val = raw * scale_int + div_u64(tmp, NANO);
+	*val2 = div_u64(do_div(tmp, NANO), MILLI);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
 /*
  * Provide both raw as well as light reading in lux.
  * light (in lux) = resolution * raw reading
@@ -765,11 +643,9 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
 				dev_err(dev, "can't read als data %d\n", ret);
 				return ret;
 			}
-			if (mask == IIO_CHAN_INFO_PROCESSED) {
-				*val = (reg * data->cur_resolution) / 10000;
-				*val2 = (reg * data->cur_resolution) % 10000 * 100;
-				return IIO_VAL_INT_PLUS_MICRO;
-			}
+			if (mask == IIO_CHAN_INFO_PROCESSED)
+				return veml6030_process_als(data, reg, val, val2);
+
 			*val = reg;
 			return IIO_VAL_INT;
 		case IIO_INTENSITY:
@@ -784,9 +660,9 @@ static int veml6030_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 	case IIO_CHAN_INFO_INT_TIME:
-		return veml6030_get_intgrn_tm(indio_dev, val, val2);
+		return veml6030_get_it(data, val, val2);
 	case IIO_CHAN_INFO_SCALE:
-		return data->chip->get_als_gain(indio_dev, val, val2);
+		return veml6030_get_scale(data, val, val2);
 	default:
 		return -EINVAL;
 	}
@@ -801,15 +677,9 @@ static int veml6030_read_avail(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
-		*vals = (int *)&veml6030_it_times;
-		*length = 2 * ARRAY_SIZE(veml6030_it_times);
-		*type = IIO_VAL_INT_PLUS_MICRO;
-		return IIO_AVAIL_LIST;
+		return iio_gts_avail_times(&data->gts, vals, type, length);
 	case IIO_CHAN_INFO_SCALE:
-		*vals = (int *)*data->chip->scale_vals;
-		*length = 2 * data->chip->num_scale_vals;
-		*type = IIO_VAL_INT_PLUS_MICRO;
-		return IIO_AVAIL_LIST;
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
 	}
 
 	return -EINVAL;
@@ -819,13 +689,25 @@ static int veml6030_write_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int val, int val2, long mask)
 {
-	struct veml6030_data *data = iio_priv(indio_dev);
-
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
-		return veml6030_set_intgrn_tm(indio_dev, val, val2);
+		return veml6030_set_it(indio_dev, val, val2);
 	case IIO_CHAN_INFO_SCALE:
-		return data->chip->set_als_gain(indio_dev, val, val2);
+		return veml6030_set_scale(indio_dev, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6030_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -923,6 +805,7 @@ static const struct iio_info veml6030_info = {
 	.read_raw  = veml6030_read_raw,
 	.read_avail  = veml6030_read_avail,
 	.write_raw = veml6030_write_raw,
+	.write_raw_get_fmt = veml6030_write_raw_get_fmt,
 	.read_event_value = veml6030_read_event_val,
 	.write_event_value	= veml6030_write_event_val,
 	.read_event_config = veml6030_read_interrupt_config,
@@ -934,6 +817,7 @@ static const struct iio_info veml6030_info_no_irq = {
 	.read_raw  = veml6030_read_raw,
 	.read_avail  = veml6030_read_avail,
 	.write_raw = veml6030_write_raw,
+	.write_raw_get_fmt = veml6030_write_raw_get_fmt,
 };
 
 static irqreturn_t veml6030_event_handler(int irq, void *private)
@@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
 	int ret, val;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
+	ret = devm_iio_init_iio_gts(dev, 2, 150400000,
+				    veml6030_gain_sel, ARRAY_SIZE(veml6030_gain_sel),
+				    veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel),
+				    &data->gts);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init iio gts\n");
+
 	ret = veml6030_als_shut_down(data);
 	if (ret)
 		return dev_err_probe(dev, ret, "can't shutdown als\n");
@@ -1119,11 +1010,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
 		return dev_err_probe(dev, ret,
 				     "can't clear als interrupt status\n");
 
-	/* Cache currently active measurement parameters */
-	data->cur_gain = 3;
-	data->cur_resolution = 5376;
-	data->cur_integration_time = 3;
-
 	return ret;
 }
 
@@ -1139,6 +1025,13 @@ static int veml6035_hw_init(struct iio_dev *indio_dev, struct device *dev)
 	int ret, val;
 	struct veml6030_data *data = iio_priv(indio_dev);
 
+	ret = devm_iio_init_iio_gts(dev, 0, 409600000,
+				    veml6035_gain_sel, ARRAY_SIZE(veml6035_gain_sel),
+				    veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel),
+				    &data->gts);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init iio gts\n");
+
 	ret = veml6030_als_shut_down(data);
 	if (ret)
 		return dev_err_probe(dev, ret, "can't shutdown als\n");
@@ -1175,11 +1068,6 @@ static int veml6035_hw_init(struct iio_dev *indio_dev, struct device *dev)
 		return dev_err_probe(dev, ret,
 				     "can't clear als interrupt status\n");
 
-	/* Cache currently active measurement parameters */
-	data->cur_gain = 5;
-	data->cur_resolution = 1024;
-	data->cur_integration_time = 3;
-
 	return 0;
 }
 
@@ -1275,40 +1163,28 @@ static DEFINE_RUNTIME_DEV_PM_OPS(veml6030_pm_ops, veml6030_runtime_suspend,
 
 static const struct veml603x_chip veml6030_chip = {
 	.name = "veml6030",
-	.scale_vals = &veml6030_scale_vals,
-	.num_scale_vals = ARRAY_SIZE(veml6030_scale_vals),
 	.channels = veml6030_channels,
 	.num_channels = ARRAY_SIZE(veml6030_channels),
 	.hw_init = veml6030_hw_init,
 	.set_info = veml6030_set_info,
-	.set_als_gain = veml6030_set_als_gain,
-	.get_als_gain = veml6030_get_als_gain,
 	.regfield_init = veml6030_regfield_init,
 };
 
 static const struct veml603x_chip veml6035_chip = {
 	.name = "veml6035",
-	.scale_vals = &veml6035_scale_vals,
-	.num_scale_vals = ARRAY_SIZE(veml6035_scale_vals),
 	.channels = veml6030_channels,
 	.num_channels = ARRAY_SIZE(veml6030_channels),
 	.hw_init = veml6035_hw_init,
 	.set_info = veml6030_set_info,
-	.set_als_gain = veml6035_set_als_gain,
-	.get_als_gain = veml6035_get_als_gain,
 	.regfield_init = veml6035_regfield_init,
 };
 
 static const struct veml603x_chip veml7700_chip = {
 	.name = "veml7700",
-	.scale_vals = &veml6030_scale_vals,
-	.num_scale_vals = ARRAY_SIZE(veml6030_scale_vals),
 	.channels = veml7700_channels,
 	.num_channels = ARRAY_SIZE(veml7700_channels),
 	.hw_init = veml6030_hw_init,
 	.set_info = veml7700_set_info,
-	.set_als_gain = veml6030_set_als_gain,
-	.get_als_gain = veml6030_get_als_gain,
 	.regfield_init = veml6030_regfield_init,
 };
 
@@ -1351,3 +1227,4 @@ module_i2c_driver(veml6030_driver);
 MODULE_AUTHOR("Rishi Gupta <gupt21@gmail.com>");
 MODULE_DESCRIPTION("VEML6030 Ambient Light Sensor");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("IIO_GTS_HELPER");

-- 
2.43.0

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Javier Carrasco]

On Tue Jan 7, 2025 at 9:50 PM CET, Javier Carrasco wrote:
> The current scale is not ABI-compliant as it is just the sensor gain
> instead of the value that acts as a multiplier to be applied to the raw
> value (there is no offset).
>
> Use the iio-gts helpers to obtain the proper scale values according to
> the gain and integration time to match the resolution tables from the
> datasheet and drop dedicated variables to store the current values of
> the integration time, gain and resolution. When at it, use 'scale'
> instead of 'gain' consistently for the get/set functions to avoid
> misunderstandings.
>
> Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/light/Kconfig    |   1 +
>  drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
>  2 files changed, 189 insertions(+), 311 deletions(-)
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index e34e551eef3e8db006de56724ce3873c07b3360a..eb7f56eaeae07c8b021dc7c0db87f46b44ed44d7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -683,6 +683,7 @@ config VEML6030
>  	select REGMAP_I2C
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
> +	select IIO_GTS_HELPER
>  	depends on I2C
>  	help
>  	  Say Y here if you want to build a driver for the Vishay VEML6030
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index a6385c6d3fba59a6b22845a3c5e252b619faed65..99c7e073ea664f815a7b1c1bc829a8eff66fd323 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -24,10 +24,12 @@
>  #include <linux/regmap.h>
>  #include <linux/interrupt.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/units.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
> +#include <linux/iio/iio-gts-helper.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>
> @@ -72,14 +74,10 @@ struct veml6030_rf {
>
>  struct veml603x_chip {
>  	const char *name;
> -	const int(*scale_vals)[][2];
> -	const int num_scale_vals;
>  	const struct iio_chan_spec *channels;
>  	const int num_channels;
>  	int (*hw_init)(struct iio_dev *indio_dev, struct device *dev);
>  	int (*set_info)(struct iio_dev *indio_dev);
> -	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
> -	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
>  	int (*regfield_init)(struct iio_dev *indio_dev);
>  };
>
> @@ -98,40 +96,55 @@ struct veml6030_data {
>  	struct i2c_client *client;
>  	struct regmap *regmap;
>  	struct veml6030_rf rf;
> -	int cur_resolution;
> -	int cur_gain;
> -	int cur_integration_time;
>  	const struct veml603x_chip *chip;
> +	struct iio_gts gts;
> +
>  };
>
> -static const int veml6030_it_times[][2] = {
> -	{ 0, 25000 },
> -	{ 0, 50000 },
> -	{ 0, 100000 },
> -	{ 0, 200000 },
> -	{ 0, 400000 },
> -	{ 0, 800000 },
> +#define VEML6030_SEL_IT_25MS  0x0C
> +#define VEML6030_SEL_IT_50MS  0x08
> +#define VEML6030_SEL_IT_100MS 0x00
> +#define VEML6030_SEL_IT_200MS 0x01
> +#define VEML6030_SEL_IT_400MS 0x02
> +#define VEML6030_SEL_IT_800MS 0x03
> +static const struct iio_itime_sel_mul veml6030_it_sel[] = {
> +	GAIN_SCALE_ITIME_US(25000, VEML6030_SEL_IT_25MS, 1),
> +	GAIN_SCALE_ITIME_US(50000, VEML6030_SEL_IT_50MS, 2),
> +	GAIN_SCALE_ITIME_US(100000, VEML6030_SEL_IT_100MS, 4),
> +	GAIN_SCALE_ITIME_US(200000, VEML6030_SEL_IT_200MS, 8),
> +	GAIN_SCALE_ITIME_US(400000, VEML6030_SEL_IT_400MS, 16),
> +	GAIN_SCALE_ITIME_US(800000, VEML6030_SEL_IT_800MS, 32),
>  };
>
> -/*
> - * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
> - * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1,
> - * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4.
> +/* Gains are multiplied by 8 to work with integers. The values in the
> + * iio-gts tables don't need corrections because the maximum value of
> + * the scale refers to GAIN = x1, and the rest of the values are
> + * obtained from the resulting linear function.
>   */
> -static const int veml6030_scale_vals[][2] = {
> -	{ 0, 125000 },
> -	{ 0, 250000 },
> -	{ 1, 0 },
> -	{ 2, 0 },
> +#define VEML6030_SEL_GAIN_X125  2
> +#define VEML6030_SEL_GAIN_X250  3
> +#define VEML6030_SEL_GAIN_X1000 0
> +#define VEML6030_SEL_GAIN_X2000 1
> +static const struct iio_gain_sel_pair veml6030_gain_sel[] = {
> +	GAIN_SCALE_GAIN(1, VEML6030_SEL_GAIN_X125),
> +	GAIN_SCALE_GAIN(2, VEML6030_SEL_GAIN_X250),
> +	GAIN_SCALE_GAIN(8, VEML6030_SEL_GAIN_X1000),
> +	GAIN_SCALE_GAIN(16, VEML6030_SEL_GAIN_X2000),
>  };

While working on a driver with a similar approach, I noticed that the
names don't reflect the fact that those values are in MILLI. I will
change them to VEML6030_SEL_MILLI_GAIN_* for v2 alongside any other
issues that might have been found during the review.

Best regards,
Javier Carrasco

Re: [PATCH 1/2] iio: light: veml6030: extend regmap to support regfields and caching

[Jonathan Cameron]

On Tue, 07 Jan 2025 21:50:21 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The configuration registers are not volatile and are not affected
> by read operations (i.e. not precious), making them suitable to be
> cached in order to reduce the number of accesses to the device.
> 
> Add support for regfields as well to simplify register operations,
> taking into account the different fields for the veml6030/veml7700 and
> veml6035.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/light/veml6030.c | 141 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 116 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 9b71825eea9bee2146be17ed2f30f5a8f7ad37e3..a6385c6d3fba59a6b22845a3c5e252b619faed65 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -65,6 +65,11 @@ enum veml6030_scan {
>  	VEML6030_SCAN_TIMESTAMP,
>  };
>  
> +struct veml6030_rf {
> +	struct regmap_field *it;
> +	struct regmap_field *gain;
> +};
> +
>  struct veml603x_chip {
>  	const char *name;
>  	const int(*scale_vals)[][2];
> @@ -75,6 +80,7 @@ struct veml603x_chip {
>  	int (*set_info)(struct iio_dev *indio_dev);
>  	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
>  	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
> +	int (*regfield_init)(struct iio_dev *indio_dev);

With only two fields, why use a callback rather than just adding the two
const struct reg_field into this structure directly?

I'd also be tempted to do the caching and regfield changes as separate patches.

Jonathan

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Jonathan Cameron]

On Tue, 07 Jan 2025 21:50:22 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The current scale is not ABI-compliant as it is just the sensor gain
> instead of the value that acts as a multiplier to be applied to the raw
> value (there is no offset).
> 
> Use the iio-gts helpers to obtain the proper scale values according to
> the gain and integration time to match the resolution tables from the
> datasheet and drop dedicated variables to store the current values of
> the integration time, gain and resolution. When at it, use 'scale'
> instead of 'gain' consistently for the get/set functions to avoid
> misunderstandings.
> 
> Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

For iio-gts useage please +CC Matti.

Matti is far more likely to spot subtle issues around that than other
reviewers such as me :)  Obviously that depends on Matti having time!

To me this looks fine

Jonathan

Re: [PATCH 1/2] iio: light: veml6030: extend regmap to support regfields and caching

[Javier Carrasco]

On Sun Jan 12, 2025 at 2:18 PM CET, Jonathan Cameron wrote:
> On Tue, 07 Jan 2025 21:50:21 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>
> > The configuration registers are not volatile and are not affected
> > by read operations (i.e. not precious), making them suitable to be
> > cached in order to reduce the number of accesses to the device.
> >
> > Add support for regfields as well to simplify register operations,
> > taking into account the different fields for the veml6030/veml7700 and
> > veml6035.
> >
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
> >  drivers/iio/light/veml6030.c | 141 +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 116 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> > index 9b71825eea9bee2146be17ed2f30f5a8f7ad37e3..a6385c6d3fba59a6b22845a3c5e252b619faed65 100644
> > --- a/drivers/iio/light/veml6030.c
> > +++ b/drivers/iio/light/veml6030.c
> > @@ -65,6 +65,11 @@ enum veml6030_scan {
> >  	VEML6030_SCAN_TIMESTAMP,
> >  };
> >
> > +struct veml6030_rf {
> > +	struct regmap_field *it;
> > +	struct regmap_field *gain;
> > +};
> > +
> >  struct veml603x_chip {
> >  	const char *name;
> >  	const int(*scale_vals)[][2];
> > @@ -75,6 +80,7 @@ struct veml603x_chip {
> >  	int (*set_info)(struct iio_dev *indio_dev);
> >  	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
> >  	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
> > +	int (*regfield_init)(struct iio_dev *indio_dev);
>
> With only two fields, why use a callback rather than just adding the two
> const struct reg_field into this structure directly?

The rationale was that extending the driver for more devices with
additional fields would not require extra elements in the struct that
would only apply to some devices. All members of this struct are rather
basic and all devices will require them, and although integration time
and gain regfields will be required too, if a new regfield for a
specific device is added, it will be added to the rest as empty element.

But that's probably too much "if" and "would", so I am fine with your
suggestion.

>
> I'd also be tempted to do the caching and regfield changes as separate patches.
>

Then I will split the patch for v2.

> Jonathan

Thank you for your feedback and best regards,
Javier Carrasco

Re: [PATCH 1/2] iio: light: veml6030: extend regmap to support regfields and caching

[Jonathan Cameron]

On Sun, 12 Jan 2025 15:10:14 +0100
"Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:

> On Sun Jan 12, 2025 at 2:18 PM CET, Jonathan Cameron wrote:
> > On Tue, 07 Jan 2025 21:50:21 +0100
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >  
> > > The configuration registers are not volatile and are not affected
> > > by read operations (i.e. not precious), making them suitable to be
> > > cached in order to reduce the number of accesses to the device.
> > >
> > > Add support for regfields as well to simplify register operations,
> > > taking into account the different fields for the veml6030/veml7700 and
> > > veml6035.
> > >
> > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > > ---
> > >  drivers/iio/light/veml6030.c | 141 +++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 116 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> > > index 9b71825eea9bee2146be17ed2f30f5a8f7ad37e3..a6385c6d3fba59a6b22845a3c5e252b619faed65 100644
> > > --- a/drivers/iio/light/veml6030.c
> > > +++ b/drivers/iio/light/veml6030.c
> > > @@ -65,6 +65,11 @@ enum veml6030_scan {
> > >  	VEML6030_SCAN_TIMESTAMP,
> > >  };
> > >
> > > +struct veml6030_rf {
> > > +	struct regmap_field *it;
> > > +	struct regmap_field *gain;
> > > +};
> > > +
> > >  struct veml603x_chip {
> > >  	const char *name;
> > >  	const int(*scale_vals)[][2];
> > > @@ -75,6 +80,7 @@ struct veml603x_chip {
> > >  	int (*set_info)(struct iio_dev *indio_dev);
> > >  	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
> > >  	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
> > > +	int (*regfield_init)(struct iio_dev *indio_dev);  
> >
> > With only two fields, why use a callback rather than just adding the two
> > const struct reg_field into this structure directly?  
> 
> The rationale was that extending the driver for more devices with
> additional fields would not require extra elements in the struct that
> would only apply to some devices. All members of this struct are rather
> basic and all devices will require them, and although integration time
> and gain regfields will be required too, if a new regfield for a
> specific device is added, it will be added to the rest as empty element.
> 
> But that's probably too much "if" and "would", so I am fine with your
> suggestion.

Absolutely - it is in kernel stuff so we can always revisit if it turns
out to make more sense this way.

> 
> >
> > I'd also be tempted to do the caching and regfield changes as separate patches.
> >  
> 
> Then I will split the patch for v2.
> 
> > Jonathan  
> 
> Thank you for your feedback and best regards,
> Javier Carrasco

Re: [PATCH 0/2] iio: light: fix scale in veml6030

[Matti Vaittinen]

On 07/01/2025 22:50, Javier Carrasco wrote:
> This series follows a similar approach as recently used for the veml3235
> by using iio-gts to manage the scale as stated in the ABI. In its
> current form, the driver exposes the hardware gain instead of the
> multiplier for the raw value to obtain a value in lux.
> 
> Although this driver and the veml3235 have many similarities, there are
> two main differences in this series compared to the one used to fix the
> other driver:
> 
> - The veml6030 has fractional gains, which are not supported by the
>    iio-gts helpers. My first attempt was adding support for them, but
>    that made the whole iio-gts implementation more complex, cumbersome,
>    and the risk of affecting existing clients was not negligible.

I do agree. If one added support for fractional gains, it should be very 
very clear implementation so that even my limited capacity could handle 
it :)

>    Instead, a x8 factor has been used for the hardware gain to present
>    the minimum value (x0.125) as x1, keeping linearity. The scales
>    iio-gts generates are therefore right without any extra conversion,
>    and they match the values provided in the different datasheets.

I didn't look through the patches yet - I'm getting to there though :) 
Anyways, I assume you don't expose this HARDWAREGAIN to users?

> - This driver included a processed value for the ambient light, maybe
>    because the scale did not follow the ABI and the conversion was not
>    direct. To avoid breaking userspace, the functionality has been kept,
>    but of course using the fixed scales. That requires using intermediate
>    u64 values u64 divisions via div_u64() and do_div() to avoid overflows.
> 
> To ease the usage of the iio-gts selectors, a previous patch to support
> regfields and caching has been included.

I don't see why iio-gts would need regfields? (I have never been able to 
fully decide whether the regfields are a nice thing or not. I feel that 
in many cases regfields just add an extra layer of obfuscation while 
providing little help - but this is just my personal opinion and I'm not 
against using them. I just don't think the iio-gts needs them to be 
used. AFAIR, selectors do not need to start from zero.).

Yours,
	-- Matti

Re: [PATCH 0/2] iio: light: fix scale in veml6030

[Javier Carrasco]

On Mon Jan 13, 2025 at 11:25 AM CET, Matti Vaittinen wrote:
> On 07/01/2025 22:50, Javier Carrasco wrote:
> > This series follows a similar approach as recently used for the veml3235
> > by using iio-gts to manage the scale as stated in the ABI. In its
> > current form, the driver exposes the hardware gain instead of the
> > multiplier for the raw value to obtain a value in lux.
> >
> > Although this driver and the veml3235 have many similarities, there are
> > two main differences in this series compared to the one used to fix the
> > other driver:
> >
> > - The veml6030 has fractional gains, which are not supported by the
> >    iio-gts helpers. My first attempt was adding support for them, but
> >    that made the whole iio-gts implementation more complex, cumbersome,
> >    and the risk of affecting existing clients was not negligible.
>
> I do agree. If one added support for fractional gains, it should be very
> very clear implementation so that even my limited capacity could handle
> it :)
>

I am working on another driver (veml6031x00, I sent a v1 with the same
flow as this one, as it inherited the gain configuration) that will
probably need some more tweaking to work with integer gains: it starts
by x0.125 like this one, but then it provides weird gains like 0.66 that
prevents a simple x8 adjustment... I will try to scale up and down instead
of adding fractional gains, though.

> >    Instead, a x8 factor has been used for the hardware gain to present
> >    the minimum value (x0.125) as x1, keeping linearity. The scales
> >    iio-gts generates are therefore right without any extra conversion,
> >    and they match the values provided in the different datasheets.
>
> I didn't look through the patches yet - I'm getting to there though :)
> Anyways, I assume you don't expose this HARDWAREGAIN to users?
>

That's right, HARDWAREGAIN is not exposed. If you believe that it should
be exposed, I am open to discuss.

> > - This driver included a processed value for the ambient light, maybe
> >    because the scale did not follow the ABI and the conversion was not
> >    direct. To avoid breaking userspace, the functionality has been kept,
> >    but of course using the fixed scales. That requires using intermediate
> >    u64 values u64 divisions via div_u64() and do_div() to avoid overflows.
> >
> > To ease the usage of the iio-gts selectors, a previous patch to support
> > regfields and caching has been included.
>
> I don't see why iio-gts would need regfields? (I have never been able to
> fully decide whether the regfields are a nice thing or not. I feel that
> in many cases regfields just add an extra layer of obfuscation while
> providing little help - but this is just my personal opinion and I'm not
> against using them. I just don't think the iio-gts needs them to be
> used. AFAIR, selectors do not need to start from zero.).
>
> Yours,
> 	-- Matti

For me, using regfields makes everything more straightforward: you
define the shifting in the configuration phase, and then you can simply
assign the selectors to the field, with the same values as the field in
question would expect for a given configuration (0 is 0, 1 is 1, and so
on). I see that easier than thinking about register-level values, that
are easier to get wrong.

Once you have defined your regfields, you don't have to think ever again
about their position. Actually, I think that iio-gts works perfect with
that approach, even if its father is not a fan of regfields ;)

Thank you for your feedback and best regards,
Javier Carrasco

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Matti Vaittinen]

On 07/01/2025 22:50, Javier Carrasco wrote:
> The current scale is not ABI-compliant as it is just the sensor gain
> instead of the value that acts as a multiplier to be applied to the raw
> value (there is no offset).
> 
> Use the iio-gts helpers to obtain the proper scale values according to
> the gain and integration time to match the resolution tables from the
> datasheet and drop dedicated variables to store the current values of
> the integration time, gain and resolution. When at it, use 'scale'
> instead of 'gain' consistently for the get/set functions to avoid
> misunderstandings.
> 
> Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Thanks for the patch Javier.

And, sorry for a review which is more about questions than suggested 
improvements. I find it hard to focus on reading code today.

> ---
>   drivers/iio/light/Kconfig    |   1 +
>   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
>   2 files changed, 189 insertions(+), 311 deletions(-)
> 

I like the diffstats of this Fix! :) It's nice you found gts-helpers 
helpful :)

...

> +
> +static int veml6030_process_als(struct veml6030_data *data, int raw,
> +				int *val, int *val2)
> +{
> +	int ret, scale_int, scale_nano;
> +	u64 tmp;
> +
> +	ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> +	if (ret < 0)
> +		return ret;
> +
> +	tmp = (u64)raw * scale_nano;
> +	*val = raw * scale_int + div_u64(tmp, NANO);
> +	*val2 = div_u64(do_div(tmp, NANO), MILLI);

do_div() is horrible on some platforms. Or, at least it used to be. Is 
there really no way to go without it? We're dealing with 16bit data and 
maximum of 512x total gain only, so maybe there was a way(?)

Maybe a stupid question (in which case I blame mucus in my head) - could 
you just divide the raw value by the total gain?

> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +

...

>   
>   static irqreturn_t veml6030_event_handler(int irq, void *private)
> @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
>   	int ret, val;
>   	struct veml6030_data *data = iio_priv(indio_dev);
>   
> +	ret = devm_iio_init_iio_gts(dev, 2, 150400000,

Can you please explain the seemingly odd choice for the max scale?

Just a quick look at the sensor spec and this driver allows me to assume 
following:

Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.

If we chose the 'total gain' 1x to match scale 1, then the smallest 
scale would be 1/512 = 0.001 953 125

This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would 
not cause precision loss. (Well, I'm not at my sharpest as I've caught 
cold - but I _think_ this is true, right?)

If I read this correctly, the only channel where the scale gets applied 
is the INTENSITY channel, right? Hence it should be possible to chose 
the scale as we see best. (Unless the idea of this seemingly odd scale 
is to maintain the old intensity / scale values in order to not shake 
userland any more - in which case this could be mentioned).

> +				    veml6030_gain_sel, ARRAY_SIZE(veml6030_gain_sel),
> +				    veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel),
> +				    &data->gts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init iio gts\n");
> +
>   	ret = veml6030_als_shut_down(data);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "can't shutdown als\n");
> @@ -1119,11 +1010,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
>   		return dev_err_probe(dev, ret,
>   				     "can't clear als interrupt status\n");
>   
> -	/* Cache currently active measurement parameters */
> -	data->cur_gain = 3;
> -	data->cur_resolution = 5376;
> -	data->cur_integration_time = 3;
> -
>   	return ret;
>   }
>   

Yours,
	-- Matti

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Javier Carrasco]

On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote:
> On 07/01/2025 22:50, Javier Carrasco wrote:
> > The current scale is not ABI-compliant as it is just the sensor gain
> > instead of the value that acts as a multiplier to be applied to the raw
> > value (there is no offset).
> >
> > Use the iio-gts helpers to obtain the proper scale values according to
> > the gain and integration time to match the resolution tables from the
> > datasheet and drop dedicated variables to store the current values of
> > the integration time, gain and resolution. When at it, use 'scale'
> > instead of 'gain' consistently for the get/set functions to avoid
> > misunderstandings.
> >
> > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> Thanks for the patch Javier.
>
> And, sorry for a review which is more about questions than suggested
> improvements. I find it hard to focus on reading code today.
>
> > ---
> >   drivers/iio/light/Kconfig    |   1 +
> >   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
> >   2 files changed, 189 insertions(+), 311 deletions(-)
> >
>
> I like the diffstats of this Fix! :) It's nice you found gts-helpers
> helpful :)

I wonder how painful the int. time/gain/scale issue in ALS was before
iio-gts existed :D

>
> ...
>
> > +
> > +static int veml6030_process_als(struct veml6030_data *data, int raw,
> > +				int *val, int *val2)
> > +{
> > +	int ret, scale_int, scale_nano;
> > +	u64 tmp;
> > +
> > +	ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	tmp = (u64)raw * scale_nano;
> > +	*val = raw * scale_int + div_u64(tmp, NANO);
> > +	*val2 = div_u64(do_div(tmp, NANO), MILLI);
>
> do_div() is horrible on some platforms. Or, at least it used to be. Is
> there really no way to go without it? We're dealing with 16bit data and
> maximum of 512x total gain only, so maybe there was a way(?)
>
> Maybe a stupid question (in which case I blame mucus in my head) - could
> you just divide the raw value by the total gain?
>

In its current form we need the 64-bit operations to handle the scale,
and it will probably have to stay like that for the reasons I give you
below.

> ...
>
> >
> >   static irqreturn_t veml6030_event_handler(int irq, void *private)
> > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
> >   	int ret, val;
> >   	struct veml6030_data *data = iio_priv(indio_dev);
> >
> > +	ret = devm_iio_init_iio_gts(dev, 2, 150400000,
>
> Can you please explain the seemingly odd choice for the max scale?
>
> Just a quick look at the sensor spec and this driver allows me to assume
> following:
>
> Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.
>
> If we chose the 'total gain' 1x to match scale 1, then the smallest
> scale would be 1/512 = 0.001 953 125
>
> This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would
> not cause precision loss. (Well, I'm not at my sharpest as I've caught
> cold - but I _think_ this is true, right?)
>
> If I read this correctly, the only channel where the scale gets applied
> is the INTENSITY channel, right? Hence it should be possible to chose
> the scale as we see best. (Unless the idea of this seemingly odd scale
> is to maintain the old intensity / scale values in order to not shake
> userland any more - in which case this could be mentioned).
>

The scale is applied to an IIO_LIGHT channel, not to the INTENSITY,
which forces us to provide the scale as a value that multiplied by the
raw measurement gives a result in lux. The maximum scale in that case,
as provided by the application note [1] (page 5, RESOLUTION AND MAXIMUM
DETECTION RANGE table) is 2.1504 to convert from cnt to lux.

The same applies for the rest of the device supported by this driver
(veml6035 and veml6035).

>
> Yours,
> 	-- Matti

Thank you for your feedback, I hope my reply could answer your
questions. If something is still not clear or simply wrong, please let
me know.

Best regards,
Javier Carrasco

Link to app note: https://www.vishay.com/docs/84367/designingveml6030.pdf [1]

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Matti Vaittinen]

ma 13.1.2025 klo 17.05 Javier Carrasco
(javier.carrasco.cruz@gmail.com) kirjoitti:
>
> On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote:
> > On 07/01/2025 22:50, Javier Carrasco wrote:
> > > The current scale is not ABI-compliant as it is just the sensor gain
> > > instead of the value that acts as a multiplier to be applied to the raw
> > > value (there is no offset).
> > >
> > > Use the iio-gts helpers to obtain the proper scale values according to
> > > the gain and integration time to match the resolution tables from the
> > > datasheet and drop dedicated variables to store the current values of
> > > the integration time, gain and resolution. When at it, use 'scale'
> > > instead of 'gain' consistently for the get/set functions to avoid
> > > misunderstandings.
> > >
> > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> >
> > Thanks for the patch Javier.
> >
> > And, sorry for a review which is more about questions than suggested
> > improvements. I find it hard to focus on reading code today.
> >
> > > ---
> > >   drivers/iio/light/Kconfig    |   1 +
> > >   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
> > >   2 files changed, 189 insertions(+), 311 deletions(-)
> > >
> >
> > I like the diffstats of this Fix! :) It's nice you found gts-helpers
> > helpful :)
>
> I wonder how painful the int. time/gain/scale issue in ALS was before
> iio-gts existed :D
>
I don't really know. I wrote the gts-helpers as soon as I wrote my
first light sensor driver :)

> > ...
> >
> > > +
> > > +static int veml6030_process_als(struct veml6030_data *data, int raw,
> > > +                           int *val, int *val2)
> > > +{
> > > +   int ret, scale_int, scale_nano;
> > > +   u64 tmp;
> > > +
> > > +   ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> > > +   if (ret < 0)
> > > +           return ret;
> > > +
> > > +   tmp = (u64)raw * scale_nano;
> > > +   *val = raw * scale_int + div_u64(tmp, NANO);
> > > +   *val2 = div_u64(do_div(tmp, NANO), MILLI);
> >
> > do_div() is horrible on some platforms. Or, at least it used to be. Is
> > there really no way to go without it? We're dealing with 16bit data and
> > maximum of 512x total gain only, so maybe there was a way(?)
> >
> > Maybe a stupid question (in which case I blame mucus in my head) - could
> > you just divide the raw value by the total gain?
> >
>
> In its current form we need the 64-bit operations to handle the scale,
> and it will probably have to stay like that for the reasons I give you
> below.

Still, I wonder if multiplying by scale really differs from dividing
by the total gain? I think the scale is inversely proportional to the
total gain, right?

> > >   static irqreturn_t veml6030_event_handler(int irq, void *private)
> > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
> > >     int ret, val;
> > >     struct veml6030_data *data = iio_priv(indio_dev);
> > >
> > > +   ret = devm_iio_init_iio_gts(dev, 2, 150400000,
> >
> > Can you please explain the seemingly odd choice for the max scale?
> >
> > Just a quick look at the sensor spec and this driver allows me to assume
> > following:
> >
> > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.
> >
> > If we chose the 'total gain' 1x to match scale 1, then the smallest
> > scale would be 1/512 = 0.001 953 125
> >
> > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would
> > not cause precision loss. (Well, I'm not at my sharpest as I've caught
> > cold - but I _think_ this is true, right?)
> >
> > If I read this correctly, the only channel where the scale gets applied
> > is the INTENSITY channel, right? Hence it should be possible to chose
> > the scale as we see best. (Unless the idea of this seemingly odd scale
> > is to maintain the old intensity / scale values in order to not shake
> > userland any more - in which case this could be mentioned).
> >
>
> The scale is applied to an IIO_LIGHT channel, not to the INTENSITY,

Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale
shouldn't be applied to it. (Driver may still apply scale internally,
but users should not see it, right? And if the driver does it only
internally, then the driver can also multiply values using (N *
scale). Well, I suppose you can as well use this "fitted MAX SCALE" -
but maybe it warrants a comment.

> which forces us to provide the scale as a value that multiplied by the
> raw measurement gives a result in lux. The maximum scale in that case,
> as provided by the application note [1] (page 5, RESOLUTION AND MAXIMUM
> DETECTION RANGE table) is 2.1504 to convert from cnt to lux.
>
> The same applies for the rest of the device supported by this driver
> (veml6035 and veml6035).
>

Yours,
  -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Javier Carrasco]

On Mon Jan 13, 2025 at 8:52 PM CET, Matti Vaittinen wrote:
> ma 13.1.2025 klo 17.05 Javier Carrasco
> (javier.carrasco.cruz@gmail.com) kirjoitti:
> >
> > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote:
> > > On 07/01/2025 22:50, Javier Carrasco wrote:
> > > > The current scale is not ABI-compliant as it is just the sensor gain
> > > > instead of the value that acts as a multiplier to be applied to the raw
> > > > value (there is no offset).
> > > >
> > > > Use the iio-gts helpers to obtain the proper scale values according to
> > > > the gain and integration time to match the resolution tables from the
> > > > datasheet and drop dedicated variables to store the current values of
> > > > the integration time, gain and resolution. When at it, use 'scale'
> > > > instead of 'gain' consistently for the get/set functions to avoid
> > > > misunderstandings.
> > > >
> > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > >
> > > Thanks for the patch Javier.
> > >
> > > And, sorry for a review which is more about questions than suggested
> > > improvements. I find it hard to focus on reading code today.
> > >
> > > > ---
> > > >   drivers/iio/light/Kconfig    |   1 +
> > > >   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
> > > >   2 files changed, 189 insertions(+), 311 deletions(-)
> > > >
> > >
> > > I like the diffstats of this Fix! :) It's nice you found gts-helpers
> > > helpful :)
> >
> > I wonder how painful the int. time/gain/scale issue in ALS was before
> > iio-gts existed :D
> >
> I don't really know. I wrote the gts-helpers as soon as I wrote my
> first light sensor driver :)
>
> > > ...
> > >
> > > > +
> > > > +static int veml6030_process_als(struct veml6030_data *data, int raw,
> > > > +                           int *val, int *val2)
> > > > +{
> > > > +   int ret, scale_int, scale_nano;
> > > > +   u64 tmp;
> > > > +
> > > > +   ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> > > > +   if (ret < 0)
> > > > +           return ret;
> > > > +
> > > > +   tmp = (u64)raw * scale_nano;
> > > > +   *val = raw * scale_int + div_u64(tmp, NANO);
> > > > +   *val2 = div_u64(do_div(tmp, NANO), MILLI);
> > >
> > > do_div() is horrible on some platforms. Or, at least it used to be. Is
> > > there really no way to go without it? We're dealing with 16bit data and
> > > maximum of 512x total gain only, so maybe there was a way(?)
> > >
> > > Maybe a stupid question (in which case I blame mucus in my head) - could
> > > you just divide the raw value by the total gain?
> > >
> >
> > In its current form we need the 64-bit operations to handle the scale,
> > and it will probably have to stay like that for the reasons I give you
> > below.
>
> Still, I wonder if multiplying by scale really differs from dividing
> by the total gain? I think the scale is inversely proportional to the
> total gain, right?
>

I am sorry, but I am not sure if I got your point here. The scale is the
multiplier to get lux from raw, and for example it's not just 1/512 for
the maximum total gain, as that is not taking the intrinsic resolution
of the sensor. Maybe I am misunderstanding something, but I don't see the
way around raw * scale with the scale being one of the discrete values
provided in the application note.

I will give you a simple example, so you can tell me where my reasoning
fails:

raw = 100 counts
scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)

The reply to your comment below explains why we have a PROCESSED
IIO_LIGHT in the first place.

> > > >   static irqreturn_t veml6030_event_handler(int irq, void *private)
> > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
> > > >     int ret, val;
> > > >     struct veml6030_data *data = iio_priv(indio_dev);
> > > >
> > > > +   ret = devm_iio_init_iio_gts(dev, 2, 150400000,
> > >
> > > Can you please explain the seemingly odd choice for the max scale?
> > >
> > > Just a quick look at the sensor spec and this driver allows me to assume
> > > following:
> > >
> > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.
> > >
> > > If we chose the 'total gain' 1x to match scale 1, then the smallest
> > > scale would be 1/512 = 0.001 953 125
> > >
> > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would
> > > not cause precision loss. (Well, I'm not at my sharpest as I've caught
> > > cold - but I _think_ this is true, right?)
> > >
> > > If I read this correctly, the only channel where the scale gets applied
> > > is the INTENSITY channel, right? Hence it should be possible to chose
> > > the scale as we see best. (Unless the idea of this seemingly odd scale
> > > is to maintain the old intensity / scale values in order to not shake
> > > userland any more - in which case this could be mentioned).
> > >
> >
> > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY,
>
> Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale
> shouldn't be applied to it. (Driver may still apply scale internally,
> but users should not see it, right? And if the driver does it only
> internally, then the driver can also multiply values using (N *
> scale). Well, I suppose you can as well use this "fitted MAX SCALE" -
> but maybe it warrants a comment.

IIO_LIGHT provides RAW and PROCESSED values, which shouldn't have
happened in the first place as PROCESSED is just raw * scale, if scale
had been right from the beginning. Actually, when I took over this
driver, I was tempted to drop the PROCESSED value, but it was too late
for that without breaking ABI. My guess is that the processed value was
provided because in_illuminance_scale was not the right multiplier, only
the gain.
Note that in_illuminance_scale is also provided to the user, and that
must comply with the ABI definitions.

Thank you again,
Javier Carrasco

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Matti Vaittinen]

ti 14.1.2025 klo 0.32 Javier Carrasco (javier.carrasco.cruz@gmail.com)
kirjoitti:
>
> On Mon Jan 13, 2025 at 8:52 PM CET, Matti Vaittinen wrote:
> > ma 13.1.2025 klo 17.05 Javier Carrasco
> > (javier.carrasco.cruz@gmail.com) kirjoitti:
> > >
> > > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote:
> > > > On 07/01/2025 22:50, Javier Carrasco wrote:
> > > > > The current scale is not ABI-compliant as it is just the sensor gain
> > > > > instead of the value that acts as a multiplier to be applied to the raw
> > > > > value (there is no offset).
> > > > >
> > > > > Use the iio-gts helpers to obtain the proper scale values according to
> > > > > the gain and integration time to match the resolution tables from the
> > > > > datasheet and drop dedicated variables to store the current values of
> > > > > the integration time, gain and resolution. When at it, use 'scale'
> > > > > instead of 'gain' consistently for the get/set functions to avoid
> > > > > misunderstandings.
> > > > >
> > > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> > > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > > >
> > > > Thanks for the patch Javier.
> > > >
> > > > And, sorry for a review which is more about questions than suggested
> > > > improvements. I find it hard to focus on reading code today.
> > > >
> > > > > ---
> > > > >   drivers/iio/light/Kconfig    |   1 +
> > > > >   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
> > > > >   2 files changed, 189 insertions(+), 311 deletions(-)
> > > > ...
> > > >
> > > > > +
> > > > > +static int veml6030_process_als(struct veml6030_data *data, int raw,
> > > > > +                           int *val, int *val2)
> > > > > +{
> > > > > +   int ret, scale_int, scale_nano;
> > > > > +   u64 tmp;
> > > > > +
> > > > > +   ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> > > > > +   if (ret < 0)
> > > > > +           return ret;
> > > > > +
> > > > > +   tmp = (u64)raw * scale_nano;
> > > > > +   *val = raw * scale_int + div_u64(tmp, NANO);
> > > > > +   *val2 = div_u64(do_div(tmp, NANO), MILLI);
> > > >
> > > > do_div() is horrible on some platforms. Or, at least it used to be. Is
> > > > there really no way to go without it? We're dealing with 16bit data and
> > > > maximum of 512x total gain only, so maybe there was a way(?)
> > > >
> > > > Maybe a stupid question (in which case I blame mucus in my head) - could
> > > > you just divide the raw value by the total gain?
> > > >
> > >
> > > In its current form we need the 64-bit operations to handle the scale,
> > > and it will probably have to stay like that for the reasons I give you
> > > below.
> >
> > Still, I wonder if multiplying by scale really differs from dividing
> > by the total gain? I think the scale is inversely proportional to the
> > total gain, right?
> >
> I am sorry, but I am not sure if I got your point here. The scale is the
> multiplier to get lux from raw, and for example it's not just 1/512 for
> the maximum total gain, as that is not taking the intrinsic resolution
> of the sensor. Maybe I am misunderstanding something, but I don't see the
> way around raw * scale with the scale being one of the discrete values
> provided in the application note.
>
> I will give you a simple example, so you can tell me where my reasoning
> fails:
>
> raw = 100 counts
> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)
>
> The reply to your comment below explains why we have a PROCESSED
> IIO_LIGHT in the first place.
>
> > > > >   static irqreturn_t veml6030_event_handler(int irq, void *private)
> > > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
> > > > >     int ret, val;
> > > > >     struct veml6030_data *data = iio_priv(indio_dev);
> > > > >
> > > > > +   ret = devm_iio_init_iio_gts(dev, 2, 150400000,
> > > >
> > > > Can you please explain the seemingly odd choice for the max scale?
> > > >
> > > > Just a quick look at the sensor spec and this driver allows me to assume
> > > > following:
> > > >
> > > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.
> > > >
> > > > If we chose the 'total gain' 1x to match scale 1, then the smallest
> > > > scale would be 1/512 = 0.001 953 125
> > > >
> > > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would
> > > > not cause precision loss. (Well, I'm not at my sharpest as I've caught
> > > > cold - but I _think_ this is true, right?)
> > > >
> > > > If I read this correctly, the only channel where the scale gets applied
> > > > is the INTENSITY channel, right? Hence it should be possible to chose
> > > > the scale as we see best. (Unless the idea of this seemingly odd scale
> > > > is to maintain the old intensity / scale values in order to not shake
> > > > userland any more - in which case this could be mentioned).
> > > >
> > >
> > > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY,
> >
> > Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale
> > shouldn't be applied to it. (Driver may still apply scale internally,
> > but users should not see it, right? And if the driver does it only
> > internally, then the driver can also multiply values using (N *
> > scale). Well, I suppose you can as well use this "fitted MAX SCALE" -
> > but maybe it warrants a comment.
>
> IIO_LIGHT provides RAW and PROCESSED values,

Thanks. This explains why you chose the MAX SCALE in IIo init to be
this oddish looking value :)

> which shouldn't have
> happened in the first place as PROCESSED is just raw * scale, if scale
> had been right from the beginning. Actually, when I took over this
> driver, I was tempted to drop the PROCESSED value, but it was too late
> for that without breaking ABI. My guess is that the processed value was
> provided because in_illuminance_scale was not the right multiplier, only
> the gain.
> Note that in_illuminance_scale is also provided to the user, and that
> must comply with the ABI definitions.

Sure. Raw * scale must be lux. You still could use some driver
internal scale multiplier which you
applied before pushing the lux values to users, but this would make
the available scales handling more complicated. Hence, I now fully
agree with you using the
2, 150400000, in devm_iio_init_iio_gts() as max scale. Thank you for
the patience :)

I copied part of your reply below as I think this is the right order
of discussion:

> I will give you a simple example, so you can tell me where my reasoning
> fails:
>
> raw = 100 counts
> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)

Your reasoning does not fail. But, the scale = 1 / (N * total_gain),
right? (N here depends on how we choose the scale/gain values) Here,
the total_gain means the effect of both the hardware_gain and the
integration time.

Hence,
processed =  X * (raw * scale)

=> processed = X * (raw * (1 / (N * total_gain))
=> processed = X * raw / (N * total_gain);

Hence I thought you might be able to get rid of this 64bit division by
using the total_gain from the iio_gts_get_total_gain() instead of
using the scale. Or, am I missing something?

Yours,
  -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Javier Carrasco]

On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote:
...
> > I will give you a simple example, so you can tell me where my reasoning
> > fails:
> >
> > raw = 100 counts
> > scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
> > processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)
>
> Your reasoning does not fail. But, the scale = 1 / (N * total_gain),
> right? (N here depends on how we choose the scale/gain values) Here,
> the total_gain means the effect of both the hardware_gain and the
> integration time.
>
> Hence,
> processed =  X * (raw * scale)
>
> => processed = X * (raw * (1 / (N * total_gain))
> => processed = X * raw / (N * total_gain);
>
> Hence I thought you might be able to get rid of this 64bit division by
> using the total_gain from the iio_gts_get_total_gain() instead of
> using the scale. Or, am I missing something?
>

I am not sure by X you mean the maximum resolution, but if that is the
case, the following would work (pseudo-code):

/* Maximum resolution (2.1504 lux/count) * 10000 */
#define VEML6030_MAX_RES 21504

total_gain = iio_gts_get_total_gain();
processed_int = raw * VEML6030_MAX_RES / total_gain / 10000;
processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100;

return INT_PLUS_MICRO;

Is that what you meant? For my previous example (100 counts, IT=25ms,
GAIN=1/8 → total_gain = 1 * 1):
processed_int = 100 * 21504 / 1 / 10000; (215)
processed_micro = 100 * 21504 / 1 % 10000 * 100; (40000)
The expected value was 215.04 lux

For IT=800ms, GAIN=2 → total_gain = 32 * 16 = 512
processed_int = 100 * 21504 / 512 / 10000; (0)
processed_micro = 100 * 21504 / 512 % 10000 * 100; (420000)
That is also the expected value: 0.42 lux

Given that the driver supports multiple devices with different maximum
scales (currently 2), it will have to be added to the chip data.

If we are now on the same page, I will implement it like that to drop
64-bit divisions.

Thanks again!

Best regards,
Javier Carrasco

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Matti Vaittinen]

On 14/01/2025 15:02, Javier Carrasco wrote:
> On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote:
> ...
>>> I will give you a simple example, so you can tell me where my reasoning
>>> fails:
>>>
>>> raw = 100 counts
>>> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
>>> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)
>>
>> Your reasoning does not fail. But, the scale = 1 / (N * total_gain),
>> right? (N here depends on how we choose the scale/gain values) Here,
>> the total_gain means the effect of both the hardware_gain and the
>> integration time.
>>
>> Hence,
>> processed =  X * (raw * scale)
>>
>> => processed = X * (raw * (1 / (N * total_gain))
>> => processed = X * raw / (N * total_gain);
>>
>> Hence I thought you might be able to get rid of this 64bit division by
>> using the total_gain from the iio_gts_get_total_gain() instead of
>> using the scale. Or, am I missing something?
>>
> 
> I am not sure by X you mean the maximum resolution, but if that is the
> case, the following would work (pseudo-code):

Yes. X denoted the value by which the count needs to be multiplied to 
get the lux (when total gain "in the terms of gts" is x1. I think in 
this particular case the "gain is x1" is a bit confusing as it appears 
this really means the hardware gain is 1/8, right?). Anyways, lux/count 
it is, so in short - yes. :)

> 
> /* Maximum resolution (2.1504 lux/count) * 10000 */
> #define VEML6030_MAX_RES 21504
> 
> total_gain = iio_gts_get_total_gain();
> processed_int = raw * VEML6030_MAX_RES / total_gain / 10000;

Yes. This is what I was thinking of.

> processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100;

gah. I didn't consider representing the micro portion. Staring this 
makes me feel dizzy :) Well, it looks correct, and I guess the precision 
is not lost by the division(?) But yes, you did perfectly get what I was 
after!

Jonathan, do you think I am just guiding Javier to make a mess? :)

If not, then this might be the way to go.

Yours,
	-- Matti

Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

[Jonathan Cameron]

On Tue, 14 Jan 2025 16:26:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 14/01/2025 15:02, Javier Carrasco wrote:
> > On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote:
> > ...  
> >>> I will give you a simple example, so you can tell me where my reasoning
> >>> fails:
> >>>
> >>> raw = 100 counts
> >>> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
> >>> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)  
> >>
> >> Your reasoning does not fail. But, the scale = 1 / (N * total_gain),
> >> right? (N here depends on how we choose the scale/gain values) Here,
> >> the total_gain means the effect of both the hardware_gain and the
> >> integration time.
> >>
> >> Hence,
> >> processed =  X * (raw * scale)
> >>  
> >> => processed = X * (raw * (1 / (N * total_gain))
> >> => processed = X * raw / (N * total_gain);  
> >>
> >> Hence I thought you might be able to get rid of this 64bit division by
> >> using the total_gain from the iio_gts_get_total_gain() instead of
> >> using the scale. Or, am I missing something?
> >>  
> > 
> > I am not sure by X you mean the maximum resolution, but if that is the
> > case, the following would work (pseudo-code):  
> 
> Yes. X denoted the value by which the count needs to be multiplied to 
> get the lux (when total gain "in the terms of gts" is x1. I think in 
> this particular case the "gain is x1" is a bit confusing as it appears 
> this really means the hardware gain is 1/8, right?). Anyways, lux/count 
> it is, so in short - yes. :)
> 
> > 
> > /* Maximum resolution (2.1504 lux/count) * 10000 */
> > #define VEML6030_MAX_RES 21504
> > 
> > total_gain = iio_gts_get_total_gain();
> > processed_int = raw * VEML6030_MAX_RES / total_gain / 10000;  
> 
> Yes. This is what I was thinking of.
> 
> > processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100;  
> 
> gah. I didn't consider representing the micro portion. Staring this 
> makes me feel dizzy :) Well, it looks correct, and I guess the precision 
> is not lost by the division(?) But yes, you did perfectly get what I was 
> after!
> 
> Jonathan, do you think I am just guiding Javier to make a mess? :)

This is an area you've thought about a lot more than me.

Whether it is worth avoiding the 64 bit maths is an interesting question
and I guess depends where this part is typically showing up.

Jonathan

> 
> If not, then this might be the way to go.
> 
> Yours,
> 	-- Matti