diff mbox series

[v2,3/3] iio: light: opt3001: add support for TI's opt3002 light sensor

Message ID 20240913-add_opt3002-v2-3-69e04f840360@axis.com (mailing list archive)
State Changes Requested
Headers show
Series iio: light: opt3001: add support for TI's opt3002 light sensor | expand

Commit Message

Emil Gedenryd Sept. 13, 2024, 9:57 a.m. UTC 
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.

Datasheet: https://www.ti.com/product/OPT3002

Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
 drivers/iio/light/Kconfig   |   2 +-
 drivers/iio/light/opt3001.c | 169 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 138 insertions(+), 33 deletions(-)

Comments

Jonathan Cameron Sept. 14, 2024, 4:19 p.m. UTC | #1 
On Fri, 13 Sep 2024 11:57:04 +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.
> 
> Datasheet: https://www.ti.com/product/OPT3002
> 
No blank line here. Datasheet: should be part of this tags block.
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> ---
>  drivers/iio/light/Kconfig   |   2 +-
>  drivers/iio/light/opt3001.c | 169 +++++++++++++++++++++++++++++++++++---------
>  2 files changed, 138 insertions(+), 33 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..83c49f4517b7 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -70,6 +70,21 @@
>  #define OPT3001_RESULT_READY_SHORT	150
>  #define OPT3001_RESULT_READY_LONG	1000
>  
> +struct opt3001_scale {
> +	int	val;
> +	int	val2;
> +};
> +
> +struct opt300x_chip_info {

Don't use wild cards.  Just call it opt3001_chip_info.
Wild cards tend to bite us as manufacturers have an 'amusing' habit
of filling in gaps with unrelated devices.


> +	const struct iio_chan_spec (*channels)[2];
> +	enum iio_chan_type chan_type;
> +	const struct opt3001_scale (*scales)[12];
> +	int factor_whole;
> +	int factor_integer;
> +	int factor_decimal;

These three aren't obvious when just looking to fill in this
structure. Add some docs to hint at what they are.

> +	bool has_id;
> +};

> @@ -610,7 +690,7 @@ static int opt3001_read_id(struct opt3001 *opt)
>  	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);
> +			OPT3001_DEVICE_ID);

In general whitespace only changes belong in their own patch, but I guess
this one is pretty minor so we can skip that requirement this time.

>  		return ret;
>  	}

> @@ -746,7 +827,7 @@ static int opt3001_probe(struct i2c_client *client)
>  	struct iio_dev *iio;
>  	struct opt3001 *opt;
>  	int irq = client->irq;
> -	int ret;
> +	int ret = 0;
>  
>  	iio = devm_iio_device_alloc(dev, sizeof(*opt));
>  	if (!iio)
> @@ -755,12 +836,14 @@ 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);
>  	i2c_set_clientdata(client, iio);
>  
> -	ret = opt3001_read_id(opt);
> +	if (opt->chip_info->has_id)
> +		ret = opt3001_read_id(opt);
>  	if (ret)
>  		return ret;
>  
Only check the ret if the function ran.  That way no need for the
dance with ret = 0 above.


> +static const struct iio_chan_spec opt3002_channels[] = {
> +	{
> +		.type = IIO_INTENSITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				BIT(IIO_CHAN_INFO_INT_TIME),

Generally intensity channels can't be processed because there are no
defined units as what you measure depends entirely on the frequency
sensitivity. There are some defined measurements such as illuminance
that use a specific sensivitiy curve, but if it's just intensity we
normally stick to _RAW..

> +		.event_spec = opt3001_event_spec,
> +		.num_event_specs = ARRAY_SIZE(opt3001_event_spec),
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
Emil Gedenryd Sept. 16, 2024, 7:17 a.m. UTC | #2 
On Sat, 2024-09-14 at 17:19 +0100, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 11:57:04 +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.
> > 
> > Datasheet: https://www.ti.com/product/OPT3002
> > 
> No blank line here. Datasheet: should be part of this tags block.

Okay, I'll remove it in the next version.

> > 
> > +
> > +struct opt300x_chip_info {
> 
> Don't use wild cards.  Just call it opt3001_chip_info.
> Wild cards tend to bite us as manufacturers have an 'amusing' habit
> of filling in gaps with unrelated devices.

Good point!

> 
> 
> > +	const struct iio_chan_spec (*channels)[2];
> > +	enum iio_chan_type chan_type;
> > +	const struct opt3001_scale (*scales)[12];
> > +	int factor_whole;
> > +	int factor_integer;
> > +	int factor_decimal;
> 
> These three aren't obvious when just looking to fill in this
> structure. Add some docs to hint at what they are.

Okay!


> > +	bool has_id;
> > +};
> 
> > @@ -610,7 +690,7 @@ static int opt3001_read_id(struct opt3001 *opt)
> >  	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);
> > +			OPT3001_DEVICE_ID);
> 
> In general whitespace only changes belong in their own patch, but I guess
> this one is pretty minor so we can skip that requirement this time.

