Re: [PATCH 7/7] iio: light: veml6030: add support for veml6035

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 13 Sep 2024 15:19:02 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The veml6035 is an ALS that shares most of its functionality with the
> veml6030, which allows for some code recycling.
> 
> Some chip-specific properties differ and dedicated functions to get and
> set the sensor gain as well as its initialization are required.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Mostly a request to first switch to using read_avail() and the relevant
bit masks instead of custom attributes.  That will require converting the
driver to that approach first, but looks straight forward.

> ---
>  drivers/iio/light/veml6030.c | 300 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 273 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 2945cc1db599..105f310c4954 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -1,13 +1,19 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * VEML6030 Ambient Light Sensor
> + * VEML6030 and VMEL6035 Ambient Light Sensors
>   *
>   * Copyright (c) 2019, Rishi Gupta <gupt21@gmail.com>
>   *
> + * VEML6030:
>   * Datasheet: https://www.vishay.com/docs/84366/veml6030.pdf
>   * Appnote-84367: https://www.vishay.com/docs/84367/designingveml6030.pdf
> + *
> + * VEML6035:
> + * Datasheet: https://www.vishay.com/docs/84889/veml6035.pdf
> + * Appnote-84944: https://www.vishay.com/docs/84944/designingveml6035.pdf
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/err.h>
> @@ -38,16 +44,33 @@
>  #define VEML6030_ALS_INT_EN   BIT(1)
>  #define VEML6030_ALS_SD       BIT(0)
>  
> +#define VEML6035_GAIN_M       GENMASK(12, 10)
> +#define VEML6035_GAIN         BIT(10)
> +#define VEML6035_DG           BIT(11)
> +#define VEML6035_SENS         BIT(12)
> +#define VEML6035_INT_CHAN     BIT(3)
> +#define VEML6035_CHAN_EN      BIT(2)
> +
> +struct veml603x_chip {
> +	const char *name;
> +	const struct iio_info *info;
> +	const struct iio_info *info_no_irq;
> +	const char * const in_illuminance_scale_avail;

For this, better with read_avail() provided and a pointer to an array of
values + a size element in here.  That way we can get rid of the
custom attribute handling.  Might end up as similar amount of code, but
will be simpler to read.

> +	int (*hw_init)(struct iio_dev *indio_dev);
> +	int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
> +	int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
> +};

>  
>  /* Integration time available in seconds */
> @@ -63,14 +87,25 @@ static IIO_CONST_ATTR(in_illuminance_integration_time_available,
>  
>  /*
>   * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
> - * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
> + * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1,
> + * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4.
>   */
> -static IIO_CONST_ATTR(in_illuminance_scale_available,
> +static IIO_CONST_ATTR_NAMED(veml6030_in_illuminance_scale_available,
> +			    in_illuminance_scale_available,
>  				"0.125 0.25 1.0 2.0");
> +static IIO_CONST_ATTR_NAMED(veml6035_in_illuminance_scale_available,
> +			    in_illuminance_scale_available,
> +				"0.125 0.25 0.5 1.0 2.0 4.0");
>  
>  static struct attribute *veml6030_attributes[] = {
>  	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> -	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> +	&iio_const_attr_veml6030_in_illuminance_scale_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *veml6035_attributes[] = {
> +	&iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> +	&iio_const_attr_veml6035_in_illuminance_scale_available.dev_attr.attr,

Using get_avail() etc would let you handle these as arrays of numbers rather than
strings + get rid of the need for any custom attributes. This should be
a very simple conversion so perhaps worth doing before adding the
new support.  Then you will have pointers to the value arrays + sizes
in your chip specific structures that just get looked up directly
by read_avail()


>  	NULL
>  };


>  
> +/*
> + * Set ALS gain to 1/8, integration time to 100 ms, ALS and WHITE
> + * channel enabled, ALS channel interrupt, PSM enabled,
> + * PSM_WAIT = 0.8 s, persistence to 1 x integration time and the
> + * threshold interrupt disabled by default. First shutdown the sensor,
> + * update registers and then power on the sensor.
> + */
> +static int veml6035_hw_init(struct iio_dev *indio_dev)
> +{
> +	int ret, val;
> +	struct veml6030_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +
> +	ret = veml6030_als_shut_down(data);
> +	if (ret) {
> +		dev_err(&client->dev, "can't shutdown als %d\n", ret);
> +		return ret;

If this is only ever called from probe() (I think that's true?)
can use return dev_err_probe() for all these error cases.
Main advantage here being shorter simpler code.

> +	}
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> +			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
> +	if (ret) {
> +		dev_err(&client->dev, "can't setup als configs %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
> +				 VEML6030_PSM | VEML6030_PSM_EN, 0x03);
> +	if (ret) {
> +		dev_err(&client->dev, "can't setup default PSM %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
> +	if (ret) {
> +		dev_err(&client->dev, "can't setup high threshold %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
> +	if (ret) {
> +		dev_err(&client->dev, "can't setup low threshold %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = veml6030_als_pwr_on(data);
> +	if (ret) {
> +		dev_err(&client->dev, "can't poweron als %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Clear stale interrupt status bits if any during start */
> +	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"can't clear als interrupt status %d\n", ret);
> +		return ret;

It's true of existing code, but I noticed it here.
Should we be powering down in this error path?

> +	}
> +
> +	/* Cache currently active measurement parameters */
> +	data->cur_gain = 5;
> +	data->cur_resolution = 1024;
> +	data->cur_integration_time = 3;
> +
> +	return 0;
> +}