[PATCH v4 0/2] iio: light: add support for Broadcom APDS9999

[PATCH v4 0/2] iio: light: add support for Broadcom APDS9999

[Jose A. Perez de Azpillaga]

Add IIO driver and DT binding for the Broadcom APDS9999 ambient light
and proximity sensor. The green channel uses optical coating technology
to approximate the human eye spectral response for ALS/lux measurements.
IIO_INTENSITY channels are also provided for red, green, blue, and IR
so userspace can compute its own weighted lux.

v4:
  - Renamed vled-supply to vcsel-supply (matches VVCSEL pin name)
  - Expanded Kconfig description (ALS, RGB, proximity)
  - Clarified mutex lock comment
  - Replaced magic number in scale formula with
    54ULL * NSEC_PER_SEC * USEC_PER_MSEC
  - Updated TODO to just proximity (color channels now exposed)

v3:
  - Dropped RFC
  - Fixed MAINTAINERS entry alphabetical order (B section)
  - Changed IIO_MOD_LIGHT_CLEAR to IIO_MOD_LIGHT_IR
  - Unified RAW read path via chan->address for all channels
  - Switched enum gains/rates to #define (hardware field values)
  - Used USEC_PER_MSEC for integration time table
  - Removed redundant comments from resolution enum
  - Added default rationale comment in _init()
  - Added ALS rationale comment on IIO_LIGHT channel
  - s/reg/regval/, fixed indent in poll loop, C99 .name for id table
  - Moved iio_info right after read_raw function
  - DT binding: expanded VCSEL acronym, explained separate binding
  - Commit messages wrapped at 75 chars, clarified RGB vs raw status

v2:
  - Added IIO_INTENSITY channels for R, G, B, clear
  - Switched to guard(mutex)(), devm_mutex_init(), dev_err_probe()
  - Replaced remove() with devm_add_action_or_reset()
  - Don't fail probe on PART_ID mismatch (fallback compatibles)
  - Replaced manual shifts with FIELD_PREP() / GENMASK
  - Used get_unaligned_le24(), sizeof(buf), NSEC_PER_SEC, fsleep()
  - Named gain register values, explicit resolution enum values
  - Removed section comments, grouped bit defines under registers
  - Squashed Kconfig/Makefile into driver patch
  - Split MAINTAINERS entry across patches
  - DT binding: added vled-supply, interrupts, required vdd-supply
  - DT binding: fixed title, added blank lines, full description
  - Dropped "DT binding" duplication from binding commit subject

Link to v3: https://lore.kernel.org/linux-iio/cover.1779181370.git.azpijr@gmail.com/
Link to v2: https://lore.kernel.org/linux-iio/cover.1778659152.git.azpijr@gmail.com/
Link to v1: https://lore.kernel.org/linux-iio/cover.1778491503.git.azpijr@gmail.com/

Jose A. Perez de Azpillaga (2):
  dt-bindings: iio: light: add Broadcom APDS9999
  iio: light: add support for APDS9999 sensor

 .../bindings/iio/light/brcm,apds9999.yaml     |  54 +++
 MAINTAINERS                                   |   7 +
 drivers/iio/light/Kconfig                     |  14 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/apds9999.c                  | 337 ++++++++++++++++++
 5 files changed, 413 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
 create mode 100644 drivers/iio/light/apds9999.c

[PATCH v4 1/2] dt-bindings: iio: light: add Broadcom APDS9999

[Jose A. Perez de Azpillaga]

Add Device Tree binding for the Broadcom APDS9999 ambient light
and proximity sensor. A separate binding file is used rather
than merging with avago,apds9300.yaml because the APDS9999
has an additional vcsel-supply for the VCSEL.

The APDS9999 features individual R, G, B, and IR channels with
a green channel that uses optical coating to approximate the
human eye spectral response for ALS/lux measurements. Calibrated
RGB color sensing is not yet implemented in the driver.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 .../bindings/iio/light/brcm,apds9999.yaml     | 54 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 60 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..9f5b3b294c2c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9999.yaml
@@ -0,0 +1,54 @@
+# 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 Digital Proximity and RGB Sensor
+
+maintainers:
+  - Jose A. Perez de Azpillaga <azpijr@gmail.com>
+
+description: |
+  Broadcom APDS-9999 is a digital proximity and RGB sensor with
+  ambient light sensing (ALS) capability. The device uses individual
+  R, G, B, and IR channels plus a Vertical Cavity Surface Emitting
+  Laser (VCSEL) for proximity detection.
+
+  Datasheet: https://docs.broadcom.com/docs/APDS-9999-DS
+
+properties:
+  compatible:
+    enum:
+      - brcm,apds9999
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  vcsel-supply:
+    description: VCSEL power supply (VVCSEL pin)
+
+  interrupts:
+    maxItems: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@52 {
+            compatible = "brcm,apds9999";
+            reg = <0x52>;
+            vdd-supply = <&vdd_reg>;
+            vcsel-supply = <&vcsel_reg>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1aa9c989973f..2d8d4e2eab6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5000,6 +5000,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
 F:	drivers/iio/light/apds9160.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
+
 BROADCOM ASP 2.0 ETHERNET DRIVER
 M:	Justin Chen <justin.chen@broadcom.com>
 M:	Florian Fainelli <florian.fainelli@broadcom.com>

[PATCH v4 2/2] 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. The driver implements the ALS/Lux functionality
using the green channel, which uses optical coating technology
to approximate the human eye spectral response.

Raw IIO_INTENSITY channels are exposed for red, green, blue,
and IR so userspace can compute its own weighted lux.
Proximity (PS) support is not yet implemented.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 MAINTAINERS                  |   1 +
 drivers/iio/light/Kconfig    |  14 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/apds9999.c | 337 +++++++++++++++++++++++++++++++++++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/iio/light/apds9999.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d8d4e2eab6e..d36ab614aa53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5005,6 +5005,7 @@ 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

 BROADCOM ASP 2.0 ETHERNET DRIVER
 M:	Justin Chen <justin.chen@broadcom.com>
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index eff33e456c70..da4807a3fd3d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -119,6 +119,20 @@ config APDS9960
 	  To compile this driver as a module, choose M here: the
 	  module will be called apds9960

+config APDS9999
+	tristate "Broadcom APDS9999 ALS, RGB and proximity sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build support for the Broadcom APDS9999
+	  ALS, RGB and proximity sensor with I2C interface.
+
+	  This driver provides ambient light sensing (ALS/Lux), raw
+	  intensity data for red, green, blue and IR channels, plus
+	  proximity detection support.
+
+	  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
diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
new file mode 100644
index 000000000000..f2fbd743abe7
--- /dev/null
+++ b/drivers/iio/light/apds9999.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for Broadcom APDS9999 Lux Light Sensor
+ *
+ * Copyright (C) 2026
+ * Author: Jose A. Perez de Azpillaga <azpijr@gmail.com>
+ *
+ * TODO: proximity sensor
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.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>
+#include <linux/unaligned.h>
+#include <linux/units.h>
+
+#define APDS9999_REG_MAIN_CTRL		0x00
+#define   APDS9999_MAIN_CTRL_LS_EN	BIT(1)
+#define APDS9999_REG_LS_MEAS_RATE	0x04
+#define   APDS9999_LS_RES_MASK		GENMASK(6, 4)
+#define   APDS9999_LS_RATE_MASK	GENMASK(2, 0)
+#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_IR_0	0x0A
+#define APDS9999_REG_LS_DATA_GREEN_0	0x0D
+#define APDS9999_REG_LS_DATA_BLUE_0	0x10
+#define APDS9999_REG_LS_DATA_RED_0	0x13
+
+#define APDS9999_PART_ID		0xC2
+
+#define APDS9999_GAIN_1X		0
+#define APDS9999_GAIN_3X		1
+#define APDS9999_GAIN_6X		2
+#define APDS9999_GAIN_9X		3
+#define APDS9999_GAIN_18X		4
+
+static const int apds9999_gains[] = {
+	[APDS9999_GAIN_1X]  = 1,
+	[APDS9999_GAIN_3X]  = 3,
+	[APDS9999_GAIN_6X]  = 6,
+	[APDS9999_GAIN_9X]  = 9,
+	[APDS9999_GAIN_18X] = 18,
+};
+
+enum apds9999_resolution {
+	APDS9999_RES_20BIT = 0,
+	APDS9999_RES_19BIT = 1,
+	APDS9999_RES_18BIT = 2,
+	APDS9999_RES_17BIT = 3,
+	APDS9999_RES_16BIT = 4,
+	APDS9999_RES_13BIT = 5,
+	APDS9999_RES_NUM
+};
+
+static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
+	[APDS9999_RES_20BIT] = 400 * USEC_PER_MSEC,
+	[APDS9999_RES_19BIT] = 200 * USEC_PER_MSEC,
+	[APDS9999_RES_18BIT] = 100 * USEC_PER_MSEC,
+	[APDS9999_RES_17BIT] =  50 * USEC_PER_MSEC,
+	[APDS9999_RES_16BIT] =  25 * USEC_PER_MSEC,
+	[APDS9999_RES_13BIT] =   3125,
+};
+
+#define APDS9999_RATE_25_MS		0
+#define APDS9999_RATE_50_MS		1
+#define APDS9999_RATE_100_MS		2
+#define APDS9999_RATE_200_MS		3
+#define APDS9999_RATE_500_MS		4
+#define APDS9999_RATE_1000_MS		5
+#define APDS9999_RATE_2000_MS		6
+
+struct apds9999_data {
+	struct i2c_client *client;
+	/* lock: serializes access to device registers and cached values */
+	struct mutex lock;
+	int als_gain_idx;
+	int als_res;
+	int als_rate;
+};
+
+static void apds9999_standby(void *client)
+{
+	i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL, 0);
+}
+
+/*
+ * Apply power-on defaults: 18-bit / 100 ms resolution and rate,
+ * 3x gain. These match the datasheet reset values.
+ */
+static int apds9999_init(struct apds9999_data *data)
+{
+	struct device *dev = &data->client->dev;
+	struct i2c_client *client = data->client;
+	u8 regval;
+	int ret;
+
+	ret = devm_add_action_or_reset(dev, apds9999_standby, client);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&data->lock);
+
+	regval = FIELD_PREP(APDS9999_LS_RES_MASK, APDS9999_RES_18BIT) |
+		 FIELD_PREP(APDS9999_LS_RATE_MASK, APDS9999_RATE_100_MS);
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_MEAS_RATE,
+					regval);
+	if (ret)
+		return ret;
+	data->als_res = APDS9999_RES_18BIT;
+	data->als_rate = APDS9999_RATE_100_MS;
+
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_LS_GAIN,
+					APDS9999_GAIN_3X);
+	if (ret)
+		return ret;
+	data->als_gain_idx = APDS9999_GAIN_3X;
+
+	return i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
+					 APDS9999_MAIN_CTRL_LS_EN);
+}
+
+static int apds9999_read_channel(struct apds9999_data *data, u8 reg,
+				 u32 *counts)
+{
+	struct i2c_client *client = data->client;
+	u8 buf[3];
+	int ret, tries;
+
+	guard(mutex)(&data->lock);
+
+	/*
+	 * Poll MAIN_STATUS for new data.  Timeout: ~2 integration periods
+	 * plus margin.  Each try sleeps 20 ms.
+	 */
+	tries = max(2, (apds9999_itimes_us[data->als_res] * 2) / 20000);
+
+	while (tries--) {
+		ret = i2c_smbus_read_byte_data(client,
+					       APDS9999_REG_MAIN_STATUS);
+		if (ret < 0)
+			return ret;
+		if (ret & APDS9999_MAIN_STATUS_LS_DATA)
+			break;
+		fsleep(20000);
+	}
+
+	if (tries < 0)
+		return -ETIMEDOUT;
+
+	ret = i2c_smbus_read_i2c_block_data(client, reg, sizeof(buf), buf);
+	if (ret < 0)
+		return ret;
+	if (ret != sizeof(buf))
+		return -EIO;
+
+	*counts = get_unaligned_le24(buf) & GENMASK(19, 0);
+	return 0;
+}
+
+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_read_channel(data, chan->address, &counts);
+		if (ret)
+			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.
+		 */
+		gain = apds9999_gains[data->als_gain_idx];
+		itime_us = apds9999_itimes_us[data->als_res];
+
+		/* scale_nano = 54 * 1e12 / (gain * itime_us) nano-lux/count */
+		scale_nano = div_u64(54ULL * NSEC_PER_SEC * USEC_PER_MSEC, (u32)(gain * itime_us));
+		*val = (int)(scale_nano / NSEC_PER_SEC);
+		*val2 = (int)(scale_nano % NSEC_PER_SEC);
+		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_info apds9999_info = {
+	.read_raw = apds9999_read_raw,
+};
+
+/*
+ * The green channel uses optical coating to approximate the human eye
+ * spectral response.  IIO_INTENSITY channels provide raw ADC data for
+ * red, green, blue, and IR so userspace can compute weighted lux.
+ */
+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),
+		.address = APDS9999_REG_LS_DATA_GREEN_0,
+	},
+	{
+		.type = IIO_INTENSITY,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_RED,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = APDS9999_REG_LS_DATA_RED_0,
+	},
+	{
+		.type = IIO_INTENSITY,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_GREEN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = APDS9999_REG_LS_DATA_GREEN_0,
+	},
+	{
+		.type = IIO_INTENSITY,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_BLUE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = APDS9999_REG_LS_DATA_BLUE_0,
+	},
+	{
+		.type = IIO_INTENSITY,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_IR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.address = APDS9999_REG_LS_DATA_IR_0,
+	},
+};
+
+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;
+
+	ret = devm_mutex_init(dev, &data->lock);
+	if (ret)
+		return ret;
+
+	part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
+	if (part_id < 0)
+		return dev_err_probe(dev, part_id,
+				     "failed to read PART_ID\n");
+	if (part_id != APDS9999_PART_ID)
+		dev_info(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
+			 part_id, APDS9999_PART_ID);
+
+	ret = apds9999_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to initialize device\n");
+
+	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)
+		return dev_err_probe(dev, ret,
+				     "failed to register IIO device\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id apds9999_id[] = {
+	{ .name = "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,
+	.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: [PATCH v4 1/2] dt-bindings: iio: light: add Broadcom APDS9999

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On Tue, May 26, 2026 at 09:55:15AM +0200, Jose A. Perez de Azpillaga wrote:
> Add Device Tree binding for the Broadcom APDS9999 ambient light
> and proximity sensor. A separate binding file is used rather
> than merging with avago,apds9300.yaml because the APDS9999
> has an additional vcsel-supply for the VCSEL.
> 
> The APDS9999 features individual R, G, B, and IR channels with
> a green channel that uses optical coating to approximate the
> human eye spectral response for ALS/lux measurements. Calibrated
> RGB color sensing is not yet implemented in the driver.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/2] iio: light: add support for APDS9999 sensor

[Jonathan Cameron]

On Tue, 26 May 2026 09:55:46 +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. The driver implements the ALS/Lux functionality
> using the green channel, which uses optical coating technology
> to approximate the human eye spectral response.
> 
> Raw IIO_INTENSITY channels are exposed for red, green, blue,
> and IR so userspace can compute its own weighted lux.
> Proximity (PS) support is not yet implemented.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
Hi Jose,

A few really minor things inline.  Given how minor they are I've applied
this series to the testing branch of iio.git with the following tweaks:
(shout if you disagree with any of them!)

diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
index f2fbd743abe7..215f3e8087c9 100644
--- a/drivers/iio/light/apds9999.c
+++ b/drivers/iio/light/apds9999.c
@@ -52,17 +52,14 @@ static const int apds9999_gains[] = {
        [APDS9999_GAIN_18X] = 18,
 };
 
-enum apds9999_resolution {
-       APDS9999_RES_20BIT = 0,
-       APDS9999_RES_19BIT = 1,
-       APDS9999_RES_18BIT = 2,
-       APDS9999_RES_17BIT = 3,
-       APDS9999_RES_16BIT = 4,
-       APDS9999_RES_13BIT = 5,
-       APDS9999_RES_NUM
-};
-
-static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
+#define APDS9999_RES_20BIT             0
+#define APDS9999_RES_19BIT             1
+#define APDS9999_RES_18BIT             2
+#define APDS9999_RES_17BIT             3
+#define APDS9999_RES_16BIT             4
+#define APDS9999_RES_13BIT             5
+
+static const int apds9999_itimes_us[] = {
        [APDS9999_RES_20BIT] = 400 * USEC_PER_MSEC,
        [APDS9999_RES_19BIT] = 200 * USEC_PER_MSEC,
        [APDS9999_RES_18BIT] = 100 * USEC_PER_MSEC,
@@ -285,16 +282,14 @@ static int apds9999_probe(struct i2c_client *client)
 
        part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
        if (part_id < 0)
-               return dev_err_probe(dev, part_id,
-                                    "failed to read PART_ID\n");
+               return dev_err_probe(dev, part_id, "failed to read PART_ID\n");
        if (part_id != APDS9999_PART_ID)
                dev_info(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
                         part_id, APDS9999_PART_ID);
 
        ret = apds9999_init(data);
        if (ret)
-               return dev_err_probe(dev, ret,
-                                    "failed to initialize device\n");
+               return dev_err_probe(dev, ret, "failed to initialize device\n");
 
        indio_dev->name = "apds9999";
        indio_dev->info = &apds9999_info;
@@ -304,8 +299,7 @@ static int apds9999_probe(struct i2c_client *client)
 
        ret = devm_iio_device_register(dev, indio_dev);
        if (ret)
-               return dev_err_probe(dev, ret,
-                                    "failed to register IIO device\n");
+               return dev_err_probe(dev, ret, "failed to register IIO device\n");
 
        return 0;
 }

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

> +
> +static const int apds9999_gains[] = {
> +	[APDS9999_GAIN_1X]  = 1,
> +	[APDS9999_GAIN_3X]  = 3,
> +	[APDS9999_GAIN_6X]  = 6,
> +	[APDS9999_GAIN_9X]  = 9,
> +	[APDS9999_GAIN_18X] = 18,
> +};
> +
> +enum apds9999_resolution {
> +	APDS9999_RES_20BIT = 0,
> +	APDS9999_RES_19BIT = 1,
> +	APDS9999_RES_18BIT = 2,
> +	APDS9999_RES_17BIT = 3,
> +	APDS9999_RES_16BIT = 4,
> +	APDS9999_RES_13BIT = 5,
> +	APDS9999_RES_NUM
> +};

Why is set of field values an enum whereas all the others
are defines?  I prefer defines if the type isn't used and they
reflect hardware filed values.

> +
> +static const int apds9999_itimes_us[APDS9999_RES_NUM] = {
> +	[APDS9999_RES_20BIT] = 400 * USEC_PER_MSEC,
> +	[APDS9999_RES_19BIT] = 200 * USEC_PER_MSEC,
> +	[APDS9999_RES_18BIT] = 100 * USEC_PER_MSEC,
> +	[APDS9999_RES_17BIT] =  50 * USEC_PER_MSEC,
> +	[APDS9999_RES_16BIT] =  25 * USEC_PER_MSEC,
> +	[APDS9999_RES_13BIT] =   3125,
> +};
> +
> +#define APDS9999_RATE_25_MS		0
> +#define APDS9999_RATE_50_MS		1
> +#define APDS9999_RATE_100_MS		2
> +#define APDS9999_RATE_200_MS		3
> +#define APDS9999_RATE_500_MS		4
> +#define APDS9999_RATE_1000_MS		5
> +#define APDS9999_RATE_2000_MS		6

>
> +static int apds9999_read_channel(struct apds9999_data *data, u8 reg,
> +				 u32 *counts)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 buf[3];
> +	int ret, tries;
> +
> +	guard(mutex)(&data->lock);
> +
> +	/*
> +	 * Poll MAIN_STATUS for new data.  Timeout: ~2 integration periods
> +	 * plus margin.  Each try sleeps 20 ms.
> +	 */
> +	tries = max(2, (apds9999_itimes_us[data->als_res] * 2) / 20000);
> +
> +	while (tries--) {
> +		ret = i2c_smbus_read_byte_data(client,
> +					       APDS9999_REG_MAIN_STATUS);
> +		if (ret < 0)
> +			return ret;
> +		if (ret & APDS9999_MAIN_STATUS_LS_DATA)
> +			break;
> +		fsleep(20000);
> +	}
> +
> +	if (tries < 0)
> +		return -ETIMEDOUT;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, reg, sizeof(buf), buf);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(buf))
> +		return -EIO;
> +
> +	*counts = get_unaligned_le24(buf) & GENMASK(19, 0);
> +	return 0;
> +}
> +
> +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_read_channel(data, chan->address, &counts);

Not necessarily worth changing but I'll note you could have returned
the value if not an error
		ret = apds9999_read_channel(data, chan->address);
		if (ret < 0)
			return ret;

		*val = ret;
		return IIO_VAL_INT;

This is one of those design decisions where there isn't a right answer.

> +		if (ret)
> +			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.
> +		 */
> +		gain = apds9999_gains[data->als_gain_idx];
> +		itime_us = apds9999_itimes_us[data->als_res];
> +
> +		/* scale_nano = 54 * 1e12 / (gain * itime_us) nano-lux/count */
> +		scale_nano = div_u64(54ULL * NSEC_PER_SEC * USEC_PER_MSEC, (u32)(gain * itime_us));
> +		*val = (int)(scale_nano / NSEC_PER_SEC);
> +		*val2 = (int)(scale_nano % NSEC_PER_SEC);
> +		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_info apds9999_info = {
> +	.read_raw = apds9999_read_raw,
> +};
> +
> +/*
> + * The green channel uses optical coating to approximate the human eye
> + * spectral response.  IIO_INTENSITY channels provide raw ADC data for
> + * red, green, blue, and IR so userspace can compute weighted lux.
> + */
> +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),
> +		.address = APDS9999_REG_LS_DATA_GREEN_0,
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_RED,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = APDS9999_REG_LS_DATA_RED_0,

More a note for the future than a 'now' thing. If you make scale writeable
then it will (I assume) affect these channels a well as the light. This is
based on assumption that write will be the control for the gain (with the
current integration time etc taken into account).  You might present
it for the non light channels as 1/(int_time * gain) as the the end result
of _raw * _scale for an IIO_INTENSITY value is not in any particular
scale - we just what the result to remain stable if the _scale is changed
and the actual incident light is the same - i.e. double _scale and _raw should
halve.

Anyhow that would be a new feature so no need to do it now.

 
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_GREEN,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = APDS9999_REG_LS_DATA_GREEN_0,
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_BLUE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = APDS9999_REG_LS_DATA_BLUE_0,
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = APDS9999_REG_LS_DATA_IR_0,
> +	},
> +};
> +
> +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;
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	part_id = i2c_smbus_read_byte_data(client, APDS9999_REG_PART_ID);
> +	if (part_id < 0)
> +		return dev_err_probe(dev, part_id,
> +				     "failed to read PART_ID\n");

Under 80 chars when on one line.

> +	if (part_id != APDS9999_PART_ID)
> +		dev_info(dev, "unexpected PART_ID 0x%02x (expected 0x%02x)\n",
> +			 part_id, APDS9999_PART_ID);
> +
> +	ret = apds9999_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to initialize device\n");
I make this under 80 chars on one line.

> +
> +	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)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register IIO device\n");

This one is 81 ish so I'll probably just make that a single line as well.

Re: [PATCH v4 2/2] iio: light: add support for APDS9999 sensor

[Jose A. Perez de Azpillaga]

On Wed, May 27, 2026 at 05:06:56PM +0100, Jonathan Cameron wrote:
> On Tue, 26 May 2026 09:55:46 +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. The driver implements the ALS/Lux functionality
> > using the green channel, which uses optical coating technology
> > to approximate the human eye spectral response.
> >
> > Raw IIO_INTENSITY channels are exposed for red, green, blue,
> > and IR so userspace can compute its own weighted lux.
> > Proximity (PS) support is not yet implemented.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> Hi Jose,
>
> A few really minor things inline.  Given how minor they are I've applied
> this series to the testing branch of iio.git with the following tweaks:
> (shout if you disagree with any of them!)
>

all look good to me, thanks!

--
jose a. p-a