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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 13 May 2026 10:10:35 +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, which uses optical coating technology to
> approximate the human eye spectral response.

Can wrap commit messages to 75 chars.

> 
> Proximity (PS) and RGB color features are not yet implemented.
Needs an update (sashiko pointed this out)
https://sashiko.dev/#/patchset/cover.1778659152.git.azpijr%40gmail.com

> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>

A few minor things inline. 

Thanks,

Jonathan

> diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
> new file mode 100644
> index 000000000000..577b93f61d31
> --- /dev/null
> +++ b/drivers/iio/light/apds9999.c
> @@ -0,0 +1,344 @@

> +static int apds9999_init(struct apds9999_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct i2c_client *client = data->client;
> +	u8 reg;

I think regval makes it more obvious this isn't a register address.

> +	int ret;
> +
> +	ret = devm_add_action_or_reset(dev, apds9999_standby, client);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	reg = 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, reg);
> +	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;
> +
> +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> +					APDS9999_MAIN_CTRL_LS_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return i2c_smbus_write_byte_data()

> +}
> +
> +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);
Off by one space and it's under 80 chars anyway (would have been fine if a little
over a well).

		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:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = apds9999_read_channel(data,
> +						    APDS9999_REG_LS_DATA_GREEN_0,
> +						    &counts);
			if (ret)
				return ret;

			*val = (int)counts;
	
			return IIO_VAL_COUNT;
(See below)

> +			break;
> +		case IIO_INTENSITY:
> +			ret = apds9999_read_channel(data, chan->address,
> +						    &counts);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		if (ret)
> +			return ret;
I'd shift this up so all errors are dealt with at source.  Means we don't need
to read on to see where they are going... 
> +		*val = (int)counts;
> +		return IIO_VAL_INT;
Not worth sharing this either given only save a tiny bit.

> +
> +	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 = 54e12 / (gain * itime_us) nano-lux/count */
> +		scale_nano = div_u64(54000000000000ULL, (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_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),
> +	},
> +	{
> +		.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_CLEAR,

Why clear rather than IR? Sashiko even noticed this. 

> +		.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 const struct i2c_device_id apds9999_id[] = {
> +	{ "apds9999" },

Whilst not traditionally done, we are moving towards using c99 for this.
	{ .name = "apds9999" }
preferred.  Doesn't actually matter if only a name, but later fields
are likely to soon become a union.  For consistency and ease of explanation
I think always using named structure fields is a good direction to move in.

> +	{ }
> +};
> +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");
>