Re: [PATCH 2/3] iio: light: opt3001: add support for TI's opt3002 light sensor

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 5 Sep 2024 12:20:46 +0200
Emil Gedenryd <emil.gedenryd@axis.com> wrote:

> TI's opt3002 light sensor shares most properties with the opt3001
> model, with the exception of supporting a wider spectrum range.
> 
> Add support for TI's opt3002 by extending the TI opt3001 driver.
> 
> See https://www.ti.com/product/OPT3002 for more information.
Make that a Datasheet tag.
> 
Datasheet: https://www.ti.com/product/OPT3002
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>

Various comments inline.
Thanks,

Jonathan

> ---
>  drivers/iio/light/Kconfig   |   2 +-
>  drivers/iio/light/opt3001.c | 199 ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 167 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index b68dcc1fbaca..c35bf962dae6 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -461,7 +461,7 @@ config OPT3001
>  	depends on I2C
>  	help
>  	  If you say Y or M here, you get support for Texas Instruments
> -	  OPT3001 Ambient Light Sensor.
> +	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor.
>  
>  	  If built as a dynamically linked module, it will be called
>  	  opt3001.
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 176e54bb48c3..e6098f88dd04 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -70,6 +70,19 @@
>  #define OPT3001_RESULT_READY_SHORT	150
>  #define OPT3001_RESULT_READY_LONG	1000
>  
> +/* The opt3002 doesn't have a device id register, predefine value instead */
> +#define OPT3002_DEVICE_ID_VALUE		3002

Why?  Just make the code not care about the value for this
device.  Add a flag to the chip info structure to say it doesn't have
one and check that before using it.


