Support for Texas Instruments OPT4060 RGBW Color sensor.

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

drivers/iio/light/opt4060.c new file +0 -0

new file mode 100644
index 000000000000..2c3761ec423a
@@ -0,0 +1,1259 @@

drivers/iio/light/opt4060.c new file +24 -0

+		.mul = 516,
+		.div = 100000,
+	}, {
+		/* GREEN 1.0 * 2.15e-3 */
+		.mul = 215,
+		.div = 100000,
+	}, {
+		/* BLUE 1.3 * 2.15e-3 */
+		.mul = 258,
+		.div = 100000,
+	}, {
+		/* CLEAR 1.0 * 2.15e-3 */
+		.mul = 215,
+		.div = 100000,
+	},
+};
 
+
+static int opt4060_trigger_one_shot(struct opt4060_chip *chip)
+{
+	int ret;
+	unsigned int ctrl_reg, oper_mode;
+
+	/* Enable interrupt for conversion ready of all channels */
+	ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);

drivers/iio/light/opt4060.c new file +14 -0

+
+	/* If the device is already in continuous mode, one-shot is not needed. */
+	if (oper_mode != OPT4060_CTRL_OPER_MODE_CONTINUOUS) {
+		ctrl_reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
+		ctrl_reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				       OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+
+		/* Trigger a new conversion by writing to CRTL register. */
+		ret = regmap_write(chip->regmap, OPT4060_CTRL, ctrl_reg);
+		if (ret)
+			dev_err(chip->dev, "Failed to set ctrl register\n");
+	}
+	return ret;
+}

drivers/iio/light/opt4060.c new file +16 -0

+}
+
+static int opt4060_read_chan_value(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2, bool processed)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	u32 adc_raw;
+	int ret;
+	/*
+	 * The conversion time should be 500us startup time plus the integration time
+	 * times the number of channels. An exact timeout isn't critical, it's better
+	 * not to get incorrect errors in the log. Setting the timeout to double the
+	 * theoretical time plus and extra 100ms margin.
+	 */
+	unsigned int timeout_us = (500 + OPT4060_NUM_CHANS *

drivers/iio/light/opt4060.c new file +15 -0

+	} else {
+		unsigned int ready;
+
+		ret = opt4060_trigger_one_shot(chip);
+		if (ret)
+			return ret;
+
+		ret = regmap_read_poll_timeout(chip->regmap, OPT4060_RES_CTRL,
+					       ready, (ready & OPT4060_RES_CTRL_CONV_READY),
+					       1000, timeout_us);
+		if (ret)
+			dev_err(chip->dev, "Conversion ready did not finish within timeout.\n");
+	}
+
+	ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);

drivers/iio/light/opt4060.c new file +6 -0

+		u32 lux_raw, rem;
+		u32 mul = opt4060_channel_factors[chan->scan_index].mul;
+		u32 div = opt4060_channel_factors[chan->scan_index].div;
+
+		lux_raw = adc_raw * mul;
+		*val = div_u64_rem(lux_raw, div, &rem);

drivers/iio/light/opt4060.c new file +21 -0

+
+	if (trig == chip->data_trig) {
+		chip->data_trigger_active = state;
+		chip->threshold_trigger_active = !state;
+	} else if (trig == chip->threshold_trig) {
+		chip->data_trigger_active = !state;
+		chip->threshold_trigger_active = state;
+	}
+
+	return ret;
+}
 
+
+static int opt4060_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	int ch_sel, ch_idx = chan->scan_index;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_INTENSITY)

drivers/iio/light/opt4060.c new file +15 -0

+}
+
+static int opt4060_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir, int state)
+{
+	int ch_sel, ch_idx = chan->scan_index;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_INTENSITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+

drivers/iio/light/opt4060.c new file +2 -0

+	return 0;
+}

drivers/iio/light/opt4060.c new file +11 -0

