[PATCH v2 0/2] Support for Texas Instruments OPT4001 Ambient Light Sensor

[PATCH v2 0/2] Support for Texas Instruments OPT4001 Ambient Light Sensor

[Stefan Windfeldt-Prytz]

This series adds support for Texas Instruments OPT4001 Ambient light sensor.

The light sensor has a i2c interface and supports continuous, oneshot and
interruptdriven measurements and has configurable conversion time and range.

This driver uses the sensors continuous mode so it always has a updated light
value available. The conversion time which is
 (integration time + time to set registers) which is used to configure
integration time through sysfs. The chip also has a configurable light
range which this driver sets to Auto where the chip chooses range itself
depending on previously read values.

Since the OPT4001 has different constants used to calculate lux values
depeding on packaging of the chip but uses the same device id, two compatible
string are used depending on the packaging, these are "ti,opt4001-picostar"
and "ti,opt4001-sot-5x3".

Signed-off-by: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
---
Changes in v2:
- Added text about differences of sot-5x3 and picostar
- Added irq and regulator to devicetree bindings
- Added regulator support to driver
- Switched from using .remove to devm_action_or_reset
- Removed own mutex and reenabled regmaps
- Updated name in sysfs
- Added i2c_device_id
- Rename package_const to chip_info
- Link to v1: https://lore.kernel.org/r/20230323-add-opt4001-driver-v1-0-1451dcc1bc8a@axis.com

---
Stefan Windfeldt-Prytz (2):
      dt-bindings: Document TI OPT4001 light sensor bindings
      iio: light: Add support for TI OPT4001 light sensor

 .../devicetree/bindings/iio/light/ti,opt4001.yaml  |  69 +++
 drivers/iio/light/Kconfig                          |  11 +
 drivers/iio/light/Makefile                         |   1 +
 drivers/iio/light/opt4001.c                        | 490 +++++++++++++++++++++
 4 files changed, 571 insertions(+)
---
base-commit: 60c5238813fdfbe167eb579d58172106916b8db0
change-id: 20230323-add-opt4001-driver-99b9aad69319

Best regards,
-- 
Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>

[PATCH v2 1/2] dt-bindings: Document TI OPT4001 light sensor bindings

[Stefan Windfeldt-Prytz]

Add devicetree bindings for opt4001 ambient light sensor.

Signed-off-by: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
---
 .../devicetree/bindings/iio/light/ti,opt4001.yaml  | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml
new file mode 100644
index 000000000000..43fd1a992aea
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ti,opt4001.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments OPT4001 Ambient Light Sensor
+
+maintainers:
+  - Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
+
+description: |
+  Ambient light sensor with an i2c interface.
+  Last part of compatible is for the packaging used.
+  Picostar is a 4 pinned SMT and sot-5x3 is a 8 pinned SOT.
+  Only sot-5x3 has an interrupt pin.
+  https://www.ti.com/lit/gpn/opt4001
+
+properties:
+  compatible:
+    enum:
+      - ti,opt4001-picostar
+      - ti,opt4001-sot-5x3
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,opt4001-sot-5x3
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
+    else:
+      properties:
+        interrupts: false
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@44 {
+            compatible = "ti,opt4001-sot-5x3";
+            reg = <0x44>;
+            vdd-supply = <&vdd_reg>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <28 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
+...

-- 
2.30.2

[PATCH v2 2/2] iio: light: Add support for TI OPT4001 light sensor

[Stefan Windfeldt-Prytz]

This driver uses the continuous mode of the chip and integration
time can be configured through sysfs.
The constants for calculating lux value differs between packaging
so it uses different compatible string for the two versions
"ti,opt4001-picostar" and "ti,opt4001-sot-5x3" since the device id
is the same.

Datasheet: https://www.ti.com/lit/gpn/opt4001
Signed-off-by: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
---
 drivers/iio/light/Kconfig   |  11 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/opt4001.c | 490 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 502 insertions(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..c4064fb35f1f 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -399,6 +399,17 @@ config OPT3001
 	  If built as a dynamically linked module, it will be called
 	  opt3001.
 
+config OPT4001
+	tristate "Texas Instruments OPT4001 Light Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say Y or M here, you get support for Texas Instruments
+	  OPT4001 Ambient Light Sensor.
+
+	  If built as a dynamically linked module, it will be called
+	  opt4001.
+
 config PA12203001
 	tristate "TXC PA12203001 light and proximity sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 6f23817fae6f..4e0a908948db 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_MAX44009)		+= max44009.o
 obj-$(CONFIG_NOA1305)		+= noa1305.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
+obj-$(CONFIG_OPT4001)		+= opt4001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o
 obj-$(CONFIG_RPR0521)		+= rpr0521.o
 obj-$(CONFIG_SENSORS_TSL2563)	+= tsl2563.o
diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
new file mode 100644
index 000000000000..e9f7b844b1f9
--- /dev/null
+++ b/drivers/iio/light/opt4001.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Axis Communications AB
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/opt4001
+ *
+ * Device driver for the Texas Instruments OPT4001.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+/* OPT4001 register set */
+#define OPT4001_LIGHT1_MSB    0x00
+#define OPT4001_LIGHT1_LSB    0x01
+#define OPT4001_CTRL          0x0A
+#define OPT4001_DEVICE_ID     0x11
+
+/* OPT4001 register mask */
+#define OPT4001_EXPONENT_MASK    GENMASK(15, 12)
+#define OPT4001_MSB_MASK         GENMASK(11, 0)
+#define OPT4001_LSB_MASK         GENMASK(15, 8)
+#define OPT4001_COUNTER_MASK     GENMASK(7, 4)
+#define OPT4001_CRC_MASK         GENMASK(3, 0)
+
+/* OPT4001 device id mask */
+#define OPT4001_DEVICE_ID_MASK   GENMASK(11, 0)
+
+/* OPT4001 control registers mask */
+#define OPT4001_CTRL_QWAKE_MASK          GENMASK(15, 15)
+#define OPT4001_CTRL_RANGE_MASK          GENMASK(13, 10)
+#define OPT4001_CTRL_CONV_TIME_MASK      GENMASK(9, 6)
+#define OPT4001_CTRL_OPER_MODE_MASK      GENMASK(5, 4)
+#define OPT4001_CTRL_LATCH_MASK          GENMASK(3, 3)
+#define OPT4001_CTRL_INT_POL_MASK        GENMASK(2, 2)
+#define OPT4001_CTRL_FAULT_COUNT         GENMASK(0, 1)
+
+/* OPT4001 constants */
+#define OPT4001_DEVICE_ID_VAL            0x121
+
+/* OPT4001 operating modes */
+#define OPT4001_CTRL_OPER_MODE_OFF        0x0
+#define OPT4001_CTRL_OPER_MODE_FORCED     0x1
+#define OPT4001_CTRL_OPER_MODE_ONE_SHOT   0x2
+#define OPT4001_CTRL_OPER_MODE_CONTINUOUS 0x3
+
+/* OPT4001 conversion control register definitions */
+#define OPT4001_CTRL_CONVERSION_0_6MS   0x0
+#define OPT4001_CTRL_CONVERSION_1MS     0x1
+#define OPT4001_CTRL_CONVERSION_1_8MS   0x2
+#define OPT4001_CTRL_CONVERSION_3_4MS   0x3
+#define OPT4001_CTRL_CONVERSION_6_5MS   0x4
+#define OPT4001_CTRL_CONVERSION_12_7MS  0x5
+#define OPT4001_CTRL_CONVERSION_25MS    0x6
+#define OPT4001_CTRL_CONVERSION_50MS    0x7
+#define OPT4001_CTRL_CONVERSION_100MS   0x8
+#define OPT4001_CTRL_CONVERSION_200MS   0x9
+#define OPT4001_CTRL_CONVERSION_400MS   0xa
+#define OPT4001_CTRL_CONVERSION_800MS   0xb
+
+/* OPT4001 scale light level range definitions */
+#define OPT4001_CTRL_LIGHT_SCALE_AUTO   12
+
+/* OPT4001 default values */
+#define OPT4001_DEFAULT_CONVERSION_TIME OPT4001_CTRL_CONVERSION_800MS
+
+/* The different packaging of OPT4001 has different constants used when calculating
+ * lux values.
+ */
+struct opt4001_chip_info {
+	int mul;
+	int div;
+	const char *name;
+};
+
+struct opt4001_settings {
+	/* Index of the chosen integration time */
+	u8 int_time;
+	u8 chip_type;
+};
+
+struct opt4001_chip {
+	struct opt4001_settings light_settings;
+	struct regmap *regmap;
+	struct i2c_client *client;
+	const struct opt4001_chip_info *chip_info;
+};
+
+static const struct opt4001_chip_info opt4001_sot_5x3_info = {
+	.mul = 4375,
+	.div = 10000000,
+	.name = "opt4001-sot-5x3"
+};
+
+static const struct opt4001_chip_info opt4001_picostar_info = {
+	.mul = 3125,
+	.div = 10000000,
+	.name = "opt4001-picostar"
+};
+
+static const int opt4001_int_time_available[][2] = {
+	{ 0,    600 },
+	{ 0,   1000 },
+	{ 0,   1800 },
+	{ 0,   3400 },
+	{ 0,   6500 },
+	{ 0,  12700 },
+	{ 0,  25000 },
+	{ 0,  50000 },
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 400000 },
+	{ 0, 800000 },
+};
+
+/*
+ * Conversion time is integration time + time to set register
+ * this is used as integration time.
+ */
+static const int opt4001_int_time_reg[][2] = {
+	{    600,  OPT4001_CTRL_CONVERSION_0_6MS  },
+	{   1000,  OPT4001_CTRL_CONVERSION_1MS    },
+	{   1800,  OPT4001_CTRL_CONVERSION_1_8MS  },
+	{   3400,  OPT4001_CTRL_CONVERSION_3_4MS  },
+	{   6500,  OPT4001_CTRL_CONVERSION_6_5MS  },
+	{  12700,  OPT4001_CTRL_CONVERSION_12_7MS },
+	{  25000,  OPT4001_CTRL_CONVERSION_25MS   },
+	{  50000,  OPT4001_CTRL_CONVERSION_50MS   },
+	{ 100000,  OPT4001_CTRL_CONVERSION_100MS  },
+	{ 200000,  OPT4001_CTRL_CONVERSION_200MS  },
+	{ 400000,  OPT4001_CTRL_CONVERSION_400MS  },
+	{ 800000,  OPT4001_CTRL_CONVERSION_800MS  },
+};
+
+static int opt4001_als_time_to_index(const u32 als_integration_time)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(opt4001_int_time_available); i++) {
+		if (als_integration_time == opt4001_int_time_available[i][1])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static u8 opt4001_calculate_crc(u8 exp, u32 mantissa, u8 count)
+{
+	u8 crc;
+
+	crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2;
+	crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA)
+		 + hweight32(count & 0xA)) % 2) << 1;
+	crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8)
+		 + hweight32(count & 0x8)) % 2) << 2;
+	crc |= (hweight32(mantissa & 0x80808) % 2) << 3;
+
+	return crc;
+}
+
+static int opt4001_read_lux_value(struct iio_dev *indio_dev,
+				  int *val, int *val2)
+{
+	struct opt4001_chip *chip = iio_priv(indio_dev);
+	struct device *dev = &chip->client->dev;
+	unsigned int light1;
+	unsigned int light2;
+	u16 msb;
+	u16 lsb;
+	u8 exp;
+	u8 count;
+	u8 crc;
+	u8 calc_crc;
+	u64 lux_raw;
+	int ret;
+
+	ret = regmap_read(chip->regmap, OPT4001_LIGHT1_MSB, &light1);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read data bytes");
+		return ret;
+	}
+
+	ret = regmap_read(chip->regmap, OPT4001_LIGHT1_LSB, &light2);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read data bytes");
+		return ret;
+	}
+
+	count = FIELD_GET(OPT4001_COUNTER_MASK, light2);
+	exp = FIELD_GET(OPT4001_EXPONENT_MASK, light1);
+	crc = FIELD_GET(OPT4001_CRC_MASK, light2);
+	msb = FIELD_GET(OPT4001_MSB_MASK, light1);
+	lsb = FIELD_GET(OPT4001_LSB_MASK, light2);
+	lux_raw = (msb << 8) + lsb;
+	calc_crc = opt4001_calculate_crc(exp, lux_raw, count);
+	if (calc_crc != crc)
+		return -EIO;
+
+	lux_raw = lux_raw << exp;
+	lux_raw = lux_raw * chip->chip_info->mul;
+	*val = div_u64_rem(lux_raw, chip->chip_info->div, val2);
+	*val2 = *val2 * 100;
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int opt4001_set_conf(struct opt4001_chip *chip)
+{
+	struct opt4001_settings light_settings = chip->light_settings;
+	struct device *dev = &chip->client->dev;
+	u16 reg;
+	int ret;
+
+	reg = FIELD_PREP(OPT4001_CTRL_RANGE_MASK, OPT4001_CTRL_LIGHT_SCALE_AUTO);
+	reg |= FIELD_PREP(OPT4001_CTRL_CONV_TIME_MASK, light_settings.int_time);
+	reg |= FIELD_PREP(OPT4001_CTRL_OPER_MODE_MASK, OPT4001_CTRL_OPER_MODE_CONTINUOUS);
+
+	ret = regmap_write(chip->regmap, OPT4001_CTRL, reg);
+	if (ret)
+		dev_err(dev, "Failed to set configuration\n");
+
+	return ret;
+}
+
+static int opt4001_power_down(struct opt4001_chip *chip)
+{
+	int ret;
+	unsigned int reg;
+	struct device *dev = &chip->client->dev;
+
+	ret = regmap_read(chip->regmap, OPT4001_DEVICE_ID, &reg);
+	if (ret) {
+		dev_err(dev, "Failed to read configuration\n");
+		return ret;
+	}
+
+	/* MODE_OFF is 0x0 so just set bits to 0 */
+	reg &= ~OPT4001_CTRL_OPER_MODE_MASK;
+
+	ret = regmap_write(chip->regmap, OPT4001_CTRL, reg);
+	if (ret)
+		dev_err(dev, "Failed to set configuration to power down\n");
+
+	return ret;
+}
+
+static void opt4001_chip_off_action(void *data)
+{
+	struct opt4001_chip *chip = data;
+
+	opt4001_power_down(chip);
+}
+
+static const struct iio_chan_spec opt4001_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
+	},
+};
+
+static int opt4001_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct opt4001_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = opt4001_read_lux_value(indio_dev, val, val2);
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		*val2 = opt4001_int_time_reg[chip->light_settings.int_time][0];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int opt4001_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct opt4001_chip *chip = iio_priv(indio_dev);
+	int int_time;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		int_time = opt4001_als_time_to_index(val2);
+		if (int_time < 0) {
+			ret = int_time;
+		} else {
+			chip->light_settings.int_time = int_time;
+			ret = opt4001_set_conf(chip);
+		}
+
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int opt4001_read_available(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_INT_TIME:
+		*length = ARRAY_SIZE(opt4001_int_time_available) * 2;
+		*vals = (const int *)opt4001_int_time_available;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info opt4001_info_no_irq = {
+	.read_raw = opt4001_read_raw,
+	.write_raw = opt4001_write_raw,
+	.read_avail = opt4001_read_available,
+};
+
+static int opt4001_load_defaults(struct opt4001_chip *chip)
+{
+	chip->light_settings.int_time = OPT4001_DEFAULT_CONVERSION_TIME;
+
+	return opt4001_set_conf(chip);
+}
+
+static bool opt4001_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case OPT4001_LIGHT1_MSB:
+	case OPT4001_LIGHT1_LSB:
+	case OPT4001_CTRL:
+	case OPT4001_DEVICE_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool opt4001_writable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case OPT4001_CTRL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool opt4001_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case OPT4001_LIGHT1_MSB:
+	case OPT4001_LIGHT1_LSB:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config opt4001_regmap_config = {
+	.name = "opt4001",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = OPT4001_DEVICE_ID,
+	.readable_reg = opt4001_readable_reg,
+	.writeable_reg = opt4001_writable_reg,
+	.volatile_reg = opt4001_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+};
+
+static int opt4001_probe(struct i2c_client *client)
+{
+	struct opt4001_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+	uint dev_id;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+
+	ret = devm_regulator_get_enable(&client->dev, "vdd");
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to enable vdd supply\n");
+
+	chip->regmap = devm_regmap_init_i2c(client, &opt4001_regmap_config);
+	if (IS_ERR(chip->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+				     "regmap initialization failed\n");
+	chip->client = client;
+
+	indio_dev->info = &opt4001_info_no_irq;
+
+	ret = regmap_reinit_cache(chip->regmap, &opt4001_regmap_config);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to reinit regmap cache\n");
+
+	ret = regmap_read(chip->regmap, OPT4001_DEVICE_ID, &dev_id);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+			"Failed to read the device ID register\n");
+
+	dev_id = FIELD_GET(OPT4001_DEVICE_ID_MASK, dev_id);
+	if (dev_id != OPT4001_DEVICE_ID_VAL) {
+		dev_err(&client->dev, "Device ID: %#04x unknown\n", dev_id);
+		return -EINVAL;
+	}
+
+	chip->chip_info = device_get_match_data(&client->dev);
+
+	indio_dev->channels = opt4001_channels;
+	indio_dev->num_channels = ARRAY_SIZE(opt4001_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = chip->chip_info->name;
+
+	ret = opt4001_load_defaults(chip);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to set sensor defaults\n");
+
+	ret = devm_add_action_or_reset(&client->dev,
+					opt4001_chip_off_action,
+					chip);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to setup power off action %d\n",
+			ret);
+		return ret;
+	}
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+/*
+ * The compatible string determines which constants to use depending on
+ * opt4001 packaging
+ */
+static const struct i2c_device_id opt4001_id[] = {
+	{ "opt4001-sot-5x3", (kernel_ulong_t)&opt4001_sot_5x3_info },
+	{ "opt4001-picostar", (kernel_ulong_t)&opt4001_picostar_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, opt4001_id);
+
+static const struct of_device_id opt4001_of_match[] = {
+	{ .compatible = "ti,opt4001-sot-5x3", .data = &opt4001_sot_5x3_info},
+	{ .compatible = "ti,opt4001-picostar", .data = &opt4001_picostar_info},
+	{}
+};
+MODULE_DEVICE_TABLE(of, opt4001_of_match);
+
+static struct i2c_driver opt4001_driver = {
+	.driver = {
+		.name = "opt4001",
+		.of_match_table = opt4001_of_match,
+	},
+	.probe_new = opt4001_probe,
+	.id_table = opt4001_id,
+};
+module_i2c_driver(opt4001_driver);
+
+MODULE_AUTHOR("Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>");
+MODULE_DESCRIPTION("Texas Instruments opt4001 ambient light sensor driver");
+MODULE_LICENSE("GPL");

-- 
2.30.2

Re: [PATCH v2 1/2] dt-bindings: Document TI OPT4001 light sensor bindings

[Krzysztof Kozlowski]

On 18/04/2023 12:36, Stefan Windfeldt-Prytz wrote:
> Add devicetree bindings for opt4001 ambient light sensor.


Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> 
> Signed-off-by: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
> ---
>  .../devicetree/bindings/iio/light/ti,opt4001.yaml  | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml
> new file mode 100644
> index 000000000000..43fd1a992aea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ti,opt4001.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/ti,opt4001.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments OPT4001 Ambient Light Sensor
> +
> +maintainers:
> +  - Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Ambient light sensor with an i2c interface.
> +  Last part of compatible is for the packaging used.
> +  Picostar is a 4 pinned SMT and sot-5x3 is a 8 pinned SOT.
> +  Only sot-5x3 has an interrupt pin.

Drop this sentence. It obvious from the schema.

> +  https://www.ti.com/lit/gpn/opt4001
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,opt4001-picostar
> +      - ti,opt4001-sot-5x3


Best regards,
Krzysztof

Re: [PATCH v2 2/2] iio: light: Add support for TI OPT4001 light sensor

[Jonathan Cameron]

On Tue, 18 Apr 2023 12:36:27 +0200
Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com> wrote:

> This driver uses the continuous mode of the chip and integration
> time can be configured through sysfs.
> The constants for calculating lux value differs between packaging
> so it uses different compatible string for the two versions
> "ti,opt4001-picostar" and "ti,opt4001-sot-5x3" since the device id
> is the same.
> 
> Datasheet: https://www.ti.com/lit/gpn/opt4001
> Signed-off-by: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>

A few minor comments inline.

Thanks,

Jonathan

> +/* The different packaging of OPT4001 has different constants used when calculating
/*
 * The different...

For IIO multiline comments. This is not consistent across different kernel subsystems but
tends to be consistent within one.

> + * lux values.
> + */
> +struct opt4001_chip_info {
> +	int mul;
> +	int div;
> +	const char *name;
> +};


> +
> +static int opt4001_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct opt4001_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = opt4001_read_lux_value(indio_dev, val, val2);

As below. Early returns make for easier to review code as we don't need to go
see if there is any cleanup when there isn't any to be done.

> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = opt4001_int_time_reg[chip->light_settings.int_time][0];
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int opt4001_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct opt4001_chip *chip = iio_priv(indio_dev);
> +	int int_time;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		int_time = opt4001_als_time_to_index(val2);
> +		if (int_time < 0) {
Early returns make this easier to review + shorten it a little.

> +			ret = int_time;
			return int_time;
> +		} else {
		}

		and you can drop indent on next bit.

> +			chip->light_settings.int_time = int_time;
> +			ret = opt4001_set_conf(chip);
			return opt...;
> +		}
> +

> +		break;
> +	default:
> +		ret = -EINVAL;
		return -EINVAL;

> +	}
> +
> +	return ret;
No need for this as can't reach here after above changes.

> +}
> +

> +static int opt4001_probe(struct i2c_client *client)
> +{
> +	struct opt4001_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	uint dev_id;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to enable vdd supply\n");
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &opt4001_regmap_config);
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
> +				     "regmap initialization failed\n");
> +	chip->client = client;
> +
> +	indio_dev->info = &opt4001_info_no_irq;
> +
> +	ret = regmap_reinit_cache(chip->regmap, &opt4001_regmap_config);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to reinit regmap cache\n");
> +
> +	ret = regmap_read(chip->regmap, OPT4001_DEVICE_ID, &dev_id);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +			"Failed to read the device ID register\n");
> +
> +	dev_id = FIELD_GET(OPT4001_DEVICE_ID_MASK, dev_id);
> +	if (dev_id != OPT4001_DEVICE_ID_VAL) {
> +		dev_err(&client->dev, "Device ID: %#04x unknown\n", dev_id);
> +		return -EINVAL;

Warn only on a failure to match and don't error out.
The reason for this is DT fallback compatibles. They only work to enable
a newer part compatible with older driver support if the older driver doesn't
error out on an ID miss match.   So the most we can do is warn that we don't
know what the device is, but then assume the dt compatible or similar is
correct.

> +	}
> +
> +	chip->chip_info = device_get_match_data(&client->dev);
> +
> +	indio_dev->channels = opt4001_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(opt4001_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = chip->chip_info->name;
> +
> +	ret = opt4001_load_defaults(chip);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to set sensor defaults\n");
> +
> +	ret = devm_add_action_or_reset(&client->dev,
> +					opt4001_chip_off_action,
> +					chip);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to setup power off action %d\n",
> +			ret);
Trivial, but in probe you can always use dev_err_probe() whether or not there
is any chance of a defer.  That simplifies this to
		return dev_err_probe(&client->dev, ret, "..");

Which is slightly nicer.  Added bonus is reviewer doesn't need to think if
something might defer or not.

> +		return ret;
> +	}
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}