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

[PATCH v2 0/4] 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>
---
Changes in v2:
- industrialio-gts-helper.c: explicitly return -EINVAL if
  iio_gts_find_new_gain_vy_gain_time_min() fails for the minimum gain
  fallback.
- industrialio-gts-helper.c: check the non-zero value of ret to continue
  to the next iteration in iio_gts_find_gan_sel_in_times().
- veml3235.c: drop code style fixes and move them to a separate patch.
- veml3235.c: add patch to support regmap cache to simplify value
  updates.
- veml3235.c: put 'val' back in the arguments of set_it() to drop the
  diff, and check it internally as before.
- veml3235.c: fix typo in comment.
- Link to v1: https://lore.kernel.org/r/20241220-veml3235_scale-v1-0-b43b190bbb6a@gmail.com

---
Javier Carrasco (4):
      iio: gts-helper: add helpers to ease searches of gain_sel and new_gain
      iio: light: veml3235: fix code style
      iio: light: veml3235: extend regmap to add cache
      iio: veml3235: fix scale to conform to ABI

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

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

[PATCH v2 1/4] 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 | 76 +++++++++++++++++++++++++++++++++++
 include/linux/iio/iio-gts-helper.h    |  5 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 3b5a998150623ba43c2f015c2c5d8a757743b893..7a96dbec45848781008f72338d63d7693abb9076 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -915,6 +915,40 @@ 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)
+			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 +1120,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 -EINVAL;
+		}
+	}
+
+	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 9cb6c80dea716cb80b69bee5277230bafe1c9a95..ae91ad008cc8cbd9d674402f3d4e50385c78e5d1 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 v2 2/4] iio: light: veml3235: fix code style

[Javier Carrasco]

Trivial fixes to drop double spacings.

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

diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
index 66361c3012a3d9f30a79630d51f329dacfb141bc..fa5c7e7dfbfaec1b96428612b1dcba91ea51603f 100644
--- a/drivers/iio/light/veml3235.c
+++ b/drivers/iio/light/veml3235.c
@@ -321,7 +321,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;
@@ -389,8 +389,8 @@ 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,
 };
 

-- 
2.43.0

[PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Javier Carrasco]

The configuration and ID 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.

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

diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
index fa5c7e7dfbfaec1b96428612b1dcba91ea51603f..f754980ea156a6e128ff159b816e09099197c5c7 100644
--- a/drivers/iio/light/veml3235.c
+++ b/drivers/iio/light/veml3235.c
@@ -101,12 +101,43 @@ static const struct iio_chan_spec veml3235_channels[] = {
 	},
 };
 
+static const struct regmap_range veml3235_readable_ranges[] = {
+	regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_ID),
+};
+
+static const struct regmap_access_table veml3235_readable_table = {
+	.yes_ranges = veml3235_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml3235_readable_ranges),
+};
+
+static const struct regmap_range veml3235_writable_ranges[] = {
+	regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_CONF),
+};
+
+static const struct regmap_access_table veml3235_writable_table = {
+	.yes_ranges = veml3235_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml3235_writable_ranges),
+};
+
+static const struct regmap_range veml3235_volatile_ranges[] = {
+	regmap_reg_range(VEML3235_REG_WH_DATA, VEML3235_REG_ALS_DATA),
+};
+
+static const struct regmap_access_table veml3235_volatile_table = {
+	.yes_ranges = veml3235_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml3235_volatile_ranges),
+};
+
 static const struct regmap_config veml3235_regmap_config = {
 	.name = "veml3235_regmap",
 	.reg_bits = 8,
 	.val_bits = 16,
 	.max_register = VEML3235_REG_ID,
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.rd_table = &veml3235_readable_table,
+	.wr_table = &veml3235_writable_table,
+	.volatile_table = &veml3235_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2)

-- 
2.43.0

[PATCH v2 4/4] 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. When at it, use 'scale' instead of 'gain' consistently for
the get/set functions to avoid misunderstandings.

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 | 237 +++++++++++++++++++++++--------------------
 2 files changed, 130 insertions(+), 108 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0cf9cf2a3f944d9f9967b0e02dcb114c116803a4..12864691a7ffa008df5b3384c9caca7ec234577f 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 f754980ea156a6e128ff159b816e09099197c5c7..56e98bf421841bf0082245f69274c7a00c1d5c8a 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
