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

Jonathan Cameron
2025-05-25T14:30:23Z

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