Added AMS tsl2591 driver implementation

Device tree binding for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

Device tree binding for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

Signed-off-by Joe Sandom <[email protected]>

Reviewed-by Rob Herring <[email protected]>

---
Changes in v8:
- No changes

Notes:
- Re-submitted to align the version with part 1 of the patch series

 .../bindings/iio/light/amstaos,tsl2591.yaml   | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml

Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml new file +50 -1

new file mode 100644
index 000000000000..596a3bc770f4
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
+
+maintainers:
+  - Joe Sandom <[email protected]>
+
+description: |
+  AMS/TAOS TSL2591 is a very-high sensitivity
+  light-to-digital converter that transforms light intensity into a digital
+  signal.
+
+properties:
+  compatible:
+    const: amstaos,tsl2591
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description:
+      Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING.
+      interrupt is used to detect if the light intensity has fallen below
+      or reached above the configured threshold values.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tsl2591@29 {
+            compatible = "amstaos,tsl2591";
+            reg = <0x29>;
+            interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
+       };
+    };
+...
--

2.17.1

Driver implementation for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

+ als_lower_l = als_lower_threshold;

Quoted 1 line

0, but it's up to you.

+ als_lower_h = als_lower_threshold >> 8;

Quoted 9 lines · Joe Sandom

Driver implementation for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

Quoted 2 lines

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

Hmm... It's v8 and the subject line is wrongly formatted. Please add the corresponding prefix "iio: light: ..."

Otherwise it's in very good shape.

...

drivers/iio/light/tsl2591.c line 68

Quoted 8 lines

drivers/iio/light/tsl2591.c new file +7 -0

+/* TSL2591 enable register definitions */
+#define TSL2591_PWR_ON              0x01
+#define TSL2591_PWR_OFF             0x00
 
+#define TSL2591_ENABLE_ALS          0x02
+#define TSL2591_ENABLE_ALS_INT      0x10
+#define TSL2591_ENABLE_SLEEP_INT    0x40
+#define TSL2591_ENABLE_NP_INT       0x80

Is it a bitfield?

...

Quoted 5 lines

+ als_lower_l = als_lower_threshold;

Quoted 1 line

0, but it's up to you.

+ als_lower_h = als_lower_threshold >> 8;

...

drivers/iio/light/tsl2591.c line 588

Quoted 2 lines

drivers/iio/light/tsl2591.c new file +2 -0

+       als_upper_l = als_upper_threshold;
+       als_upper_h = als_upper_threshold >> 8;

Ditto.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Driver implementation for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

Datasheet: https://ams.com/tsl25911#tab/documents

Signed-off-by Joe Sandom <[email protected]>

---
Changes in v8;
- tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not necessary
- tsl2591_read_event_value() and tsl2591_write_event_value() - break instead of goto in default case
- Introduced tsl2591_info_no_irq iio_info structure which is used when the interrupt is disabled. This variant doesn't expose anything that can't be used when the interrupt is disabled
- Corrected ordering of includes for <asm/unaligned.h>
- Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME
- Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC
- Changed TSL2591_STS_ALS* defines to all use BIT for consistency
- Use (BIT(4) - 1) instead of value from list for TSL2591_PRST_ALS_INT_CYCLE_MAX
- Cleaned up a few blank lines that were unneccesary
- Use sysfs_emit() instead of snprintf() in tsl2591_in_illuminance_period_available_show()
- Use err_unlock branch instead of err to be clear on mutex_unlock cases
- Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent with other functions

NOTE;
- tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold() forward declaration needed because tsl2591_set_als_lower_threshold() calls tsl2591_set_als_upper_threshold() and vice versa

 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/Makefile  |    1 +
 drivers/iio/light/tsl2591.c | 1227 +++++++++++++++++++++++++++++++++++
 3 files changed, 1239 insertions(+)
 create mode 100644 drivers/iio/light/tsl2591.c

drivers/iio/light/Makefile +1 -0

