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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

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