Message ID | 20250107-veml6030-scale-v1-2-1281e3ad012c@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: light: fix scale in veml6030 | expand |
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
On Tue, 07 Jan 2025 21:50:22 +0100 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The current scale is not ABI-compliant as it is just the sensor gain > instead of the value that acts as a multiplier to be applied to the raw > value (there is no offset). > > Use the iio-gts helpers to obtain the proper scale values according to > the gain and integration time to match the resolution tables from the > datasheet and drop dedicated variables to store the current values of > the integration time, gain and resolution. When at it, use 'scale' > instead of 'gain' consistently for the get/set functions to avoid > misunderstandings. > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> For iio-gts useage please +CC Matti. Matti is far more likely to spot subtle issues around that than other reviewers such as me :) Obviously that depends on Matti having time! To me this looks fine Jonathan
On 07/01/2025 22:50, Javier Carrasco wrote: > The current scale is not ABI-compliant as it is just the sensor gain > instead of the value that acts as a multiplier to be applied to the raw > value (there is no offset). > > Use the iio-gts helpers to obtain the proper scale values according to > the gain and integration time to match the resolution tables from the > datasheet and drop dedicated variables to store the current values of > the integration time, gain and resolution. When at it, use 'scale' > instead of 'gain' consistently for the get/set functions to avoid > misunderstandings. > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Thanks for the patch Javier. And, sorry for a review which is more about questions than suggested improvements. I find it hard to focus on reading code today. > --- > drivers/iio/light/Kconfig | 1 + > drivers/iio/light/veml6030.c | 499 ++++++++++++++++--------------------------- > 2 files changed, 189 insertions(+), 311 deletions(-) > I like the diffstats of this Fix! :) It's nice you found gts-helpers helpful :) ... > + > +static int veml6030_process_als(struct veml6030_data *data, int raw, > + int *val, int *val2) > +{ > + int ret, scale_int, scale_nano; > + u64 tmp; > + > + ret = veml6030_get_scale(data, &scale_int, &scale_nano); > + if (ret < 0) > + return ret; > + > + tmp = (u64)raw * scale_nano; > + *val = raw * scale_int + div_u64(tmp, NANO); > + *val2 = div_u64(do_div(tmp, NANO), MILLI); do_div() is horrible on some platforms. Or, at least it used to be. Is there really no way to go without it? We're dealing with 16bit data and maximum of 512x total gain only, so maybe there was a way(?) Maybe a stupid question (in which case I blame mucus in my head) - could you just divide the raw value by the total gain? > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + ... > > static irqreturn_t veml6030_event_handler(int irq, void *private) > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > int ret, val; > struct veml6030_data *data = iio_priv(indio_dev); > > + ret = devm_iio_init_iio_gts(dev, 2, 150400000, Can you please explain the seemingly odd choice for the max scale? Just a quick look at the sensor spec and this driver allows me to assume following: Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512. If we chose the 'total gain' 1x to match scale 1, then the smallest scale would be 1/512 = 0.001 953 125 This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would not cause precision loss. (Well, I'm not at my sharpest as I've caught cold - but I _think_ this is true, right?) If I read this correctly, the only channel where the scale gets applied is the INTENSITY channel, right? Hence it should be possible to chose the scale as we see best. (Unless the idea of this seemingly odd scale is to maintain the old intensity / scale values in order to not shake userland any more - in which case this could be mentioned). > + veml6030_gain_sel, ARRAY_SIZE(veml6030_gain_sel), > + veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel), > + &data->gts); > + if (ret) > + return dev_err_probe(dev, ret, "failed to init iio gts\n"); > + > ret = veml6030_als_shut_down(data); > if (ret) > return dev_err_probe(dev, ret, "can't shutdown als\n"); > @@ -1119,11 +1010,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > return dev_err_probe(dev, ret, > "can't clear als interrupt status\n"); > > - /* Cache currently active measurement parameters */ > - data->cur_gain = 3; > - data->cur_resolution = 5376; > - data->cur_integration_time = 3; > - > return ret; > } > Yours, -- Matti
On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote: > On 07/01/2025 22:50, Javier Carrasco wrote: > > The current scale is not ABI-compliant as it is just the sensor gain > > instead of the value that acts as a multiplier to be applied to the raw > > value (there is no offset). > > > > Use the iio-gts helpers to obtain the proper scale values according to > > the gain and integration time to match the resolution tables from the > > datasheet and drop dedicated variables to store the current values of > > the integration time, gain and resolution. When at it, use 'scale' > > instead of 'gain' consistently for the get/set functions to avoid > > misunderstandings. > > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > Thanks for the patch Javier. > > And, sorry for a review which is more about questions than suggested > improvements. I find it hard to focus on reading code today. > > > --- > > drivers/iio/light/Kconfig | 1 + > > drivers/iio/light/veml6030.c | 499 ++++++++++++++++--------------------------- > > 2 files changed, 189 insertions(+), 311 deletions(-) > > > > I like the diffstats of this Fix! :) It's nice you found gts-helpers > helpful :) I wonder how painful the int. time/gain/scale issue in ALS was before iio-gts existed :D > > ... > > > + > > +static int veml6030_process_als(struct veml6030_data *data, int raw, > > + int *val, int *val2) > > +{ > > + int ret, scale_int, scale_nano; > > + u64 tmp; > > + > > + ret = veml6030_get_scale(data, &scale_int, &scale_nano); > > + if (ret < 0) > > + return ret; > > + > > + tmp = (u64)raw * scale_nano; > > + *val = raw * scale_int + div_u64(tmp, NANO); > > + *val2 = div_u64(do_div(tmp, NANO), MILLI); > > do_div() is horrible on some platforms. Or, at least it used to be. Is > there really no way to go without it? We're dealing with 16bit data and > maximum of 512x total gain only, so maybe there was a way(?) > > Maybe a stupid question (in which case I blame mucus in my head) - could > you just divide the raw value by the total gain? > In its current form we need the 64-bit operations to handle the scale, and it will probably have to stay like that for the reasons I give you below. > ... > > > > > static irqreturn_t veml6030_event_handler(int irq, void *private) > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > > int ret, val; > > struct veml6030_data *data = iio_priv(indio_dev); > > > > + ret = devm_iio_init_iio_gts(dev, 2, 150400000, > > Can you please explain the seemingly odd choice for the max scale? > > Just a quick look at the sensor spec and this driver allows me to assume > following: > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512. > > If we chose the 'total gain' 1x to match scale 1, then the smallest > scale would be 1/512 = 0.001 953 125 > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would > not cause precision loss. (Well, I'm not at my sharpest as I've caught > cold - but I _think_ this is true, right?) > > If I read this correctly, the only channel where the scale gets applied > is the INTENSITY channel, right? Hence it should be possible to chose > the scale as we see best. (Unless the idea of this seemingly odd scale > is to maintain the old intensity / scale values in order to not shake > userland any more - in which case this could be mentioned). > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY, which forces us to provide the scale as a value that multiplied by the raw measurement gives a result in lux. The maximum scale in that case, as provided by the application note [1] (page 5, RESOLUTION AND MAXIMUM DETECTION RANGE table) is 2.1504 to convert from cnt to lux. The same applies for the rest of the device supported by this driver (veml6035 and veml6035). > > Yours, > -- Matti Thank you for your feedback, I hope my reply could answer your questions. If something is still not clear or simply wrong, please let me know. Best regards, Javier Carrasco Link to app note: https://www.vishay.com/docs/84367/designingveml6030.pdf [1]
ma 13.1.2025 klo 17.05 Javier Carrasco (javier.carrasco.cruz@gmail.com) kirjoitti: > > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote: > > On 07/01/2025 22:50, Javier Carrasco wrote: > > > The current scale is not ABI-compliant as it is just the sensor gain > > > instead of the value that acts as a multiplier to be applied to the raw > > > value (there is no offset). > > > > > > Use the iio-gts helpers to obtain the proper scale values according to > > > the gain and integration time to match the resolution tables from the > > > datasheet and drop dedicated variables to store the current values of > > > the integration time, gain and resolution. When at it, use 'scale' > > > instead of 'gain' consistently for the get/set functions to avoid > > > misunderstandings. > > > > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > > > Thanks for the patch Javier. > > > > And, sorry for a review which is more about questions than suggested > > improvements. I find it hard to focus on reading code today. > > > > > --- > > > drivers/iio/light/Kconfig | 1 + > > > drivers/iio/light/veml6030.c | 499 ++++++++++++++++--------------------------- > > > 2 files changed, 189 insertions(+), 311 deletions(-) > > > > > > > I like the diffstats of this Fix! :) It's nice you found gts-helpers > > helpful :) > > I wonder how painful the int. time/gain/scale issue in ALS was before > iio-gts existed :D > I don't really know. I wrote the gts-helpers as soon as I wrote my first light sensor driver :) > > ... > > > > > + > > > +static int veml6030_process_als(struct veml6030_data *data, int raw, > > > + int *val, int *val2) > > > +{ > > > + int ret, scale_int, scale_nano; > > > + u64 tmp; > > > + > > > + ret = veml6030_get_scale(data, &scale_int, &scale_nano); > > > + if (ret < 0) > > > + return ret; > > > + > > > + tmp = (u64)raw * scale_nano; > > > + *val = raw * scale_int + div_u64(tmp, NANO); > > > + *val2 = div_u64(do_div(tmp, NANO), MILLI); > > > > do_div() is horrible on some platforms. Or, at least it used to be. Is > > there really no way to go without it? We're dealing with 16bit data and > > maximum of 512x total gain only, so maybe there was a way(?) > > > > Maybe a stupid question (in which case I blame mucus in my head) - could > > you just divide the raw value by the total gain? > > > > In its current form we need the 64-bit operations to handle the scale, > and it will probably have to stay like that for the reasons I give you > below. Still, I wonder if multiplying by scale really differs from dividing by the total gain? I think the scale is inversely proportional to the total gain, right? > > > static irqreturn_t veml6030_event_handler(int irq, void *private) > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > > > int ret, val; > > > struct veml6030_data *data = iio_priv(indio_dev); > > > > > > + ret = devm_iio_init_iio_gts(dev, 2, 150400000, > > > > Can you please explain the seemingly odd choice for the max scale? > > > > Just a quick look at the sensor spec and this driver allows me to assume > > following: > > > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512. > > > > If we chose the 'total gain' 1x to match scale 1, then the smallest > > scale would be 1/512 = 0.001 953 125 > > > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would > > not cause precision loss. (Well, I'm not at my sharpest as I've caught > > cold - but I _think_ this is true, right?) > > > > If I read this correctly, the only channel where the scale gets applied > > is the INTENSITY channel, right? Hence it should be possible to chose > > the scale as we see best. (Unless the idea of this seemingly odd scale > > is to maintain the old intensity / scale values in order to not shake > > userland any more - in which case this could be mentioned). > > > > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY, Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale shouldn't be applied to it. (Driver may still apply scale internally, but users should not see it, right? And if the driver does it only internally, then the driver can also multiply values using (N * scale). Well, I suppose you can as well use this "fitted MAX SCALE" - but maybe it warrants a comment. > which forces us to provide the scale as a value that multiplied by the > raw measurement gives a result in lux. The maximum scale in that case, > as provided by the application note [1] (page 5, RESOLUTION AND MAXIMUM > DETECTION RANGE table) is 2.1504 to convert from cnt to lux. > > The same applies for the rest of the device supported by this driver > (veml6035 and veml6035). > Yours, -- Matti
On Mon Jan 13, 2025 at 8:52 PM CET, Matti Vaittinen wrote: > ma 13.1.2025 klo 17.05 Javier Carrasco > (javier.carrasco.cruz@gmail.com) kirjoitti: > > > > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote: > > > On 07/01/2025 22:50, Javier Carrasco wrote: > > > > The current scale is not ABI-compliant as it is just the sensor gain > > > > instead of the value that acts as a multiplier to be applied to the raw > > > > value (there is no offset). > > > > > > > > Use the iio-gts helpers to obtain the proper scale values according to > > > > the gain and integration time to match the resolution tables from the > > > > datasheet and drop dedicated variables to store the current values of > > > > the integration time, gain and resolution. When at it, use 'scale' > > > > instead of 'gain' consistently for the get/set functions to avoid > > > > misunderstandings. > > > > > > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > > > > > Thanks for the patch Javier. > > > > > > And, sorry for a review which is more about questions than suggested > > > improvements. I find it hard to focus on reading code today. > > > > > > > --- > > > > drivers/iio/light/Kconfig | 1 + > > > > drivers/iio/light/veml6030.c | 499 ++++++++++++++++--------------------------- > > > > 2 files changed, 189 insertions(+), 311 deletions(-) > > > > > > > > > > I like the diffstats of this Fix! :) It's nice you found gts-helpers > > > helpful :) > > > > I wonder how painful the int. time/gain/scale issue in ALS was before > > iio-gts existed :D > > > I don't really know. I wrote the gts-helpers as soon as I wrote my > first light sensor driver :) > > > > ... > > > > > > > + > > > > +static int veml6030_process_als(struct veml6030_data *data, int raw, > > > > + int *val, int *val2) > > > > +{ > > > > + int ret, scale_int, scale_nano; > > > > + u64 tmp; > > > > + > > > > + ret = veml6030_get_scale(data, &scale_int, &scale_nano); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + tmp = (u64)raw * scale_nano; > > > > + *val = raw * scale_int + div_u64(tmp, NANO); > > > > + *val2 = div_u64(do_div(tmp, NANO), MILLI); > > > > > > do_div() is horrible on some platforms. Or, at least it used to be. Is > > > there really no way to go without it? We're dealing with 16bit data and > > > maximum of 512x total gain only, so maybe there was a way(?) > > > > > > Maybe a stupid question (in which case I blame mucus in my head) - could > > > you just divide the raw value by the total gain? > > > > > > > In its current form we need the 64-bit operations to handle the scale, > > and it will probably have to stay like that for the reasons I give you > > below. > > Still, I wonder if multiplying by scale really differs from dividing > by the total gain? I think the scale is inversely proportional to the > total gain, right? > I am sorry, but I am not sure if I got your point here. The scale is the multiplier to get lux from raw, and for example it's not just 1/512 for the maximum total gain, as that is not taking the intrinsic resolution of the sensor. Maybe I am misunderstanding something, but I don't see the way around raw * scale with the scale being one of the discrete values provided in the application note. I will give you a simple example, so you can tell me where my reasoning fails: raw = 100 counts scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) The reply to your comment below explains why we have a PROCESSED IIO_LIGHT in the first place. > > > > static irqreturn_t veml6030_event_handler(int irq, void *private) > > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > > > > int ret, val; > > > > struct veml6030_data *data = iio_priv(indio_dev); > > > > > > > > + ret = devm_iio_init_iio_gts(dev, 2, 150400000, > > > > > > Can you please explain the seemingly odd choice for the max scale? > > > > > > Just a quick look at the sensor spec and this driver allows me to assume > > > following: > > > > > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512. > > > > > > If we chose the 'total gain' 1x to match scale 1, then the smallest > > > scale would be 1/512 = 0.001 953 125 > > > > > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would > > > not cause precision loss. (Well, I'm not at my sharpest as I've caught > > > cold - but I _think_ this is true, right?) > > > > > > If I read this correctly, the only channel where the scale gets applied > > > is the INTENSITY channel, right? Hence it should be possible to chose > > > the scale as we see best. (Unless the idea of this seemingly odd scale > > > is to maintain the old intensity / scale values in order to not shake > > > userland any more - in which case this could be mentioned). > > > > > > > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY, > > Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale > shouldn't be applied to it. (Driver may still apply scale internally, > but users should not see it, right? And if the driver does it only > internally, then the driver can also multiply values using (N * > scale). Well, I suppose you can as well use this "fitted MAX SCALE" - > but maybe it warrants a comment. IIO_LIGHT provides RAW and PROCESSED values, which shouldn't have happened in the first place as PROCESSED is just raw * scale, if scale had been right from the beginning. Actually, when I took over this driver, I was tempted to drop the PROCESSED value, but it was too late for that without breaking ABI. My guess is that the processed value was provided because in_illuminance_scale was not the right multiplier, only the gain. Note that in_illuminance_scale is also provided to the user, and that must comply with the ABI definitions. Thank you again, Javier Carrasco
ti 14.1.2025 klo 0.32 Javier Carrasco (javier.carrasco.cruz@gmail.com) kirjoitti: > > On Mon Jan 13, 2025 at 8:52 PM CET, Matti Vaittinen wrote: > > ma 13.1.2025 klo 17.05 Javier Carrasco > > (javier.carrasco.cruz@gmail.com) kirjoitti: > > > > > > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote: > > > > On 07/01/2025 22:50, Javier Carrasco wrote: > > > > > The current scale is not ABI-compliant as it is just the sensor gain > > > > > instead of the value that acts as a multiplier to be applied to the raw > > > > > value (there is no offset). > > > > > > > > > > Use the iio-gts helpers to obtain the proper scale values according to > > > > > the gain and integration time to match the resolution tables from the > > > > > datasheet and drop dedicated variables to store the current values of > > > > > the integration time, gain and resolution. When at it, use 'scale' > > > > > instead of 'gain' consistently for the get/set functions to avoid > > > > > misunderstandings. > > > > > > > > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor") > > > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > > > > > > > Thanks for the patch Javier. > > > > > > > > And, sorry for a review which is more about questions than suggested > > > > improvements. I find it hard to focus on reading code today. > > > > > > > > > --- > > > > > drivers/iio/light/Kconfig | 1 + > > > > > drivers/iio/light/veml6030.c | 499 ++++++++++++++++--------------------------- > > > > > 2 files changed, 189 insertions(+), 311 deletions(-) > > > > ... > > > > > > > > > + > > > > > +static int veml6030_process_als(struct veml6030_data *data, int raw, > > > > > + int *val, int *val2) > > > > > +{ > > > > > + int ret, scale_int, scale_nano; > > > > > + u64 tmp; > > > > > + > > > > > + ret = veml6030_get_scale(data, &scale_int, &scale_nano); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + tmp = (u64)raw * scale_nano; > > > > > + *val = raw * scale_int + div_u64(tmp, NANO); > > > > > + *val2 = div_u64(do_div(tmp, NANO), MILLI); > > > > > > > > do_div() is horrible on some platforms. Or, at least it used to be. Is > > > > there really no way to go without it? We're dealing with 16bit data and > > > > maximum of 512x total gain only, so maybe there was a way(?) > > > > > > > > Maybe a stupid question (in which case I blame mucus in my head) - could > > > > you just divide the raw value by the total gain? > > > > > > > > > > In its current form we need the 64-bit operations to handle the scale, > > > and it will probably have to stay like that for the reasons I give you > > > below. > > > > Still, I wonder if multiplying by scale really differs from dividing > > by the total gain? I think the scale is inversely proportional to the > > total gain, right? > > > I am sorry, but I am not sure if I got your point here. The scale is the > multiplier to get lux from raw, and for example it's not just 1/512 for > the maximum total gain, as that is not taking the intrinsic resolution > of the sensor. Maybe I am misunderstanding something, but I don't see the > way around raw * scale with the scale being one of the discrete values > provided in the application note. > > I will give you a simple example, so you can tell me where my reasoning > fails: > > raw = 100 counts > scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) > processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) > > The reply to your comment below explains why we have a PROCESSED > IIO_LIGHT in the first place. > > > > > > static irqreturn_t veml6030_event_handler(int irq, void *private) > > > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) > > > > > int ret, val; > > > > > struct veml6030_data *data = iio_priv(indio_dev); > > > > > > > > > > + ret = devm_iio_init_iio_gts(dev, 2, 150400000, > > > > > > > > Can you please explain the seemingly odd choice for the max scale? > > > > > > > > Just a quick look at the sensor spec and this driver allows me to assume > > > > following: > > > > > > > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512. > > > > > > > > If we chose the 'total gain' 1x to match scale 1, then the smallest > > > > scale would be 1/512 = 0.001 953 125 > > > > > > > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would > > > > not cause precision loss. (Well, I'm not at my sharpest as I've caught > > > > cold - but I _think_ this is true, right?) > > > > > > > > If I read this correctly, the only channel where the scale gets applied > > > > is the INTENSITY channel, right? Hence it should be possible to chose > > > > the scale as we see best. (Unless the idea of this seemingly odd scale > > > > is to maintain the old intensity / scale values in order to not shake > > > > userland any more - in which case this could be mentioned). > > > > > > > > > > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY, > > > > Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale > > shouldn't be applied to it. (Driver may still apply scale internally, > > but users should not see it, right? And if the driver does it only > > internally, then the driver can also multiply values using (N * > > scale). Well, I suppose you can as well use this "fitted MAX SCALE" - > > but maybe it warrants a comment. > > IIO_LIGHT provides RAW and PROCESSED values, Thanks. This explains why you chose the MAX SCALE in IIo init to be this oddish looking value :) > which shouldn't have > happened in the first place as PROCESSED is just raw * scale, if scale > had been right from the beginning. Actually, when I took over this > driver, I was tempted to drop the PROCESSED value, but it was too late > for that without breaking ABI. My guess is that the processed value was > provided because in_illuminance_scale was not the right multiplier, only > the gain. > Note that in_illuminance_scale is also provided to the user, and that > must comply with the ABI definitions. Sure. Raw * scale must be lux. You still could use some driver internal scale multiplier which you applied before pushing the lux values to users, but this would make the available scales handling more complicated. Hence, I now fully agree with you using the 2, 150400000, in devm_iio_init_iio_gts() as max scale. Thank you for the patience :) I copied part of your reply below as I think this is the right order of discussion: > I will give you a simple example, so you can tell me where my reasoning > fails: > > raw = 100 counts > scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) > processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) Your reasoning does not fail. But, the scale = 1 / (N * total_gain), right? (N here depends on how we choose the scale/gain values) Here, the total_gain means the effect of both the hardware_gain and the integration time. Hence, processed = X * (raw * scale) => processed = X * (raw * (1 / (N * total_gain)) => processed = X * raw / (N * total_gain); Hence I thought you might be able to get rid of this 64bit division by using the total_gain from the iio_gts_get_total_gain() instead of using the scale. Or, am I missing something? Yours, -- Matti
On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote: ... > > I will give you a simple example, so you can tell me where my reasoning > > fails: > > > > raw = 100 counts > > scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) > > processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) > > Your reasoning does not fail. But, the scale = 1 / (N * total_gain), > right? (N here depends on how we choose the scale/gain values) Here, > the total_gain means the effect of both the hardware_gain and the > integration time. > > Hence, > processed = X * (raw * scale) > > => processed = X * (raw * (1 / (N * total_gain)) > => processed = X * raw / (N * total_gain); > > Hence I thought you might be able to get rid of this 64bit division by > using the total_gain from the iio_gts_get_total_gain() instead of > using the scale. Or, am I missing something? > I am not sure by X you mean the maximum resolution, but if that is the case, the following would work (pseudo-code): /* Maximum resolution (2.1504 lux/count) * 10000 */ #define VEML6030_MAX_RES 21504 total_gain = iio_gts_get_total_gain(); processed_int = raw * VEML6030_MAX_RES / total_gain / 10000; processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100; return INT_PLUS_MICRO; Is that what you meant? For my previous example (100 counts, IT=25ms, GAIN=1/8 → total_gain = 1 * 1): processed_int = 100 * 21504 / 1 / 10000; (215) processed_micro = 100 * 21504 / 1 % 10000 * 100; (40000) The expected value was 215.04 lux For IT=800ms, GAIN=2 → total_gain = 32 * 16 = 512 processed_int = 100 * 21504 / 512 / 10000; (0) processed_micro = 100 * 21504 / 512 % 10000 * 100; (420000) That is also the expected value: 0.42 lux Given that the driver supports multiple devices with different maximum scales (currently 2), it will have to be added to the chip data. If we are now on the same page, I will implement it like that to drop 64-bit divisions. Thanks again! Best regards, Javier Carrasco
On 14/01/2025 15:02, Javier Carrasco wrote: > On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote: > ... >>> I will give you a simple example, so you can tell me where my reasoning >>> fails: >>> >>> raw = 100 counts >>> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) >>> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) >> >> Your reasoning does not fail. But, the scale = 1 / (N * total_gain), >> right? (N here depends on how we choose the scale/gain values) Here, >> the total_gain means the effect of both the hardware_gain and the >> integration time. >> >> Hence, >> processed = X * (raw * scale) >> >> => processed = X * (raw * (1 / (N * total_gain)) >> => processed = X * raw / (N * total_gain); >> >> Hence I thought you might be able to get rid of this 64bit division by >> using the total_gain from the iio_gts_get_total_gain() instead of >> using the scale. Or, am I missing something? >> > > I am not sure by X you mean the maximum resolution, but if that is the > case, the following would work (pseudo-code): Yes. X denoted the value by which the count needs to be multiplied to get the lux (when total gain "in the terms of gts" is x1. I think in this particular case the "gain is x1" is a bit confusing as it appears this really means the hardware gain is 1/8, right?). Anyways, lux/count it is, so in short - yes. :) > > /* Maximum resolution (2.1504 lux/count) * 10000 */ > #define VEML6030_MAX_RES 21504 > > total_gain = iio_gts_get_total_gain(); > processed_int = raw * VEML6030_MAX_RES / total_gain / 10000; Yes. This is what I was thinking of. > processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100; gah. I didn't consider representing the micro portion. Staring this makes me feel dizzy :) Well, it looks correct, and I guess the precision is not lost by the division(?) But yes, you did perfectly get what I was after! Jonathan, do you think I am just guiding Javier to make a mess? :) If not, then this might be the way to go. Yours, -- Matti
On Tue, 14 Jan 2025 16:26:16 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 14/01/2025 15:02, Javier Carrasco wrote: > > On Tue Jan 14, 2025 at 7:43 AM CET, Matti Vaittinen wrote: > > ... > >>> I will give you a simple example, so you can tell me where my reasoning > >>> fails: > >>> > >>> raw = 100 counts > >>> scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8) > >>> processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT) > >> > >> Your reasoning does not fail. But, the scale = 1 / (N * total_gain), > >> right? (N here depends on how we choose the scale/gain values) Here, > >> the total_gain means the effect of both the hardware_gain and the > >> integration time. > >> > >> Hence, > >> processed = X * (raw * scale) > >> > >> => processed = X * (raw * (1 / (N * total_gain)) > >> => processed = X * raw / (N * total_gain); > >> > >> Hence I thought you might be able to get rid of this 64bit division by > >> using the total_gain from the iio_gts_get_total_gain() instead of > >> using the scale. Or, am I missing something? > >> > > > > I am not sure by X you mean the maximum resolution, but if that is the > > case, the following would work (pseudo-code): > > Yes. X denoted the value by which the count needs to be multiplied to > get the lux (when total gain "in the terms of gts" is x1. I think in > this particular case the "gain is x1" is a bit confusing as it appears > this really means the hardware gain is 1/8, right?). Anyways, lux/count > it is, so in short - yes. :) > > > > > /* Maximum resolution (2.1504 lux/count) * 10000 */ > > #define VEML6030_MAX_RES 21504 > > > > total_gain = iio_gts_get_total_gain(); > > processed_int = raw * VEML6030_MAX_RES / total_gain / 10000; > > Yes. This is what I was thinking of. > > > processed_micro = ((raw * VEML6030_MAX_RES / total_gain) % 10000) * 100; > > gah. I didn't consider representing the micro portion. Staring this > makes me feel dizzy :) Well, it looks correct, and I guess the precision > is not lost by the division(?) But yes, you did perfectly get what I was > after! > > Jonathan, do you think I am just guiding Javier to make a mess? :) This is an area you've thought about a lot more than me. Whether it is worth avoiding the 64 bit maths is an interesting question and I guess depends where this part is typically showing up. Jonathan > > If not, then this might be the way to go. > > Yours, > -- Matti
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index e34e551eef3e8db006de56724ce3873c07b3360a..eb7f56eaeae07c8b021dc7c0db87f46b44ed44d7 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -683,6 +683,7 @@ config VEML6030 select REGMAP_I2C select IIO_BUFFER select IIO_TRIGGERED_BUFFER + select IIO_GTS_HELPER depends on I2C help Say Y here if you want to build a driver for the Vishay VEML6030 diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c index a6385c6d3fba59a6b22845a3c5e252b619faed65..99c7e073ea664f815a7b1c1bc829a8eff66fd323 100644 --- a/drivers/iio/light/veml6030.c +++ b/drivers/iio/light/veml6030.c @@ -24,10 +24,12 @@ #include <linux/regmap.h> #include <linux/interrupt.h> #include <linux/pm_runtime.h> +#include <linux/units.h> #include <linux/regulator/consumer.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/iio/events.h> +#include <linux/iio/iio-gts-helper.h> #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> @@ -72,14 +74,10 @@ struct veml6030_rf { struct veml603x_chip { const char *name; - const int(*scale_vals)[][2]; - const int num_scale_vals; const struct iio_chan_spec *channels; const int num_channels; int (*hw_init)(struct iio_dev *indio_dev, struct device *dev); int (*set_info)(struct iio_dev *indio_dev); - int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2); - int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2); int (*regfield_init)(struct iio_dev *indio_dev); }; @@ -98,40 +96,55 @@ struct veml6030_data { struct i2c_client *client; struct regmap *regmap; struct veml6030_rf rf; - int cur_resolution; - int cur_gain; - int cur_integration_time; const struct veml603x_chip *chip; + struct iio_gts gts; + }; -static const int veml6030_it_times[][2] = { - { 0, 25000 }, - { 0, 50000 }, - { 0, 100000 }, - { 0, 200000 }, - { 0, 400000 }, - { 0, 800000 }, +#define VEML6030_SEL_IT_25MS 0x0C +#define VEML6030_SEL_IT_50MS 0x08 +#define VEML6030_SEL_IT_100MS 0x00 +#define VEML6030_SEL_IT_200MS 0x01 +#define VEML6030_SEL_IT_400MS 0x02 +#define VEML6030_SEL_IT_800MS 0x03 +static const struct iio_itime_sel_mul veml6030_it_sel[] = { + GAIN_SCALE_ITIME_US(25000, VEML6030_SEL_IT_25MS, 1), + GAIN_SCALE_ITIME_US(50000, VEML6030_SEL_IT_50MS, 2), + GAIN_SCALE_ITIME_US(100000, VEML6030_SEL_IT_100MS, 4), + GAIN_SCALE_ITIME_US(200000, VEML6030_SEL_IT_200MS, 8), + GAIN_SCALE_ITIME_US(400000, VEML6030_SEL_IT_400MS, 16), + GAIN_SCALE_ITIME_US(800000, VEML6030_SEL_IT_800MS, 32), }; -/* - * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is - * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1, - * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4. +/* Gains are multiplied by 8 to work with integers. The values in the + * iio-gts tables don't need corrections because the maximum value of + * the scale refers to GAIN = x1, and the rest of the values are + * obtained from the resulting linear function. */ -static const int veml6030_scale_vals[][2] = { - { 0, 125000 }, - { 0, 250000 }, - { 1, 0 }, - { 2, 0 }, +#define VEML6030_SEL_GAIN_X125 2 +#define VEML6030_SEL_GAIN_X250 3 +#define VEML6030_SEL_GAIN_X1000 0 +#define VEML6030_SEL_GAIN_X2000 1 +static const struct iio_gain_sel_pair veml6030_gain_sel[] = { + GAIN_SCALE_GAIN(1, VEML6030_SEL_GAIN_X125), + GAIN_SCALE_GAIN(2, VEML6030_SEL_GAIN_X250), + GAIN_SCALE_GAIN(8, VEML6030_SEL_GAIN_X1000), + GAIN_SCALE_GAIN(16, VEML6030_SEL_GAIN_X2000), }; -static const int veml6035_scale_vals[][2] = { - { 0, 125000 }, - { 0, 250000 }, - { 0, 500000 }, - { 1, 0 }, - { 2, 0 }, - { 4, 0 }, +#define VEML6035_SEL_GAIN_X125 4 +#define VEML6035_SEL_GAIN_X250 5 +#define VEML6035_SEL_GAIN_X500 7 +#define VEML6035_SEL_GAIN_X1000 0 +#define VEML6035_SEL_GAIN_X2000 1 +#define VEML6035_SEL_GAIN_X4000 3 +static const struct iio_gain_sel_pair veml6035_gain_sel[] = { + GAIN_SCALE_GAIN(1, VEML6035_SEL_GAIN_X125), + GAIN_SCALE_GAIN(2, VEML6035_SEL_GAIN_X250), + GAIN_SCALE_GAIN(4, VEML6035_SEL_GAIN_X500), + GAIN_SCALE_GAIN(8, VEML6035_SEL_GAIN_X1000), + GAIN_SCALE_GAIN(16, VEML6035_SEL_GAIN_X2000), + GAIN_SCALE_GAIN(32, VEML6035_SEL_GAIN_X4000), }; /* @@ -365,104 +378,73 @@ static const struct regmap_config veml6030_regmap_config = { .cache_type = REGCACHE_RBTREE, }; -static int veml6030_get_intgrn_tm(struct iio_dev *indio_dev, - int *val, int *val2) +static int veml6030_get_it(struct veml6030_data *data, int *val, int *val2) { - int it_idx, ret; - struct veml6030_data *data = iio_priv(indio_dev); + int ret, it_idx; ret = regmap_field_read(data->rf.it, &it_idx); - if (ret) { - dev_err(&data->client->dev, - "can't read als conf register %d\n", ret); + if (ret) return ret; - } - switch (it_idx) { - case 0: - *val2 = 100000; - break; - case 1: - *val2 = 200000; - break; - case 2: - *val2 = 400000; - break; - case 3: - *val2 = 800000; - break; - case 8: - *val2 = 50000; - break; - case 12: - *val2 = 25000; - break; - default: - return -EINVAL; - } + ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx); + if (ret < 0) + return ret; + *val2 = ret; *val = 0; + return IIO_VAL_INT_PLUS_MICRO; } -static int veml6030_set_intgrn_tm(struct iio_dev *indio_dev, - int val, int val2) +static int veml6030_set_it(struct iio_dev *indio_dev, int val, int val2) { - int ret, new_int_time, int_idx; struct veml6030_data *data = iio_priv(indio_dev); + int ret, gain_idx, it_idx, new_gain, prev_gain, prev_it; + bool in_range; - if (val) + if (val || !iio_gts_valid_time(&data->gts, val2)) return -EINVAL; - switch (val2) { - case 25000: - new_int_time = 0x300; - int_idx = 5; - break; - case 50000: - new_int_time = 0x200; - int_idx = 4; - break; - case 100000: - new_int_time = 0x00; - int_idx = 3; - break; - case 200000: - new_int_time = 0x40; - int_idx = 2; - break; - case 400000: - new_int_time = 0x80; - int_idx = 1; - break; - case 800000: - new_int_time = 0xC0; - int_idx = 0; - break; - default: - return -EINVAL; - } + ret = regmap_field_read(data->rf.it, &it_idx); + if (ret) + return ret; - ret = regmap_field_write(data->rf.it, new_int_time); - if (ret) { - dev_err(&data->client->dev, - "can't update als integration time %d\n", ret); + ret = regmap_field_read(data->rf.gain, &gain_idx); + if (ret) return ret; - } - /* - * Cache current integration time and update resolution. For every - * increase in integration time to next level, resolution is halved - * and vice-versa. - */ - if (data->cur_integration_time < int_idx) - data->cur_resolution <<= int_idx - data->cur_integration_time; - else if (data->cur_integration_time > int_idx) - data->cur_resolution >>= data->cur_integration_time - int_idx; + prev_it = iio_gts_find_int_time_by_sel(&data->gts, it_idx); + if (prev_it < 0) + return prev_it; - data->cur_integration_time = int_idx; + if (prev_it == val2) + return 0; - return ret; + prev_gain = iio_gts_find_gain_by_sel(&data->gts, gain_idx); + if (prev_gain < 0) + return prev_gain; + + ret = iio_gts_find_new_gain_by_gain_time_min(&data->gts, prev_gain, prev_it, + val2, &new_gain, &in_range); + if (ret) + return ret; + + if (!in_range) + dev_dbg(&data->client->dev, "Optimal gain out of range\n"); + + ret = iio_gts_find_sel_by_int_time(&data->gts, val2); + if (ret < 0) + return ret; + + ret = regmap_field_write(data->rf.it, ret); + if (ret) + return ret; + + ret = iio_gts_find_sel_by_gain(&data->gts, new_gain); + if (ret < 0) + return ret; + + return regmap_field_write(data->rf.gain, ret); } static int veml6030_read_persistence(struct iio_dev *indio_dev, @@ -471,7 +453,7 @@ static int veml6030_read_persistence(struct iio_dev *indio_dev, int ret, reg, period, x, y; struct veml6030_data *data = iio_priv(indio_dev); - ret = veml6030_get_intgrn_tm(indio_dev, &x, &y); + ret = veml6030_get_it(data, &x, &y); if (ret < 0) return ret; @@ -496,7 +478,7 @@ static int veml6030_write_persistence(struct iio_dev *indio_dev, int ret, period, x, y; struct veml6030_data *data = iio_priv(indio_dev); - ret = veml6030_get_intgrn_tm(indio_dev, &x, &y); + ret = veml6030_get_it(data, &x, &y); if (ret < 0) return ret; @@ -525,177 +507,29 @@ static int veml6030_write_persistence(struct iio_dev *indio_dev, return ret; } -/* - * Cache currently set gain & update resolution. For every - * increase in the gain to next level, resolution is halved - * and vice-versa. - */ -static void veml6030_update_gain_res(struct veml6030_data *data, int gain_idx) +static int veml6030_set_scale(struct iio_dev *indio_dev, int val, int val2) { - if (data->cur_gain < gain_idx) - data->cur_resolution <<= gain_idx - data->cur_gain; - else if (data->cur_gain > gain_idx) - data->cur_resolution >>= data->cur_gain - gain_idx; - - data->cur_gain = gain_idx; -} - -static int veml6030_set_als_gain(struct iio_dev *indio_dev, - int val, int val2) -{ - int ret, new_gain, gain_idx; + int ret, gain_sel, it_idx, it_sel; struct veml6030_data *data = iio_priv(indio_dev); - if (val == 0 && val2 == 125000) { - new_gain = 0x01; - gain_idx = 3; - } else if (val == 0 && val2 == 250000) { - new_gain = 0x11; - gain_idx = 2; - } else if (val == 1 && val2 == 0) { - new_gain = 0x00; - gain_idx = 1; - } else if (val == 2 && val2 == 0) { - new_gain = 0x01; - gain_idx = 0; - } else { - return -EINVAL; - } - - ret = regmap_field_write(data->rf.gain, new_gain); - if (ret) { - dev_err(&data->client->dev, - "can't set als gain %d\n", ret); + ret = regmap_field_read(data->rf.it, &it_idx); + if (ret) return ret; - } - veml6030_update_gain_res(data, gain_idx); - - return 0; -} - -static int veml6035_set_als_gain(struct iio_dev *indio_dev, int val, int val2) -{ - int ret, new_gain, gain_idx; - struct veml6030_data *data = iio_priv(indio_dev); - - if (val == 0 && val2 == 125000) { - new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS); - gain_idx = 5; - } else if (val == 0 && val2 == 250000) { - new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS | - VEML6035_GAIN); - gain_idx = 4; - } else if (val == 0 && val2 == 500000) { - new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_SENS | - VEML6035_GAIN | VEML6035_DG); - gain_idx = 3; - } else if (val == 1 && val2 == 0) { - new_gain = 0x0000; - gain_idx = 2; - } else if (val == 2 && val2 == 0) { - new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN); - gain_idx = 1; - } else if (val == 4 && val2 == 0) { - new_gain = FIELD_GET(VEML6035_GAIN_M, VEML6035_GAIN | - VEML6035_DG); - gain_idx = 0; - } else { - return -EINVAL; - } - - ret = regmap_field_write(data->rf.gain, new_gain); - if (ret) { - dev_err(&data->client->dev, "can't set als gain %d\n", ret); + ret = iio_gts_find_gain_time_sel_for_scale(&data->gts, val, val2, + &gain_sel, &it_sel); + if (ret) return ret; - } - - veml6030_update_gain_res(data, gain_idx); - return 0; -} - -static int veml6030_get_als_gain(struct iio_dev *indio_dev, - int *val, int *val2) -{ - int gain, ret; - struct veml6030_data *data = iio_priv(indio_dev); - - ret = regmap_field_read(data->rf.gain, &gain); - if (ret) { - dev_err(&data->client->dev, - "can't read als conf register %d\n", ret); + ret = regmap_field_write(data->rf.it, it_sel); + if (ret) return ret; - } - - switch (gain) { - case 0: - *val = 1; - *val2 = 0; - break; - case 1: - *val = 2; - *val2 = 0; - break; - case 2: - *val = 0; - *val2 = 125000; - break; - case 3: - *val = 0; - *val2 = 250000; - break; - default: - return -EINVAL; - } - - return IIO_VAL_INT_PLUS_MICRO; -} - -static int veml6035_get_als_gain(struct iio_dev *indio_dev, int *val, int *val2) -{ - int gain, ret; - struct veml6030_data *data = iio_priv(indio_dev); - ret = regmap_field_read(data->rf.gain, &gain); - if (ret) { - dev_err(&data->client->dev, - "can't read als conf register %d\n", ret); + ret = regmap_field_write(data->rf.gain, gain_sel); + if (ret) return ret; - } - - switch (gain) { - case 0: - *val = 1; - *val2 = 0; - break; - case 1: - case 2: - *val = 2; - *val2 = 0; - break; - case 3: - *val = 4; - *val2 = 0; - break; - case 4: - *val = 0; - *val2 = 125000; - break; - case 5: - case 6: - *val = 0; - *val2 = 250000; - break; - case 7: - *val = 0; - *val2 = 500000; - break; - default: - return -EINVAL; - } - return IIO_VAL_INT_PLUS_MICRO; + return 0; } static int veml6030_read_thresh(struct iio_dev *indio_dev, @@ -742,6 +576,50 @@ static int veml6030_write_thresh(struct iio_dev *indio_dev, return ret; } +static int veml6030_get_scale(struct veml6030_data *data, int *val, int *val2) +{ + int gain, it, reg, ret; + + ret = regmap_field_read(data->rf.gain, ®); + if (ret) + return ret; + + gain = iio_gts_find_gain_by_sel(&data->gts, reg); + if (gain < 0) + return gain; + + ret = regmap_field_read(data->rf.it, ®); + if (ret) + return ret; + + it = iio_gts_find_int_time_by_sel(&data->gts, reg); + if (it < 0) + return it; + + ret = iio_gts_get_scale(&data->gts, gain, it, val, val2); + if (ret) + return ret; + + return IIO_VAL_INT_PLUS_NANO; +} + +static int veml6030_process_als(struct veml6030_data *data, int raw, + int *val, int *val2) +{ + int ret, scale_int, scale_nano; + u64 tmp; + + ret = veml6030_get_scale(data, &scale_int, &scale_nano); + if (ret < 0) + return ret; + + tmp = (u64)raw * scale_nano; + *val = raw * scale_int + div_u64(tmp, NANO); + *val2 = div_u64(do_div(tmp, NANO), MILLI); + + return IIO_VAL_INT_PLUS_MICRO; +} + /* * Provide both raw as well as light reading in lux. * light (in lux) = resolution * raw reading @@ -765,11 +643,9 @@ static int veml6030_read_raw(struct iio_dev *indio_dev, dev_err(dev, "can't read als data %d\n", ret); return ret; } - if (mask == IIO_CHAN_INFO_PROCESSED) { - *val = (reg * data->cur_resolution) / 10000; - *val2 = (reg * data->cur_resolution) % 10000 * 100; - return IIO_VAL_INT_PLUS_MICRO; - } + if (mask == IIO_CHAN_INFO_PROCESSED) + return veml6030_process_als(data, reg, val, val2); + *val = reg; return IIO_VAL_INT; case IIO_INTENSITY: @@ -784,9 +660,9 @@ static int veml6030_read_raw(struct iio_dev *indio_dev, return -EINVAL; } case IIO_CHAN_INFO_INT_TIME: - return veml6030_get_intgrn_tm(indio_dev, val, val2); + return veml6030_get_it(data, val, val2); case IIO_CHAN_INFO_SCALE: - return data->chip->get_als_gain(indio_dev, val, val2); + return veml6030_get_scale(data, val, val2); default: return -EINVAL; } @@ -801,15 +677,9 @@ static int veml6030_read_avail(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_INT_TIME: - *vals = (int *)&veml6030_it_times; - *length = 2 * ARRAY_SIZE(veml6030_it_times); - *type = IIO_VAL_INT_PLUS_MICRO; - return IIO_AVAIL_LIST; + return iio_gts_avail_times(&data->gts, vals, type, length); case IIO_CHAN_INFO_SCALE: - *vals = (int *)*data->chip->scale_vals; - *length = 2 * data->chip->num_scale_vals; - *type = IIO_VAL_INT_PLUS_MICRO; - return IIO_AVAIL_LIST; + return iio_gts_all_avail_scales(&data->gts, vals, type, length); } return -EINVAL; @@ -819,13 +689,25 @@ static int veml6030_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, int val2, long mask) { - struct veml6030_data *data = iio_priv(indio_dev); - switch (mask) { case IIO_CHAN_INFO_INT_TIME: - return veml6030_set_intgrn_tm(indio_dev, val, val2); + return veml6030_set_it(indio_dev, val, val2); case IIO_CHAN_INFO_SCALE: - return data->chip->set_als_gain(indio_dev, val, val2); + return veml6030_set_scale(indio_dev, val, val2); + default: + return -EINVAL; + } +} + +static int veml6030_write_raw_get_fmt(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_SCALE: + return IIO_VAL_INT_PLUS_NANO; + case IIO_CHAN_INFO_INT_TIME: + return IIO_VAL_INT_PLUS_MICRO; default: return -EINVAL; } @@ -923,6 +805,7 @@ static const struct iio_info veml6030_info = { .read_raw = veml6030_read_raw, .read_avail = veml6030_read_avail, .write_raw = veml6030_write_raw, + .write_raw_get_fmt = veml6030_write_raw_get_fmt, .read_event_value = veml6030_read_event_val, .write_event_value = veml6030_write_event_val, .read_event_config = veml6030_read_interrupt_config, @@ -934,6 +817,7 @@ static const struct iio_info veml6030_info_no_irq = { .read_raw = veml6030_read_raw, .read_avail = veml6030_read_avail, .write_raw = veml6030_write_raw, + .write_raw_get_fmt = veml6030_write_raw_get_fmt, }; static irqreturn_t veml6030_event_handler(int irq, void *private) @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) int ret, val; struct veml6030_data *data = iio_priv(indio_dev); + ret = devm_iio_init_iio_gts(dev, 2, 150400000, + veml6030_gain_sel, ARRAY_SIZE(veml6030_gain_sel), + veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel), + &data->gts); + if (ret) + return dev_err_probe(dev, ret, "failed to init iio gts\n"); + ret = veml6030_als_shut_down(data); if (ret) return dev_err_probe(dev, ret, "can't shutdown als\n"); @@ -1119,11 +1010,6 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev) return dev_err_probe(dev, ret, "can't clear als interrupt status\n"); - /* Cache currently active measurement parameters */ - data->cur_gain = 3; - data->cur_resolution = 5376; - data->cur_integration_time = 3; - return ret; } @@ -1139,6 +1025,13 @@ static int veml6035_hw_init(struct iio_dev *indio_dev, struct device *dev) int ret, val; struct veml6030_data *data = iio_priv(indio_dev); + ret = devm_iio_init_iio_gts(dev, 0, 409600000, + veml6035_gain_sel, ARRAY_SIZE(veml6035_gain_sel), + veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel), + &data->gts); + if (ret) + return dev_err_probe(dev, ret, "failed to init iio gts\n"); + ret = veml6030_als_shut_down(data); if (ret) return dev_err_probe(dev, ret, "can't shutdown als\n"); @@ -1175,11 +1068,6 @@ static int veml6035_hw_init(struct iio_dev *indio_dev, struct device *dev) return dev_err_probe(dev, ret, "can't clear als interrupt status\n"); - /* Cache currently active measurement parameters */ - data->cur_gain = 5; - data->cur_resolution = 1024; - data->cur_integration_time = 3; - return 0; } @@ -1275,40 +1163,28 @@ static DEFINE_RUNTIME_DEV_PM_OPS(veml6030_pm_ops, veml6030_runtime_suspend, static const struct veml603x_chip veml6030_chip = { .name = "veml6030", - .scale_vals = &veml6030_scale_vals, - .num_scale_vals = ARRAY_SIZE(veml6030_scale_vals), .channels = veml6030_channels, .num_channels = ARRAY_SIZE(veml6030_channels), .hw_init = veml6030_hw_init, .set_info = veml6030_set_info, - .set_als_gain = veml6030_set_als_gain, - .get_als_gain = veml6030_get_als_gain, .regfield_init = veml6030_regfield_init, }; static const struct veml603x_chip veml6035_chip = { .name = "veml6035", - .scale_vals = &veml6035_scale_vals, - .num_scale_vals = ARRAY_SIZE(veml6035_scale_vals), .channels = veml6030_channels, .num_channels = ARRAY_SIZE(veml6030_channels), .hw_init = veml6035_hw_init, .set_info = veml6030_set_info, - .set_als_gain = veml6035_set_als_gain, - .get_als_gain = veml6035_get_als_gain, .regfield_init = veml6035_regfield_init, }; static const struct veml603x_chip veml7700_chip = { .name = "veml7700", - .scale_vals = &veml6030_scale_vals, - .num_scale_vals = ARRAY_SIZE(veml6030_scale_vals), .channels = veml7700_channels, .num_channels = ARRAY_SIZE(veml7700_channels), .hw_init = veml6030_hw_init, .set_info = veml7700_set_info, - .set_als_gain = veml6030_set_als_gain, - .get_als_gain = veml6030_get_als_gain, .regfield_init = veml6030_regfield_init, }; @@ -1351,3 +1227,4 @@ module_i2c_driver(veml6030_driver); MODULE_AUTHOR("Rishi Gupta <gupt21@gmail.com>"); MODULE_DESCRIPTION("VEML6030 Ambient Light Sensor"); MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS("IIO_GTS_HELPER");
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(-)