[PATCH 0/2] iio: light: fix scale in veml3235 and add helpers to iio-gts

[PATCH 0/2] iio: light: fix scale in veml3235 and add helpers to iio-gts

[Javier Carrasco]

This series addresses an issue in the veml3235 that was inherited from
an older driver (veml6030, not covered here but probably addressed after
discussing this series), where the scale is does not follow ABI.

To simplify the gain/integration time handling, the iio-gts helpers have
been used. And to further simplify the process, two new helpers have
been proposed to address repetitive patterns that are found in all users
of iio-gts.

The additions to iio-gts are wrappers around existing helpers, and I
have tried to keep their names short, as adding more prefixes to the
existing functions looked too cumbersome and inconvenient to follow the
80-char/line recommendation. I have not added any test for the new
helpers because I would prefer to discuss them first.

This series has been tested with a veml3235sl under all supported gains
and integration times as well as with a few unsupported values to make
sure the operations fail in those cases.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      iio: gts-helper: add helpers to ease searches of gain_sel and new_gain
      iio: veml3235: fix scale to conform to ABI

 drivers/iio/industrialio-gts-helper.c |  74 ++++++++++
 drivers/iio/light/Kconfig             |   1 +
 drivers/iio/light/veml3235.c          | 252 +++++++++++++++++++---------------
 include/linux/iio/iio-gts-helper.h    |   5 +
 4 files changed, 219 insertions(+), 113 deletions(-)
---
base-commit: e25c8d66f6786300b680866c0e0139981273feba
change-id: 20241215-veml3235_scale-62de98c7b5fa

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

[PATCH 1/2] iio: gts-helper: add helpers to ease searches of gain_sel and new_gain

[Javier Carrasco]

This helper functions reduce the burden in the drivers that want to
fetch a gain selector in all available times or a new optimal gain.

The former is currently achieved by calling
iio_gts_find_gain_sel_for_scale_using_time() for the current time
selector, and then iterating over the rest of time selectors if the
gain selector was not found.

The latter requires a combination of multiple iio-gts helpers to find
the new gain, look for an optimal gain if there was no exact match, and
set a minimum gain if the optimal gain is not in the range of available
gains.

Provide simpler workflows by means of functions that address common
patterns in the users of the iio-gts helpers.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/industrialio-gts-helper.c | 74 +++++++++++++++++++++++++++++++++++
 include/linux/iio/iio-gts-helper.h    |  5 +++
 2 files changed, 79 insertions(+)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 3b5a99815062..f88b0b7192dd 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -915,6 +915,38 @@ int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel
 }
 EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_for_scale_using_time, "IIO_GTS_HELPER");
 
+/**
+ * iio_gts_find_gain_sel_in_times - Fetch gain selector in the available times.
+ * @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_sel:	Pointer to value where gain selector is stored.
+ * @time_sel:	Pointer to value where time selector is stored.
+ *
+ * Wrapper around iio_gts_find_gain_for_scale_using_time() to fetch the
+ * gain selector for all supported integration times.
+ *
+ * Return: 0 on success and -EINVAL on error.
+ */
+int iio_gts_find_gain_sel_in_times(struct iio_gts *gts, int scale_int,
+				   int scale_nano, int *gain_sel, int *time_sel)
+{
+	int i, ret;
+
+	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_gain_sel_in_times, "IIO_GTS_HELPER");
+
 static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
 {
 	const struct iio_itime_sel_mul *itime;
@@ -1086,6 +1118,48 @@ int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
 }
 EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_old_gain_time, "IIO_GTS_HELPER");
 
