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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Nov 06, 2026 at 09:56:30PM +0100, Patrick Fischer via B4 Relay wrote:

> 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 (drivers/staging/iio/light/cm3217.c in the 3.4 kernel
> rel-tegranote-r8 and the 3.10 kernel
> android-tegra-flounder-3.10-nougat-mr1.1). One value differs: the vendor
> header defines 4 T as 4 << 2, which is outside the two-bit field. On a
> Tegra Note 7 field value 3 gave four times the 1 T count, so the driver
> uses that. 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.

...

> +/*
> + * 0.01875 lux per count at 4 T and 800 ms, the finest resolution. The
> + * resolution is inversely proportional to IT_T and FD_IT, so keep it as
> + * lux per count times milliseconds: 0.01875 lux * 800 ms = 15 lux * ms.
> + */
> +#define CM3217_NLUX_MS		(15ULL * NANO)

Hmm... Can we use rather formula inside the definition? Also one of the
multipliers from time.h might make sense here as MS are mentioned.

18.75 per 800 µs, or 1875 per 8 µs if I'm not mistaken. Hence

	(1875 * 8 * NSEC_PER_MSEC)

?

...

> +/* Scale in lux per count at FD_IT = ms milliseconds and IT_T = 1/2 T << it_t */
> +#define CM3217_SCALE(ms, it_t) {				\
> +	(CM3217_NLUX_MS << (3 - (it_t))) / (ms) / NANO,		\
> +	(CM3217_NLUX_MS << (3 - (it_t))) / (ms) % NANO,		\

These NANO again are misleading.

> +}

...

> +static_assert(ARRAY_SIZE(cm3217_scale_avail) ==
> +	      ARRAY_SIZE(cm3217_int_time_avail));

Having this on a single line is fine.

...

> +struct cm3217_data {
> +	struct i2c_client *client;
> +	struct i2c_client *client_lsb;
> +	/* protects the command bytes and ready */
> +	struct mutex lock;
> +	u8 cmd1;
> +	u8 cmd2;
> +	ktime_t ready;

Have you checked with `pahole`?

> +};

> +	/* the first count is ready after one conversion */
> +	conv_ms = cm3217_conv_ms(data);

You can add 10% margin right here with a comment.

	/* Add 10% margin along with additional ... for ... */
	conv_ms += conv_ms / 10 + CM3217_MARGIN_MS;

> +	data->ready = ktime_add_ms(ktime_get(),
> +				   conv_ms + conv_ms / 10 + CM3217_MARGIN_MS);

	data->ready = ktime_add_ms(ktime_get(), conv_ms);

> +	return 0;
> +}

...

> +static int cm3217_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct cm3217_data *data = iio_priv(indio_dev);
> +	unsigned int fd_it;
> +	u8 cmd1, cmd2;
> +	int i;
> +
> +	guard(mutex)(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		fd_it = FIELD_GET(CM3217_CMD2_FD_IT, data->cmd2);
> +		i = cm3217_find_avail(cm3217_scale_avail[fd_it],
> +				      ARRAY_SIZE(cm3217_scale_avail[0]),
> +				      val, val2);
> +		if (i < 0)
> +			return i;

> +		cmd1 = data->cmd1 & ~CM3217_CMD1_IT_T;
> +		cmd1 |= FIELD_PREP(CM3217_CMD1_IT_T, i);

FIELD_MODIFY()

> +		return cm3217_write_cmd(data, cmd1, data->cmd2);
> +	case IIO_CHAN_INFO_INT_TIME:
> +		i = cm3217_find_avail(cm3217_int_time_avail,
> +				      ARRAY_SIZE(cm3217_int_time_avail),
> +				      val, val2);
> +		if (i < 0)
> +			return i;
> +
> +		cmd2 = FIELD_PREP(CM3217_CMD2_FD_IT, i);
> +
> +		return cm3217_write_cmd(data, data->cmd1, cmd2);
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +	scoped_guard(mutex, &data->lock)
> +		ret = cm3217_write_cmd(data, cmd1, cmd2);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to power on\n");

It's better to have it inside scoped_guard().

-- 
With Best Regards,
Andy Shevchenko