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

From: Jonathan Cameron
Date: Mon Mar 17 2025 - 07:51:00 EST

Next message: Johan Hovold: "Re: [PATCH v2 0/6] arm64: dts: qcom: x1e80100: enable rtc"
Previous message: Alice Ryhl: "Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe"
In reply to: Andreas Klinger: "[PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor"
Next in thread: Andreas Klinger: "Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, 16 Mar 2025 12:31:30 +0100
Andreas Klinger <ak@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>

Hi Andreas,

A nice clean driver.
A few comments and questions inline.

Jonathan

> diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> new file mode 100644
> index 000000000000..8e6232e1ab70
> --- /dev/null
> +++ b/drivers/iio/light/veml6046x00.c
> @@ -0,0 +1,890 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML6046X00 High Accuracy RGBIR Color Sensor
> + *
> + * Copyright (c) 2025 Andreas Klinger <ak@xxxxxxxxxxxxx>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>

So far you aren't providing a trigger so I'm not
expect to see this header used. Looking at the datasheet
I see there is a dataready interrupt, but it doesn't look like
the device has an autonomous / sequencer type mode, so will always
need to use another trigger (hrtimer, sysfs or some other hardware
source).

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

> +
> +struct veml6046x00_scan_buf {
> + __le16 chans[4];
> +
> + s64 timestamp __aligned(8);
aligned_s64 now available as a type to use here.

> +};

> +static IIO_DEVICE_ATTR_RO(in_illuminance_period_available, 0);
> +
> +static struct attribute *veml6046x00_event_attributes[] = {
> + &iio_dev_attr_in_illuminance_period_available.dev_attr.attr,
I thought we didn't have event support yet? If not why do we
have event related attributes?
> + NULL
> +};
> +
> +static const struct attribute_group veml6046x00_event_attr_group = {
> + .attrs = veml6046x00_event_attributes,
> +};

> +
> +static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int ret, new_it;
> +
> + if (val)
> + return -EINVAL;
> +
> + switch (val2) {
> + case 3125:
> + new_it = 0x00;
> + break;
> + case 6250:
> + new_it = 0x01;
> + break;
> + case 12500:
> + new_it = 0x02;
> + break;
> + case 25000:
> + new_it = 0x03;
> + break;
> + case 50000:
> + new_it = 0x04;
> + break;
> + case 100000:
> + new_it = 0x05;
> + break;
> + case 200000:
> + new_it = 0x06;
> + break;
> + case 400000:
> + new_it = 0x07;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = regmap_field_write(data->rf.it, new_it);
> + if (ret)
> + return ret;

return regmap_field_write()

> +
> + return 0;
> +}
> +
> +static int veml6046x00_set_scale(struct iio_dev *iio, int val, int val2)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int new_scale;
> +
> + if (val == 0 && val2 == 250000) {

Maybe a lookup table and a loop?

> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5) |
> + VEML6046X00_CONF1_PD_D2;
> + } else if (val == 0 && val2 == 330000) {
> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66) |
> + VEML6046X00_CONF1_PD_D2;
> + } else if (val == 0 && val2 == 500000) {
> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5);
> + } else if (val == 0 && val2 == 660000) {
> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66);
> + } else if (val == 1 && val2 == 0) {
> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_1);
> + } else if (val == 2 && val2 == 0) {
> + new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_2);
> + } else {
> + return -EINVAL;
> + }
> +
> + return regmap_update_bits(data->regmap, VEML6046X00_REG_CONF1,
> + VEML6046X00_CONF1_GAIN |
> + VEML6046X00_CONF1_PD_D2,
> + new_scale);
> +}
> +
> +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> + int *val, int *val2)

How is this related to integration time? I'd normally expect
to see that read in here somewhere as well as doubling integration
time tends to double scale.

> +{
> + int ret, reg;
> +
> + ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
> + if (ret)
> + return ret;
> +
> + switch (FIELD_GET(VEML6046X00_CONF1_GAIN, reg)) {
> + case 0:
> + *val = 1;
> + *val2 = 0;
> + break;
> + case 1:
> + *val = 2;
> + *val2 = 0;
> + break;
> + case 2:
> + *val = 0;
> + *val2 = 660000;
> + break;
> + case 3:
> + *val = 0;
> + *val2 = 500000;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (reg & VEML6046X00_CONF1_PD_D2)
> + *val2 /= 2;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int veml6046x00_set_mode(struct veml6046x00_data *data, bool state)
> +{
> + return regmap_field_write(data->rf.mode, state);
as below.

> +}
> +
> +static int veml6046x00_set_trig(struct veml6046x00_data *data, bool state)
> +{
> + return regmap_field_write(data->rf.trig, state);

I'd argue these two aren't worth bothering with because the
field naming etc makes a direct call to regmap_field_write() obvious enough.

> +}
> +
> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)

Document return value as non obvious.

> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int ret, reg;
> + int cnt = 2;
> + int i;
> +
> + for (i = 0; i < cnt; i++) {
> + ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
> + if (ret) {
> + dev_err(data->dev,
> + "Failed to read interrupt register %d\n", ret);
> + return -EIO;
> + }
> +
> + if (reg & VEML6046X00_INT_DRDY)
> + return 1;
> +
> + if (i < cnt)
> + fsleep(usecs);
> + }
> +
> + return 0;
> +}
> +
> +static int veml6046x00_single_read(struct iio_dev *iio,
> + enum iio_modifier modifier, int *val)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int addr, it_usec, ret;
> + uint8_t reg[2];
> +
> + switch (modifier) {
> + case IIO_MOD_LIGHT_RED:
> + addr = VEML6046X00_REG_R_L;
> + break;

break indent not matching kernel style. Needs to be one more tab in.

> + case IIO_MOD_LIGHT_GREEN:
> + addr = VEML6046X00_REG_G_L;
> + break;
> + case IIO_MOD_LIGHT_BLUE:
> + addr = VEML6046X00_REG_B_L;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + addr = VEML6046X00_REG_IR_L;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret)
> + return ret;
> +
> + ret = veml6046x00_get_it_usec(data, &it_usec);
> + if (ret < 0)
> + return ret;
> +
> + veml6046x00_set_mode(data, 1);
Check for errors.
> +
> + veml6046x00_set_trig(data, 1);
> +
> + /* integration time + 10 % to ensure completion */
> + fsleep(it_usec + (it_usec / 10));
> +
> + ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> + if (ret == 1) {
> + dev_dbg(data->dev, "data ready\n");
> + } else {
> + dev_warn(data->dev, "no data ready ret: %d\n", ret);
> + goto no_data;
> + }
> +
> + ret = iio_device_claim_direct_mode(iio);
I'm killing these off slowly. To save a follow up patch,
if (!iio_device_claim_direct(iio))
return -EBUSY;
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_read(data->regmap, addr, reg, sizeof(reg));
> + iio_device_release_direct_mode(iio);