+static int opt4060_load_defaults(struct opt4060_chip *chip)
+{
+	u16 reg;
+	int ret;
+
+	chip->int_time = OPT4060_DEFAULT_CONVERSION_TIME;
+
+	/* Set initial MIN/MAX thresholds */
+	ret = opt4060_set_thresholds(chip, 0, UINT_MAX);
+	if (ret)
+		return ret;

drivers/iio/light/opt4060.c new file +12 -0

+}
+
+static const struct iio_buffer_setup_ops opt4060_buffer_ops = {
+	.preenable = opt4060_buffer_preenable,
+};
+
+static irqreturn_t opt4060_irq_thread(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct opt4060_chip *chip = iio_priv(idev);
+	int ret, dummy;
+	unsigned int int_res;

drivers/iio/light/opt4060.c new file +88 -0

+		if (ret) {
+			dev_err(chip->dev, "Failed to set interrupt state\n");
+			return IRQ_NONE;
+		}
+	}
+
+	/* Read OPT4060_CTRL to clear interrupt */
+	ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to clear interrupt\n");
+		return IRQ_NONE;
+	}
+
+	/* Handle events */
+	if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) {
+		u64 code;
+		int chan = 0;
+
+		ret = opt4060_get_channel_sel(chip, &chan);
+		if (ret) {
+			dev_err(chip->dev, "Failed to read threshold channel.\n");
+			return IRQ_NONE;
+		}
+
+		/* Check if the interrupt is from the lower threshold */
+		if (int_res & OPT4060_RES_CTRL_FLAG_L) {
+			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
+						    chan,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_FALLING);
+			iio_push_event(idev, code, iio_get_time_ns(idev));
+		}
+		/* Check if the interrupt is from the upper threshold */
+		if (int_res & OPT4060_RES_CTRL_FLAG_H) {
+			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
+						    chan,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING);
+			iio_push_event(idev, code, iio_get_time_ns(idev));
+		}
+		/* Handle threshold triggers */
+		if (chip->threshold_trigger_active && iio_buffer_enabled(idev))
+			iio_trigger_poll_nested(chip->threshold_trig);
+	}
+
+	/* Handle conversion ready */
+	if (int_res & OPT4060_RES_CTRL_CONV_READY) {
+		/* Signal completion for potentially waiting reads */
+		complete(&chip->completion);
+
+		/* Handle data ready triggers */
+		if (chip->data_trigger_active && iio_buffer_enabled(idev))
+			iio_trigger_poll_nested(chip->data_trig);
+	}
+	return IRQ_HANDLED;
+}
+
+static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev)
+{
+	struct iio_trigger *data_trigger;
+	struct iio_trigger *threshold_trigger;
+	char *name;
+	int ret;
+
+	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
+					      &iio_pollfunc_store_time,
+					      opt4060_trigger_handler,
+					      &opt4060_buffer_ops);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL\n");
+
+	data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d",
+					      idev->name, iio_device_id(idev));
+	if (!data_trigger)
+		return -ENOMEM;
+
+	/* The data trigger allows for sample capture on each new conversion ready interrupt. */
+	chip->data_trig = data_trigger;
+	data_trigger->ops = &opt4060_trigger_ops;
+	iio_trigger_set_drvdata(data_trigger, idev);
+	ret = devm_iio_trigger_register(chip->dev, data_trigger);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Data ready trigger registration failed\n");
+
+	threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d",
+				       idev->name, iio_device_id(idev));

drivers/iio/light/opt4060.c new file +41 -0

+	iio_trigger_set_drvdata(threshold_trigger, idev);
+	ret = devm_iio_trigger_register(chip->dev, threshold_trigger);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Threshold trigger registration failed\n");
+
+	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
+			      dev_name(chip->dev));
+	if (!name)
+		return dev_err_probe(chip->dev, -ENOMEM, "Failed to alloc chip name\n");
+
+	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread,
+					IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(chip->dev, ret, "Could not request IRQ\n");
+
+	ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL,
+				OPT4060_INT_CTRL_OUTPUT,
+				OPT4060_INT_CTRL_OUTPUT);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Failed to set interrupt as output\n");
+
+	return 0;
+}
+
+static int opt4060_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct opt4060_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+	uint dev_id;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+

drivers/iio/light/opt4060.c new file +23 -0

+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to setup power off action\n");
+
+	chip->dev = dev;
+	chip->irq = client->irq;
+	init_completion(&chip->completion);
+
+	indio_dev->info = &opt4060_info;
+
+	ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to reinit regmap cache\n");
+
+	ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+			"Failed to read the device ID register\n");
+
+	dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id);
+	if (dev_id != OPT4060_DEVICE_ID_VAL)
+		dev_info(dev, "Device ID: %#04x unknown\n", dev_id);

drivers/iio/light/opt4060.c new file +9 -0

