Message-ID: <Z9jXLYYbY7nKDwA-@mail.your-server.de>
Date: Tue, 18 Mar 2025 03:15:09 +0100
From: Andreas Klinger <ak@...klinger.de>
To: Jonathan Cameron <jic23@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	lars@...afoo.de, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	javier.carrasco.cruz@...il.com, mazziesaccount@...il.com,
	subhajit.ghosh@...aklogic.com, muditsharma.info@...il.com,
	arthur.becker@...tec.com, ivan.orlov0322@...il.com
Subject: Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color
 sensor

Hi Jonathan,

thanks for the review and comments. Answers are inline below.

Andreas


Jonathan Cameron <jic23@...nel.org> schrieb am Mo, 17. Mär 11:50:
> On Sun, 16 Mar 2025 12:31:30 +0100
> Andreas Klinger <ak@...klinger.de> 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@...klinger.de>
> 
> 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@...klinger.de>
> > + */
> > +
> > +#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, &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()

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

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists