iio: light: cm36283: add support for Capella CM36283 and CM36686 ambient light and proximity sensors by erikas9987 · Pull Request #424 · msm8916-mainline/linux

erikas9987 · 2025-12-03T10:23:10Z

This PR adds support for Capella CM36283 and CM36686 ambient light and proximity sensors.
The following features are implemented:

Ambient light and proximity data register reading
Proximity event interrupts
Integration time adjustment
Proximity event threshold adjustment and disabling

These ambient light and proximity sensors are used in Asus ZenFone 2 Laser (all variants), Samsung Galaxy Grand Max and Xiaomi Mi Note 2.

TravMurav

Sorry for a delay, a few stylistic nitpicks below. I don't see any obvious issues and I think this is a totally upstreamable driver and the best review, ultimately, will be given upstream as well...

drivers/iio/light/cm36283.c

erikas9987 · 2025-12-14T21:28:39Z

I replaced capella,proximity-close-threshold with proximity-near-level so it gets picked up automatically by iio-sensor-proxy, and yet, it doesn't - monitor-sensor reports no proximity sensor available. Does anyone know why?

TravMurav · 2025-12-15T07:40:03Z

I replaced capella,proximity-close-threshold with proximity-near-level so it gets picked up automatically by iio-sensor-proxy, and yet, it doesn't - monitor-sensor reports no proximity sensor available. Does anyone know why?

I think you need to manually expose the value like here. In this case I'd think keeping vendor-specific near/far makes more sense but not sure what should be the userspace near-level, possibly the middle value between them (though idk how userspace will respond to hysteresis then)

https://elixir.bootlin.com/linux/v6.18.1/source/drivers/iio/light/ltr501.c#L524-L541

drivers/iio/light/cm36283.c

TravMurav · 2025-12-15T08:05:49Z

drivers/iio/light/cm36283.c

+}
+
+static int cm36283_set_prox_thresh(struct cm36283_data *chip)
+{


Okay, this is more interesting now that I look at it closer...

So we support two chips: one that can do hysteresis and needs two values (far/near) and one that doesn't where (far==near)...

Now we get an awkward situation with the dt binding: you mandate both of them to have near and far levels, but only one of the chips actually use both, and you don't have an IF check for the other one in the binding to make it clear it only applies to one of the chips...

I'm not sure if proximity-far-level is in the generic binding, but it feels to me that proximity-near-level has a fixed semantic and your use is slightly different. So unless there is a generic binding for far-level (or, well, if you want to argue upstream to add it) I'd define proximity-near-level as the middle point, then define some capella,proximity-hysteresis only for the chip that has far/near and define, say far=near_level+(hysteresis/2) near=near_level - (hysteresis/2). Then you have a clear middle point to expose to iio-sensor-proxy as well as for disambiguation for when you get both interrupt flags

So we support two chips: one that can do hysteresis and needs two values (far/near) and one that doesn't where (far==near)...

Both support hysteresis.
It's just that in CM36283 near and far values are packed into one register (CM36283_REG_PS_THD), while in CM36686 threshold values are split across two registers (CM36686_REG_PS_THDH for near, CM36686_REG_PS_THDL for far)

ah, sorry, I guess I shouldn't review code while a bit distracted xD

Then disregard the comment about dt binding checks though still not sure which is better - explicit near/far in the dt or near-level as middle point and capella,hysteresis to define actual values...

So unless there is a generic binding for far-level

There isn't. I added it for my device for consistency. It'd be strange to have such similar properties named so differently, though I'm not sure if I'd want to argue that it should be a generic binding, because not all proximity sensors support hysteresis...

TravMurav · 2025-12-15T08:17:38Z

drivers/iio/light/cm36283.c

+	else if (data > chip->ps_close)
+		return IIO_EV_DIR_RISING;
+	else
+		return IIO_EV_DIR_NONE;


What happens in this case? so, we lost an interrupt due to high load, then, when we re-read to settle it just so happens the near level is in the middle of the hysteresis window. Is the old reading kept?

not sure if this can happen:

far | mid | near * internal state = far ------------->* close=1 (missed) *<----------- close=1 far=1 (missed too) ---------->* close=1 far=1 (missed too) *<---- here we settle, see middle, say "none" (keep old?) so internal state = far --->* hysteresis not crossed so no new interrupt - internal state=far, real state=near

There is not really an "internal state" (IIUC by that you mean a variable that would track the proximity state in C code)
The driver reads the interrupt flag and then returns the appropriate IIO_EV_DIR right away. If both flags are set (meaning one of the interrupts was missed), proximity value is read from the PS_DATA register instead.
To be honest, I wasn't sure how to resolve this. Downstream just reads the near threshold value and compares it to that, but I figured that would not be appropriate for upstream.

yes, I meant more like, what the reast of the stack after the driver sees if the driver reports DIR_NONE? i.e. kernel iio core or iio-sensor-proxy et-al userspace

(...) so like, assuming we were looking at a phone dialer, would the display be blanked after above sequence of events happens or not, if it wasn't blanked initially?

I couldn't find a way to test that.
Also, now I think IIO_EV_DIR_EITHER would be more suitable, like here
IIO_EV_DIR_NONE would be for devices where there is no concept of direction

TravMurav · 2025-12-15T08:35:09Z

drivers/iio/light/cm36283.c

+	if (ret < 0)
+		dev_err(&client->dev, "Failed to shutdown ALS");
+
+	int ps_shutdown = chip->ps_conf[CM36283_PS_CONF1] | CM36283_PS_SD;


fwiw a few more non-c89 variables in this function

TravMurav · 2025-12-19T12:46:59Z

drivers/iio/light/cm36283.c

+			CM36283_PS_PERS_2,
+			CM36283_PS_SMART_PERS_ENABLE
+		},
+		.num_ps_it = ARRAY_SIZE(cm36283_ps_it_times),


nit: maybe len should be right after the array

a-andre · 2025-12-19T13:06:21Z

Documentation/devicetree/bindings/iio/light/capella,cm36283.yaml

+    maxItems: 1
+
+  vdd-supply:
+    description: |


Suggested change

description: |

description:

The same for the other descriptions

a-andre · 2025-12-19T13:10:23Z

Documentation/devicetree/bindings/iio/light/capella,cm36283.yaml

+  proximity-near-level:
+    description: |
+        Proximity hysteresis setting for detecting that the object is put
+        close to the sensor. Once this threshold is hit, the proximity
+        interrupt is triggered and the PS_CLOSE flag is raised.
+    type: integer


Add

allOf: - $ref: ../common.yaml#

and replace this by proximity-near-level: true

See https://github.com/msm8916-mainline/linux/blob/4c64d168c673b7e483451d3671e2a470e5eebbd4/Documentation/devicetree/bindings/iio/light/liteon%2Cltr501.yaml

a-andre · 2025-12-19T13:15:35Z

Documentation/devicetree/bindings/iio/light/capella,cm36283.yaml

+  proximity-far-level:
+    description: |
+        Proximity hysteresis setting for detecting that the object is put
+        away from the sensor. Once this threshold is hit, the proximity
+        interrupt is triggered and the PS_AWAY flag is raised.
+    type: integer


Maybe this could be moved to ../common.yaml, although this device is currently the only user.

If it stays here, you might have to add a prefix: proximity-far-level -> capella,proximity-far-level because it is a device specific property.

a-andre · 2025-12-19T13:18:36Z

Documentation/devicetree/bindings/iio/light/capella,cm36283.yaml

+  - interrupts
+  - vdd-supply
+  - vio-supply


Are these properties really required, especially interrupts?

The sensor can work in polling mode, but the driver doesn't support that yet.

The bindings should describe the hardware, i.e. also features not included in the driver yet. So maybe remove interrupts from required

a-andre · 2025-12-19T13:20:18Z

drivers/iio/light/cm36283.c

+#include "asm-generic/errno-base.h"
+#include "linux/array_size.h"
+#include "linux/bitfield.h"
+#include "linux/dev_printk.h"


This and maybe other includes shouldn't be necessary.

Replace "" by <> and fix order.

a-andre · 2025-12-19T13:25:24Z

drivers/iio/light/cm36283.c

+static const struct i2c_device_id cm36283_id_table[] = { { "cm36283" },
+							 { "cm36686" },
+							 {} };


Improve formatting.

Is this used by something?

a-andre · 2025-12-19T13:27:35Z

drivers/iio/light/cm36283.c

+
+	ret = devm_add_action_or_reset(&client->dev, cm36283_shutdown, chip);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,


The enabled regulators should be disabled again here and in the following error cases.

It might be sufficient to add it to the shutdown function.

a-andre · 2025-12-19T13:34:14Z

drivers/iio/light/cm36283.c

+		ret = cm36283_read_int_time(chip, chan, val, val2);
+		break;
+	default:
+		return -EINVAL;


Suggested change

return -EINVAL;

ret = -EINVAL;

To avoid issues with mutex.

a-andre · 2025-12-19T13:36:23Z

drivers/iio/light/cm36283.c

