Hi Jonathan,
thanks for the review and comments. Answers are inline below.
Andreas
Jonathan Cameron <jic23@xxxxxxxxxx> schrieb am Mo, 17. Mär 11:50:
> 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).
I'll remove.
> > +#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?
I started implementing including event support by using the interrupt and the
threshold values which can be programmed on the sensor. It turned out that this
is not working as i expected on my side. Then i removed everything which is
about events, but forgot this here. I'm in the process of clarifying this with
the vendor. The plan is to submit a follow up patch later.
So i'll remove for this turn.
> > + 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?
Yes, would be nicer.
> > + 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.
>
That's true.
So i'll also need to present different gain values depending on the integration time.
> > +{
> > + int ret, reg;
> > +
> > + ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, ®);
> > + 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, ®);
> > + 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, ®, 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, ®16, 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, ®16, 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()
In veml6046x00_setup_device() all registers are set up to bring the device in a
known state. This function also switches the device on. I could move the call to
setup_device() up to here and add a comment to make it clear.
>
> > + 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;
> > +}
>
Attachment:
signature.asc
Description: PGP signature
