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

[Andy Shevchenko]

On Wed, Aug 12, 2026 at 10:27:41PM +0200, Javier Carrasco 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.

This paragraph needs to be rephrased. In the current form it suits cover letter
and not the commit message. Here, just list the features supported.

The "the subsequent patches of the series." in the commit message is very
ambiguous. What patch series? Which patches? Are they landed in the upstream?
If yes, which commit IDs? If not, when if ever? Et cetera! Usually it can be
simply said "The other features may be implemented later on."

The basic hint (one of) when writing a commit message is to put yourself in
the shoes of the unprepared reader who sees it in the Git history for the first
time in their life. What will your message give to them?

Besides the above some comments in the ->probe() to be addressed and other
minor style issues all over, but in general this looks very good and on track
(for v7.4).

...

> +/*
> + * The shutdown bits (SD and ALS_IR_SD) are in different registers, and both
> + * must be updated when changing the device power state.
> + */
> +static int veml6031x00_set_power(struct veml6031x00_data *data, bool state)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_SD,
> +				 state ? 0 : VEML6031X00_CONF0_SD);

_assign_bits()?

> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF1,
> +				  VEML6031X00_CONF1_IR_SD,
> +				  state ? 0 : VEML6031X00_CONF1_IR_SD);

Ditto.

> +}

...

> +static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, gain_sel, new_gain, prev_gain, prev_it;
> +	unsigned int gain_reg, it_idx, pd_div4;
> +	bool gain_in_range;
> +
> +	if (val || !iio_gts_valid_time(&data->gts, val2))
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->scale_lock);
> +
> +	ret = regmap_field_read(data->rf.it, &it_idx);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_read(data->rf.gain, &gain_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_read(data->rf.pd_div4, &pd_div4);
> +	if (ret)
> +		return ret;
> +
> +	prev_it = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
> +	if (prev_it < 0)
> +		return prev_it;
> +
> +	if (prev_it == val2)
> +		return 0;
> +
> +	prev_gain = iio_gts_find_gain_by_sel(&data->gts,
> +					     VEML6031X00_GAIN_SEL(pd_div4, gain_reg));

What about temporary for &data->gts? Say you have local 'gts' variable, this becomes

	prev_gain = iio_gts_find_gain_by_sel(gts,VEML6031X00_GAIN_SEL(pd_div4, gain_reg));

> +	if (prev_gain < 0)
> +		return prev_gain;
> +
> +	ret = iio_gts_find_new_gain_by_gain_time_min(&data->gts, prev_gain, prev_it,
> +						     val2, &new_gain, &gain_in_range);

	ret = iio_gts_find_new_gain_by_gain_time_min(gts, prev_gain, prev_it, val2,
						     &new_gain, &gain_in_range);

> +	if (ret)
> +		return ret;
> +
> +	if (!gain_in_range)
> +		dev_dbg(regmap_get_device(data->regmap), "Optimal gain out of range\n");
> +
> +	ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_field_write(data->rf.it, ret);
> +	if (ret)
> +		return ret;
> +
> +	gain_sel = iio_gts_find_sel_by_gain(&data->gts, new_gain);
> +	if (gain_sel < 0)
> +		return gain_sel;
> +
> +	return veml6031x00_write_gain(data, gain_sel);
> +}
> +
> +static int veml6031x00_set_scale(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int gain_sel, it_sel, ret;
> +
> +	ret = iio_gts_find_gain_time_sel_for_scale(&data->gts, val, val2,
> +						   &gain_sel, &it_sel);

In the similar way

	ret = iio_gts_find_gain_time_sel_for_scale(gts, val, val2, &gain_sel, &it_sel);

OR (for stricter limits)

	ret = iio_gts_find_gain_time_sel_for_scale(gts, val, val2,
						   &gain_sel, &it_sel);


> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&data->scale_lock);
> +
> +	ret = regmap_field_write(data->rf.it, it_sel);
> +	if (ret)
> +		return ret;
> +
> +	return veml6031x00_write_gain(data, gain_sel);
> +}

...

> +static int veml6031x00_get_scale(struct veml6031x00_data *data, int *val,
> +				 int *val2)

Split this logically

static int veml6031x00_get_scale(struct veml6031x00_data *data, int *val, int *val2)

OR

static int veml6031x00_get_scale(struct veml6031x00_data *data,
				 int *val, int *val2)

> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int part_id, ret;

Why is 'part_id' signed?

> +	__le16 regval;

> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_ID_L, &regval,
> +			       sizeof(regval));

With

	struct regmap *map = data->regmap;
	struct device *dev = regmap_get_device(map);
	unsigned int part_id;
	__le16 val;
	int ret;
	...

	ret = regmap_bulk_read(map, VEML6031X00_REG_ID_L, &val, sizeof(val));

Please, revisit the whole series for this type of improvements.

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

-ENODATA

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

Is debugfs access already enabled for regmap after this call? Perhaps you want
mutex to be initialised before that?

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

Hmm... This is usually a red flag to see a goto after devm_*() calls.

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko