[PATCH 0/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[PATCH 0/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Erikas Bitovtas]

This patch series adds support for Capella cm36686 and cm36672p ambient
light and proximity sensors.

Capella cm36686 is a combined ambient light and proximity sensor with
adjustable integration time, interrupt and hysteresis support. It has
the slave address of 0x60. cm36672p is fully compatible with cm36686,
except that it is a proximity-only sensor.

Unfortunately, datasheets for these sensors are not publicly
available. This code is based on Android downstream kernels for devices
which did use these sensors and a previous submission for cm36672p to
mailing lists:
https://github.com/LineageOS/android_kernel_xiaomi_msm8992/blob/cm-14.1/drivers/iio/light/cm36686.c
https://github.com/shakalaca/ASUS_ZenFone_ZD551KL/blob/android-6.0/kernel/drivers/input/misc/cm36283.c
https://lore.kernel.org/linux-iio/1465462845-1571-1-git-send-email-capellamicro@gmail.com/

The following code has been tested on Asus ZenFone 2 Laser/Selfie, which
uses cm36686 as its ambient light and proximity sensor.

Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
---
Erikas Bitovtas (2):
      dt-bindings: Add binding document for cm36686
      iio: light: Add support for Capella cm36686 and cm36672p sensors

 .../bindings/iio/light/capella,cm36686.yaml        |  74 ++
 drivers/iio/light/Kconfig                          |  11 +
 drivers/iio/light/Makefile                         |   1 +
 drivers/iio/light/cm36686.c                        | 810 +++++++++++++++++++++
 4 files changed, 896 insertions(+)
---
base-commit: 4c87cdd0328495759f6e9f9f4e1e53ef8032a76f
change-id: 20260201-cm36686-fc7a8385f1cd

Best regards,
-- 
Erikas Bitovtas <xerikasxx@gmail.com>

[PATCH 1/2] dt-bindings: Add binding document for cm36686

[Erikas Bitovtas]

Document the Capella cm36686 ambient light and proximity sensor devicetree
bindings.

Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
---
 .../bindings/iio/light/capella,cm36686.yaml        | 74 ++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
new file mode 100644
index 000000000000..5f0a585e3d87
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/capella,cm36686.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Capella cm36686 I2C Ambient Light and Proximity sensor
+
+maintainers:
+  - Erikas Bitovtas <xerikasxx@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - capella,cm36686
+      - capella,cm36672p
+
+  reg:
+    maxItems: 1
+    description:
+      I2C slave address of the device. Must be 0x60 for both cm36686
+      and cm36672p sensors.
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description:
+      Regulator that provides power to the sensor.
+
+  vddio-supply:
+    description:
+      Regulator used to power IO and I2C bus.
+
+  vled-supply:
+    description:
+      Regulator used to power proximity LED
+
+  proximity-near-level: true
+
+  capella,proximity-led-current:
+    description:
+      Current for proximity IR LED (in uA)
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [50, 75, 100, 120, 140, 160, 180, 200]
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      light-sensor@60 {
+        compatible = "capella,cm36686";
+        reg = <0x60>;
+
+        vdd-supply = <&pm8916_l8>;
+        vddio-supply = <&pm8916_l6>;
+        vled-supply = <&reg_prox_vled>;
+
+        interrupts-extended = <&tlmm 113 IRQ_TYPE_EDGE_FALLING>;
+
+        proximity-near-level = <30>;
+        capella,proximity-led-current = <100>;
+      };
+    };

-- 
2.52.0

Re: [PATCH 1/2] dt-bindings: Add binding document for cm36686

[David Lechner]

On 2/1/26 11:03 AM, Erikas Bitovtas wrote:
> Document the Capella cm36686 ambient light and proximity sensor devicetree
> bindings.
> 
> Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
> ---
>  .../bindings/iio/light/capella,cm36686.yaml        | 74 ++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
> new file mode 100644
> index 000000000000..5f0a585e3d87
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/capella,cm36686.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Capella cm36686 I2C Ambient Light and Proximity sensor
> +
> +maintainers:
> +  - Erikas Bitovtas <xerikasxx@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - capella,cm36686
> +      - capella,cm36672p

The cover letter mentioned that one is fully compatible with the other,
so we should have a fallback here.

  compatible:
    oneOf:
      - const: capella,cm36672p
      - items:
          -const: capella,cm36686
         - const: capella,cm36672p

> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      I2C slave address of the device. Must be 0x60 for both cm36686
> +      and cm36672p sensors.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description:
> +      Regulator that provides power to the sensor.
> +
> +  vddio-supply:
> +    description:
> +      Regulator used to power IO and I2C bus.
> +
> +  vled-supply:
> +    description:
> +      Regulator used to power proximity LED
> +
> +  proximity-near-level: true
> +
> +  capella,proximity-led-current:

Should use the standard -microamp suffix [1].

[1]: https://github.com/devicetree-org/dt-schema/blob/d16dc68b093e59ec4ae6a32a2e1179cb9cc0fada/dtschema/schemas/property-units.yaml#L86C4-L86C13

> +    description:
> +      Current for proximity IR LED (in uA)
> +    $ref: /schemas/types.yaml#/definitions/uint32

Then we don't need the $ref.

> +    enum: [50, 75, 100, 120, 140, 160, 180, 200]

Should have a default: value since this isn't a required property.

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg

Power supplies should be required.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      light-sensor@60 {
> +        compatible = "capella,cm36686";
> +        reg = <0x60>;
> +
> +        vdd-supply = <&pm8916_l8>;
> +        vddio-supply = <&pm8916_l6>;
> +        vled-supply = <&reg_prox_vled>;
> +
> +        interrupts-extended = <&tlmm 113 IRQ_TYPE_EDGE_FALLING>;
> +
> +        proximity-near-level = <30>;
> +        capella,proximity-led-current = <100>;
> +      };
> +    };
> 

Re: [PATCH 1/2] dt-bindings: Add binding document for cm36686

[Jonathan Cameron]

On Sun, 01 Feb 2026 19:03:48 +0200
Erikas Bitovtas <xerikasxx@gmail.com> wrote:

> Document the Capella cm36686 ambient light and proximity sensor devicetree
> bindings.
> 
> Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
One comment inline.

Thanks,

Jonathan

> ---
>  .../bindings/iio/light/capella,cm36686.yaml        | 74 ++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
> new file mode 100644
> index 000000000000..5f0a585e3d87
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/capella,cm36686.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/capella,cm36686.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Capella cm36686 I2C Ambient Light and Proximity sensor
> +
> +maintainers:
> +  - Erikas Bitovtas <xerikasxx@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - capella,cm36686
> +      - capella,cm36672p
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      I2C slave address of the device. Must be 0x60 for both cm36686

Try to avoid the word slave in new docs

      I2C address of the device ...

Is fine here.

> +      and cm36672p sensors.
>

[PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Erikas Bitovtas]

This driver adds the initial support for the Capella cm36686
ambient light and proximity sensor and cm36672p proximity sensor.

Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
---
 drivers/iio/light/Kconfig   |  11 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/cm36686.c | 810 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 822 insertions(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index ac1408d374c9..b1d1943dec33 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -220,6 +220,17 @@ config CM36651
 	  To compile this driver as a module, choose M here:
 	  the module will be called cm36651.
 
+config CM36686
+	depends on I2C
+	tristate "CM36686 driver"
+	help
+	  Say Y here if you use cm36686.
+	  This option enables ambient light & proximity sensor using
+	  Capella cm36686 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36686.
+
 config IIO_CROS_EC_LIGHT_PROX
 	tristate "ChromeOS EC Light and Proximity Sensors"
 	depends on IIO_CROS_EC_SENSORS_CORE
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0048e0d5ca8..806df80f6454 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_CM3232)		+= cm3232.o
 obj-$(CONFIG_CM3323)		+= cm3323.o
 obj-$(CONFIG_CM3605)		+= cm3605.o
 obj-$(CONFIG_CM36651)		+= cm36651.o
+obj-$(CONFIG_CM36686)		+= cm36686.o
 obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
 obj-$(CONFIG_GP2AP002)		+= gp2ap002.o
 obj-$(CONFIG_GP2AP020A00F)	+= gp2ap020a00f.o
