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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 26 May 2025 10:50:40 +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,

I missed a channel type issue. See below.

> diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> new file mode 100644
> index 000000000000..9972eeb57fd2
> --- /dev/null
> +++ b/drivers/iio/light/veml6046x00.c
> @@ -0,0 +1,1007 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML6046X00 High Accuracy RGBIR Color Sensor
> + *
> + * Copyright (c) 2025 Andreas Klinger <ak@it-klinger.de>
> + */

> +static const struct iio_chan_spec veml6046x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,

Sorry - I've been half asleep on earlier versions or maybe we discussed this
before and I've forgotten.  IIO_LIGHT is for illuminance with units of lux.

How does that apply to a colour channel given it's a measure of a specific
sensitivity curve for the human eye and such a thing only applies to 'whiteish'
light.

Normally we cover colour channels as IIO_INTENSITY which doesn't have
strong rules for scaling - so lets us get away with the many weird and wonderful
ideals different sensor manufacturers have of what RED / GREEN / BLUE
mean.  (There is an oddity for historical reasons IIRC of an IR light channel
but don't use that for new code).

So basically the request is to use .type = IIO_INTENSITY for these

> +		.address = VEML6046X00_REG_R,
> +		.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) |
> +					   BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = VEML6046X00_SCAN_R,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6046X00_REG_G,
> +		.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) |
> +					   BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = VEML6046X00_SCAN_G,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6046X00_REG_B,
> +		.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) |
> +					   BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = VEML6046X00_SCAN_B,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6046X00_REG_IR,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> +					   BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						     BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = VEML6046X00_SCAN_IR,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(VEML6046X00_SCAN_TIMESTAMP),
> +};