iio_device_release_direct(iio);

If not I'll roll in the change with the many other driver updates
this entails.

> + if (ret)
> + return ret;
> +
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
> +
> + *val = reg[1] << 8 | reg[0];

That's an endian conversion. Use get_unaligned_le16() I think
Or read into an __le16 in the first place and you can use
le16_to_cpu() or similar.

> +
> + return IIO_VAL_INT;
> +
> +no_data:
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
> +
> + return -EINVAL;
> +}

> +static int veml6046x00_buffer_preenable(struct iio_dev *iio)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + struct device *dev = data->dev;
> + int ret;
> +
> + ret = veml6046x00_set_mode(data, 0);
> + if (ret)
> + dev_err(data->dev, "Failed to set mode %d\n", ret);

If these fail, error out. We will fail to enter buffered mode, but
that is probably the correct thing to do if we are having comms
issues or similar.

> +
> + ret = veml6046x00_set_trig(data, 0);
> + if (ret)
> + dev_err(data->dev, "Failed to set trigger %d\n", ret);
> +
> + return pm_runtime_resume_and_get(dev);
> +}

> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> + int part_id, ret;
> + __le16 reg;
> +
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));
> + if (ret) {
> + dev_info(data->dev, "Failed to read ID\n");

return dev_err_probe() for this one.

> + return -EIO;
> + }
>
> +static int veml6046x00_setup_device(struct iio_dev *iio)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + struct device *dev = data->dev;
> + int ret, val;
> + __le16 reg16;
> + uint8_t reg[2];
> +
> + reg[0] = VEML6046X00_CONF0_AF;
> + reg[1] = 0x00;
> + ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set configuration\n");
> +
> + reg16 = cpu_to_le16(0);
> + ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> + reg16 = cpu_to_le16(U16_MAX);
> + ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> + ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
> + if (ret < 0)

if (ret) here as well. Good to be consistent for all regmap calls. None of them
return > 0 as far as I know.

> + return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> + return 0;
> +}
> +
> +static int veml6046x00_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct veml6046x00_data *data;
> + struct iio_dev *iio;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to set regmap\n");
> +
> + iio = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!iio)
> + return -ENOMEM;
> +
> + data = iio_priv(iio);
> + i2c_set_clientdata(i2c, iio);
> + data->dev = dev;
> + data->regmap = regmap;
> +
> + ret = veml6046x00_regfield_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to init regfield\n");
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> +
> + ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);

Mostly we want a devm action to match against a specific setup operation. Here is
it that the device comes up in non shut down state? Perhaps a comment to
make it clear. Also, how do we know it's in a good state rather than part
configured by someone else? I'm not seeing a reset sequence though perhaps
that effectively happens in setup_device()

> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to add shut down action\n");
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> + pm_runtime_get_noresume(dev);
> + pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
> + pm_runtime_use_autosuspend(dev);
> +
> + ret = veml6046x00_validate_part_id(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to validate device ID\n");
> +
> + iio->name = i2c->name;

Prefer this hard coded. Depending on the firmware type, things like that have
an annoying habbit of not remaining predictable or stable.

> + iio->channels = veml6046x00_channels;
> + iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
> + iio->modes = INDIO_DIRECT_MODE;
> +
> + iio->info = &veml6046x00_info_no_irq;
> +
> + ret = veml6046x00_setup_device(iio);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> + veml6046x00_trig_handler,
> + &veml6046x00_buffer_setup_ops);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to register triggered buffer");
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + ret = devm_iio_device_register(dev, iio);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register iio device");
> +
> + return 0;
> +}

Next message: Johan Hovold: "Re: [PATCH v2 0/6] arm64: dts: qcom: x1e80100: enable rtc"
Previous message: Alice Ryhl: "Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe"
In reply to: Andreas Klinger: "[PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor"
Next in thread: Andreas Klinger: "Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]