diff --git a/drivers/iio/light/cm36686.c b/drivers/iio/light/cm36686.c
new file mode 100644
index 000000000000..eb108af7226d
--- /dev/null
+++ b/drivers/iio/light/cm36686.c
@@ -0,0 +1,810 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <asm-generic/errno-base.h>
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
+#include <linux/iio/types.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+#include <linux/interrupt.h>
+
+/* Device registers */
+#define CM36686_REG_ALS_CONF		0x00
+#define CM36686_REG_PS_CONF1		0x03
+#define CM36686_REG_PS_CONF3		0x04
+#define CM36686_REG_PS_THDL		0x06
+#define CM36686_REG_PS_THDH		0x07
+#define CM36686_REG_PS_DATA		0x08
+#define CM36686_REG_ALS_DATA		0x09
+#define CM36686_REG_INT_FLAG		0x0B
+#define CM36686_REG_ID_FLAG		0x0C
+
+/* ALS_CONF */
+#define CM36686_ALS_IT			GENMASK(7, 6)
+#define CM36686_ALS_GAIN		GENMASK(3, 2)
+#define CM36686_ALS_INT_EN		BIT(1)
+#define CM36686_ALS_SD			BIT(0)
+
+/* PS_CONF1 */
+#define CM36686_PS_DR			GENMASK(7, 6)
+#define CM36686_PS_PERS			GENMASK(5, 4)
+#define CM36686_PS_IT			GENMASK(3, 1)
+#define CM36686_PS_SD			BIT(0)
+
+#define CM36686_PS_INT_OUT		BIT(9)
+#define CM36686_PS_INT_IN		BIT(8)
+
+/* PS_CONF3 */
+#define CM36686_PS_SMART_PERS_ENABLE	BIT(4)
+
+#define CM36686_LED_I			GENMASK(10, 8)
+
+/* INT_FLAG */
+#define CM36686_PS_IF			GENMASK(9, 8)
+
+/* Default values */
+#define CM36686_ALS_ENABLE		0x00
+#define CM36686_PS_DR_1_320		FIELD_PREP(CM36686_PS_DR, 3)
+#define CM36686_PS_PERS_2		FIELD_PREP(CM36686_PS_PERS, 1)
+#define CM36686_PS_IT_2_5T		FIELD_PREP(CM36686_PS_IT, 3)
+#define CM36686_LED_I_100		FIELD_PREP(CM36686_LED_I, 2)
+
+/* Shifts */
+#define CM36686_INT_FLAG_SHIFT		8
+
+/* Max proximity thresholds */
+#define CM36686_MAX_PS_VALUE		(BIT(12) - 1)
+
+#define CM36686_DEVICE_ID		0x86
+
+enum {
+	CM36686,
+	CM36672P,
+};
+
+enum cm36686_distance {
+	CM36686_AWAY = 1,
+	CM36686_CLOSE,
+	CM36686_BOTH
+};
+
+enum {
+	CM36686_PS_CONF1,
+	CM36686_PS_CONF3,
+	CM36686_PS_CONF_NUM
+};
+
+enum {
+	CM36686_SUPPLY_VDD,
+	CM36686_SUPPLY_VDDIO,
+	CM36686_SUPPLY_VLED,
+	CM36686_SUPPLY_NUM,
+};
+
+static const int cm36686_als_it_times[][2] = {
+	{0, 80000},
+	{0, 160000},
+	{0, 320000},
+	{0, 640000}
+};
+
+static const int cm36686_ps_it_times[][2] = {
+	{0, 320},
+	{0, 480},
+	{0, 640},
+	{0, 800},
+	{0, 960},
+	{0, 1120},
+	{0, 1280},
+	{0, 2560}
+};
+
+static const int cm36686_ps_led_current[] = {
+	50,
+	75,
+	100,
+	120,
+	140,
+	160,
+	180,
+	200
+};
+
+struct cm36686_data {
+	struct mutex lock;
+	struct i2c_client *client;
+	struct regulator_bulk_data supplies[CM36686_SUPPLY_NUM];
+	int als_conf;
+	int ps_conf[CM36686_PS_CONF_NUM];
+	int ps_close;
+	int ps_away;
+};
+
+struct cm36686_chip_info {
+	const char *name;
+	const struct iio_chan_spec *channels;
+	const int num_channels;
+};
+
+static int cm36686_current_to_index(int led_current)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cm36686_ps_led_current); i++)
+		if (led_current < cm36686_ps_led_current[i])
+			break;
+
+	return i > 0 ? i - 1 : -EINVAL;
+}
+
+static ssize_t cm36686_read_near_level(struct iio_dev *indio_dev,
+				       uintptr_t priv,
+				       const struct iio_chan_spec *chan,
+				       char *buf)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%u\n", chip->ps_close);
+}
+
+static const struct iio_chan_spec_ext_info cm36686_ext_info[] = {
+	{
+		.name = "nearlevel",
+		.shared = IIO_SEPARATE,
+		.read = cm36686_read_near_level,
+	},
+	{}
+};
+
+static const struct iio_event_spec cm36686_proximity_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	}
+};
+
+static const struct iio_chan_spec cm36686_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = CM36686_REG_ALS_DATA,
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = CM36686_REG_PS_DATA,
+		.event_spec = cm36686_proximity_event_spec,
+		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
+		.ext_info = cm36686_ext_info
+	}
+};
+
+static const struct iio_chan_spec cm36672p_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = CM36686_REG_PS_DATA,
+		.event_spec = cm36686_proximity_event_spec,
+		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
+		.ext_info = cm36686_ext_info
+	}
+};
+
+static int cm36686_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	if (mask != IIO_CHAN_INFO_INT_TIME)
+		return -EINVAL;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		*vals = (int *)(cm36686_als_it_times);
+		*length = 2 * ARRAY_SIZE(cm36686_als_it_times);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_PROXIMITY:
+		*vals = (int *)(cm36686_ps_it_times);
+		*length = 2 * ARRAY_SIZE(cm36686_ps_it_times);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cm36686_read_channel(struct cm36686_data *chip,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = chip->client;
+	int ret = IIO_VAL_INT;
+
+	int data = i2c_smbus_read_word_data(client, chan->address);
+
+	if (data < 0) {
+		dev_err(&client->dev, "Failed to read register: %pe", ERR_PTR(data));
+		ret = -EIO;
+	} else {
+		*val = data;
+	}
+	return ret;
+}
+
+static int cm36686_read_int_time(struct cm36686_data *chip,
+				 struct iio_chan_spec const *chan, int *val,
+				 int *val2)
+{
+	int als_it_index, ps_it_index;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		als_it_index = FIELD_GET(CM36686_ALS_IT, chip->als_conf);
+		*val = cm36686_als_it_times[als_it_index][0];
+		*val2 = cm36686_als_it_times[als_it_index][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_PROXIMITY:
+		ps_it_index = FIELD_GET(CM36686_PS_IT,
+			chip->ps_conf[CM36686_PS_CONF1]);
+		*val = cm36686_ps_it_times[ps_it_index][0];
+		*val2 = cm36686_ps_it_times[ps_it_index][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cm36686_write_light_int_time(struct cm36686_data *chip, int val2)
+{
+	struct i2c_client *client = chip->client;
+	int index = -1, ret, new_it_time;
+
+	for (int i = 0; i < ARRAY_SIZE(cm36686_als_it_times); i++) {
+		if (cm36686_als_it_times[i][1] == val2) {
+			index = i;
+			break;
+		}
+	}
+
+	if (index == -1)
+		return -EINVAL;
+
+	new_it_time = chip->als_conf & ~CM36686_ALS_IT;
+	new_it_time |= FIELD_PREP(CM36686_ALS_IT, index);
+
+	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_ALS_CONF,
+					new_it_time);
+	if (ret < 0)
+		dev_err(&client->dev,
+			"Failed to set ALS integration time: %pe", ERR_PTR(ret));
+	else
+		chip->als_conf = new_it_time;
+
+	return ret;
+}
+
+static int cm36686_write_prox_int_time(struct cm36686_data *chip, int val2)
+{
+	struct i2c_client *client = chip->client;
+	int index = -1, ret, new_it_time;
+
+	for (int i = 0; i < ARRAY_SIZE(cm36686_ps_it_times); i++) {
+		if (cm36686_ps_it_times[i][1] == val2) {
+			index = i;
+			break;
+		}
+	}
+
+	if (index == -1)
+		return -EINVAL;
+
+	new_it_time = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_IT;
+	new_it_time |= FIELD_PREP(CM36686_PS_IT, index);
+
+	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1,
+					new_it_time);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to set PS integration time: %pe",
+			ERR_PTR(ret));
+	else
+		chip->ps_conf[CM36686_PS_CONF1] = new_it_time;
+
+	return ret;
+}
+
+static int cm36686_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&chip->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36686_read_channel(chip, chan, val);
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = cm36686_read_int_time(chip, chan, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+
+static int cm36686_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	int ret;
+
+	if (val) /* Integration time more than 1s is not supported */
+		return -EINVAL;
+
+	if (mask != IIO_CHAN_INFO_INT_TIME)
+		return -EINVAL;
+
+	mutex_lock(&chip->lock);
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		ret = cm36686_write_light_int_time(chip, val2);
+		break;
+	case IIO_PROXIMITY:
+		ret = cm36686_write_prox_int_time(chip, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+
+static int cm36686_read_prox_thresh(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)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		*val = chip->ps_close;
+		break;
+	case IIO_EV_DIR_FALLING:
+		*val = chip->ps_away;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int cm36686_write_prox_thresh(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)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int ret = 0, address, *thresh;
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_FALLING:
+		if (val > chip->ps_close || val < 0)
+			return -EINVAL;
+
+		address = CM36686_REG_PS_THDL;
+		thresh = &chip->ps_away;
+		break;
+	case IIO_EV_DIR_RISING:
+		if (val < chip->ps_away || val > CM36686_MAX_PS_VALUE)
+			return -EINVAL;
+
+		address = CM36686_REG_PS_THDH;
+		thresh = &chip->ps_close;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&chip->lock);
+
+	ret = i2c_smbus_write_word_data(client, address, val);
+	if (ret < 0)
+		dev_err(&client->dev,
+			"Failed to set PS threshold value: %pe", ERR_PTR(ret));
+	else
+		*thresh = val;
+
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+
+static int cm36686_read_prox_event_config(struct iio_dev *indio_dev,
+					  const struct iio_chan_spec *chan,
+					  enum iio_event_type type,
+					  enum iio_event_direction dir)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_FALLING:
+		return FIELD_GET(CM36686_PS_INT_OUT, chip->ps_conf[CM36686_PS_CONF1]);
+	case IIO_EV_DIR_RISING:
+		return FIELD_GET(CM36686_PS_INT_IN, chip->ps_conf[CM36686_PS_CONF1]);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cm36686_write_prox_event_config(struct iio_dev *indio_dev,
+					   const struct iio_chan_spec *chan,
+					   enum iio_event_type type,
+					   enum iio_event_direction dir,
+					   bool state)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int ret = 0, new_ps_conf;
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_FALLING:
+		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_OUT;
+		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_OUT, state);
+		break;
+	case IIO_EV_DIR_RISING:
+		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_IN;
+		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_IN, state);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&chip->lock);
+
+	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1, new_ps_conf);
+	if (ret < 0)
+		dev_err(&client->dev,
+			"Failed to set proximity event interrupt config: %pe", ERR_PTR(ret));
+	else
+		chip->ps_conf[CM36686_PS_CONF1] = new_ps_conf;
+
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int cm36686_fallback_read_ps(struct iio_dev *indio_dev)
+{
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int data = i2c_smbus_read_word_data(client, CM36686_REG_PS_DATA);
+
+	if (data < 0)
+		return data;
+
+	if (data < chip->ps_away)
+		return IIO_EV_DIR_FALLING;
+	else if (data > chip->ps_close)
+		return IIO_EV_DIR_RISING;
+	else
+		return IIO_EV_DIR_EITHER;
+}
+
+static irqreturn_t cm36686_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct cm36686_data *chip = iio_priv(indio_dev);
+	struct i2c_client *client = chip->client;
+	int ev_dir, ret;
+	u64 ev_code;
+
+	/* Reading the interrupt flag acknowledges the interrupt */
+	ret = i2c_smbus_read_word_data(client, CM36686_REG_INT_FLAG);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Interrupt flag register read failed: %pe", ERR_PTR(ret));
+		return IRQ_HANDLED;
+	}
+
+	ret >>= CM36686_INT_FLAG_SHIFT;
+	switch (ret) {
+	case CM36686_CLOSE:
+		ev_dir = IIO_EV_DIR_RISING;
+		break;
+	case CM36686_AWAY:
+		ev_dir = IIO_EV_DIR_FALLING;
+		break;
+	case CM36686_BOTH:
+		ev_dir = cm36686_fallback_read_ps(indio_dev);
+		if (ev_dir < 0) {
+			dev_err(&client->dev, "Failed to settle interrupt state: %pe",
+				ERR_PTR(ret));
+			return IRQ_HANDLED;
+		}
+		break;
+	default:
+		dev_err(&client->dev, "Unknown interrupt state: %x", ret);
+		return IRQ_HANDLED;
+	}
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, IIO_EV_INFO_VALUE,
+				       IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns(indio_dev));
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info cm36686_info = {
+	.read_avail =		cm36686_read_avail,
+	.read_raw =		cm36686_read_raw,
+	.write_raw =		cm36686_write_raw,
+	.read_event_value =	cm36686_read_prox_thresh,
+	.write_event_value =	cm36686_write_prox_thresh,
+	.read_event_config =	cm36686_read_prox_event_config,
+	.write_event_config =	cm36686_write_prox_event_config,
+};
+
+static struct cm36686_chip_info cm36686_chip_info_tbl[] = {
+	[CM36686] = {
+		.name = "cm36686",
+		.channels = cm36686_channels,
+		.num_channels = ARRAY_SIZE(cm36686_channels),
+	},
+	[CM36672P] = {
+		.name = "cm36672p",
+		.channels = cm36672p_channels,
+		.num_channels = ARRAY_SIZE(cm36672p_channels),
+	},
+};
+
+static int cm36686_setup(struct cm36686_data *chip)
+{
+	struct i2c_client *client = chip->client;
+	int ret, led_current, led_index;
+
+	chip->als_conf = CM36686_ALS_ENABLE;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
+					chip->als_conf);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable ambient light sensor: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	chip->ps_conf[CM36686_PS_CONF1] = CM36686_PS_INT_IN |
+		CM36686_PS_INT_OUT | CM36686_PS_DR_1_320 |
+		CM36686_PS_PERS_2 | CM36686_PS_IT_2_5T;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
+					chip->ps_conf[CM36686_PS_CONF1]);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	chip->ps_conf[CM36686_PS_CONF3] = CM36686_PS_SMART_PERS_ENABLE;
+
+	ret = device_property_read_u32(&client->dev,
+		"capella,proximity-led-current", &led_current);
+	if (!ret) {
+		led_index = cm36686_current_to_index(led_current);
+		if (led_index < 0) {
+			dev_err(&client->dev, "No appropriate current for IR LED found.");
+			return led_index;
+		}
+
+		chip->ps_conf[CM36686_PS_CONF3] &= ~CM36686_LED_I;
+		chip->ps_conf[CM36686_PS_CONF3] |= FIELD_PREP(CM36686_LED_I, led_index);
+	}
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF3,
+					chip->ps_conf[CM36686_PS_CONF3]);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = device_property_read_u32(&client->dev, "proximity-near-level",
+					    &chip->ps_close);
+	if (ret < 0)
+		chip->ps_close = 0;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDH,
+		chip->ps_close);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to set close proximity threshold: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	chip->ps_away = chip->ps_close;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDL,
+		chip->ps_away);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to set away proximity threshold: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	return 0;
+}
+
+static void cm36686_shutdown(void *data)
+{
+	struct cm36686_data *chip = data;
+	struct i2c_client *client = chip->client;
+	int ret, als_shutdown, ps_shutdown;
+
+	als_shutdown = chip->als_conf | CM36686_ALS_SD;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
+					als_shutdown);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to shutdown ALS");
+
+	ps_shutdown = chip->ps_conf[CM36686_PS_CONF1] | CM36686_PS_SD;
+
+	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
+					ps_shutdown);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to shutdown PS");
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(chip->supplies), chip->supplies);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to disable regulators");
+}
+
+static int cm36686_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct cm36686_data *chip;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev,
+					  sizeof(struct cm36686_data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+
+	const struct cm36686_chip_info *info =
+		&cm36686_chip_info_tbl[id->driver_data];
+
+	ret = i2c_smbus_read_byte_data(client, CM36686_REG_ID_FLAG);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret, "Failed to read device ID");
+
+	if (ret != CM36686_DEVICE_ID)
+		return dev_err_probe(&client->dev, -ENODEV, "Device not recognized!");
+
+	i2c_set_clientdata(client, indio_dev);
+	chip->client = client;
+	ret = devm_mutex_init(&client->dev, &chip->lock);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+		       "Failed to initialize mutex");
+
+	chip->supplies[CM36686_SUPPLY_VDD].supply = "vdd";
+	chip->supplies[CM36686_SUPPLY_VDDIO].supply = "vddio";
+	chip->supplies[CM36686_SUPPLY_VLED].supply = "vled";
+
+	ret = devm_regulator_bulk_get(&client->dev,
+		ARRAY_SIZE(chip->supplies), chip->supplies);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to enable regulators");
+
+	ret = devm_add_action_or_reset(&client->dev, cm36686_shutdown, chip);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to set shutdown action");
+
+	ret = cm36686_setup(chip);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to set up registers");
+
+	indio_dev->name = info->name;
+	indio_dev->channels = info->channels;
+	indio_dev->num_channels = info->num_channels;
+	indio_dev->info = &cm36686_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					cm36686_irq_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to request irq");
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to register iio device");
+
+	return 0;
+}
+
+static const struct i2c_device_id cm36686_id[] = {
+	{ "cm36686", CM36686 },
+	{ "cm36672p", CM36672P },
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36686_id);
+
+static const struct of_device_id cm36686_of_match[] = {
+	{ .compatible = "capella,cm36686" },
+	{ .compatible = "capella,cm36672p" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cm36686_of_match);
+
+static struct i2c_driver cm36686_driver = {
+	.driver = {
+		.name = "cm36686",
+		.of_match_table = cm36686_of_match,
+	},
+	.probe = cm36686_probe,
+	.id_table = cm36686_id
+};
+
+module_i2c_driver(cm36686_driver);
+
+MODULE_AUTHOR("Erikas Bitovtas <xerikasxx@gmail.com>");
+MODULE_DESCRIPTION("CM36686 ambient light and proximity sensor driver");
+MODULE_LICENSE("GPL");

-- 
2.52.0

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[David Lechner]

On 2/1/26 11:03 AM, Erikas Bitovtas wrote:
> This driver adds the initial support for the Capella cm36686
> ambient light and proximity sensor and cm36672p proximity sensor.
> 
> Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
> ---
>  drivers/iio/light/Kconfig   |  11 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/cm36686.c | 810 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 822 insertions(+)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index ac1408d374c9..b1d1943dec33 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -220,6 +220,17 @@ config CM36651
>  	  To compile this driver as a module, choose M here:
>  	  the module will be called cm36651.
>  
> +config CM36686
> +	depends on I2C
> +	tristate "CM36686 driver"
> +	help
> +	  Say Y here if you use cm36686.
> +	  This option enables ambient light & proximity sensor using
> +	  Capella cm36686 device driver.
> +
> +	  To compile this driver as a module, choose M here:
> +	  the module will be called cm36686.
> +
>  config IIO_CROS_EC_LIGHT_PROX
>  	tristate "ChromeOS EC Light and Proximity Sensors"
>  	depends on IIO_CROS_EC_SENSORS_CORE
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c0048e0d5ca8..806df80f6454 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_CM3232)		+= cm3232.o
>  obj-$(CONFIG_CM3323)		+= cm3323.o
>  obj-$(CONFIG_CM3605)		+= cm3605.o
>  obj-$(CONFIG_CM36651)		+= cm36651.o
> +obj-$(CONFIG_CM36686)		+= cm36686.o
>  obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
>  obj-$(CONFIG_GP2AP002)		+= gp2ap002.o
>  obj-$(CONFIG_GP2AP020A00F)	+= gp2ap020a00f.o
> diff --git a/drivers/iio/light/cm36686.c b/drivers/iio/light/cm36686.c
> new file mode 100644
> index 000000000000..eb108af7226d
> --- /dev/null
> +++ b/drivers/iio/light/cm36686.c
> @@ -0,0 +1,810 @@
> +// SPDX-License-Identifier: GPL-2.0-only

