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

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

[Emil Gedenryd]

TI's opt3002 light-to-digital sensor provides the functionality
of an optical power meter within a single device. It shares a lot of
similarities with their opt3001 model but has a wide spectral bandwidth,
ranging from 300 nm to 1000 nm.

This patch set adds support for the TI opt3002 by extending the opt3001
driver. In addition, a missing full-scale range value for the opt3001 is
added, resulting in higher precision when setting event trigger values.

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

Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
Changes in v3:
- Include difference between opt3001 and opt3002 in commit message for
  dt-binding patch.
- Remove whitespace between tags in commit message for opt3002 patch.
- Rename opt300x_chip_info to opt3001_chip_info.
- Add documentation for mathematical constants in opt3001_chip_info.
- Only check return value after opt3001_read_id if chip has id register
  in opt3001_probe.
- Change opt3002 channel mask to IIO_CHAN_INFO_RAW.
- Link to v2: https://lore.kernel.org/r/20240913-add_opt3002-v2-0-69e04f840360@axis.com

Changes in v2:
- Move dt-binding patch to before implementation.
- Fix dt-binding compatible definition.
- Clarify bug description for missing full-scale range value.
- Remove model enum from chip info and all in-function switch-case
  statements.
- Move model-specific channels and mathematic constants to chip info.
- Add valid match data to opt3001_id array
- Skip call to function opt3001_read_id() if model doesn't have a device
  id register.
- Link to v1: https://lore.kernel.org/r/20240905-add_opt3002-v1-0-a5ae21b924fb@axis.com

---
Emil Gedenryd (3):
      iio: light: opt3001: add missing full-scale range value
      dt-bindings: iio: light: opt3001: add compatible for opt3002
      iio: light: opt3001: add support for TI's opt3002 light sensor

 .../devicetree/bindings/iio/light/ti,opt3001.yaml  |   4 +-
 drivers/iio/light/Kconfig                          |   2 +-
 drivers/iio/light/opt3001.c                        | 190 +++++++++++++++++----
 3 files changed, 161 insertions(+), 35 deletions(-)
---
base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
change-id: 20240828-add_opt3002-40552c1a2f77

Best regards,
-- 
Emil Gedenryd <emil.gedenryd@axis.com>

[PATCH v3 1/3] iio: light: opt3001: add missing full-scale range value

[Emil Gedenryd]

The opt3001 driver uses predetermined full-scale range values to
determine what exponent to use for event trigger threshold values.
The problem is that one of the values specified in the datasheet is
missing from the implementation. This causes larger values to be
scaled down to an incorrect exponent, effectively reducing the
maximum settable threshold value by a factor of 2.

Add missing full-scale range array value.

Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor")
Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
 drivers/iio/light/opt3001.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 887c4b776a86..176e54bb48c3 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -138,6 +138,10 @@ static const struct opt3001_scale opt3001_scales[] = {
 		.val = 20966,
 		.val2 = 400000,
 	},
+	{
+		.val = 41932,
+		.val2 = 800000,
+	},
 	{
 		.val = 83865,
 		.val2 = 600000,

-- 
2.39.2

[PATCH v3 2/3] dt-bindings: iio: light: opt3001: add compatible for opt3002

[Emil Gedenryd]

The opt3002 is a Light-to-Digital Sensor by TI with support for wide-range
spectrum light. It shares most properties with their opt3001 model with
the exception of having a wide spectral bandwidth, ranging from 300 nm
to 1000 nm.

Add the compatible string of opt3002.

Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
 Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
index 441e9343fc97..67ca8d08256a 100644
--- a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
@@ -15,7 +15,9 @@ description: |
 
 properties:
   compatible:
-    const: ti,opt3001
+    enum:
+      - ti,opt3001
+      - ti,opt3002
 
   reg:
     maxItems: 1

-- 
2.39.2

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

[Emil Gedenryd]

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 | 186 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 154 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..5e3fe42c5b59 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -70,6 +70,34 @@
 #define OPT3001_RESULT_READY_SHORT	150
 #define OPT3001_RESULT_READY_LONG	1000
 
+struct opt3001_scale {
+	int	val;
+	int	val2;
+};
+
+struct opt3001_chip_info {
+	const struct iio_chan_spec (*channels)[2];
+	enum iio_chan_type chan_type;
+	const struct opt3001_scale (*scales)[12];
+
+	/*
+	 * Factor as specified by conversion equation in datasheet.
+	 * eg. 0.01 (scaled to integer 10) for opt3001.
+	 */
+	int factor_whole;
+	/*
+	 * Factor to compensate for potentially scaled factor_whole.
+	 */
+	int factor_integer;
+	/*
+	 * Factor used to align decimal part of proccessed value to six decimal
+	 * places.
+	 */
+	int factor_decimal;
+
+	bool has_id;
+};
+
 struct opt3001 {
 	struct i2c_client	*client;
 	struct device		*dev;
@@ -79,6 +107,7 @@ struct opt3001 {
 	bool			result_ready;
 	wait_queue_head_t	result_ready_queue;
 	u16			result;
+	const struct opt3001_chip_info *chip_info;
 
 	u32			int_time;
 	u32			mode;
@@ -92,11 +121,6 @@ struct opt3001 {
 	bool			use_irq;
 };
 
-struct opt3001_scale {
-	int	val;
-	int	val2;
-};
-
 static const struct opt3001_scale opt3001_scales[] = {
 	{
 		.val = 40,
@@ -148,21 +172,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 +245,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 +299,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_RAW) |
+				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 +482,15 @@ 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_RAW:
 	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 +514,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 +565,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 +586,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 +704,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 +786,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 +802,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));
@@ -755,22 +850,25 @@ 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 (ret)
-		return ret;
+	if (opt->chip_info->has_id) {
+		ret = opt3001_read_id(opt);
+		if (ret)
+			return ret;
+	}
 
 	ret = opt3001_configure(opt);
 	if (ret)
 		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 +923,36 @@ static void opt3001_remove(struct i2c_client *client)
 	}
 }
 
