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

[PATCH 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.

See http://www.ti.com/product/OPT3002 for more information.

Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
Emil Gedenryd (3):
      iio: light: opt3001: add missing full-scale range value
      iio: light: opt3001: add support for TI's opt3002 light sensor
      dt-bindings: iio: light: opt3001: add compatible for opt3002

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

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

[PATCH 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, causing a big gap in settable values.

Add missing full-scale range array value.

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 2/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.

See https://www.ti.com/product/OPT3002 for more information.

Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
---
 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
+
+enum chip_model {
+	OPT3001,
+	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,
+	.chan_type = IIO_LIGHT,
+};
+
+static const struct opt300x_chip_info opt3002_chip_info = {
+	.model = OPT3002,
+	.chan_type = IIO_INTENSITY,
+};
+
 static const struct opt3001_scale opt3001_scales[] = {
 	{
 		.val = 40,
@@ -148,21 +172,82 @@ 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;
+	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;
+		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;
+		break;
+	}
 }
 
 static void opt3001_set_mode(struct opt3001 *opt, u16 *reg, u16 mode)
@@ -216,7 +310,18 @@ static const struct iio_event_spec opt3001_event_spec[] = {
 
 static const struct iio_chan_spec opt3001_channels[] = {
 	{
-		.type = IIO_LIGHT,
+		.type = opt3001_chip_info.chan_type,
+		.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 const struct iio_chan_spec opt3002_channels[] = {
+	{
+		.type = opt3002_chip_info.chan_type,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				BIT(IIO_CHAN_INFO_INT_TIME),
 		.event_spec = opt3001_event_spec,
@@ -225,7 +330,7 @@ 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 int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 {
 	int ret;
 	u16 mantissa;
@@ -397,14 +502,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 +533,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)
@@ -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;
+		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) {
+	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;
 	}
 
-	device_id = ret;
-
 	dev_info(opt->dev, "Found %c%c OPT%04x\n", manufacturer[0],
 			manufacturer[1], device_id);
 
@@ -707,15 +827,17 @@ 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_EV_TYPE_THRESH,
-							IIO_EV_DIR_RISING),
+					IIO_UNMOD_EVENT_CODE(
+						      opt->chip_info->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_EV_TYPE_THRESH,
-							IIO_EV_DIR_FALLING),
+					IIO_UNMOD_EVENT_CODE(
+						      opt->chip_info->chan_type,
+						      0, IIO_EV_TYPE_THRESH,
+						      IIO_EV_DIR_FALLING),
 					iio_get_time_ns(iio));
 	} else if (ret & OPT3001_CONFIGURATION_CRF) {
 		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
@@ -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);
+		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);

-- 
2.39.2

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

[Emil Gedenryd]

OPT3002 is a Light-to-Digital Sensor by TI with support for wide-range
spectrum light.
Add the compatible string of opt3002 to the existing list.

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

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

-- 
2.39.2

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

[Krzysztof Kozlowski]

On 05/09/2024 12:20, Emil Gedenryd 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.
> 

...

>  
>  	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);
> +		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 },

Use the same match data for all ID tables. Otherwise you run into
problems for different match methods.