I would expect copyright statements to be preserved here since the cover
letter mentions much of this was derived from existing code.

> +
> +#include <asm-generic/errno-base.h>

Should not be using generic, it may be different from arch-specific.

Generally we #include <linux/err.h> instead.

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device/devres.h>
> +#include <linux/iio/types.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/i2c.h>

Alphabetial order please.

> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/interrupt.h>

linux/iio/*.h can be grouped seperately.

> +
> +/* Device registers */
> +#define CM36686_REG_ALS_CONF		0x00
> +#define CM36686_REG_PS_CONF1		0x03
> +#define CM36686_REG_PS_CONF3		0x04
> +#define CM36686_REG_PS_THDL		0x06
> +#define CM36686_REG_PS_THDH		0x07
> +#define CM36686_REG_PS_DATA		0x08
> +#define CM36686_REG_ALS_DATA		0x09
> +#define CM36686_REG_INT_FLAG		0x0B
> +#define CM36686_REG_ID_FLAG		0x0C
> +
> +/* ALS_CONF */
> +#define CM36686_ALS_IT			GENMASK(7, 6)
> +#define CM36686_ALS_GAIN		GENMASK(3, 2)
> +#define CM36686_ALS_INT_EN		BIT(1)
> +#define CM36686_ALS_SD			BIT(0)
> +
> +/* PS_CONF1 */
> +#define CM36686_PS_DR			GENMASK(7, 6)
> +#define CM36686_PS_PERS			GENMASK(5, 4)
> +#define CM36686_PS_IT			GENMASK(3, 1)
> +#define CM36686_PS_SD			BIT(0)
> +
> +#define CM36686_PS_INT_OUT		BIT(9)
> +#define CM36686_PS_INT_IN		BIT(8)
> +
> +/* PS_CONF3 */
> +#define CM36686_PS_SMART_PERS_ENABLE	BIT(4)
> +
> +#define CM36686_LED_I			GENMASK(10, 8)
> +
> +/* INT_FLAG */
> +#define CM36686_PS_IF			GENMASK(9, 8)
> +
> +/* Default values */
> +#define CM36686_ALS_ENABLE		0x00
> +#define CM36686_PS_DR_1_320		FIELD_PREP(CM36686_PS_DR, 3)
> +#define CM36686_PS_PERS_2		FIELD_PREP(CM36686_PS_PERS, 1)
> +#define CM36686_PS_IT_2_5T		FIELD_PREP(CM36686_PS_IT, 3)
> +#define CM36686_LED_I_100		FIELD_PREP(CM36686_LED_I, 2)
> +
> +/* Shifts */
> +#define CM36686_INT_FLAG_SHIFT		8
> +
> +/* Max proximity thresholds */
> +#define CM36686_MAX_PS_VALUE		(BIT(12) - 1)
> +
> +#define CM36686_DEVICE_ID		0x86
> +
> +enum {
> +	CM36686,
> +	CM36672P,
> +};

We try to avoid an ID enum in new drivers.

> +
> +enum cm36686_distance {
> +	CM36686_AWAY = 1,
> +	CM36686_CLOSE,
> +	CM36686_BOTH
> +};
> +
> +enum {
> +	CM36686_PS_CONF1,
> +	CM36686_PS_CONF3,
> +	CM36686_PS_CONF_NUM
> +};
> +
> +enum {
> +	CM36686_SUPPLY_VDD,
> +	CM36686_SUPPLY_VDDIO,
> +	CM36686_SUPPLY_VLED,
> +	CM36686_SUPPLY_NUM,
> +};
> +
> +static const int cm36686_als_it_times[][2] = {
> +	{0, 80000},
> +	{0, 160000},
> +	{0, 320000},
> +	{0, 640000}
> +};

We try to keep a consistent style of spaces inside of braces and
trailing common on the last item (unless it is a sentinl value).

	{ 0, 640000 },

> +
> +static const int cm36686_ps_it_times[][2] = {
> +	{0, 320},
> +	{0, 480},
> +	{0, 640},
> +	{0, 800},
> +	{0, 960},
> +	{0, 1120},
> +	{0, 1280},
> +	{0, 2560}
> +};
> +
> +static const int cm36686_ps_led_current[] = {

We try to always include the units in the identifier name.
Is this milliamps or microamps?

> +	50,
> +	75,
> +	100,
> +	120,
> +	140,
> +	160,
> +	180,
> +	200
> +};
> +
> +struct cm36686_data {
> +	struct mutex lock;

Locks need to always have a comment explaining what they are protecting.

> +	struct i2c_client *client;
> +	struct regulator_bulk_data supplies[CM36686_SUPPLY_NUM];
> +	int als_conf;
> +	int ps_conf[CM36686_PS_CONF_NUM];
> +	int ps_close;
> +	int ps_away;
> +};
> +
> +struct cm36686_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec *channels;
> +	const int num_channels;
> +};
> +
> +static int cm36686_current_to_index(int led_current)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cm36686_ps_led_current); i++)
> +		if (led_current < cm36686_ps_led_current[i])
> +			break;
> +
> +	return i > 0 ? i - 1 : -EINVAL;

Not sure I follow the logic here. Shouldn't this just `return i;`
instead of `break;` and then unconditnionally `return -EINVAL;` at
the end?

If not, it could use some comments to explain.

> +}
> +
> +static ssize_t cm36686_read_near_level(struct iio_dev *indio_dev,
> +				       uintptr_t priv,
> +				       const struct iio_chan_spec *chan,
> +				       char *buf)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +
> +	return sysfs_emit(buf, "%u\n", chip->ps_close);
> +}
> +
> +static const struct iio_chan_spec_ext_info cm36686_ext_info[] = {
> +	{
> +		.name = "nearlevel",
> +		.shared = IIO_SEPARATE,
> +		.read = cm36686_read_near_level,
> +	},
> +	{}

IIO style for sentinil values is `{ }` (with space in between).

> +};
> +
> +static const struct iio_event_spec cm36686_proximity_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				 BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				 BIT(IIO_EV_INFO_ENABLE),
> +	}

All of these should have trailing comma.

> +};
> +
> +static const struct iio_chan_spec cm36686_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |

IIO_LIGHT should have IIO_CHAN_INFO_SCALE to convert raw to convert the
raw value to lux. If we don't have that info due to lack of documentation,
then we should add a comment to explain that. Or maybe we can make an
educated guess?

