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

Andy Shevchenko
2025-05-19T11:04:17Z

On Mon, May 19, 2025 at 08:08:03AM +0200, Andreas Klinger 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.

> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +/*
> + * 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

This is made to look like kernel-doc, but it's not marked as a such, why?

> + */
> +struct veml6046x00_gain_pd {
> + int gain_sen;
> + int pd;
> +};

...

> +/*
> + * Factors for lux / raw count in dependency of integration time (IT) as rows
> + * and driver gain in columns

Missing period at the end. Please, fix all your multi-line comments
accordingly.

> + */

...

> + ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> + VEML6046X00_CONF0_ON_0);

Something wrong with the indentation. Please, fix all places like this...

> + if (ret) {
> + dev_err(dev, "Failed to set bit for power on %d\n", ret);
> + return ret;
> + }
> +
> + return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
> + VEML6046X00_CONF1_ON_1);

...or like this.

> +}

...

> +static int veml6046x00_get_it_index(struct veml6046x00_data *data)
> +{
> + int ret;
> + int reg;

Why the 'reg' is signed? regmap API doesn't operate on signed values. Please
fix all places in your code.

> +
> + ret = regmap_field_read(data->rf.it, &reg);
> + if (ret)
> + return ret;
> +
> + /* register value is identical with index of array */
> + if ((reg < 0) || (reg >= ARRAY_SIZE(veml6046x00_it)))

in_range() ?

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

...

> +static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)

Same comments as per above function.

...

> +static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
> + int val2)
> +{
> + u32 i;

Why fixed-width type? Wouldn't unsigned int i work?
Please, fix in all places. The rule of thumb is to use fixed-width types either
when it's HW / protocol specific, or when the respective API uses the same type.
Otherwise use PODs.

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

...

> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret, i, cnt = 2;
> + u8 reg[2];
> +
> + for (i = 0; i < cnt; i++) {
> + /*
> + * Note from the vendor, but not explicitly in the datasheet: we
> + * should always read both registers together
> + */
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT_L,

Please, drop _L if not used as a single byte access.

> + &reg, sizeof(reg));
> + if (ret) {
> + dev_err(dev,
> + "Failed to read interrupt register %d\n", ret);
> + return -EIO;
> + }
> +
> + if (reg[1] & VEML6046X00_INT_DRDY)
> + return 1;
> +
> + fsleep(usecs);
> + }
> +
> + return 0;
> +}

...

> + /* integration time + 10 % to ensure completion */
> + fsleep(it_usec + it_usec / 10);

I would suggest / 8 as it gives much better code generation. Divisions are
slow and hard.

> + ret = veml6046x00_wait_data_available(iio, it_usec * 10);

Also it won't mess with semantics of '10' here.

> + if (ret != 1)

Can it return negative error? If not, why is error code shadowed?

> + goto no_data;

...

> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int part_id, ret;
> + __le16 reg;
> +
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID,
> + &reg, sizeof(reg));
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to read ID\n");
> +
> + part_id = le16_to_cpu(reg);
> + if (part_id != 0x0001)

Here you put 4 digits...

> + dev_info(dev, "Unknown ID %#02x\n", part_id);

...and here you are expecting that it may be two only. Please, make these two
consistent.

> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko