Message-ID: <aCsQKUwGeq4Ed4ai@smile.fi.intel.com>
Date: Mon, 19 May 2025 14:04:09 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Andreas Klinger <ak@...klinger.de>
Cc: jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, lars@...afoo.de,
	javier.carrasco.cruz@...il.com, mazziesaccount@...il.com,
	arthur.becker@...tec.com, perdaniel.olsson@...s.com,
	mgonellabolduc@...onoff.com, muditsharma.info@...il.com,
	clamor95@...il.com, emil.gedenryd@...s.com,
	devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] iio: light: add support for veml6046x00 RGBIR
 color sensor

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

Powered by blists - more mailing lists