[RFC 0/4] iio: light: add driver for Broadcom APDS9999

[RFC 0/4] iio: light: add driver for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

This series adds support for the Broadcom APDS9999 ambient light
sensor. The APDS9999 is a digital proximity and RGB sensor with
ALS capability, featuring I2C interface, programmable gain and
integration time.

This chip is considered by Broadcom to be the 'successor' to the
APDS-9960 which has technically been discontinued.

The initial driver implements the ALS/Lux functionality using the
green channel, which approximates the human eye spectral response.

Sent as RFC to gather feedback on the driver structure
and DT binding.

Jose A. Perez de Azpillaga (4):
  dt-bindings: iio: light: add DT binding for Broadcom APDS9999
  iio: light: add support for APDS9999 sensor
  iio: light: add build support for APDS9999
  MAINTAINERS: add myself as APDS9999 maintainer

 .../bindings/iio/light/brcm,apds9999.yaml     |  33 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/apds9999.c                  | 325 ++++++++++++++++++
 5 files changed, 376 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
 create mode 100644 drivers/iio/light/apds9999.c

[RFC 1/4] dt-bindings: iio: light: add DT binding for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

Add Device Tree binding documentation for the Broadcom APDS9999
ambient light sensor.

Proximity and RGB features are not yet implemented in the driver
and therefore not included in this binding.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 .../bindings/iio/light/brcm,apds9999.yaml     | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
new file mode 100644
index 000000000000..b20ef3677670
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/brcm,apds9999.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Broadcom APDS-9999 Ambient Light Sensor
+maintainers:
+  - Jose A. Perez de Azpillaga <azpijr@gmail.com>
+description: |
+  Datasheet: https://docs.broadcom.com/docs/APDS-9999-DS
+
+properties:
+  compatible:
+    enum:
+      - brcm,apds9999
+  reg:
+    maxItems: 1
+  vdd-supply: true
+additionalProperties: false
+required:
+  - compatible
+  - reg
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        light-sensor@52 {
+            compatible = "brcm,apds9999";
+            reg = <0x52>;
+            vdd-supply = <&vdd_reg>;
+        };
+    };

Re: [RFC 1/4] dt-bindings: iio: light: add DT binding for Broadcom APDS9999

[Jonathan Cameron]

On Mon, 11 May 2026 12:14:44 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

Title doesn't need to mention binding twice.

dt-bindings: iio: light: add Broadcomm ADPS9999

> Add Device Tree binding documentation for the Broadcom APDS9999
> ambient light sensor.
> 
> Proximity and RGB features are not yet implemented in the driver
> and therefore not included in this binding.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>

Can you combine this with the existing avago,apds9300.yaml binding?
Other than the missing interrupt in this file they look identical.

Note that a separate driver doesn't necessarily need a separate binding
doc.  May not be worth doing given the extra supply this part takes
(which you are currently missing by the look of it)

Other comments inline.

Thanks,

Jonathan

 
> ---
>  .../bindings/iio/light/brcm,apds9999.yaml     | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
> new file mode 100644
> index 000000000000..b20ef3677670
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/brcm,apds9999.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +title: Broadcom APDS-9999 Ambient Light Sensor
> +maintainers:
> +  - Jose A. Perez de Azpillaga <azpijr@gmail.com>
> +description: |
> +  Datasheet: https://docs.broadcom.com/docs/APDS-9999-DS
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,apds9999
blank line.  In general add some more above as well. Just follow style of
other similar files.

> +  reg:
> +    maxItems: 1
Blank line normal here.
> +  vdd-supply: true
and here
> +additionalProperties: false
and here
> +required:
> +  - compatible
> +  - reg
and here

Power supply should be required
Also you need to fully describe the part where possible. Here there
is an interrupt and a vcsel-supply by the look of it.

> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        light-sensor@52 {
> +            compatible = "brcm,apds9999";
> +            reg = <0x52>;
> +            vdd-supply = <&vdd_reg>;
> +        };
> +    };

Re: [RFC 1/4] dt-bindings: iio: light: add DT binding for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 01:26:35PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 12:14:44 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> Title doesn't need to mention binding twice.

I will fix it in v2.

> dt-bindings: iio: light: add Broadcomm ADPS9999
>
> > Add Device Tree binding documentation for the Broadcom APDS9999
> > ambient light sensor.
> >
> > Proximity and RGB features are not yet implemented in the driver
> > and therefore not included in this binding.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
>
> Can you combine this with the existing avago,apds9300.yaml binding?
> Other than the missing interrupt in this file they look identical.
>
> Note that a separate driver doesn't necessarily need a separate binding
> doc.  May not be worth doing given the extra supply this part takes
> (which you are currently missing by the look of it)

the VCSEL supply and required vdd-supply make a combined binding messy
with conditional schemas. keeping it separate.

...

> Power supply should be required

noted for v2.

> Also you need to fully describe the part where possible. Here there
> is an interrupt and a vcsel-supply by the look of it.

okay, I'll address everything said in v2.

--
jose a. p-a

[RFC 2/4] iio: light: add support for APDS9999 sensor

[Jose A. Perez de Azpillaga]

Add IIO driver for Broadcom APDS9999 ambient light sensor.

The APDS9999 is a digital proximity and RGB sensor with ALS
capability. This driver implements the ALS/Lux functionality using the
green channel.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/iio/light/apds9999.c | 325 +++++++++++++++++++++++++++++++++++
 1 file changed, 325 insertions(+)
 create mode 100644 drivers/iio/light/apds9999.c

diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
new file mode 100644
index 000000000000..cf44c3003415
--- /dev/null
+++ b/drivers/iio/light/apds9999.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for Broadcom APDS9999 Lux Light Sensor
+ *
+ * Uses the green channel (ALS) which approximates human eye spectral
+ * response for ambient light sensing (lux). RGB and proximity (PS)
+ * functionality not yet implemented.
+ *
+ * Copyright (C) 2026
+ * Author: Jose A. Perez de Azpillaga <azpijr@gmail.com>
+ *
+ * TODO: proximity and color sensor
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+/* Registers */
+#define APDS9999_REG_MAIN_CTRL		0x00
+#define APDS9999_REG_LS_MEAS_RATE	0x04
+#define APDS9999_REG_LS_GAIN		0x05
+#define APDS9999_REG_PART_ID		0x06
+#define APDS9999_REG_MAIN_STATUS	0x07
+#define APDS9999_REG_LS_DATA_GREEN_0	0x0D
+
+#define APDS9999_PART_ID		0xC2
+
+/* MAIN_CTRL */
+#define APDS9999_MAIN_CTRL_LS_EN	BIT(1)
+
+/* MAIN_STATUS */
+#define APDS9999_MAIN_STATUS_LS_DATA	BIT(3)
+
+/* LS_MEAS_RATE */
+#define APDS9999_LS_RES_SHIFT		4
+
+/*
+ * ALS gain options (register value -> factor)
+ */
+static const int apds9999_gains[] = {
+	1, 3, 6, 9, 18,
+};
+
+/*
+ * ALS resolution / integration time
+ * Resolution bits [6:4] set both the ADC bit-width and the
+ * integration time used in the lux conversion formula
+ * Value: integration time in microseconds
+ */
+enum apds9999_resolution {
+	APDS9999_RES_20BIT,	/* 400 ms */
+	APDS9999_RES_19BIT,	/* 200 ms */
+	APDS9999_RES_18BIT,	/* 100 ms (default) */
+	APDS9999_RES_17BIT,	/*  50 ms */
+	APDS9999_RES_16BIT,	/*  25 ms */
+	APDS9999_RES_13BIT,	/*  3.125 ms */
+	APDS9999_RES_NUM,
+};
+
+static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
+	[APDS9999_RES_20BIT] =   400000,
+	[APDS9999_RES_19BIT] =   200000,
+	[APDS9999_RES_18BIT] =   100000,
+	[APDS9999_RES_17BIT] =    50000,
+	[APDS9999_RES_16BIT] =    25000,
+	[APDS9999_RES_13BIT] =     3125,
+};
+
+#define APDS9999_DEFAULT_RES		APDS9999_RES_18BIT
+#define APDS9999_DEFAULT_GAIN_IDX	1	/* 3x */
+#define APDS9999_DEFAULT_RATE		2	/* 100 ms */
+
+struct apds9999_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	int als_gain_idx;	/* index into apds9999_gains[] */
+	int als_res;		/* index into apds9999_itimes_us[] */
+	int als_rate;		/* LS_MEAS_RATE[2:0] register value */
+};
+
+/*
+ * Hardware init, write default register values and enable ALS
+ */
+static int apds9999_init(struct apds9999_data *data)
+{
+	struct i2c_client *client = data->client;
+	u8 reg;
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	reg = (APDS9999_DEFAULT_RES << APDS9999_LS_RES_SHIFT) |
+	      APDS9999_DEFAULT_RATE;
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_MEAS_RATE, reg);
+	if (ret < 0)
+		goto out;
+
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_GAIN,
+					APDS9999_DEFAULT_GAIN_IDX);
+	if (ret < 0)
+		goto out;
+
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
+					APDS9999_MAIN_CTRL_LS_EN);
+	if (ret < 0)
+		goto out;
+
+	data->als_gain_idx = APDS9999_DEFAULT_GAIN_IDX;
+	data->als_res = APDS9999_DEFAULT_RES;
+	data->als_rate = APDS9999_DEFAULT_RATE;
+
+out:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+/*
+ * Read the 3-byte green-channel ADC value from 0x0D..0x0F
+ * Waits for LS_DATA_STATUS to be asserted (up to ~2x integration time)
+ */
+static int apds9999_als_read(struct apds9999_data *data, u32 *counts)
+{
+	struct i2c_client *client = data->client;
+	u8 buf[3];
+	int ret, tries;
+
+	mutex_lock(&data->lock);
+
+	/*
+	 * Poll MAIN_STATUS for new data.  Timeout: ~2 integration periods
+	 * plus margin.  Each try sleeps 20 ms
+	 */
+	tries = (apds9999_itimes_us[data->als_res] * 2) / 20000;
+	if (tries < 2)
+		tries = 2;
+
+	while (tries--) {
+		ret = i2c_smbus_read_byte_data(client,
+						APDS9999_REG_MAIN_STATUS);
+		if (ret < 0)
+			goto out;
+		if (ret & APDS9999_MAIN_STATUS_LS_DATA)
+			break;
+		msleep(20);
+	}
+
+	if (tries < 0) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	/* Block-read all 3 bytes of LS_DATA_GREEN */
+	ret = i2c_smbus_read_i2c_block_data(client,
+					     APDS9999_REG_LS_DATA_GREEN_0,
+					     3, buf);
+	if (ret < 0)
+		goto out;
+	if (ret != 3) {
+		ret = -EIO;
+		goto out;
+	}
+
+	*counts = buf[0] | ((u32)buf[1] << 8) | ((u32)(buf[2] & 0x0F) << 16);
+	ret = 0;
+
+out:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int apds9999_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct apds9999_data *data = iio_priv(indio_dev);
+	int gain, itime_us;
+	u64 scale_nano;
+	u32 counts;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = apds9999_als_read(data, &counts);
+		if (ret < 0)
+			return ret;
+		*val = (int)counts;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Scale (lux per count) = 54 / (gain * integration_time_ms)
+		 *
+		 * The constant 54 is derived from the datasheet table:
+		 *   at gain = 3x, itime = 100 ms → 0.180 lux/count
+		 *   → C = 0.180 * 3 * 100 = 54
+		 *
+		 * Expressed as IIO_VAL_INT_PLUS_NANO.
+		 * Uses itime_us directly to avoid truncation for
+		 * sub-millisecond integration times (e.g. 3.125 ms).
+		 */
+		gain = apds9999_gains[data->als_gain_idx];
+		itime_us = apds9999_itimes_us[data->als_res];
+
+		/* scale_nano = 54e12 / (gain * itime_us) nano-lux/count */
+		scale_nano = div_u64(54000000000000ULL,
+				     (u32)(gain * itime_us));
+		*val = (int)(scale_nano / 1000000000);
+		*val2 = (int)(scale_nano % 1000000000);
+		return IIO_VAL_INT_PLUS_NANO;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		*val2 = apds9999_itimes_us[data->als_res];
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec apds9999_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
+
+static const struct iio_info apds9999_info = {
+	.read_raw = apds9999_read_raw,
+};
+
+static int apds9999_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct apds9999_data *data;
+	struct iio_dev *indio_dev;
+	int ret, part_id;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+
+	part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
+	if (part_id < 0) {
+		dev_err(dev, "failed to read PART_ID: %d\n", part_id);
+		return part_id;
+	}
+	if (part_id != APDS9999_PART_ID) {
+		dev_err(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
+			part_id, APDS9999_PART_ID);
+		return -ENODEV;
+	}
+
+	dev_dbg(dev, "APDS-9999 detected, PART_ID=0x%02x\n", part_id);
+
+	ret = apds9999_init(data);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize device: %d\n", ret);
+		return ret;
+	}
+
+	indio_dev->name = "apds9999";
+	indio_dev->info = &apds9999_info;
+	indio_dev->channels = apds9999_channels;
+	indio_dev->num_channels = ARRAY_SIZE(apds9999_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register IIO device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Disable the light sensor on driver removal
+ */
+static void apds9999_remove(struct i2c_client *client)
+{
+	/* Set MAIN_CTRL to 0 → LS_EN cleared, sensor enters standby */
+	i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL, 0);
+}
+
+static const struct i2c_device_id apds9999_id[] = {
+	{ "apds9999", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, apds9999_id);
+
+static const struct of_device_id apds9999_of_match[] = {
+	{ .compatible = "brcm,apds9999" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, apds9999_of_match);
+
+static struct i2c_driver apds9999_driver = {
+	.driver = {
+		.name = "apds9999",
+		.of_match_table = apds9999_of_match,
+	},
+	.probe = apds9999_probe,
+	.remove = apds9999_remove,
+	.id_table = apds9999_id,
+};
+
+module_i2c_driver(apds9999_driver);
+
+MODULE_AUTHOR("Jose A. Perez de Azpillaga <azpijr@gmail.com>");
+MODULE_DESCRIPTION("APDS-9999 Lux Light Sensor IIO Driver");
+MODULE_LICENSE("GPL");

Re: [RFC 2/4] iio: light: add support for APDS9999 sensor

[Jonathan Cameron]

On Mon, 11 May 2026 12:14:47 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

> Add IIO driver for Broadcom APDS9999 ambient light sensor.
> 
> The APDS9999 is a digital proximity and RGB sensor with ALS
> capability. This driver implements the ALS/Lux functionality using the
> green channel.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>

Hi Jose,

For an RFC I'd normally expect to see a question list or other statement
of why it's an RFC rather than a formal submission.  One thing to note
is many reviewers won't even look at an RFC that doesn't have such
a list of questions.

Various comments inline but so far looks like a fairly standard light
sensor driver so all minor stuff.

Thanks,

Jonathan

> ---
>  drivers/iio/light/apds9999.c | 325 +++++++++++++++++++++++++++++++++++
>  1 file changed, 325 insertions(+)
>  create mode 100644 drivers/iio/light/apds9999.c
> 
> diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
> new file mode 100644
> index 000000000000..cf44c3003415
> --- /dev/null
> +++ b/drivers/iio/light/apds9999.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for Broadcom APDS9999 Lux Light Sensor
> + *
> + * Uses the green channel (ALS) which approximates human eye spectral
> + * response for ambient light sensing (lux). RGB and proximity (PS)
> + * functionality not yet implemented.

I'd not mention what is todo twice. That just makes churn in patches
adding them!

> + *
> + * Copyright (C) 2026
> + * Author: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> + *
> + * TODO: proximity and color sensor
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +/* Registers */
Ideally a set of defines should be self describing enough to
not need 'section' comments.  These comments have a nasty habbit
of becoming wrong over time!

> +#define APDS9999_REG_MAIN_CTRL		0x00
> +#define APDS9999_REG_LS_MEAS_RATE	0x04
> +#define APDS9999_REG_LS_GAIN		0x05
> +#define APDS9999_REG_PART_ID		0x06
> +#define APDS9999_REG_MAIN_STATUS	0x07
> +#define APDS9999_REG_LS_DATA_GREEN_0	0x0D
> +
> +#define APDS9999_PART_ID		0xC2
> +
> +/* MAIN_CTRL */
> +#define APDS9999_MAIN_CTRL_LS_EN	BIT(1)

I would keep these with the registers addresses above.
A nice trick to make them look visually different is to use some
additional indenting.  e.g.

#define APDS9999_REG_MAIN_CTRL		0x00
#define   APDS9999_MAIN_CTRL_LS_EN	BIT(1)

#define APDS9999_REG_LS_MEAS_RATE	0x04
#define APDS9999_REG_LS_GAIN		0x05
#define APDS9999_REG_PART_ID		0x06
#define APDS9999_REG_MAIN_STATUS	0x07
#define   APDS9999_MAIN_STATUS_LS_DATA	BIT(3)
#define APDS9999_REG_LS_DATA_GREEN_0	0x0D
> +
> +/* MAIN_STATUS */
> +#define APDS9999_MAIN_STATUS_LS_DATA	BIT(3)
> +
> +/* LS_MEAS_RATE */
> +#define APDS9999_LS_RES_SHIFT		4
Define a mask instead then use FIELD_PREP() / FIELD_GET()
That combines ensuring no overflow with a single representation of
the register field.

> +
> +/*
> + * ALS gain options (register value -> factor)
> + */
Where it fits use single line comments
/* ALS gain options (register value -> factor) */

Given they are specific register values, it may make sense
to use defines or an enum then explicit initialization

> +static const int apds9999_gains[] = {
> +	1, 3, 6, 9, 18,
	[ADPDS9999_GAIN_X1] = 1,
	[ADPDS9999_GAIN_X3] = 3,

Then if you are initializing defaults etc you can use the register
value define directly.  Similar to you've done for RES.

> +};
> +
> +/*
> + * ALS resolution / integration time
> + * Resolution bits [6:4] set both the ADC bit-width and the
> + * integration time used in the lux conversion formula
> + * Value: integration time in microseconds
> + */
> +enum apds9999_resolution {
> +	APDS9999_RES_20BIT,	/* 400 ms */
> +	APDS9999_RES_19BIT,	/* 200 ms */
> +	APDS9999_RES_18BIT,	/* 100 ms (default) */
> +	APDS9999_RES_17BIT,	/*  50 ms */
> +	APDS9999_RES_16BIT,	/*  25 ms */
> +	APDS9999_RES_13BIT,	/*  3.125 ms */
> +	APDS9999_RES_NUM,
No trailing comma as that has to be last one.
For the others I'd use explicit assignment e.g.
	APDS9999_RES_20BIT = 0,
etc so it is clear they are actual register field values not
just an enum where any unique values are enough.
> +};
> +
> +static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
> +	[APDS9999_RES_20BIT] =   400000,
> +	[APDS9999_RES_19BIT] =   200000,
> +	[APDS9999_RES_18BIT] =   100000,
> +	[APDS9999_RES_17BIT] =    50000,
> +	[APDS9999_RES_16BIT] =    25000,
> +	[APDS9999_RES_13BIT] =     3125,
> +};
> +
> +#define APDS9999_DEFAULT_RES		APDS9999_RES_18BIT
> +#define APDS9999_DEFAULT_GAIN_IDX	1	/* 3x */
> +#define APDS9999_DEFAULT_RATE		2	/* 100 ms */

Give defines for the actual register values.  Then use them in init()
rather than 'DEFAULT magic values. 

> +
> +struct apds9999_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
Locks must have a comment saying what data they are protecting.

> +	int als_gain_idx;	/* index into apds9999_gains[] */
> +	int als_res;		/* index into apds9999_itimes_us[] */
> +	int als_rate;		/* LS_MEAS_RATE[2:0] register value */
> +};
> +
> +/*
> + * Hardware init, write default register values and enable ALS
> + */
> +static int apds9999_init(struct apds9999_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 reg;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
As below - guard() helps in simple cases like this.

> +
> +	reg = (APDS9999_DEFAULT_RES << APDS9999_LS_RES_SHIFT) |
> +	      APDS9999_DEFAULT_RATE;

FIELD_PREP() for both of them so we won't need to figure out if they
are shifted or not.

> +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_MEAS_RATE, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_GAIN,
> +					APDS9999_DEFAULT_GAIN_IDX);