index ea376deaca54..d10912faf964 100644
@@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI)	+= st_uvis25_spi.o
 obj-$(CONFIG_TCS3414)		+= tcs3414.o
 obj-$(CONFIG_TCS3472)		+= tcs3472.o
 obj-$(CONFIG_TSL2583)		+= tsl2583.o
+obj-$(CONFIG_TSL2591)		+= tsl2591.o
 obj-$(CONFIG_TSL2772)		+= tsl2772.o
 obj-$(CONFIG_TSL4531)		+= tsl4531.o
 obj-$(CONFIG_US5182D)		+= us5182d.o

drivers/iio/light/tsl2591.c new file +0 -0

new file mode 100644
index 000000000000..4f9c5e19ee35

drivers/iio/light/tsl2591.c new file +18 -0

+ *
+ * Datasheet: https://ams.com/tsl25911#tab/documents
+ *
+ * Device driver for the TAOS TSL2591. This is a very-high sensitivity
+ * light-to-digital converter that transforms light intensity into a digital
+ * signal.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>

drivers/iio/light/tsl2591.c new file +3 -0

+/* ADC integration time, time in ms to field value */
+#define TSL2591_ATIME_TO_MSEC(x) (((x) / 100) - 1)
+

drivers/iio/light/tsl2591.c new file +7 -0

+
+/*
+ * Period table is ALS persist cycle x integration time setting
+ * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
+ * ALS cycles: 1, 2, 3, 5, 10, 20, 25, 30, 35, 40, 45, 50, 55, 60
+ */
+static const char * const tsl2591_als_period_list[] = {

+};

drivers/iio/light/tsl2591.c new file +4 -0

+static int tsl2591_wait_adc_complete(struct tsl2591_chip *chip)
+{
+	struct tsl2591_als_settings settings = chip->als_settings;
+	struct i2c_client *client = chip->client;

drivers/iio/light/tsl2591.c new file +16 -0

+	 * for an ADC read cycle to complete. Check status after
+	 * delay for ALS valid.
+	 */
+	msleep(delay);
+
+	/* Check for status ALS valid flag for up to 100ms */
+	ret = readx_poll_timeout(tsl2591_check_als_valid, client,
+				 val, val == TSL2591_STS_VAL_HIGH_MASK,
+				 TSL2591_DELAY_PERIOD_US,
+				 TSL2591_DELAY_PERIOD_US * TSL2591_ALS_STS_VALID_COUNT);
+	if (ret)
+		dev_err(&client->dev, "Timed out waiting for valid ALS data\n");
+
+	return ret;
+}
+

drivers/iio/light/tsl2591.c new file +35 -0

+
+static int tsl2591_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct tsl2591_chip *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int ret;
+
+	pm_runtime_get_sync(&client->dev);
+
+	mutex_lock(&chip->als_mutex);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_INTENSITY) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		ret = tsl2591_read_channel_data(indio_dev, chan, val, val2);
+		if (ret < 0)
+			goto err_unlock;
+
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_LIGHT) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		ret = tsl2591_read_channel_data(indio_dev, chan, val, val2);
+		if (ret < 0)
+			break;

drivers/iio/light/tsl2591.c new file +25 -0

+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		if (chan->type != IIO_INTENSITY) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		*val = tsl2591_gain_to_multiplier(chip->als_settings.als_gain);
+		ret = IIO_VAL_INT;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+err_unlock:
+	mutex_unlock(&chip->als_mutex);
+
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+}
+

drivers/iio/light/tsl2591.c new file +38 -0

+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		int_time = tsl2591_als_time_to_fval(val);
+		if (int_time < 0) {
+			ret = int_time;
+			goto err_unlock;
+		}
+		ret = tsl2591_compatible_int_time(chip, int_time);
+		if (ret < 0)
+			goto err_unlock;
+
+		chip->als_settings.als_int_time = int_time;
+		break;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		gain = tsl2591_multiplier_to_gain(val);
+		if (gain < 0) {
+			ret = gain;
+			goto err_unlock;
+		}
+		ret = tsl2591_compatible_gain(chip, gain);
+		if (ret < 0)
+			goto err_unlock;
+
+		chip->als_settings.als_gain = gain;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	ret = tsl2591_set_als_gain_int_time(chip);
+
+err_unlock:
+	mutex_unlock(&chip->als_mutex);
+	return ret;
+}
+

