[PATCH v2 0/4] Threshold event and Sampling freq support for LTR390

[PATCH v2 0/4] Threshold event and Sampling freq support for LTR390

[Abhash Jha]

Hello,

The first patch adds support for configuring the Sampling frequency of
the sensor. The available values for the sampling freqeuncy are provided
by the `read_avail` callback and they are in miliHertz.

Then the second patch adds support for suspending and resuming
the sensor by providing the necessary callbacks. And registering
the ops with the driver.

The third patch in the series adds support for Threshold events and interrupts.
Exposed rising and falling threshold events for both the channels. The events
can be configured via the write_event_config callback. The desired rising or falling
threshold value can be written to from userspace.

The fourth patch adds support for threshold interrupt persistance.
It triggers when the UVS/ALS data is out of thresholds for a specific number
of consecutive measurements.
Exposed the IIO_EV_INFO_PERIOD attribute by which userspace can set the persistance
value in miliseconds. The persistance period should be greater than or equal
to the sampling period.

Changes in v2:
- Added "linux/irq.h" include to fix `-Wimplicit-function-declaration`.
- The above error was pointed during testing by kernel-test-robot

Thanks,
Abhash


Abhash Jha (4):
  iio: light: ltr390: Added configurable sampling frequency support
  iio: light: ltr390: Suspend and Resume support
  iio: light: ltr390: Interrupts and threshold event support
  iio: light: ltr390: Add interrupt persistance support

 drivers/iio/light/ltr390.c | 363 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 361 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] iio: light: ltr390: Added configurable sampling frequency support

[Abhash Jha]

Provied configurable sampling frequency(Measurement rate) support.
Also exposed the available sampling frequency values using read_avail
callback.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 68 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 7e58b50f3..73ef4a5a0 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -39,6 +39,7 @@
 
 #define LTR390_PART_NUMBER_ID		0xb
 #define LTR390_ALS_UVS_GAIN_MASK	0x07
+#define LTR390_ALS_UVS_MEAS_RATE_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))
 
@@ -87,6 +88,18 @@ static const struct regmap_config ltr390_regmap_config = {
 	.val_bits = 8,
 };
 
+/* Sampling frequency is in mili Hz and mili Seconds */
+static const int ltr390_samp_freq_table[][2] = {
+		[0] = {40000, 25},
+		[1] = {20000, 50},
+		[2] = {10000, 100},
+		[3] = {5000, 200},
+		[4] = {2000, 500},
+		[5] = {1000, 1000},
+		[6] = {500, 2000},
+		[7] = {500, 2000}
+};
+
 static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
 {
 	struct device *dev = &data->client->dev;
@@ -135,6 +148,18 @@ static int ltr390_counts_per_uvi(struct ltr390_data *data)
 	return DIV_ROUND_CLOSEST(23 * data->gain * data->int_time_us, 10 * orig_gain * orig_int_time);
 }
 
+static int ltr390_get_samp_freq(struct ltr390_data *data)
+{
+	int ret, value;
+
+	ret = regmap_read(data->regmap, LTR390_ALS_UVS_MEAS_RATE, &value);
+	if (ret < 0)
+		return ret;
+	value &= LTR390_ALS_UVS_MEAS_RATE_MASK;
+
+	return ltr390_samp_freq_table[value][0];
+}
+
 static int ltr390_read_raw(struct iio_dev *iio_device,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
@@ -191,6 +216,10 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 		*val = data->int_time_us;
 		return IIO_VAL_INT;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = ltr390_get_samp_freq(data);
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -199,6 +228,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 /* 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 int ltr390_freq_map[] = { 40000, 20000, 10000, 5000, 2000, 1000, 500, 500 };
 
 static const struct iio_chan_spec ltr390_channels[] = {
 	/* UV sensor */
@@ -206,16 +236,18 @@ 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_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
 	},
 	/* ALS sensor */
 	{
 		.type = IIO_LIGHT,
 		.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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
 	},
 };
 
@@ -264,6 +296,27 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val)
 	return -EINVAL;
 }
 
+static int ltr390_set_samp_freq(struct ltr390_data *data, int val)
+{
+	int ret, idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(ltr390_samp_freq_table); idx++) {
+		if (ltr390_samp_freq_table[idx][0] != val)
+			continue;
+
+		guard(mutex)(&data->lock);
+		ret = regmap_update_bits(data->regmap,
+					LTR390_ALS_UVS_MEAS_RATE,
+					LTR390_ALS_UVS_MEAS_RATE_MASK, idx);
+		if (ret)
+			return ret;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 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)
 {
@@ -278,6 +331,11 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con
 		*type = IIO_VAL_INT;
 		*vals = ltr390_int_time_map_us;
 		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*length = ARRAY_SIZE(ltr390_freq_map);
+		*type = IIO_VAL_INT;
+		*vals = ltr390_freq_map;
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
@@ -301,6 +359,12 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
 
 		return ltr390_set_int_time(data, val);
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (val2 != 0)
+			return -EINVAL;
+
+		return ltr390_set_samp_freq(data, val);
+
 	default:
 		return -EINVAL;
 	}
-- 
2.43.0

[PATCH v2 2/4] iio: light: ltr390: Suspend and Resume support

[Abhash Jha]

Added support for suspend and resume PM ops.
We suspend the sensor by clearing the ALS_UVS_EN bit in the MAIN CONTROL
register. And we resume it by setting that bit.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 73ef4a5a0..c4ff26d68 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -432,6 +432,24 @@ static int ltr390_probe(struct i2c_client *client)
 	return devm_iio_device_register(dev, indio_dev);
 }
 
+static int ltr390_suspend(struct device *dev)
+{
+	struct ltr390_data *data = iio_priv(i2c_get_clientdata(
+						to_i2c_client(dev)));
+
+	return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
+}
+
+static int ltr390_resume(struct device *dev)
+{
+	struct ltr390_data *data = iio_priv(i2c_get_clientdata(
+						to_i2c_client(dev)));
+
+	return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ltr390_pm_ops, ltr390_suspend, ltr390_resume);
+
 static const struct i2c_device_id ltr390_id[] = {
 	{ "ltr390" },
 	{ /* Sentinel */ }
@@ -448,6 +466,7 @@ static struct i2c_driver ltr390_driver = {
 	.driver = {
 		.name = "ltr390",
 		.of_match_table = ltr390_of_table,
+		.pm	= pm_sleep_ptr(&ltr390_pm_ops),
 	},
 	.probe = ltr390_probe,
 	.id_table = ltr390_id,
-- 
2.43.0

[PATCH v2 3/4] iio: light: ltr390: Interrupts and threshold event support

[Abhash Jha]

Added support for threshold events for both the ALS and UVI channels.
The events are reported when the threshold interrupt is triggered. Both
rising and falling threshold types are supported.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 222 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 220 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index c4ff26d68..8a8a118ca 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -24,8 +24,11 @@
 #include <linux/mutex.h>
 #include <linux/regmap.h>
 #include <linux/bitfield.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/events.h>
 
 #include <asm/unaligned.h>
 
@@ -33,9 +36,12 @@
 #define LTR390_ALS_UVS_MEAS_RATE	0x04
 #define LTR390_ALS_UVS_GAIN		0x05
 #define LTR390_PART_ID			0x06
+#define LTR390_MAIN_STATUS		0x07
 #define LTR390_ALS_DATA			0x0D
 #define LTR390_UVS_DATA			0x10
 #define LTR390_INT_CFG			0x19
+#define LTR390_THRESH_UP		0x21
+#define LTR390_THRESH_LOW		0x24
 
 #define LTR390_PART_NUMBER_ID		0xb
 #define LTR390_ALS_UVS_GAIN_MASK	0x07
@@ -46,6 +52,8 @@
 #define LTR390_SW_RESET	      BIT(4)
 #define LTR390_UVS_MODE	      BIT(3)
 #define LTR390_SENSOR_ENABLE  BIT(1)
+#define LTR390_LS_INT_EN      BIT(2)
+#define LTR390_LS_INT_SEL_UVS BIT(5)
 
 #define LTR390_FRACTIONAL_PRECISION 100
 
@@ -230,6 +238,22 @@ static const int ltr390_int_time_map_us[] = { 400000, 200000, 100000, 50000, 250
 static const int ltr390_gain_map[] = { 1, 3, 6, 9, 18 };
 static const int ltr390_freq_map[] = { 40000, 20000, 10000, 5000, 2000, 1000, 500, 500 };
 
+static const struct iio_event_spec ltr390_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	}
+};
+
 static const struct iio_chan_spec ltr390_channels[] = {
 	/* UV sensor */
 	{
@@ -238,7 +262,9 @@ static const struct iio_chan_spec ltr390_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
-						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
+						| BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.event_spec = ltr390_event_spec,
+		.num_event_specs = ARRAY_SIZE(ltr390_event_spec),
 	},
 	/* ALS sensor */
 	{
@@ -247,7 +273,9 @@ static const struct iio_chan_spec ltr390_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
-						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
+						| BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.event_spec = ltr390_event_spec,
+		.num_event_specs = ARRAY_SIZE(ltr390_event_spec),
 	},
 };
 
@@ -370,12 +398,186 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
 	}
 }
 
+static int ltr390_read_threshold(const struct iio_dev *indio_dev,
+				enum iio_event_direction dir,
+				int *val, int *val2)
+{
+	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = ltr390_register_read(data, LTR390_THRESH_UP);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+
+	case IIO_EV_DIR_FALLING:
+		ret = ltr390_register_read(data, LTR390_THRESH_LOW);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltr390_write_threshold(struct iio_dev *indio_dev,
+				enum iio_event_direction dir,
+				int val, int val2)
+{
+	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&data->lock);
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = regmap_bulk_write(data->regmap,
+					LTR390_THRESH_UP,
+					&val, 3);
+		return ret;
+	case IIO_EV_DIR_FALLING:
+		ret = regmap_bulk_write(data->regmap,
+					LTR390_THRESH_LOW,
+					&val, 3);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltr390_read_event_value(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int *val, int *val2)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		return ltr390_read_threshold(indio_dev, dir, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltr390_write_event_value(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (val2 != 0)
+			return -EINVAL;
+
+		return ltr390_write_threshold(indio_dev, dir, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltr390_read_event_config(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir)
+{
+	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret, status;
+
+	ret = regmap_read(data->regmap, LTR390_INT_CFG, &status);
+	if (ret < 0)
+		return ret;
+
+	return status & LTR390_LS_INT_EN;
+}
+
+static int ltr390_write_event_config(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				int state)
+{
+	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (state != 1  && state != 0)
+		return -EINVAL;
+
+	if (state == 0)
+		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
+
+	guard(mutex)(&data->lock);
+	ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
+	if (ret < 0)
+		return ret;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+		if (ret < 0)
+			return ret;
+
+		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+
+	case IIO_UVINDEX:
+		ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+		if (ret < 0)
+			return ret;
+
+		return regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+
+	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,
+	.read_event_value = ltr390_read_event_value,
+	.read_event_config = ltr390_read_event_config,
+	.write_event_value = ltr390_write_event_value,
+	.write_event_config = ltr390_write_event_config,
 };
 
+static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret, status;
+
+	/* Reading the status register to clear the interrupt flag, Datasheet pg: 17*/
+	ret = regmap_read(data->regmap, LTR390_MAIN_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	switch (data->mode) {
+	case LTR390_SET_ALS_MODE:
+		iio_push_event(indio_dev,
+			IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+			IIO_EV_TYPE_THRESH,
+			IIO_EV_DIR_EITHER),
+			iio_get_time_ns(indio_dev));
+		break;
+
+	case LTR390_SET_UVS_MODE:
+		iio_push_event(indio_dev,
+			IIO_UNMOD_EVENT_CODE(IIO_UVINDEX, 0,
+			IIO_EV_TYPE_THRESH,
+			IIO_EV_DIR_EITHER),
+			iio_get_time_ns(indio_dev));
+		break;
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int ltr390_probe(struct i2c_client *client)
 {
 	struct ltr390_data *data;
@@ -429,6 +631,22 @@ static int ltr390_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
 
+	if (client->irq) {
+		int irq_flags = irq_get_trigger_type(client->irq);
+
+		if (!irq_flags)
+			irq_flags = IRQF_TRIGGER_FALLING;
+
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, ltr390_interrupt_handler,
+						irq_flags | IRQF_ONESHOT,
+						"ltr390_thresh_event", indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "request irq (%d) failed\n", client->irq);
+			return ret;
+		}
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 
-- 
2.43.0

[PATCH v2 4/4] iio: light: ltr390: Add interrupt persistance support

[Abhash Jha]

Added support to configure the threshold interrupt persistance value by
providing IIO_EV_INFO_PERIOD attribute. The value written to the
attribute should be in miliseconds and should be greater than the
sampling rate of the sensor.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 66 +++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 8a8a118ca..9706105c6 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -40,6 +40,7 @@
 #define LTR390_ALS_DATA			0x0D
 #define LTR390_UVS_DATA			0x10
 #define LTR390_INT_CFG			0x19
+#define LTR390_INT_PST			0x1A
 #define LTR390_THRESH_UP		0x21
 #define LTR390_THRESH_LOW		0x24
 
@@ -48,6 +49,8 @@
 #define LTR390_ALS_UVS_MEAS_RATE_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_INT_PST_MASK		0xF0
+#define LTR390_INT_PST_VAL(x)		FIELD_PREP(LTR390_INT_PST_MASK, (x))
 
 #define LTR390_SW_RESET	      BIT(4)
 #define LTR390_UVS_MODE	      BIT(3)
@@ -79,6 +82,11 @@ enum ltr390_mode {
 	LTR390_SET_UVS_MODE,
 };
 
+enum ltr390_meas_rate {
+	LTR390_GET_FREQ,
+	LTR390_GET_PERIOD,
+};
+
 struct ltr390_data {
 	struct regmap *regmap;
 	struct i2c_client *client;
@@ -156,7 +164,7 @@ static int ltr390_counts_per_uvi(struct ltr390_data *data)
 	return DIV_ROUND_CLOSEST(23 * data->gain * data->int_time_us, 10 * orig_gain * orig_int_time);
 }
 
-static int ltr390_get_samp_freq(struct ltr390_data *data)
+static int ltr390_get_samp_freq_or_period(struct ltr390_data *data, enum ltr390_meas_rate option)
 {
 	int ret, value;
 
@@ -165,7 +173,7 @@ static int ltr390_get_samp_freq(struct ltr390_data *data)
 		return ret;
 	value &= LTR390_ALS_UVS_MEAS_RATE_MASK;
 
-	return ltr390_samp_freq_table[value][0];
+	return ltr390_samp_freq_table[value][option];
 }
 
 static int ltr390_read_raw(struct iio_dev *iio_device,
@@ -225,7 +233,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = ltr390_get_samp_freq(data);
+		*val = ltr390_get_samp_freq_or_period(data, LTR390_GET_FREQ);
 		return IIO_VAL_INT;
 
 	default:
@@ -250,7 +258,8 @@ static const struct iio_event_spec ltr390_event_spec[] = {
 	}, {
 		.type = IIO_EV_TYPE_THRESH,
 		.dir = IIO_EV_DIR_EITHER,
-		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				BIT(IIO_EV_INFO_PERIOD),
 	}
 };
 
@@ -398,6 +407,44 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
 	}
 }
 
+static int ltr390_read_intr_prst(struct ltr390_data *data, int *val)
+{
+	int ret, prst, samp_period;
+
+	samp_period = ltr390_get_samp_freq_or_period(data, LTR390_GET_PERIOD);
+	ret = regmap_read(data->regmap, LTR390_INT_PST, &prst);
+	if (ret < 0)
+		return ret;
+	*val = prst * samp_period;
+
+	return IIO_VAL_INT;
+}
+
+static int ltr390_write_intr_prst(struct ltr390_data *data, int val)
+{
+	int ret, samp_period, new_val;
+
+	samp_period = ltr390_get_samp_freq_or_period(data, LTR390_GET_PERIOD);
+
+	/* persist period should be greater than or equal to samp period */
+	if (val < samp_period)
+		return -EINVAL;
+
+	new_val = DIV_ROUND_UP(val, samp_period);
+	if (new_val < 0 || new_val > 0x0f)
+		return -EINVAL;
+
+	guard(mutex)(&data->lock);
+	ret = regmap_update_bits(data->regmap,
+				LTR390_INT_PST,
+				LTR390_INT_PST_MASK,
+				LTR390_INT_PST_VAL(new_val));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ltr390_read_threshold(const struct iio_dev *indio_dev,
 				enum iio_event_direction dir,
 				int *val, int *val2)
@@ -458,6 +505,10 @@ static int ltr390_read_event_value(struct iio_dev *indio_dev,
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
 		return ltr390_read_threshold(indio_dev, dir, val, val2);
+
+	case IIO_EV_INFO_PERIOD:
+		return ltr390_read_intr_prst(iio_priv(indio_dev), val);
+
 	default:
 		return -EINVAL;
 	}
@@ -476,6 +527,13 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		return ltr390_write_threshold(indio_dev, dir, val, val2);
+
+	case IIO_EV_INFO_PERIOD:
+		if (val2 != 0)
+			return -EINVAL;
+
+		return ltr390_write_intr_prst(iio_priv(indio_dev), val);
+
 	default:
 		return -EINVAL;
 	}
-- 
2.43.0

Re: [PATCH v2 1/4] iio: light: ltr390: Added configurable sampling frequency support

[Jonathan Cameron]

On Tue, 10 Sep 2024 10:20:26 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Provied configurable sampling frequency(Measurement rate) support.
Spell check: Provide

> Also exposed the available sampling frequency values using read_avail
> callback.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
Hi Abhash,

A few minor comments inline and an (optional) request to cleanup
the mask definitions in the existing code.
> ---
>  drivers/iio/light/ltr390.c | 68 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 7e58b50f3..73ef4a5a0 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -39,6 +39,7 @@
>  
>  #define LTR390_PART_NUMBER_ID		0xb
>  #define LTR390_ALS_UVS_GAIN_MASK	0x07
> +#define LTR390_ALS_UVS_MEAS_RATE_MASK	0x07
These masks should be converted to GENMASK().
If you don't mind doing it a precursor patch to do so
would be nice to have.

However whether or not you cleanup existing mask definitions,
please use GENMASK() for this new one.

>  #define LTR390_ALS_UVS_INT_TIME_MASK	0x70
>  #define LTR390_ALS_UVS_INT_TIME(x)	FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
>  
> @@ -87,6 +88,18 @@ static const struct regmap_config ltr390_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> +/* Sampling frequency is in mili Hz and mili Seconds */
> +static const int ltr390_samp_freq_table[][2] = {
> +		[0] = {40000, 25},
I'm trying to slowly get IIO to standardise strongly around
		[0] = { 4000, 25 },

etc.  So space after { and before }
> +		[1] = {20000, 50},
> +		[2] = {10000, 100},
> +		[3] = {5000, 200},
> +		[4] = {2000, 500},
> +		[5] = {1000, 1000},
> +		[6] = {500, 2000},
> +		[7] = {500, 2000}

Add a trailing comma.  Sure we probably will never get any more entries
but it isn't a terminator entry so convention is put the comma anyway.

> +};
> +
>  static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
>  {
>  	struct device *dev = &data->client->dev;
> @@ -135,6 +148,18 @@ static int ltr390_counts_per_uvi(struct ltr390_data *data)
>  	return DIV_ROUND_CLOSEST(23 * data->gain * data->int_time_us, 10 * orig_gain * orig_int_time);
>  }
>  
> +static int ltr390_get_samp_freq(struct ltr390_data *data)
> +{
> +	int ret, value;
> +
> +	ret = regmap_read(data->regmap, LTR390_ALS_UVS_MEAS_RATE, &value);
> +	if (ret < 0)
> +		return ret;
> +	value &= LTR390_ALS_UVS_MEAS_RATE_MASK;

FIELD_GET() preferred because then the reader doesn't have to check
if this mask includes the LSB.  It slightly helps review and compiler
will get rid of the shift by nothing anyway.

> +
> +	return ltr390_samp_freq_table[value][0];
> +}
> +
>  static int ltr390_read_raw(struct iio_dev *iio_device,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
> @@ -191,6 +216,10 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  		*val = data->int_time_us;
>  		return IIO_VAL_INT;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = ltr390_get_samp_freq(data);
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -199,6 +228,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  /* 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 int ltr390_freq_map[] = { 40000, 20000, 10000, 5000, 2000, 1000, 500, 500 };
>  
>  static const struct iio_chan_spec ltr390_channels[] = {
>  	/* UV sensor */
> @@ -206,16 +236,18 @@ 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_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
Obviously a long line above, but | should generally be on that previous line.
Probably best to reformat it as
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
						     BIT(IIO_CHAN_INFO_SCALE) |
						     BIT(IIO_CHAN_INFO_SAMP_FREQ),

Note should have always had a trailing comma.  Add that whilst here.

	
>  	},
>  	/* ALS sensor */
>  	{
>  		.type = IIO_LIGHT,
>  		.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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +						| BIT(IIO_CHAN_INFO_SAMP_FREQ)
>  	},
>  };
>  
> @@ -264,6 +296,27 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val)
>  	return -EINVAL;
>  }
>  
> +static int ltr390_set_samp_freq(struct ltr390_data *data, int val)
> +{
> +	int ret, idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(ltr390_samp_freq_table); idx++) {
> +		if (ltr390_samp_freq_table[idx][0] != val)
> +			continue;
> +
> +		guard(mutex)(&data->lock);
> +		ret = regmap_update_bits(data->regmap,
> +					LTR390_ALS_UVS_MEAS_RATE,
> +					LTR390_ALS_UVS_MEAS_RATE_MASK, idx);

		return regmap_update_bits()

is the same thing as what you have here.


> +		if (ret)
> +			return ret;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +

Re: [PATCH v2 2/4] iio: light: ltr390: Suspend and Resume support

[Jonathan Cameron]

On Tue, 10 Sep 2024 10:20:27 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Added support for suspend and resume PM ops.
> We suspend the sensor by clearing the ALS_UVS_EN bit in the MAIN CONTROL
> register. And we resume it by setting that bit.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
>  drivers/iio/light/ltr390.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 73ef4a5a0..c4ff26d68 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -432,6 +432,24 @@ static int ltr390_probe(struct i2c_client *client)
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> +static int ltr390_suspend(struct device *dev)
> +{
> +	struct ltr390_data *data = iio_priv(i2c_get_clientdata(
> +						to_i2c_client(dev)));
That's somewhat ugly.  I'd jsut have another local variable.
Can also use the access directly from the dev, not via the i2c_client
(it's the same data either way and common practice to get it directly from
dev).

	struct iio_dev *indio_dev = dev_get_drvdata(dev);

> +
> +	return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);

Lines is a little long: wrap after CTRL,

> +}
> +
> +static int ltr390_resume(struct device *dev)
> +{
> +	struct ltr390_data *data = iio_priv(i2c_get_clientdata(
> +						to_i2c_client(dev)));
As above.
> +
> +	return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ltr390_pm_ops, ltr390_suspend, ltr390_resume);
> +
>  static const struct i2c_device_id ltr390_id[] = {
>  	{ "ltr390" },
>  	{ /* Sentinel */ }
> @@ -448,6 +466,7 @@ static struct i2c_driver ltr390_driver = {
>  	.driver = {
>  		.name = "ltr390",
>  		.of_match_table = ltr390_of_table,
> +		.pm	= pm_sleep_ptr(&ltr390_pm_ops),
>  	},
>  	.probe = ltr390_probe,
>  	.id_table = ltr390_id,

Re: [PATCH v2 3/4] iio: light: ltr390: Interrupts and threshold event support

[Jonathan Cameron]

On Tue, 10 Sep 2024 10:20:28 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Added support for threshold events for both the ALS and UVI channels.
> The events are reported when the threshold interrupt is triggered. Both
> rising and falling threshold types are supported.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
Hi Abhash,

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/light/ltr390.c | 222 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 220 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index c4ff26d68..8a8a118ca 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -24,8 +24,11 @@

>  
> @@ -370,12 +398,186 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
>  	}
>  }
>  
> +static int ltr390_read_threshold(const struct iio_dev *indio_dev,
> +				enum iio_event_direction dir,

Alignment doesn't look quite right.  Is it off one space?
That is, e should be below c.  Hard to tell this in a diff so I
maybe wrong about it being off.


> +				int *val, int *val2)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = ltr390_register_read(data, LTR390_THRESH_UP);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_EV_DIR_FALLING:
> +		ret = ltr390_register_read(data, LTR390_THRESH_LOW);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltr390_write_threshold(struct iio_dev *indio_dev,
> +				enum iio_event_direction dir,
> +				int val, int val2)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_bulk_write(data->regmap,
> +					LTR390_THRESH_UP,
Wrap this bit less to get nearer 80 chars.
> +					&val, 3);
> +		return ret;
> +	case IIO_EV_DIR_FALLING:
> +		ret = regmap_bulk_write(data->regmap,
> +					LTR390_THRESH_LOW,
> +					&val, 3);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int ltr390_read_event_config(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	ret = regmap_read(data->regmap, LTR390_INT_CFG, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	return status & LTR390_LS_INT_EN;

Slight preference for FIELD_GET() here which will make this 0 or 1
rather than 0 or 4 (I think?)


> +}
> +


> +static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	/* Reading the status register to clear the interrupt flag, Datasheet pg: 17*/
> +	ret = regmap_read(data->regmap, LTR390_MAIN_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (data->mode) {
> +	case LTR390_SET_ALS_MODE:
> +		iio_push_event(indio_dev,
> +			IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +			IIO_EV_TYPE_THRESH,
Align after (

> +			IIO_EV_DIR_EITHER),
> +			iio_get_time_ns(indio_dev));
> +		break;
> +
> +	case LTR390_SET_UVS_MODE:
> +		iio_push_event(indio_dev,
> +			IIO_UNMOD_EVENT_CODE(IIO_UVINDEX, 0,

Same on alignment as this is hard to read.

> +			IIO_EV_TYPE_THRESH,
> +			IIO_EV_DIR_EITHER),
> +			iio_get_time_ns(indio_dev));
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ltr390_probe(struct i2c_client *client)
>  {
>  	struct ltr390_data *data;
> @@ -429,6 +631,22 @@ static int ltr390_probe(struct i2c_client *client)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
>  
> +	if (client->irq) {
> +		int irq_flags = irq_get_trigger_type(client->irq);
> +
> +		if (!irq_flags)
> +			irq_flags = IRQF_TRIGGER_FALLING;
Don't override the firmware description. It should be correct for new drives
as probably no legacy out there. The only time we play this sort of
game is when we have to carry on supporting a wrong configuration that
'works' because the driver was setting more than it should in the past.

> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, ltr390_interrupt_handler,
> +						irq_flags | IRQF_ONESHOT,

Drop irq_flags use here.

> +						"ltr390_thresh_event", indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "request irq (%d) failed\n", client->irq);
> +			return ret;
> +		}
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  

Re: [PATCH v2 4/4] iio: light: ltr390: Add interrupt persistance support

[Jonathan Cameron]

On Tue, 10 Sep 2024 10:20:29 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Added support to configure the threshold interrupt persistance value by
> providing IIO_EV_INFO_PERIOD attribute. The value written to the
> attribute should be in miliseconds and should be greater than the
> sampling rate of the sensor.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
>  drivers/iio/light/ltr390.c | 66 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 8a8a118ca..9706105c6 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -40,6 +40,7 @@

>  
> -static int ltr390_get_samp_freq(struct ltr390_data *data)
> +static int ltr390_get_samp_freq_or_period(struct ltr390_data *data, enum ltr390_meas_rate option)

Wrap this as the line is getting too long.

Otherwise LGTM

Thanks,

Jonathan

Re: [PATCH v2 1/4] iio: light: ltr390: Added configurable sampling frequency support

[Abhash jha]

> Hi Abhash,
>
> A few minor comments inline and an (optional) request to cleanup
> the mask definitions in the existing code.
> >
> >  #define LTR390_PART_NUMBER_ID                0xb
> >  #define LTR390_ALS_UVS_GAIN_MASK     0x07
> > +#define LTR390_ALS_UVS_MEAS_RATE_MASK        0x07
> These masks should be converted to GENMASK().
> If you don't mind doing it a precursor patch to do so
> would be nice to have.
>
Can I do the mask to GENMASK conversion in an additional cleanup patch
at the end? The patch would clean up stuff related to newlines and such.

Meanwhile I would use GENMASK for the new ones

Thanks,
Abhash

Re: [PATCH v2 1/4] iio: light: ltr390: Added configurable sampling frequency support

[Jonathan Cameron]

On Sat, 14 Sep 2024 22:57:24 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:

> > Hi Abhash,
> >
> > A few minor comments inline and an (optional) request to cleanup
> > the mask definitions in the existing code.  
> > >
> > >  #define LTR390_PART_NUMBER_ID                0xb
> > >  #define LTR390_ALS_UVS_GAIN_MASK     0x07
> > > +#define LTR390_ALS_UVS_MEAS_RATE_MASK        0x07  
> > These masks should be converted to GENMASK().
> > If you don't mind doing it a precursor patch to do so
> > would be nice to have.
> >  
> Can I do the mask to GENMASK conversion in an additional cleanup patch
> at the end? The patch would clean up stuff related to newlines and such.
Doing it at the end is fine, but remember one patch per type of cleanup.
So if you have whitespace stuff and genmask, then two patches.
> 
> Meanwhile I would use GENMASK for the new ones
Perfect. Thanks,

Jonathan

> 
> Thanks,
> Abhash