> +				      BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = CM36686_REG_ALS_DATA,
> +	},
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = CM36686_REG_PS_DATA,
> +		.event_spec = cm36686_proximity_event_spec,
> +		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
> +		.ext_info = cm36686_ext_info
> +	}
> +};
> +
> +static const struct iio_chan_spec cm36672p_channels[] = {
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = CM36686_REG_PS_DATA,
> +		.event_spec = cm36686_proximity_event_spec,
> +		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
> +		.ext_info = cm36686_ext_info
> +	}
> +};
> +
> +static int cm36686_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long mask)
> +{
> +	if (mask != IIO_CHAN_INFO_INT_TIME)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		*vals = (int *)(cm36686_als_it_times);
> +		*length = 2 * ARRAY_SIZE(cm36686_als_it_times);
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	case IIO_PROXIMITY:
> +		*vals = (int *)(cm36686_ps_it_times);
> +		*length = 2 * ARRAY_SIZE(cm36686_ps_it_times);
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cm36686_read_channel(struct cm36686_data *chip,
> +				struct iio_chan_spec const *chan, int *val)
> +{
> +	struct i2c_client *client = chip->client;
> +	int ret = IIO_VAL_INT;
> +
> +	int data = i2c_smbus_read_word_data(client, chan->address);
> +
> +	if (data < 0) {
> +		dev_err(&client->dev, "Failed to read register: %pe", ERR_PTR(data));

The error is passed to userspace, so this error message doesn't
seem useful (could end up flooding logs if userspace keeps reading).

> +		ret = -EIO;

Can just return directly here and avoid the local variable.

> +	} else {
> +		*val = data;
> +	}
> +	return ret;
> +}
> +
> +static int cm36686_read_int_time(struct cm36686_data *chip,
> +				 struct iio_chan_spec const *chan, int *val,
> +				 int *val2)
> +{
> +	int als_it_index, ps_it_index;
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		als_it_index = FIELD_GET(CM36686_ALS_IT, chip->als_conf);
> +		*val = cm36686_als_it_times[als_it_index][0];
> +		*val2 = cm36686_als_it_times[als_it_index][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_PROXIMITY:
> +		ps_it_index = FIELD_GET(CM36686_PS_IT,
> +			chip->ps_conf[CM36686_PS_CONF1]);
> +		*val = cm36686_ps_it_times[ps_it_index][0];
> +		*val2 = cm36686_ps_it_times[ps_it_index][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cm36686_write_light_int_time(struct cm36686_data *chip, int val2)
> +{
> +	struct i2c_client *client = chip->client;
> +	int index = -1, ret, new_it_time;
> +
> +	for (int i = 0; i < ARRAY_SIZE(cm36686_als_it_times); i++) {
> +		if (cm36686_als_it_times[i][1] == val2) {
> +			index = i;
> +			break;
> +		}
> +	}
> +
> +	if (index == -1)
> +		return -EINVAL;
> +
> +	new_it_time = chip->als_conf & ~CM36686_ALS_IT;
> +	new_it_time |= FIELD_PREP(CM36686_ALS_IT, index);

Can use FIELD_MODIFY()?

> +
> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_ALS_CONF,
> +					new_it_time);
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"Failed to set ALS integration time: %pe", ERR_PTR(ret));

This error message can go too.

> +	else
> +		chip->als_conf = new_it_time;
> +
> +	return ret;
> +}
> +
> +static int cm36686_write_prox_int_time(struct cm36686_data *chip, int val2)
> +{
> +	struct i2c_client *client = chip->client;
> +	int index = -1, ret, new_it_time;
> +
> +	for (int i = 0; i < ARRAY_SIZE(cm36686_ps_it_times); i++) {
> +		if (cm36686_ps_it_times[i][1] == val2) {
> +			index = i;
> +			break;
> +		}
> +	}
> +
> +	if (index == -1)
> +		return -EINVAL;
> +
> +	new_it_time = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_IT;
> +	new_it_time |= FIELD_PREP(CM36686_PS_IT, index);
> +
> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1,
> +					new_it_time);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to set PS integration time: %pe",
> +			ERR_PTR(ret));
> +	else
> +		chip->ps_conf[CM36686_PS_CONF1] = new_it_time;
> +
> +	return ret;
> +}
> +
> +static int cm36686_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&chip->lock);

Can use linux/cleanup.h `guard(mutex)(&chip->lock);`. Then we can return
directly below and get rid of ret.

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = cm36686_read_channel(chip, chan, val);
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = cm36686_read_int_time(chip, chan, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&chip->lock);
> +	return ret;
> +}
> +
> +static int cm36686_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (val) /* Integration time more than 1s is not supported */
> +		return -EINVAL;
> +
> +	if (mask != IIO_CHAN_INFO_INT_TIME)
> +		return -EINVAL;
> +
> +	mutex_lock(&chip->lock);
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		ret = cm36686_write_light_int_time(chip, val2);
> +		break;
> +	case IIO_PROXIMITY:
> +		ret = cm36686_write_prox_int_time(chip, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&chip->lock);
> +	return ret;
> +}
> +
> +static int cm36686_read_prox_thresh(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)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		*val = chip->ps_close;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		*val = chip->ps_away;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int cm36686_write_prox_thresh(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)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	struct i2c_client *client = chip->client;
> +	int ret = 0, address, *thresh;
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_FALLING:
> +		if (val > chip->ps_close || val < 0)
> +			return -EINVAL;
> +
> +		address = CM36686_REG_PS_THDL;
> +		thresh = &chip->ps_away;
> +		break;
> +	case IIO_EV_DIR_RISING:
> +		if (val < chip->ps_away || val > CM36686_MAX_PS_VALUE)
> +			return -EINVAL;
> +
> +		address = CM36686_REG_PS_THDH;
> +		thresh = &chip->ps_close;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&chip->lock);
> +
> +	ret = i2c_smbus_write_word_data(client, address, val);
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"Failed to set PS threshold value: %pe", ERR_PTR(ret));
> +	else
> +		*thresh = val;
> +
> +	mutex_unlock(&chip->lock);
> +	return ret;

In case it isn't obvious, previous comments about error messages and locks
apply here and elsewhere too.

> +}
> +
> +static int cm36686_read_prox_event_config(struct iio_dev *indio_dev,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_FALLING:
> +		return FIELD_GET(CM36686_PS_INT_OUT, chip->ps_conf[CM36686_PS_CONF1]);
> +	case IIO_EV_DIR_RISING:
> +		return FIELD_GET(CM36686_PS_INT_IN, chip->ps_conf[CM36686_PS_CONF1]);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cm36686_write_prox_event_config(struct iio_dev *indio_dev,
> +					   const struct iio_chan_spec *chan,
> +					   enum iio_event_type type,
> +					   enum iio_event_direction dir,
> +					   bool state)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	struct i2c_client *client = chip->client;
> +	int ret = 0, new_ps_conf;
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_FALLING:
> +		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_OUT;
> +		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_OUT, state);
> +		break;
> +	case IIO_EV_DIR_RISING:
> +		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_IN;
> +		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_IN, state);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&chip->lock);
> +
> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1, new_ps_conf);
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"Failed to set proximity event interrupt config: %pe", ERR_PTR(ret));
> +	else
> +		chip->ps_conf[CM36686_PS_CONF1] = new_ps_conf;
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static int cm36686_fallback_read_ps(struct iio_dev *indio_dev)
> +{
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	struct i2c_client *client = chip->client;
> +	int data = i2c_smbus_read_word_data(client, CM36686_REG_PS_DATA);
> +
> +	if (data < 0)
> +		return data;
> +
> +	if (data < chip->ps_away)
> +		return IIO_EV_DIR_FALLING;
> +	else if (data > chip->ps_close)

Don't need else after if with unconditinoal return.

> +		return IIO_EV_DIR_RISING;
> +	else
> +		return IIO_EV_DIR_EITHER;
> +}
> +
> +static irqreturn_t cm36686_irq_handler(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct cm36686_data *chip = iio_priv(indio_dev);
> +	struct i2c_client *client = chip->client;
> +	int ev_dir, ret;
> +	u64 ev_code;
> +
> +	/* Reading the interrupt flag acknowledges the interrupt */
> +	ret = i2c_smbus_read_word_data(client, CM36686_REG_INT_FLAG);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Interrupt flag register read failed: %pe", ERR_PTR(ret));

Error messages in interrupt handlers should probably be rate-limited.

> +		return IRQ_HANDLED;
> +	}
> +
> +	ret >>= CM36686_INT_FLAG_SHIFT;

I think a FIELD_GET() would make more sense here.

> +	switch (ret) {
> +	case CM36686_CLOSE:
> +		ev_dir = IIO_EV_DIR_RISING;
> +		break;
> +	case CM36686_AWAY:
> +		ev_dir = IIO_EV_DIR_FALLING;
> +		break;
> +	case CM36686_BOTH:
> +		ev_dir = cm36686_fallback_read_ps(indio_dev);
> +		if (ev_dir < 0) {
> +			dev_err(&client->dev, "Failed to settle interrupt state: %pe",
> +				ERR_PTR(ret));
> +			return IRQ_HANDLED;
> +		}
> +		break;
> +	default:
> +		dev_err(&client->dev, "Unknown interrupt state: %x", ret);
> +		return IRQ_HANDLED;
> +	}
> +	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, IIO_EV_INFO_VALUE,
> +				       IIO_EV_TYPE_THRESH, ev_dir);
> +
> +	iio_push_event(indio_dev, ev_code, iio_get_time_ns(indio_dev));
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info cm36686_info = {
> +	.read_avail =		cm36686_read_avail,
> +	.read_raw =		cm36686_read_raw,
> +	.write_raw =		cm36686_write_raw,
> +	.read_event_value =	cm36686_read_prox_thresh,
> +	.write_event_value =	cm36686_write_prox_thresh,
> +	.read_event_config =	cm36686_read_prox_event_config,
> +	.write_event_config =	cm36686_write_prox_event_config,
> +};
> +
> +static struct cm36686_chip_info cm36686_chip_info_tbl[] = {
> +	[CM36686] = {
> +		.name = "cm36686",
> +		.channels = cm36686_channels,
> +		.num_channels = ARRAY_SIZE(cm36686_channels),
> +	},
> +	[CM36672P] = {
> +		.name = "cm36672p",
> +		.channels = cm36672p_channels,
> +		.num_channels = ARRAY_SIZE(cm36672p_channels),
> +	},
> +};

We avoid this array style in new drivers in favor of individual struct
per chip.

> +
> +static int cm36686_setup(struct cm36686_data *chip)
> +{
> +	struct i2c_client *client = chip->client;
> +	int ret, led_current, led_index;
> +
> +	chip->als_conf = CM36686_ALS_ENABLE;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
> +					chip->als_conf);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to enable ambient light sensor: %pe", ERR_PTR(ret));
> +		return ret;

This setup function is only called from probe, so we can use

		return dev_err_probe() here and below.

> +	}
> +

Wouldn't mind a comment here explaining why these values were chosen as
the default.

> +	chip->ps_conf[CM36686_PS_CONF1] = CM36686_PS_INT_IN |
> +		CM36686_PS_INT_OUT | CM36686_PS_DR_1_320 |
> +		CM36686_PS_PERS_2 | CM36686_PS_IT_2_5T;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
> +					chip->ps_conf[CM36686_PS_CONF1]);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	chip->ps_conf[CM36686_PS_CONF3] = CM36686_PS_SMART_PERS_ENABLE;
> +
> +	ret = device_property_read_u32(&client->dev,
> +		"capella,proximity-led-current", &led_current);

Only -EINVAL means the property was not present. Other errors can be
propigated. Also, handling of the default value if the property isn't
present isn't clear.

