* [PATCH v9 0/2] Support for Texas Instruments OPT4060 RGBW Color sensor.
@ 2024-12-11 14:04 Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 1/2] dt-bindings: iio: light: Document TI OPT4060 RGBW sensor Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor Per-Daniel Olsson
0 siblings, 2 replies; 5+ messages in thread
From: Per-Daniel Olsson @ 2024-12-11 14:04 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Javier Carrasco
Cc: linux-iio, linux-kernel, devicetree, rickard.andersson, kernel,
Per-Daniel Olsson
This patch series adds support for Texas Instruments OPT4060 RGBW Color sensor
using the i2c interface.
The driver exposes raw adc values for red, green, blue and clear. The
illuminance is exposed as a calculated value in lux, the calculation uses the
wide spectrum green channel as base. The driver supports scale values for red,
green and blue. The raw values are scaled so that for a particular test light
source, typically white, the measurement intensity is the same across the
different color channels. Integration time can be configured through sysfs as
well. The OPT4060 sensor supports both rising and falling threshold interrupts.
These interrupts are exposed as IIO events. The driver also implements an IIO
triggered buffer with a trigger for conversion ready interrupts.
Changes in v9:
- Implemented mitigation for race conditions between buffers, sysfs read and events.
This resulted in fairly big changes around opt4060_trigger_one_shot(...),
opt4060_trigger_set_state(...) and opt4060_event_set_state(...). Some functions
are renamed and moved within the file. The general concept is that direct mode
has to be claimed for removing continuous sampling and irq.
- Fixed some typos.
- Changed in_intensity_xxx_scale to return factors instead of scaled raw values.
- Documentation: Adapted opt4060.rst to scaled factors instead of scaled raw values.
- Link to V8: https://lore.kernel.org/lkml/20241126155312.1660271-1-perdaniel.olsson@axis.com/
Changes in v8:
- Documentation: Fixed new line warning in opt4060.rst
- dt-bindings: Re-added "Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>"
- Link to V7: https://lore.kernel.org/lkml/20241126140002.1564564-1-perdaniel.olsson@axis.com/
Changes in v7:
- Calculation for scaled values changed to remove normalization.
- Fixed alignment in opt4060_write_ev_period(...).
- Fixed alignment in opt4060_read_ev_period(...).
- Updates state to bool in opt4060_write_event_config(...).
- dt-bindings: Define vdd-supply as required.
- dt-bindings: Removed description of vdd-supply.
- dt-bindings: Removed "Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>"
due to changes.
- Documentation: Added missing _scale parameters in Documentation/ABI/testing/sysfs-bus-iio
- Documentation: Updated opt4060.rst with description of scaled values without normalization.
- Cover letter: Description adapted to the new implementation of scaled values.
- Link to V6: https://lore.kernel.org/lkml/20241120163247.2791600-1-perdaniel.olsson@axis.com/
Changes in v6:
- Modified the driver to work with any trigger. Verified using iio-trig-sysfs.
- Fixed return in opt4060_trigger_set_state(...).
- Break added in opt4060_read_ev_period(...).
- Struct and variable declaration chnages in opt4060_trigger_handler(...).
- Events changed to modified and not unmodified.
- Moved variable declaration from case statement in opt4060_read_event(...), and
opt4060_write_event(...), found by test robot.
- Init_completion() moved to a place related to IRQ.
- Documentation: Added opt4060.rst with description of calculations.
- Ducumentation: Added missing _raw parameters in Documentation/ABI/testing/sysfs-bus-iio
- Link to V5: https://lore.kernel.org/lkml/20241106120036.986755-1-perdaniel.olsson@axis.com/
Changes in v5:
- Cover letter: Description adapted to new channel and trigger setup.
- Trigger for threshold removed.
- Channel setup modified according to email discussion.
- Switched to aligned_s64 from linux-next.
- Endianness in buffer changed to IIO_CPU.
- opt4060_read_raw_value(...) changed to avoid endian issue plus early return.
- opt4060_read_chan_value(...) split of in several for new channel setup.
- Improved return values in multiple places.
- The preenable callback removed, functionality moved to set_state.
- Irq setup removed from opt4060_irq_thread(...).
- Flipped logic with early return in opt4060_trigger_one_shot(...).
- devm_add_action_or_reset(...) moved to after opt4060_load_defaults(...).
- Documentation: Added sysfs-bus-iio-light-opt4060 with channel descriptions.
- Added hard coded name indio_dev->name.
- Modified opt4060_volatile_reg(...) to make test robot happy.
- Link to V4: https://lore.kernel.org/lkml/20241016213409.3823162-1-perdaniel.olsson@axis.com/
Changes in v4:
- Fix for a warning found by test robot in opt4060_write_event(...).
- Correction of early return in opt4060_write_event(...) and opt4060_read_event(...),
missed from review of version 2.
- Correction of timeout for sample conversion.
- Correction of bug when changing integration time with buffer enabled.
- Link to V3: https://lore.kernel.org/lkml/20241015143713.2017626-1-perdaniel.olsson@axis.com/
Changes in v3:
- Cover letter: Removed lux from description.
- OPT_4060_DRV_NAME define removed.
- Corrected alignment for struct opt4060_buffer.
- Added description of the CRC calculation.
- Cleaned variable declaration in several places.
- Added a path for the non-irq case in opt4060_read_chan_value(...).
- Added a description of processed values.
- Use of regmap_clear_bits in opt4060_power_down(...).
- Switched to IIO_INTENSITY instead of IIO_LIGHT.
- Correction of channel index in IIO_UNMOD_EVENT_CODE, found by test robot.
- Added iio_chan_spec for the non-irq case without events.
- Fixed braces in a few if-else statements.
- Refactoring with early returns in a few places to reduce indentation.
- Replaced for_each_set_bit with iio_for_each_active_channel.
- Removed various too obvious comments.
- Fixed various other code style problems.
- Link to V2: https://lore.kernel.org/lkml/20241005165119.3549472-1-perdaniel.olsson@axis.com/
Changes in v2:
- dt-bindings: Removed incorrect allOf.
- dt-bindings: Changed to generic node name.
- Correction in opt4060_trigger_one_shot(...) for continuous mode.
- Correction in opt4060_power_down(...), wrong register was read.
- Corrected usage of active_scan_mask in opt4060_trigger_handler(...).
- Clean-up of various comments.
- Link to V1: https://lore.kernel.org/lkml/20241003164932.1162049-1-perdaniel.olsson@axis.com/
Per-Daniel Olsson (2):
dt-bindings: iio: light: Document TI OPT4060 RGBW sensor
iio: light: Add support for TI OPT4060 color sensor
Documentation/ABI/testing/sysfs-bus-iio | 7 +
.../bindings/iio/light/ti,opt4060.yaml | 51 +
Documentation/iio/index.rst | 1 +
Documentation/iio/opt4060.rst | 61 +
drivers/iio/light/Kconfig | 13 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/opt4060.c | 1306 +++++++++++++++++
7 files changed, 1440 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml
create mode 100644 Documentation/iio/opt4060.rst
create mode 100644 drivers/iio/light/opt4060.c
--
2.39.5
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v9 1/2] dt-bindings: iio: light: Document TI OPT4060 RGBW sensor
2024-12-11 14:04 [PATCH v9 0/2] Support for Texas Instruments OPT4060 RGBW Color sensor Per-Daniel Olsson
@ 2024-12-11 14:04 ` Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor Per-Daniel Olsson
1 sibling, 0 replies; 5+ messages in thread
From: Per-Daniel Olsson @ 2024-12-11 14:04 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Javier Carrasco
Cc: linux-iio, linux-kernel, devicetree, rickard.andersson, kernel,
Per-Daniel Olsson, Krzysztof Kozlowski
Add devicetree bindings for the OPT4060 RGBW color sensor.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
---
.../bindings/iio/light/ti,opt4060.yaml | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml
new file mode 100644
index 000000000000..568fb2a9b7a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ti,opt4060.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments OPT4060 RGBW Color Sensor
+
+maintainers:
+ - Per-Daniel Olsson <perdaniel.olsson@axis.com>
+
+description:
+ Texas Instrument RGBW high resolution color sensor over I2C.
+ https://www.ti.com/lit/gpn/opt4060
+
+properties:
+ compatible:
+ enum:
+ - ti,opt4060
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light-sensor@44 {
+ compatible = "ti,opt4060";
+ reg = <0x44>;
+ vdd-supply = <&vdd_reg>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+ };
+ };
+...
--
2.39.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor
2024-12-11 14:04 [PATCH v9 0/2] Support for Texas Instruments OPT4060 RGBW Color sensor Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 1/2] dt-bindings: iio: light: Document TI OPT4060 RGBW sensor Per-Daniel Olsson
@ 2024-12-11 14:04 ` Per-Daniel Olsson
2024-12-14 18:21 ` Jonathan Cameron
1 sibling, 1 reply; 5+ messages in thread
From: Per-Daniel Olsson @ 2024-12-11 14:04 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Javier Carrasco
Cc: linux-iio, linux-kernel, devicetree, rickard.andersson, kernel,
Per-Daniel Olsson
Add support for Texas Instruments OPT4060 RGBW Color sensor.
Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 7 +
Documentation/iio/index.rst | 1 +
Documentation/iio/opt4060.rst | 61 ++
drivers/iio/light/Kconfig | 13 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/opt4060.c | 1306 +++++++++++++++++++++++
6 files changed, 1389 insertions(+)
create mode 100644 Documentation/iio/opt4060.rst
create mode 100644 drivers/iio/light/opt4060.c
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index f83bd6829285..9a2aac22010d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -508,6 +508,9 @@ What: /sys/bus/iio/devices/iio:deviceX/in_angl_scale
What: /sys/bus/iio/devices/iio:deviceX/in_intensity_x_scale
What: /sys/bus/iio/devices/iio:deviceX/in_intensity_y_scale
What: /sys/bus/iio/devices/iio:deviceX/in_intensity_z_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
What: /sys/bus/iio/devices/iio:deviceX/in_concentration_co2_scale
KernelVersion: 2.6.35
Contact: linux-iio@vger.kernel.org
@@ -1633,6 +1636,10 @@ What: /sys/.../iio:deviceX/in_intensityY_uv_raw
What: /sys/.../iio:deviceX/in_intensityY_uva_raw
What: /sys/.../iio:deviceX/in_intensityY_uvb_raw
What: /sys/.../iio:deviceX/in_intensityY_duv_raw
+What: /sys/.../iio:deviceX/in_intensity_red_raw
+What: /sys/.../iio:deviceX/in_intensity_green_raw
+What: /sys/.../iio:deviceX/in_intensity_blue_raw
+What: /sys/.../iio:deviceX/in_intensity_clear_raw
KernelVersion: 3.4
Contact: linux-iio@vger.kernel.org
Description:
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 074dbbf7ba0a..5710f5b9e958 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -29,3 +29,4 @@ Industrial I/O Kernel Drivers
adxl380
bno055
ep93xx_adc
+ opt4060
diff --git a/Documentation/iio/opt4060.rst b/Documentation/iio/opt4060.rst
new file mode 100644
index 000000000000..eb155089b6d2
--- /dev/null
+++ b/Documentation/iio/opt4060.rst
@@ -0,0 +1,61 @@
+==============================
+OPT4060 driver
+==============================
+
+1. Overview
+=============================
+
+This driver supports the Texas Instrument RGBW high resolution color sensor over
+I2C.
+https://www.ti.com/lit/gpn/opt4060
+
+The driver supports:
+- Raw values for red, green, blue and clear.
+- Illuminance values.
+- Scaled color values for red, green and blue.
+- IIO events for thresholds.
+- IIO triggered buffer using both its own data ready trigger and triggers from
+other drivers.
+
+2. Illuminance calculation
+=============================
+
+Illuminance is calculated using the wide spectrum green channel.
+
+lux = GREEN_RAW x 2.15e-3
+
+The value is accessed from:
+/sys/bus/iio/devices/iio:deviceX/in_illuminance_input
+
+See section 8.4.5.2 in the data sheet for additional details.
+
+3. Color scale values
+=============================
+
+The sensor has different sensitivity for the different color components and
+compensating factors are exposed from the driver.
+
+The values are accessed from:
+/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
+/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+
+A userspace application can multiply the raw values with the scale values so
+that for a particular test light source, typically white, the measurement
+intensity is the same across the different color channels. This is calculated
+in the following way:
+
+R = RED_RAW x SCALE_RED(2.4)
+G = GREEN_RAW x SCALE_GREEN(1.0)
+B = BLUE_RAW x SCALE_BLUE(1.3)
+
+The data sheet suggests using the scaled values to normalize the scaled R, G
+and B values. This is useful to get a value for the ratio between colors
+independent of light intensity. A userspace application can do this in the
+following way:
+
+R_NORMALIZED = R / (R + G + B)
+G_NORMALIZED = G / (R + G + B)
+B_NORMALIZED = B / (R + G + B)
+
+See section 8.4.5.2 in the data sheet for additional details.
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0cf9cf2a3f94..4897258e3da5 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -475,6 +475,19 @@ config OPT4001
If built as a dynamically linked module, it will be called
opt4001.
+config OPT4060
+ tristate "Texas Instruments OPT4060 RGBW Color Sensor"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ If you say Y or M here, you get support for Texas Instruments
+ OPT4060 RGBW Color Sensor.
+
+ If built as a dynamically linked module, it will be called
+ opt4060.
+
config PA12203001
tristate "TXC PA12203001 light and proximity sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 56beb324f34f..11a4041b918a 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_MAX44009) += max44009.o
obj-$(CONFIG_NOA1305) += noa1305.o
obj-$(CONFIG_OPT3001) += opt3001.o
obj-$(CONFIG_OPT4001) += opt4001.o
+obj-$(CONFIG_OPT4060) += opt4060.o
obj-$(CONFIG_PA12203001) += pa12203001.o
obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
obj-$(CONFIG_RPR0521) += rpr0521.o
diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
new file mode 100644
index 000000000000..4a7d970c5d7c
--- /dev/null
+++ b/drivers/iio/light/opt4060.c
@@ -0,0 +1,1306 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Axis Communications AB
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/opt4060
+ *
+ * Device driver for the Texas Instruments OPT4060 RGBW Color Sensor.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/math64.h>
+#include <linux/units.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* OPT4060 register set */
+#define OPT4060_RED_MSB 0x00
+#define OPT4060_RED_LSB 0x01
+#define OPT4060_GREEN_MSB 0x02
+#define OPT4060_GREEN_LSB 0x03
+#define OPT4060_BLUE_MSB 0x04
+#define OPT4060_BLUE_LSB 0x05
+#define OPT4060_CLEAR_MSB 0x06
+#define OPT4060_CLEAR_LSB 0x07
+#define OPT4060_THRESHOLD_LOW 0x08
+#define OPT4060_THRESHOLD_HIGH 0x09
+#define OPT4060_CTRL 0x0a
+#define OPT4060_INT_CTRL 0x0b
+#define OPT4060_RES_CTRL 0x0c
+#define OPT4060_DEVICE_ID 0x11
+
+/* OPT4060 register mask */
+#define OPT4060_EXPONENT_MASK GENMASK(15, 12)
+#define OPT4060_MSB_MASK GENMASK(11, 0)
+#define OPT4060_LSB_MASK GENMASK(15, 8)
+#define OPT4060_COUNTER_MASK GENMASK(7, 4)
+#define OPT4060_CRC_MASK GENMASK(3, 0)
+
+/* OPT4060 device id mask */
+#define OPT4060_DEVICE_ID_MASK GENMASK(11, 0)
+
+/* OPT4060 control register masks */
+#define OPT4060_CTRL_QWAKE_MASK BIT(15)
+#define OPT4060_CTRL_RANGE_MASK GENMASK(13, 10)
+#define OPT4060_CTRL_CONV_TIME_MASK GENMASK(9, 6)
+#define OPT4060_CTRL_OPER_MODE_MASK GENMASK(5, 4)
+#define OPT4060_CTRL_LATCH_MASK BIT(3)
+#define OPT4060_CTRL_INT_POL_MASK BIT(2)
+#define OPT4060_CTRL_FAULT_COUNT_MASK GENMASK(1, 0)
+
+/* OPT4060 interrupt control register masks */
+#define OPT4060_INT_CTRL_THRESH_SEL GENMASK(6, 5)
+#define OPT4060_INT_CTRL_OUTPUT BIT(4)
+#define OPT4060_INT_CTRL_INT_CFG GENMASK(3, 2)
+#define OPT4060_INT_CTRL_THRESHOLD 0x0
+#define OPT4060_INT_CTRL_NEXT_CH 0x1
+#define OPT4060_INT_CTRL_ALL_CH 0x3
+
+/* OPT4060 result control register masks */
+#define OPT4060_RES_CTRL_OVERLOAD BIT(3)
+#define OPT4060_RES_CTRL_CONV_READY BIT(2)
+#define OPT4060_RES_CTRL_FLAG_H BIT(1)
+#define OPT4060_RES_CTRL_FLAG_L BIT(0)
+
+/* OPT4060 constants */
+#define OPT4060_DEVICE_ID_VAL 0x821
+
+/* OPT4060 operating modes */
+#define OPT4060_CTRL_OPER_MODE_OFF 0x0
+#define OPT4060_CTRL_OPER_MODE_FORCED 0x1
+#define OPT4060_CTRL_OPER_MODE_ONE_SHOT 0x2
+#define OPT4060_CTRL_OPER_MODE_CONTINUOUS 0x3
+
+/* OPT4060 conversion control register definitions */
+#define OPT4060_CTRL_CONVERSION_0_6MS 0x0
+#define OPT4060_CTRL_CONVERSION_1MS 0x1
+#define OPT4060_CTRL_CONVERSION_1_8MS 0x2
+#define OPT4060_CTRL_CONVERSION_3_4MS 0x3
+#define OPT4060_CTRL_CONVERSION_6_5MS 0x4
+#define OPT4060_CTRL_CONVERSION_12_7MS 0x5
+#define OPT4060_CTRL_CONVERSION_25MS 0x6
+#define OPT4060_CTRL_CONVERSION_50MS 0x7
+#define OPT4060_CTRL_CONVERSION_100MS 0x8
+#define OPT4060_CTRL_CONVERSION_200MS 0x9
+#define OPT4060_CTRL_CONVERSION_400MS 0xa
+#define OPT4060_CTRL_CONVERSION_800MS 0xb
+
+/* OPT4060 fault count control register definitions */
+#define OPT4060_CTRL_FAULT_COUNT_1 0x0
+#define OPT4060_CTRL_FAULT_COUNT_2 0x1
+#define OPT4060_CTRL_FAULT_COUNT_4 0x2
+#define OPT4060_CTRL_FAULT_COUNT_8 0x3
+
+/* OPT4060 scale light level range definitions */
+#define OPT4060_CTRL_LIGHT_SCALE_AUTO 12
+
+/* OPT4060 default values */
+#define OPT4060_DEFAULT_CONVERSION_TIME OPT4060_CTRL_CONVERSION_50MS
+
+/*
+ * enum opt4060_chan_type - OPT4060 channel types
+ * @OPT4060_RED: Red channel.
+ * @OPT4060_GREEN: Green channel.
+ * @OPT4060_BLUE: Blue channel.
+ * @OPT4060_CLEAR: Clear (white) channel.
+ * @OPT4060_ILLUM: Calculated illuminance channel.
+ * @OPT4060_NUM_CHANS: Number of channel types.
+ */
+enum opt4060_chan_type {
+ OPT4060_RED,
+ OPT4060_GREEN,
+ OPT4060_BLUE,
+ OPT4060_CLEAR,
+ OPT4060_ILLUM,
+ OPT4060_NUM_CHANS
+};
+
+struct opt4060_chip {
+ struct regmap *regmap;
+ struct device *dev;
+ struct iio_trigger *trig;
+ u8 int_time;
+ int irq;
+ struct mutex irq_setting_lock;
+ struct completion completion;
+ bool thresh_event_lo_active;
+ bool thresh_event_hi_active;
+};
+
+struct opt4060_channel_factor {
+ u32 mul;
+ u32 div;
+};
+
+static const int opt4060_int_time_available[][2] = {
+ { 0, 600 },
+ { 0, 1000 },
+ { 0, 1800 },
+ { 0, 3400 },
+ { 0, 6500 },
+ { 0, 12700 },
+ { 0, 25000 },
+ { 0, 50000 },
+ { 0, 100000 },
+ { 0, 200000 },
+ { 0, 400000 },
+ { 0, 800000 },
+};
+
+/*
+ * Conversion time is integration time + time to set register
+ * this is used as integration time.
+ */
+static const int opt4060_int_time_reg[][2] = {
+ { 600, OPT4060_CTRL_CONVERSION_0_6MS },
+ { 1000, OPT4060_CTRL_CONVERSION_1MS },
+ { 1800, OPT4060_CTRL_CONVERSION_1_8MS },
+ { 3400, OPT4060_CTRL_CONVERSION_3_4MS },
+ { 6500, OPT4060_CTRL_CONVERSION_6_5MS },
+ { 12700, OPT4060_CTRL_CONVERSION_12_7MS },
+ { 25000, OPT4060_CTRL_CONVERSION_25MS },
+ { 50000, OPT4060_CTRL_CONVERSION_50MS },
+ { 100000, OPT4060_CTRL_CONVERSION_100MS },
+ { 200000, OPT4060_CTRL_CONVERSION_200MS },
+ { 400000, OPT4060_CTRL_CONVERSION_400MS },
+ { 800000, OPT4060_CTRL_CONVERSION_800MS },
+};
+
+static int opt4060_als_time_to_index(const u32 als_integration_time)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(opt4060_int_time_available); i++) {
+ if (als_integration_time == opt4060_int_time_available[i][1])
+ return i;
+ }
+
+ return -EINVAL;
+}
+
+static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count)
+{
+ u8 crc;
+
+ /*
+ * Calculates a 4-bit CRC from a 20-bit mantissa, 4-bit exponent and a 4-bit counter.
+ * crc[0] = XOR(mantissa[19:0], exp[3:0], count[3:0])
+ * crc[1] = XOR(mantissa[1,3,5,7,9,11,13,15,17,19], exp[1,3], count[1,3])
+ * crc[2] = XOR(mantissa[3,7,11,15,19], exp[3], count[3])
+ * crc[3] = XOR(mantissa[3,11,19])
+ */
+ crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2;
+ crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA)
+ + hweight32(count & 0xA)) % 2) << 1;
+ crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8)
+ + hweight32(count & 0x8)) % 2) << 2;
+ crc |= (hweight32(mantissa & 0x80808) % 2) << 3;
+
+ return crc;
+}
+
+static void opt4060_claim_irq_setting_lock(struct opt4060_chip *chip)
+{
+ if (chip->irq)
+ mutex_lock(&chip->irq_setting_lock);
+}
+
+static void opt4060_release_irq_setting_lock(struct opt4060_chip *chip)
+{
+ if (chip->irq)
+ mutex_unlock(&chip->irq_setting_lock);
+}
+
+static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state)
+{
+ int ret;
+ unsigned int regval;
+
+ opt4060_claim_irq_setting_lock(chip);
+ regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state);
+ ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
+ OPT4060_INT_CTRL_INT_CFG, regval);
+ if (ret)
+ dev_err(chip->dev, "Failed to set interrupt config\n");
+ opt4060_release_irq_setting_lock(chip);
+ return ret;
+}
+
+static int opt4060_set_continuous_mode(struct opt4060_chip *chip,
+ bool continuous)
+{
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(chip->regmap, OPT4060_CTRL, ®);
+ if (ret < 0) {
+ dev_err(chip->dev, "Failed to read ctrl register\n");
+ return ret;
+ }
+ reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
+ if (continuous)
+ reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+ OPT4060_CTRL_OPER_MODE_CONTINUOUS);
+ else
+ reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+ OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+
+ /* Trigger a new conversions by writing to CRTL register. */
+ ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
+ if (ret)
+ dev_err(chip->dev, "Failed to set ctrl register\n");
+ return ret;
+}
+
+static bool opt4060_event_active(struct opt4060_chip *chip)
+{
+ return chip->thresh_event_lo_active || chip->thresh_event_hi_active;
+}
+
+static int opt4060_set_state_common(struct opt4060_chip *chip,
+ bool continuous_sampling,
+ bool continuous_irq, bool direct_mode)
+{
+ int ret = 0;
+
+ /* It is important to setup irq before sampling to avoid missing samples. */
+ if (continuous_irq || !direct_mode)
+ ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
+ else if (direct_mode)
+ ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
+ if (ret) {
+ dev_err(chip->dev, "Failed to set irq state.\n");
+ return ret;
+ }
+
+ if (continuous_sampling || !direct_mode || opt4060_event_active(chip))
+ ret = opt4060_set_continuous_mode(chip, true);
+ else if (direct_mode)
+ ret = opt4060_set_continuous_mode(chip, false);
+ if (ret)
+ dev_err(chip->dev, "Failed to set sampling state.\n");
+ return ret;
+}
+
+/*
+ * Function for setting the driver state for sampling and irq. When disabling
+ * continuous sampling or irq, the IIO direct mode must be claimed to prevent
+ * races with buffer enabling/disabling. In the case when the direct mode is
+ * not possible to claim, the function will keep continuous mode. All
+ * functions, sysfs read, events and buffer, work in continuous mode.
+ */
+static int opt4060_set_driver_state(struct iio_dev *indio_dev,
+ bool continuous_sampling,
+ bool continuous_irq)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ bool direct_mode = false;
+ int ret = 0;
+
+ if (!iio_device_claim_direct_mode(indio_dev))
+ direct_mode = true;
+
+ ret = opt4060_set_state_common(chip, continuous_sampling,
+ continuous_irq, direct_mode);
+
+ if (direct_mode)
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+}
+
+/*
+ * This function is called in direct mode from the framework.
+ */
+static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ int ret = 0;
+
+ return ret = opt4060_set_state_common(chip, state, state, true);
+}
+
+static int opt4060_read_raw_value(struct opt4060_chip *chip,
+ unsigned long address, u32 *raw)
+{
+ int ret;
+ u16 result[2];
+ u32 mantissa_raw;
+ u16 msb, lsb;
+ u8 exp, count, crc, calc_crc;
+
+ ret = regmap_bulk_read(chip->regmap, address, result, 2);
+ if (ret) {
+ dev_err(chip->dev, "Reading channel data failed\n");
+ return ret;
+ }
+ exp = FIELD_GET(OPT4060_EXPONENT_MASK, result[0]);
+ msb = FIELD_GET(OPT4060_MSB_MASK, result[0]);
+ count = FIELD_GET(OPT4060_COUNTER_MASK, result[1]);
+ crc = FIELD_GET(OPT4060_CRC_MASK, result[1]);
+ lsb = FIELD_GET(OPT4060_LSB_MASK, result[1]);
+ mantissa_raw = (msb << 8) + lsb;
+ calc_crc = opt4060_calculate_crc(exp, mantissa_raw, count);
+ if (calc_crc != crc)
+ return -EIO;
+ *raw = mantissa_raw << exp;
+ return 0;
+}
+
+static int opt4060_trigger_new_samples(struct iio_dev *indio_dev)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ 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 *
+ opt4060_int_time_reg[chip->int_time][0]) * 2 + 100000;
+
+ /* Setting the state in one shot mode with irq on each sample. */
+ ret = opt4060_set_driver_state(indio_dev, false, true);
+ if (ret)
+ return ret;
+
+ if (chip->irq) {
+ reinit_completion(&chip->completion);
+ opt4060_claim_irq_setting_lock(chip);
+ if (wait_for_completion_timeout(&chip->completion,
+ usecs_to_jiffies(timeout_us)) == 0) {
+ dev_err(chip->dev, "Completion timed out.\n");
+ opt4060_release_irq_setting_lock(chip);
+ return -ETIME;
+ }
+ opt4060_release_irq_setting_lock(chip);
+ } else {
+ unsigned int ready;
+
+ 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");
+ }
+ /* Setting the state in one shot mode with irq on thresholds. */
+ ret = opt4060_set_driver_state(indio_dev, false, false);
+
+ return ret;
+}
+
+static int opt4060_read_chan_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ u32 adc_raw;
+ int ret;
+
+ ret = opt4060_trigger_new_samples(indio_dev);
+ if (ret) {
+ dev_err(chip->dev, "Failed to trigger new samples.\n");
+ return ret;
+ }
+
+ ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);
+ if (ret) {
+ dev_err(chip->dev, "Reading raw channel data failed.\n");
+ return ret;
+ }
+ *val = adc_raw;
+ return IIO_VAL_INT;
+}
+
+/*
+ * Returns the scale values used for red, green and blue. Scales the raw value
+ * so that for a particular test light source, typically white, the measurement
+ * intensity is the same across different color channels.
+ */
+static int opt4060_get_chan_scale(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+
+ switch (chan->scan_index) {
+ case OPT4060_RED:
+ /* 2.4 */
+ *val = 2;
+ *val2 = 400000;
+ break;
+ case OPT4060_GREEN:
+ /* 1.0 */
+ *val = 1;
+ *val2 = 0;
+ break;
+ case OPT4060_BLUE:
+ /* 1.3 */
+ *val = 1;
+ *val2 = 300000;
+ break;
+ default:
+ dev_err(chip->dev, "Unexpected channel index.\n");
+ return -EINVAL;
+ }
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int opt4060_calc_illuminance(struct opt4060_chip *chip, int *val)
+{
+ u32 lux_raw;
+ int ret;
+
+ /* The green wide spectral channel is used for illuminance. */
+ ret = opt4060_read_raw_value(chip, OPT4060_GREEN_MSB, &lux_raw);
+ if (ret) {
+ dev_err(chip->dev, "Reading raw channel data failed\n");
+ return ret;
+ }
+
+ /* Illuminance is calculated by ADC_RAW * 2.15e-3. */
+ *val = DIV_U64_ROUND_CLOSEST((u64)(lux_raw * 215), 1000);
+ return ret;
+}
+
+static int opt4060_read_illuminance(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ int ret;
+
+ ret = opt4060_trigger_new_samples(indio_dev);
+ if (ret) {
+ dev_err(chip->dev, "Failed to trigger new samples.\n");
+ return ret;
+ }
+ ret = opt4060_calc_illuminance(chip, val);
+ if (ret) {
+ dev_err(chip->dev, "Failed to calculate illuminance.\n");
+ return ret;
+ }
+
+ return IIO_VAL_INT;
+}
+
+static int opt4060_set_int_time(struct opt4060_chip *chip)
+{
+ unsigned int regval;
+ int ret;
+
+ regval = FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time);
+ ret = regmap_update_bits(chip->regmap, OPT4060_CTRL,
+ OPT4060_CTRL_CONV_TIME_MASK, regval);
+ if (ret)
+ dev_err(chip->dev, "Failed to set integration time.\n");
+
+ return ret;
+}
+
+static int opt4060_power_down(struct opt4060_chip *chip)
+{
+ int ret;
+
+ ret = regmap_clear_bits(chip->regmap, OPT4060_CTRL, OPT4060_CTRL_OPER_MODE_MASK);
+ if (ret)
+ dev_err(chip->dev, "Failed to power down\n");
+
+ return ret;
+}
+
+static void opt4060_chip_off_action(void *chip)
+{
+ opt4060_power_down(chip);
+}
+
+#define _OPT4060_COLOR_CHANNEL(_color, _mask, _ev_spec, _num_ev_spec) \
+{ \
+ .type = IIO_INTENSITY, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_LIGHT_##_color, \
+ .info_mask_separate = _mask, \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .address = OPT4060_##_color##_MSB, \
+ .scan_index = OPT4060_##_color, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 32, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }, \
+ .event_spec = _ev_spec, \
+ .num_event_specs = _num_ev_spec, \
+}
+
+#define OPT4060_COLOR_CHANNEL(_color, _mask) \
+ _OPT4060_COLOR_CHANNEL(_color, _mask, opt4060_event_spec, \
+ ARRAY_SIZE(opt4060_event_spec)) \
+
+#define OPT4060_COLOR_CHANNEL_NO_EVENTS(_color, _mask) \
+ _OPT4060_COLOR_CHANNEL(_color, _mask, NULL, 0) \
+
+#define OPT4060_LIGHT_CHANNEL(_channel) \
+{ \
+ .type = IIO_LIGHT, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .scan_index = OPT4060_##_channel, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 32, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }, \
+}
+
+static const struct iio_event_spec opt4060_event_spec[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_PERIOD),
+ },
+};
+
+static const struct iio_chan_spec opt4060_channels[] = {
+ OPT4060_COLOR_CHANNEL(RED, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL(GREEN, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL(BLUE, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL(CLEAR, BIT(IIO_CHAN_INFO_RAW)),
+ OPT4060_LIGHT_CHANNEL(ILLUM),
+ IIO_CHAN_SOFT_TIMESTAMP(OPT4060_NUM_CHANS),
+};
+
+static const struct iio_chan_spec opt4060_channels_no_events[] = {
+ OPT4060_COLOR_CHANNEL_NO_EVENTS(RED, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL_NO_EVENTS(GREEN, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL_NO_EVENTS(BLUE, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+ OPT4060_COLOR_CHANNEL_NO_EVENTS(CLEAR, BIT(IIO_CHAN_INFO_RAW)),
+ OPT4060_LIGHT_CHANNEL(ILLUM),
+ IIO_CHAN_SOFT_TIMESTAMP(OPT4060_NUM_CHANS),
+};
+
+static int opt4060_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ return opt4060_read_chan_raw(indio_dev, chan, val);
+ case IIO_CHAN_INFO_SCALE:
+ return opt4060_get_chan_scale(indio_dev, chan, val, val2);
+ case IIO_CHAN_INFO_PROCESSED:
+ return opt4060_read_illuminance(indio_dev, chan, val);
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+ *val2 = opt4060_int_time_reg[chip->int_time][0];
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4060_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ int int_time;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ int_time = opt4060_als_time_to_index(val2);
+ if (int_time < 0)
+ return int_time;
+ chip->int_time = int_time;
+ return opt4060_set_int_time(chip);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static u32 opt4060_calc_th_reg(u32 adc_val)
+{
+ u32 th_val, th_exp, bits;
+ /*
+ * The threshold registers take 4 bits of exponent and 12 bits of data
+ * ADC = TH_VAL << (8 + TH_EXP)
+ */
+ bits = fls(adc_val);
+
+ if (bits > 31)
+ th_exp = 11; /* Maximum exponent */
+ else if (bits > 20)
+ th_exp = bits - 20;
+ else
+ th_exp = 0;
+ th_val = (adc_val >> (8 + th_exp)) & 0xfff;
+
+ return (th_exp << 12) + th_val;
+}
+
+static u32 opt4060_calc_val_from_th_reg(u32 th_reg)
+{
+ /*
+ * The threshold registers take 4 bits of exponent and 12 bits of data
+ * ADC = TH_VAL << (8 + TH_EXP)
+ */
+ u32 th_val, th_exp;
+
+ th_exp = (th_reg >> 12) & 0xf;
+ th_val = th_reg & 0xfff;
+
+ return th_val << (8 + th_exp);
+}
+
+static int opt4060_read_available(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ *length = ARRAY_SIZE(opt4060_int_time_available) * 2;
+ *vals = (const int *)opt4060_int_time_available;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static ssize_t opt4060_read_ev_period(struct opt4060_chip *chip, int *val,
+ int *val2)
+{
+ int ret, pers, fault_count, int_time;
+ u64 uval;
+
+ int_time = opt4060_int_time_reg[chip->int_time][0];
+
+ ret = regmap_read(chip->regmap, OPT4060_CTRL, &fault_count);
+ if (ret < 0)
+ return ret;
+
+ fault_count = fault_count & OPT4060_CTRL_FAULT_COUNT_MASK;
+ switch (fault_count) {
+ case OPT4060_CTRL_FAULT_COUNT_2:
+ pers = 2;
+ break;
+ case OPT4060_CTRL_FAULT_COUNT_4:
+ pers = 4;
+ break;
+ case OPT4060_CTRL_FAULT_COUNT_8:
+ pers = 8;
+ break;
+
+ default:
+ pers = 1;
+ break;
+ }
+
+ uval = mul_u32_u32(int_time, pers);
+ *val = div_u64_rem(uval, MICRO, val2);
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static ssize_t opt4060_write_ev_period(struct opt4060_chip *chip, int val,
+ int val2)
+{
+ u64 uval, int_time;
+ unsigned int regval, fault_count_val;
+
+ uval = mul_u32_u32(val, MICRO) + val2;
+ int_time = opt4060_int_time_reg[chip->int_time][0];
+
+ /* Check if the period is closest to 1, 2, 4 or 8 times integration time.*/
+ if (uval <= int_time)
+ fault_count_val = OPT4060_CTRL_FAULT_COUNT_1;
+ else if (uval <= int_time * 2)
+ fault_count_val = OPT4060_CTRL_FAULT_COUNT_2;
+ else if (uval <= int_time * 4)
+ fault_count_val = OPT4060_CTRL_FAULT_COUNT_4;
+ else
+ fault_count_val = OPT4060_CTRL_FAULT_COUNT_8;
+
+ regval = FIELD_PREP(OPT4060_CTRL_FAULT_COUNT_MASK, fault_count_val);
+ return regmap_update_bits(chip->regmap, OPT4060_CTRL,
+ OPT4060_CTRL_FAULT_COUNT_MASK, regval);
+}
+
+static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel)
+{
+ int ret;
+ u32 regval;
+
+ ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val);
+ if (ret)
+ dev_err(chip->dev, "Failed to get channel selection.\n");
+ *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
+ return ret;
+}
+
+static int opt4060_set_channel_sel(struct opt4060_chip *chip, int ch_sel)
+{
+ int ret;
+ u32 regval;
+
+ regval = FIELD_PREP(OPT4060_INT_CTRL_THRESH_SEL, ch_sel);
+ ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
+ OPT4060_INT_CTRL_THRESH_SEL, regval);
+ if (ret)
+ dev_err(chip->dev, "Failed to set channel selection.\n");
+ return ret;
+}
+
+static int opt4060_get_thresholds(struct opt4060_chip *chip, u32 *th_lo, u32 *th_hi)
+{
+ int ret;
+ u32 regval;
+
+ ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_LOW, ®val);
+ if (ret) {
+ dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n");
+ return ret;
+ }
+ *th_lo = opt4060_calc_val_from_th_reg(regval);
+
+ ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_HIGH, ®val);
+ if (ret) {
+ dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n");
+ return ret;
+ }
+ *th_hi = opt4060_calc_val_from_th_reg(regval);
+
+ return ret;
+}
+
+static int opt4060_set_thresholds(struct opt4060_chip *chip, u32 th_lo, u32 th_hi)
+{
+ int ret;
+
+ ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_LOW, opt4060_calc_th_reg(th_lo));
+ if (ret) {
+ dev_err(chip->dev, "Failed to write THRESHOLD_LOW.\n");
+ return ret;
+ }
+
+ ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_HIGH, opt4060_calc_th_reg(th_hi));
+ if (ret)
+ dev_err(chip->dev, "Failed to write THRESHOLD_HIGH.\n");
+
+ return ret;
+}
+
+static int opt4060_read_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ u32 th_lo, th_hi;
+ int ret;
+
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
+ if (ret)
+ return ret;
+ if (dir == IIO_EV_DIR_FALLING) {
+ *val = th_lo;
+ ret = IIO_VAL_INT;
+ } else if (dir == IIO_EV_DIR_RISING) {
+ *val = th_hi;
+ ret = IIO_VAL_INT;
+ }
+ return ret;
+ case IIO_EV_INFO_PERIOD:
+ return opt4060_read_ev_period(chip, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4060_write_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ u32 th_lo, th_hi;
+ int ret;
+
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
+ if (ret)
+ return ret;
+ if (dir == IIO_EV_DIR_FALLING)
+ th_lo = val;
+ else if (dir == IIO_EV_DIR_RISING)
+ th_hi = val;
+ return opt4060_set_thresholds(chip, th_lo, th_hi);
+ case IIO_EV_INFO_PERIOD:
+ return opt4060_write_ev_period(chip, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+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);
+ int ret;
+
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ ret = opt4060_get_channel_sel(chip, &ch_sel);
+ if (ret)
+ return ret;
+
+ if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
+ ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
+ return ch_sel == ch_idx;
+
+ return ret;
+}
+
+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, bool state)
+{
+ int ch_sel, ch_idx = chan->scan_index;
+ struct opt4060_chip *chip = iio_priv(indio_dev);
+ int ret;
+
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ ret = opt4060_get_channel_sel(chip, &ch_sel);
+ if (ret)
+ return ret;
+
+ if (state) {
+ /* Only one channel can be active at the same time */
+ if ((chip->thresh_event_lo_active ||
+ chip->thresh_event_hi_active) && (ch_idx != ch_sel))
+ return -EBUSY;
+ if (dir == IIO_EV_DIR_FALLING)
+ chip->thresh_event_lo_active = true;
+ else if (dir == IIO_EV_DIR_RISING)
+ chip->thresh_event_hi_active = true;
+ ret = opt4060_set_channel_sel(chip, ch_idx);
+ if (ret)
+ return ret;
+ } else {
+ if (ch_idx == ch_sel) {
+ if (dir == IIO_EV_DIR_FALLING)
+ chip->thresh_event_lo_active = false;
+ else if (dir == IIO_EV_DIR_RISING)
+ chip->thresh_event_hi_active = false;
+ }
+ }
+
+ return opt4060_set_driver_state(indio_dev, chip->thresh_event_hi_active |
+ chip->thresh_event_lo_active, false);
+}
+
+static const struct iio_info opt4060_info = {
+ .read_raw = opt4060_read_raw,
+ .write_raw = opt4060_write_raw,
+ .write_raw_get_fmt = opt4060_write_raw_get_fmt,
+ .read_avail = opt4060_read_available,
+ .read_event_value = opt4060_read_event,
+ .write_event_value = opt4060_write_event,
+ .read_event_config = opt4060_read_event_config,
+ .write_event_config = opt4060_write_event_config,
+};
+
+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;
+
+ /*
+ * Setting auto-range, latched window for thresholds, one-shot conversion
+ * and quick wake-up mode as default.
+ */
+ reg = FIELD_PREP(OPT4060_CTRL_RANGE_MASK,
+ OPT4060_CTRL_LIGHT_SCALE_AUTO);
+ reg |= FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time);
+ reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+ OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+ reg |= OPT4060_CTRL_QWAKE_MASK | OPT4060_CTRL_LATCH_MASK;
+
+ ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
+ if (ret)
+ dev_err(chip->dev, "Failed to set configuration\n");
+
+ return ret;
+}
+
+static bool opt4060_volatile_reg(struct device *dev, unsigned int reg)
+{
+ return reg <= OPT4060_CLEAR_LSB || reg == OPT4060_RES_CTRL;
+}
+
+static bool opt4060_writable_reg(struct device *dev, unsigned int reg)
+{
+ return reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL;
+}
+
+static bool opt4060_readonly_reg(struct device *dev, unsigned int reg)
+{
+ return reg == OPT4060_DEVICE_ID;
+}
+
+static bool opt4060_readable_reg(struct device *dev, unsigned int reg)
+{
+ /* Volatile, writable and read-only registers are readable. */
+ return opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) ||
+ opt4060_readonly_reg(dev, reg);
+}
+
+static const struct regmap_config opt4060_regmap_config = {
+ .name = "opt4060",
+ .reg_bits = 8,
+ .val_bits = 16,
+ .cache_type = REGCACHE_RBTREE,
+ .max_register = OPT4060_DEVICE_ID,
+ .readable_reg = opt4060_readable_reg,
+ .writeable_reg = opt4060_writable_reg,
+ .volatile_reg = opt4060_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_BIG,
+};
+
+static const struct iio_trigger_ops opt4060_trigger_ops = {
+ .set_trigger_state = opt4060_trigger_set_state,
+};
+
+static irqreturn_t opt4060_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *idev = pf->indio_dev;
+ struct opt4060_chip *chip = iio_priv(idev);
+ struct {
+ u32 chan[OPT4060_NUM_CHANS];
+ aligned_s64 ts;
+ } raw;
+ int i = 0;
+ int chan, ret;
+
+ /* If the trigger is coming for a different driver, a new sample is needed.*/
+ if (iio_trigger_validate_own_device(idev->trig, idev))
+ opt4060_trigger_new_samples(idev);
+
+ memset(&raw, 0, sizeof(raw));
+
+ iio_for_each_active_channel(idev, chan) {
+ if (chan == OPT4060_ILLUM)
+ ret = opt4060_calc_illuminance(chip, &raw.chan[i++]);
+ else
+ ret = opt4060_read_raw_value(chip,
+ idev->channels[chan].address,
+ &raw.chan[i++]);
+ if (ret) {
+ dev_err(chip->dev, "Reading channel data failed\n");
+ goto err_read;
+ }
+ }
+
+ iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
+err_read:
+ iio_trigger_notify_done(idev->trig);
+ return IRQ_HANDLED;
+}
+
+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;
+
+ ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
+ if (ret < 0) {
+ dev_err(chip->dev, "Failed to read interrupt reasons.\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_MOD_EVENT_CODE(IIO_INTENSITY,
+ chan,
+ idev->channels[chan].channel2,
+ 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_MOD_EVENT_CODE(IIO_INTENSITY,
+ chan,
+ idev->channels[chan].channel2,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING);
+ iio_push_event(idev, code, iio_get_time_ns(idev));
+ }
+ }
+
+ /* 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 (iio_buffer_enabled(idev))
+ iio_trigger_poll_nested(chip->trig);
+ }
+ return IRQ_HANDLED;
+}
+
+static int opt4060_setup_buffer(struct opt4060_chip *chip, struct iio_dev *idev)
+{
+ int ret;
+
+ ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
+ &iio_pollfunc_store_time,
+ opt4060_trigger_handler, NULL);
+ if (ret)
+ return dev_err_probe(chip->dev, ret,
+ "Buffer setup failed.\n");
+ return ret;
+}
+
+static int opt4060_setup_trigger(struct opt4060_chip *chip, struct iio_dev *idev)
+{
+ struct iio_trigger *data_trigger;
+ char *name;
+ int ret;
+
+ 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->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");
+
+ 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");
+
+ init_completion(&chip->completion);
+
+ mutex_init(&chip->irq_setting_lock);
+
+ 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;
+ unsigned int regval, dev_id;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ chip = iio_priv(indio_dev);
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
+
+ chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config);
+ if (IS_ERR(chip->regmap))
+ return dev_err_probe(dev, PTR_ERR(chip->regmap),
+ "regmap initialization failed\n");
+
+ chip->dev = dev;
+ chip->irq = client->irq;
+
+ 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, ®val);
+ 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, regval);
+ if (dev_id != OPT4060_DEVICE_ID_VAL)
+ dev_info(dev, "Device ID: %#04x unknown\n", dev_id);
+
+ if (chip->irq) {
+ indio_dev->channels = opt4060_channels;
+ indio_dev->num_channels = ARRAY_SIZE(opt4060_channels);
+ } else {
+ indio_dev->channels = opt4060_channels_no_events;
+ indio_dev->num_channels = ARRAY_SIZE(opt4060_channels_no_events);
+ }
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->name = "opt4060";
+
+ ret = opt4060_load_defaults(chip);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to set sensor defaults\n");
+
+ ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to setup power off action\n");
+
+ ret = opt4060_setup_buffer(chip, indio_dev);
+ if (ret)
+ return ret;
+
+ if (chip->irq) {
+ ret = opt4060_setup_trigger(chip, indio_dev);
+ if (ret)
+ return ret;
+ }
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id opt4060_id[] = {
+ { "opt4060", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, opt4060_id);
+
+static const struct of_device_id opt4060_of_match[] = {
+ { .compatible = "ti,opt4060" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, opt4060_of_match);
+
+static struct i2c_driver opt4060_driver = {
+ .driver = {
+ .name = "opt4060",
+ .of_match_table = opt4060_of_match,
+ },
+ .probe = opt4060_probe,
+ .id_table = opt4060_id,
+};
+module_i2c_driver(opt4060_driver);
+
+MODULE_AUTHOR("Per-Daniel Olsson <perdaniel.olsson@axis.com>");
+MODULE_DESCRIPTION("Texas Instruments OPT4060 RGBW color sensor driver");
+MODULE_LICENSE("GPL");
--
2.39.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor
2024-12-11 14:04 ` [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor Per-Daniel Olsson
@ 2024-12-14 18:21 ` Jonathan Cameron
2024-12-18 10:46 ` Per-Daniel Olsson
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2024-12-14 18:21 UTC (permalink / raw)
To: Per-Daniel Olsson
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Javier Carrasco, linux-iio, linux-kernel,
devicetree, rickard.andersson, kernel
On Wed, 11 Dec 2024 15:04:09 +0100
Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:
> Add support for Texas Instruments OPT4060 RGBW Color sensor.
>
> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
Hi Per-Daniel,
I think this is nearly there, but still some races or at least places
I can't currently convince myself aren't races.
I remember long ago being a wimp and failing to implement a similar dance
in the max1363 ADC driver. That manages one more complexity in that in
it's continuous mode if events are enabled, the data fields move.
It still only supports one of events, sysfs read back or buffered output,
not any combination. Maybe if I can find the hardware I'll revisit that
one day.
Thanks,
Jonathan
> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> new file mode 100644
> index 000000000000..4a7d970c5d7c
> --- /dev/null
> +++ b/drivers/iio/light/opt4060.c
> +struct opt4060_chip {
> + struct regmap *regmap;
> + struct device *dev;
> + struct iio_trigger *trig;
> + u8 int_time;
> + int irq;
> + struct mutex irq_setting_lock;
General rule is all locks need a comment on what data they protect
even when they have a nice specific name. Bit advantage is it makes
it clear what they are not designed to protect!
> + struct completion completion;
> + bool thresh_event_lo_active;
> + bool thresh_event_hi_active;
> +};
> +static void opt4060_claim_irq_setting_lock(struct opt4060_chip *chip)
> +{
> + if (chip->irq)
I'm struggling to see why you'd be messing with irqs if you don't have
any? I can't see a path in which chip->irq isn't set (which makes sense!)
and you get here.
So I think you can just move the mutex inline which avoids the mess
of lockdep warnings etc and let's you use guard() to simplify things.
> + mutex_lock(&chip->irq_setting_lock);
> +}
> +
> +static void opt4060_release_irq_setting_lock(struct opt4060_chip *chip)
> +{
> + if (chip->irq)
> + mutex_unlock(&chip->irq_setting_lock);
> +}
> +
> +static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state)
> +{
> + int ret;
> + unsigned int regval;
> +
> + opt4060_claim_irq_setting_lock(chip);
> + regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state);
> + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
> + OPT4060_INT_CTRL_INT_CFG, regval);
> + if (ret)
> + dev_err(chip->dev, "Failed to set interrupt config\n");
> + opt4060_release_irq_setting_lock(chip);
> + return ret;
> +}
> +
> +static int opt4060_set_continuous_mode(struct opt4060_chip *chip,
> + bool continuous)
> +{
> + unsigned int reg;
> + int ret;
> +
> + ret = regmap_read(chip->regmap, OPT4060_CTRL, ®);
You could use a regmap_update_bits() call to simplify this like you
do for the int_state above.
> + if (ret < 0) {
> + dev_err(chip->dev, "Failed to read ctrl register\n");
> + return ret;
> + }
> + reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
> + if (continuous)
> + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
> + OPT4060_CTRL_OPER_MODE_CONTINUOUS);
> + else
> + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
> + OPT4060_CTRL_OPER_MODE_ONE_SHOT);
> +
> + /* Trigger a new conversions by writing to CRTL register. */
> + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
> + if (ret)
> + dev_err(chip->dev, "Failed to set ctrl register\n");
> + return ret;
> +}
> +
> +static bool opt4060_event_active(struct opt4060_chip *chip)
> +{
> + return chip->thresh_event_lo_active || chip->thresh_event_hi_active;
> +}
> +
> +static int opt4060_set_state_common(struct opt4060_chip *chip,
> + bool continuous_sampling,
> + bool continuous_irq, bool direct_mode)
> +{
> + int ret = 0;
> +
> + /* It is important to setup irq before sampling to avoid missing samples. */
> + if (continuous_irq || !direct_mode)
> + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
> + else if (direct_mode)
> + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set irq state.\n");
> + return ret;
> + }
> +
> + if (continuous_sampling || !direct_mode || opt4060_event_active(chip))
I think there may also a race around the event active check. You could have
one event direction being enabled concurrently with the other being disabled.
I'm not sure it matters but worth checking.
Side effect of either claiming direct or buffered mode is that only one
caller can do it at a time, so that would close this race as well.
Having said that, it's an implementation detail of the core (be it one that
has been there a long time) so you should really have your own driver
specific locking scheme prevent that.
> + ret = opt4060_set_continuous_mode(chip, true);
> + else if (direct_mode)
> + ret = opt4060_set_continuous_mode(chip, false);
> + if (ret)
> + dev_err(chip->dev, "Failed to set sampling state.\n");
> + return ret;
> +}
> +
> +/*
> + * Function for setting the driver state for sampling and irq. When disabling
> + * continuous sampling or irq, the IIO direct mode must be claimed to prevent
> + * races with buffer enabling/disabling. In the case when the direct mode is
> + * not possible to claim, the function will keep continuous mode. All
> + * functions, sysfs read, events and buffer, work in continuous mode.
> + */
> +static int opt4060_set_driver_state(struct iio_dev *indio_dev,
> + bool continuous_sampling,
> + bool continuous_irq)
> +{
> + struct opt4060_chip *chip = iio_priv(indio_dev);
> + bool direct_mode = false;
> + int ret = 0;
> +
> + if (!iio_device_claim_direct_mode(indio_dev))
> + direct_mode = true;
Hmm. I'm dubious about this pattern. Why is it fine if the driver
leaves buffered mode right here? I was expecting this to do
the dance with claiming either direct mode or buffered mode.
(with the retry loop). Direct mode that you pass into the next
call may well be false when it should be true.
Even if you can reason why that isn't a problem (and there are worse
dances where it switches mode multiple times during your call
of the next function to consider) I think it is easier to reason
about if we know it is definitely not changing state until we
release it.
> +
> + ret = opt4060_set_state_common(chip, continuous_sampling,
> + continuous_irq, direct_mode);
> +
> + if (direct_mode)
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> +}
> +
> +/*
> + * This function is called in direct mode from the framework.
> + */
> +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct opt4060_chip *chip = iio_priv(indio_dev);
> + int ret = 0;
> +
> + return ret = opt4060_set_state_common(chip, state, state, true);
return opt_set_state_common() is probably the intent.
> +}
> +static int opt4060_trigger_new_samples(struct iio_dev *indio_dev)
> +{
> + struct opt4060_chip *chip = iio_priv(indio_dev);
> + 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 *
> + opt4060_int_time_reg[chip->int_time][0]) * 2 + 100000;
> +
> + /* Setting the state in one shot mode with irq on each sample. */
> + ret = opt4060_set_driver_state(indio_dev, false, true);
> + if (ret)
> + return ret;
> +
> + if (chip->irq) {
> + reinit_completion(&chip->completion);
> + opt4060_claim_irq_setting_lock(chip);
> + if (wait_for_completion_timeout(&chip->completion,
> + usecs_to_jiffies(timeout_us)) == 0) {
> + dev_err(chip->dev, "Completion timed out.\n");
> + opt4060_release_irq_setting_lock(chip);
This is where exposing the lock directly will simplify things as you can just use
a guard.
> + return -ETIME;
> + }
> + opt4060_release_irq_setting_lock(chip);
> + } else {
> + unsigned int ready;
> +
> + 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");
> + }
> + /* Setting the state in one shot mode with irq on thresholds. */
> + ret = opt4060_set_driver_state(indio_dev, false, false);
> +
> + return ret;
return opt4060_...
> +}
> +static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + return IIO_VAL_INT_PLUS_MICRO;
IIRC That's the default, so you don't need to provide write_raw_get_fmt,
though no harm in doing so I guess.
> + default:
> + return -EINVAL;
> + }
> +}
> +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel)
> +{
> + int ret;
> + u32 regval;
> +
> + ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val);
> + if (ret)
> + dev_err(chip->dev, "Failed to get channel selection.\n");
if you have garbage, not sure it's valid to update ch_sel.
> + *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
> + return ret;
> +}
> +
> +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, bool state)
> +{
> + int ch_sel, ch_idx = chan->scan_index;
> + struct opt4060_chip *chip = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_INTENSITY)
> + return -EINVAL;
> + if (type != IIO_EV_TYPE_THRESH)
> + return -EINVAL;
> +
> + ret = opt4060_get_channel_sel(chip, &ch_sel);
> + if (ret)
> + return ret;
> +
> + if (state) {
> + /* Only one channel can be active at the same time */
> + if ((chip->thresh_event_lo_active ||
> + chip->thresh_event_hi_active) && (ch_idx != ch_sel))
That's a bit nasty to ready. I'd use a slightly long line and get the || pair
on the first line.
Hmm. We've never made rules on this but some devices to fifo type
selection if they have limitations on events enabled at the same time.
With hindsight I think this scheme of just saying no is probably more
user friendly.
> + return -EBUSY;
> + if (dir == IIO_EV_DIR_FALLING)
> + chip->thresh_event_lo_active = true;
> + else if (dir == IIO_EV_DIR_RISING)
> + chip->thresh_event_hi_active = true;
> + ret = opt4060_set_channel_sel(chip, ch_idx);
> + if (ret)
> + return ret;
> + } else {
> + if (ch_idx == ch_sel) {
> + if (dir == IIO_EV_DIR_FALLING)
> + chip->thresh_event_lo_active = false;
> + else if (dir == IIO_EV_DIR_RISING)
> + chip->thresh_event_hi_active = false;
> + }
> + }
> +
> + return opt4060_set_driver_state(indio_dev, chip->thresh_event_hi_active |
> + chip->thresh_event_lo_active, false);
Maybe wrap it to have the | pair on lines with nothing else. They are a little bit burried
otherwise.
return opt4060_set_driver_state(indio_dev,
chip->thresh_event_hi_active |
chip->thresh_event_lo_active,
false);
> +}
> +
> +static const struct iio_info opt4060_info = {
> + .read_raw = opt4060_read_raw,
> + .write_raw = opt4060_write_raw,
> + .write_raw_get_fmt = opt4060_write_raw_get_fmt,
> + .read_avail = opt4060_read_available,
> + .read_event_value = opt4060_read_event,
> + .write_event_value = opt4060_write_event,
> + .read_event_config = opt4060_read_event_config,
> + .write_event_config = opt4060_write_event_config,
> +};
Given you have option for no irq it is probably worth picking a version of this
info structure with all the event callbacks removed. Technically it isn't
required but it does harden the code (by crashing horribly if you call them ;)
> +static irqreturn_t opt4060_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *idev = pf->indio_dev;
> + struct opt4060_chip *chip = iio_priv(idev);
> + struct {
> + u32 chan[OPT4060_NUM_CHANS];
> + aligned_s64 ts;
> + } raw;
> + int i = 0;
> + int chan, ret;
> +
> + /* If the trigger is coming for a different driver, a new sample is needed.*/
from a different driver?
> + if (iio_trigger_validate_own_device(idev->trig, idev))
> + opt4060_trigger_new_samples(idev);
> +
> + memset(&raw, 0, sizeof(raw));
> +
> + iio_for_each_active_channel(idev, chan) {
> + if (chan == OPT4060_ILLUM)
> + ret = opt4060_calc_illuminance(chip, &raw.chan[i++]);
> + else
> + ret = opt4060_read_raw_value(chip,
> + idev->channels[chan].address,
> + &raw.chan[i++]);
> + if (ret) {
> + dev_err(chip->dev, "Reading channel data failed\n");
> + goto err_read;
> + }
> + }
> +
> + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
> +err_read:
> + iio_trigger_notify_done(idev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +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;
> +
> + ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
> + if (ret < 0) {
> + dev_err(chip->dev, "Failed to read interrupt reasons.\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_MOD_EVENT_CODE(IIO_INTENSITY,
> + chan,
> + idev->channels[chan].channel2,
> + 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_MOD_EVENT_CODE(IIO_INTENSITY,
> + chan,
> + idev->channels[chan].channel2,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING);
> + iio_push_event(idev, code, iio_get_time_ns(idev));
> + }
> + }
> +
> + /* Handle conversion ready */
> + if (int_res & OPT4060_RES_CTRL_CONV_READY) {
> + /* Signal completion for potentially waiting reads */
> + complete(&chip->completion);
That looks problematic as you haven't necessarily reset the completion
if the buffer is enabled. So you probably need a flag or something similar
to say a sysfs read has been requested.
> +
> + /* Handle data ready triggers */
> + if (iio_buffer_enabled(idev))
> + iio_trigger_poll_nested(chip->trig);
> + }
> + return IRQ_HANDLED;
> +}
> +static int opt4060_setup_trigger(struct opt4060_chip *chip, struct iio_dev *idev)
> +{
> + struct iio_trigger *data_trigger;
> + char *name;
> + int ret;
> +
> + 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. */
Make that a multiline comment.
> + chip->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");
> +
> + 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 |
That's unusual for a trigger type interrupt and seems likely to give a lot
of spurious interrupts. Even if the pulse is short, some interrupt controllers
will hang on to the bonus edge and trigger again when you reenable the interrupt.
If intent is to use this for events, then I think you can configure it to latched
window mode and one edge type and it will all work.
> + IRQF_ONESHOT, name, idev);
> + if (ret)
> + return dev_err_probe(chip->dev, ret, "Could not request IRQ\n");
> +
> + init_completion(&chip->completion);
> +
> + mutex_init(&chip->irq_setting_lock);
Trivial and I might not even bother changing it, but slightly preference for
ret = devm_mutex_init(...)
if (ret)
return ret;
> +
> + 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;
> +}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor
2024-12-14 18:21 ` Jonathan Cameron
@ 2024-12-18 10:46 ` Per-Daniel Olsson
0 siblings, 0 replies; 5+ messages in thread
From: Per-Daniel Olsson @ 2024-12-18 10:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Javier Carrasco, linux-iio, linux-kernel,
devicetree, rickard.andersson, kernel
On 12/14/24 19:21, Jonathan Cameron wrote:
> On Wed, 11 Dec 2024 15:04:09 +0100
> Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:
>
>> Add support for Texas Instruments OPT4060 RGBW Color sensor.
>>
>> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
>
> Hi Per-Daniel,
>
> I think this is nearly there, but still some races or at least places
> I can't currently convince myself aren't races.
>
> I remember long ago being a wimp and failing to implement a similar dance
> in the max1363 ADC driver. That manages one more complexity in that in
> it's continuous mode if events are enabled, the data fields move.
> It still only supports one of events, sysfs read back or buffered output,
> not any combination. Maybe if I can find the hardware I'll revisit that
> one day.
>
> Thanks,
>
> Jonathan
Hi Jonathan,
Thank you for your comments. Also thank you for sharing your story about max1363,
sounds like a tricky piece of silicon. Great to hear that you think it can be
worth the time to implement concurrent use cases, I was a little worried that I
was pushing that part a little too far... :)
I have added comments below.
Thanks (and happy holidays if I don't hear back from you before),
Per-Daniel
>
>> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
>> new file mode 100644
>> index 000000000000..4a7d970c5d7c
>> --- /dev/null
>> +++ b/drivers/iio/light/opt4060.c
>
>
>> +struct opt4060_chip {
>> + struct regmap *regmap;
>> + struct device *dev;
>> + struct iio_trigger *trig;
>> + u8 int_time;
>> + int irq;
>> + struct mutex irq_setting_lock;
>
> General rule is all locks need a comment on what data they protect
> even when they have a nice specific name. Bit advantage is it makes
> it clear what they are not designed to protect!
Done in v10.
>
>> + struct completion completion;
>> + bool thresh_event_lo_active;
>> + bool thresh_event_hi_active;
>> +};
>
>> +static void opt4060_claim_irq_setting_lock(struct opt4060_chip *chip)
>> +{
>> + if (chip->irq)
> I'm struggling to see why you'd be messing with irqs if you don't have
> any? I can't see a path in which chip->irq isn't set (which makes sense!)
> and you get here.
Good point, fixed in v10.
>
> So I think you can just move the mutex inline which avoids the mess
> of lockdep warnings etc and let's you use guard() to simplify things.
>
>> + mutex_lock(&chip->irq_setting_lock);
>> +}
>> +
>> +static void opt4060_release_irq_setting_lock(struct opt4060_chip *chip)
>> +{
>> + if (chip->irq)
>> + mutex_unlock(&chip->irq_setting_lock);
>> +}
>> +
>> +static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state)
>> +{
>> + int ret;
>> + unsigned int regval;
>> +
>> + opt4060_claim_irq_setting_lock(chip);
>> + regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state);
>> + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
>> + OPT4060_INT_CTRL_INT_CFG, regval);
>> + if (ret)
>> + dev_err(chip->dev, "Failed to set interrupt config\n");
>> + opt4060_release_irq_setting_lock(chip);
>> + return ret;
>> +}
>> +
>> +static int opt4060_set_continuous_mode(struct opt4060_chip *chip,
>> + bool continuous)
>> +{
>> + unsigned int reg;
>> + int ret;
>> +
>> + ret = regmap_read(chip->regmap, OPT4060_CTRL, ®);
>
> You could use a regmap_update_bits() call to simplify this like you
> do for the int_state above.
That register is a little special. It requires a real write sometimes
even is the data hasn't changed, so regmap_update_bits won't work. This
is needed for triggering one shot mode. I have renamed the function in
v10 since it don't just set continuous mode but also one shot. I have
also changed the comment in the code.
>
>
>> + if (ret < 0) {
>> + dev_err(chip->dev, "Failed to read ctrl register\n");
>> + return ret;
>> + }
>> + reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
>> + if (continuous)
>> + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
>> + OPT4060_CTRL_OPER_MODE_CONTINUOUS);
>> + else
>> + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
>> + OPT4060_CTRL_OPER_MODE_ONE_SHOT);
>> +
>> + /* Trigger a new conversions by writing to CRTL register. */
>> + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
>> + if (ret)
>> + dev_err(chip->dev, "Failed to set ctrl register\n");
>> + return ret;
>> +}
>> +
>> +static bool opt4060_event_active(struct opt4060_chip *chip)
>> +{
>> + return chip->thresh_event_lo_active || chip->thresh_event_hi_active;
>> +}
>> +
>> +static int opt4060_set_state_common(struct opt4060_chip *chip,
>> + bool continuous_sampling,
>> + bool continuous_irq, bool direct_mode)
>> +{
>> + int ret = 0;
>> +
>> + /* It is important to setup irq before sampling to avoid missing samples. */
>> + if (continuous_irq || !direct_mode)
>> + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
>> + else if (direct_mode)
>> + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
>> + if (ret) {
>> + dev_err(chip->dev, "Failed to set irq state.\n");
>> + return ret;
>> + }
>> +
>> + if (continuous_sampling || !direct_mode || opt4060_event_active(chip))
>
> I think there may also a race around the event active check. You could have
> one event direction being enabled concurrently with the other being disabled.
> I'm not sure it matters but worth checking.
I think you might be correct even if I haven't been able to trigger the case in
my tests. I have added a guard for this in v10. If opt4060_write_event_config()
is called from several callers at the same time, the whole sequence all the way
down to setting sampling and irq will be synchronized with the mutex/guard.
>
> Side effect of either claiming direct or buffered mode is that only one
> caller can do it at a time, so that would close this race as well.
> Having said that, it's an implementation detail of the core (be it one that
> has been there a long time) so you should really have your own driver
> specific locking scheme prevent that.
I have implemented the "dance" in v10 and it seems to work well in my tests.
>
>
>> + ret = opt4060_set_continuous_mode(chip, true);
>> + else if (direct_mode)
>> + ret = opt4060_set_continuous_mode(chip, false);
>> + if (ret)
>> + dev_err(chip->dev, "Failed to set sampling state.\n");
>> + return ret;
>> +}
>> +
>> +/*
>> + * Function for setting the driver state for sampling and irq. When disabling
>> + * continuous sampling or irq, the IIO direct mode must be claimed to prevent
>> + * races with buffer enabling/disabling. In the case when the direct mode is
>> + * not possible to claim, the function will keep continuous mode. All
>> + * functions, sysfs read, events and buffer, work in continuous mode.
>> + */
>> +static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>> + bool continuous_sampling,
>> + bool continuous_irq)
>> +{
>> + struct opt4060_chip *chip = iio_priv(indio_dev);
>> + bool direct_mode = false;
>> + int ret = 0;
>> +
>> + if (!iio_device_claim_direct_mode(indio_dev))
>> + direct_mode = true;
>
> Hmm. I'm dubious about this pattern. Why is it fine if the driver
> leaves buffered mode right here? I was expecting this to do
> the dance with claiming either direct mode or buffered mode.
> (with the retry loop). Direct mode that you pass into the next
> call may well be false when it should be true.
>
> Even if you can reason why that isn't a problem (and there are worse
> dances where it switches mode multiple times during your call
> of the next function to consider) I think it is easier to reason
> about if we know it is definitely not changing state until we
> release it.
"dance" implemented in v10.
>
>> +
>> + ret = opt4060_set_state_common(chip, continuous_sampling,
>> + continuous_irq, direct_mode);
>> +
>> + if (direct_mode)
>> + iio_device_release_direct_mode(indio_dev);
>> + return ret;
>> +}
>> +
>> +/*
>> + * This function is called in direct mode from the framework.
>> + */
>> +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state)
>> +{
>> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>> + struct opt4060_chip *chip = iio_priv(indio_dev);
>> + int ret = 0;
>> +
>> + return ret = opt4060_set_state_common(chip, state, state, true);
>
> return opt_set_state_common() is probably the intent.
>
>> +}
>
>> +static int opt4060_trigger_new_samples(struct iio_dev *indio_dev)
>> +{
>> + struct opt4060_chip *chip = iio_priv(indio_dev);
>> + 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 *
>> + opt4060_int_time_reg[chip->int_time][0]) * 2 + 100000;
>> +
>> + /* Setting the state in one shot mode with irq on each sample. */
>> + ret = opt4060_set_driver_state(indio_dev, false, true);
>> + if (ret)
>> + return ret;
>> +
>> + if (chip->irq) {
>> + reinit_completion(&chip->completion);
>> + opt4060_claim_irq_setting_lock(chip);
>> + if (wait_for_completion_timeout(&chip->completion,
>> + usecs_to_jiffies(timeout_us)) == 0) {
>> + dev_err(chip->dev, "Completion timed out.\n");
>> + opt4060_release_irq_setting_lock(chip);
>
> This is where exposing the lock directly will simplify things as you can just use
> a guard.
Fixed with guard in v10.
>
>> + return -ETIME;
>> + }
>> + opt4060_release_irq_setting_lock(chip);
>> + } else {
>> + unsigned int ready;
>> +
>> + 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");
>> + }
>> + /* Setting the state in one shot mode with irq on thresholds. */
>> + ret = opt4060_set_driver_state(indio_dev, false, false);
>> +
>> + return ret;
>
> return opt4060_...
>
>> +}
>
>> +static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + long mask)
>> +{
>> + switch (mask) {
>> + case IIO_CHAN_INFO_INT_TIME:
>> + return IIO_VAL_INT_PLUS_MICRO;
> IIRC That's the default, so you don't need to provide write_raw_get_fmt,
> though no harm in doing so I guess.
Ok, wasn't aware of that being the default.
>
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>
>
>> +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel)
>> +{
>> + int ret;
>> + u32 regval;
>> +
>> + ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val);
>> + if (ret)
>> + dev_err(chip->dev, "Failed to get channel selection.\n");
>
> if you have garbage, not sure it's valid to update ch_sel.
>
>> + *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
>> + return ret;
>> +}
>> +
>
>
>> +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, bool state)
>> +{
>> + int ch_sel, ch_idx = chan->scan_index;
>> + struct opt4060_chip *chip = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (chan->type != IIO_INTENSITY)
>> + return -EINVAL;
>> + if (type != IIO_EV_TYPE_THRESH)
>> + return -EINVAL;
>> +
>> + ret = opt4060_get_channel_sel(chip, &ch_sel);
>> + if (ret)
>> + return ret;
>> +
>> + if (state) {
>> + /* Only one channel can be active at the same time */
>> + if ((chip->thresh_event_lo_active ||
>> + chip->thresh_event_hi_active) && (ch_idx != ch_sel))
>
> That's a bit nasty to ready. I'd use a slightly long line and get the || pair
> on the first line.
Fixed in v10.
>
> Hmm. We've never made rules on this but some devices to fifo type
> selection if they have limitations on events enabled at the same time.
> With hindsight I think this scheme of just saying no is probably more
> user friendly.
Ok.
>
>> + return -EBUSY;
>> + if (dir == IIO_EV_DIR_FALLING)
>> + chip->thresh_event_lo_active = true;
>> + else if (dir == IIO_EV_DIR_RISING)
>> + chip->thresh_event_hi_active = true;
>> + ret = opt4060_set_channel_sel(chip, ch_idx);
>> + if (ret)
>> + return ret;
>> + } else {
>> + if (ch_idx == ch_sel) {
>> + if (dir == IIO_EV_DIR_FALLING)
>> + chip->thresh_event_lo_active = false;
>> + else if (dir == IIO_EV_DIR_RISING)
>> + chip->thresh_event_hi_active = false;
>> + }
>> + }
>> +
>> + return opt4060_set_driver_state(indio_dev, chip->thresh_event_hi_active |
>> + chip->thresh_event_lo_active, false);
> Maybe wrap it to have the | pair on lines with nothing else. They are a little bit burried
> otherwise.
Fixed in v10.
> return opt4060_set_driver_state(indio_dev,
> chip->thresh_event_hi_active |
> chip->thresh_event_lo_active,
> false);
>
>> +}
>> +
>> +static const struct iio_info opt4060_info = {
>> + .read_raw = opt4060_read_raw,
>> + .write_raw = opt4060_write_raw,
>> + .write_raw_get_fmt = opt4060_write_raw_get_fmt,
>> + .read_avail = opt4060_read_available,
>> + .read_event_value = opt4060_read_event,
>> + .write_event_value = opt4060_write_event,
>> + .read_event_config = opt4060_read_event_config,
>> + .write_event_config = opt4060_write_event_config,
>> +};
>
> Given you have option for no irq it is probably worth picking a version of this
> info structure with all the event callbacks removed. Technically it isn't
> required but it does harden the code (by crashing horribly if you call them ;)
>
Good point, I have added a separate version without those callbacks.
>
>
>> +static irqreturn_t opt4060_trigger_handler(int irq, void *p)
>> +{
>> + struct iio_poll_func *pf = p;
>> + struct iio_dev *idev = pf->indio_dev;
>> + struct opt4060_chip *chip = iio_priv(idev);
>> + struct {
>> + u32 chan[OPT4060_NUM_CHANS];
>> + aligned_s64 ts;
>> + } raw;
>> + int i = 0;
>> + int chan, ret;
>> +
>> + /* If the trigger is coming for a different driver, a new sample is needed.*/
>
> from a different driver?
I have tried to clarify this comment in v10. When an external trigger such as
iio_sysfs_trigger is used, the sensor will not be running in continuous mode
and a new sample must be triggered.
>
>> + if (iio_trigger_validate_own_device(idev->trig, idev))
>> + opt4060_trigger_new_samples(idev);
>> +
>> + memset(&raw, 0, sizeof(raw));
>> +
>> + iio_for_each_active_channel(idev, chan) {
>> + if (chan == OPT4060_ILLUM)
>> + ret = opt4060_calc_illuminance(chip, &raw.chan[i++]);
>> + else
>> + ret = opt4060_read_raw_value(chip,
>> + idev->channels[chan].address,
>> + &raw.chan[i++]);
>> + if (ret) {
>> + dev_err(chip->dev, "Reading channel data failed\n");
>> + goto err_read;
>> + }
>> + }
>> +
>> + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
>> +err_read:
>> + iio_trigger_notify_done(idev->trig);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +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;
>> +
>> + ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "Failed to read interrupt reasons.\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_MOD_EVENT_CODE(IIO_INTENSITY,
>> + chan,
>> + idev->channels[chan].channel2,
>> + 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_MOD_EVENT_CODE(IIO_INTENSITY,
>> + chan,
>> + idev->channels[chan].channel2,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_RISING);
>> + iio_push_event(idev, code, iio_get_time_ns(idev));
>> + }
>> + }
>> +
>> + /* Handle conversion ready */
>> + if (int_res & OPT4060_RES_CTRL_CONV_READY) {
>> + /* Signal completion for potentially waiting reads */
>> + complete(&chip->completion);
>
> That looks problematic as you haven't necessarily reset the completion
> if the buffer is enabled. So you probably need a flag or something similar
> to say a sysfs read has been requested.
The completion is only used when triggering a new sample. The code will call
reinit_completion() which will set the internal counter to zero. The code will
then call wait_for_completion_timeout() which will wait for the counter to
increase. The call to complete() here will increase the counter each time it's
called but since reinit_completion() is called every time before waiting, I don't
think this is an issue.
>
>
>> +
>> + /* Handle data ready triggers */
>> + if (iio_buffer_enabled(idev))
>> + iio_trigger_poll_nested(chip->trig);
>> + }
>> + return IRQ_HANDLED;
>> +}
>
>> +static int opt4060_setup_trigger(struct opt4060_chip *chip, struct iio_dev *idev)
>> +{
>> + struct iio_trigger *data_trigger;
>> + char *name;
>> + int ret;
>> +
>> + 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. */
>
> Make that a multiline comment.
Done in v10.
>
>> + chip->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");
>> +
>> + 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 |
> That's unusual for a trigger type interrupt and seems likely to give a lot
> of spurious interrupts. Even if the pulse is short, some interrupt controllers
> will hang on to the bonus edge and trigger again when you reenable the interrupt.
>
> If intent is to use this for events, then I think you can configure it to latched
> window mode and one edge type and it will all work.
Removed IRQF_TRIGGER_RISING in v10, don't know how both ended up there...
>
>> + IRQF_ONESHOT, name, idev);
>> + if (ret)
>> + return dev_err_probe(chip->dev, ret, "Could not request IRQ\n");
>> +
>> + init_completion(&chip->completion);
>> +
>> + mutex_init(&chip->irq_setting_lock);
>
> Trivial and I might not even bother changing it, but slightly preference for
> ret = devm_mutex_init(...)
> if (ret)
> return ret;
>
>> +
>> + 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;
>> +}
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-18 10:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-11 14:04 [PATCH v9 0/2] Support for Texas Instruments OPT4060 RGBW Color sensor Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 1/2] dt-bindings: iio: light: Document TI OPT4060 RGBW sensor Per-Daniel Olsson
2024-12-11 14:04 ` [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color sensor Per-Daniel Olsson
2024-12-14 18:21 ` Jonathan Cameron
2024-12-18 10:46 ` Per-Daniel Olsson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).