Message-ID: <aHdWAUMMH43tIqV4@smile.fi.intel.com>
Date: Wed, 16 Jul 2025 10:34:25 +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 v6 2/3] iio: light: add support for veml6046x00 RGBIR
 color sensor

On Tue, Jul 15, 2025 at 10:58:09AM +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.

...

> +#define	VEML6046X00_GAIN_1          0x0
> +#define	VEML6046X00_GAIN_2          0x1
> +#define	VEML6046X00_GAIN_0_66       0x2
> +#define	VEML6046X00_GAIN_0_5        0x3

Is it defined as hexadecimal in the datasheet? Otherwise use plain decimal
numbers.

...

> +static int veml6046x00_get_it_usec(struct veml6046x00_data *data, unsigned int *it_usec)
> +{
> +	int ret;
> +	unsigned int reg;
> +
> +	ret = regmap_field_read(data->rf.it, &reg);
> +	if (ret)
> +		return ret;
> +
> +	if (reg >= ARRAY_SIZE(veml6046x00_it))
> +		return -EINVAL;
> +
> +	*it_usec = (unsigned int)veml6046x00_it[reg][1];

Is this casting needed? According to the C standard unsigned has higher rank
than signed.

> +	return IIO_VAL_INT_PLUS_MICRO;
> +}

...

> +static int veml6046x00_wait_data_available(struct iio_dev *iio, unsigned int usecs)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	ret = veml6046x00_read_data_ready(data);

> +	if (ret == 0) {

What's wrong with the regular pattern, i.e.

	if (ret)
		return ret;

?

> +		fsleep(usecs);
> +		ret = veml6046x00_read_data_ready(data);
> +	}
> +
> +	return ret;
> +}

...

> +	ret = veml6046x00_wait_data_available(iio, it_usec * 4);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0)
> +		return -EAGAIN;
> +
> +	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;

All error paths above are missing the below runtime PM calls, is it on purpose?
If so, comment is a must to explain what's going on here.

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

Do you need this (nullification) assignment?

> +	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));
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	unsigned int part_id;
> +	int 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)
> +		dev_info(dev, "Unknown ID %#04x\n", part_id);

For 0 it will print 0 and not 0x0000. Is it okay?

> +	return 0;
> +}

...

> +static int veml6046x00_setup_device(struct iio_dev *iio)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +	__le16 reg16;
> +	u8 reg[2] = { VEML6046X00_CONF0_AF, 0x00 };

0x00 looks like a part of the reg. If so, this should be actually typed as
__le16. Otherwise, in case this is just a simple terminator, use (decimal) 0.

> +	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");

So, this actually will be as simple as

	reg16 = cpu_to_le16(VEML6046X00_CONF0_AF);
	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0,
				&reg16, sizeof(reg16));
	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,
> +				&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,
> +				&reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT,
> +			       &reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> +	return 0;
> +}

...

> +static const struct of_device_id veml6046x00_of_match[] = {
> +	{
> +		.compatible = "vishay,veml6046x00",
> +	},

Can be just a single line.

	{ .compatible = "vishay,veml6046x00" },

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists