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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Jul 2025 09:54:45 +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 things inline, but they are all the sort of thing I'll just tweak
whilst applying, if nothing else comes up. Saves time for both of us!

Given Andy has been active in reviewing earlier versions I'll leave this
for a little while.  If Andy is otherwise happy (or busy!) then I'll pick
this up and tweak some or maybe all of the things commented on

Thanks,

Jonathan

> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 8229ebe6edc4..c0048e0d5ca8 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_VCNL4035)		+= vcnl4035.o
>  obj-$(CONFIG_VEML3235)		+= veml3235.o
>  obj-$(CONFIG_VEML6030)		+= veml6030.o
>  obj-$(CONFIG_VEML6040)		+= veml6040.o
> +obj-$(CONFIG_VEML6046X00)	+= veml6046x00.o
>  obj-$(CONFIG_VEML6070)		+= veml6070.o
>  obj-$(CONFIG_VEML6075)		+= veml6075.o
>  obj-$(CONFIG_VL6180)		+= vl6180.o
> diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> new file mode 100644
> index 000000000000..adcb849b1c1f
> --- /dev/null
> +++ b/drivers/iio/light/veml6046x00.c

> +static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
> +					int val2)
> +{
> +	unsigned int i;
> +	int it_idx;
> +
> +	it_idx = veml6046x00_get_it_index(data);
> +	if (it_idx < 0)
> +		return it_idx;
> +
> +	for (i = 0; i < ARRAY_SIZE(veml6046x00_it_gains[it_idx]); i++) {
> +		if ((veml6046x00_it_gains[it_idx][i][0] == val) &&
> +		    (veml6046x00_it_gains[it_idx][i][1] == val2)) {
> +			return i;

As below.

> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int veml6046x00_get_gain_idx(struct veml6046x00_data *data)
> +{
> +	int ret;
> +	unsigned int i, reg, reg_gain, reg_pd;
> +
> +	ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
> +	if (ret)
> +		return ret;
> +
> +	reg_gain = FIELD_GET(VEML6046X00_CONF1_GAIN, reg);
> +	reg_pd = reg & VEML6046X00_CONF1_PD_D2;
> +
> +	for (i = 0; i < ARRAY_SIZE(veml6046x00_gain_pd); i++) {
> +		if ((veml6046x00_gain_pd[i].gain_sen == reg_gain) &&
> +		    (veml6046x00_gain_pd[i].pd == reg_pd)) {
> +			return i;

Technically style is no {} for this as only a single statement.
I can see why you did this given the more complex condition, but
I think the difference in indent is still enough to not need it. 

> +		}
> +	}
> +
> +	return -EINVAL;
> +}

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

If we spin again, preference for
	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan),
				    iio_get_time_ns(iio));

If not I might tweak whilst applying or might just leave this to get
swept up in a future series moving to that safer interface.

Jonathan