As mentioned above, actual define for that index rather than a default one.
The only place I expect to see what chosen defaults actually are is what
is passed to calls in this function.

> +	if (ret < 0)
> +		goto out;
> +
> +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> +					APDS9999_MAIN_CTRL_LS_EN);
> +	if (ret < 0)
> +		goto out;
> +
> +	data->als_gain_idx = APDS9999_DEFAULT_GAIN_IDX;
> +	data->als_res = APDS9999_DEFAULT_RES;
> +	data->als_rate = APDS9999_DEFAULT_RATE;
Given you don't unwind any of these written before an error (which is fine)
it makes more logical sense to assign these caches are each write is known
to succeed - that is spread them out in the function above.
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/*
> + * Read the 3-byte green-channel ADC value from 0x0D..0x0F
> + * Waits for LS_DATA_STATUS to be asserted (up to ~2x integration time)
> + */
> +static int apds9999_als_read(struct apds9999_data *data, u32 *counts)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 buf[3];
> +	int ret, tries;
> +
> +	mutex_lock(&data->lock);
Use guard(mutex)(&data->lock); and direct returns in error paths given no need
to get to the unlock once it is happening on leaving the scope of the function.

Make sure to also included cleanup.h for that.
> +
> +	/*
> +	 * Poll MAIN_STATUS for new data.  Timeout: ~2 integration periods
> +	 * plus margin.  Each try sleeps 20 ms

Do we have no way of having a better idea of how long we might need
to sleep?

> +	 */
> +	tries = (apds9999_itimes_us[data->als_res] * 2) / 20000;
> +	if (tries < 2)
> +		tries = 2;
	tries = max(2, tries);
Maybe combine with line above.

> +
> +	while (tries--) {
> +		ret = i2c_smbus_read_byte_data(client,
> +						APDS9999_REG_MAIN_STATUS);
> +		if (ret < 0)
> +			goto out;
> +		if (ret & APDS9999_MAIN_STATUS_LS_DATA)
> +			break;
> +		msleep(20);

fsleep() probably appropriate.

> +	}
> +
> +	if (tries < 0) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	/* Block-read all 3 bytes of LS_DATA_GREEN */
> +	ret = i2c_smbus_read_i2c_block_data(client,
> +					     APDS9999_REG_LS_DATA_GREEN_0,
> +					     3, buf);
sizeof(buf)
> +	if (ret < 0)
> +		goto out;
> +	if (ret != 3) {
sizeof(buf)
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	*counts = buf[0] | ((u32)buf[1] << 8) | ((u32)(buf[2] & 0x0F) << 16);
Smells like get_unaligned_le24() & GENMASK(20, 0)
or something like that.

> +	ret = 0;
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static int apds9999_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct apds9999_data *data = iio_priv(indio_dev);
> +	int gain, itime_us;
> +	u64 scale_nano;
> +	u32 counts;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = apds9999_als_read(data, &counts);
> +		if (ret < 0)
> +			return ret;
> +		*val = (int)counts;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Scale (lux per count) = 54 / (gain * integration_time_ms)
> +		 *
> +		 * The constant 54 is derived from the datasheet table:
> +		 *   at gain = 3x, itime = 100 ms → 0.180 lux/count
> +		 *   → C = 0.180 * 3 * 100 = 54
> +		 *
> +		 * Expressed as IIO_VAL_INT_PLUS_NANO.
> +		 * Uses itime_us directly to avoid truncation for
> +		 * sub-millisecond integration times (e.g. 3.125 ms).
> +		 */
> +		gain = apds9999_gains[data->als_gain_idx];
> +		itime_us = apds9999_itimes_us[data->als_res];
> +
> +		/* scale_nano = 54e12 / (gain * itime_us) nano-lux/count */
> +		scale_nano = div_u64(54000000000000ULL,
> +				     (u32)(gain * itime_us));
> +		*val = (int)(scale_nano / 1000000000);

NANO rather than making a reviewer count 0s ;)

> +		*val2 = (int)(scale_nano % 1000000000);
> +		return IIO_VAL_INT_PLUS_NANO;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = apds9999_itimes_us[data->als_res];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec apds9999_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),

Given this is mainly an RGB sensor I'd want to see the IIO_INTENSITY
channels there as well for an initial submission.  Understood
that you've sent this RFC for early feedback.


> +	},
> +};
> +
> +static const struct iio_info apds9999_info = {
> +	.read_raw = apds9999_read_raw,
> +};
> +
> +static int apds9999_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct apds9999_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret, part_id;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	mutex_init(&data->lock);
	ret = devm_mutex_init(&data->lock);
	if (ret)
		return ret;

The advantage this brings for lock debugging is pretty minor but it's
also cheap so for new code always use it.