+
+	if (chip->irq) {
+		ret = opt4060_setup_triggers(chip, indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}

Quoted 3 lines · On Wed, 16 Oct 2024 23:34:09 +0200 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

Hi Per-Daniel,

Comments inline.

Jonathan

Quoted 6 lines

drivers/iio/light/opt4060.c new file +0 -0

new file mode 100644
index 000000000000..2c3761ec423a
@@ -0,0 +1,1259 @@

...

drivers/iio/light/opt4060.c line 146

Quoted 4 lines

drivers/iio/light/opt4060.c new file +4 -0

+
+struct opt4060_buffer {
+	u32 chan[OPT4060_NUM_CHANS];
+	s64 ts __aligned(8);

aligned_s64 is now available in linux-next + the IIO tree.

drivers/iio/light/opt4060.c line 151

Quoted 5 lines

drivers/iio/light/opt4060.c new file +5 -0

+};
+
+static const struct opt4060_channel_factor opt4060_channel_factors[] = {
+	{
+		/* RED 2.4 * 2.15e-3 */

This needs more details on wrt to what standard etc.

The datasheet is a little vague, but it seems to me like TI invented their own standard. To use this stuff in a consistent ABI we need to have a common standard or at least an approximation of one.
The illuminance estimates from some devices are bad approximations, but they are at least attempting to approximate a well defined standard.

drivers/iio/light/opt4060.c line 377

Quoted 6 lines

drivers/iio/light/opt4060.c new file +6 -0

+	if (processed) {
+		/*
+		 * The processed value are multiplied by factors that correspond to optical
+		 * parameters in the sensor. The processed values gives more correct comparison
+		 * between channels.
+		 */

No to these. We discussed this in review thread of earlier version. There is no standard unit of comparison defined so this ABI is not something that userspace can currently understand.

I'm not against introducing one, though it would need a bunch of investigation into what the appropriate way to define this is. TI have chosen an option that I don't recall seeing used by anyone else but maybe I'm wrong on that. There are several competing RGB standards. https://en.wikipedia.org/wiki/RGB_color_spaces
provides some info

drivers/iio/light/opt4060.c line 1136

Quoted 7 lines

drivers/iio/light/opt4060.c new file +7 -0

+	/*
+	 * The threshold trigger allows for sample capture on threshold interrupts.
+	 * Just using events does not enable a way to get the samples that actually
+	 * triggered the threshold interrupt.
+	 */
+	chip->threshold_trig = threshold_trigger;
+	threshold_trigger->ops = &opt4060_trigger_ops;

Whilst I understand your usecase for this, I'd rather not have it in the initial driver. The use of events to trigger data capture should be a generic feature not one buried in a specific driver. Best bet for initial merge would be to just stick to the dataready trigger alone.
+ I'd also encourage you to look at if your usespace code can be made less sensitive to slightly late read back and hence potentially not a precise match with the trigger event.

Hi Jonathan,

Thank you for your feedback, much appreciated. I have added questions and comments inline below regarding channels and triggers. I will address the other comments in the next patch.

Best regards / Per-Daniel

Quoted 36 lines · Jonathan Cameron

Quoted 3 lines · On Wed, 16 Oct 2024 23:34:09 +0200 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

Hi Per-Daniel,

Comments inline.

Jonathan

Quoted 6 lines

drivers/iio/light/opt4060.c new file +0 -0

new file mode 100644
index 000000000000..2c3761ec423a
@@ -0,0 +1,1259 @@  

...

drivers/iio/light/opt4060.c line 146

Quoted 4 lines

drivers/iio/light/opt4060.c new file +4 -0

+
+struct opt4060_buffer {
+	u32 chan[OPT4060_NUM_CHANS];
+	s64 ts __aligned(8);  

aligned_s64 is now available in linux-next + the IIO tree.

drivers/iio/light/opt4060.c line 151

Quoted 5 lines

drivers/iio/light/opt4060.c new file +5 -0

+};
+
+static const struct opt4060_channel_factor opt4060_channel_factors[] = {
+	{
+		/* RED 2.4 * 2.15e-3 */  

This needs more details on wrt to what standard etc.

Quoted 5 lines

The datasheet is a little vague, but it seems to me like TI invented their own standard. To use this stuff in a consistent ABI we need to have a common standard or at least an approximation of one.
The illuminance estimates from some devices are bad approximations, but they are at least attempting to approximate a well defined standard.

I have read the datasheet again to try to figure out what TI means. When I read it now with your remarks from this email and previous emails in mind, I think I'm starting to understand more.

I think we should expose the data from the sensor in the following way:
- Four raw channels (R, G, B and Clear)
- Three processed IIO_INTENSITY channels with normalized values (R, G, B) to get the relative color components independent of light intensity.
- One IIO_LIGHT channel giving the lux value.

This is basically what TI is stating in chapter 8.4.5.2. I know that you don't like how TI are calculating the lux value using the green channel. But after reading the description and detailed description parts of the datasheet again, I think it sort of makes sense. Looking at the spectral response curves on the first page, the green curve covers the whole visible spectrum. It seems like this is what the sensor is actually designed for, measuring light intensity in lux and color independent of the light intensity.

Does this sound like a way forward you think?

Quoted 69 lines · On Wed, 23 Oct 2024 09:29:08 +0200 Per-Daniel Olsson…

Hi Jonathan,

Thank you for your feedback, much appreciated. I have added questions and comments inline below regarding channels and triggers. I will address the other comments in the next patch.

Best regards / Per-Daniel

Quoted 36 lines · Jonathan Cameron

Quoted 3 lines · On Wed, 16 Oct 2024 23:34:09 +0200 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

Hi Per-Daniel,

Comments inline.

Jonathan

Quoted 6 lines

drivers/iio/light/opt4060.c new file +0 -0

new file mode 100644
index 000000000000..2c3761ec423a
@@ -0,0 +1,1259 @@  

...

drivers/iio/light/opt4060.c line 146

Quoted 4 lines

drivers/iio/light/opt4060.c new file +4 -0

+
+struct opt4060_buffer {
+	u32 chan[OPT4060_NUM_CHANS];
+	s64 ts __aligned(8);  

aligned_s64 is now available in linux-next + the IIO tree.

drivers/iio/light/opt4060.c line 151

Quoted 5 lines

drivers/iio/light/opt4060.c new file +5 -0

+};
+
+static const struct opt4060_channel_factor opt4060_channel_factors[] = {
+	{
+		/* RED 2.4 * 2.15e-3 */  

This needs more details on wrt to what standard etc.

Quoted 5 lines

The datasheet is a little vague, but it seems to me like TI invented their own standard. To use this stuff in a consistent ABI we need to have a common standard or at least an approximation of one.
The illuminance estimates from some devices are bad approximations, but they are at least attempting to approximate a well defined standard.

I have read the datasheet again to try to figure out what TI means. When I read it now with your remarks from this email and previous emails in mind, I think I'm starting to understand more.

I think we should expose the data from the sensor in the following way:
- Four raw channels (R, G, B and Clear)
- Three processed IIO_INTENSITY channels with normalized values (R, G, B) to get the relative color components independent of light intensity.
- One IIO_LIGHT channel giving the lux value.

This is basically what TI is stating in chapter 8.4.5.2. I know that you don't like how TI are calculating the lux value using the green channel. But after reading the description and detailed description parts of the datasheet again, I think it sort of makes sense. Looking at the spectral response curves on the first page, the green curve covers the whole visible spectrum. It seems like this is what the sensor is actually designed for, measuring light intensity in lux and color independent of the light intensity.

Quoted 1 line

Does this sound like a way forward you think?

Not keen on the colour part.

As far as I can tell TI made up a colour standard. If it were
CIE 1931 RGB or then 'maybe' we could consider presenting them as processed, though as they are linear scales even then should present _raw and _scale, not _input (processed). We would still need to figure out if we needed to handle multiple colour space definitions.
As it is, if we have two different colour sensors, there is no way to compare the values. In particular that Green is way too broad for the colour standards I quickly compared this with.

The green curve does (based on eyeballing it rather than anything formal) look much closer to the luminosity function (one used for illuminance) than I was assuming (given it's called green!)

So not ideal but that one feels ok (with comments in the code explaining this) to use for illuminance.

For the color channels maybe we could present with _scale provided if we add suitable documentation to say that the scaling is to arbitrary datasheet specified normalization and that the resulting _raw * _scale values cannot be compared across different sensors. I don't like that but it does seem silly to not present the scaling if it might be useful to someone. So if you want to do this, propose some additions to Documentation/testing/ABI/sysfs-bus-iio
to cover this for in_intensity_red_scale
etc.

Jonathan