Re: [PATCH v4 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 May 2025 08:08:03 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
> 
> This sensor provides three colour (red, green and blue) as well as one
> infrared (IR) channel through I2C.
> 
> Support direct and buffered mode.
> 
> An optional interrupt for signaling green colour threshold underflow or
> overflow is not supported so far.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
Hi Andreas

A few little additions from me.

The one about iio_push_to_buffers_with_ts() is entirely up to you.
When I introduce new features like that I tend to give a window before insisting
on them being done by driver authors. It's unfair to insist when they crossed
with your code and the extra cost of one more driver on a big conversion is
trivial.

Jonathan


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

> +struct veml6046x00_scan_buf {
> +	__le16 chans[4];
> +	aligned_s64 timestamp;
> +};

You moved the definition inline but forgot to delete this one I think.
So drop it.


> +/*
Might as well be kernel-doc

/**

> + * veml6046x00_gain_pd - translation from gain index (used in the driver) to
> + * gain (sensor) and PD
> + * @gain_sen:	Gain used in the sensor as described in the datasheet of the
> + *		sensor
> + * @pd:		Photodiode size in the sensor
> + */
> +struct veml6046x00_gain_pd {
> +	int gain_sen;
> +	int pd;
> +};
> +
> +#define VEML6046X00_GAIN_PD(_gain_sen, _pd)				\
> +{									\
> +	.gain_sen = (_gain_sen),					\
> +	.pd = (_pd),							\
> +}

Macro not used any more so drop it.

> +
> +static const struct veml6046x00_gain_pd veml6046x00_gain_pd[] = {
> +	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_1_2},
{ .gain 

So add spaces after { and before }
> +	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_1_2},
> +	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_1, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_2, .pd = VEML6046X00_PD_2_2},
> +};
> +
> +/*
> + * Factors for lux / raw count in dependency of integration time (IT) as rows
> + * and driver gain in columns
> + */
> +static const u32 veml6046x00_it_gains[][6][2] = {
> +	/* integration time: 3.125 ms */

> +	/* integration time: 400 ms */
> +	{
> +		{ 0,  42000 },	/* gain: x0.25 */
> +		{ 0,  31817 },	/* gain: x0.33 */
> +		{ 0,  21000 },	/* gain: x0.5 */
> +		{ 0,  15909 },	/* gain: x0.66 */
> +		{ 0,  10500 },	/* gain: x1 */
> +		{ 0,   5250 }	/* gain: x2 */

Convention is trailing comma for anything that isn't an explicit
terminator.  That tends to apply even when w know there will never
be more elements like here.  Same for earlier sub arrays.

> +	},
> +};

> +
> +static int veml6046x00_single_read(struct iio_dev *iio,
> +					enum iio_modifier modifier, int *val)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int addr, it_usec, ret;
> +	__le16 reg;
> +
> +	switch (modifier) {
> +	case IIO_MOD_LIGHT_RED:
> +		addr = VEML6046X00_REG_R;
> +		break;
> +	case IIO_MOD_LIGHT_GREEN:
> +		addr = VEML6046X00_REG_G;
> +		break;
> +	case IIO_MOD_LIGHT_BLUE:
> +		addr = VEML6046X00_REG_B;
> +		break;
> +	case IIO_MOD_LIGHT_IR:
> +		addr = VEML6046X00_REG_IR;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6046x00_get_it_usec(data, &it_usec);
> +	if (ret < 0)
> +		return ret;

Why is suspending not appropriate if this fails?
Sure comms may well be dead, but maybe we should try?
If not a comment is needed on why not.

> +
> +	ret = regmap_field_write(data->rf.mode, 1);
> +	if (ret)
> +		return ret;

As above.

> +
> +	ret = regmap_field_write(data->rf.trig, 1);
> +	if (ret)
> +		return ret;

As above.

> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + it_usec / 10);
> +
> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> +	if (ret != 1)
> +		goto no_data;
> +
> +	if (!iio_device_claim_direct(iio))
> +		return -EBUSY;
> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	iio_device_release_direct(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	*val = le16_to_cpu(reg);
> +
> +	return IIO_VAL_INT;
> +
> +no_data:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return -EAGAIN;
> +}

> +
> +static irqreturn_t veml6046x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret;
> +	struct {
> +		__le16 chans[4];
> +		aligned_s64 timestamp;
> +	} scan;
> +
> +	memset(&scan, 0, sizeof(scan));

Can use
	} scan = {};

as a shorter way to ensure it's zeroed.
The scattering of memset date back to me not being sure if {} zero's
holes. The C spec recent versions say that it does, and the kernel build options
should ensure it does.


> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R,
> +					&scan.chans, sizeof(scan.chans));
> +	if (ret)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));

I've recently merged 
iio_push_buffers_with_ts() that in addition takes sizeof(scan) as a parameter
and allows the core code to run a sanity check that we haven't gotten the
size wrong. As it looks like you are respinning anyway, please consider
using that. I won't insist on it as it is very new and there are lots
of drivers to convert anyway so one more doesn't matter much!

> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}