>  	{ } /* 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 },
Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 05/09/2024 12:20, Emil Gedenryd wrote:
> OPT3002 is a Light-to-Digital Sensor by TI with support for wide-range
> spectrum light.
> Add the compatible string of opt3002 to the existing list.
> 
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> ---
>  Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
> index 441e9343fc97..7a48a06968ca 100644
> --- a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
> @@ -16,6 +16,7 @@ description: |
>  properties:
>    compatible:
>      const: ti,opt3001
> +    const: ti,opt3002

Never tested and obviously broken.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 05/09/2024 12:20, Emil Gedenryd wrote:
> 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.
> 
> See http://www.ti.com/product/OPT3002 for more information.
> 
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> ---
> Emil Gedenryd (3):
>       iio: light: opt3001: add missing full-scale range value
>       iio: light: opt3001: add support for TI's opt3002 light sensor
>       dt-bindings: iio: light: opt3001: add compatible for opt3002

Bindings are before their users.

Best regards,
Krzysztof

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

[Emil Gedenryd]

On Thu, 2024-09-05 at 12:38 +0200, Krzysztof Kozlowski wrote:
> On 05/09/2024 12:20, Emil Gedenryd wrote:
> > 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.
> > 
> > See http://www.ti.com/product/OPT3002 for more information.
> > 
> > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> > ---
> > Emil Gedenryd (3):
> >       iio: light: opt3001: add missing full-scale range value
> >       iio: light: opt3001: add support for TI's opt3002 light sensor
> >       dt-bindings: iio: light: opt3001: add compatible for opt3002
> 
> Bindings are before their users.
> 
> Best regards,
> Krzysztof
> 
Thank you for taking a look at the patch set.
I'll submit a new version that fixes the issues you highlighted once more 
people have had time to review the changes.

Best regards,
Emil

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

[Rob Herring (Arm)]

On Thu, 05 Sep 2024 12:20:47 +0200, Emil Gedenryd wrote:
> OPT3002 is a Light-to-Digital Sensor by TI with support for wide-range
> spectrum light.
> Add the compatible string of opt3002 to the existing list.
> 
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> ---
>  Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml:19:5: [error] duplication of key "const" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml:19:5: found duplicate key "const" with value "ti,opt3002" (original value: "ti,opt3001")
make[2]: *** Deleting file 'Documentation/devicetree/bindings/iio/light/ti,opt3001.example.dts'
Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml:19:5: found duplicate key "const" with value "ti,opt3002" (original value: "ti,opt3001")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/iio/light/ti,opt3001.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1432: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240905-add_opt3002-v1-3-a5ae21b924fb@axis.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[kernel test robot]

Hi Emil,

kernel test robot noticed the following build errors:

[auto build test ERROR on 5be63fc19fcaa4c236b307420483578a56986a37]

url:    https://github.com/intel-lab-lkp/linux/commits/Emil-Gedenryd/iio-light-opt3001-add-missing-full-scale-range-value/20240905-182748
base:   5be63fc19fcaa4c236b307420483578a56986a37
patch link:    https://lore.kernel.org/r/20240905-add_opt3002-v1-2-a5ae21b924fb%40axis.com
patch subject: [PATCH 2/3] iio: light: opt3001: add support for TI's opt3002 light sensor
config: arm-randconfig-004-20240907 (https://download.01.org/0day-ci/archive/20240907/202409070539.HDm71PcK-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409070539.HDm71PcK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409070539.HDm71PcK-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/light/opt3001.c:313:29: error: initializer element is not a compile-time constant
                   .type = opt3001_chip_info.chan_type,
                           ~~~~~~~~~~~~~~~~~~^~~~~~~~~
   drivers/iio/light/opt3001.c:324:29: error: initializer element is not a compile-time constant
                   .type = opt3002_chip_info.chan_type,
                           ~~~~~~~~~~~~~~~~~~^~~~~~~~~
   2 errors generated.


vim +313 drivers/iio/light/opt3001.c

   310	
   311	static const struct iio_chan_spec opt3001_channels[] = {
   312		{
 > 313			.type = opt3001_chip_info.chan_type,
   314			.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
   315					BIT(IIO_CHAN_INFO_INT_TIME),
   316			.event_spec = opt3001_event_spec,
   317			.num_event_specs = ARRAY_SIZE(opt3001_event_spec),
   318		},
   319		IIO_CHAN_SOFT_TIMESTAMP(1),
   320	};
   321	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jonathan Cameron]

On Thu, 5 Sep 2024 12:20:45 +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, causing a big gap in settable values.
> 
> Add missing full-scale range array value.
> 
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
I assume this has more of an impact in that if you write values beyond this one
the index will be off by one and the value written to the register
will be incorrect?

Please clarify if that is the case and also add a fixes tag
to the commit that introduced this bug.

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 2/3] iio: light: opt3001: add support for TI's opt3002 light sensor

[Jonathan Cameron]

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);
> 

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

[Emil Gedenryd]

On Sat, 2024-09-07 at 18:28 +0100, Jonathan Cameron wrote:
> On Thu, 5 Sep 2024 12:20:45 +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, causing a big gap in settable values.
> > 
> > Add missing full-scale range array value.
> > 
> > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
> I assume this has more of an impact in that if you write values beyond this one
> the index will be off by one and the value written to the register
> will be incorrect?
> 
> Please clarify if that is the case and also add a fixes tag
> to the commit that introduced this bug.
> 
> Thanks,
> 
> Jonathan
Hi Jonathan,
You are correct regarding the behaviour. Thanks for the suggestion 
on how to clarify the message, I'll update it as well as add a fixes 
tag when I submit a new version either later today or tomorrow.

Best regards,
Emil
> 
> > ---
> >  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 2/3] iio: light: opt3001: add support for TI's opt3002 light sensor

[Emil Gedenryd]

On Sat, 2024-09-07 at 18:35 +0100, Jonathan Cameron wrote:
> 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

Thank you for having a look at the patch set.
I'll submit a new version that fixes the issues you highlighted
in the code and commit messages either later today or tomorrow.

Best regards,
Emil
> 
> > ---
> >  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.

The device id is used to log the model. Should I not log the
model for the opt3002 then or should I have the callback just return
3002? I thought it would be cleaner to have the id value as a defined
constant instead of a "magic" number in the code. Is there a preferred
way of doing 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.

Good point!
I'll move as much model-specific code as possible to the struct.

> 
> > +	.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);
> > 
> 

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

[Jonathan Cameron]

Hi Emil,

> > > 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.  
> 
> The device id is used to log the model. Should I not log the
> model for the opt3002 then or should I have the callback just return
> 3002? I thought it would be cleaner to have the id value as a defined
> constant instead of a "magic" number in the code. Is there a preferred
> way of doing it?

Given the lack of register means you can't check the model, don't
report one at all. So don't print that message for this
device.

For future replies crop out anything that doesn't need a reply.
Saves a reader having to scroll and potentially miss something
important!

Thanks,

Jonathan

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

[Emil Gedenryd]

On Mon, 2024-09-09 at 20:13 +0100, Jonathan Cameron wrote:
> Hi Emil,
> 
> > > > 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.  
> > 
> > The device id is used to log the model. Should I not log the
> > model for the opt3002 then or should I have the callback just return
> > 3002? I thought it would be cleaner to have the id value as a defined
> > constant instead of a "magic" number in the code. Is there a preferred
> > way of doing it?
> 
> Given the lack of register means you can't check the model, don't
> report one at all. So don't print that message for this
> device.
> 
> For future replies crop out anything that doesn't need a reply.
> Saves a reader having to scroll and potentially miss something
> important!
> 
> Thanks,
> 
> Jonathan

Hi Jonathan,
Sorry for the delay and thank you for the suggestion. I'll do
the finishing touches on version 2 and submit it as soon as possible.

Best regards,
Emil