+ * handling 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)
 {
@@ -142,32 +159,17 @@ 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;
@@ -176,78 +178,78 @@ static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2)
 static int veml3235_set_it(struct iio_dev *indio_dev, int val, 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 (val || !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;
+	ret = regmap_field_read(data->rf.it, &it_idx);
+	if (ret)
+		return ret;
 
-	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 = iio_gts_find_gain_sel_in_times(&data->gts, val, val2, &gain_sel,
+					     &time_sel);
+	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 = 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) {
@@ -255,25 +257,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,
@@ -307,7 +309,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;
 	}
@@ -318,17 +320,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;
 	}
@@ -342,7 +354,7 @@ static int veml3235_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_INT_TIME:
 		return veml3235_set_it(indio_dev, val, val2);
 	case IIO_CHAN_INFO_SCALE:
-		return veml3235_set_gain(indio_dev, val, val2);
+		return veml3235_set_scale(indio_dev, val, val2);
 	}
 
 	return -EINVAL;
@@ -402,6 +414,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)
@@ -423,6 +442,7 @@ static const struct iio_info veml3235_info = {
 	.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)
@@ -524,3 +544,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 v2 1/4] iio: gts-helper: add helpers to ease searches of gain_sel and new_gain

[Jonathan Cameron]

On Tue, 24 Dec 2024 11:59:00 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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>
Matti gave an Ack.   If you intentionally dropped it due to significant
changes, you should say so...

> ---

here.

Other than that, looks fine to me.

Jonathan

Re: [PATCH v2 2/4] iio: light: veml3235: fix code style

[Jonathan Cameron]

On Tue, 24 Dec 2024 11:59:01 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Trivial fixes to drop double spacings.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
This stands fine on it's own.

Applied to the togreg branch of iio.git and pushed out as testing
for 0-day to take a look.

Thanks,

Jonathan

> ---
>  drivers/iio/light/veml3235.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
> index 66361c3012a3d9f30a79630d51f329dacfb141bc..fa5c7e7dfbfaec1b96428612b1dcba91ea51603f 100644
> --- a/drivers/iio/light/veml3235.c
> +++ b/drivers/iio/light/veml3235.c
> @@ -321,7 +321,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;
> @@ -389,8 +389,8 @@ 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,
>  };
>  
> 

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Jonathan Cameron]

On Tue, 24 Dec 2024 11:59:02 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The configuration and ID 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.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
This is also fine on it's own.

Applied.

> ---
>  drivers/iio/light/veml3235.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
> index fa5c7e7dfbfaec1b96428612b1dcba91ea51603f..f754980ea156a6e128ff159b816e09099197c5c7 100644
> --- a/drivers/iio/light/veml3235.c
> +++ b/drivers/iio/light/veml3235.c
> @@ -101,12 +101,43 @@ static const struct iio_chan_spec veml3235_channels[] = {
>  	},
>  };
>  
> +static const struct regmap_range veml3235_readable_ranges[] = {
> +	regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_ID),
> +};
> +
> +static const struct regmap_access_table veml3235_readable_table = {
> +	.yes_ranges = veml3235_readable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(veml3235_readable_ranges),
> +};
> +
> +static const struct regmap_range veml3235_writable_ranges[] = {
> +	regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_CONF),
> +};
> +
> +static const struct regmap_access_table veml3235_writable_table = {
> +	.yes_ranges = veml3235_writable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(veml3235_writable_ranges),
> +};
> +
> +static const struct regmap_range veml3235_volatile_ranges[] = {
> +	regmap_reg_range(VEML3235_REG_WH_DATA, VEML3235_REG_ALS_DATA),
> +};
> +
> +static const struct regmap_access_table veml3235_volatile_table = {
> +	.yes_ranges = veml3235_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(veml3235_volatile_ranges),
> +};
> +
>  static const struct regmap_config veml3235_regmap_config = {
>  	.name = "veml3235_regmap",
>  	.reg_bits = 8,
>  	.val_bits = 16,
>  	.max_register = VEML3235_REG_ID,
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.rd_table = &veml3235_readable_table,
> +	.wr_table = &veml3235_writable_table,
> +	.volatile_table = &veml3235_volatile_table,
> +	.cache_type = REGCACHE_RBTREE,
>  };
>  
>  static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2)
> 

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

[Jonathan Cameron]