> +
> +	part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
> +	if (part_id < 0) {
> +		dev_err(dev, "failed to read PART_ID: %d\n", part_id);
> +		return part_id;
> +	}
> +	if (part_id != APDS9999_PART_ID) {
> +		dev_err(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
> +			part_id, APDS9999_PART_ID);

We don't fail on missmatched IDs - it's fine to bring an info message but
not more than that.  The reason is fallback compatibles in device tree.
If a future device is fully compatible with this (it may have more features
and typically has a different part ID) we want the driver from
an old kernel to support it.  That only works if we don't error out on
failure to match IDs.
 
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(dev, "APDS-9999 detected, PART_ID=0x%02x\n", part_id);
> +
> +	ret = apds9999_init(data);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize device: %d\n", ret);
> +		return ret;
As below - return dev_err_probe()
> +	}
> +
> +	indio_dev->name = "apds9999";
> +	indio_dev->info = &apds9999_info;
> +	indio_dev->channels = apds9999_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(apds9999_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register IIO device: %d\n", ret);
> +		return ret;

For all error messages in probe() or functions called from probe() use
		return dev_err_probe(dev, ret, "failed to register IIO device\n");
which handles deferred probing, drop unnecessary prints such as those on memory
failure, pretty prints the return value and usefully is more compact.

> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Disable the light sensor on driver removal
> + */
> +static void apds9999_remove(struct i2c_client *client)
> +{
> +	/* Set MAIN_CTRL to 0 → LS_EN cleared, sensor enters standby */
> +	i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL, 0);
This looks to me like you will be doing tear down in a different order to
setup. For example the userspace interfaces will not be removeed until
after you've put the sensor in stand by.

There are two solutions to this (1st preferred)
1. Use devm_add_action_or_reset() to register the standby write here
   in a driver specific callback that is cleaned up in the right place.
   That might be registered in adps9999_init() rather than directly in probe.
   Once you've done that drop the remove() callback as no longer needed.
2. Stop using devm_ registration calls from whereever in probe this matches
   up with.

Basically you mustn't mix and match devm and non devm based cleanup.

> +}
> +
> +static const struct i2c_device_id apds9999_id[] = {
> +	{ "apds9999", 0 },
The , 0 doesn't add anything so drop it
	{ "apds9999" },

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, apds9999_id);
> +
> +static const struct of_device_id apds9999_of_match[] = {
> +	{ .compatible = "brcm,apds9999" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, apds9999_of_match);
> +
> +static struct i2c_driver apds9999_driver = {
> +	.driver = {
> +		.name = "apds9999",
> +		.of_match_table = apds9999_of_match,
> +	},
> +	.probe = apds9999_probe,
> +	.remove = apds9999_remove,
> +	.id_table = apds9999_id,
> +};
> +
Common convention to tightly compule this macro with the struct i2c_driver
by not having a blank line here.

> +module_i2c_driver(apds9999_driver);
> +
> +MODULE_AUTHOR("Jose A. Perez de Azpillaga <azpijr@gmail.com>");
> +MODULE_DESCRIPTION("APDS-9999 Lux Light Sensor IIO Driver");
> +MODULE_LICENSE("GPL");

Re: [RFC 2/4] iio: light: add support for APDS9999 sensor

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 02:06:44PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 12:14:47 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> > Add IIO driver for Broadcom APDS9999 ambient light sensor.
> >
> > The APDS9999 is a digital proximity and RGB sensor with ALS
> > capability. This driver implements the ALS/Lux functionality using the
> > green channel.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
>
> Hi Jose,
>
> For an RFC I'd normally expect to see a question list or other statement
> of why it's an RFC rather than a formal submission.  One thing to note
> is many reviewers won't even look at an RFC that doesn't have such
> a list of questions.
>
> Various comments inline but so far looks like a fairly standard light
> sensor driver so all minor stuff.

okay. I should've done a better research before submiting the RFC, I'll
add questions to the v2 cover letter.

> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/iio/light/apds9999.c | 325 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 325 insertions(+)
> >  create mode 100644 drivers/iio/light/apds9999.c
> >
> > diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
> > new file mode 100644
> > index 000000000000..cf44c3003415
> > --- /dev/null
> > +++ b/drivers/iio/light/apds9999.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * IIO driver for Broadcom APDS9999 Lux Light Sensor
> > + *
> > + * Uses the green channel (ALS) which approximates human eye spectral
> > + * response for ambient light sensing (lux). RGB and proximity (PS)
> > + * functionality not yet implemented.
>
> I'd not mention what is todo twice. That just makes churn in patches
> adding them!

I'll remove the duplicate.

> > + *
> > + * Copyright (C) 2026
> > + * Author: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> > + *
> > + * TODO: proximity and color sensor
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/math64.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +
> > +/* Registers */
> Ideally a set of defines should be self describing enough to
> not need 'section' comments.  These comments have a nasty habbit
> of becoming wrong over time!
>
> > +#define APDS9999_REG_MAIN_CTRL		0x00
> > +#define APDS9999_REG_LS_MEAS_RATE	0x04
> > +#define APDS9999_REG_LS_GAIN		0x05
> > +#define APDS9999_REG_PART_ID		0x06
> > +#define APDS9999_REG_MAIN_STATUS	0x07
> > +#define APDS9999_REG_LS_DATA_GREEN_0	0x0D
> > +
> > +#define APDS9999_PART_ID		0xC2
> > +
> > +/* MAIN_CTRL */
> > +#define APDS9999_MAIN_CTRL_LS_EN	BIT(1)
>
> I would keep these with the registers addresses above.
> A nice trick to make them look visually different is to use some
> additional indenting.  e.g.
>
> #define APDS9999_REG_MAIN_CTRL		0x00
> #define   APDS9999_MAIN_CTRL_LS_EN	BIT(1)
>
> #define APDS9999_REG_LS_MEAS_RATE	0x04
> #define APDS9999_REG_LS_GAIN		0x05
> #define APDS9999_REG_PART_ID		0x06
> #define APDS9999_REG_MAIN_STATUS	0x07
> #define   APDS9999_MAIN_STATUS_LS_DATA	BIT(3)
> #define APDS9999_REG_LS_DATA_GREEN_0	0x0D

okay, I'll remove section comments and use indent to look visually
different. thanks for the trick!

> > +
> > +/* MAIN_STATUS */
> > +#define APDS9999_MAIN_STATUS_LS_DATA	BIT(3)
> > +
> > +/* LS_MEAS_RATE */
> > +#define APDS9999_LS_RES_SHIFT		4
> Define a mask instead then use FIELD_PREP() / FIELD_GET()
> That combines ensuring no overflow with a single representation of
> the register field.

I'll address it in v2.

> > +
> > +/*
> > + * ALS gain options (register value -> factor)
> > + */
> Where it fits use single line comments
> /* ALS gain options (register value -> factor) */

okay, i used a shortcut and didn't notice that. I'll change it to a
single comment.

> Given they are specific register values, it may make sense
> to use defines or an enum then explicit initialization

> > +static const int apds9999_gains[] = {
> > +	1, 3, 6, 9, 18,
> 	[ADPDS9999_GAIN_X1] = 1,
> 	[ADPDS9999_GAIN_X3] = 3,
>
> Then if you are initializing defaults etc you can use the register
> value define directly.  Similar to you've done for RES.

noted.

> > +};
> > +
> > +/*
> > + * ALS resolution / integration time
> > + * Resolution bits [6:4] set both the ADC bit-width and the
> > + * integration time used in the lux conversion formula
> > + * Value: integration time in microseconds
> > + */
> > +enum apds9999_resolution {
> > +	APDS9999_RES_20BIT,	/* 400 ms */
> > +	APDS9999_RES_19BIT,	/* 200 ms */
> > +	APDS9999_RES_18BIT,	/* 100 ms (default) */
> > +	APDS9999_RES_17BIT,	/*  50 ms */
> > +	APDS9999_RES_16BIT,	/*  25 ms */
> > +	APDS9999_RES_13BIT,	/*  3.125 ms */
> > +	APDS9999_RES_NUM,
> No trailing comma as that has to be last one.
> For the others I'd use explicit assignment e.g.
> 	APDS9999_RES_20BIT = 0,
> etc so it is clear they are actual register field values not
> just an enum where any unique values are enough.

I'll add explicit register values to the resolution enum.

> > +};
> > +
> > +static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
> > +	[APDS9999_RES_20BIT] =   400000,
> > +	[APDS9999_RES_19BIT] =   200000,
> > +	[APDS9999_RES_18BIT] =   100000,
> > +	[APDS9999_RES_17BIT] =    50000,
> > +	[APDS9999_RES_16BIT] =    25000,
> > +	[APDS9999_RES_13BIT] =     3125,
> > +};
> > +
> > +#define APDS9999_DEFAULT_RES		APDS9999_RES_18BIT
> > +#define APDS9999_DEFAULT_GAIN_IDX	1	/* 3x */
> > +#define APDS9999_DEFAULT_RATE		2	/* 100 ms */
>
> Give defines for the actual register values.  Then use them in init()
> rather than 'DEFAULT magic values.

okay. I'll address it in v2.

> > +
> > +struct apds9999_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> Locks must have a comment saying what data they are protecting.

noted.

> > +	int als_gain_idx;	/* index into apds9999_gains[] */
> > +	int als_res;		/* index into apds9999_itimes_us[] */
> > +	int als_rate;		/* LS_MEAS_RATE[2:0] register value */
> > +};
> > +
> > +/*
> > + * Hardware init, write default register values and enable ALS
> > + */
> > +static int apds9999_init(struct apds9999_data *data)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	u8 reg;
> > +	int ret;
> > +
> > +	mutex_lock(&data->lock);
> As below - guard() helps in simple cases like this.

okay, noted.

> > +
> > +	reg = (APDS9999_DEFAULT_RES << APDS9999_LS_RES_SHIFT) |
> > +	      APDS9999_DEFAULT_RATE;
>
> FIELD_PREP() for both of them so we won't need to figure out if they
> are shifted or not.

okay, I'll address it for both fields.

> > +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_MEAS_RATE, reg);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_GAIN,
> > +					APDS9999_DEFAULT_GAIN_IDX);
>
> As mentioned above, actual define for that index rather than a default one.
> The only place I expect to see what chosen defaults actually are is what
> is passed to calls in this function.
>
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> > +					APDS9999_MAIN_CTRL_LS_EN);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	data->als_gain_idx = APDS9999_DEFAULT_GAIN_IDX;
> > +	data->als_res = APDS9999_DEFAULT_RES;
> > +	data->als_rate = APDS9999_DEFAULT_RATE;
> Given you don't unwind any of these written before an error (which is fine)
> it makes more logical sense to assign these caches are each write is known
> to succeed - that is spread them out in the function above.

I'll move the cached value assignments right after each successful
write, not al the end of the function.

> > +
> > +out:
> > +	mutex_unlock(&data->lock);
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Read the 3-byte green-channel ADC value from 0x0D..0x0F
> > + * Waits for LS_DATA_STATUS to be asserted (up to ~2x integration time)
> > + */
> > +static int apds9999_als_read(struct apds9999_data *data, u32 *counts)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	u8 buf[3];
> > +	int ret, tries;
> > +
> > +	mutex_lock(&data->lock);
> Use guard(mutex)(&data->lock); and direct returns in error paths given no need
> to get to the unlock once it is happening on leaving the scope of the function.
>
> Make sure to also included cleanup.h for that.

noted.

> > +
> > +	/*
> > +	 * Poll MAIN_STATUS for new data.  Timeout: ~2 integration periods
> > +	 * plus margin.  Each try sleeps 20 ms
>
> Do we have no way of having a better idea of how long we might need
> to sleep?

poll will sleep for half the integration time per try... the minimum
floor will still be applied via max()

> > +	 */
> > +	tries = (apds9999_itimes_us[data->als_res] * 2) / 20000;
> > +	if (tries < 2)
> > +		tries = 2;
> 	tries = max(2, tries);
> Maybe combine with line above.

noted.

> > +
> > +	while (tries--) {
> > +		ret = i2c_smbus_read_byte_data(client,
> > +						APDS9999_REG_MAIN_STATUS);
> > +		if (ret < 0)
> > +			goto out;
> > +		if (ret & APDS9999_MAIN_STATUS_LS_DATA)
> > +			break;
> > +		msleep(20);
>
> fsleep() probably appropriate.

right. I'll change it.

> > +	}
> > +
> > +	if (tries < 0) {
> > +		ret = -ETIMEDOUT;
> > +		goto out;
> > +	}
> > +
> > +	/* Block-read all 3 bytes of LS_DATA_GREEN */
> > +	ret = i2c_smbus_read_i2c_block_data(client,
> > +					     APDS9999_REG_LS_DATA_GREEN_0,
> > +					     3, buf);
> sizeof(buf)
> > +	if (ret < 0)
> > +		goto out;
> > +	if (ret != 3) {
> sizeof(buf)
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +	*counts = buf[0] | ((u32)buf[1] << 8) | ((u32)(buf[2] & 0x0F) << 16);
> Smells like get_unaligned_le24() & GENMASK(20, 0)
> or something like that.

will switch to get_unaligned_le24(buf) & GENMASK(19, 0) in v2.

> > +	ret = 0;
> > +
> > +out:
> > +	mutex_unlock(&data->lock);
> > +	return ret;
> > +}
> > +
> > +static int apds9999_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long mask)
> > +{
> > +	struct apds9999_data *data = iio_priv(indio_dev);
> > +	int gain, itime_us;
> > +	u64 scale_nano;
> > +	u32 counts;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = apds9999_als_read(data, &counts);
> > +		if (ret < 0)
> > +			return ret;
> > +		*val = (int)counts;
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/*
> > +		 * Scale (lux per count) = 54 / (gain * integration_time_ms)
> > +		 *
> > +		 * The constant 54 is derived from the datasheet table:
> > +		 *   at gain = 3x, itime = 100 ms → 0.180 lux/count
> > +		 *   → C = 0.180 * 3 * 100 = 54
> > +		 *
> > +		 * Expressed as IIO_VAL_INT_PLUS_NANO.
> > +		 * Uses itime_us directly to avoid truncation for
> > +		 * sub-millisecond integration times (e.g. 3.125 ms).
> > +		 */
> > +		gain = apds9999_gains[data->als_gain_idx];
> > +		itime_us = apds9999_itimes_us[data->als_res];
> > +
> > +		/* scale_nano = 54e12 / (gain * itime_us) nano-lux/count */
> > +		scale_nano = div_u64(54000000000000ULL,
> > +				     (u32)(gain * itime_us));
> > +		*val = (int)(scale_nano / 1000000000);
>
> NANO rather than making a reviewer count 0s ;)

sorry for that. I'll use NSEC_PER_SEC.


> > +		*val2 = (int)(scale_nano % 1000000000);
> > +		return IIO_VAL_INT_PLUS_NANO;
> > +
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		*val = 0;
> > +		*val2 = apds9999_itimes_us[data->als_res];
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_chan_spec apds9999_channels[] = {
> > +	{
> > +		.type = IIO_LIGHT,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
>
> Given this is mainly an RGB sensor I'd want to see the IIO_INTENSITY
> channels there as well for an initial submission.  Understood
> that you've sent this RFC for early feedback.

okay, I'll add it.

> > +	},
> > +};
> > +
> > +static const struct iio_info apds9999_info = {
> > +	.read_raw = apds9999_read_raw,
> > +};
> > +
> > +static int apds9999_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct apds9999_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret, part_id;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	mutex_init(&data->lock);
> 	ret = devm_mutex_init(&data->lock);
> 	if (ret)
> 		return ret;
>
> The advantage this brings for lock debugging is pretty minor but it's
> also cheap so for new code always use it.

noted.

> > +
> > +	part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
> > +	if (part_id < 0) {
> > +		dev_err(dev, "failed to read PART_ID: %d\n", part_id);
> > +		return part_id;
> > +	}
> > +	if (part_id != APDS9999_PART_ID) {
> > +		dev_err(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
> > +			part_id, APDS9999_PART_ID);
>
> We don't fail on missmatched IDs - it's fine to bring an info message but
> not more than that.  The reason is fallback compatibles in device tree.
> If a future device is fully compatible with this (it may have more features
> and typically has a different part ID) we want the driver from
> an old kernel to support it.  That only works if we don't error out on
> failure to match IDs.

okay, I'll change to dev_info() and continue with probe on mismatch.

> > +		return -ENODEV;
> > +	}
> > +
> > +	dev_dbg(dev, "APDS-9999 detected, PART_ID=0x%02x\n", part_id);
> > +
> > +	ret = apds9999_init(data);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to initialize device: %d\n", ret);
> > +		return ret;
> As below - return dev_err_probe()

all error paths in probe will use dev_err_probe() then.

