drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++--- 1 file changed, 238 insertions(+), 18 deletions(-)
1) Add support for configuring the gain and resolution(integration time)
for the sensor.
2) Add a channel for ALS and provide support for reading the raw and
scale values.
3) Add automatic mode switching between UVS and ALS based on the
channel type.
4) Calculate 'counts_per_uvi' based on the current gain and integration
time.
Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++---
1 file changed, 238 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index fff1e8990..56f3c74ae 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -25,19 +25,33 @@
#include <linux/regmap.h>
#include <linux/iio/iio.h>
-
+#include <linux/iio/sysfs.h>
#include <asm/unaligned.h>
#define LTR390_MAIN_CTRL 0x00
#define LTR390_PART_ID 0x06
#define LTR390_UVS_DATA 0x10
+#define LTR390_ALS_DATA 0x0D
+#define LTR390_ALS_UVS_GAIN 0x05
+#define LTR390_ALS_UVS_MEAS_RATE 0x04
+#define LTR390_INT_CFG 0x19
+
#define LTR390_SW_RESET BIT(4)
#define LTR390_UVS_MODE BIT(3)
#define LTR390_SENSOR_ENABLE BIT(1)
#define LTR390_PART_NUMBER_ID 0xb
+#define LTR390_ALS_UVS_GAIN_MASK 0x07
+#define LTR390_ALS_UVS_INT_TIME_MASK 0x70
+#define LTR390_ALS_UVS_INT_TIME_MASK_SHIFT 4
+
+#define LTR390_SET_ALS_MODE 1
+#define LTR390_SET_UVS_MODE 2
+
+#define LTR390_FRACTIONAL_PRECISION 100
+
/*
* At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
* the sensor are equal to 1 UV Index [Datasheet Page#8].
@@ -60,6 +74,9 @@ struct ltr390_data {
struct i2c_client *client;
/* Protects device from simulataneous reads */
struct mutex lock;
+ int mode;
+ int gain;
+ int int_time_us;
};
static const struct regmap_config ltr390_regmap_config = {
@@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
return get_unaligned_le24(recieve_buffer);
}
+
+static int ltr390_set_mode(struct ltr390_data *data, int mode)
+{
+ if (data->mode == mode)
+ return 0;
+
+ if (mode == LTR390_SET_ALS_MODE) {
+ regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+ data->mode = LTR390_SET_ALS_MODE;
+ } else if (mode == LTR390_SET_UVS_MODE) {
+ regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+ data->mode = LTR390_SET_UVS_MODE;
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int ltr390_counts_per_uvi(struct ltr390_data *data)
+{
+ int orig_gain = 18;
+ int orig_int_time = 400;
+ int divisor = orig_gain * orig_int_time;
+ int gain = data->gain;
+
+ int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
+ int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor);
+
+ return uvi;
+}
+
static int ltr390_read_raw(struct iio_dev *iio_device,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
{
- int ret;
struct ltr390_data *data = iio_priv(iio_device);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = ltr390_register_read(data, LTR390_UVS_DATA);
- if (ret < 0)
- return ret;
+ switch (chan->type) {
+ case IIO_UVINDEX:
+ ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+ if (ret < 0)
+ return ret;
+
+ ret = ltr390_register_read(data, LTR390_UVS_DATA);
+ if (ret < 0)
+ return ret;
+
+ break;
+
+ case IIO_INTENSITY:
+ ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+ if (ret < 0)
+ return ret;
+
+ ret = ltr390_register_read(data, LTR390_ALS_DATA);
+ if (ret < 0)
+ return ret;
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
*val = ret;
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
+ break;
+
case IIO_CHAN_INFO_SCALE:
- *val = LTR390_WINDOW_FACTOR;
- *val2 = LTR390_COUNTS_PER_UVI;
- return IIO_VAL_FRACTIONAL;
+ mutex_lock(&data->lock);
+
+ switch (chan->type) {
+ case IIO_UVINDEX:
+ ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+ if (ret < 0)
+ return ret;
+
+ *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
+ *val2 = ltr390_counts_per_uvi(data);
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ case IIO_INTENSITY:
+ ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+ if (ret < 0)
+ return ret;
+
+ *val = LTR390_WINDOW_FACTOR;
+ *val2 = data->gain;
+
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ mutex_unlock(&data->lock);
+ break;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ mutex_lock(&data->lock);
+ *val = data->int_time_us;
+ mutex_unlock(&data->lock);
+ ret = IIO_VAL_INT;
+ break;
+
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+
+ return ret;
}
-static const struct iio_info ltr390_info = {
- .read_raw = ltr390_read_raw,
+/* integration time in us */
+static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
+static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
+
+static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");
+static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");
+
+static struct attribute *ltr390_attributes[] = {
+ &iio_const_attr_integration_time_available.dev_attr.attr,
+ &iio_const_attr_gain_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group ltr390_attribute_group = {
+ .attrs = ltr390_attributes,
};
-static const struct iio_chan_spec ltr390_channel = {
+static const struct iio_chan_spec ltr390_channels[] = {
+ /* UV sensor */
+ {
.type = IIO_UVINDEX,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
+ .scan_index = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
+ },
+ /* ALS sensor */
+ {
+ .type = IIO_INTENSITY,
+ .scan_index = 1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
+ },
+};
+
+static int ltr390_set_gain(struct ltr390_data *data, int val)
+{
+ int ret, idx;
+
+ for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
+ if (ltr390_gain_map[idx] == val) {
+ mutex_lock(&data->lock);
+ ret = regmap_update_bits(data->regmap,
+ LTR390_ALS_UVS_GAIN,
+ LTR390_ALS_UVS_GAIN_MASK, idx);
+ if (!ret)
+ data->gain = ltr390_gain_map[idx];
+
+ mutex_unlock(&data->lock);
+ break;
+ }
+ }
+
+ return 0;
+}
+
+static int ltr390_set_int_time(struct ltr390_data *data, int val)
+{
+ int ret, idx;
+
+ for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
+ if (ltr390_int_time_map_us[idx] == val) {
+ mutex_lock(&data->lock);
+ ret = regmap_update_bits(data->regmap,
+ LTR390_ALS_UVS_MEAS_RATE,
+ LTR390_ALS_UVS_INT_TIME_MASK,
+ idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT);
+ if (!ret)
+ data->int_time_us = ltr390_int_time_map_us[idx];
+
+ mutex_unlock(&data->lock);
+ break;
+ }
+ }
+
+ return 0;
+}
+
+static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ltr390_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ if (val2 != 0)
+ ret = -EINVAL;
+
+ ret = ltr390_set_gain(data, val);
+ break;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ if (val2 != 0)
+ ret = -EINVAL;
+
+ ret = ltr390_set_int_time(data, val);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static const struct iio_info ltr390_info = {
+ .attrs = <r390_attribute_group,
+ .read_raw = ltr390_read_raw,
+ .write_raw = ltr390_write_raw,
};
static int ltr390_probe(struct i2c_client *client)
@@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client)
"regmap initialization failed\n");
data->client = client;
+ /* default value of int time from pg: 15 of the datasheet */
+ data->int_time_us = 100000;
+ /* default value of gain from pg: 16 of the datasheet */
+ data->gain = 3;
+ /* default mode for ltr390 is ALS mode */
+ data->mode = LTR390_SET_ALS_MODE;
+
mutex_init(&data->lock);
indio_dev->info = <r390_info;
- indio_dev->channels = <r390_channel;
- indio_dev->num_channels = 1;
+ indio_dev->channels = ltr390_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
indio_dev->name = "ltr390";
ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
@@ -161,8 +381,7 @@ static int ltr390_probe(struct i2c_client *client)
/* Wait for the registers to reset before proceeding */
usleep_range(1000, 2000);
- ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
- LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
+ ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
if (ret)
return dev_err_probe(dev, ret, "failed to enable the sensor\n");
@@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = {
.probe = ltr390_probe,
.id_table = ltr390_id,
};
+
module_i2c_driver(ltr390_driver);
MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
--
2.43.0On Thu, 18 Jul 2024 16:19:45 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:
> 1) Add support for configuring the gain and resolution(integration time)
> for the sensor.
> 2) Add a channel for ALS and provide support for reading the raw and
> scale values.
> 3) Add automatic mode switching between UVS and ALS based on the
> channel type.
> 4) Calculate 'counts_per_uvi' based on the current gain and integration
> time.
Hi Abhash,
When a patch lists more than one thing, key thing to think is
"maybe this should be multiple patches?"
Here at very least separate resolution / gain into one or two patches
and the new channel support into another.
Probably yet another patch for point 4,
Various other comments inline.
Jonathan
>
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
> drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 238 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index fff1e8990..56f3c74ae 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -25,19 +25,33 @@
> #include <linux/regmap.h>
>
> #include <linux/iio/iio.h>
> -
> +#include <linux/iio/sysfs.h>
> #include <asm/unaligned.h>
>
> #define LTR390_MAIN_CTRL 0x00
> #define LTR390_PART_ID 0x06
> #define LTR390_UVS_DATA 0x10
>
> +#define LTR390_ALS_DATA 0x0D
> +#define LTR390_ALS_UVS_GAIN 0x05
> +#define LTR390_ALS_UVS_MEAS_RATE 0x04
> +#define LTR390_INT_CFG 0x19
If these are register addresses put them in numeric order so
it is easy to compare with a datasheet table
> +
> #define LTR390_SW_RESET BIT(4)
> #define LTR390_UVS_MODE BIT(3)
> #define LTR390_SENSOR_ENABLE BIT(1)
>
> #define LTR390_PART_NUMBER_ID 0xb
>
> +#define LTR390_ALS_UVS_GAIN_MASK 0x07
> +#define LTR390_ALS_UVS_INT_TIME_MASK 0x70
> +#define LTR390_ALS_UVS_INT_TIME_MASK_SHIFT 4
Used FIELD_GET() and FIELD_PREP() then you never
need a separate SHIFT defintion.
> +
> +#define LTR390_SET_ALS_MODE 1
> +#define LTR390_SET_UVS_MODE 2
If these are being use to pick options and not writen to hw
use an enum. I don't think we care what value they take.
> +
> +#define LTR390_FRACTIONAL_PRECISION 100
> +
> /*
> * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
> * the sensor are equal to 1 UV Index [Datasheet Page#8].
> @@ -60,6 +74,9 @@ struct ltr390_data {
> struct i2c_client *client;
> /* Protects device from simulataneous reads */
> struct mutex lock;
> + int mode;
> + int gain;
> + int int_time_us;
> };
>
> static const struct regmap_config ltr390_regmap_config = {
> @@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> return get_unaligned_le24(recieve_buffer);
> }
>
> +
one blank line is neough.
> +static int ltr390_set_mode(struct ltr390_data *data, int mode)
As suggested above, use an enum for mode. Give than a type name and you
can use that here.
> +{
> + if (data->mode == mode)
> + return 0;
> +
> + if (mode == LTR390_SET_ALS_MODE) {
> + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> + data->mode = LTR390_SET_ALS_MODE;
> + } else if (mode == LTR390_SET_UVS_MODE) {
> + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> + data->mode = LTR390_SET_UVS_MODE;
Drop this out of the if / else stack and use
data->mode = mode;
A switch statement may be more appropriate here even if it's a few more lines of
code.
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ltr390_counts_per_uvi(struct ltr390_data *data)
> +{
> + int orig_gain = 18;
> + int orig_int_time = 400;
> + int divisor = orig_gain * orig_int_time;
> + int gain = data->gain;
> +
> + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
> + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor);
Spaces around *
> +
> + return uvi;
> +}
> +
> static int ltr390_read_raw(struct iio_dev *iio_device,
> struct iio_chan_spec const *chan, int *val,
> int *val2, long mask)
> {
> - int ret;
> struct ltr390_data *data = iio_priv(iio_device);
> + int ret;
Don't move code unless there is a strong reason. Fine to
tidy this sort of thing up, but not in a patch doing anything else
as it becomes noise.
>
Almost certainly need locking here as concurrent accesses to sysfs
files will result in mode changing whilst the read has not yet happened.
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - ret = ltr390_register_read(data, LTR390_UVS_DATA);
> - if (ret < 0)
> - return ret;
> + switch (chan->type) {
> + case IIO_UVINDEX:
> + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ltr390_register_read(data, LTR390_UVS_DATA);
Fix the alignment - looks like mix of spaces and tabs.
scripts/checkpatch.pl would have pointed that out.
> + if (ret < 0)
> + return ret;
> +
> + break;
> +
> + case IIO_INTENSITY:
> + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ltr390_register_read(data, LTR390_ALS_DATA);
> + if (ret < 0)
> + return ret;
> + break;
> +
> + default:
> + ret = -EINVAL;
return here. Otherwise you overwrite the value below.
> + }
> +
> *val = ret;
> - return IIO_VAL_INT;
> + ret = IIO_VAL_INT;
return here and drop the break.
It is much simpler to follow code if it doesn't unnecessarily not
return in cases like this as we have to scroll down to see if anything else
happens.
> + break;
> +
> case IIO_CHAN_INFO_SCALE:
> - *val = LTR390_WINDOW_FACTOR;
> - *val2 = LTR390_COUNTS_PER_UVI;
> - return IIO_VAL_FRACTIONAL;
> + mutex_lock(&data->lock);
Add appropriate scope using {} and use
guard(mutex)(&data->lock) as then in error paths you can
return without unlocking...
> +
> + switch (chan->type) {
> + case IIO_UVINDEX:
> + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> + if (ret < 0)
mutex held. Result is deadlock. Above scoped unlocking avoids that without
needing to make sure you unlock in all paths.
> + return ret;
> +
> + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> + *val2 = ltr390_counts_per_uvi(data);
> + ret = IIO_VAL_FRACTIONAL;
return here.
> + break;
> +
> + case IIO_INTENSITY:
> + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> + if (ret < 0)
> + return ret;
> +
> + *val = LTR390_WINDOW_FACTOR;
> + *val2 = data->gain;
> +
> + ret = IIO_VAL_FRACTIONAL;
> + break;
return here.
> +
> + default:
> + ret = -EINVAL;
return here.
> + }
> +
> + mutex_unlock(&data->lock);
With guard() change above, not needed.
But close scope here with }
> + break;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + mutex_lock(&data->lock);
Given all paths other than invalid ones need the lock, maybe just take
it outside of the switch statement - still use guard() though to avoid
need to manually unlock.
> + *val = data->int_time_us;
> + mutex_unlock(&data->lock);
> + ret = IIO_VAL_INT;
> + break;
> +
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> +
> + return ret;
This is a bad change as now I need to read to end of function in all
code paths. Some code styles insist on single exit points, but
the kernel style does not. (not worth a long discussion of why the
two common styles came about). Keep those early returns.
> }
>
> -static const struct iio_info ltr390_info = {
> - .read_raw = ltr390_read_raw,
> +/* integration time in us */
> +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
> +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
> +
> +static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");
Please use read_avail() callback and the appropriate mask to provide this.
That enables it to be used from in kernel consumers and enforces the
ABI without a reviewer having to check what you have aligns.
> +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");
Given we don't have a 'gain' control, what is the available applying to?
> +
> +static struct attribute *ltr390_attributes[] = {
> + &iio_const_attr_integration_time_available.dev_attr.attr,
> + &iio_const_attr_gain_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ltr390_attribute_group = {
> + .attrs = ltr390_attributes,
> };
>
> -static const struct iio_chan_spec ltr390_channel = {
> +static const struct iio_chan_spec ltr390_channels[] = {
> + /* UV sensor */
> + {
> .type = IIO_UVINDEX,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> + .scan_index = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
Fix style.
{
.type = ...
> + },
> + /* ALS sensor */
> + {
> + .type = IIO_INTENSITY,
> + .scan_index = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> + },
> +};
> +
> +static int ltr390_set_gain(struct ltr390_data *data, int val)
> +{
> + int ret, idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
> + if (ltr390_gain_map[idx] == val) {
> + mutex_lock(&data->lock);
guard here.
> + ret = regmap_update_bits(data->regmap,
> + LTR390_ALS_UVS_GAIN,
> + LTR390_ALS_UVS_GAIN_MASK, idx);
> + if (!ret)
if (ret)
return ret;
prefer to keep error paths as the out of line ones as if you review
a lot of code, predictability helps review quickly.
> + data->gain = ltr390_gain_map[idx];
> +
> + mutex_unlock(&data->lock);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int ltr390_set_int_time(struct ltr390_data *data, int val)
> +{
> + int ret, idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
> + if (ltr390_int_time_map_us[idx] == val) {
flip logic to reduce indent.
if (ltr390_int_time_map_us[idx] != val)
continue;
guard(mutex)...
> + mutex_lock(&data->lock);
> + ret = regmap_update_bits(data->regmap,
> + LTR390_ALS_UVS_MEAS_RATE,
> + LTR390_ALS_UVS_INT_TIME_MASK,
> + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT);
spaces around <<
Though FIELD_PREP() probably better solution.
> + if (!ret)
As in previous funciton.
> + data->int_time_us = ltr390_int_time_map_us[idx];
> +
> + mutex_unlock(&data->lock);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ltr390_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + if (val2 != 0)
> + ret = -EINVAL;
> +
> + ret = ltr390_set_gain(data, val);
> + break;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + if (val2 != 0)
> + ret = -EINVAL;
> +
> + ret = ltr390_set_int_time(data, val);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + }
> +
> + return ret;
Use early returns.
> +}
> +
> +static const struct iio_info ltr390_info = {
> + .attrs = <r390_attribute_group,
> + .read_raw = ltr390_read_raw,
> + .write_raw = ltr390_write_raw,
> };
>
> static int ltr390_probe(struct i2c_client *client)
> @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client)
> "regmap initialization failed\n");
>
> data->client = client;
> + /* default value of int time from pg: 15 of the datasheet */
I'd spell out integration in the comment.
> + data->int_time_us = 100000;
> + /* default value of gain from pg: 16 of the datasheet */
> + data->gain = 3;
> + /* default mode for ltr390 is ALS mode */
> + data->mode = LTR390_SET_ALS_MODE;
> +
> mutex_init(&data->lock);
>
> indio_dev->info = <r390_info;
> - indio_dev->channels = <r390_channel;
> - indio_dev->num_channels = 1;
> + indio_dev->channels = ltr390_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
> indio_dev->name = "ltr390";
>
> ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
> @@ -161,8 +381,7 @@ static int ltr390_probe(struct i2c_client *client)
> /* Wait for the registers to reset before proceeding */
> usleep_range(1000, 2000);
>
> - ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> - LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> + ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> if (ret)
> return dev_err_probe(dev, ret, "failed to enable the sensor\n");
>
> @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = {
> .probe = ltr390_probe,
> .id_table = ltr390_id,
> };
> +
Lack of space is intentional to keep the macro closely coupled to what
it applies to.
> module_i2c_driver(ltr390_driver);
>
> MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
On Sat, Jul 20, 2024 at 9:26 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 18 Jul 2024 16:19:45 +0530
> Abhash Jha <abhashkumarjha123@gmail.com> wrote:
>
> > 1) Add support for configuring the gain and resolution(integration time)
> > for the sensor.
> > 2) Add a channel for ALS and provide support for reading the raw and
> > scale values.
> > 3) Add automatic mode switching between UVS and ALS based on the
> > channel type.
> > 4) Calculate 'counts_per_uvi' based on the current gain and integration
> > time.
>
> Hi Abhash,
>
> When a patch lists more than one thing, key thing to think is
> "maybe this should be multiple patches?"
>
> Here at very least separate resolution / gain into one or two patches
> and the new channel support into another.
> Probably yet another patch for point 4,
>
> Various other comments inline.
>
> Jonathan
>
> >
> > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> > ---
> > drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 238 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> > index fff1e8990..56f3c74ae 100644
> > --- a/drivers/iio/light/ltr390.c
> > +++ b/drivers/iio/light/ltr390.c
> > @@ -25,19 +25,33 @@
> > #include <linux/regmap.h>
> >
> > #include <linux/iio/iio.h>
> > -
> > +#include <linux/iio/sysfs.h>
>
> > #include <asm/unaligned.h>
> >
> > #define LTR390_MAIN_CTRL 0x00
> > #define LTR390_PART_ID 0x06
> > #define LTR390_UVS_DATA 0x10
> >
> > +#define LTR390_ALS_DATA 0x0D
> > +#define LTR390_ALS_UVS_GAIN 0x05
> > +#define LTR390_ALS_UVS_MEAS_RATE 0x04
> > +#define LTR390_INT_CFG 0x19
> If these are register addresses put them in numeric order so
> it is easy to compare with a datasheet table
>
> > +
> > #define LTR390_SW_RESET BIT(4)
> > #define LTR390_UVS_MODE BIT(3)
> > #define LTR390_SENSOR_ENABLE BIT(1)
> >
> > #define LTR390_PART_NUMBER_ID 0xb
> >
> > +#define LTR390_ALS_UVS_GAIN_MASK 0x07
> > +#define LTR390_ALS_UVS_INT_TIME_MASK 0x70
> > +#define LTR390_ALS_UVS_INT_TIME_MASK_SHIFT 4
>
> Used FIELD_GET() and FIELD_PREP() then you never
> need a separate SHIFT defintion.
>
> > +
> > +#define LTR390_SET_ALS_MODE 1
> > +#define LTR390_SET_UVS_MODE 2
>
> If these are being use to pick options and not writen to hw
> use an enum. I don't think we care what value they take.
>
>
> > +
> > +#define LTR390_FRACTIONAL_PRECISION 100
> > +
> > /*
> > * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
> > * the sensor are equal to 1 UV Index [Datasheet Page#8].
> > @@ -60,6 +74,9 @@ struct ltr390_data {
> > struct i2c_client *client;
> > /* Protects device from simulataneous reads */
> > struct mutex lock;
> > + int mode;
> > + int gain;
> > + int int_time_us;
> > };
> >
> > static const struct regmap_config ltr390_regmap_config = {
> > @@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> > return get_unaligned_le24(recieve_buffer);
> > }
> >
> > +
> one blank line is neough.
>
> > +static int ltr390_set_mode(struct ltr390_data *data, int mode)
> As suggested above, use an enum for mode. Give than a type name and you
> can use that here.
>
> > +{
> > + if (data->mode == mode)
> > + return 0;
> > +
> > + if (mode == LTR390_SET_ALS_MODE) {
> > + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> > + data->mode = LTR390_SET_ALS_MODE;
> > + } else if (mode == LTR390_SET_UVS_MODE) {
> > + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> > + data->mode = LTR390_SET_UVS_MODE;
> Drop this out of the if / else stack and use
> data->mode = mode;
> A switch statement may be more appropriate here even if it's a few more lines of
> code.
>
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ltr390_counts_per_uvi(struct ltr390_data *data)
> > +{
> > + int orig_gain = 18;
> > + int orig_int_time = 400;
> > + int divisor = orig_gain * orig_int_time;
> > + int gain = data->gain;
> > +
> > + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
> > + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor);
>
> Spaces around *
>
> > +
> > + return uvi;
> > +}
> > +
> > static int ltr390_read_raw(struct iio_dev *iio_device,
> > struct iio_chan_spec const *chan, int *val,
> > int *val2, long mask)
> > {
> > - int ret;
> > struct ltr390_data *data = iio_priv(iio_device);
> > + int ret;
> Don't move code unless there is a strong reason. Fine to
> tidy this sort of thing up, but not in a patch doing anything else
> as it becomes noise.
>
> >
> Almost certainly need locking here as concurrent accesses to sysfs
> files will result in mode changing whilst the read has not yet happened.
>
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > - ret = ltr390_register_read(data, LTR390_UVS_DATA);
> > - if (ret < 0)
> > - return ret;
> > + switch (chan->type) {
> > + case IIO_UVINDEX:
> > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ltr390_register_read(data, LTR390_UVS_DATA);
> Fix the alignment - looks like mix of spaces and tabs.
> scripts/checkpatch.pl would have pointed that out.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + break;
> > +
> > + case IIO_INTENSITY:
> > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ltr390_register_read(data, LTR390_ALS_DATA);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> return here. Otherwise you overwrite the value below.
>
> > + }
> > +
> > *val = ret;
> > - return IIO_VAL_INT;
> > + ret = IIO_VAL_INT;
> return here and drop the break.
> It is much simpler to follow code if it doesn't unnecessarily not
> return in cases like this as we have to scroll down to see if anything else
> happens.
>
> > + break;
> > +
> > case IIO_CHAN_INFO_SCALE:
> > - *val = LTR390_WINDOW_FACTOR;
> > - *val2 = LTR390_COUNTS_PER_UVI;
> > - return IIO_VAL_FRACTIONAL;
> > + mutex_lock(&data->lock);
> Add appropriate scope using {} and use
> guard(mutex)(&data->lock) as then in error paths you can
> return without unlocking...
> > +
> > + switch (chan->type) {
> > + case IIO_UVINDEX:
> > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> > + if (ret < 0)
> mutex held. Result is deadlock. Above scoped unlocking avoids that without
> needing to make sure you unlock in all paths.
>
>
> > + return ret;
> > +
> > + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> > + *val2 = ltr390_counts_per_uvi(data);
> > + ret = IIO_VAL_FRACTIONAL;
> return here.
>
> > + break;
> > +
> > + case IIO_INTENSITY:
> > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = LTR390_WINDOW_FACTOR;
> > + *val2 = data->gain;
> > +
> > + ret = IIO_VAL_FRACTIONAL;
> > + break;
> return here.
> > +
> > + default:
> > + ret = -EINVAL;
> return here.
> > + }
> > +
> > + mutex_unlock(&data->lock);
> With guard() change above, not needed.
> But close scope here with }
> > + break;
> > +
> > + case IIO_CHAN_INFO_INT_TIME:
> > + mutex_lock(&data->lock);
> Given all paths other than invalid ones need the lock, maybe just take
> it outside of the switch statement - still use guard() though to avoid
> need to manually unlock.
>
> > + *val = data->int_time_us;
> > + mutex_unlock(&data->lock);
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > }
> > +
> > + return ret;
> This is a bad change as now I need to read to end of function in all
> code paths. Some code styles insist on single exit points, but
> the kernel style does not. (not worth a long discussion of why the
> two common styles came about). Keep those early returns.
>
>
> > }
> >
> > -static const struct iio_info ltr390_info = {
> > - .read_raw = ltr390_read_raw,
> > +/* integration time in us */
> > +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
> > +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
> > +
> > +static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");
> Please use read_avail() callback and the appropriate mask to provide this.
> That enables it to be used from in kernel consumers and enforces the
> ABI without a reviewer having to check what you have aligns.
>
> > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");
> Given we don't have a 'gain' control, what is the available applying to?
>
The gain gets controlled by writing to the iio_info_scale attribute,
we write one of the above available values.
So that we can scale the raw ALS and UVI values. I could use
read_avail() for this too for the IIO_INFO_SCALE channel. Should I do
that?
Can you elaborate more on your comment?
> > +
> > +static struct attribute *ltr390_attributes[] = {
> > + &iio_const_attr_integration_time_available.dev_attr.attr,
> > + &iio_const_attr_gain_available.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group ltr390_attribute_group = {
> > + .attrs = ltr390_attributes,
> > };
> >
> > -static const struct iio_chan_spec ltr390_channel = {
> > +static const struct iio_chan_spec ltr390_channels[] = {
> > + /* UV sensor */
> > + {
> > .type = IIO_UVINDEX,
> > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> > + .scan_index = 0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> Fix style.
> {
> .type = ...
>
> > + },
> > + /* ALS sensor */
> > + {
> > + .type = IIO_INTENSITY,
> > + .scan_index = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> > + },
> > +};
> > +
> > +static int ltr390_set_gain(struct ltr390_data *data, int val)
> > +{
> > + int ret, idx;
> > +
> > + for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
> > + if (ltr390_gain_map[idx] == val) {
> > + mutex_lock(&data->lock);
> guard here.
> > + ret = regmap_update_bits(data->regmap,
> > + LTR390_ALS_UVS_GAIN,
> > + LTR390_ALS_UVS_GAIN_MASK, idx);
> > + if (!ret)
> if (ret)
> return ret;
> prefer to keep error paths as the out of line ones as if you review
> a lot of code, predictability helps review quickly.
>
> > + data->gain = ltr390_gain_map[idx];
> > +
> > + mutex_unlock(&data->lock);
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ltr390_set_int_time(struct ltr390_data *data, int val)
> > +{
> > + int ret, idx;
> > +
> > + for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
> > + if (ltr390_int_time_map_us[idx] == val) {
> flip logic to reduce indent.
> if (ltr390_int_time_map_us[idx] != val)
> continue;
>
> guard(mutex)...
>
> > + mutex_lock(&data->lock);
> > + ret = regmap_update_bits(data->regmap,
> > + LTR390_ALS_UVS_MEAS_RATE,
> > + LTR390_ALS_UVS_INT_TIME_MASK,
> > + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT);
> spaces around <<
> Though FIELD_PREP() probably better solution.
>
> > + if (!ret)
> As in previous funciton.
> > + data->int_time_us = ltr390_int_time_map_us[idx];
> > +
> > + mutex_unlock(&data->lock);
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ltr390_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + if (val2 != 0)
> > + ret = -EINVAL;
> > +
> > + ret = ltr390_set_gain(data, val);
> > + break;
> > +
> > + case IIO_CHAN_INFO_INT_TIME:
> > + if (val2 != 0)
> > + ret = -EINVAL;
> > +
> > + ret = ltr390_set_int_time(data, val);
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret;
> Use early returns.
>
> > +}
> > +
> > +static const struct iio_info ltr390_info = {
> > + .attrs = <r390_attribute_group,
> > + .read_raw = ltr390_read_raw,
> > + .write_raw = ltr390_write_raw,
> > };
> >
> > static int ltr390_probe(struct i2c_client *client)
> > @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client)
> > "regmap initialization failed\n");
> >
> > data->client = client;
> > + /* default value of int time from pg: 15 of the datasheet */
> I'd spell out integration in the comment.
>
> > + data->int_time_us = 100000;
> > + /* default value of gain from pg: 16 of the datasheet */
> > + data->gain = 3;
> > + /* default mode for ltr390 is ALS mode */
> > + data->mode = LTR390_SET_ALS_MODE;
> > +
> > mutex_init(&data->lock);
> >
> > indio_dev->info = <r390_info;
> > - indio_dev->channels = <r390_channel;
> > - indio_dev->num_channels = 1;
> > + indio_dev->channels = ltr390_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
> > indio_dev->name = "ltr390";
> >
> > ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
> > @@ -161,8 +381,7 @@ static int ltr390_probe(struct i2c_client *client)
> > /* Wait for the registers to reset before proceeding */
> > usleep_range(1000, 2000);
> >
> > - ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> > - LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> > + ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to enable the sensor\n");
> >
> > @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = {
> > .probe = ltr390_probe,
> > .id_table = ltr390_id,
> > };
> > +
> Lack of space is intentional to keep the macro closely coupled to what
> it applies to.
>
> > module_i2c_driver(ltr390_driver);
> >
> > MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
>
ACK. Will do the necessary changes and send V2 after splitting it into
4 patches (replying again because I missed replying with CC last time)
Please crop replies to only leave the section being discussed.
It saves time for everyone reading the thread.
> > > -static const struct iio_info ltr390_info = {
> > > - .read_raw = ltr390_read_raw,
> > > +/* integration time in us */
> > > +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
> > > +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
> > > +
> > > +static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");
> > Please use read_avail() callback and the appropriate mask to provide this.
> > That enables it to be used from in kernel consumers and enforces the
> > ABI without a reviewer having to check what you have aligns.
> >
> > > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");
> > Given we don't have a 'gain' control, what is the available applying to?
> >
> The gain gets controlled by writing to the iio_info_scale attribute,
> we write one of the above available values.
> So that we can scale the raw ALS and UVI values. I could use
> read_avail() for this too for the IIO_INFO_SCALE channel. Should I do
> that?
Yes, it would be appropriate to provide read_avail for IIO_INFO_SCALE
as that is standard ABI that userspace will have way to interpret.
> Can you elaborate more on your comment?
Basic rule of thumb is think very hard about whether there is an alternative
if you are providing attributes directly to an IIO driver.
There are a few corners where that is necessary for standard ABI
around FIFOs or certain event related attributes + a few special
corners for complex hardwware.
None of those apply here, so read_avail callback and choosing standard
ABI elements to match what you are trying to control / describe is the
way to go. That's the stuff that userspace tooling knows how to use.
Jonathan
Add support for configuring and reading the gain and resolution
(integration time). Also provide the available values for gain and
resoltion respectively via `read_avail` callback.
Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
drivers/iio/light/ltr390.c | 144 +++++++++++++++++++++++++++++++++----
1 file changed, 130 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index fff1e8990..9f00661c3 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -25,19 +25,26 @@
#include <linux/regmap.h>
#include <linux/iio/iio.h>
-
+#include <linux/iio/sysfs.h>
#include <asm/unaligned.h>
-#define LTR390_MAIN_CTRL 0x00
-#define LTR390_PART_ID 0x06
-#define LTR390_UVS_DATA 0x10
+#define LTR390_MAIN_CTRL 0x00
+#define LTR390_ALS_UVS_MEAS_RATE 0x04
+#define LTR390_ALS_UVS_GAIN 0x05
+#define LTR390_PART_ID 0x06
+#define LTR390_ALS_DATA 0x0D
+#define LTR390_UVS_DATA 0x10
+#define LTR390_INT_CFG 0x19
+
+#define LTR390_PART_NUMBER_ID 0xb
+#define LTR390_ALS_UVS_GAIN_MASK 0x07
+#define LTR390_ALS_UVS_INT_TIME_MASK 0x70
+#define LTR390_ALS_UVS_INT_TIME(x) FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, x)
#define LTR390_SW_RESET BIT(4)
#define LTR390_UVS_MODE BIT(3)
#define LTR390_SENSOR_ENABLE BIT(1)
-#define LTR390_PART_NUMBER_ID 0xb
-
/*
* At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
* the sensor are equal to 1 UV Index [Datasheet Page#8].
@@ -60,6 +67,8 @@ struct ltr390_data {
struct i2c_client *client;
/* Protects device from simulataneous reads */
struct mutex lock;
+ int gain;
+ int int_time_us;
};
static const struct regmap_config ltr390_regmap_config = {
@@ -75,8 +84,6 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
int ret;
u8 recieve_buffer[3];
- guard(mutex)(&data->lock);
-
ret = regmap_bulk_read(data->regmap, register_address, recieve_buffer,
sizeof(recieve_buffer));
if (ret) {
@@ -91,32 +98,135 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
{
- int ret;
struct ltr390_data *data = iio_priv(iio_device);
+ int ret;
+ guard(mutex)(&data->lock);
switch (mask) {
case IIO_CHAN_INFO_RAW:
ret = ltr390_register_read(data, LTR390_UVS_DATA);
if (ret < 0)
return ret;
+
*val = ret;
return IIO_VAL_INT;
+
case IIO_CHAN_INFO_SCALE:
*val = LTR390_WINDOW_FACTOR;
*val2 = LTR390_COUNTS_PER_UVI;
return IIO_VAL_FRACTIONAL;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = data->int_time_us;
+ return IIO_VAL_INT;
+
default:
return -EINVAL;
}
}
-static const struct iio_info ltr390_info = {
- .read_raw = ltr390_read_raw,
-};
+/* integration time in us */
+static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
+static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
static const struct iio_chan_spec ltr390_channel = {
.type = IIO_UVINDEX,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+};
+
+static int ltr390_set_gain(struct ltr390_data *data, int val)
+{
+ int ret, idx;
+
+ for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
+ if (ltr390_gain_map[idx] != val)
+ continue;
+
+ guard(mutex)(&data->lock);
+ ret = regmap_update_bits(data->regmap,
+ LTR390_ALS_UVS_GAIN,
+ LTR390_ALS_UVS_GAIN_MASK, idx);
+ if (ret)
+ return ret;
+
+ data->gain = ltr390_gain_map[idx];
+ break;
+ }
+
+ return 0;
+}
+
+static int ltr390_set_int_time(struct ltr390_data *data, int val)
+{
+ int ret, idx;
+
+ for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
+ if (ltr390_int_time_map_us[idx] != val)
+ continue;
+
+ guard(mutex)(&data->lock);
+ ret = regmap_update_bits(data->regmap,
+ LTR390_ALS_UVS_MEAS_RATE,
+ LTR390_ALS_UVS_INT_TIME_MASK,
+ LTR390_ALS_UVS_INT_TIME(idx));
+ if (ret)
+ return ret;
+
+ data->int_time_us = ltr390_int_time_map_us[idx];
+ break;
+ }
+
+ return 0;
+}
+
+static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *length = ARRAY_SIZE(ltr390_gain_map);
+ *type = IIO_VAL_INT;
+ *vals = ltr390_gain_map;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_INT_TIME:
+ *length = ARRAY_SIZE(ltr390_int_time_map_us);
+ *type = IIO_VAL_INT;
+ *vals = ltr390_int_time_map_us;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ltr390_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ if (val2 != 0)
+ return -EINVAL;
+
+ return ltr390_set_gain(data, val);
+
+ case IIO_CHAN_INFO_INT_TIME:
+ if (val2 != 0)
+ return -EINVAL;
+
+ return ltr390_set_int_time(data, val);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info ltr390_info = {
+ .read_raw = ltr390_read_raw,
+ .write_raw = ltr390_write_raw,
+ .read_avail = ltr390_read_avail,
};
static int ltr390_probe(struct i2c_client *client)
@@ -139,6 +249,11 @@ static int ltr390_probe(struct i2c_client *client)
"regmap initialization failed\n");
data->client = client;
+ /* default value of integration time from pg: 15 of the datasheet */
+ data->int_time_us = 100000;
+ /* default value of gain from pg: 16 of the datasheet */
+ data->gain = 3;
+
mutex_init(&data->lock);
indio_dev->info = <r390_info;
@@ -162,7 +277,7 @@ static int ltr390_probe(struct i2c_client *client)
usleep_range(1000, 2000);
ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
- LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
+ LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
if (ret)
return dev_err_probe(dev, ret, "failed to enable the sensor\n");
@@ -189,6 +304,7 @@ static struct i2c_driver ltr390_driver = {
.probe = ltr390_probe,
.id_table = ltr390_id,
};
+
module_i2c_driver(ltr390_driver);
MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
--
2.43.0On Sun, 28 Jul 2024 20:49:54 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:
> Add support for configuring and reading the gain and resolution
> (integration time). Also provide the available values for gain and
> resoltion respectively via `read_avail` callback.
Hi Abhash,
Don't indent patch description like this.
Also from a process point of view, for IIO (and I think most of the kernel)
don't reply to a previous version thread with a new version.
The upshot is that it ends up far from the most recent emails in everyone's
inboxes as pretty much everyone uses threading.
Also, if sending multiple patches, please add a cover letter.
--cover-letter in git.
That provides a general place for comments like this one and also
gives the series a nice pretty title in patch work and similar tooling ;)
https://patchwork.kernel.org/project/linux-iio/list/?
See the series title column - that's from cover letters.
Anyhow, on to the code. Various minor comments inline.
In particularly for v3, please look for accidental or unnecessary changes
of code formatting etc.
They add considerable noise to a fairly simple real change.
Jonathan
>
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
> drivers/iio/light/ltr390.c | 144 +++++++++++++++++++++++++++++++++----
> 1 file changed, 130 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index fff1e8990..9f00661c3 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -25,19 +25,26 @@
> #include <linux/regmap.h>
>
> #include <linux/iio/iio.h>
> -
> +#include <linux/iio/sysfs.h>
Keep a blank line after the iio includes.
However, why this include? You aren't defining custom attributes in this
patch.
> #include <asm/unaligned.h>
>
> -#define LTR390_MAIN_CTRL 0x00
> -#define LTR390_PART_ID 0x06
> -#define LTR390_UVS_DATA 0x10
Hmm. Annoying to have to realign this. However, it probably isn't
worth a precursor patch for this one.
If it were many more than 3 lines it would be.
> +#define LTR390_MAIN_CTRL 0x00
> +#define LTR390_ALS_UVS_MEAS_RATE 0x04
> +#define LTR390_ALS_UVS_GAIN 0x05
> +#define LTR390_PART_ID 0x06
> +#define LTR390_ALS_DATA 0x0D
> +#define LTR390_UVS_DATA 0x10
> +#define LTR390_INT_CFG 0x19
> +
> +#define LTR390_PART_NUMBER_ID 0xb
> +#define LTR390_ALS_UVS_GAIN_MASK 0x07
> +#define LTR390_ALS_UVS_INT_TIME_MASK 0x70
> +#define LTR390_ALS_UVS_INT_TIME(x) FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, x)
Brackets around x
define LTR390_ALS_UVS_INT_TIME(x) FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
>
> #define LTR390_SW_RESET BIT(4)
> #define LTR390_UVS_MODE BIT(3)
> #define LTR390_SENSOR_ENABLE BIT(1)
>
> -#define LTR390_PART_NUMBER_ID 0xb
..
> @@ -91,32 +98,135 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
> struct iio_chan_spec const *chan, int *val,
> int *val2, long mask)
> {
> - int ret;
> struct ltr390_data *data = iio_priv(iio_device);
> + int ret;
>
> + guard(mutex)(&data->lock);
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> ret = ltr390_register_read(data, LTR390_UVS_DATA);
> if (ret < 0)
> return ret;
> +
Not related to the patch content, so shouldn't be here.
> *val = ret;
> return IIO_VAL_INT;
> +
Similarly. It's a reasonable change, but not in this patch as it
adds noise. Feel free to send another patch in the series that improves
the white space though if you like.
> case IIO_CHAN_INFO_SCALE:
> *val = LTR390_WINDOW_FACTOR;
> *val2 = LTR390_COUNTS_PER_UVI;
> return IIO_VAL_FRACTIONAL;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = data->int_time_us;
> + return IIO_VAL_INT;
> +
> default:
> return -EINVAL;
> }
> }
...
> +/* integration time in us */
> +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
space after { and before } preferred.
> +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
>
> static const struct iio_chan_spec ltr390_channel = {
> .type = IIO_UVINDEX,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +};
> +
> +static int ltr390_set_gain(struct ltr390_data *data, int val)
> +{
> + int ret, idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
> + if (ltr390_gain_map[idx] != val)
> + continue;
> +
> + guard(mutex)(&data->lock);
> + ret = regmap_update_bits(data->regmap,
> + LTR390_ALS_UVS_GAIN,
> + LTR390_ALS_UVS_GAIN_MASK, idx);
> + if (ret)
> + return ret;
> +
> + data->gain = ltr390_gain_map[idx];
> + break;
As below.
return 0;
> + }
> +
> + return 0;
return -EINVAL; to indicate no match.
> +}
> +
> +static int ltr390_set_int_time(struct ltr390_data *data, int val)
> +{
> + int ret, idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
> + if (ltr390_int_time_map_us[idx] != val)
> + continue;
> +
> + guard(mutex)(&data->lock);
> + ret = regmap_update_bits(data->regmap,
> + LTR390_ALS_UVS_MEAS_RATE,
> + LTR390_ALS_UVS_INT_TIME_MASK,
> + LTR390_ALS_UVS_INT_TIME(idx));
> + if (ret)
> + return ret;
> +
> + data->int_time_us = ltr390_int_time_map_us[idx];
> + break;
return 0;
No point in carrying on if we are done.
> + }
> +
> + return 0;
If you get here with suggested return 0 above, it will be an error as no
match occured. In that case, return -EINVAL;
> +}
...
> static int ltr390_probe(struct i2c_client *client)
> @@ -139,6 +249,11 @@ static int ltr390_probe(struct i2c_client *client)
> "regmap initialization failed\n");
>
> data->client = client;
> + /* default value of integration time from pg: 15 of the datasheet */
> + data->int_time_us = 100000;
> + /* default value of gain from pg: 16 of the datasheet */
> + data->gain = 3;
> +
> mutex_init(&data->lock);
>
> indio_dev->info = <r390_info;
> @@ -162,7 +277,7 @@ static int ltr390_probe(struct i2c_client *client)
> usleep_range(1000, 2000);
>
> ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> - LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> + LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
Avoid accidental white space changes. If you want to make them to cleanup
some inconsistencies or similar, that is fine, but they belong in a patch
that doesn't do anything else. Here it is adding noise and slowing down
review.
> if (ret)
> return dev_err_probe(dev, ret, "failed to enable the sensor\n");
>
> @@ -189,6 +304,7 @@ static struct i2c_driver ltr390_driver = {
> .probe = ltr390_probe,
> .id_table = ltr390_id,
> };
> +
Don't add this line. We often use whitespace to indicate connections and
it is common to do it in cases like this one where module_i2c_driver()
is tightly coupled with the i2c_driver structure.
> module_i2c_driver(ltr390_driver);
>
> MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
Add new ALS channel and allow reading raw and scale values.
Also provide gain and resolution configuration for ALS channel.
Add automatic mode switching between the UVS and ALS channel
based on which channel is being accessed.
The default mode in which the sensor start is ALS mode.
Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
drivers/iio/light/ltr390.c | 111 ++++++++++++++++++++++++++++++++-----
1 file changed, 96 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 9f00661c3..d1a259141 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -62,11 +62,17 @@
*/
#define LTR390_WINDOW_FACTOR 1
+enum ltr390_mode {
+ LTR390_SET_ALS_MODE,
+ LTR390_SET_UVS_MODE,
+};
+
struct ltr390_data {
struct regmap *regmap;
struct i2c_client *client;
/* Protects device from simulataneous reads */
struct mutex lock;
+ int mode;
int gain;
int int_time_us;
};
@@ -94,6 +100,28 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
return get_unaligned_le24(recieve_buffer);
}
+static int ltr390_set_mode(struct ltr390_data *data, int mode)
+{
+ if (data->mode == mode)
+ return 0;
+
+ switch (mode) {
+ case LTR390_SET_ALS_MODE:
+ regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+ break;
+
+ case LTR390_SET_UVS_MODE:
+ regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ data->mode = mode;
+ return 0;
+}
+
static int ltr390_read_raw(struct iio_dev *iio_device,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
@@ -104,17 +132,57 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
guard(mutex)(&data->lock);
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = ltr390_register_read(data, LTR390_UVS_DATA);
- if (ret < 0)
- return ret;
+ switch (chan->type) {
+ case IIO_UVINDEX:
+ ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+ if (ret < 0)
+ return ret;
+
+ ret = ltr390_register_read(data, LTR390_UVS_DATA);
+ if (ret < 0)
+ return ret;
+ break;
+
+ case IIO_INTENSITY:
+ ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+ if (ret < 0)
+ return ret;
+
+ ret = ltr390_register_read(data, LTR390_ALS_DATA);
+ if (ret < 0)
+ return ret;
+ break;
+
+ default:
+ return -EINVAL;
+ }
*val = ret;
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- *val = LTR390_WINDOW_FACTOR;
- *val2 = LTR390_COUNTS_PER_UVI;
- return IIO_VAL_FRACTIONAL;
+ switch (chan->type) {
+ case IIO_UVINDEX:
+ ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+ if (ret < 0)
+ return ret;
+
+ *val = LTR390_WINDOW_FACTOR;
+ *val2 = LTR390_COUNTS_PER_UVI;
+ return IIO_VAL_FRACTIONAL;
+
+ case IIO_INTENSITY:
+ ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+ if (ret < 0)
+ return ret;
+
+ *val = LTR390_WINDOW_FACTOR;
+ *val2 = data->gain;
+ return IIO_VAL_FRACTIONAL;
+
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_INT_TIME:
*val = data->int_time_us;
@@ -129,11 +197,23 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
-static const struct iio_chan_spec ltr390_channel = {
- .type = IIO_UVINDEX,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
- .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
- .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+static const struct iio_chan_spec ltr390_channels[] = {
+ /* UV sensor */
+ {
+ .type = IIO_UVINDEX,
+ .scan_index = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+ },
+ /* ALS sensor */
+ {
+ .type = IIO_INTENSITY,
+ .scan_index = 1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+ },
};
static int ltr390_set_gain(struct ltr390_data *data, int val)
@@ -253,12 +333,14 @@ static int ltr390_probe(struct i2c_client *client)
data->int_time_us = 100000;
/* default value of gain from pg: 16 of the datasheet */
data->gain = 3;
+ /* default mode for ltr390 is ALS mode */
+ data->mode = LTR390_SET_ALS_MODE;
mutex_init(&data->lock);
indio_dev->info = <r390_info;
- indio_dev->channels = <r390_channel;
- indio_dev->num_channels = 1;
+ indio_dev->channels = ltr390_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
indio_dev->name = "ltr390";
ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
@@ -276,8 +358,7 @@ static int ltr390_probe(struct i2c_client *client)
/* Wait for the registers to reset before proceeding */
usleep_range(1000, 2000);
- ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
- LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
+ ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
if (ret)
return dev_err_probe(dev, ret, "failed to enable the sensor\n");
--
2.43.0On Sun, 28 Jul 2024 20:49:55 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:
> Add new ALS channel and allow reading raw and scale values.
> Also provide gain and resolution configuration for ALS channel.
> Add automatic mode switching between the UVS and ALS channel
> based on which channel is being accessed.
> The default mode in which the sensor start is ALS mode.
Same issue with indent as in patch 1.
>
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
Later patches should not be in reply to the first one.
Use a cover letter.
Minor comments inline on using the enum to maintain the type of the mode
setting.
> ---
> drivers/iio/light/ltr390.c | 111 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 96 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 9f00661c3..d1a259141 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -62,11 +62,17 @@
> */
> #define LTR390_WINDOW_FACTOR 1
>
> +enum ltr390_mode {
> + LTR390_SET_ALS_MODE,
> + LTR390_SET_UVS_MODE,
> +};
> +
> struct ltr390_data {
> struct regmap *regmap;
> struct i2c_client *client;
> /* Protects device from simulataneous reads */
> struct mutex lock;
> + int mode;
enum ltr380_mode mode;
> int gain;
> int int_time_us;
> };
> @@ -94,6 +100,28 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> return get_unaligned_le24(recieve_buffer);
> }
>
> +static int ltr390_set_mode(struct ltr390_data *data, int mode)
pass in the enum lt380_mode.
> +{
> + if (data->mode == mode)
> + return 0;
> +
> + switch (mode) {
> + case LTR390_SET_ALS_MODE:
> + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> + break;
> +
> + case LTR390_SET_UVS_MODE:
> + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> + break;
> +
> + default:
> + return -EINVAL;
Should be able to drop this if passing in the enum as the compiler can see you
have cases for every value.
> + }
> +
> + data->mode = mode;
> + return 0;
> +}
> +
counts_per_uvi depends on the current value of gain and
resolution. Hence we cannot use the hardcoded value of 96.
The `counts_per_uvi` function gives the count based on the
current gain and resolution (integration time).
Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
drivers/iio/light/ltr390.c | 41 ++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index d1a259141..aceb97e3d 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -45,6 +45,8 @@
#define LTR390_UVS_MODE BIT(3)
#define LTR390_SENSOR_ENABLE BIT(1)
+#define LTR390_FRACTIONAL_PRECISION 100
+
/*
* At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
* the sensor are equal to 1 UV Index [Datasheet Page#8].
@@ -122,6 +124,19 @@ static int ltr390_set_mode(struct ltr390_data *data, int mode)
return 0;
}
+static int ltr390_counts_per_uvi(struct ltr390_data *data)
+{
+ int orig_gain = 18;
+ int orig_int_time = 400;
+ int divisor = orig_gain * orig_int_time;
+ int gain = data->gain;
+
+ int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
+ int uvi = DIV_ROUND_CLOSEST(2300 * gain * int_time_ms, divisor);
+
+ return uvi;
+}
+
static int ltr390_read_raw(struct iio_dev *iio_device,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
@@ -167,8 +182,8 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
if (ret < 0)
return ret;
- *val = LTR390_WINDOW_FACTOR;
- *val2 = LTR390_COUNTS_PER_UVI;
+ *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
+ *val2 = ltr390_counts_per_uvi(data);
return IIO_VAL_FRACTIONAL;
case IIO_INTENSITY:
@@ -202,17 +217,21 @@ static const struct iio_chan_spec ltr390_channels[] = {
{
.type = IIO_UVINDEX,
.scan_index = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
- .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SCALE)
},
/* ALS sensor */
{
.type = IIO_INTENSITY,
.scan_index = 1,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
- .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SCALE)
},
};
@@ -261,8 +280,9 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val)
return 0;
}
-static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
- const int **vals, int *type, int *length, long mask)
+static int ltr390_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length, long mask)
{
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -280,8 +300,9 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con
}
}
-static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
- int val, int val2, long mask)
+static int ltr390_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
{
struct ltr390_data *data = iio_priv(indio_dev);
--
2.43.0On Sun, 28 Jul 2024 20:49:56 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:
> counts_per_uvi depends on the current value of gain and
> resolution. Hence we cannot use the hardcoded value of 96.
> The `counts_per_uvi` function gives the count based on the
> current gain and resolution (integration time).
Fix the indent of this description and rewrap to take advantage of up
to 75 chars.
Some unrelated changes in this patch make it look like a lot more than
the relatively small 'real' change. Make sure those are gone
for v3. If you feel like adding a patch 4 that makes the white
space cleanup, then that is fine as well.
>
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
> drivers/iio/light/ltr390.c | 41 ++++++++++++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index d1a259141..aceb97e3d 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -45,6 +45,8 @@
> #define LTR390_UVS_MODE BIT(3)
> #define LTR390_SENSOR_ENABLE BIT(1)
>
> +#define LTR390_FRACTIONAL_PRECISION 100
> +
> /*
> * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
> * the sensor are equal to 1 UV Index [Datasheet Page#8].
> @@ -122,6 +124,19 @@ static int ltr390_set_mode(struct ltr390_data *data, int mode)
> return 0;
> }
>
> +static int ltr390_counts_per_uvi(struct ltr390_data *data)
> +{
> + int orig_gain = 18;
> + int orig_int_time = 400;
> + int divisor = orig_gain * orig_int_time;
> + int gain = data->gain;
> +
> + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
> + int uvi = DIV_ROUND_CLOSEST(2300 * gain * int_time_ms, divisor);
return DIV_ROUND_CLOSEST()
I assume maths is done like this to avoid overflow? e.g. why not
return DIV_ROUND_CLOSEST(23 * gain * data->int_time_us, 10 * divisor)
which I think is the same 'maths' as long as 23 * gain * data->int_time_us < 2^31 - 1
If you are avoiding overflow, then add a comment on that so it doesn't get
accidentally broken by a future simplification.
> +
> + return uvi;
> +}
> +
> static int ltr390_read_raw(struct iio_dev *iio_device,
> struct iio_chan_spec const *chan, int *val,
> int *val2, long mask)
> @@ -167,8 +182,8 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
> if (ret < 0)
> return ret;
>
> - *val = LTR390_WINDOW_FACTOR;
> - *val2 = LTR390_COUNTS_PER_UVI;
> + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> + *val2 = ltr390_counts_per_uvi(data);
> return IIO_VAL_FRACTIONAL;
>
> case IIO_INTENSITY:
> @@ -202,17 +217,21 @@ static const struct iio_chan_spec ltr390_channels[] = {
> {
> .type = IIO_UVINDEX,
> .scan_index = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> - .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SCALE)
> },
> /* ALS sensor */
> {
> .type = IIO_INTENSITY,
> .scan_index = 1,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> - .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SCALE)
All unrelated chagnes.
> },
> };
>
> @@ -261,8 +280,9 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val)
> return 0;
> }
>
> -static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> - const int **vals, int *type, int *length, long mask)
> +static int ltr390_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length, long mask)
Unrelated change.
> {
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> @@ -280,8 +300,9 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con
> }
> }
>
> -static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> - int val, int val2, long mask)
> +static int ltr390_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
Unrelted change. Get rid of this for v3.
> {
> struct ltr390_data *data = iio_priv(indio_dev);
>
© 2016 - 2025 Red Hat, Inc.