On Tue, 24 Dec 2024 11:59:03 +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. When at it, use 'scale' instead of 'gain' consistently for
> the get/set functions to avoid misunderstandings.
> 
> Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Hi Javier,

To me this looks fine, but I'd like Matti to take another quick
look at this and patch 1 if time allows given Matti is a lot more
familiar with the GTS helpers than I am.

Jonathan

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

[Matti Vaittinen]

On 24/12/2024 12:59, 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. When at it, use 'scale' instead of 'gain' consistently for
> the get/set functions to avoid misunderstandings.
> 
> Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---

This looks good to me, although I now think we made a mistake with the 
naming of the iio_gts_find_gain_sel_in_times().

The intended use is finding the gain and time (selector) for the new 
scale (while preferring keeping the time unchanged if possible), right?

So, in this regard it'd be better to use name which reflects the fact 
that the function finds gain and time for given scale.

I would now (after having to look the doc of this new function while 
reviewing the code 2 weeks after reviewing this new function :rolleyes:)
name it something like:

iio_gts_find_gain_time_sel_for_scale()

Well, it's not really in the scope of the review anymore, but I'd love 
to see a renaming patch while we have only one user... :)

Anyways:

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

Yours,
	-- Matti

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

[Javier Carrasco]

On Sat Dec 28, 2024 at 4:41 PM CET, Jonathan Cameron wrote:
> On Tue, 24 Dec 2024 11:59:00 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> 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>
> Matti gave an Ack.   If you intentionally dropped it due to significant
> changes, you should say so...
>
> > ---
>
> here.
>
> Other than that, looks fine to me.
>
> Jonathan

Hi Jonathan, you are absolutely right.

I did not add the Ack on purpose because I thought that I had to modify
the helpers functions, but in the end that was not the case. Matti's tag
should have stayed.

Best regards,
Javier Carrasco

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

[Javier Carrasco]

On Sun Dec 29, 2024 at 7:53 AM CET, Matti Vaittinen wrote:
> On 24/12/2024 12:59, 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. When at it, use 'scale' instead of 'gain' consistently for
> > the get/set functions to avoid misunderstandings.
> >
> > Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
>
> This looks good to me, although I now think we made a mistake with the
> naming of the iio_gts_find_gain_sel_in_times().
>
> The intended use is finding the gain and time (selector) for the new
> scale (while preferring keeping the time unchanged if possible), right?
>
> So, in this regard it'd be better to use name which reflects the fact
> that the function finds gain and time for given scale.
>
> I would now (after having to look the doc of this new function while
> reviewing the code 2 weeks after reviewing this new function :rolleyes:)
> name it something like:
>
> iio_gts_find_gain_time_sel_for_scale()
>
> Well, it's not really in the scope of the review anymore, but I'd love
> to see a renaming patch while we have only one user... :)
>
> Anyways:
>
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> Yours,
> 	-- Matti

Hi Matti,

Thank you for your suggestion, I will add it to v3 as this patch and the
one that introduced the helper functions have not been applied yet, so
we don't need an extra patch to rename the function. I will add your
tag too because I will only change what you suggested.

Best regards,
Javier Carrasco

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

[Matti Vaittinen]

On 30/12/2024 12:01, Javier Carrasco wrote:
> On Sun Dec 29, 2024 at 7:53 AM CET, Matti Vaittinen wrote:
>> On 24/12/2024 12:59, 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. When at it, use 'scale' instead of 'gain' consistently for
>>> the get/set functions to avoid misunderstandings.
>>>
>>> Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>> ---
>>
>> This looks good to me, although I now think we made a mistake with the
>> naming of the iio_gts_find_gain_sel_in_times().
>>
>> The intended use is finding the gain and time (selector) for the new
>> scale (while preferring keeping the time unchanged if possible), right?
>>
>> So, in this regard it'd be better to use name which reflects the fact
>> that the function finds gain and time for given scale.
>>
>> I would now (after having to look the doc of this new function while
>> reviewing the code 2 weeks after reviewing this new function :rolleyes:)
>> name it something like:
>>
>> iio_gts_find_gain_time_sel_for_scale()
>>
>> Well, it's not really in the scope of the review anymore, but I'd love
>> to see a renaming patch while we have only one user... :)
>>
>> Anyways:
>>
>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>


> Thank you for your suggestion, I will add it to v3 as this patch and the
> one that introduced the helper functions have not been applied yet, so
> we don't need an extra patch to rename the function.

Great, Thanks!

> I will add your
> tag too because I will only change what you suggested.

Sure!

Yours,
	-- Matti

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Andy Shevchenko]

Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:
> The configuration and ID 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.

...

>  static const struct regmap_config veml3235_regmap_config = {
>  	.name = "veml3235_regmap",
>  	.reg_bits = 8,
>  	.val_bits = 16,
>  	.max_register = VEML3235_REG_ID,
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.rd_table = &veml3235_readable_table,
> +	.wr_table = &veml3235_writable_table,
> +	.volatile_table = &veml3235_volatile_table,
> +	.cache_type = REGCACHE_RBTREE,

Any specific reason why this is NOT a maple tree?

>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Javier Carrasco]

On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote:
> Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:
> > The configuration and ID 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.
>
> ...
>
> >  static const struct regmap_config veml3235_regmap_config = {
> >  	.name = "veml3235_regmap",
> >  	.reg_bits = 8,
> >  	.val_bits = 16,
> >  	.max_register = VEML3235_REG_ID,
> >  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +	.rd_table = &veml3235_readable_table,
> > +	.wr_table = &veml3235_writable_table,
> > +	.volatile_table = &veml3235_volatile_table,
> > +	.cache_type = REGCACHE_RBTREE,
>
> Any specific reason why this is NOT a maple tree?
>

Hello Andy,

I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE),
assuming that the "low-end systems" comment for the different REGCACHE_*
applies well to the typical systems that will make use of this driver,
and many others under IIO. I considered that *possible* performance
advantage for low-end systems above other considerations, like the
general rule about using maple tree.

Thanks and best regards,
Javier Carrasco

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Andy Shevchenko]

On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
>
> On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote:
> > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:
> > > The configuration and ID 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.
> >
> > ...
> >
> > >  static const struct regmap_config veml3235_regmap_config = {
> > >     .name = "veml3235_regmap",
> > >     .reg_bits = 8,
> > >     .val_bits = 16,
> > >     .max_register = VEML3235_REG_ID,
> > >     .val_format_endian = REGMAP_ENDIAN_LITTLE,
> > > +   .rd_table = &veml3235_readable_table,
> > > +   .wr_table = &veml3235_writable_table,
> > > +   .volatile_table = &veml3235_volatile_table,
> > > +   .cache_type = REGCACHE_RBTREE,
> >
> > Any specific reason why this is NOT a maple tree?
>
> Hello Andy,
>
> I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE),

But it's historical and can't be taken as an argument.

> assuming that the "low-end systems" comment for the different REGCACHE_*
> applies well to the typical systems that will make use of this driver,
> and many others under IIO. I considered that *possible* performance
> advantage for low-end systems above other considerations, like the
> general rule about using maple tree.

https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58

"Any new caches
 * should usually use the maple tree cache unless they specifically
 * require that there are never any allocations at runtime and can't
 * provide defaults in which case they should use the flat cache."

Can you reconsider?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Javier Carrasco]

On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote:
> On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco
> <javier.carrasco.cruz@gmail.com> wrote:
> >
> > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote:
> > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:
> > > > The configuration and ID 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.
> > >
> > > ...
> > >
> > > >  static const struct regmap_config veml3235_regmap_config = {
> > > >     .name = "veml3235_regmap",
> > > >     .reg_bits = 8,
> > > >     .val_bits = 16,
> > > >     .max_register = VEML3235_REG_ID,
> > > >     .val_format_endian = REGMAP_ENDIAN_LITTLE,
> > > > +   .rd_table = &veml3235_readable_table,
> > > > +   .wr_table = &veml3235_writable_table,
> > > > +   .volatile_table = &veml3235_volatile_table,
> > > > +   .cache_type = REGCACHE_RBTREE,
> > >
> > > Any specific reason why this is NOT a maple tree?
> >
> > Hello Andy,
> >
> > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE),
>
> But it's historical and can't be taken as an argument.
>
> > assuming that the "low-end systems" comment for the different REGCACHE_*
> > applies well to the typical systems that will make use of this driver,
> > and many others under IIO. I considered that *possible* performance
> > advantage for low-end systems above other considerations, like the
> > general rule about using maple tree.
>
> https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58
>
> "Any new caches
>  * should usually use the maple tree cache unless they specifically
>  * require that there are never any allocations at runtime and can't
>  * provide defaults in which case they should use the flat cache."
>
> Can you reconsider?

That was exactly the comment I referenced, actually the part about
low-end systems that appears right after what you highlighted.

I have nothing against switching to MAPLE, if that is preferred even if
the main user of this driver will be a low-end system. I think that IIO
is a typical subsystem that addresses needs for very low-end systems
that are sometimes slightly more powerful than a microcontroller, but on
the other hand I am by no means an expert, and if MAPLE is the way to go
here as well, I will send a follow-up patch for it.

Thank you for your feedback!

Javier Carrasco

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Andy Shevchenko]

On Sun, Jan 12, 2025 at 6:21 PM Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
> On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote:
> > On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco
> > <javier.carrasco.cruz@gmail.com> wrote:
> > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote:
> > > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:
> > > > > The configuration and ID 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.

...

> > > > > +   .cache_type = REGCACHE_RBTREE,
> > > >
> > > > Any specific reason why this is NOT a maple tree?
> > >
> > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE),
> >
> > But it's historical and can't be taken as an argument.
> >
> > > assuming that the "low-end systems" comment for the different REGCACHE_*
> > > applies well to the typical systems that will make use of this driver,
> > > and many others under IIO. I considered that *possible* performance
> > > advantage for low-end systems above other considerations, like the
> > > general rule about using maple tree.
> >
> > https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58
> >
> > "Any new caches
> >  * should usually use the maple tree cache unless they specifically
> >  * require that there are never any allocations at runtime and can't
> >  * provide defaults in which case they should use the flat cache."
> >
> > Can you reconsider?
>
> That was exactly the comment I referenced, actually the part about
> low-end systems that appears right after what you highlighted.
>
> I have nothing against switching to MAPLE, if that is preferred even if
> the main user of this driver will be a low-end system. I think that IIO
> is a typical subsystem that addresses needs for very low-end systems
> that are sometimes slightly more powerful than a microcontroller, but on
> the other hand I am by no means an expert, and if MAPLE is the way to go
> here as well, I will send a follow-up patch for it.

Ah, I see now. Okay, I leave it then to Jonathan as I am okay with any
choice as long as it's understood and justified.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: light: veml3235: extend regmap to add cache

[Jonathan Cameron]

On Sun, 12 Jan 2025 20:50:41 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jan 12, 2025 at 6:21 PM Javier Carrasco
> <javier.carrasco.cruz@gmail.com> wrote:
> > On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote:  
> > > On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco
> > > <javier.carrasco.cruz@gmail.com> wrote:  
> > > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote:  
> > > > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti:  
> > > > > > The configuration and ID 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.  
> 
> ...
> 
> > > > > > +   .cache_type = REGCACHE_RBTREE,  
> > > > >
> > > > > Any specific reason why this is NOT a maple tree?  
> > > >
> > > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE),  
> > >
> > > But it's historical and can't be taken as an argument.
> > >  
> > > > assuming that the "low-end systems" comment for the different REGCACHE_*
> > > > applies well to the typical systems that will make use of this driver,
> > > > and many others under IIO. I considered that *possible* performance
> > > > advantage for low-end systems above other considerations, like the
> > > > general rule about using maple tree.  
> > >
> > > https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58
> > >
> > > "Any new caches
> > >  * should usually use the maple tree cache unless they specifically
> > >  * require that there are never any allocations at runtime and can't
> > >  * provide defaults in which case they should use the flat cache."
> > >
> > > Can you reconsider?  
> >
> > That was exactly the comment I referenced, actually the part about
> > low-end systems that appears right after what you highlighted.
> >
> > I have nothing against switching to MAPLE, if that is preferred even if
> > the main user of this driver will be a low-end system. I think that IIO
> > is a typical subsystem that addresses needs for very low-end systems
> > that are sometimes slightly more powerful than a microcontroller, but on
> > the other hand I am by no means an expert, and if MAPLE is the way to go
> > here as well, I will send a follow-up patch for it.  
> 
> Ah, I see now. Okay, I leave it then to Jonathan as I am okay with any
> choice as long as it's understood and justified.

For a fairly small regmap, I doubt there is a strong reason to care about the choice.
So stick to what you have.  For future drivers we can reassess as makes sense.

Jonathan

> 
>