+	if (mask == IIO_CHAN_INFO_INT_TIME) {
+		switch (chan->type) {
+		case IIO_LIGHT:
+			*vals = chip->chip_info->als_it_times;
+			*length = chip->chip_info->num_als_it;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
+		case IIO_PROXIMITY:
+			*vals = chip->chip_info->ps_it_times;
+			*length = chip->chip_info->num_ps_it;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}


Use early return here, i.e.

if (mask != IIO_CHAN_INFO_INT_TIME) return -EINVAL;

wonderfulShrineMaidenOfParadise · 2025-12-24T23:38:34Z

drivers/iio/light/cm36283.c

+	const struct iio_chan_spec *channels;
+	const int num_channels;
+	const char **supplies;
+	const int num_supplies;


I am starting to learn regulators in drivers. Not sure why we need this.

cm36672p does not have an IIO_LIGHT channel, so it needs its own channel settings.
cm36283 does not have a vled-supply, so it needs its own supply strings.
As for num_{channels,supplies}, they need to be compile-time constants.

wonderfulShrineMaidenOfParadise · 2025-12-24T23:39:03Z

drivers/iio/light/cm36283.c

+	mutex_init(&chip->lock);
+
+	ret = devm_regulator_bulk_get_enable(
+		&client->dev, chip->chip_info->num_supplies, chip->chip_info->supplies);


Suggested change

&client->dev, chip->chip_info->num_supplies, chip->chip_info->supplies);

&client->dev, ARRAY_SIZE(chip->chip_info->supplies), chip->chip_info->supplies);

ARRAY_SIZE accepts compile-time constants only.

I haven't gotten the point yet. Does it have to be constant? Here is the reference.

msm8916-mainline/linux-mdss-dsi-panel-driver-generator@59775c7#diff-dba3766d7cec900b8de500f888c64a392cd9780f9baf00aae7e3f87a7d3fefc4L187

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

CM36672p sensors This driver adds the initial support for the Capella CM36283 and CM36686 ambient light and proximity sensors and CM36672P proximity sensor. Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>

erikas9987 force-pushed the cm36283-upstream branch from 584a418 to 4b02d56 Compare December 3, 2025 10:41

TravMurav reviewed Dec 14, 2025

View reviewed changes

erikas9987 force-pushed the cm36283-upstream branch 3 times, most recently from 7787d6b to cced392 Compare December 14, 2025 21:12

TravMurav reviewed Dec 15, 2025

View reviewed changes

erikas9987 force-pushed the cm36283-upstream branch from 1e753da to e4731d5 Compare December 19, 2025 12:28

TravMurav reviewed Dec 19, 2025

View reviewed changes

erikas9987 force-pushed the cm36283-upstream branch from e4731d5 to 4c64d16 Compare December 19, 2025 12:55

a-andre reviewed Dec 19, 2025

View reviewed changes

erikas9987 force-pushed the cm36283-upstream branch 3 times, most recently from 17944c7 to f95f871 Compare December 23, 2025 22:47

wonderfulShrineMaidenOfParadise reviewed Dec 24, 2025

View reviewed changes

erikas9987 force-pushed the cm36283-upstream branch from 160da58 to c37a427 Compare December 25, 2025 23:52

erikas9987 added 2 commits December 30, 2025 16:30

dt-bindings: Add binding document for CM36283

9d12c4a

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

iio: light: cm36283: Add support for Capella CM36283, CM36686 and

a601d8e

CM36672p sensors This driver adds the initial support for the Capella CM36283 and CM36686 ambient light and proximity sensors and CM36672P proximity sensor. Signed-off-by: Erikas Bitovtas <xerikasxx@gmail.com>

erikas9987 force-pushed the cm36283-upstream branch from c37a427 to a601d8e Compare December 30, 2025 14:35

	&client->dev, chip->chip_info->num_supplies, chip->chip_info->supplies);
	&client->dev, ARRAY_SIZE(chip->chip_info->supplies), chip->chip_info->supplies);

Conversation

erikas9987 commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravMurav left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikas9987 commented Dec 14, 2025

Uh oh!

TravMurav commented Dec 15, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikas9987 Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravMurav Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-andre Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonderfulShrineMaidenOfParadise Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

erikas9987 commented Dec 3, 2025 •

edited

Loading

erikas9987 Dec 16, 2025 •

edited

Loading

TravMurav Dec 15, 2025 •

edited

Loading

a-andre Dec 19, 2025 •

edited

Loading

wonderfulShrineMaidenOfParadise Dec 25, 2025 •

edited

Loading