Re: [PATCH v6 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Wed, 12 Aug 2026 22:27:41 +0200
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:

> These sensors provide two light channels (ALS and IR), I2C communication
> and a multiplexed interrupt line to signal data ready and configurable
> threshold alarms.
> 
> This first implementation provides basic functionality (measurement
> configuration, raw reads and ID validation) and defines the different
> register regions in preparation for extended features in the subsequent
> patches of the series.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
Hi Javier,

A few minor things inline

Thanks,

Jonathan

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 000000000000..021bf726c60b
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c

> +
> +static const struct iio_chan_spec veml6031x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6031X00_REG_ALS_L,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_INT_TIME) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.address = VEML6031X00_REG_IR_L,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}

Trailing comma.  Maybe we'll add more after this and they will fit on one line.
Yeah it's a stretch but generally always add comma unless it is a specific
terminating entry.

> +};


> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int part_id, ret;
> +	__le16 regval;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_ID_L, &regval,
> +			       sizeof(regval));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read ID\n");
> +
> +	part_id = le16_to_cpu(regval);
> +	if (part_id != data->chip->part_id)
> +		dev_info(dev, "Unknown ID %04x\n", part_id);

This will trigger on unknown or unexpected ID (i.e one we know about
but not the one firmware indicated we should expect).  I'd rephrase
to include that.

> +
> +	return 0;
> +}

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct veml6031x00_data *data;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio);
> +	i2c_set_clientdata(i2c, iio);
> +
> +	data->chip = i2c_get_match_data(i2c);
> +	if (!data->chip)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
> +
> +	data->regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "Failed to set regmap\n");
> +
> +	iio->name = data->chip->name;
> +	iio->channels = veml6031x00_channels;
> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &veml6031x00_info;
> +
> +	ret = devm_mutex_init(dev, &data->scale_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6031x00_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");
> +
> +	/* The device starts in power down mode by default */
> +	ret = veml6031x00_set_power(data, true);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to power on the device\n");
> +
> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	pm_runtime_get_noresume(dev);
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		goto err_pm_put;
> +
> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		goto err_pm_put;
> +
> +	pm_runtime_put_autosuspend(dev);

This and the get_noresume() above smell like they are probably here to ensure
a 1->0 transition for the runtime pm reference count and hence kick off the timer
for autosuspend.   That shouldn't be necessary as there is code in the driver
core to try a suspend. Look for pm_request_idle() calls in drivers/base/dd.c

Note that we've had sashiko give what we have concluded in the past are false
positives on this dance being required.  
Given you have device to hand, just probe it and check if it suspends.

I haven't quite had the nerve to yet post a series removing this dance from
drivers I can't test!

> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +
> +err_pm_put:
> +	pm_runtime_put_noidle(dev);
> +	return ret;
> +}