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

[Jonathan Cameron <jic23@kernel.org>]

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");