+/**
+ * iio_gts_find_new_gain_by_gain_time_min - compensate for time change
+ * @gts:	Gain time scale descriptor
+ * @old_gain:	Previously set gain
+ * @old_time:	Selector corresponding previously set time
+ * @new_time:	Selector corresponding new time to be set
+ * @new_gain:	Pointer to value where new gain is to be written
+ * @in_range:	Indicate if the @new_gain was in the range of
+ *		supported gains.
+ *
+ * Wrapper around iio_gts_find_new_gain_by_old_gain_time() that tries to
+ * set an optimal value if no exact match was found, defaulting to the
+ * minimum gain to avoid saturations if the optimal value is not in the
+ * range of supported gains.
+ *
+ * Return: 0 on success and a negative value if no gain was found.
+ */
+int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
+					   int old_time, int new_time,
+					   int *new_gain, bool *in_range)
+{
+	int ret;
+
+	*in_range = true;
+	ret = iio_gts_find_new_gain_by_old_gain_time(gts, old_gain, old_time,
+						     new_time, new_gain);
+	if (*new_gain < 0)
+		return -EINVAL;
+
+	if (ret) {
+		*new_gain = iio_find_closest_gain_low(gts, *new_gain, in_range);
+		if (*new_gain < 0) {
+			*new_gain = iio_gts_get_min_gain(gts);
+			if (*new_gain < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_gain_time_min, "IIO_GTS_HELPER");
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
 MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
diff --git a/include/linux/iio/iio-gts-helper.h b/include/linux/iio/iio-gts-helper.h
index 9cb6c80dea71..ae91ad008cc8 100644
--- a/include/linux/iio/iio-gts-helper.h
+++ b/include/linux/iio/iio-gts-helper.h
@@ -188,6 +188,8 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
 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_sel_in_times(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,
@@ -196,6 +198,9 @@ int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
 int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
 					   int old_time, int new_time,
 					   int *new_gain);
+int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
+					   int old_time, int new_time,
+					   int *new_gain, bool *in_range);
 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,

-- 
2.43.0

[PATCH 2/2] iio: veml3235: 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.

Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/Kconfig    |   1 +
 drivers/iio/light/veml3235.c | 252 ++++++++++++++++++++++++-------------------
 2 files changed, 140 insertions(+), 113 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0cf9cf2a3f94..12864691a7ff 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -656,6 +656,7 @@ config VCNL4035
 config VEML3235
 	tristate "VEML3235 ambient light sensor"
 	select REGMAP_I2C
+	select IIO_GTS_HELPER
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VEML3235
diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
index 66361c3012a3..4ace3f8e95f1 100644
--- a/drivers/iio/light/veml3235.c
+++ b/drivers/iio/light/veml3235.c
@@ -11,6 +11,7 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -35,17 +36,33 @@ struct veml3235_data {
 	struct device *dev;
 	struct regmap *regmap;
 	struct veml3235_rf rf;
+	struct iio_gts gts;
 };
 
-static const int veml3235_it_times[][2] = {
-	{ 0, 50000 },
-	{ 0, 100000 },
-	{ 0, 200000 },
-	{ 0, 400000 },
-	{ 0, 800000 },
+static const struct iio_itime_sel_mul veml3235_it_sel[] = {
+	GAIN_SCALE_ITIME_US(50000, 0, 1),
+	GAIN_SCALE_ITIME_US(100000, 1, 2),
+	GAIN_SCALE_ITIME_US(200000, 2, 4),
+	GAIN_SCALE_ITIME_US(400000, 3, 8),
+	GAIN_SCALE_ITIME_US(800000, 4, 16),
 };
 
-static const int veml3235_scale_vals[] = { 1, 2, 4, 8 };
+/*
+ * The MSB (DG) doubles the value of the rest of the field, which leads to
+ * two possible combinations to obtain gain = 2 and gain = 4. The gain
+ * handlding can be simplified by restricting DG = 1 to the only gain that
+ * really requires it, gain = 8. Note that "X10" is a reserved value.
+ */
+#define VEML3235_SEL_GAIN_X1 0
+#define VEML3235_SEL_GAIN_X2 1
+#define VEML3235_SEL_GAIN_X4 3
+#define VEML3235_SEL_GAIN_X8 7
+static const struct iio_gain_sel_pair veml3235_gain_sel[] = {
+	GAIN_SCALE_GAIN(1, VEML3235_SEL_GAIN_X1),
+	GAIN_SCALE_GAIN(2, VEML3235_SEL_GAIN_X2),
+	GAIN_SCALE_GAIN(4, VEML3235_SEL_GAIN_X4),
+	GAIN_SCALE_GAIN(8, VEML3235_SEL_GAIN_X8),
+};
 
 static int veml3235_power_on(struct veml3235_data *data)
 {
@@ -111,112 +128,99 @@ static const struct regmap_config veml3235_regmap_config = {
 
 static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2)
 {
-	int ret, reg;
+	int ret, it_idx;
 
-	ret = regmap_field_read(data->rf.it, &reg);
+	ret = regmap_field_read(data->rf.it, &it_idx);
 	if (ret)
 		return ret;
 
-	switch (reg) {
-	case 0:
-		*val2 = 50000;
-		break;
-	case 1:
-		*val2 = 100000;
-		break;
-	case 2:
-		*val2 = 200000;
-		break;
-	case 3:
-		*val2 = 400000;
-		break;
-	case 4:
-		*val2 = 800000;
-		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 veml3235_set_it(struct iio_dev *indio_dev, int val, int val2)
+static int veml3235_set_it(struct iio_dev *indio_dev, int val2)
 {
 	struct veml3235_data *data = iio_priv(indio_dev);
-	int ret, new_it;
+	int ret, gain_idx, it_idx, new_gain, prev_gain, prev_it;
+	bool in_range;
 
-	if (val)
+	if (!iio_gts_valid_time(&data->gts, val2))
 		return -EINVAL;
 
-	switch (val2) {
-	case 50000:
-		new_it = 0x00;
-		break;
-	case 100000:
-		new_it = 0x01;
-		break;
-	case 200000:
-		new_it = 0x02;
-		break;
-	case 400000:
-		new_it = 0x03;
-		break;
-	case 800000:
-		new_it = 0x04;
-		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_it);
-	if (ret) {
-		dev_err(data->dev,
-			"failed to update integration time: %d\n", ret);
+	ret = regmap_field_read(data->rf.gain, &gain_idx);
+	if (ret)
 		return ret;
-	}
 
-	return 0;
+	prev_it = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
+	if (prev_it < 0)
+		return prev_it;
+
+	if (prev_it == val2)
+		return 0;
+
+	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->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 veml3235_set_gain(struct iio_dev *indio_dev, int val, int val2)
+static int veml3235_set_scale(struct iio_dev *indio_dev, int val, int val2)
 {
 	struct veml3235_data *data = iio_priv(indio_dev);
-	int ret, new_gain;
+	int ret, it_idx, gain_sel, time_sel;
 
-	if (val2 != 0)
-		return -EINVAL;
-
-	switch (val) {
-	case 1:
-		new_gain = 0x00;
-		break;
-	case 2:
-		new_gain = 0x01;
-		break;
-	case 4:
-		new_gain = 0x03;
-		break;
-	case 8:
-		new_gain = 0x07;
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = regmap_field_read(data->rf.it, &it_idx);
+	if (ret)
+		return ret;
 
-	ret = regmap_field_write(data->rf.gain, new_gain);
-	if (ret) {
-		dev_err(data->dev, "failed to set gain: %d\n", ret);
+	ret = iio_gts_find_gain_sel_in_times(&data->gts, val, val2, &gain_sel,
+					     &time_sel);
+	if (ret)
 		return ret;
+
+	if (it_idx != time_sel) {
+		ret = regmap_field_write(data->rf.it, time_sel);
+		if (ret)
+			return ret;
 	}
 
-	return 0;
+	return regmap_field_write(data->rf.gain, gain_sel);
 }
 
-static int veml3235_get_gain(struct veml3235_data *data, int *val)
+static int veml3235_get_scale(struct veml3235_data *data, int *val, int *val2)
 {
-	int ret, reg;
+	int gain, it, reg, ret;
 
 	ret = regmap_field_read(data->rf.gain, &reg);
 	if (ret) {
@@ -224,25 +228,25 @@ static int veml3235_get_gain(struct veml3235_data *data, int *val)
 		return ret;
 	}
 
-	switch (reg & 0x03) {
-	case 0:
-		*val = 1;
-		break;
-	case 1:
-		*val = 2;
-		break;
-	case 3:
-		*val = 4;
-		break;
-	default:
-		return -EINVAL;
+	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) {
+		dev_err(data->dev, "failed to read integration time %d\n", ret);
+		return ret;
 	}
 
-	/* Double gain */
-	if (reg & 0x04)
-		*val *= 2;
+	it = iio_gts_find_int_time_by_sel(&data->gts, reg);
+	if (it < 0)
+		return it;
 
-	return IIO_VAL_INT;
+	ret = iio_gts_get_scale(&data->gts, gain, it, val, val2);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT_PLUS_NANO;
 }
 
 static int veml3235_read_raw(struct iio_dev *indio_dev,
@@ -276,7 +280,7 @@ static int veml3235_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_INT_TIME:
 		return veml3235_get_it(data, val, val2);
 	case IIO_CHAN_INFO_SCALE:
-		return veml3235_get_gain(data, val);
+		return veml3235_get_scale(data, val, val2);
 	default:
 		return -EINVAL;
 	}
@@ -287,17 +291,27 @@ static int veml3235_read_avail(struct iio_dev *indio_dev,
 			       const int **vals, int *type, int *length,
 			       long mask)
 {
+	struct veml3235_data *data = iio_priv(indio_dev);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
-		*vals = (int *)&veml3235_it_times;
-		*length = 2 * ARRAY_SIZE(veml3235_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 *)&veml3235_scale_vals;
-		*length = ARRAY_SIZE(veml3235_scale_vals);
-		*type = IIO_VAL_INT;
-		return IIO_AVAIL_LIST;
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml3235_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;
 	}
@@ -309,9 +323,12 @@ static int veml3235_write_raw(struct iio_dev *indio_dev,
 {
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
-		return veml3235_set_it(indio_dev, val, val2);
+		if (val)
+			return -EINVAL;
+
+		return veml3235_set_it(indio_dev, val2);
 	case IIO_CHAN_INFO_SCALE:
-		return veml3235_set_gain(indio_dev, val, val2);
+		return veml3235_set_scale(indio_dev, val, val2);
 	}
 
 	return -EINVAL;
@@ -321,7 +338,7 @@ static void veml3235_read_id(struct veml3235_data *data)
 {
 	int ret, reg;
 
-	ret = regmap_field_read(data->rf.id,  &reg);
+	ret = regmap_field_read(data->rf.id, &reg);
 	if (ret) {
 		dev_info(data->dev, "failed to read ID\n");
 		return;
@@ -371,6 +388,13 @@ static int veml3235_hw_init(struct iio_dev *indio_dev)
 	struct device *dev = data->dev;
 	int ret;
 
+	ret = devm_iio_init_iio_gts(data->dev, 0, 272640000,
+				    veml3235_gain_sel, ARRAY_SIZE(veml3235_gain_sel),
+				    veml3235_it_sel, ARRAY_SIZE(veml3235_it_sel),
+				    &data->gts);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "failed to init iio gts\n");
+
 	/* Set gain to 1 and integration time to 100 ms */
 	ret = regmap_field_write(data->rf.gain, 0x00);
 	if (ret)
@@ -389,9 +413,10 @@ static int veml3235_hw_init(struct iio_dev *indio_dev)
 }
 
 static const struct iio_info veml3235_info = {
-	.read_raw  = veml3235_read_raw,
-	.read_avail  = veml3235_read_avail,
+	.read_raw = veml3235_read_raw,
+	.read_avail = veml3235_read_avail,
 	.write_raw = veml3235_write_raw,
+	.write_raw_get_fmt = veml3235_write_raw_get_fmt,
 };
 
 static int veml3235_probe(struct i2c_client *client)
@@ -493,3 +518,4 @@ module_i2c_driver(veml3235_driver);
 MODULE_AUTHOR("Javier Carrasco <javier.carrasco.cruz@gmail.com>");
 MODULE_DESCRIPTION("VEML3235 Ambient Light Sensor");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_GTS_HELPER");

-- 
2.43.0

Re: [PATCH 1/2] iio: gts-helper: add helpers to ease searches of gain_sel and new_gain

[Matti Vaittinen]

On 20/12/2024 21:28, Javier Carrasco wrote:
> This helper functions reduce the burden in the drivers that want to
> fetch a gain selector in all available times or a new optimal gain.
> 
> The former is currently achieved by calling
> iio_gts_find_gain_sel_for_scale_using_time() for the current time
> selector, and then iterating over the rest of time selectors if the
> gain selector was not found.
> 
> The latter requires a combination of multiple iio-gts helpers to find
> the new gain, look for an optimal gain if there was no exact match, and
> set a minimum gain if the optimal gain is not in the range of available
> gains.
> 
> Provide simpler workflows by means of functions that address common
> patterns in the users of the iio-gts helpers.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   drivers/iio/industrialio-gts-helper.c | 74 +++++++++++++++++++++++++++++++++++
>   include/linux/iio/iio-gts-helper.h    |  5 +++
>   2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 3b5a99815062..f88b0b7192dd 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -915,6 +915,38 @@ int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel
>   }
>   EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_for_scale_using_time, "IIO_GTS_HELPER");
>   
> +/**
> + * iio_gts_find_gain_sel_in_times - Fetch gain selector in the available times.
> + * @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_sel:	Pointer to value where gain selector is stored.
> + * @time_sel:	Pointer to value where time selector is stored.
> + *
> + * Wrapper around iio_gts_find_gain_for_scale_using_time() to fetch the
> + * gain selector for all supported integration times.
> + *
> + * Return: 0 on success and -EINVAL on error.
> + */
> +int iio_gts_find_gain_sel_in_times(struct iio_gts *gts, int scale_int,
> +				   int scale_nano, int *gain_sel, int *time_sel)
> +{
> +	int i, ret;
> +
> +	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_gain_sel_in_times, "IIO_GTS_HELPER");
> +
>   static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
>   {
>   	const struct iio_itime_sel_mul *itime;
> @@ -1086,6 +1118,48 @@ int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
>   }
>   EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_old_gain_time, "IIO_GTS_HELPER");
>   
> +/**
> + * iio_gts_find_new_gain_by_gain_time_min - compensate for time change
> + * @gts:	Gain time scale descriptor
> + * @old_gain:	Previously set gain
> + * @old_time:	Selector corresponding previously set time
> + * @new_time:	Selector corresponding new time to be set
> + * @new_gain:	Pointer to value where new gain is to be written
> + * @in_range:	Indicate if the @new_gain was in the range of
> + *		supported gains.
> + *
> + * Wrapper around iio_gts_find_new_gain_by_old_gain_time() that tries to
> + * set an optimal value if no exact match was found, defaulting to the
> + * minimum gain to avoid saturations if the optimal value is not in the
> + * range of supported gains.
> + *
> + * Return: 0 on success and a negative value if no gain was found.
> + */
> +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> +					   int old_time, int new_time,
> +					   int *new_gain, bool *in_range)
> +{
> +	int ret;
> +
> +	*in_range = true;
> +	ret = iio_gts_find_new_gain_by_old_gain_time(gts, old_gain, old_time,
> +						     new_time, new_gain);
> +	if (*new_gain < 0)
> +		return -EINVAL;
> +
> +	if (ret) {
> +		*new_gain = iio_find_closest_gain_low(gts, *new_gain, in_range);
> +		if (*new_gain < 0) {
> +			*new_gain = iio_gts_get_min_gain(gts);
> +			if (*new_gain < 0)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_gain_time_min, "IIO_GTS_HELPER");
> +
>   MODULE_LICENSE("GPL");
>   MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
>   MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
> diff --git a/include/linux/iio/iio-gts-helper.h b/include/linux/iio/iio-gts-helper.h
> index 9cb6c80dea71..ae91ad008cc8 100644
> --- a/include/linux/iio/iio-gts-helper.h
> +++ b/include/linux/iio/iio-gts-helper.h
> @@ -188,6 +188,8 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
>   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_sel_in_times(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,
> @@ -196,6 +198,9 @@ int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
>   int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
>   					   int old_time, int new_time,
>   					   int *new_gain);
> +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> +					   int old_time, int new_time,
> +					   int *new_gain, bool *in_range);
>   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,
> 

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

[Matti Vaittinen]

On 20/12/2024 21:28, 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.
> 
> Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---

...

> +static const struct iio_itime_sel_mul veml3235_it_sel[] = {
> +	GAIN_SCALE_ITIME_US(50000, 0, 1),
> +	GAIN_SCALE_ITIME_US(100000, 1, 2),
> +	GAIN_SCALE_ITIME_US(200000, 2, 4),
> +	GAIN_SCALE_ITIME_US(400000, 3, 8),
> +	GAIN_SCALE_ITIME_US(800000, 4, 16),
>   };
>   
> -static const int veml3235_scale_vals[] = { 1, 2, 4, 8 };
> +/*
> + * The MSB (DG) doubles the value of the rest of the field, which leads to
> + * two possible combinations to obtain gain = 2 and gain = 4. The gain
> + * handlding can be simplified by restricting DG = 1 to the only gain that
> + * really requires it, gain = 8. Note that "X10" is a reserved value.

Just a question - do you ensure there is no "invalid" register values? I 
think Jonathan has prefered doing this by writing known initialization 
values at probe.

I *think* the GTS should survive cases where multiple bit patterns can 
be used to represent same gain/time - but I don't remember this for sure.

I didn't have the time to do a proper thorough review - sorry. Still, 
what I browsed quickly looked good :) Thanks!

Yours,
	-- Matti

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

[Javier Carrasco]

On Sun Dec 22, 2024 at 2:43 PM CET, Matti Vaittinen wrote:
> On 20/12/2024 21:28, 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.
> >
> > Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
>
> ...
>
> > +static const struct iio_itime_sel_mul veml3235_it_sel[] = {
> > +	GAIN_SCALE_ITIME_US(50000, 0, 1),
> > +	GAIN_SCALE_ITIME_US(100000, 1, 2),
> > +	GAIN_SCALE_ITIME_US(200000, 2, 4),
> > +	GAIN_SCALE_ITIME_US(400000, 3, 8),
> > +	GAIN_SCALE_ITIME_US(800000, 4, 16),
> >   };
> >
> > -static const int veml3235_scale_vals[] = { 1, 2, 4, 8 };
> > +/*
> > + * The MSB (DG) doubles the value of the rest of the field, which leads to
> > + * two possible combinations to obtain gain = 2 and gain = 4. The gain
> > + * handlding can be simplified by restricting DG = 1 to the only gain that
> > + * really requires it, gain = 8. Note that "X10" is a reserved value.
>
> Just a question - do you ensure there is no "invalid" register values? I
> think Jonathan has prefered doing this by writing known initialization
> values at probe.
>
> I *think* the GTS should survive cases where multiple bit patterns can
> be used to represent same gain/time - but I don't remember this for sure.
>
> I didn't have the time to do a proper thorough review - sorry. Still,
> what I browsed quickly looked good :) Thanks!
>
> Yours,
> 	-- Matti

Hi Matti,

Thanks for your feedback. The initial value of this register is indeed
set during the initialization (default is gain = 1). That comment is
only to explain why some combinations are missing, and why only one bit
patter per combination is required. The user does not care which one is
used, and it should be transparent to them.

I just noticed that there is a small typo in that comment (handlding
instead of handling), and I will update it for v2 after I collect more
feedback.

Best regards,
Javier Carrasco

Re: [PATCH 1/2] iio: gts-helper: add helpers to ease searches of gain_sel and new_gain

[Jonathan Cameron]

On Sat, 21 Dec 2024 20:19:21 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 20/12/2024 21:28, Javier Carrasco wrote:
> > This helper functions reduce the burden in the drivers that want to
> > fetch a gain selector in all available times or a new optimal gain.
> > 
> > The former is currently achieved by calling
> > iio_gts_find_gain_sel_for_scale_using_time() for the current time
> > selector, and then iterating over the rest of time selectors if the
> > gain selector was not found.
> > 
> > The latter requires a combination of multiple iio-gts helpers to find
> > the new gain, look for an optimal gain if there was no exact match, and
> > set a minimum gain if the optimal gain is not in the range of available
> > gains.
> > 
> > Provide simpler workflows by means of functions that address common
> > patterns in the users of the iio-gts helpers.
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> 
> Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
A couple of comments inline.

Thanks,

Jonathan

> > ---
> >   drivers/iio/industrialio-gts-helper.c | 74 +++++++++++++++++++++++++++++++++++
> >   include/linux/iio/iio-gts-helper.h    |  5 +++
> >   2 files changed, 79 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> > index 3b5a99815062..f88b0b7192dd 100644
> > --- a/drivers/iio/industrialio-gts-helper.c
> > +++ b/drivers/iio/industrialio-gts-helper.c
> > @@ -915,6 +915,38 @@ int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel
> >   }
> >   EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_for_scale_using_time, "IIO_GTS_HELPER");
> >   
> > +/**
> > + * iio_gts_find_gain_sel_in_times - Fetch gain selector in the available times.
> > + * @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_sel:	Pointer to value where gain selector is stored.
> > + * @time_sel:	Pointer to value where time selector is stored.
> > + *
> > + * Wrapper around iio_gts_find_gain_for_scale_using_time() to fetch the
> > + * gain selector for all supported integration times.
> > + *
> > + * Return: 0 on success and -EINVAL on error.
> > + */
> > +int iio_gts_find_gain_sel_in_times(struct iio_gts *gts, int scale_int,
> > +				   int scale_nano, int *gain_sel, int *time_sel)
> > +{
> > +	int i, ret;
> > +
> > +	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;
Trivial but slight preference for keeping the error path out of line rather than
the good path.

Easily done with:

		if (ret)
			continue;

		return 0;

> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_in_times, "IIO_GTS_HELPER");
> > +
> >   static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> >   {
> >   	const struct iio_itime_sel_mul *itime;
> > @@ -1086,6 +1118,48 @@ int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
> >   }
> >   EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_old_gain_time, "IIO_GTS_HELPER");
> >   
> > +/**
> > + * iio_gts_find_new_gain_by_gain_time_min - compensate for time change
> > + * @gts:	Gain time scale descriptor
> > + * @old_gain:	Previously set gain
> > + * @old_time:	Selector corresponding previously set time
> > + * @new_time:	Selector corresponding new time to be set
> > + * @new_gain:	Pointer to value where new gain is to be written
> > + * @in_range:	Indicate if the @new_gain was in the range of
> > + *		supported gains.
> > + *
> > + * Wrapper around iio_gts_find_new_gain_by_old_gain_time() that tries to
> > + * set an optimal value if no exact match was found, defaulting to the
> > + * minimum gain to avoid saturations if the optimal value is not in the
> > + * range of supported gains.
> > + *
> > + * Return: 0 on success and a negative value if no gain was found.
> > + */
> > +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> > +					   int old_time, int new_time,
> > +					   int *new_gain, bool *in_range)
> > +{
> > +	int ret;
> > +
> > +	*in_range = true;
> > +	ret = iio_gts_find_new_gain_by_old_gain_time(gts, old_gain, old_time,
> > +						     new_time, new_gain);
> > +	if (*new_gain < 0)
> > +		return -EINVAL;
> > +
> > +	if (ret) {
> > +		*new_gain = iio_find_closest_gain_low(gts, *new_gain, in_range);
> > +		if (*new_gain < 0) {
> > +			*new_gain = iio_gts_get_min_gain(gts);
> > +			if (*new_gain < 0)
> > +				return ret;

The return value here is possibly misleading as it comes from the original
iio_gts_find_new_gain_by_old_gain_time() call.
Sure it may have the same value, but I'd rather it was set explicitly here
so it was clear that we have a case where all fallbacks failed.


> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_gain_time_min, "IIO_GTS_HELPER");
> > +
> >   MODULE_LICENSE("GPL");
> >   MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
> >   MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
> > diff --git a/include/linux/iio/iio-gts-helper.h b/include/linux/iio/iio-gts-helper.h
> > index 9cb6c80dea71..ae91ad008cc8 100644
> > --- a/include/linux/iio/iio-gts-helper.h
> > +++ b/include/linux/iio/iio-gts-helper.h
> > @@ -188,6 +188,8 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
> >   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_sel_in_times(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,
> > @@ -196,6 +198,9 @@ int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> >   int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
> >   					   int old_time, int new_time,
> >   					   int *new_gain);
> > +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> > +					   int old_time, int new_time,
> > +					   int *new_gain, bool *in_range);
> >   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,
> >   
> 

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

[Jonathan Cameron]

On Fri, 20 Dec 2024 20:28:29 +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.
> 
> Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Hi Javier,

A few non fix related changes that make no functional difference made
it in here. Those should be done in a additional patch after this one
(to make the backport more minimal).

This change is large enough I probably won't directly apply it as a fix anyway.
Most likely it's material for the next merge window that will then get
backported after it is upstream.  That will give a small window in which
the broken code is in a release kernel, but it should hit stable at .2/.3 or
so and no one sane builds product before that point!

Thanks,

Jonathan

> ---
>  drivers/iio/light/Kconfig    |   1 +
>  drivers/iio/light/veml3235.c | 252 ++++++++++++++++++++++++-------------------
>  2 files changed, 140 insertions(+), 113 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 0cf9cf2a3f94..12864691a7ff 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -656,6 +656,7 @@ config VCNL4035
>  config VEML3235
>  	tristate "VEML3235 ambient light sensor"
>  	select REGMAP_I2C
> +	select IIO_GTS_HELPER
>  	depends on I2C
>  	help
>  	  Say Y here if you want to build a driver for the Vishay VEML3235
> diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
> index 66361c3012a3..4ace3f8e95f1 100644
> --- a/drivers/iio/light/veml3235.c
> +++ b/drivers/iio/light/veml3235.c

>  
> -static int veml3235_set_gain(struct iio_dev *indio_dev, int val, int val2)
> +static int veml3235_set_scale(struct iio_dev *indio_dev, int val, int val2)
>  {
>  	struct veml3235_data *data = iio_priv(indio_dev);
> -	int ret, new_gain;
> +	int ret, it_idx, gain_sel, time_sel;
>  
> -	if (val2 != 0)
> -		return -EINVAL;
> -
> -	switch (val) {
> -	case 1:
> -		new_gain = 0x00;
> -		break;
> -	case 2:
> -		new_gain = 0x01;
> -		break;
> -	case 4:
> -		new_gain = 0x03;
> -		break;
> -	case 8:
> -		new_gain = 0x07;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	ret = regmap_field_read(data->rf.it, &it_idx);
> +	if (ret)
> +		return ret;
>  
> -	ret = regmap_field_write(data->rf.gain, new_gain);
> -	if (ret) {
> -		dev_err(data->dev, "failed to set gain: %d\n", ret);
> +	ret = iio_gts_find_gain_sel_in_times(&data->gts, val, val2, &gain_sel,
> +					     &time_sel);
> +	if (ret)
>  		return ret;
> +
> +	if (it_idx != time_sel) {

Not part of this series, but might be worth turning on regcache for this driver.
Then you can do this sort of write unconditionally as it will hit in the cache
and do nothing anyway.  Mind you, this isn't a high performance path, so maybe
just write it anyway.

> +		ret = regmap_field_write(data->rf.it, time_sel);
> +		if (ret)
> +			return ret;
>  	}
>  
> -	return 0;
> +	return regmap_field_write(data->rf.gain, gain_sel);
>  }


> @@ -309,9 +323,12 @@ static int veml3235_write_raw(struct iio_dev *indio_dev,
>  {
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
> -		return veml3235_set_it(indio_dev, val, val2);
> +		if (val)
> +			return -EINVAL;

This yanking of the test out of the set function is fine, but maybe
as a precursor patch so as to reduce noise in the main change.

I'm not sure it's technically necessary either. More of a sensible
cleanup?

> +
> +		return veml3235_set_it(indio_dev, val2);
>  	case IIO_CHAN_INFO_SCALE:
> -		return veml3235_set_gain(indio_dev, val, val2);
> +		return veml3235_set_scale(indio_dev, val, val2);
>  	}
>  
>  	return -EINVAL;
> @@ -321,7 +338,7 @@ static void veml3235_read_id(struct veml3235_data *data)
>  {
>  	int ret, reg;
>  
> -	ret = regmap_field_read(data->rf.id,  &reg);
> +	ret = regmap_field_read(data->rf.id, &reg);

As below.  Follow up patch would be great, but not in here.

>  	if (ret) {
>  		dev_info(data->dev, "failed to read ID\n");
>  		return;
> @@ -371,6 +388,13 @@ static int veml3235_hw_init(struct iio_dev *indio_dev)
>  	struct device *dev = data->dev;
>  	int ret;
>  
> +	ret = devm_iio_init_iio_gts(data->dev, 0, 272640000,
> +				    veml3235_gain_sel, ARRAY_SIZE(veml3235_gain_sel),
> +				    veml3235_it_sel, ARRAY_SIZE(veml3235_it_sel),
> +				    &data->gts);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "failed to init iio gts\n");
> +
>  	/* Set gain to 1 and integration time to 100 ms */
>  	ret = regmap_field_write(data->rf.gain, 0x00);
>  	if (ret)
> @@ -389,9 +413,10 @@ static int veml3235_hw_init(struct iio_dev *indio_dev)
>  }
>  
>  static const struct iio_info veml3235_info = {
> -	.read_raw  = veml3235_read_raw,
> -	.read_avail  = veml3235_read_avail,
> +	.read_raw = veml3235_read_raw,
> +	.read_avail = veml3235_read_avail,

Whilst it would be good to fix that indent, doesn't belong in this patch
as it means the reader has to check very carefully that there are no subtle
changes in this line.  Feel free to send a follow up white space cleanup
patch that clearly states it makes not functional changes though.

>  	.write_raw = veml3235_write_raw,
> +	.write_raw_get_fmt = veml3235_write_raw_get_fmt,
>  };
>  
>  static int veml3235_probe(struct i2c_client *client)
> @@ -493,3 +518,4 @@ module_i2c_driver(veml3235_driver);
>  MODULE_AUTHOR("Javier Carrasco <javier.carrasco.cruz@gmail.com>");
>  MODULE_DESCRIPTION("VEML3235 Ambient Light Sensor");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_GTS_HELPER");
> 

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

[Javier Carrasco]

On Mon Dec 23, 2024 at 12:29 PM CET, Jonathan Cameron wrote:
> On Fri, 20 Dec 2024 20:28:29 +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.
> >
> > Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> Hi Javier,
>
> A few non fix related changes that make no functional difference made
> it in here. Those should be done in a additional patch after this one
> (to make the backport more minimal).
>
> This change is large enough I probably won't directly apply it as a fix anyway.
> Most likely it's material for the next merge window that will then get
> backported after it is upstream.  That will give a small window in which
> the broken code is in a release kernel, but it should hit stable at .2/.3 or
> so and no one sane builds product before that point!
>
> Thanks,
>
> Jonathan
>

I am fine with applying this patch later if you think it makes more
sense. A couple of diffs will go away after dropping the code style
issues and the 'val' check, but it will still be a bit over 200 lines.

I am planning to fix the old veml6030 with a similar approach
after the iio-gts helpers become available. That code has been broken
from the beginning (around 5 years ago), and it did not seem to bother
anyone, so in that case it will make even more sense to apply it in the
merge window.

> > -	ret = regmap_field_write(data->rf.gain, new_gain);
> > -	if (ret) {
> > -		dev_err(data->dev, "failed to set gain: %d\n", ret);
> > +	ret = iio_gts_find_gain_sel_in_times(&data->gts, val, val2, &gain_sel,
> > +					     &time_sel);
> > +	if (ret)
> >  		return ret;
> > +
> > +	if (it_idx != time_sel) {
>
> Not part of this series, but might be worth turning on regcache for this driver.
> Then you can do this sort of write unconditionally as it will hit in the cache
> and do nothing anyway.  Mind you, this isn't a high performance path, so maybe
> just write it anyway.
>

That makes sense, I will add a follow-up patch to use regcache. Even
though it is not a high performance path, doing it right will not cost
much more effort.

> > @@ -309,9 +323,12 @@ static int veml3235_write_raw(struct iio_dev *indio_dev,
> >  {
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_INT_TIME:
> > -		return veml3235_set_it(indio_dev, val, val2);
> > +		if (val)
> > +			return -EINVAL;
>
> This yanking of the test out of the set function is fine, but maybe
> as a precursor patch so as to reduce noise in the main change.
>
> I'm not sure it's technically necessary either. More of a sensible
> cleanup?
>

Actually, it could just stay as it was before by checking val
internally, which drops this diff. I will do that for v2.

> > +
> > +		return veml3235_set_it(indio_dev, val2);
> >  	case IIO_CHAN_INFO_SCALE:
> > -		return veml3235_set_gain(indio_dev, val, val2);
> > +		return veml3235_set_scale(indio_dev, val, val2);
> >  static const struct iio_info veml3235_info = {
> > -	.read_raw  = veml3235_read_raw,
> > -	.read_avail  = veml3235_read_avail,
> > +	.read_raw = veml3235_read_raw,
> > +	.read_avail = veml3235_read_avail,
>
> Whilst it would be good to fix that indent, doesn't belong in this patch
> as it means the reader has to check very carefully that there are no subtle
> changes in this line.  Feel free to send a follow up white space cleanup
> patch that clearly states it makes not functional changes though.
>

I will move the code style fixes to another patch. I was a bit too lazy
here :)

Thanks for your feedback and best regards,
Javier Carrasco