> +	if (!ret) {
> +		led_index = cm36686_current_to_index(led_current);
> +		if (led_index < 0) {
> +			dev_err(&client->dev, "No appropriate current for IR LED found.");
> +			return led_index;
> +		}
> +
> +		chip->ps_conf[CM36686_PS_CONF3] &= ~CM36686_LED_I;
> +		chip->ps_conf[CM36686_PS_CONF3] |= FIELD_PREP(CM36686_LED_I, led_index);
> +	}
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF3,
> +					chip->ps_conf[CM36686_PS_CONF3]);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(&client->dev, "proximity-near-level",
> +					    &chip->ps_close);
> +	if (ret < 0)
> +		chip->ps_close = 0;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDH,
> +		chip->ps_close);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to set close proximity threshold: %pe", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	chip->ps_away = chip->ps_close;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDL,
> +		chip->ps_away);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to set away proximity threshold: %pe", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cm36686_shutdown(void *data)
> +{
> +	struct cm36686_data *chip = data;
> +	struct i2c_client *client = chip->client;
> +	int ret, als_shutdown, ps_shutdown;
> +
> +	als_shutdown = chip->als_conf | CM36686_ALS_SD;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
> +					als_shutdown);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to shutdown ALS");
> +
> +	ps_shutdown = chip->ps_conf[CM36686_PS_CONF1] | CM36686_PS_SD;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
> +					ps_shutdown);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to shutdown PS");
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(chip->supplies), chip->supplies);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to disable regulators");

If we use devm_regulator_bulk_get_enable(), then we don't need to disable
regulators here and we can drop the chip->supplies field from the struct.

> +}
> +
> +static int cm36686_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct cm36686_data *chip;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev,
> +					  sizeof(struct cm36686_data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +

> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +
> +	const struct cm36686_chip_info *info =
> +		&cm36686_chip_info_tbl[id->driver_data];

After getting rid of the array, this is replaced with i2c_get_match_data().

> +
> +	ret = i2c_smbus_read_byte_data(client, CM36686_REG_ID_FLAG);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret, "Failed to read device ID");
> +
> +	if (ret != CM36686_DEVICE_ID)
> +		return dev_err_probe(&client->dev, -ENODEV, "Device not recognized!");

If anything, this should only be a warning. Too many times, we get a chip that
doesn't have the "right" ID but is still compatible.

> +
> +	i2c_set_clientdata(client, indio_dev);
> +	chip->client = client;
> +	ret = devm_mutex_init(&client->dev, &chip->lock);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +		       "Failed to initialize mutex");
> +
> +	chip->supplies[CM36686_SUPPLY_VDD].supply = "vdd";
> +	chip->supplies[CM36686_SUPPLY_VDDIO].supply = "vddio";
> +	chip->supplies[CM36686_SUPPLY_VLED].supply = "vled";
> +
> +	ret = devm_regulator_bulk_get(&client->dev,

This needs to be devm_regulator_bulk_get_enable(), or it doesn't actually
enable the regulators.

> +		ARRAY_SIZE(chip->supplies), chip->supplies);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to enable regulators");
> +
> +	ret = devm_add_action_or_reset(&client->dev, cm36686_shutdown, chip);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to set shutdown action");

This cleanup action should not be added until after the setup function
since that is what it is undoing.

> +
> +	ret = cm36686_setup(chip);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to set up registers");
> +
> +	indio_dev->name = info->name;
> +	indio_dev->channels = info->channels;
> +	indio_dev->num_channels = info->num_channels;
> +	indio_dev->info = &cm36686_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					cm36686_irq_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to request irq");

It should be OK to make the interrupt optional since that only affects
events AFAICT. There would need to be a second cm36686_info struct without
the events to handle this.

> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to register iio device");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id cm36686_id[] = {
> +	{ "cm36686", CM36686 },
> +	{ "cm36672p", CM36672P },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm36686_id);
> +
> +static const struct of_device_id cm36686_of_match[] = {
> +	{ .compatible = "capella,cm36686" },
> +	{ .compatible = "capella,cm36672p" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cm36686_of_match);
> +
> +static struct i2c_driver cm36686_driver = {
> +	.driver = {
> +		.name = "cm36686",
> +		.of_match_table = cm36686_of_match,
> +	},
> +	.probe = cm36686_probe,
> +	.id_table = cm36686_id

Trailing comma here.

> +};
> +

Usually no blank line here.

> +module_i2c_driver(cm36686_driver);
> +
> +MODULE_AUTHOR("Erikas Bitovtas <xerikasxx@gmail.com>");
> +MODULE_DESCRIPTION("CM36686 ambient light and proximity sensor driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Erikas Bitovtas]

On 2/1/26 8:45 PM, David Lechner wrote:
> On 2/1/26 11:03 AM, Erikas Bitovtas wrote:
>> This driver adds the initial support for the Capella cm36686
>> ambient light and proximity sensor and cm36672p proximity sensor.
>>
>> Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
>> ---
>>  drivers/iio/light/Kconfig   |  11 +
>>  drivers/iio/light/Makefile  |   1 +
>>  drivers/iio/light/cm36686.c | 810 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 822 insertions(+)
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index ac1408d374c9..b1d1943dec33 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -220,6 +220,17 @@ config CM36651
>>  	  To compile this driver as a module, choose M here:
>>  	  the module will be called cm36651.
>>  
>> +config CM36686
>> +	depends on I2C
>> +	tristate "CM36686 driver"
>> +	help
>> +	  Say Y here if you use cm36686.
>> +	  This option enables ambient light & proximity sensor using
>> +	  Capella cm36686 device driver.
>> +
>> +	  To compile this driver as a module, choose M here:
>> +	  the module will be called cm36686.
>> +
>>  config IIO_CROS_EC_LIGHT_PROX
>>  	tristate "ChromeOS EC Light and Proximity Sensors"
>>  	depends on IIO_CROS_EC_SENSORS_CORE
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index c0048e0d5ca8..806df80f6454 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_CM3232)		+= cm3232.o
>>  obj-$(CONFIG_CM3323)		+= cm3323.o
>>  obj-$(CONFIG_CM3605)		+= cm3605.o
>>  obj-$(CONFIG_CM36651)		+= cm36651.o
>> +obj-$(CONFIG_CM36686)		+= cm36686.o
>>  obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
>>  obj-$(CONFIG_GP2AP002)		+= gp2ap002.o
>>  obj-$(CONFIG_GP2AP020A00F)	+= gp2ap020a00f.o
>> diff --git a/drivers/iio/light/cm36686.c b/drivers/iio/light/cm36686.c
>> new file mode 100644
>> index 000000000000..eb108af7226d
>> --- /dev/null
>> +++ b/drivers/iio/light/cm36686.c
>> @@ -0,0 +1,810 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
> 
> I would expect copyright statements to be preserved here since the cover
> letter mentions much of this was derived from existing code.
> 
>> +
>> +#include <asm-generic/errno-base.h>
> 
> Should not be using generic, it may be different from arch-specific.
> 
> Generally we #include <linux/err.h> instead.
> 
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device/devres.h>
>> +#include <linux/iio/types.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/property.h>
>> +#include <linux/i2c.h>
> 
> Alphabetial order please.
> 
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/interrupt.h>
> 
> linux/iio/*.h can be grouped seperately.
> 
>> +
>> +/* Device registers */
>> +#define CM36686_REG_ALS_CONF		0x00
>> +#define CM36686_REG_PS_CONF1		0x03
>> +#define CM36686_REG_PS_CONF3		0x04
>> +#define CM36686_REG_PS_THDL		0x06
>> +#define CM36686_REG_PS_THDH		0x07
>> +#define CM36686_REG_PS_DATA		0x08
>> +#define CM36686_REG_ALS_DATA		0x09
>> +#define CM36686_REG_INT_FLAG		0x0B
>> +#define CM36686_REG_ID_FLAG		0x0C
>> +
>> +/* ALS_CONF */
>> +#define CM36686_ALS_IT			GENMASK(7, 6)
>> +#define CM36686_ALS_GAIN		GENMASK(3, 2)
>> +#define CM36686_ALS_INT_EN		BIT(1)
>> +#define CM36686_ALS_SD			BIT(0)
>> +
>> +/* PS_CONF1 */
>> +#define CM36686_PS_DR			GENMASK(7, 6)
>> +#define CM36686_PS_PERS			GENMASK(5, 4)
>> +#define CM36686_PS_IT			GENMASK(3, 1)
>> +#define CM36686_PS_SD			BIT(0)
>> +
>> +#define CM36686_PS_INT_OUT		BIT(9)
>> +#define CM36686_PS_INT_IN		BIT(8)
>> +
>> +/* PS_CONF3 */
>> +#define CM36686_PS_SMART_PERS_ENABLE	BIT(4)
>> +
>> +#define CM36686_LED_I			GENMASK(10, 8)
>> +
>> +/* INT_FLAG */
>> +#define CM36686_PS_IF			GENMASK(9, 8)
>> +
>> +/* Default values */
>> +#define CM36686_ALS_ENABLE		0x00
>> +#define CM36686_PS_DR_1_320		FIELD_PREP(CM36686_PS_DR, 3)
>> +#define CM36686_PS_PERS_2		FIELD_PREP(CM36686_PS_PERS, 1)
>> +#define CM36686_PS_IT_2_5T		FIELD_PREP(CM36686_PS_IT, 3)
>> +#define CM36686_LED_I_100		FIELD_PREP(CM36686_LED_I, 2)
>> +
>> +/* Shifts */
>> +#define CM36686_INT_FLAG_SHIFT		8
>> +
>> +/* Max proximity thresholds */
>> +#define CM36686_MAX_PS_VALUE		(BIT(12) - 1)
>> +
>> +#define CM36686_DEVICE_ID		0x86
>> +
>> +enum {
>> +	CM36686,
>> +	CM36672P,
>> +};
> 
> We try to avoid an ID enum in new drivers.
> 
>> +
>> +enum cm36686_distance {
>> +	CM36686_AWAY = 1,
>> +	CM36686_CLOSE,
>> +	CM36686_BOTH
>> +};
>> +
>> +enum {
>> +	CM36686_PS_CONF1,
>> +	CM36686_PS_CONF3,
>> +	CM36686_PS_CONF_NUM
>> +};
>> +
>> +enum {
>> +	CM36686_SUPPLY_VDD,
>> +	CM36686_SUPPLY_VDDIO,
>> +	CM36686_SUPPLY_VLED,
>> +	CM36686_SUPPLY_NUM,
>> +};
>> +
>> +static const int cm36686_als_it_times[][2] = {
>> +	{0, 80000},
>> +	{0, 160000},
>> +	{0, 320000},
>> +	{0, 640000}
>> +};
> 
> We try to keep a consistent style of spaces inside of braces and
> trailing common on the last item (unless it is a sentinl value).
> 
> 	{ 0, 640000 },
> 
>> +
>> +static const int cm36686_ps_it_times[][2] = {
>> +	{0, 320},
>> +	{0, 480},
>> +	{0, 640},
>> +	{0, 800},
>> +	{0, 960},
>> +	{0, 1120},
>> +	{0, 1280},
>> +	{0, 2560}
>> +};
>> +
>> +static const int cm36686_ps_led_current[] = {
> 
> We try to always include the units in the identifier name.
> Is this milliamps or microamps?
Original submission for cm36672p says milliamps. In the device tree I wrote
microamps. My mistake, will fix in v2.
> 
>> +	50,
>> +	75,
>> +	100,
>> +	120,
>> +	140,
>> +	160,
>> +	180,
>> +	200
>> +};
>> +
>> +struct cm36686_data {
>> +	struct mutex lock;
> 
> Locks need to always have a comment explaining what they are protecting.
> 
>> +	struct i2c_client *client;
>> +	struct regulator_bulk_data supplies[CM36686_SUPPLY_NUM];
>> +	int als_conf;
>> +	int ps_conf[CM36686_PS_CONF_NUM];
>> +	int ps_close;
>> +	int ps_away;
>> +};
>> +
>> +struct cm36686_chip_info {
>> +	const char *name;
>> +	const struct iio_chan_spec *channels;
>> +	const int num_channels;
>> +};
>> +
>> +static int cm36686_current_to_index(int led_current)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cm36686_ps_led_current); i++)
>> +		if (led_current < cm36686_ps_led_current[i])
>> +			break;
>> +
>> +	return i > 0 ? i - 1 : -EINVAL;
> 
> Not sure I follow the logic here. Shouldn't this just `return i;`
> instead of `break;` and then unconditnionally `return -EINVAL;` at
> the end?
> 
> If not, it could use some comments to explain.
> 
>> +}
>> +
>> +static ssize_t cm36686_read_near_level(struct iio_dev *indio_dev,
>> +				       uintptr_t priv,
>> +				       const struct iio_chan_spec *chan,
>> +				       char *buf)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> +	return sysfs_emit(buf, "%u\n", chip->ps_close);
>> +}
>> +
>> +static const struct iio_chan_spec_ext_info cm36686_ext_info[] = {
>> +	{
>> +		.name = "nearlevel",
>> +		.shared = IIO_SEPARATE,
>> +		.read = cm36686_read_near_level,
>> +	},
>> +	{}
> 
> IIO style for sentinil values is `{ }` (with space in between).
> 
>> +};
>> +
>> +static const struct iio_event_spec cm36686_proximity_event_spec[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_FALLING,
>> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> +				 BIT(IIO_EV_INFO_ENABLE),
>> +	},
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_RISING,
>> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> +				 BIT(IIO_EV_INFO_ENABLE),
>> +	}
> 
> All of these should have trailing comma.
> 
>> +};
>> +
>> +static const struct iio_chan_spec cm36686_channels[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 
> IIO_LIGHT should have IIO_CHAN_INFO_SCALE to convert raw to convert the
> raw value to lux. If we don't have that info due to lack of documentation,
> then we should add a comment to explain that. Or maybe we can make an
> educated guess?
No documentation on this is available, but we know that ASUS in their driver for
cm36686 use 160ms integration time for light and, after applying their custom
calibration functions, divide received raw value by 25 if the device is cm36686,
or by 20 if it's cm36283. Maybe from there we can extrapolate the lux value like
this:
80ms -> raw / 12.5
160ms -> raw / 25
320ms -> raw / 50
640ms -> raw / 100
Not sure how accurate that would be, however.
Xiaomi gets IIO_CHAN_INFO_SCALE by multiplying a value retrieved from a device
tree property called "als_trans_ratio", which is not documented anywhere.
> 
>> +				      BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.address = CM36686_REG_ALS_DATA,
>> +	},
>> +	{
>> +		.type = IIO_PROXIMITY,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.address = CM36686_REG_PS_DATA,
>> +		.event_spec = cm36686_proximity_event_spec,
>> +		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
>> +		.ext_info = cm36686_ext_info
>> +	}
>> +};
>> +
>> +static const struct iio_chan_spec cm36672p_channels[] = {
>> +	{
>> +		.type = IIO_PROXIMITY,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.address = CM36686_REG_PS_DATA,
>> +		.event_spec = cm36686_proximity_event_spec,
>> +		.num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
>> +		.ext_info = cm36686_ext_info
>> +	}
>> +};
>> +
>> +static int cm36686_read_avail(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      const int **vals, int *type, int *length,
>> +			      long mask)
>> +{
>> +	if (mask != IIO_CHAN_INFO_INT_TIME)
>> +		return -EINVAL;
>> +
>> +	switch (chan->type) {
>> +	case IIO_LIGHT:
>> +		*vals = (int *)(cm36686_als_it_times);
>> +		*length = 2 * ARRAY_SIZE(cm36686_als_it_times);
>> +		*type = IIO_VAL_INT_PLUS_MICRO;
>> +		return IIO_AVAIL_LIST;
>> +	case IIO_PROXIMITY:
>> +		*vals = (int *)(cm36686_ps_it_times);
>> +		*length = 2 * ARRAY_SIZE(cm36686_ps_it_times);
>> +		*type = IIO_VAL_INT_PLUS_MICRO;
>> +		return IIO_AVAIL_LIST;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int cm36686_read_channel(struct cm36686_data *chip,
>> +				struct iio_chan_spec const *chan, int *val)
>> +{
>> +	struct i2c_client *client = chip->client;
>> +	int ret = IIO_VAL_INT;
>> +
>> +	int data = i2c_smbus_read_word_data(client, chan->address);
>> +
>> +	if (data < 0) {
>> +		dev_err(&client->dev, "Failed to read register: %pe", ERR_PTR(data));
> 
> The error is passed to userspace, so this error message doesn't
> seem useful (could end up flooding logs if userspace keeps reading).
> 
>> +		ret = -EIO;
> 
> Can just return directly here and avoid the local variable.
> 
>> +	} else {
>> +		*val = data;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int cm36686_read_int_time(struct cm36686_data *chip,
>> +				 struct iio_chan_spec const *chan, int *val,
>> +				 int *val2)
>> +{
>> +	int als_it_index, ps_it_index;
>> +
>> +	switch (chan->type) {
>> +	case IIO_LIGHT:
>> +		als_it_index = FIELD_GET(CM36686_ALS_IT, chip->als_conf);
>> +		*val = cm36686_als_it_times[als_it_index][0];
>> +		*val2 = cm36686_als_it_times[als_it_index][1];
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	case IIO_PROXIMITY:
>> +		ps_it_index = FIELD_GET(CM36686_PS_IT,
>> +			chip->ps_conf[CM36686_PS_CONF1]);
>> +		*val = cm36686_ps_it_times[ps_it_index][0];
>> +		*val2 = cm36686_ps_it_times[ps_it_index][1];
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int cm36686_write_light_int_time(struct cm36686_data *chip, int val2)
>> +{
>> +	struct i2c_client *client = chip->client;
>> +	int index = -1, ret, new_it_time;
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(cm36686_als_it_times); i++) {
>> +		if (cm36686_als_it_times[i][1] == val2) {
>> +			index = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (index == -1)
>> +		return -EINVAL;
>> +
>> +	new_it_time = chip->als_conf & ~CM36686_ALS_IT;
>> +	new_it_time |= FIELD_PREP(CM36686_ALS_IT, index);
> 
> Can use FIELD_MODIFY()?
> 
>> +
>> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_ALS_CONF,
>> +					new_it_time);
>> +	if (ret < 0)
>> +		dev_err(&client->dev,
>> +			"Failed to set ALS integration time: %pe", ERR_PTR(ret));
> 
> This error message can go too.
> 
>> +	else
>> +		chip->als_conf = new_it_time;
>> +
>> +	return ret;
>> +}
>> +
>> +static int cm36686_write_prox_int_time(struct cm36686_data *chip, int val2)
>> +{
>> +	struct i2c_client *client = chip->client;
>> +	int index = -1, ret, new_it_time;
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(cm36686_ps_it_times); i++) {
>> +		if (cm36686_ps_it_times[i][1] == val2) {
>> +			index = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (index == -1)
>> +		return -EINVAL;
>> +
>> +	new_it_time = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_IT;
>> +	new_it_time |= FIELD_PREP(CM36686_PS_IT, index);
>> +
>> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1,
>> +					new_it_time);
>> +	if (ret < 0)
>> +		dev_err(&client->dev, "Failed to set PS integration time: %pe",
>> +			ERR_PTR(ret));
>> +	else
>> +		chip->ps_conf[CM36686_PS_CONF1] = new_it_time;
>> +
>> +	return ret;
>> +}
>> +
>> +static int cm36686_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan, int *val,
>> +			    int *val2, long mask)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&chip->lock);
> 
> Can use linux/cleanup.h `guard(mutex)(&chip->lock);`. Then we can return
> directly below and get rid of ret.
> 
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = cm36686_read_channel(chip, chan, val);
>> +		break;
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		ret = cm36686_read_int_time(chip, chan, val, val2);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	mutex_unlock(&chip->lock);
>> +	return ret;
>> +}
>> +
>> +static int cm36686_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan, int val,
>> +			     int val2, long mask)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (val) /* Integration time more than 1s is not supported */
>> +		return -EINVAL;
>> +
>> +	if (mask != IIO_CHAN_INFO_INT_TIME)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&chip->lock);
>> +
>> +	switch (chan->type) {
>> +	case IIO_LIGHT:
>> +		ret = cm36686_write_light_int_time(chip, val2);
>> +		break;
>> +	case IIO_PROXIMITY:
>> +		ret = cm36686_write_prox_int_time(chip, val2);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	mutex_unlock(&chip->lock);
>> +	return ret;
>> +}
>> +
>> +static int cm36686_read_prox_thresh(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)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> +	if (chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>> +	switch (dir) {
>> +	case IIO_EV_DIR_RISING:
>> +		*val = chip->ps_close;
>> +		break;
>> +	case IIO_EV_DIR_FALLING:
>> +		*val = chip->ps_away;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static int cm36686_write_prox_thresh(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)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	struct i2c_client *client = chip->client;
>> +	int ret = 0, address, *thresh;
>> +
>> +	if (chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>> +	switch (dir) {
>> +	case IIO_EV_DIR_FALLING:
>> +		if (val > chip->ps_close || val < 0)
>> +			return -EINVAL;
>> +
>> +		address = CM36686_REG_PS_THDL;
>> +		thresh = &chip->ps_away;
>> +		break;
>> +	case IIO_EV_DIR_RISING:
>> +		if (val < chip->ps_away || val > CM36686_MAX_PS_VALUE)
>> +			return -EINVAL;
>> +
>> +		address = CM36686_REG_PS_THDH;
>> +		thresh = &chip->ps_close;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&chip->lock);
>> +
>> +	ret = i2c_smbus_write_word_data(client, address, val);
>> +	if (ret < 0)
>> +		dev_err(&client->dev,
>> +			"Failed to set PS threshold value: %pe", ERR_PTR(ret));
>> +	else
>> +		*thresh = val;
>> +
>> +	mutex_unlock(&chip->lock);
>> +	return ret;
> 
> In case it isn't obvious, previous comments about error messages and locks
> apply here and elsewhere too.
> 
>> +}
>> +
>> +static int cm36686_read_prox_event_config(struct iio_dev *indio_dev,
>> +					  const struct iio_chan_spec *chan,
>> +					  enum iio_event_type type,
>> +					  enum iio_event_direction dir)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> +	if (chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>> +	switch (dir) {
>> +	case IIO_EV_DIR_FALLING:
>> +		return FIELD_GET(CM36686_PS_INT_OUT, chip->ps_conf[CM36686_PS_CONF1]);
>> +	case IIO_EV_DIR_RISING:
>> +		return FIELD_GET(CM36686_PS_INT_IN, chip->ps_conf[CM36686_PS_CONF1]);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int cm36686_write_prox_event_config(struct iio_dev *indio_dev,
>> +					   const struct iio_chan_spec *chan,
>> +					   enum iio_event_type type,
>> +					   enum iio_event_direction dir,
>> +					   bool state)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	struct i2c_client *client = chip->client;
>> +	int ret = 0, new_ps_conf;
>> +
>> +	if (chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>> +	switch (dir) {
>> +	case IIO_EV_DIR_FALLING:
>> +		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_OUT;
>> +		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_OUT, state);
>> +		break;
>> +	case IIO_EV_DIR_RISING:
>> +		new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_INT_IN;
>> +		new_ps_conf |= FIELD_PREP(CM36686_PS_INT_IN, state);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&chip->lock);
>> +
>> +	ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1, new_ps_conf);
>> +	if (ret < 0)
>> +		dev_err(&client->dev,
>> +			"Failed to set proximity event interrupt config: %pe", ERR_PTR(ret));
>> +	else
>> +		chip->ps_conf[CM36686_PS_CONF1] = new_ps_conf;
>> +
>> +	mutex_unlock(&chip->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int cm36686_fallback_read_ps(struct iio_dev *indio_dev)
>> +{
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	struct i2c_client *client = chip->client;
>> +	int data = i2c_smbus_read_word_data(client, CM36686_REG_PS_DATA);
>> +
>> +	if (data < 0)
>> +		return data;
>> +
>> +	if (data < chip->ps_away)
>> +		return IIO_EV_DIR_FALLING;
>> +	else if (data > chip->ps_close)
> 
> Don't need else after if with unconditinoal return.
> 
>> +		return IIO_EV_DIR_RISING;
>> +	else
>> +		return IIO_EV_DIR_EITHER;
>> +}
>> +
>> +static irqreturn_t cm36686_irq_handler(int irq, void *data)
>> +{
>> +	struct iio_dev *indio_dev = data;
>> +	struct cm36686_data *chip = iio_priv(indio_dev);
>> +	struct i2c_client *client = chip->client;
>> +	int ev_dir, ret;
>> +	u64 ev_code;
>> +
>> +	/* Reading the interrupt flag acknowledges the interrupt */
>> +	ret = i2c_smbus_read_word_data(client, CM36686_REG_INT_FLAG);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Interrupt flag register read failed: %pe", ERR_PTR(ret));
> 
> Error messages in interrupt handlers should probably be rate-limited.
> 
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	ret >>= CM36686_INT_FLAG_SHIFT;
> 
> I think a FIELD_GET() would make more sense here.
> 
>> +	switch (ret) {
>> +	case CM36686_CLOSE:
>> +		ev_dir = IIO_EV_DIR_RISING;
>> +		break;
>> +	case CM36686_AWAY:
>> +		ev_dir = IIO_EV_DIR_FALLING;
>> +		break;
>> +	case CM36686_BOTH:
>> +		ev_dir = cm36686_fallback_read_ps(indio_dev);
>> +		if (ev_dir < 0) {
>> +			dev_err(&client->dev, "Failed to settle interrupt state: %pe",
>> +				ERR_PTR(ret));
>> +			return IRQ_HANDLED;
>> +		}
>> +		break;
>> +	default:
>> +		dev_err(&client->dev, "Unknown interrupt state: %x", ret);
>> +		return IRQ_HANDLED;
>> +	}
>> +	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, IIO_EV_INFO_VALUE,
>> +				       IIO_EV_TYPE_THRESH, ev_dir);
>> +
>> +	iio_push_event(indio_dev, ev_code, iio_get_time_ns(indio_dev));
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info cm36686_info = {
>> +	.read_avail =		cm36686_read_avail,
>> +	.read_raw =		cm36686_read_raw,
>> +	.write_raw =		cm36686_write_raw,
>> +	.read_event_value =	cm36686_read_prox_thresh,
>> +	.write_event_value =	cm36686_write_prox_thresh,
>> +	.read_event_config =	cm36686_read_prox_event_config,
>> +	.write_event_config =	cm36686_write_prox_event_config,
>> +};
>> +
>> +static struct cm36686_chip_info cm36686_chip_info_tbl[] = {
>> +	[CM36686] = {
>> +		.name = "cm36686",
>> +		.channels = cm36686_channels,
>> +		.num_channels = ARRAY_SIZE(cm36686_channels),
>> +	},
>> +	[CM36672P] = {
>> +		.name = "cm36672p",
>> +		.channels = cm36672p_channels,
>> +		.num_channels = ARRAY_SIZE(cm36672p_channels),
>> +	},
>> +};
> 
> We avoid this array style in new drivers in favor of individual struct
> per chip.
> 
>> +
>> +static int cm36686_setup(struct cm36686_data *chip)
>> +{
>> +	struct i2c_client *client = chip->client;
>> +	int ret, led_current, led_index;
>> +
>> +	chip->als_conf = CM36686_ALS_ENABLE;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
>> +					chip->als_conf);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Failed to enable ambient light sensor: %pe", ERR_PTR(ret));
>> +		return ret;
> 
> This setup function is only called from probe, so we can use
> 
> 		return dev_err_probe() here and below.
> 
>> +	}
>> +
> 
> Wouldn't mind a comment here explaining why these values were chosen as
> the default.
> 
>> +	chip->ps_conf[CM36686_PS_CONF1] = CM36686_PS_INT_IN |
>> +		CM36686_PS_INT_OUT | CM36686_PS_DR_1_320 |
>> +		CM36686_PS_PERS_2 | CM36686_PS_IT_2_5T;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
>> +					chip->ps_conf[CM36686_PS_CONF1]);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
>> +		return ret;
>> +	}
>> +
>> +	chip->ps_conf[CM36686_PS_CONF3] = CM36686_PS_SMART_PERS_ENABLE;
>> +
>> +	ret = device_property_read_u32(&client->dev,
>> +		"capella,proximity-led-current", &led_current);
> 
> Only -EINVAL means the property was not present. Other errors can be
> propigated. Also, handling of the default value if the property isn't
> present isn't clear.
> 
>> +	if (!ret) {
>> +		led_index = cm36686_current_to_index(led_current);
>> +		if (led_index < 0) {
>> +			dev_err(&client->dev, "No appropriate current for IR LED found.");
>> +			return led_index;
>> +		}
>> +
>> +		chip->ps_conf[CM36686_PS_CONF3] &= ~CM36686_LED_I;
>> +		chip->ps_conf[CM36686_PS_CONF3] |= FIELD_PREP(CM36686_LED_I, led_index);
>> +	}
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF3,
>> +					chip->ps_conf[CM36686_PS_CONF3]);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Failed to enable proximity sensor: %pe", ERR_PTR(ret));
>> +		return ret;
>> +	}
>> +
>> +	ret = device_property_read_u32(&client->dev, "proximity-near-level",
>> +					    &chip->ps_close);
>> +	if (ret < 0)
>> +		chip->ps_close = 0;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDH,
>> +		chip->ps_close);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Failed to set close proximity threshold: %pe", ERR_PTR(ret));
>> +		return ret;
>> +	}
>> +
>> +	chip->ps_away = chip->ps_close;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDL,
>> +		chip->ps_away);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev,
>> +			"Failed to set away proximity threshold: %pe", ERR_PTR(ret));
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void cm36686_shutdown(void *data)
>> +{
>> +	struct cm36686_data *chip = data;
>> +	struct i2c_client *client = chip->client;
>> +	int ret, als_shutdown, ps_shutdown;
>> +
>> +	als_shutdown = chip->als_conf | CM36686_ALS_SD;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
>> +					als_shutdown);
>> +	if (ret < 0)
>> +		dev_err(&client->dev, "Failed to shutdown ALS");
>> +
>> +	ps_shutdown = chip->ps_conf[CM36686_PS_CONF1] | CM36686_PS_SD;
>> +
>> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
>> +					ps_shutdown);
>> +	if (ret < 0)
>> +		dev_err(&client->dev, "Failed to shutdown PS");
>> +
>> +	ret = regulator_bulk_disable(ARRAY_SIZE(chip->supplies), chip->supplies);
>> +	if (ret < 0)
>> +		dev_err(&client->dev, "Failed to disable regulators");
> 
> If we use devm_regulator_bulk_get_enable(), then we don't need to disable
> regulators here and we can drop the chip->supplies field from the struct.
> 
>> +}
>> +
>> +static int cm36686_probe(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct cm36686_data *chip;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev,
>> +					  sizeof(struct cm36686_data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	chip = iio_priv(indio_dev);
>> +
> 
>> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> +
>> +	const struct cm36686_chip_info *info =
>> +		&cm36686_chip_info_tbl[id->driver_data];
> 
> After getting rid of the array, this is replaced with i2c_get_match_data().
> 
>> +
>> +	ret = i2c_smbus_read_byte_data(client, CM36686_REG_ID_FLAG);
>> +	if (ret < 0)
>> +		return dev_err_probe(&client->dev, ret, "Failed to read device ID");
>> +
>> +	if (ret != CM36686_DEVICE_ID)
>> +		return dev_err_probe(&client->dev, -ENODEV, "Device not recognized!");
> 
> If anything, this should only be a warning. Too many times, we get a chip that
> doesn't have the "right" ID but is still compatible.
> 
>> +
>> +	i2c_set_clientdata(client, indio_dev);
>> +	chip->client = client;
>> +	ret = devm_mutex_init(&client->dev, &chip->lock);
>> +	if (ret < 0)
>> +		return dev_err_probe(&client->dev, ret,
>> +		       "Failed to initialize mutex");
>> +
>> +	chip->supplies[CM36686_SUPPLY_VDD].supply = "vdd";
>> +	chip->supplies[CM36686_SUPPLY_VDDIO].supply = "vddio";
>> +	chip->supplies[CM36686_SUPPLY_VLED].supply = "vled";
>> +
>> +	ret = devm_regulator_bulk_get(&client->dev,
> 
> This needs to be devm_regulator_bulk_get_enable(), or it doesn't actually
> enable the regulators.
> 
>> +		ARRAY_SIZE(chip->supplies), chip->supplies);
>> +	if (ret < 0)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to enable regulators");
>> +
>> +	ret = devm_add_action_or_reset(&client->dev, cm36686_shutdown, chip);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to set shutdown action");
> 
> This cleanup action should not be added until after the setup function
> since that is what it is undoing.
> 
>> +
>> +	ret = cm36686_setup(chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to set up registers");
>> +
>> +	indio_dev->name = info->name;
>> +	indio_dev->channels = info->channels;
>> +	indio_dev->num_channels = info->num_channels;
>> +	indio_dev->info = &cm36686_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +					cm36686_irq_handler,
>> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +					indio_dev->name, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to request irq");
> 
> It should be OK to make the interrupt optional since that only affects
> events AFAICT. There would need to be a second cm36686_info struct without
> the events to handle this.
> 
>> +
>> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to register iio device");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id cm36686_id[] = {
>> +	{ "cm36686", CM36686 },
>> +	{ "cm36672p", CM36672P },
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, cm36686_id);
>> +
>> +static const struct of_device_id cm36686_of_match[] = {
>> +	{ .compatible = "capella,cm36686" },
>> +	{ .compatible = "capella,cm36672p" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, cm36686_of_match);
>> +
>> +static struct i2c_driver cm36686_driver = {
>> +	.driver = {
>> +		.name = "cm36686",
>> +		.of_match_table = cm36686_of_match,
>> +	},
>> +	.probe = cm36686_probe,
>> +	.id_table = cm36686_id
> 
> Trailing comma here.
> 
>> +};
>> +
> 
> Usually no blank line here.
> 
>> +module_i2c_driver(cm36686_driver);
>> +
>> +MODULE_AUTHOR("Erikas Bitovtas <xerikasxx@gmail.com>");
>> +MODULE_DESCRIPTION("CM36686 ambient light and proximity sensor driver");
>> +MODULE_LICENSE("GPL");
>>
> 

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[David Lechner]

On 2/2/26 6:04 AM, Erikas Bitovtas wrote:
> 
> 
> On 2/1/26 8:45 PM, David Lechner wrote:
>> On 2/1/26 11:03 AM, Erikas Bitovtas wrote:

Pro tip: when you reply, trim out the not relevant parts like this.
It was hard to find your actual reply.

...

>>> +static const struct iio_chan_spec cm36686_channels[] = {
>>> +	{
>>> +		.type = IIO_LIGHT,
>>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>
>> IIO_LIGHT should have IIO_CHAN_INFO_SCALE to convert raw to convert the
>> raw value to lux. If we don't have that info due to lack of documentation,
>> then we should add a comment to explain that. Or maybe we can make an
>> educated guess?
> No documentation on this is available, but we know that ASUS in their driver for
> cm36686 use 160ms integration time for light and, after applying their custom
> calibration functions, divide received raw value by 25 if the device is cm36686,
> or by 20 if it's cm36283. Maybe from there we can extrapolate the lux value like
> this:
> 80ms -> raw / 12.5
> 160ms -> raw / 25
> 320ms -> raw / 50
> 640ms -> raw / 100
> Not sure how accurate that would be, however.
> Xiaomi gets IIO_CHAN_INFO_SCALE by multiplying a value retrieved from a device
> tree property called "als_trans_ratio", which is not documented anywhere.
The ASUS driver is an input driver, not IIO, so not sure that helps.

We could start with hard-coding the als_trans_ratio in this driver for the
scale (and write comments explaining where it came from and what we know
and don't know). Then at least we would have a scale. And if anyone needs
something more accurate and reverse-engineers it we could fix it up later
with a DT property or something like that.

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Erikas Bitovtas]

> Pro tip: when you reply, trim out the not relevant parts like this.

Noted.
> We could start with hard-coding the als_trans_ratio in this driver for the
> scale (and write comments explaining where it came from and what we know
> and don't know). Then at least we would have a scale. And if anyone needs
> something more accurate and reverse-engineers it we could fix it up later
> with a DT property or something like that.
> 

In arch/arm/boot/dts/qcom/libra/common-proximity.dtsi Xiaomi sets this property
to 16, and they set integration time in their driver to 160ms. Maybe
als_trans_ratio has something to do with integration time? In that case, we can
calculate the scale from a formula, like this:
scale = cm36686_als_it_times[index][1] / 10000 * 40000
val = scale / 1000000
val2 = scale % 1000000
This would allow us to preserve lux value when changing integration time.

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[David Lechner]

On 2/3/26 9:41 AM, Erikas Bitovtas wrote:
>> Pro tip: when you reply, trim out the not relevant parts like this.
> 
> Noted.
>> We could start with hard-coding the als_trans_ratio in this driver for the
>> scale (and write comments explaining where it came from and what we know
>> and don't know). Then at least we would have a scale. And if anyone needs
>> something more accurate and reverse-engineers it we could fix it up later
>> with a DT property or something like that.
>>
> 
> In arch/arm/boot/dts/qcom/libra/common-proximity.dtsi Xiaomi sets this property
> to 16, and they set integration time in their driver to 160ms. Maybe
> als_trans_ratio has something to do with integration time? 

I assume you have the hardware and can do a test to confirm this. :-)

> In that case, we can
> calculate the scale from a formula, like this:
> scale = cm36686_als_it_times[index][1] / 10000 * 40000
> val = scale / 1000000
> val2 = scale % 1000000
> This would allow us to preserve lux value when changing integration time.

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Erikas Bitovtas]

> I assume you have the hardware and can do a test to confirm this. :-)

I tested the formula used in Xiaomi's driver in the mainline kernel and
compared it against the downstream kernel, and the lux readings match between
them. Therefore, I suppose this solution could be good for now.
Although my formula for calculating scale is wrong - it should be inverse
proportionate to integration time. So:
80ms -> 1.28
160ms -> 0.64 (what Xiaomi uses)
320ms -> 0.32
640ms -> 0.16
This can be achieved by multiplying als_trans_ratio by integration time used in
Xiaomi's driver, and then dividing by integration time we have set up at the
given moment.
For example, suppose we set our int time in mainline to 80ms
16 (als_trans_ration) * 16 (160ms / 10000) = 256
256 / 8 (80ms / 10000) = 32
Starting here, we calculate based on how Xiaomi calculates scale for their values:
32 * 40000 = 1280000
val = 1280000 / 1000000 = 1
val2 = 1280000 % 1000000 = 280000
And that leaves us with scale of 1.280000.
Of course, comments on how we came to this value would be included as well.

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Jonathan Cameron]

On Wed, 4 Feb 2026 18:34:50 +0200
Erikas Bitovtas <xerikasxx@gmail.com> wrote:

> > I assume you have the hardware and can do a test to confirm this. :-)  
> 
> I tested the formula used in Xiaomi's driver in the mainline kernel and
> compared it against the downstream kernel, and the lux readings match between
> them. Therefore, I suppose this solution could be good for now.
> Although my formula for calculating scale is wrong - it should be inverse
> proportionate to integration time. So:
> 80ms -> 1.28
> 160ms -> 0.64 (what Xiaomi uses)
> 320ms -> 0.32
> 640ms -> 0.16
> This can be achieved by multiplying als_trans_ratio by integration time used in
> Xiaomi's driver, and then dividing by integration time we have set up at the
> given moment.
> For example, suppose we set our int time in mainline to 80ms
> 16 (als_trans_ration) * 16 (160ms / 10000) = 256
> 256 / 8 (80ms / 10000) = 32
> Starting here, we calculate based on how Xiaomi calculates scale for their values:
> 32 * 40000 = 1280000
> val = 1280000 / 1000000 = 1
> val2 = 1280000 % 1000000 = 280000
> And that leaves us with scale of 1.280000.
> Of course, comments on how we came to this value would be included as well.

Only think I'd add to this is that if we have numbers for
a sensor in a specific device, it's common for that devices
packaging to attenuate the measured light level a little.

For that reason we do sometimes have a tweak factor in DT.

Jonathan

Re: [PATCH 2/2] iio: light: Add support for Capella cm36686 and cm36672p sensors

[Jonathan Cameron]

On Sun, 01 Feb 2026 19:03:49 +0200
Erikas Bitovtas <xerikasxx@gmail.com> wrote:

> This driver adds the initial support for the Capella cm36686
> ambient light and proximity sensor and cm36672p proximity sensor.
> 
> Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>
Hi Erikas

Welcome to IIO.
A few comments from me to supplement existing review from David.
There may be some overlap!

Jonathan



> diff --git a/drivers/iio/light/cm36686.c b/drivers/iio/light/cm36686.c
> new file mode 100644
> index 000000000000..eb108af7226d
> --- /dev/null
> +++ b/drivers/iio/light/cm36686.c

> +/* PS_CONF3 */
> +#define CM36686_PS_SMART_PERS_ENABLE	BIT(4)
> +
> +#define CM36686_LED_I			GENMASK(10, 8)
> +
> +/* INT_FLAG */
> +#define CM36686_PS_IF			GENMASK(9, 8)
Use field names that incorporate the register and that they are masks

#define CM36686_INT_FLAG_PS_IF_MASK		GENMASK(9, 8)

That way it is easy to see they are being written to the right register.

> +
> +/* Default values */
> +#define CM36686_ALS_ENABLE		0x00
> +#define CM36686_PS_DR_1_320		FIELD_PREP(CM36686_PS_DR, 3)
> +#define CM36686_PS_PERS_2		FIELD_PREP(CM36686_PS_PERS, 1)
> +#define CM36686_PS_IT_2_5T		FIELD_PREP(CM36686_PS_IT, 3)
> +#define CM36686_LED_I_100		FIELD_PREP(CM36686_LED_I, 2)
I'd just put the FIELD_PREP() calls inline rather than having these macros
for defaults.  Looks like the values being written maybe want defines.
Not obvious 2 means 100 as per this last one for instance.


> +enum {
> +	CM36686_SUPPLY_VDD,
> +	CM36686_SUPPLY_VDDIO,
> +	CM36686_SUPPLY_VLED,
> +	CM36686_SUPPLY_NUM,
I'm not sure this enum really helps as you just
enable them all in one go anyway. Just use indexes there.

> +};
> +
> +static const int cm36686_als_it_times[][2] = {
> +	{0, 80000},
> +	{0, 160000},
> +	{0, 320000},
> +	{0, 640000}
> +};
> +
> +static const int cm36686_ps_it_times[][2] = {
> +	{0, 320},
> +	{0, 480},
> +	{0, 640},
> +	{0, 800},
> +	{0, 960},
> +	{0, 1120},
> +	{0, 1280},
> +	{0, 2560}
	{ 0, 2560 },
preferred style for these.

> +};

> +static const struct iio_chan_spec_ext_info cm36686_ext_info[] = {
> +	{
> +		.name = "nearlevel",
> +		.shared = IIO_SEPARATE,
> +		.read = cm36686_read_near_level,
> +	},
> +	{}
	{ }

> +};

> +
>
> +
> +static void cm36686_shutdown(void *data)
> +{
> +	struct cm36686_data *chip = data;
> +	struct i2c_client *client = chip->client;
> +	int ret, als_shutdown, ps_shutdown;
> +
> +	als_shutdown = chip->als_conf | CM36686_ALS_SD;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
> +					als_shutdown);

If I'm not missing anything, it would be easy to convert this driver
to regmap + enable regcache.  At that point you can use the rich
set of registration RMW functions in there and avoid need to do
your own caching like you have in als_conf / ps_conf etc.

> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to shutdown ALS");
> +
> +	ps_shutdown = chip->ps_conf[CM36686_PS_CONF1] | CM36686_PS_SD;
> +
> +	ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
> +					ps_shutdown);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to shutdown PS");
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(chip->supplies), chip->supplies);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to disable regulators");
> +}
> +
> +static int cm36686_probe(struct i2c_client *client)
> +{
I'd have
	struct device *dev = &client->dev;
just because it gets used a lot in probe.

> +	struct iio_dev *indio_dev;
> +	struct cm36686_data *chip;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev,
> +					  sizeof(struct cm36686_data));

