Re: [PATCH 2/2] iio: light: Add Capella CM3217 ambient light sensor driver

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 01 Nov 2026 15:46:07 +0100
Patrick Fischer via B4 Relay <devnull+admin.tsv-stahl-riesa.de@kernel.org> wrote:

> From: Patrick Fischer <admin@tsv-stahl-riesa.de>
> 
> The CM3217 has no registers and no ID. Writing a byte to address 0x10
> or 0x11 sets command byte 1 or 2, reading a byte from 0x10 or 0x11
> returns the high or low byte of the 16-bit light count.
> 
> Command byte 1 holds the shutdown bit and the IT_T field (1/2 T, 1 T,
> 2 T or 4 T), command byte 2 the FD_IT field (800 ms down to 66 ms). The
> count scales linearly with both. The driver exposes FD_IT as the
> integration time and IT_T through the scale, so the scale list offers
> four ranges for each integration time.
> 
> The datasheet is not public. The bit fields, the reserved bits that are
> written as 1 and the startup setting of 2 T and 100 ms come from NVIDIA's
> downstream drivers for the Tegra Note 7 (drivers/staging/iio/light/
> cm3217.c in the 3.4 and 3.10 kernels). For the resolution, Capella
> gives a range of 0.01875 to 119156 lux. The driver takes 0.01875 lux
> per count at 4 T and 800 ms. This matches both the upper end (65535
> counts at 1/2 T and 66 ms) and the 0.3 lux per count that the NVIDIA
> 3.4 driver uses at 2 T and 100 ms.
> 
> The sensor has no data-ready flag, and a new setting only takes effect
> when the conversion in progress ends. A conversion takes IT_T times
> FD_IT, up to 3.2 s. Leaving shutdown starts a new one at once, so the
> driver passes through shutdown on every change and waits one conversion
> period plus a margin before it reads the count. The timing was measured
> on a Tegra Note 7.

I don't think we have a current active Capella Micro developer on the
list as it's been a while since I heard from Kevin Tsai who contributed
drivers for them, so your approach of using the vendor driver as a
guide is probably the best we can do.

> 
> Signed-off-by: Patrick Fischer <admin@tsv-stahl-riesa.de>
A few comments inline.

If it is really hard to precompute all the available values, then
we can probably get away with not doing so.  I don't think it is
though and picking between precomputed sets is both cleaner and avoids
a race if we ever get light driver inkernel consumers.

Thanks

Jonathan

> diff --git a/drivers/iio/light/cm3217.c b/drivers/iio/light/cm3217.c
> new file mode 100644
> index 000000000000..9e0e9afee092
> --- /dev/null
> +++ b/drivers/iio/light/cm3217.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Capella CM3217 ambient light sensor driver
> + *
> + * The CM3217 has no registers. It answers on two fixed I2C addresses:
> + * writing a byte to 0x10 or 0x11 sets command byte 1 or 2, reading a byte
> + * from 0x10 or 0x11 returns the MSB or LSB of the 16-bit light count.
> + *
> + * Copyright (C) 2026 Patrick Fischer <admin@tsv-stahl-riesa.de>
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/ktime.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define CM3217_ADDR_CMD2_LSB	0x11
> +
> +#define CM3217_CMD1_RSVD	BIT(5)		/* reserved, set to 1 */
> +#define CM3217_CMD1_IT_T	GENMASK(3, 2)	/* 1/2 T, 1 T, 2 T, 4 T */
> +#define CM3217_CMD1_WDM		BIT(1)		/* set to 1 like the vendor driver */
> +#define CM3217_CMD1_SD		BIT(0)		/* shutdown when set */
> +
> +#define CM3217_CMD2_FD_IT	GENMASK(7, 5)
> +
> +#define CM3217_IT_T_2T		2
> +#define CM3217_FD_IT_100MS	5
> +
> +/* 0.01875 lux per count at 4 T and 800 ms, the finest resolution */
> +#define CM3217_NLUX_MS		15000000000ULL

Use a muiltiplier for that definition. Various ones in units.h.
I haven't figured out what makes sense, but that is too many 
zeros to eyeball!

> +
> +#define CM3217_MARGIN_MS	10


> +
> +static unsigned int cm3217_conv_ms(struct cm3217_data *data)
> +	__must_hold(&data->lock)
> +{
> +	unsigned int fd_it = FIELD_GET(CM3217_CMD2_FD_IT, data->cmd2);
> +	unsigned int it_t = FIELD_GET(CM3217_CMD1_IT_T, data->cmd1);
> +
> +	return (cm3217_fd_it_ms[fd_it] << it_t) / 2;
> +}
> +
> +static void cm3217_update_scale_avail(struct cm3217_data *data)
There are some unfortunately complex race conditions around
read_avail where a consumer driver (iio/inkern.c) can hold a pointer to the
array and there is no visibility in the consumer of the fact the
data might be changing under it. The concern isn't so much getting
wrong values as getting torn ones as the writes are occuring. 
There have been a few goes at solving the race generically but none
have landed yet.  Generally avoiding that sort of handoff of
dynamic data is a nice to have and often easy enough to do.

I'm not that concerned for a light sensor as we have no consumers
in upstream anyway, but how bad would it be to precompute all possible
tables at probe time or even at compile time, and then just pick between them?

> +	__must_hold(&data->lock)
> +{
> +	unsigned int fd_it = FIELD_GET(CM3217_CMD2_FD_IT, data->cmd2);
> +	unsigned int i;
> +	u32 rem;
> +	u64 nlux;
> +
> +	for (i = 0; i < ARRAY_SIZE(data->scale_avail); i++) {
Might as well do

	for (unsigned int i = 0; .. {
	

> +		nlux = div_u64(CM3217_NLUX_MS << (3 - i), cm3217_fd_it_ms[fd_it]);
> +		data->scale_avail[i][0] = div_u64_rem(nlux, NANO, &rem);
> +		data->scale_avail[i][1] = rem;
> +	}
> +}



> +
> +static int cm3217_suspend(struct device *dev)
> +{
> +	struct cm3217_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	guard(mutex)(&data->lock);
> +
> +	return i2c_smbus_write_byte(data->client, data->cmd1 | CM3217_CMD1_SD);

Given this lines up with the power off callback above and the start of
the write_cmd() sequence, maybe have a little helper to put this write
command in just one place?

> +}
> +
> +static int cm3217_resume(struct device *dev)
> +{
> +	struct cm3217_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	guard(mutex)(&data->lock);
> +
> +	return cm3217_write_cmd(data, data->cmd1, data->cmd2);
> +}

>