> +
> +enum chip_model {
> +	OPT3001,
This should not be needed. See below.

> +	OPT3002,
> +};
> +
> +struct opt300x_chip_info {
> +	enum chip_model model;
> +	enum iio_chan_type chan_type;
> +};
> +
>  struct opt3001 {
>  	struct i2c_client	*client;
>  	struct device		*dev;
> @@ -79,6 +92,7 @@ struct opt3001 {
>  	bool			result_ready;
>  	wait_queue_head_t	result_ready_queue;
>  	u16			result;
> +	const struct opt300x_chip_info *chip_info;
>  
>  	u32			int_time;
>  	u32			mode;
> @@ -97,6 +111,16 @@ struct opt3001_scale {
>  	int	val2;
>  };
>  
> +static const struct opt300x_chip_info opt3001_chip_info = {
> +	.model = OPT3001,
Having a model in a chip_info structure is almost always a sign
of a design that won't scale well to lots of additional devices.

Get rid of that and instead add all the 'data' that you are looking
up with that model number to this structure so it can be just
referenced without caring which mode it is for.

> +	.chan_type = IIO_LIGHT,
> +};
> +
> +static const struct opt300x_chip_info opt3002_chip_info = {
> +	.model = OPT3002,
> +	.chan_type = IIO_INTENSITY,
> +};

> +
>  static int opt3001_find_scale(const struct opt3001 *opt, int val,
>  		int val2, u8 *exponent)
>  {
>  	int i;
> +	const struct opt3001_scale (*scale_arr)[12];
>  
> -	for (i = 0; i < ARRAY_SIZE(opt3001_scales); i++) {
> -		const struct opt3001_scale *scale = &opt3001_scales[i];
> +	switch (opt->chip_info->model) {
> +	case OPT3001:
> +		scale_arr = &opt3001_scales;
Put them in chip_info directly, not look them up here.

> +		break;
> +	case OPT3002:
> +		scale_arr = &opt3002_scales;
> +		break;
> +	default:
> +		dev_err(opt->dev, "scale not configured for chip model\n");
> +		return -EINVAL;
> +	}
>  
> +	for (i = 0; i < ARRAY_SIZE(*scale_arr); i++) {
> +		const struct opt3001_scale *scale = &(*scale_arr)[i];
>  		/*
> -		 * Combine the integer and micro parts for comparison
> -		 * purposes. Use milli lux precision to avoid 32-bit integer
> -		 * overflows.
> +		 * Compare the integer and micro parts to determine value scale.
>  		 */
> -		if ((val * 1000 + val2 / 1000) <=
> -				(scale->val * 1000 + scale->val2 / 1000)) {
> +		if (val < scale->val ||
> +		    (val == scale->val && val2 <= scale->val2)) {
>  			*exponent = i;
>  			return 0;
>  		}
> @@ -174,11 +259,20 @@ static int opt3001_find_scale(const struct opt3001 *opt, int val,
>  static void opt3001_to_iio_ret(struct opt3001 *opt, u8 exponent,
>  		u16 mantissa, int *val, int *val2)
>  {
> -	int lux;
> +	int ret;
>  
> -	lux = 10 * (mantissa << exponent);
> -	*val = lux / 1000;
> -	*val2 = (lux - (*val * 1000)) * 1000;
> +	switch (opt->chip_info->model) {
> +	case OPT3001:
> +		ret = 10 * (mantissa << exponent);
> +		*val = ret / 1000;
> +		*val2 = (ret - (*val * 1000)) * 1000;
> +		break;
> +	case OPT3002:
> +		ret = 12 * (mantissa << exponent);
> +		*val = ret / 10;
> +		*val2 = (ret - (*val * 10)) * 100000;

As below - constants in the chip_info structure so this becomes
a simple case of using them without needing to know the chip type
in the code.

> +		break;
> +	}
>  }

> @@ -497,7 +602,15 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>  		goto err;
>  	}
>  
> -	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> +	switch (opt->chip_info->model) {
> +	case OPT3001:
> +		mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;

Encode the sections of this maths that is different as values in the chip
info structure and use them directly here rather than having a switch statement.

> +		break;
> +	case OPT3002:
> +		mantissa = (((val * 10) + (val2 / 100000)) / 12) >> exponent;
> +		break;
> +	}
> +
>  	value = (exponent << 12) | mantissa;
>  
>  	switch (dir) {
> @@ -607,15 +720,22 @@ static int opt3001_read_id(struct opt3001 *opt)
>  	manufacturer[0] = ret >> 8;
>  	manufacturer[1] = ret & 0xff;
>  
> -	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to read register %02x\n",
> +	switch (opt->chip_info->model) {

Add a callback for this to the chip_info structure. That will make it
much cleaner to add future devices.

> +	case OPT3001:
> +		ret = i2c_smbus_read_word_swapped(opt->client,
> +						  OPT3001_DEVICE_ID);
> +		if (ret == 0) {
> +			dev_err(opt->dev, "failed to read register %02x\n",
>  				OPT3001_DEVICE_ID);
> -		return ret;
> +			return ret;
> +		}
> +		device_id = ret;
> +		break;
> +	case OPT3002:
> +		device_id = OPT3002_DEVICE_ID_VALUE;
> +		break;

> @@ -755,6 +877,7 @@ static int opt3001_probe(struct i2c_client *client)
>  	opt = iio_priv(iio);
>  	opt->client = client;
>  	opt->dev = dev;
> +	opt->chip_info = device_get_match_data(&client->dev);
>  
>  	mutex_init(&opt->lock);
>  	init_waitqueue_head(&opt->result_ready_queue);
> @@ -769,10 +892,18 @@ static int opt3001_probe(struct i2c_client *client)
>  		return ret;
>  
>  	iio->name = client->name;
> -	iio->channels = opt3001_channels;
> -	iio->num_channels = ARRAY_SIZE(opt3001_channels);
>  	iio->modes = INDIO_DIRECT_MODE;
>  	iio->info = &opt3001_info;
> +	switch (opt->chip_info->model) {
> +	case OPT3001:
> +		iio->channels = opt3001_channels;
> +		iio->num_channels = ARRAY_SIZE(opt3001_channels);
Add this to the chip info structure so this can become a simple assignment
rather than having to look up by model.

> +		break;
> +	case OPT3002:
> +		iio->channels = opt3002_channels;
> +		iio->num_channels = ARRAY_SIZE(opt3002_channels);
> +		break;
> +	}
>  
>  	ret = devm_iio_device_register(dev, iio);
>  	if (ret) {
> @@ -826,13 +957,15 @@ static void opt3001_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id opt3001_id[] = {
> -	{ "opt3001" },
> +	{ "opt3001", 0 },
> +	{ "opt3002", 1 },
>  	{ } /* Terminating Entry */
>  };
>  MODULE_DEVICE_TABLE(i2c, opt3001_id);
>  
>  static const struct of_device_id opt3001_of_match[] = {
> -	{ .compatible = "ti,opt3001" },
> +	{ .compatible = "ti,opt3001", .data = &opt3001_chip_info },
> +	{ .compatible = "ti,opt3002", .data = &opt3002_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, opt3001_of_match);
>