Thank you for the info, I'll keep that in mind in the future.

> 
> > @@ -755,12 +836,14 @@ 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);
> >  	i2c_set_clientdata(client, iio);
> >  
> > -	ret = opt3001_read_id(opt);
> > +	if (opt->chip_info->has_id)
> > +		ret = opt3001_read_id(opt);
> >  	if (ret)
> >  		return ret;
> >  
> Only check the ret if the function ran.  That way no need for the
> dance with ret = 0 above.

Good point, I'll change that.

> 
> 
> > +static const struct iio_chan_spec opt3002_channels[] = {
> > +	{
> > +		.type = IIO_INTENSITY,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > +				BIT(IIO_CHAN_INFO_INT_TIME),
> 
> Generally intensity channels can't be processed because there are no
> defined units as what you measure depends entirely on the frequency
> sensitivity. There are some defined measurements such as illuminance
> that use a specific sensivitiy curve, but if it's just intensity we
> normally stick to _RAW..

Okay, I'll switch to the raw type instead.

Thank you for the feedback, I'll start working on a new version
as soon as possible.

Best Regards,
Emil

> > +		.event_spec = opt3001_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(opt3001_event_spec),
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(1),
> > +};
> > +
diff mbox series

Patch

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..83c49f4517b7 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -70,6 +70,21 @@ 
 #define OPT3001_RESULT_READY_SHORT	150
 #define OPT3001_RESULT_READY_LONG	1000
 
+struct opt3001_scale {
+	int	val;
+	int	val2;
+};
+
+struct opt300x_chip_info {
+	const struct iio_chan_spec (*channels)[2];
+	enum iio_chan_type chan_type;
+	const struct opt3001_scale (*scales)[12];
+	int factor_whole;
+	int factor_integer;
+	int factor_decimal;
+	bool has_id;
+};
+
 struct opt3001 {
 	struct i2c_client	*client;
 	struct device		*dev;
@@ -79,6 +94,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;
@@ -92,11 +108,6 @@  struct opt3001 {
 	bool			use_irq;
 };
 
-struct opt3001_scale {
-	int	val;
-	int	val2;
-};
-
 static const struct opt3001_scale opt3001_scales[] = {
 	{
 		.val = 40,
@@ -148,21 +159,68 @@  static const struct opt3001_scale opt3001_scales[] = {
 	},
 };
 
+static const struct opt3001_scale opt3002_scales[] = {
+	{
+		.val = 4914,
+		.val2 = 0,
+	},
+	{
+		.val = 9828,
+		.val2 = 0,
+	},
+	{
+		.val = 19656,
+		.val2 = 0,
+	},
+	{
+		.val = 39312,
+		.val2 = 0,
+	},
+	{
+		.val = 78624,
+		.val2 = 0,
+	},
+	{
+		.val = 157248,
+		.val2 = 0,
+	},
+	{
+		.val = 314496,
+		.val2 = 0,
+	},
+	{
+		.val = 628992,
+		.val2 = 0,
+	},
+	{
+		.val = 1257984,
+		.val2 = 0,
+	},
+	{
+		.val = 2515968,
+		.val2 = 0,
+	},
+	{
+		.val = 5031936,
+		.val2 = 0,
+	},
+	{
+		.val = 10063872,
+		.val2 = 0,
+	},
+};
+
 static int opt3001_find_scale(const struct opt3001 *opt, int val,
 		int val2, u8 *exponent)
 {
 	int i;
-
-	for (i = 0; i < ARRAY_SIZE(opt3001_scales); i++) {
-		const struct opt3001_scale *scale = &opt3001_scales[i];
-
+	for (i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {
+		const struct opt3001_scale *scale = &(*opt->chip_info->scales)[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 +232,14 @@  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;
+	int whole = opt->chip_info->factor_whole;
+	int integer = opt->chip_info->factor_integer;
+	int decimal = opt->chip_info->factor_decimal;
 
-	lux = 10 * (mantissa << exponent);
-	*val = lux / 1000;
-	*val2 = (lux - (*val * 1000)) * 1000;
+	ret = whole * (mantissa << exponent);
+	*val = ret / integer;
+	*val2 = (ret - (*val * integer)) * decimal;
 }
 
 static void opt3001_set_mode(struct opt3001 *opt, u16 *reg, u16 mode)
@@ -225,7 +286,18 @@  static const struct iio_chan_spec opt3001_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(1),
 };
 
-static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
+static const struct iio_chan_spec opt3002_channels[] = {
+	{
+		.type = IIO_INTENSITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				BIT(IIO_CHAN_INFO_INT_TIME),
+		.event_spec = opt3001_event_spec,
+		.num_event_specs = ARRAY_SIZE(opt3001_event_spec),
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 {
 	int ret;
 	u16 mantissa;
@@ -397,14 +469,14 @@  static int opt3001_read_raw(struct iio_dev *iio,
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
 
-	if (chan->type != IIO_LIGHT)
+	if (chan->type != opt->chip_info->chan_type)
 		return -EINVAL;
 
 	mutex_lock(&opt->lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
-		ret = opt3001_get_lux(opt, val, val2);
+		ret = opt3001_get_processed(opt, val, val2);
 		break;
 	case IIO_CHAN_INFO_INT_TIME:
 		ret = opt3001_get_int_time(opt, val, val2);
@@ -428,7 +500,7 @@  static int opt3001_write_raw(struct iio_dev *iio,
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
 
-	if (chan->type != IIO_LIGHT)
+	if (chan->type != opt->chip_info->chan_type)
 		return -EINVAL;
 
 	if (mask != IIO_CHAN_INFO_INT_TIME)
@@ -479,6 +551,9 @@  static int opt3001_write_event_value(struct iio_dev *iio,
 {
 	struct opt3001 *opt = iio_priv(iio);
 	int ret;
+	int whole;
+	int integer;
+	int decimal;
 
 	u16 mantissa;
 	u16 value;
@@ -497,7 +572,12 @@  static int opt3001_write_event_value(struct iio_dev *iio,
 		goto err;
 	}
 
-	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
+	whole = opt->chip_info->factor_whole;
+	integer = opt->chip_info->factor_integer;
+	decimal = opt->chip_info->factor_decimal;
+
+	mantissa = (((val * integer) + (val2 / decimal)) / whole) >> exponent;
+
 	value = (exponent << 12) | mantissa;
 
 	switch (dir) {
@@ -610,7 +690,7 @@  static int opt3001_read_id(struct opt3001 *opt)
 	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);
+			OPT3001_DEVICE_ID);
 		return ret;
 	}
 
@@ -692,6 +772,7 @@  static irqreturn_t opt3001_irq(int irq, void *_iio)
 	struct opt3001 *opt = iio_priv(iio);
 	int ret;
 	bool wake_result_ready_queue = false;
+	enum iio_chan_type chan_type = opt->chip_info->chan_type;
 
 	if (!opt->ok_to_ignore_lock)
 		mutex_lock(&opt->lock);
@@ -707,13 +788,13 @@  static irqreturn_t opt3001_irq(int irq, void *_iio)
 			OPT3001_CONFIGURATION_M_CONTINUOUS) {
 		if (ret & OPT3001_CONFIGURATION_FH)
 			iio_push_event(iio,
-					IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+					IIO_UNMOD_EVENT_CODE(chan_type, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_RISING),
 					iio_get_time_ns(iio));
 		if (ret & OPT3001_CONFIGURATION_FL)
 			iio_push_event(iio,
-					IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+					IIO_UNMOD_EVENT_CODE(chan_type, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_FALLING),
 					iio_get_time_ns(iio));
@@ -746,7 +827,7 @@  static int opt3001_probe(struct i2c_client *client)
 	struct iio_dev *iio;
 	struct opt3001 *opt;
 	int irq = client->irq;
-	int ret;
+	int ret = 0;
 
 	iio = devm_iio_device_alloc(dev, sizeof(*opt));
 	if (!iio)
@@ -755,12 +836,14 @@  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);
 	i2c_set_clientdata(client, iio);
 
-	ret = opt3001_read_id(opt);
+	if (opt->chip_info->has_id)
+		ret = opt3001_read_id(opt);
 	if (ret)
 		return ret;
 
@@ -769,8 +852,8 @@  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->channels = *opt->chip_info->channels;
+	iio->num_channels = ARRAY_SIZE(*opt->chip_info->channels);
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &opt3001_info;
 
@@ -825,14 +908,36 @@  static void opt3001_remove(struct i2c_client *client)
 	}
 }
 
+static const struct opt300x_chip_info opt3001_chip_info = {
+	.channels = &opt3001_channels,
+	.chan_type = IIO_LIGHT,
+	.scales = &opt3001_scales,
+	.factor_whole = 10,
+	.factor_integer = 1000,
+	.factor_decimal = 1000,
+	.has_id = true,
+};
+
+static const struct opt300x_chip_info opt3002_chip_info = {
+	.channels = &opt3002_channels,
+	.chan_type = IIO_INTENSITY,
+	.scales = &opt3002_scales,
+	.factor_whole = 12,
+	.factor_integer = 10,
+	.factor_decimal = 100000,
+	.has_id = false,
+};
+
 static const struct i2c_device_id opt3001_id[] = {
-	{ "opt3001" },
+	{ "opt3001", (kernel_ulong_t)&opt3001_chip_info },
+	{ "opt3002", (kernel_ulong_t)&opt3002_chip_info },
 	{ } /* 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);