> > +	}
> > +
> > +	indio_dev->name = "apds9999";
> > +	indio_dev->info = &apds9999_info;
> > +	indio_dev->channels = apds9999_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(apds9999_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register IIO device: %d\n", ret);
> > +		return ret;
>
> For all error messages in probe() or functions called from probe() use
> 		return dev_err_probe(dev, ret, "failed to register IIO device\n");
> which handles deferred probing, drop unnecessary prints such as those on memory
> failure, pretty prints the return value and usefully is more compact.

noted.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Disable the light sensor on driver removal
> > + */
> > +static void apds9999_remove(struct i2c_client *client)
> > +{
> > +	/* Set MAIN_CTRL to 0 → LS_EN cleared, sensor enters standby */
> > +	i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL, 0);
> This looks to me like you will be doing tear down in a different order to
> setup. For example the userspace interfaces will not be removeed until
> after you've put the sensor in stand by.
>
> There are two solutions to this (1st preferred)
> 1. Use devm_add_action_or_reset() to register the standby write here
>    in a driver specific callback that is cleaned up in the right place.
>    That might be registered in adps9999_init() rather than directly in probe.
>    Once you've done that drop the remove() callback as no longer needed.
> 2. Stop using devm_ registration calls from whereever in probe this matches
>    up with.
>
> Basically you mustn't mix and match devm and non devm based cleanup.

noted. I won't mix devm/non-devm cleanup.

> > +}
> > +
> > +static const struct i2c_device_id apds9999_id[] = {
> > +	{ "apds9999", 0 },
> The , 0 doesn't add anything so drop it
> 	{ "apds9999" },

I'll drop it.

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, apds9999_id);
> > +
> > +static const struct of_device_id apds9999_of_match[] = {
> > +	{ .compatible = "brcm,apds9999" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, apds9999_of_match);
> > +
> > +static struct i2c_driver apds9999_driver = {
> > +	.driver = {
> > +		.name = "apds9999",
> > +		.of_match_table = apds9999_of_match,
> > +	},
> > +	.probe = apds9999_probe,
> > +	.remove = apds9999_remove,
> > +	.id_table = apds9999_id,
> > +};
> > +
> Common convention to tightly compule this macro with the struct i2c_driver
> by not having a blank line here.

I'll remove the blank line in v2.

> > +module_i2c_driver(apds9999_driver);
> > +
> > +MODULE_AUTHOR("Jose A. Perez de Azpillaga <azpijr@gmail.com>");
> > +MODULE_DESCRIPTION("APDS-9999 Lux Light Sensor IIO Driver");
> > +MODULE_LICENSE("GPL");
>

--
jose a. p-a

[RFC 3/4] iio: light: add build support for APDS9999

[Jose A. Perez de Azpillaga]

Add Kconfig entry and Makefile rule for the APDS9999 driver.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/iio/light/Kconfig  | 10 ++++++++++
 drivers/iio/light/Makefile |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index eff33e456c70..c7256ffb972d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -119,6 +119,16 @@ config APDS9960
 	  To compile this driver as a module, choose M here: the
 	  module will be called apds9960
 
+config APDS9999
+	tristate "APDS9999 ambient light sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build support for the Broadcom APDS9999
+	  ambient light sensor (ALS/Lux).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called apds9999.
+
 config AS73211
 	tristate "AMS AS73211 XYZ color sensor and AMS AS7331 UV sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0048e0d5ca8..39e62dfc10c7 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_APDS9160)		+= apds9160.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9306)		+= apds9306.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
+obj-$(CONFIG_APDS9999)		+= apds9999.o
 obj-$(CONFIG_AS73211)		+= as73211.o
 obj-$(CONFIG_BH1745)		+= bh1745.o
 obj-$(CONFIG_BH1750)		+= bh1750.o

Re: [RFC 3/4] iio: light: add build support for APDS9999

[Jonathan Cameron]

On Mon, 11 May 2026 12:14:50 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

> Add Kconfig entry and Makefile rule for the APDS9999 driver.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
Squash with previous patch.   There are some corners of the kernel
where they have a pattern of building up particularly complex drivers
in multiple patches that won't compile then adding build support.

We never do that in IIO each patch must build whatever was added
(slightly exception for complex multi module drivers where it can
 be tricky to do).

Anyhow, short story is merge with previous.

> ---
>  drivers/iio/light/Kconfig  | 10 ++++++++++
>  drivers/iio/light/Makefile |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index eff33e456c70..c7256ffb972d 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -119,6 +119,16 @@ config APDS9960
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called apds9960
>  
> +config APDS9999
> +	tristate "APDS9999 ambient light sensor"
> +	depends on I2C
> +	help
> +	  Say Y here if you want to build support for the Broadcom APDS9999
> +	  ambient light sensor (ALS/Lux).
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called apds9999.
> +
>  config AS73211
>  	tristate "AMS AS73211 XYZ color sensor and AMS AS7331 UV sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c0048e0d5ca8..39e62dfc10c7 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_APDS9160)		+= apds9160.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
>  obj-$(CONFIG_APDS9306)		+= apds9306.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
> +obj-$(CONFIG_APDS9999)		+= apds9999.o
>  obj-$(CONFIG_AS73211)		+= as73211.o
>  obj-$(CONFIG_BH1745)		+= bh1745.o
>  obj-$(CONFIG_BH1750)		+= bh1750.o

Re: [RFC 3/4] iio: light: add build support for APDS9999

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 01:28:04PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 12:14:50 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> > Add Kconfig entry and Makefile rule for the APDS9999 driver.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> Squash with previous patch.   There are some corners of the kernel
> where they have a pattern of building up particularly complex drivers
> in multiple patches that won't compile then adding build support.
>
> We never do that in IIO each patch must build whatever was added
> (slightly exception for complex multi module drivers where it can
>  be tricky to do).
>
> Anyhow, short story is merge with previous.

okay, I'll squash Kconfig and Makefile into the driver patch in v2.

--
jose a. p-a

[RFC 4/4] MAINTAINERS: add myself as APDS9999 maintainer

[Jose A. Perez de Azpillaga]

Add maintainer entry for the APDS9999 driver.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1aa9c989973f..cdb703f37f06 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4302,6 +4302,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
 F:	drivers/iio/light/apds9306.c
 
+BROADCOM APDS9999 AMBIENT LIGHT SENSOR DRIVER
+M:	Jose A. Perez de Azpillaga <azpijr@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
+F:	drivers/iio/light/apds9999.c
+
 AVIA HX711 ANALOG DIGITAL CONVERTER IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org

Re: [RFC 4/4] MAINTAINERS: add myself as APDS9999 maintainer

[Jonathan Cameron]

On Mon, 11 May 2026 12:14:52 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

> Add maintainer entry for the APDS9999 driver.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1aa9c989973f..cdb703f37f06 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4302,6 +4302,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
>  F:	drivers/iio/light/apds9306.c
>  
> +BROADCOM APDS9999 AMBIENT LIGHT SENSOR DRIVER
> +M:	Jose A. Perez de Azpillaga <azpijr@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
> +F:	drivers/iio/light/apds9999.c
Build this up as part of the earlier patches.

So add the initial entry in the dt-binding patch (if that's a separate
file) then add th driver in the follow on patch.

There is no advantage in having it separate and it annoys some tooling
that checks for valid coverage for each file added on a patch by
patch basis.

> +
>  AVIA HX711 ANALOG DIGITAL CONVERTER IIO DRIVER
>  M:	Andreas Klinger <ak@it-klinger.de>
>  L:	linux-iio@vger.kernel.org

Re: [RFC 4/4] MAINTAINERS: add myself as APDS9999 maintainer

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 01:29:08PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 12:14:52 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> > Add maintainer entry for the APDS9999 driver.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> > ---
> >  MAINTAINERS | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1aa9c989973f..cdb703f37f06 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4302,6 +4302,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> >  F:	drivers/iio/light/apds9306.c
> >
> > +BROADCOM APDS9999 AMBIENT LIGHT SENSOR DRIVER
> > +M:	Jose A. Perez de Azpillaga <azpijr@gmail.com>
> > +L:	linux-iio@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
> > +F:	drivers/iio/light/apds9999.c
> Build this up as part of the earlier patches.
>
> So add the initial entry in the dt-binding patch (if that's a separate
> file) then add th driver in the follow on patch.
>
> There is no advantage in having it separate and it annoys some tooling
> that checks for valid coverage for each file added on a patch by
> patch basis.

okay, I'm sorry for that. noted for v2.

--
jose a. p-a

Re: [RFC 0/4] iio: light: add driver for Broadcom APDS9999

[Jonathan Cameron]

On Mon, 11 May 2026 12:14:42 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

> This series adds support for the Broadcom APDS9999 ambient light
> sensor. The APDS9999 is a digital proximity and RGB sensor with
> ALS capability, featuring I2C interface, programmable gain and
> integration time.
> 
> This chip is considered by Broadcom to be the 'successor' to the
> APDS-9960 which has technically been discontinued.
> 
> The initial driver implements the ALS/Lux functionality using the
> green channel, which approximates the human eye spectral response.

That's not normally a good approximation unless it is also has
very bad colour curves for green!  Better than any other channel
on it's own but still not good. If we had calibrated sRGB it would be:

 Something like: 0.2126R+0.7152G+0.0722B
for example.

The lux vs white light and lux vs incandecent light are fairly linear
so 'maybe' it's good enough but it's a bit of a stretch to describe it
as an illuminance measure which is what we'd need for it to be measured
in lux.

So I'm on the fence at the moment.  We might be better off teaching userspace
code to do it's own approximation if it has an RGB sensor.

Jonathan

> 
> Sent as RFC to gather feedback on the driver structure
> and DT binding.
> 
> Jose A. Perez de Azpillaga (4):
>   dt-bindings: iio: light: add DT binding for Broadcom APDS9999
>   iio: light: add support for APDS9999 sensor
>   iio: light: add build support for APDS9999
>   MAINTAINERS: add myself as APDS9999 maintainer
> 
>  .../bindings/iio/light/brcm,apds9999.yaml     |  33 ++
>  MAINTAINERS                                   |   7 +
>  drivers/iio/light/Kconfig                     |  10 +
>  drivers/iio/light/Makefile                    |   1 +
>  drivers/iio/light/apds9999.c                  | 325 ++++++++++++++++++
>  5 files changed, 376 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
>  create mode 100644 drivers/iio/light/apds9999.c
> 

Re: [RFC 0/4] iio: light: add driver for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 01:21:12PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 12:14:42 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> > This series adds support for the Broadcom APDS9999 ambient light
> > sensor. The APDS9999 is a digital proximity and RGB sensor with
> > ALS capability, featuring I2C interface, programmable gain and
> > integration time.
> >
> > This chip is considered by Broadcom to be the 'successor' to the
> > APDS-9960 which has technically been discontinued.
> >
> > The initial driver implements the ALS/Lux functionality using the
> > green channel, which approximates the human eye spectral response.
>
> That's not normally a good approximation unless it is also has
> very bad colour curves for green!  Better than any other channel
> on it's own but still not good. If we had calibrated sRGB it would be:
>
>  Something like: 0.2126R+0.7152G+0.0722B
> for example.
>
> The lux vs white light and lux vs incandecent light are fairly linear
> so 'maybe' it's good enough but it's a bit of a stretch to describe it
> as an illuminance measure which is what we'd need for it to be measured
> in lux.

the datasheet states: "Approximates human eye response with green
channel" and "Uses optical coating technology to emulate human eye
spectral response."

that's the reason why I chose to use green channel.

> So I'm on the fence at the moment.  We might be better off teaching userspace
> code to do it's own approximation if it has an RGB sensor.

I'll also add IIO_INTENSITY channels for R, G, B in v2 so userspace can
comapute its own weighted lux if it prefers.

--
jose a. p-a

Re: [RFC 0/4] iio: light: add driver for Broadcom APDS9999

[Jonathan Cameron]

On Mon, 11 May 2026 17:19:54 +0200
"Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:

> On Mon, May 11, 2026 at 01:21:12PM +0100, Jonathan Cameron wrote:
> > On Mon, 11 May 2026 12:14:42 +0200
> > "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
> >  
> > > This series adds support for the Broadcom APDS9999 ambient light
> > > sensor. The APDS9999 is a digital proximity and RGB sensor with
> > > ALS capability, featuring I2C interface, programmable gain and
> > > integration time.
> > >
> > > This chip is considered by Broadcom to be the 'successor' to the
> > > APDS-9960 which has technically been discontinued.
> > >
> > > The initial driver implements the ALS/Lux functionality using the
> > > green channel, which approximates the human eye spectral response.  
> >
> > That's not normally a good approximation unless it is also has
> > very bad colour curves for green!  Better than any other channel
> > on it's own but still not good. If we had calibrated sRGB it would be:
> >
> >  Something like: 0.2126R+0.7152G+0.0722B
> > for example.
> >
> > The lux vs white light and lux vs incandecent light are fairly linear
> > so 'maybe' it's good enough but it's a bit of a stretch to describe it
> > as an illuminance measure which is what we'd need for it to be measured
> > in lux.  
> 
> the datasheet states: "Approximates human eye response with green
> channel" and "Uses optical coating technology to emulate human eye
> spectral response."
Hehe. Marketing. They provide the light curves that show it approximates
very well the green part of human eye spectral response and very badly the
rest of it! :)