sizeof(*chip)
is more compact and directly relates to us getting that space in 
chip = iio_priv() just below.

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
Keep declarations to the top of functions unless we are using
cleanup.h stuff or there is some other strong reason we can't.

Otherwise, this should move to getting the structs directly as David
called out.

> +
> +	const struct cm36686_chip_info *info =
> +		&cm36686_chip_info_tbl[id->driver_data];
> +
> +	ret = i2c_smbus_read_byte_data(client, CM36686_REG_ID_FLAG);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret, "Failed to read device ID");
> +
> +	if (ret != CM36686_DEVICE_ID)
> +		return dev_err_probe(&client->dev, -ENODEV, "Device not recognized!");
As David said already, we don't error out on this in modern drivers
because of DT fallback compatibles.  We have a long tail of old
drivers that do still check this and error out that really want updating.

> +
> +	i2c_set_clientdata(client, indio_dev);

Used?

> +	chip->client = client;
> +	ret = devm_mutex_init(&client->dev, &chip->lock);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +		       "Failed to initialize mutex");
I'm fairly sure that can only fail with -ENOMEM due to the devres bit
failing and in that case we don't print messages with dev_err_probe()
anyway so you can drop this print.

> +
> +	chip->supplies[CM36686_SUPPLY_VDD].supply = "vdd";
> +	chip->supplies[CM36686_SUPPLY_VDDIO].supply = "vddio";
> +	chip->supplies[CM36686_SUPPLY_VLED].supply = "vled";
> +
> +	ret = devm_regulator_bulk_get(&client->dev,
> +		ARRAY_SIZE(chip->supplies), chip->supplies);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to enable regulators");
> +
> +	ret = devm_add_action_or_reset(&client->dev, cm36686_shutdown, chip);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to set shutdown action");
> +
> +	ret = cm36686_setup(chip);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to set up registers");
> +
> +	indio_dev->name = info->name;
> +	indio_dev->channels = info->channels;
> +	indio_dev->num_channels = info->num_channels;
> +	indio_dev->info = &cm36686_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					cm36686_irq_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to request irq");
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to register iio device");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id cm36686_id[] = {
> +	{ "cm36686", CM36686 },
> +	{ "cm36672p", CM36672P },
> +	{}
As below.

> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm36686_id);
> +
> +static const struct of_device_id cm36686_of_match[] = {
> +	{ .compatible = "capella,cm36686" },
> +	{ .compatible = "capella,cm36672p" },
> +	{}
Really trivial style preference for IIO of
	{ }

A while back I wanted to have a consistent answer to how we
should format these and pretty much randomly selected that
one..

> +};
> +MODULE_DEVICE_TABLE(of, cm36686_of_match);