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@xxxxxxxxx>
> ---
> 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