> 
> that's the reason why I chose to use green channel.
> 
> > So I'm on the fence at the moment.  We might be better off teaching userspace
> > code to do it's own approximation if it has an RGB sensor.  
> 
> I'll also add IIO_INTENSITY channels for R, G, B in v2 so userspace can
> comapute its own weighted lux if it prefers.
> 
> --
> jose a. p-a
> 

Re: [RFC 0/4] iio: light: add driver for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

On Mon, May 11, 2026 at 04:53:10PM +0100, Jonathan Cameron wrote:
> On Mon, 11 May 2026 17:19:54 +0200
> "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
>
> > On Mon, May 11, 2026 at 01:21:12PM +0100, Jonathan Cameron wrote:
> > > On Mon, 11 May 2026 12:14:42 +0200
> > > "Jose A. Perez de Azpillaga" <azpijr@gmail.com> wrote:
> > >
> > > > This series adds support for the Broadcom APDS9999 ambient light
> > > > sensor. The APDS9999 is a digital proximity and RGB sensor with
> > > > ALS capability, featuring I2C interface, programmable gain and
> > > > integration time.
> > > >
> > > > This chip is considered by Broadcom to be the 'successor' to the
> > > > APDS-9960 which has technically been discontinued.
> > > >
> > > > The initial driver implements the ALS/Lux functionality using the
> > > > green channel, which approximates the human eye spectral response.
> > >
> > > That's not normally a good approximation unless it is also has
> > > very bad colour curves for green!  Better than any other channel
> > > on it's own but still not good. If we had calibrated sRGB it would be:
> > >
> > >  Something like: 0.2126R+0.7152G+0.0722B
> > > for example.
> > >
> > > The lux vs white light and lux vs incandecent light are fairly linear
> > > so 'maybe' it's good enough but it's a bit of a stretch to describe it
> > > as an illuminance measure which is what we'd need for it to be measured
> > > in lux.
> >
> > the datasheet states: "Approximates human eye response with green
> > channel" and "Uses optical coating technology to emulate human eye
> > spectral response."
> Hehe. Marketing. They provide the light curves that show it approximates
> very well the green part of human eye spectral response and very badly the
> rest of it! :)

haha. fair enough.

v2 will keep thr IIO_LIGHT channel and add IIO_INTENSITY for all four
channels so userspace can do its own weighted lux.

--
jose a. p-a