Re: [PATCH 2/2] iio: light: add support for veml3235

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Wed, 16 Oct 2024 23:39:32 +0200
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:

> The Vishay veml3235 is a low-power ambient light sensor with I2C
> interface. It provides a minimum detectable intensity of
> 0.0021 lx/cnt, configurable integration time and gain, and an additional
> white channel to distinguish between different light sources.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>

A few minor things inline but looking good in general to me.


> diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
> new file mode 100644
> index 000000000000..9bda42379456
> --- /dev/null
> +++ b/drivers/iio/light/veml3235.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML3235 Ambient Light Sensor
> + *
> + * Copyright (c) 2024, Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
> + *
> + * Datasheet: https://www.vishay.com/docs/80131/veml3235.pdf
> + * Appnote-80222: https://www.vishay.com/docs/80222/designingveml3235.pdf
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define VEML3235_REG_CONF       0x00
> +#define VEML3235_REG_WH_DATA    0x04
> +#define VEML3235_REG_ALS_DATA   0x05
> +#define VEML3235_REG_ID         0x09
> +
> +#define VEML3235_SD             BIT(0)
> +#define VEML3235_SD0            BIT(8)

Name these so it's obvious they are in REG_CONF.
#define VEML3235_CONF_SD
etc.



> +
> +static int veml3235_set_gain(struct iio_dev *indio_dev, int val, int val2)
> +{
> +	struct veml3235_data *data = iio_priv(indio_dev);
> +	int ret, new_gain, gain_idx;

	if (val2 !== 0)
		return -EINVAL;

	switch (val)
.. 
	default:
		return -EINVAL;

> +
> +	if (val == 1 && val2 == 0) {
> +		new_gain = 0x00;
> +		gain_idx = 3;
> +	} else if (val == 2 && val2 == 0) {
> +		new_gain = 0x01;
> +		gain_idx = 2;
> +	} else if (val == 4 && val2 == 0) {
> +		new_gain = 0x03;
> +		gain_idx = 1;
> +	} else if (val == 8 && val2 == 0) {
> +		new_gain = 0x07;
> +		gain_idx = 0;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_field_write(data->rf.gain, new_gain);
> +	if (ret) {
> +		dev_err(data->dev, "failed to set gain: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (data->gain < gain_idx)
> +		data->resolution <<= gain_idx - data->gain;
> +	else if (data->gain > gain_idx)
> +		data->resolution >>= data->gain - gain_idx;
> +
> +	data->gain = gain_idx;
> +
> +	return 0;
> +}

> +static int veml3235_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*vals = (int *)&veml3235_it_times;
> +		*length = 2 * ARRAY_SIZE(veml3235_it_times);
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)&veml3235_scale_vals;
> +		*length = ARRAY_SIZE(veml3235_scale_vals);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	}
> +
> +	return -EINVAL;

Push this into a default: as it lets static analysis tools be sure you intended
to only handle those two cases.

> +}
>
> +static int veml3235_read_id(struct veml3235_data *data)
> +{
> +	int ret, reg;
> +
> +	ret = regmap_field_read(data->rf.id,  &reg);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "failed to read ID\n");
> +
> +	if (reg != 0x35)
> +		return dev_err_probe(data->dev, -ENODEV, "Unknown ID %d\n", reg);

This looks to be breaking any potential use of fallback dt compatibles for
future devices that are fully backwards compatible.

Should just be a dev_info() to log that we aren't sure what the device is then
carry on anyway on basis the firmware tables should be correct.


> +
> +	return 0;
> +}