drivers/iio/light/tsl2591.c new file +4 -0

+
+static const struct iio_info tsl2591_info = {
+	.event_attrs = &tsl2591_event_attribute_group,
+	.read_raw = tsl2591_read_raw,

drivers/iio/light/tsl2591.c new file +5 -0

+	.read_raw = tsl2591_read_raw,
+	.write_raw = tsl2591_write_raw,
+	.read_avail = tsl2591_read_available,
+};
+

drivers/iio/light/tsl2591.c new file +18 -0

+
+static int tsl2591_probe(struct i2c_client *client)
+{
+	struct tsl2591_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_err(&client->dev,
+			"I2C smbus byte data functionality is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);

drivers/iio/light/tsl2591.c new file +61 -0

+			return -EINVAL;
+		}
+		indio_dev->info = &tsl2591_info;
+	} else {
+		indio_dev->info = &tsl2591_info_no_irq;
+	}
+
+	mutex_init(&chip->als_mutex);
+
+	ret = i2c_smbus_read_byte_data(client,
+				       TSL2591_CMD_NOP | TSL2591_DEVICE_ID);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read the device ID register\n");
+		return ret;
+	}
+	ret = FIELD_GET(TSL2591_DEVICE_ID_MASK, ret);
+	if (ret != TSL2591_DEVICE_ID_VAL) {
+		dev_err(&client->dev, "Device ID: %#04x unknown\n", ret);
+		return -EINVAL;
+	}
+
+	indio_dev->channels = tsl2591_channels;
+	indio_dev->num_channels = ARRAY_SIZE(tsl2591_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = chip->client->name;
+	chip->events_enabled = false;
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev,
+					 TSL2591_POWER_OFF_DELAY_MS);
+	pm_runtime_use_autosuspend(&client->dev);
+
+	/*
+	 * Add chip off to automatically managed path and disable runtime
+	 * power management. This ensures that the chip power management
+	 * is handled correctly on driver remove. tsl2591_chip_off() must be
+	 * added to the managed path after pm runtime is enabled and before
+	 * any error exit paths are met to ensure we're not left in a state
+	 * of pm runtime not being disabled properly.
+	 */
+	ret = devm_add_action_or_reset(&client->dev, tsl2591_chip_off,
+				       indio_dev);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = tsl2591_load_defaults(chip);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to load sensor defaults\n");
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte(client, TSL2591_CMD_SF_CALS_NPI);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to clear als irq\n");
+		return -EINVAL;
+	}
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+

Quoted 8 lines · On Fri, 16 Apr 2021 18:49:01 +0100 Joe Sandom <[email protected]> wrote:

Driver implementation for AMS/TAOS tsl2591 ambient light sensor.

This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds.

Quoted 5 lines

This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal.

Datasheet: https://ams.com/tsl25911#tab/documents

Signed-off-by Joe Sandom <[email protected]>

Hi Joe,

I was in two minds about whether to just apply this and roll in the below suggestions + those Andy made or whether to ask you to do a v9.

The naming and units things below are complex enough that I'm not 100% sure I'd get the right so please take a look and clean up those last few corners!

Thanks,

Jonathan

Quoted 27 lines

---
Changes in v8;
- tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not necessary
- tsl2591_read_event_value() and tsl2591_write_event_value() - break instead of goto in default case
- Introduced tsl2591_info_no_irq iio_info structure which is used when the interrupt is disabled. This variant doesn't expose anything that can't be used when the interrupt is disabled
- Corrected ordering of includes for <asm/unaligned.h>
- Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME
- Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC
- Changed TSL2591_STS_ALS* defines to all use BIT for consistency
- Use (BIT(4) - 1) instead of value from list for TSL2591_PRST_ALS_INT_CYCLE_MAX
- Cleaned up a few blank lines that were unneccesary
- Use sysfs_emit() instead of snprintf() in tsl2591_in_illuminance_period_available_show()
- Use err_unlock branch instead of err to be clear on mutex_unlock cases
- Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent with other functions