+static const struct opt3001_chip_info opt3001_chip_information = {
+	.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 opt3001_chip_info opt3002_chip_information = {
+	.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_information },
+	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
 	{ } /* 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_information },
+	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, opt3001_of_match);

-- 
2.39.2

Re: [PATCH v3 2/3] dt-bindings: iio: light: opt3001: add compatible for opt3002

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Mon, Sep 16, 2024 at 04:56:38PM +0200, Emil Gedenryd wrote:
> The opt3002 is a Light-to-Digital Sensor by TI with support for wide-range
> spectrum light. It shares most properties with their opt3001 model with
> the exception of having a wide spectral bandwidth, ranging from 300 nm
> to 1000 nm.
> 
> Add the compatible string of opt3002.
> 
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>

Pretty sure I gave you an ack for this already, provided you added the
a description of the differences, which you have.

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/3] dt-bindings: iio: light: opt3001: add compatible for opt3002

[Emil Gedenryd]

On Mon, 2024-09-16 at 16:24 +0100, Conor Dooley wrote:
> On Mon, Sep 16, 2024 at 04:56:38PM +0200, Emil Gedenryd wrote:
> > The opt3002 is a Light-to-Digital Sensor by TI with support for wide-range
> > spectrum light. It shares most properties with their opt3001 model with
> > the exception of having a wide spectral bandwidth, ranging from 300 nm
> > to 1000 nm.
> > 
> > Add the compatible string of opt3002.
> > 
> > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> 
> Pretty sure I gave you an ack for this already, provided you added the
> a description of the differences, which you have.
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Cheers,
> Conor.

Thank you!

Regards,
Emil 

Re: [PATCH v3 1/3] iio: light: opt3001: add missing full-scale range value

[Jonathan Cameron]

On Mon, 16 Sep 2024 16:56:37 +0200
Emil Gedenryd <emil.gedenryd@axis.com> wrote:

> The opt3001 driver uses predetermined full-scale range values to
> determine what exponent to use for event trigger threshold values.
> The problem is that one of the values specified in the datasheet is
> missing from the implementation. This causes larger values to be
> scaled down to an incorrect exponent, effectively reducing the
> maximum settable threshold value by a factor of 2.
> 
> Add missing full-scale range array value.
> 
> Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor")
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
This one is already queued up, so you can drop it from future
versions of this series posted for review.

Thanks,

Jonathan

> ---
>  drivers/iio/light/opt3001.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 887c4b776a86..176e54bb48c3 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -138,6 +138,10 @@ static const struct opt3001_scale opt3001_scales[] = {
>  		.val = 20966,
>  		.val2 = 400000,
>  	},
> +	{
> +		.val = 41932,
> +		.val2 = 800000,
> +	},
>  	{
>  		.val = 83865,
>  		.val2 = 600000,
> 

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

[Jonathan Cameron]

On Mon, 16 Sep 2024 16:56:39 +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
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
Hi Emil,

A few things inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 176e54bb48c3..5e3fe42c5b59 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c


> @@ -479,6 +565,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 +586,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 +704,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);

Unrelated change so in theory should be in a separate patch but
meh, it's trivial so leave it here if you like.

>  		return ret;
>  	}

> @@ -755,22 +850,25 @@ 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);

Use the i2c specific way to to do this.
https://elixir.bootlin.com/linux/v6.11/source/drivers/i2c/i2c-core-base.c#L120
i2c_get_match_data() because it will also handle falling back to matching
via the i2c_match_id() path against the old style match tables in a few
cases.

>  
>  	mutex_init(&opt->lock);
>  	init_waitqueue_head(&opt->result_ready_queue);
>  	i2c_set_clientdata(client, iio);
>  
> -	ret = opt3001_read_id(opt);
> -	if (ret)
> -		return ret;
> +	if (opt->chip_info->has_id) {
> +		ret = opt3001_read_id(opt);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = opt3001_configure(opt);
>  	if (ret)
>  		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);
This won't work as it has no way to perform a sizeof
through a pointer.

Add a num_channels filed to your opt3001_chip_info structure
as then it can be ARRAY_SIZE(&opt3001_channels) which can work.

>  	iio->modes = INDIO_DIRECT_MODE;
>  	iio->info = &opt3001_info;
>  
> @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client)
>  	}
>  }
>  
> +static const struct opt3001_chip_info opt3001_chip_information = {
> +	.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 opt3001_chip_info opt3002_chip_information = {
> +	.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_information },
> +	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
>  	{ } /* 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_information },
> +	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, opt3001_of_match);
> 

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

[Emil Gedenryd]

On Sat, 2024-09-28 at 17:05 +0100, Jonathan Cameron wrote:
> On Mon, 16 Sep 2024 16:56:39 +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
> > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> Hi Emil,
> 
> A few things inline,
> 
> Thanks,
> 
> Jonathan

Hi Jonathan,

Thank you for the comments, I'll start working on fixing them during
the week.

Best Regards,
Emil
> 
> > 
> > @@ -610,7 +704,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);
> 
> Unrelated change so in theory should be in a separate patch but
> meh, it's trivial so leave it here if you like.

Good to know! I'll keep it since it's minor but I'll remember this for
the future.

> 
> >  		return ret;
> >  	}
> 
> > @@ -755,22 +850,25 @@ 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);
> 
> Use the i2c specific way to to do this.
> https://elixir.bootlin.com/linux/v6.11/source/drivers/i2c/i2c-core-base.c#L120
> i2c_get_match_data() because it will also handle falling back to matching
> via the i2c_match_id() path against the old style match tables in a few
> cases.

Okay!

> >  
> >  	mutex_init(&opt->lock);
> >  	init_waitqueue_head(&opt->result_ready_queue);
> >  	i2c_set_clientdata(client, iio);
> >  
> > -	ret = opt3001_read_id(opt);
> > -	if (ret)
> > -		return ret;
> > +	if (opt->chip_info->has_id) {
> > +		ret = opt3001_read_id(opt);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	ret = opt3001_configure(opt);
> >  	if (ret)
> >  		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);
> This won't work as it has no way to perform a sizeof
> through a pointer.
> 
> Add a num_channels filed to your opt3001_chip_info structure
> as then it can be ARRAY_SIZE(&opt3001_channels) which can work.

Good catch and thanks for the suggestion, I'll do it that way!

> 
> >  	iio->modes = INDIO_DIRECT_MODE;
> >  	iio->info = &opt3001_info;
> >  
> > @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client)
> >  	}
> >  }
> >  
> > +static const struct opt3001_chip_info opt3001_chip_information = {
> > +	.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 opt3001_chip_info opt3002_chip_information = {
> > +	.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_information },
> > +	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
> >  	{ } /* 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_information },
> > +	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, opt3001_of_match);
> > 
> 

Re: [PATCH v3 1/3] iio: light: opt3001: add missing full-scale range value

[Emil Gedenryd]

On Sat, 2024-09-28 at 16:57 +0100, Jonathan Cameron wrote:
> On Mon, 16 Sep 2024 16:56:37 +0200
> Emil Gedenryd <emil.gedenryd@axis.com> wrote:
> 
> > The opt3001 driver uses predetermined full-scale range values to
> > determine what exponent to use for event trigger threshold values.
> > The problem is that one of the values specified in the datasheet is
> > missing from the implementation. This causes larger values to be
> > scaled down to an incorrect exponent, effectively reducing the
> > maximum settable threshold value by a factor of 2.
> > 
> > Add missing full-scale range array value.
> > 
> > Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor")
> > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> This one is already queued up, so you can drop it from future
> versions of this series posted for review.
> 
> Thanks,
> 
> Jonathan

Okay, I'll leave it out in the future.

Thanks,
Emil