NOTE;
- tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold() forward declaration needed because tsl2591_set_als_lower_threshold() calls tsl2591_set_als_upper_threshold() and vice versa

 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/Makefile  |    1 +
 drivers/iio/light/tsl2591.c | 1227 +++++++++++++++++++++++++++++++++++
 3 files changed, 1239 insertions(+)
 create mode 100644 drivers/iio/light/tsl2591.c

...

Quoted 18 lines

drivers/iio/light/Makefile +1 -0

index ea376deaca54..d10912faf964 100644
@@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI)	+= st_uvis25_spi.o
 obj-$(CONFIG_TCS3414)		+= tcs3414.o
 obj-$(CONFIG_TCS3472)		+= tcs3472.o
 obj-$(CONFIG_TSL2583)		+= tsl2583.o
+obj-$(CONFIG_TSL2591)		+= tsl2591.o
 obj-$(CONFIG_TSL2772)		+= tsl2772.o
 obj-$(CONFIG_TSL4531)		+= tsl4531.o
 obj-$(CONFIG_US5182D)		+= us5182d.o

drivers/iio/light/tsl2591.c new file +0 -0

new file mode 100644
index 000000000000..4f9c5e19ee35

drivers/iio/light/tsl2591.c line 3

Quoted 3 lines

drivers/iio/light/tsl2591.c new file +3 -0

@@ -0,0 +1,1227 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Joe Sandom <[email protected]>

Update perhaps?

Quoted 18 lines

drivers/iio/light/tsl2591.c new file +18 -0

+ *
+ * Datasheet: https://ams.com/tsl25911#tab/documents
+ *
+ * Device driver for the TAOS TSL2591. This is a very-high sensitivity
+ * light-to-digital converter that transforms light intensity into a digital
+ * signal.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>

drivers/iio/light/tsl2591.c line 31

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+#include <linux/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+/* ADC integration time, field value to time in ms */
+#define TSL2591_MSEC_TO_ATIME(x) (((x) + 1) * 100)

Naming here seems backwards. I'd assume that at MSEC_TO_ATIME
function went msecs -> atime but it seems to be doing the opposite.

Quoted 3 lines

drivers/iio/light/tsl2591.c new file +3 -0

+/* ADC integration time, time in ms to field value */
+#define TSL2591_ATIME_TO_MSEC(x) (((x) / 100) - 1)
+

...

Quoted 7 lines

drivers/iio/light/tsl2591.c new file +7 -0

+
+/*
+ * Period table is ALS persist cycle x integration time setting
+ * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
+ * ALS cycles: 1, 2, 3, 5, 10, 20, 25, 30, 35, 40, 45, 50, 55, 60
+ */
+static const char * const tsl2591_als_period_list[] = {

drivers/iio/light/tsl2591.c line 186

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+	"0.1 0.2 0.3 0.5 1.0 2.0 2.5 3.0 3.5 4.0 4.5 5.0 5.5 6.0",
+	"0.2 0.4 0.6 1.0 2.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0 11.0 12.0",
+	"0.3 0.6 0.9 1.5 3.0 6.0 7.5 9.0 10.5 12.0 13.5 15.0 16.5 18.0",
+	"0.4 0.8 1.2 2.0 4.0 8.0 10.0 12.0 14.0 16.0 18.0 20.0 22.0 24.0",
+	"0.5 1.0 1.5 2.5 5.0 10.0 12.5 15.0 17.5 20.0 22.5 25.0 27.5 30.0",
+	"0.6 1.2 1.8 3.0 6.0 12.0 15.0 18.0 21.0 24.0 27.0 30.0 33.0 36.0",
+};
+
+static const int tsl2591_int_time_available[] = {
+	100, 200, 300, 400, 500, 600,

As mentioned below, these should be in seconds and I think here they are in milisecs.

Quoted 1 line

+};

...

Quoted 4 lines

drivers/iio/light/tsl2591.c new file +4 -0

+static int tsl2591_wait_adc_complete(struct tsl2591_chip *chip)
+{
+	struct tsl2591_als_settings settings = chip->als_settings;
+	struct i2c_client *client = chip->client;

drivers/iio/light/tsl2591.c line 398

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+	int delay;
+	int val;
+	int ret;
+
+	delay = TSL2591_MSEC_TO_ATIME(settings.als_int_time);
+	if (!delay)
+		return -EINVAL;
+
+	/*
+	 * Sleep for ALS integration time to allow enough time

Strangely short lines; perhaps rewrap?

Quoted 16 lines

drivers/iio/light/tsl2591.c new file +16 -0

+	 * for an ADC read cycle to complete. Check status after
+	 * delay for ALS valid.
+	 */
+	msleep(delay);
+
+	/* Check for status ALS valid flag for up to 100ms */
+	ret = readx_poll_timeout(tsl2591_check_als_valid, client,
+				 val, val == TSL2591_STS_VAL_HIGH_MASK,
+				 TSL2591_DELAY_PERIOD_US,
+				 TSL2591_DELAY_PERIOD_US * TSL2591_ALS_STS_VALID_COUNT);
+	if (ret)
+		dev_err(&client->dev, "Timed out waiting for valid ALS data\n");
+
+	return ret;
+}
+

...

Quoted 35 lines

drivers/iio/light/tsl2591.c new file +35 -0

+
+static int tsl2591_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct tsl2591_chip *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int ret;
+
+	pm_runtime_get_sync(&client->dev);
+
+	mutex_lock(&chip->als_mutex);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_INTENSITY) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		ret = tsl2591_read_channel_data(indio_dev, chan, val, val2);
+		if (ret < 0)
+			goto err_unlock;
+
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_LIGHT) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		ret = tsl2591_read_channel_data(indio_dev, chan, val, val2);
+		if (ret < 0)
+			break;

drivers/iio/light/tsl2591.c line 756

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->type != IIO_INTENSITY) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		*val = TSL2591_MSEC_TO_ATIME(chip->als_settings.als_int_time);

My gut feeling was that integration time should be in seconds, not milliseconds. Seems our documentation on this is lacking, though there are other integration_time entries that are stated as being in seconds. I haven't checked all the drivers, but the first few I looked at do have this in seconds.
The vast majority of IIO units effectively the base SI unit. Exceptions exist where we (unwisely) decided to try and match hwmon, or where the particular thing being measured is always so small in the base units that it gets silly.

Documentation/ABI/testing/sysfs-bus-iio

If you fancy improving the docs as a follow up patch to make the unit more explicit that would be great, if not I'll get to it at some point.

Quoted 25 lines

drivers/iio/light/tsl2591.c new file +25 -0

+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		if (chan->type != IIO_INTENSITY) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		*val = tsl2591_gain_to_multiplier(chip->als_settings.als_gain);
+		ret = IIO_VAL_INT;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+err_unlock:
+	mutex_unlock(&chip->als_mutex);
+
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+}
+

drivers/iio/light/tsl2591.c line 791

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+static int tsl2591_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct tsl2591_chip *chip = iio_priv(indio_dev);
+	u32 int_time;
+	u8 gain;
+	int ret;
+
+	mutex_lock(&chip->als_mutex);

It isn't particularly important but i would have been inclined to tighten the scope of this mutex lock so that it only covered the parts of this code that touch state rather than the conversions and range checks.

Up to you however on whether you bother at this stage!

Quoted 38 lines

drivers/iio/light/tsl2591.c new file +38 -0

+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		int_time = tsl2591_als_time_to_fval(val);
+		if (int_time < 0) {
+			ret = int_time;
+			goto err_unlock;
+		}
+		ret = tsl2591_compatible_int_time(chip, int_time);
+		if (ret < 0)
+			goto err_unlock;
+
+		chip->als_settings.als_int_time = int_time;
+		break;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		gain = tsl2591_multiplier_to_gain(val);
+		if (gain < 0) {
+			ret = gain;
+			goto err_unlock;
+		}
+		ret = tsl2591_compatible_gain(chip, gain);
+		if (ret < 0)
+			goto err_unlock;
+
+		chip->als_settings.als_gain = gain;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	ret = tsl2591_set_als_gain_int_time(chip);
+
+err_unlock:
+	mutex_unlock(&chip->als_mutex);
+	return ret;
+}
+

...

Quoted 4 lines

drivers/iio/light/tsl2591.c new file +4 -0

+
+static const struct iio_info tsl2591_info = {
+	.event_attrs = &tsl2591_event_attribute_group,
+	.read_raw = tsl2591_read_raw,

drivers/iio/light/tsl2591.c line 1018

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+	.write_raw = tsl2591_write_raw,
+	.read_avail = tsl2591_read_available,
+	.read_event_value = tsl2591_read_event_value,
+	.write_event_value = tsl2591_write_event_value,
+	.read_event_config = tsl2591_read_event_config,
+	.write_event_config = tsl2591_write_event_config,
+};
+
+static const struct iio_info tsl2591_info_no_irq = {
+	.event_attrs = &tsl2591_event_attribute_group,

Why have event attributes if no irq and hence no events?

Quoted 5 lines

drivers/iio/light/tsl2591.c new file +5 -0

+	.read_raw = tsl2591_read_raw,
+	.write_raw = tsl2591_write_raw,
+	.read_avail = tsl2591_read_available,
+};
+

...

Quoted 18 lines

drivers/iio/light/tsl2591.c new file +18 -0

+
+static int tsl2591_probe(struct i2c_client *client)
+{
+	struct tsl2591_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_err(&client->dev,
+			"I2C smbus byte data functionality is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);

drivers/iio/light/tsl2591.c line 1147

Quoted 10 lines

drivers/iio/light/tsl2591.c new file +10 -0

+	chip->client = client;
+	i2c_set_clientdata(client, indio_dev);
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, tsl2591_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						"tsl2591_irq", indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "IRQ request error %d\n", -ret);

dev_err_probe() as that won't print an error message if you get a deferred probe error (and so will come around again later). That could happen here if the irq chip driver hasn't loaded yet.

Quoted 61 lines

drivers/iio/light/tsl2591.c new file +61 -0

+			return -EINVAL;
+		}
+		indio_dev->info = &tsl2591_info;
+	} else {
+		indio_dev->info = &tsl2591_info_no_irq;
+	}
+
+	mutex_init(&chip->als_mutex);
+
+	ret = i2c_smbus_read_byte_data(client,
+				       TSL2591_CMD_NOP | TSL2591_DEVICE_ID);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read the device ID register\n");
+		return ret;
+	}
+	ret = FIELD_GET(TSL2591_DEVICE_ID_MASK, ret);
+	if (ret != TSL2591_DEVICE_ID_VAL) {
+		dev_err(&client->dev, "Device ID: %#04x unknown\n", ret);
+		return -EINVAL;
+	}
+
+	indio_dev->channels = tsl2591_channels;
+	indio_dev->num_channels = ARRAY_SIZE(tsl2591_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = chip->client->name;
+	chip->events_enabled = false;
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev,
+					 TSL2591_POWER_OFF_DELAY_MS);
+	pm_runtime_use_autosuspend(&client->dev);
+
+	/*
+	 * Add chip off to automatically managed path and disable runtime
+	 * power management. This ensures that the chip power management
+	 * is handled correctly on driver remove. tsl2591_chip_off() must be
+	 * added to the managed path after pm runtime is enabled and before
+	 * any error exit paths are met to ensure we're not left in a state
+	 * of pm runtime not being disabled properly.
+	 */
+	ret = devm_add_action_or_reset(&client->dev, tsl2591_chip_off,
+				       indio_dev);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = tsl2591_load_defaults(chip);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to load sensor defaults\n");
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte(client, TSL2591_CMD_SF_CALS_NPI);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to clear als irq\n");
+		return -EINVAL;
+	}
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+

...